Allow push notifications in user's notifications settings by quinHD · Pull Request #8749 · decidim/decidim

quinHD · 2022-01-25T02:52:34Z

🎩 What? Why?

This PR adds option to the user to allow push notifications in the profile panel.

📌 Related Issues

Testing

You can test this feature visiting /notifications_settings. When you enable Push notifications you have to allow also notifications in the browser popup.

Acceptance criteria

Given that I'm a registered participant and my browser allows push notifications
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
Given that I'm a registered participant and my browser don't allows push notifications
When I go to "My account" -> "Notifications settings"
Then I don't see the "Push notifications" section.
Given that I'm a registered participant, my browser allows push notifications and I've revoked this permission in my browser
When I go to "My account" -> "Notifications settings"
Then I see a section called "Push notifications" with the following message:

 To get notifications from Decidim, you\u2019ll need to allow them in your browser settings first.

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

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

📷 Screenshots

Please add screenshots of the changes you're proposing

♥️ Thank you!

andreslucena · 2022-02-07T08:41:21Z

Can you fill the PR template @quinHD 🙏🏽?

ferblape · 2022-02-15T08:32:44Z

@quinHD could you review the broken spec? The rest is fine, so ping product when ready

ferblape · 2022-02-16T05:26:11Z

Ready to review @andreslucena

andreslucena

Some small changes to apply 🙏🏽

decidim-core/config/locales/en.yml

decidim-core/app/forms/decidim/notifications_settings_form.rb

decidim-core/spec/forms/notifications_settings_form_spec.rb

andreslucena · 2022-02-16T12:53:07Z

I was trying to force the notification browser consent dialog but couldn't do it locally, so I'd probably need to check it in the staging server. Let me know when you have it please.

andreslucena

I've just tested one last time and the flow is exactly what I had in mind, congratulations!!

I have a couple doubts and a minor CSS reflow that we need to change.

decidim-core/app/views/decidim/notifications_settings/show.html.erb

andreslucena · 2022-03-29T09:56:22Z

I see that there's a missing Acceptance Criteria:

Given that I'm a registered participant and my browser don't allows push notifications
When I go to "My account" -> "Notifications settings"
Then I don't see the "Push notifications" section.

This is with Opera Mini:

andreslucena · 2022-04-21T08:33:26Z

@quinHD I see that you have two flaky tests:

It was about NPM and a lost connection server side, I run it again and now it works
There was a problem related with seeds and passwords, that comes after Enforce password validation rules on 'Forgot your password?' form #9090. It should be fixed when Fix flaky factory when the user's sequence name arrives to 1234 #9167 it's merged, but meanwhile I retried and it works again.

andreslucena · 2022-04-21T08:41:13Z

Is this ready to be reviewed @quinHD? Have you deploy it to https://decidim.populate.tools? Is the bug from my last comment fixed?

quinHD · 2022-04-21T09:36:43Z

Yes to all. The only thing to keep in mind is that parte of the logic of the allow push notifications is modified in the other PR

andreslucena · 2022-04-21T09:52:29Z

@quinHD I still see the same behavior with Opera Mini: it shows the checkbox, and it doesn't ask for my permissions at the browser because it doesn't support it.

quinHD · 2022-04-21T10:11:52Z

@quinHD I still see the same behavior with Opera Mini: it shows the checkbox, and it doesn't ask for my permissions at the browser because it doesn't support it.

Hmm, I think the problem is that the browser also doesn't support new JS features, so it breaks the flow. This way, when it's going to check if it has to render the check button or not, it can't because there's an error.
Let me ask if there's any work around.

andreslucena · 2022-04-22T10:53:56Z

Just talked about the Opera Mini thing that I was mentioning in my last couple of comments with @quinHD outside of GitHub, and it seems like this is only a thing with Opera Mini:

Safari in an iPad Mini: it doesn't support it, and it isn't shown
IE11 in a Win8 VM: it doesn't support it, and it isn't shown
Opera in a Win11 VM: it supports it, it's shown, and it also shows the browser notification

So as discussed, in this particular case as it seems a problem with this particular browser, and it isn't a browser with much traction, we'll do a hard-coded User Agent reject list or a similar strategy.

quinHD · 2022-04-25T14:51:54Z

Just talked about the Opera Mini thing that I was mentioning in my last couple of comments with @quinHD outside of GitHub, and it seems like this is only a thing with Opera Mini:

Safari in an iPad Mini: it doesn't support it, and it isn't shown

IE11 in a Win8 VM: it doesn't support it, and it isn't shown

Opera in a Win11 VM: it supports it, it's shown, and it also shows the browser notification

