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

Show alert if map cannot load #12963

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

murjax
Copy link
Contributor

@murjax murjax commented Nov 1, 2024

What? Why?

Some browsers may fail to load Google Maps if 3rd party cookies are disabled. This causes downstream errors where the search bar attempts to render. More importantly, the user doesn't know why the map might have failed to load. This solution shows an alert instructing the user to check their network and 3rd party cookie settings.

What should we test?

Visit /map. Ensure the map still loads correctly.

The 3rd party cookie issue cannot be directly reproduced. Instead, the page has to be loaded while offline. This can only be done in local development where the page is locally hosted and Google Maps is loaded from the network.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Copy link
Collaborator

@chahmedejaz chahmedejaz left a comment

Choose a reason for hiding this comment

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

Great work @murjax. We are almost there, we just need to address a couple of robocop lint warnings. Thanks. 😄

Lint Warnings:
https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/11632701992/job/32396353444?pr=12963#step:5:74

@chahmedejaz chahmedejaz added the user facing changes Thes pull requests affect the user experience label Nov 2, 2024
@murjax
Copy link
Contributor Author

murjax commented Nov 2, 2024

@chahmedejaz Thanks! Lint warnings are fixed.

Copy link
Collaborator

@chahmedejaz chahmedejaz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks 👍

@@ -2784,6 +2784,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using
confirm_hub_change: "Are you sure? This will change your selected hub and remove any items in your shopping cart."
confirm_oc_change: "Are you sure? This will change your selected order cycle and remove any items in your shopping cart."
location_placeholder: "Type in a location..."
gmap_load_failure: "Unable to load map. Please check your network and allow 3rd party cookies."
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the error message is a bit confusing, maybe something like this would be better :
"Unable to load map. Please check your browser settings and allow 3rd party cookies for this website"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. I made the change.

context 'map cannot load' do
it 'shows alert' do
message = accept_alert { visit '/map' }
expect(message).to eq(I18n.t('gmap_load_failure'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a general rule we prefer to check with the actual message, so we can check the translation is working ,ie:

Suggested change
expect(message).to eq(I18n.t('gmap_load_failure'))
expect(message).to eq("Unable to load map. Please check your browser settings and allow 3rd party cookies for this website.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The string needs to be broken across multiple lines in this case to keep rubocop happy. I pushed a change that uses String#squish, but let me know if you have a preferred method for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally prefer to split the string with a \ but squish work as well. ie:

 expect(message).to eq(
  "Unable to load map. Please check your browser" \
  " settings and allow 3rd party cookies for this website."
)

Copy link
Contributor Author

@murjax murjax Nov 6, 2024

Choose a reason for hiding this comment

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

I like this better. I committed this change.

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Thanks @murjax Looks good now!
For future reference, once a PR has been reviewed it's better to add new commit instead updating existing commit. Otherwise we loose the review history and it can get confusing for other reviewer.

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Nov 22, 2024

Hey @murjax ,

Apologies, I'm not very sure how to test this PR. Indeed, I could not reproduce issue #8230, I've disabled third-party cookies in Firefox/Chrome but the map still loaded.

Also, locally, the map was loading even though third party cookies were disabled.

I've pulled your branch to my local environment, but still could not spot a difference in behavior.

I think I might be missing something obvious; what would be the best way to test your code changes?

Thanks for your help.

@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Nov 22, 2024
@murjax
Copy link
Contributor Author

murjax commented Nov 24, 2024

@filipefurtad0
The third-party cookie path is not reproducible.

However, the TypeError error mentioned in the description of #8230 was reproducible by going offline and reloading the map page locally. The assumption here is that third-party cookies being disabled prevents the map resources from loading on some browsers.

With this fix, reloading the map page while offline should present the alert indicating the map was unable to load and prompting the user to check their settings.

To clarify, this offline scenario won't occur in a staging or production environment because the page can't load to begin with while offline. It's possible in local development though because the app is hosted locally but pulls the map resources from the network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Test Ready 🧪
Development

Successfully merging this pull request may close these issues.

Inform users they need to unblock 3rd party browser cookies in Firefox to make /map work
4 participants