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

notifications: Disable default permission pop up. #12504

Closed
wants to merge 1 commit into from

Conversation

YashRE42
Copy link
Collaborator

@YashRE42 YashRE42 commented Jun 5, 2019

The issue here was that if the user has not allowed desktop
notifications, the browser would give its own pop up requesting for
permission, this would happen concurrently with zulip's custom green bar
that requests for permission, we would prefer that users interact with
our green bar and so this commit disables the pop up.

This fixes #11504 by removing the cause of the problem.

Testing Plan:
Open firefox, navigate to local env, log in as user 1.
Open chrome, navigate to local env, log in as user 2.
click "i" icon and reset notification permissions to default (ask), allow a reload, confirm that green bar is present. [on chrome]
send message to user 2 from user 1. [from firefox to chrome]

  • prior to this, you would see a browser pop up
  • after this, that pop up no longer triggers.

repeat in opposite order and confirm everything to be same.

The gifs bellow show manual testing:
chrome-notification-working
chrome-notification-working
chrome-notification-blocked
chrome-notification-blocked

firefox-notifcations-working
firefox-notifcations-working
firefox-notification-blocked
firefox-notification-blocked

some extra testing:
chrome-notification-ask-later
chrome-notification-ask-later
chrome-notification-never-ask-later
chrome-notification-never-ask-later

firefox-notifcations-ask-later
firefox-notifcations-ask-later
firefox-notification-never-ask-later
firefox-notification-never-ask-later

node tests:
I've just restored our previous test cases, but we could possibly increase coverage here by writing more tests, but maybe that would be a good follow up?

The issue here was that if the user has not allowed desktop
notifications, the browser would give its own pop up requesting for
permission, this would happen concurrently with Zulip's custom green bar
that requests for permission, we would prefer that users interact with
our green bar and so this commit disables the pop up.

This fixes zulip#11504 by removing the cause of the problem.
@YashRE42
Copy link
Collaborator Author

YashRE42 commented Jun 7, 2019

@timabbott This is good to go, tests are cleared.

@YashRE42 YashRE42 changed the title WIP: notifications: Disable default permission pop up. notifications: Disable default permission pop up. Jun 7, 2019
@timabbott
Copy link
Sponsor Member

This is great, merged as 9369672, thanks @YashRE42! I'd absolutely love to have better test coverage for this module; it's a very old module written long before we had the node test suite, so test coverage isn't great.

One note: I used an edited version of the text and GIT_AUTHOR_* data from @snickell's original PR description, because I thought the description was cleaner and in any case that seems like the most correct attribution.

Thanks for getting this finished @YashRE42!

@timabbott timabbott closed this Jun 12, 2019
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.

frontend: Green bar is even visible after clicking Allow in permission browser prompt.
3 participants