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

Fix wrong internet providers showing up #2492

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Conversation

kimadactyl
Copy link
Member

@kimadactyl kimadactyl commented May 27, 2024

  • Adds byebug gem back into test gems so you can console out of a test (as suggested by the official docs).
  • Removes faulty :for_site_with_tag method, just chain :for_site and :with_tags instead
  • Update tests

Note I am not 100% confident the tests are doing what they should be doing. They're hard to interpret and read and are deffo due a refactor - factories are adding in way too much context. I've tested this manually a bunch for now though.

Fixes #2365

@kimadactyl kimadactyl requested a review from katjam May 27, 2024 19:21
@kimadactyl kimadactyl self-assigned this May 27, 2024
Copy link
Member

@katjam katjam left a comment

Choose a reason for hiding this comment

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

Just double checking if we need to add that visibility check back in. Otherwise looks ok to try.

@@ -25,12 +25,12 @@ def robots
def set_places_to_get_computer_access
tag = Tag.find_by(slug: 'computers')

@places_to_get_computer_access = Partner.for_site_with_tag(current_site, tag)
@places_to_get_computer_access = Partner.for_site(current_site).with_tags(tag)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add the visible flag back in to this query?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thinking - looks like the for_site method does check visibility too tho. Which does raise the question if it should - but poss a tech debt question for antoher day?

Copy link
Member

@katjam katjam left a comment

Choose a reason for hiding this comment

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

One more teeny thought is that actually we do maybe want to show partners by just area in those sections - because internet & wifi access are kind of agnostic of interest group. But maybe also this is a safeguarding issue?

@kimadactyl
Copy link
Member Author

I think the logic here breaks down a bit for interest-based groups over wider areas, and I don't think we should have partners listed that aren't visible on the main partners page, that just sounds confusing.

Might be more interesting to think about making these blocks modular and togglable? Let's keep an eye out for if anything that comes up that people are looking to highlight?

@kimadactyl kimadactyl merged commit a440263 into main Jun 6, 2024
2 checks passed
@kimadactyl kimadactyl deleted the fix-internet-providers branch June 6, 2024 13:03
@honor-gfsc
Copy link

I think this should be verified by someone with more technical knowledge

@kimadactyl
Copy link
Member Author

kimadactyl commented Jun 25, 2024

validated working on:

  • hulme
  • moss side
  • gmsc (no listings)
  • norwich (no listings)
  • manchester
  • torbay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants