Skip to content
This repository was archived by the owner on Jan 4, 2023. It is now read-only.

Conversation

featheredtoast
Copy link
Member

Add an option to prompt a banner for a user's consent on the main screen (off by default).

  • Do not show the banner if a user has already subscribed, or has dismissed the banner.
  • Do not show the banner if a user is logged out.
  • Add a universal confirmation notification once a user subscribes, similar to slack/discord. This also can help confirm that the notifications are in good working order.

Most of the work here was inspired by the way slack handles notifications, by having a predominant prompt without nagging about it.

@ZogStriP
Copy link
Member

ZogStriP commented Nov 6, 2017

Screenshots?

@featheredtoast
Copy link
Member Author

Consent banner prompt:
push-notification-prompt-preview

Subscription confirmation notification:
notification-confirmation

Admin options:
admin-plugin-options

@xfalcox
Copy link
Member

xfalcox commented Nov 6, 2017

When I click on the banner there is a post to the rails backend:

Started POST "/push_notifications/subscribe" for 1.2.3.4 at 2017-11-06 21:16:01 +0000                                            
Processing by DiscoursePushNotifications::PushController#subscribe as */*                                                              
  Parameters: {"subscription"=>{"endpoint"=>"https://updates.push.services.mozilla.com/wpush/v2/longcode", "keys"=>{"auth"=>"longkey", "p256dh"=>"evenlongerkey"}}, "send_confirmation"=>"true"}
Completed 200 OK in 782ms (Views: 0.5ms | ActiveRecord: 8.6ms) 

But I never get the confirmation notification. Tested on latest stable Chrome and nightly Firefox on Android.

@featheredtoast
Copy link
Member Author

Oh dear, another case of the "it works on my machine"s -- I'll try and track this down, thanks for the feedback!

@featheredtoast
Copy link
Member Author

I suspect you might need to update your service workers, too -- It could be possible that there's an old version lying around.
I hit this a few times where a service worker would need to be forcefully updated to receive messages.

At least for the chrome devtools, application -> service workers tab will allow you to forcefully re-register.
update-service-worker

This specific issue with automatically registering new service workers should be dealt with, but I don't think that's necessarily in scope here.

@featheredtoast
Copy link
Member Author

I'm able to reproduce your issue by changing the subscription back to "ask" and refreshing the page, and toggling the notifications back and forth again, but I get a TypeError: no implicit conversion of String into Integer

Logging out and back in seems to resolve it for me - I'll document that in another ticket though. If that was what was preventing the notification, it's unrelated to this.

@@ -0,0 +1 @@
{{push-notification-consent}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an opportunity to re-use {{discourse-banner}} instead?

}

begin
response = Webpush.payload_send(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should extract the common code in Pusher into a method for this method to call.

@featheredtoast
Copy link
Member Author

Alright, I'll fix this up and apply the reviews on this one when I've got some time!

@featheredtoast featheredtoast force-pushed the consent-prompt-banner branch from a9819af to dfef2a6 Compare April 6, 2018 00:07
@featheredtoast
Copy link
Member Author

This PR is back in working order. A few notes:

After looking into it, {{discourse-banner}} seems specific to banner topics, and this is a per-user banner that will appear to notify users that push notifications are available, so it doesn't seem feasible to re-use the component.

I am planning to re-style the banner to support the same look+feel as the other notification before I merge this.

Add confirmation notification once a user subscribes

Do not display banner when permission is denied

Disable button when notification permission is denied

re-use CSS from alert classes

consider the banner dismissed if the user interacts with it at all: Stops some nagging if the user enabled then disabled notifications.
@featheredtoast featheredtoast force-pushed the consent-prompt-banner branch from cffbddb to c312507 Compare April 6, 2018 18:21
@featheredtoast featheredtoast merged commit 407570b into discourse:master Apr 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants