Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test graphql image urls #1469

Merged
merged 4 commits into from
Sep 28, 2022
Merged

Test graphql image urls #1469

merged 4 commits into from
Sep 28, 2022

Conversation

ivan-kocienski-gfsc
Copy link
Contributor

@ivan-kocienski-gfsc ivan-kocienski-gfsc commented Sep 27, 2022

Follows on from #1462

An expansion on the image URL feature.

Features

  • Generate full URLs for GraphQL queries involving partner logos and news item images
  • Uses Rails configuration to get correct protocol, domain, and port

Tests

  • Systems tests for connecting to server like it was a remote network endpoint
  • Check ping works
  • Check image URLs have correct protocol and domain on partner logos and news item images

In production environments the protocol should be HTTPS (which is set in
the environments folder).

Tests that check that the correct protocol is being used in the GQL API
Copy link
Contributor

@erbridge erbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could give PRs that fix things names and descriptions that reflect those fixes, please? I was expecting this PR to only contain some tests without any behaviour changes, and I'm not sure I follow why the behaviour has changed in the way it has to be able to review it. What was it doing before, why was that bad, and what is it doing now?

app/graphql/types/article_type.rb Outdated Show resolved Hide resolved
@ivan-kocienski-gfsc
Copy link
Contributor Author

@erbridge The situation is this: i am working on TD tickets that relied on functionality that was broken in PC. I implemented the functionality (for the specific issue of partner logos) in #1465 which itself involved setting up PC environments. This was done and the work pushed and merged.

Whilst it worked for the TD problem I was working on I couldn't figure out a way to test the functionality automatically (like as an external API user). After a weekend I thought of a way to test the GQL endpoint like an end-user would by (ab)using the system tests. This was not really the thing I was supposed to be working on (TD tickets) but I thought putting in tests was A Good Idea so I continued.

The next TD ticket I got was putting images on news items. So I applied the same fix from earlier (on PC) and had gotten the system tests up to be useful. It's not a good idea at all to try to sneak extra work in but as this was the same functionality as in 1465 and this PR was already covering tests I felt that it wasn't an egregious creep of scope.

@erbridge
Copy link
Contributor

@ivan-kocienski-gfsc Failing test.

Copy link
Contributor

@erbridge erbridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with the caveat that I'm not sure I understand why it wasn't working before, and that makes me nervous.

@ivan-kocienski-gfsc ivan-kocienski-gfsc merged commit 9837a4f into main Sep 28, 2022
@ivan-kocienski-gfsc ivan-kocienski-gfsc deleted the ik-fix-graphql-image-urls branch September 28, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants