Send push notifications to client#8774
Merged
Conversation
288c81d to
428df07
Compare
1 task
Member
|
Can you fill the PR template @quinHD 🙏🏽? |
Contributor
Author
sure! |
ferblape
suggested changes
Feb 14, 2022
ferblape
suggested changes
Feb 22, 2022
Contributor
ferblape
left a comment
There was a problem hiding this comment.
Just a couple of comments and a request:
in the Notifications page, the notifications link to the comment of the notification (with the anchor to the comment itself) while the push notification takes me to the container resource. Would it be possible to include the anchor too?
decidim-core/app/controllers/decidim/notifications_subscriptions_controller.rb
Outdated
Show resolved
Hide resolved
decidim-core/app/controllers/decidim/notifications_subscriptions_controller.rb
Outdated
Show resolved
Hide resolved
ahukkanen
previously approved these changes
May 16, 2022
Contributor
ahukkanen
left a comment
There was a problem hiding this comment.
LGTM!
The only thing that was still bothering me a bit is that the user subscribes to the notifications straight after they switch the "Push notifications" enabled.
This can be a bit confusing as I have not saved my changes with the button shown below the toggle element:
That said, I understand perfectly well why you do this. It's because the subscription needs to happen at the client side and not at the server side.
If we didn't do that, we would have to wait for the page to reload and do some front-end logic to subscribe/unsubscribe from the notifications which can get pretty complex.
So, I would say we can live with this issue. Just to keep in mind that we may hear back about this in future accessibility evaluations (as they typically complain about this kind of stuff).
Contributor
|
Before merging this, I'd still like to hear @andreslucena opinion about the comment I made above in my review. For me it's all good for now but just that you are also aware of this potential issue. We may need to come back to this later. |
andreslucena
requested changes
May 17, 2022
Member
andreslucena
left a comment
There was a problem hiding this comment.
Just a small typo to change and a general comment, great job!
decidim-core/app/services/decidim/notifications_subscriptions_persistor.rb
Outdated
Show resolved
Hide resolved
Member
I know that this behavior is a bit weird. Other platforms that implement this kind of configurations client-side (like Twitter), for what I've seen they don't have this problem as all the user settings forms are updated with AJAX. We'll need to take this flow into account for the redesign, for sure. As I said, I think is weird, but is the best that we've come up to until now. |
Member
ahukkanen
previously approved these changes
May 17, 2022
Contributor
|
@andreslucena done! |
andreslucena
previously approved these changes
May 17, 2022
ahukkanen
approved these changes
May 17, 2022
andreslucena
added a commit
that referenced
this pull request
May 19, 2022
* Add push notifications request to user * Fix 18n spec * push permissions refactor js * Fix lint and specs * Fix gemfile.lock * Restore Gemfile.lock * Move webpush from dev to core * Fix Gemfile.lock in engines * Use vapid keys through secrets * Add notification suscription * Add send push notification service * saving possible non-existent dom code * open tab or focus existing when notification clicked * simplify things * Move url to data in notification params * Add specs to send_push_notification * Fix lint * Fix lint * Fix erb lint * Add unsubscription to push notificaionts * Add better vapid keys presence validation * Make delete subscription more safe * Add CSRF token in js subscriptions requests * Add vapid validations in webpush service * Fix subscriptions spec * Add notifications enabled? check before subscription * Add allow push notification check * Fix spec * Fix spam a2hs button * Fix lint * Add notifications subscriptions to user settings * Fix spec * Fix enable notifications button functionality * Fix lint * Fix lint * Add push notifications request to user * Fix 18n spec * push permissions refactor js * Fix lint and specs * Fix gemfile.lock * Restore Gemfile.lock * Use vapid keys through secrets * Fix overlaping button * Hide sw mandatory elements when there's no sw * Fix remove element when no present * Add js revisions * Fix if to avoid null issues * Reverse the loding order for sw mandatory items * Fix await bug * Fix selectors for push notifications * Fix spec * Remove duplicate attribute * Remove allow notifications attribute * Fix lint * Remove html safe calls * Update generator task * Changelog * Only render vapid keys when enabled * Complete specs * Check returns notification * Make methods private * Comment clarification * Capture webpush exceptions * Refactor JS to run only when scope node is present * Revert "Refactor JS to run only when scope node is present" This reverts commit 9966259. * Remove extra div * Refactor JS * Test subscriptions * Fix lint issues * Sync gemfiles * Remove deprecation warning * Activate display for chrome driver headed * Run tests with xvfb * Remove unnecessary partial * Update xvfb parameters * Fix lint issues * Export display * Run xvfb in the same run * Don't launch chromedriver * Fix wrong merge * Comments * Use env var * Fix deprecation warning * Arrow function Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Double start operator after Ruby 3 upgrade Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Log Webpush exception * Fix opera mini user agents * Show setting by default * Typo Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Limit route to specific actions Co-authored-by: Hugoren Martinako <aumpfbahn@gmail.com> Co-authored-by: Fernando Blat <fernando@blat.es> Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
andreslucena
added a commit
that referenced
this pull request
May 19, 2022
* Add push notifications request to user * Fix 18n spec * push permissions refactor js * Fix lint and specs * Fix gemfile.lock * Restore Gemfile.lock * Move webpush from dev to core * Fix Gemfile.lock in engines * Use vapid keys through secrets * Add notification suscription * Add send push notification service * saving possible non-existent dom code * open tab or focus existing when notification clicked * simplify things * Move url to data in notification params * Add specs to send_push_notification * Fix lint * Fix lint * Fix erb lint * Add unsubscription to push notificaionts * Add better vapid keys presence validation * Make delete subscription more safe * Add CSRF token in js subscriptions requests * Add vapid validations in webpush service * Fix subscriptions spec * Add notifications enabled? check before subscription * Add allow push notification check * Fix spec * Fix spam a2hs button * Fix lint * Add notifications subscriptions to user settings * Fix spec * Fix enable notifications button functionality * Fix lint * Fix lint * Add push notifications request to user * Fix 18n spec * push permissions refactor js * Fix lint and specs * Fix gemfile.lock * Restore Gemfile.lock * Use vapid keys through secrets * Fix overlaping button * Hide sw mandatory elements when there's no sw * Fix remove element when no present * Add js revisions * Fix if to avoid null issues * Reverse the loding order for sw mandatory items * Fix await bug * Fix selectors for push notifications * Fix spec * Remove duplicate attribute * Remove allow notifications attribute * Fix lint * Remove html safe calls * Update generator task * Changelog * Only render vapid keys when enabled * Complete specs * Check returns notification * Make methods private * Comment clarification * Capture webpush exceptions * Refactor JS to run only when scope node is present * Revert "Refactor JS to run only when scope node is present" This reverts commit 9966259. * Remove extra div * Refactor JS * Test subscriptions * Fix lint issues * Sync gemfiles * Remove deprecation warning * Activate display for chrome driver headed * Run tests with xvfb * Remove unnecessary partial * Update xvfb parameters * Fix lint issues * Export display * Run xvfb in the same run * Don't launch chromedriver * Fix wrong merge * Comments * Use env var * Fix deprecation warning * Arrow function Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Double start operator after Ruby 3 upgrade Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Log Webpush exception * Fix opera mini user agents * Show setting by default * Typo Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Limit route to specific actions Co-authored-by: Hugoren Martinako <aumpfbahn@gmail.com> Co-authored-by: Fernando Blat <fernando@blat.es> Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
eliegaboriau
pushed a commit
to eliegaboriau/decidim
that referenced
this pull request
Oct 25, 2022
* Add push notifications request to user * Fix 18n spec * push permissions refactor js * Fix lint and specs * Fix gemfile.lock * Restore Gemfile.lock * Move webpush from dev to core * Fix Gemfile.lock in engines * Use vapid keys through secrets * Add notification suscription * Add send push notification service * saving possible non-existent dom code * open tab or focus existing when notification clicked * simplify things * Move url to data in notification params * Add specs to send_push_notification * Fix lint * Fix lint * Fix erb lint * Add unsubscription to push notificaionts * Add better vapid keys presence validation * Make delete subscription more safe * Add CSRF token in js subscriptions requests * Add vapid validations in webpush service * Fix subscriptions spec * Add notifications enabled? check before subscription * Add allow push notification check * Fix spec * Fix spam a2hs button * Fix lint * Add notifications subscriptions to user settings * Fix spec * Fix enable notifications button functionality * Fix lint * Fix lint * Add push notifications request to user * Fix 18n spec * push permissions refactor js * Fix lint and specs * Fix gemfile.lock * Restore Gemfile.lock * Use vapid keys through secrets * Fix overlaping button * Hide sw mandatory elements when there's no sw * Fix remove element when no present * Add js revisions * Fix if to avoid null issues * Reverse the loding order for sw mandatory items * Fix await bug * Fix selectors for push notifications * Fix spec * Remove duplicate attribute * Remove allow notifications attribute * Fix lint * Remove html safe calls * Update generator task * Changelog * Only render vapid keys when enabled * Complete specs * Check returns notification * Make methods private * Comment clarification * Capture webpush exceptions * Refactor JS to run only when scope node is present * Revert "Refactor JS to run only when scope node is present" This reverts commit 9966259. * Remove extra div * Refactor JS * Test subscriptions * Fix lint issues * Sync gemfiles * Remove deprecation warning * Activate display for chrome driver headed * Run tests with xvfb * Remove unnecessary partial * Update xvfb parameters * Fix lint issues * Export display * Run xvfb in the same run * Don't launch chromedriver * Fix wrong merge * Comments * Use env var * Fix deprecation warning * Arrow function Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Double start operator after Ruby 3 upgrade Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Log Webpush exception * Fix opera mini user agents * Show setting by default * Typo Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Limit route to specific actions Co-authored-by: Hugoren Martinako <aumpfbahn@gmail.com> Co-authored-by: Fernando Blat <fernando@blat.es> Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Contributor
|
@ferblape Sorry to bother but we have a question regarding the port range you introduced for Capybara server in this commit: Is there some particular reason you decided to use the port range of 5000-6999? We have a recurring problem right now in the CI due to this port range and we already figured out a solution to that (#10435). But just out of interest, if you have any insight why you decided to use this particular range, it might be valuable information for the future. |
Contributor
|
No special reason I can remember, I just wanted a range of ports above the 1024. So I guess the range can be modified without any consequences in the correct setup of Capybara. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎩 What? Why?
This PR solves two issues.
first of all, it allow users to configure their push notification settings.
allow users to receive push notifications when a new notification is created in the app. It implements the necessary logic to perform this task.
📌 Related Issues
Issue #8490
Issue #8489
Testing
To allow push notifications:
You can test this feature visiting
/notifications_settings. When you enablePush notificationsyou have to allow also notifications in the browser popup.To sendd push notification:
You can trigger a notification login in the app with a user and from other browser (or incognito) make a comment quoting the first user. This way a
new commentpush notification will prompt.Acceptance criteria
When I go to "My account" -> "Notifications settings"
Then I see a section called "Push notifications"
and when I check this setting
Then I get asked for permission to receive Push notifications
When I go to "My account" -> "Notifications settings"
Then I don't see the "Push notifications" section.
When I go to "My account" -> "Notifications settings"
Then I see a section called "Push notifications" with the following message:
Given that I'm a registered participant, my browser allows push notifications and I've accepted the Push notifications
When I go to "My account" -> "Notifications settings"
Then I see a section called "Push notifications"
and when I uncheck this setting
Then I've revoked permission to receive Push notifications
Given that I'm a participant that has given permissions to receive push notifications,
When I get a new notification in /notifications
Then I also receive it in my device
and this notification has a title and body
and has a link to the resource
and has the icon of the organization.
📋 Checklist
🚨 Please review the guidelines for contributing to this repository.
docs/.📷 Screenshots
Please add screenshots of the changes you're proposing