So as discussed, in this particular case as it seems a problem with this particular browser, and it isn't a browser with much traction, we'll do a hard-coded User Agent reject list or a similar strategy.

Done! I added a User Agent rejection. Opera mini doesn't have its own User Agent. So I use the opera identifier and the mobile tag. I think it will affect also the Opera mobile browser.

andreslucena · 2022-04-26T08:21:45Z

decidim-core/db/migrate/20220119151540_add_allow_push_notifications_to_users.rb

+
+class AddAllowPushNotificationsToUsers < ActiveRecord::Migration[6.0]
+  def change
+    add_column :decidim_users, :allow_push_notifications, :boolean, default: false


After #8757 we have the notifications_settings JSONB column.

I see that's actually being used by the related PR #8774, so I think it makes sense to also migrate this column to notifications_settings, so it's all consistent. I know it can be a bit more of an effort but in the long term it's going to be there, so I think it's better to do it sooner than later. What do you think @quinHD?

since this column will disappear in the other PR. We have considered that this PR can be closed and its functionality will be taken over by the push-notifications-to-client PR.

quinHD self-assigned this Jan 25, 2022

andreslucena added project: PWA Barcelona City Council contract module: core type: feature labels Jan 26, 2022

quinHD marked this pull request as ready for review January 27, 2022 09:50

quinHD linked an issue Jan 28, 2022 that may be closed by this pull request

Ask permission to client for Push Notifications #8489

Closed

4 tasks

quinHD force-pushed the feature/allow-push-notifications branch from 3553450 to 041f069 Compare February 2, 2022 16:47

andreslucena requested a review from ferblape February 7, 2022 08:41

ferblape previously approved these changes Feb 15, 2022

View reviewed changes

quinHD dismissed ferblape’s stale review via 2f4f1e4 February 15, 2022 13:55

andreslucena mentioned this pull request Feb 16, 2022

Replace Decidim mentions in UI with 'the platform' #8827

Merged

andreslucena requested changes Feb 16, 2022

View reviewed changes

quinHD force-pushed the feature/allow-push-notifications branch from a7de219 to 42411b8 Compare February 24, 2022 16:50

quinHD changed the title ~~Allow push notifications~~ Allow push notifications in user's menu Feb 24, 2022

andreslucena requested changes Mar 22, 2022

View reviewed changes

andreslucena linked an issue Mar 29, 2022 that may be closed by this pull request

Notifications settings reflow on mobile #9016

Closed

quino and others added 9 commits April 20, 2022 14:56

Add push notifications request to user

e825f56

Fix 18n spec

b88218c

push permissions refactor js

45c34da

Fix lint and specs

b393135

Fix gemfile.lock

126af77

Restore Gemfile.lock

fae2b71

Use vapid keys through secrets

d0921e6

Fix overlaping button

4aee458

Hide sw mandatory elements when there's no sw

a687708

quinHD force-pushed the feature/allow-push-notifications branch from 10e6153 to a687708 Compare April 20, 2022 13:10

quino added 2 commits April 20, 2022 15:45

Fix remove element when no present

c4bb2bb

Fix if to avoid null issues

a51d00a

andreslucena added the release: v0.27 Issues or PRs that need to be tackled for v0.27 label Apr 21, 2022

Reverse the loding order for sw mandatory items

df13770

andreslucena changed the title ~~Allow push notifications in user's menu~~ Allow push notifications in user's notifications settings Apr 21, 2022

Fix await bug

668fe98

Add opera mini restriction for notifications

af6b9b5

andreslucena reviewed Apr 26, 2022

View reviewed changes

quinHD closed this Apr 28, 2022

Uh oh!

Conversation

quinHD commented Jan 25, 2022 • edited by andreslucena Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

Testing

Acceptance criteria

📋 Checklist

📷 Screenshots

Uh oh!

andreslucena commented Feb 7, 2022

Uh oh!

ferblape commented Feb 15, 2022

Uh oh!

ferblape commented Feb 16, 2022

Uh oh!

andreslucena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreslucena commented Feb 16, 2022

Uh oh!

andreslucena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreslucena commented Mar 29, 2022

Uh oh!

andreslucena commented Apr 21, 2022

Uh oh!

andreslucena commented Apr 21, 2022

Uh oh!

quinHD commented Apr 21, 2022

Uh oh!

andreslucena commented Apr 21, 2022

Uh oh!

quinHD commented Apr 21, 2022

Uh oh!

andreslucena commented Apr 22, 2022

Uh oh!

quinHD commented Apr 25, 2022

Uh oh!

andreslucena Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

quinHD Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quinHD commented Jan 25, 2022 •

edited by andreslucena

Loading