-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 privacy settings banner for users in GDPR jurisdictions #18696
Conversation
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
…ue/18567-show-popover-for-eu-users
449aea8
to
de3c6fd
Compare
Ouch, I "closed" by mistake, glad I could reopen the PR |
made ready for review as I will be focused on patching things if required, otherwise merging this PR |
Thanks for your help on this Ovi! |
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected, I tested the happy flow and would merge this after testing and validating the unhappy flow 😅 , ie. the flow when it shouldn't be shown.
IMHO the landscape view isn't very much the best UX we can do: But I won't block the merge just because of this. I will try to fix it on trunk, with another PR 👍🏻 All in all this is awesome work and many goodies added to the codebase, as well as solving the actual problem, and improving our compliance to the GDPR laws. Thanks a million @mkevins for your effort on this work! 🙇🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own patches and consider them appropriate.
Thanks for all your help Ovi, and special thanks for wrangling the landscape issue and remaining failing test! |
Fixes #18567
To test:
Launch (either) app
Login with a user from an EU country subject to GDPR (e.g. France)
Expect to see the popup asking your consent
Verify the events mentioned in this task are tracked
Retry with an user from a non-GDPR country (e.g. US)
Expect to not see the popup
Restart the app after approving or denying consent
Verify the popup is no longer shown
Regression Notes
Potential unintended areas of impact
First load of My Site dashboard.
What I did to test those areas of impact (or what existing automated tests I relied on)
n/a
What automated tests I added (or what prevented me from doing so)
The new logic is covered by unit tests.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: