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 currency switcher notice until customer explicitly dismisses it #2967

Merged

Conversation

oaratovskyi
Copy link
Contributor

@oaratovskyi oaratovskyi commented Sep 22, 2021

Fixes #2941

Changes proposed in this Pull Request

Currently, currency switcher notice is being shown to customers only once. Then geolocated currency becomes selected currency and notice isn't shown anymore due to existing logic. That might be confusing for customers if they moved out from the page too quickly or in case the customer didn't see it at first.

I fixed currency switcher notice logic that it shows every time until explicitly dismissed with Dismiss button or by clicking Use ... instead, where ... is the default currency.

Testing instructions

  1. Open the shop on the incognito page.
  2. Observe the notice that currency was switched to a geolocated one.
  3. Reload the page with Ctrl+R / press Enter in the address bar / Close the page and open new.
  4. The notice should still show.
  5. Click Dismiss / Use ... instead and repeat instruction 3, now the notice shouldn't show up.
  6. Repeat instructions 1-6 in non-incognito mode. If the notice does not show up, clean the cookie.
  7. Check the simulation flow under WooCommerce -> Settings -> Multi-Currency. That's Preview button near the Customers will be notified via store alert banner.

  • Added changelog entry (or does not apply)
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

- Changed display_geolocation_currency_update_notice() logic to show the notice only for geolocated currency (if it's not simulation mode)
- fixed tests
@oaratovskyi oaratovskyi marked this pull request as ready for review October 5, 2021 14:16
@oaratovskyi oaratovskyi changed the title Currency switcher customer notice doesn't appear Show currency switcher notice until customer explicitly dismisses it Oct 5, 2021
@oaratovskyi oaratovskyi requested a review from a team October 6, 2021 07:28
Copy link
Contributor

@dmallory42 dmallory42 left a comment

Choose a reason for hiding this comment

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

LGTM and works well, let's 🚢 it!

@@ -452,6 +452,20 @@ function() {
return 'US';
}
);
// todo delete the comment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this can be deleted 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I forgot about that 🙈 Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@oaratovskyi
Copy link
Contributor Author

image
Merging since e2e test passed from attempt #3, I didn't make changes

And everything else passed in attempt #5.
image

@oaratovskyi oaratovskyi merged commit 34e849c into develop Oct 7, 2021
@oaratovskyi oaratovskyi deleted the fix/2941-currency-switcher-customer-notice-does-not-appear branch October 7, 2021 20:27
mattallan pushed a commit that referenced this pull request Oct 18, 2021
…2967)

* Add the fix to changelog.

* Changes to currency switch notice display
- Changed display_geolocation_currency_update_notice() logic to show the notice only for geolocated currency (if it's not simulation mode)
- fixed tests

* Fix changelog entry

* Commit for CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Currency switcher customer notice doesn't appear
2 participants