Mail notifications digest by quinHD · Pull Request #8833 · decidim/decidim

quinHD · 2022-02-16T23:23:02Z

🎩 What? Why?

This PR adds the mail digest feature for user notifications. It makes available a rake task to send the email only to the selected users. It can be invoked from a cron job.

📌 Related Issues

#8486

Testing

Describe the best way to test or validate your PR.

#####Acceptance criteria#####

Given that I'm an implementer,
When I go to https://docs.decidim.org/en/install/#_scheduled_tasks I have instructions on how to set this feature for new apps. (docs/modules/install/pages/index.adoc)
Given that I'm an implementer,
When I go to the CHANGELOG I have instructions on how to set this feature for existing apps.
Given that I'm a participant,
When I go to "My account" -> "Notifications settings"
Then I see a new section "How often do you want to receive the notifications emails?" with the options "None", "Real time", "Daily" and "Weekly"
Given that I'm a participant that had selected "I want to receive an email every time I receive a notification",
When I go to "My account" -> "Notifications settings"
Then I have selected the "Real time" option in "How often do you want to receive the notifications emails?"
Given that I'm a new registered participant,
When I go to "My account" -> "Notifications settings"
Then I have selected the "Daily" option by default in "How often do you want to receive the notifications emails?"
Given that I'm a participant,
When I have selected the "None" option in "How often do you want to receive the notifications emails?"
Then I don't receive any notifications emails.
Given that I'm a participant,
When I have selected the "Real time" option in "How often do you want to receive the notifications emails?"
Then I receive emails every time I have a notification.
Given that I'm a participant,
When I have selected the "Daily" option in "How often do you want to receive the notifications emails?"
Then I receive one email with my notifications from yesterday.
Given that I'm a participant,
When I have selected the "Weekly" option in "How often do you want to receive the notifications emails?"
Then I receive one email with my notifications from the last week.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

📷 Screenshots

Please add screenshots of the changes you're proposing

♥️ Thank you!

quinHD · 2022-02-17T17:58:33Z

@andreslucena. Where should be the documentation relative to this feature? in a page in this repo, or in docs repo?

andreslucena · 2022-02-18T08:59:12Z

@andreslucena. Where should be the documentation relative to this feature? in a page in this repo, or in docs repo?

It depends in which kind of docs did you have in mind. If it's for developers/implementers, then it should be here (docs/ directory). If it's for admins/users it should be in https://github.com/decidim/documentation

andreslucena · 2022-02-18T08:59:18Z

Uops sorry for closing this, wrong click 😅

alecslupu · 2022-03-23T06:36:45Z

@quinHD Can you normalize the locales?

ferblape

Just a few details to review, plus I miss a test about the use of the digest_sent_at attribute, for example checking that this field value blocks emails already been sent.

docs/modules/customize/pages/mailers.adoc

decidim-core/lib/tasks/decidim_mailers_tasks.rake

decidim-core/db/migrate/20220203121137_add_notifications_sending_frequency_to_users.rb

decidim-core/app/mailers/decidim/notifications_digest_mailer.rb

decidim-core/app/jobs/decidim/email_notifications_digest_generator_job.rb

ferblape · 2022-03-23T14:45:50Z

Also, take care with the I18n, it needs to be normalized

ferblape · 2022-03-24T14:29:07Z

ready to review @decidim/product

decidim-core/app/models/decidim/notification.rb

decidim-core/app/presenters/decidim/notifications_digest_presenter.rb

andreslucena · 2022-04-07T10:45:14Z

I have reviewed it locally, and there are some things that we need to change regarding the flow and feature.

Please merge with develop and also configure it in decidim.populate.tools/letter_opener so we can check the UI with @carolromero

I found confusing having to check the "Send notifications by email" and also the "How often do you want to receive the notifications digest email?" at the same time to receive notifications. The whole point of this feature is that I already receive too many emails from notifications. I think this could be easily fixed by:

changing the question from "How often do you want to receive the notifications digest email?" to "How often do you want to receive the notifications emails?"
adding a new radio for "Real time" (as you've in the mockup)
reordering the options so it makes sense (now its "Daily", "None" and "Weekly"), it should be "None", "Real time", "Daily" and "Weekly"
dropping the "Send notifications by email" section in this page (and column in the DB), migrating the users that have selected that to "Real time"

Regarding where to add the docs for implementers, I think it's easier to discover on the section for scheduled tasks in the install guide (docs/modules/install/pages/index.adoc) and also a few lines in the CHANGELOG so it's easier to find by existing implementers.

So the ACs have changed reflecting better this so it's easier to keep track of it.

andreslucena · 2022-04-07T13:41:58Z

This is how it'd look like the final result of the notifications settings:

ahukkanen · 2022-05-04T14:04:22Z

@quinHD @ferblape Could you kindly rebase (or merge with develop)? There are some conflicts. 🙏

andreslucena

There are some merge conflicts, and I have a couple suggestions. Can you give it a look please? Thanks!

docs/modules/install/pages/index.adoc

CHANGELOG.md

decidim-core/db/migrate/20220203121137_add_notifications_sending_frequency_to_users.rb

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

ferblape · 2022-05-05T04:22:12Z

@andreslucena feedback added and conflicts fixed

docs/modules/install/pages/index.adoc

ahukkanen

I really like how elegantly this is implemented, great work! It was a pleasure to read through.

I just noticed one unnecessary addition as far as I understood, so could you take a look at that?

Also, can you solve the merge conflicts and fix the Rubocop issue after merging with develop please? 🙏

Also, I noticed a potential bug but I'll post it as a separate message below.

config/i18n-tasks.yml

decidim-core/spec/commands/decidim/update_notifications_settings_spec.rb

ahukkanen · 2022-05-13T14:17:00Z

I'm not sure if I have something misconfigured but when I tried the "real time" notifications locally, it was generating duplicate records for me as shown in the video below:

decidim-dupicate-notification.mp4

Because of this, it was also sending duplicate emails to me.

I tried this locally and I was running the jobs through the delayed_job adapter using the rake jobs:work process.

I tried the same also on develop and it was working correctly on develop, i.e. generating only a single notification.

Could you take a look at this?

Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi>

ferblape · 2022-05-16T08:58:18Z

Thanks for your time and your review @ahukkanen

I haven't been able to reproduce the double notification issue. I've tried both without async_adapter, and with a job adapter configured, and in both cases only one notification was generated

Also, I reviewed the logs and everything look ok: just a single job enqueued, job which internally generates a couple more of jobs, one for the notification and the other for the email.

Could you pase your development log when you have time to debug what could fail?

ahukkanen · 2022-05-16T15:41:15Z

I haven't been able to reproduce the double notification issue. I've tried both without async_adapter, and with a job adapter configured, and in both cases only one notification was generated

Also, I reviewed the logs and everything look ok: just a single job enqueued, job which internally generates a couple more of jobs, one for the notification and the other for the email.

Could you pase your development log when you have time to debug what could fail?

Thanks @ferblape !

It must have been some misconfiguration on my end because today I just regenerated the development_app from scratch and now only one job is enqueued and I cannot replicate the bug.

So functionality-wise it all good now! I'll take another look at the changes and it should be good to go on my part.

* Add notifications digest email * Fix specs & lints * Fix erb lint * Remove unused frequency radio button * Refactor radio buttons in notifications settings * Fix i18n spec * Add notifications frequency to form * Add notifications digest documentation * Add none to notification frequency * Remove relations in mailer arguments * Refactor mail notifications digest sending method * Fix error in migration * Fix lint * Add spec for notifications digest sending decider * Rearrange notification settings * Remove email_on_notification attribute * Fix locales order * Fix lint * Fix i18n specs * Fix meetings specs * Update docs with mailer * Add to changelog * Refactor migration * Clarification on the documentation * Convert to local variable * Remove comment * Add field to serializer * Test on form * Simplify migrations * Fix default value for new users * Add more details to changelog * Typo * Fix spec * Update CHANGELOG.md Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Update docs/modules/install/pages/index.adoc Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Ruby linter * Remove docs from installation page * Use symbols Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> * Fix offenses * Fix i18n * Fix offenses Co-authored-by: Fernando Blat <fernando@blat.es> Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi>

quinHD self-assigned this Feb 16, 2022

quinHD force-pushed the feature/mail-notifications-digest branch from e931bcb to f5c047b Compare February 16, 2022 23:28

quinHD linked an issue Feb 16, 2022 that may be closed by this pull request

Notifications digest #8486

Closed

9 tasks

quinHD marked this pull request as ready for review February 17, 2022 01:35

quinHD changed the title ~~Add mail notifications digest~~ Mail notifications digest Feb 17, 2022

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

andreslucena closed this Feb 18, 2022

andreslucena reopened this Feb 18, 2022

andreslucena mentioned this pull request Feb 21, 2022

Add reminders for publishing reports to meeting authors #8757

Merged

12 tasks

ferblape suggested changes Mar 23, 2022

View reviewed changes

ferblape previously approved these changes Mar 24, 2022

View reviewed changes

andreslucena reviewed Apr 7, 2022

View reviewed changes

decidim-core/app/models/decidim/notification.rb Outdated Show resolved Hide resolved

andreslucena reviewed Apr 7, 2022

View reviewed changes

decidim-core/app/presenters/decidim/notifications_digest_presenter.rb Outdated Show resolved Hide resolved

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

quinHD dismissed ferblape’s stale review via 8bf2462 April 27, 2022 17:54

quino added 5 commits April 27, 2022 20:00

Add notifications digest email

c4e0aaf

Fix specs & lints

0b9c7d6

Fix erb lint

eb61ea2

Remove unused frequency radio button

58a6d1b

Refactor radio buttons in notifications settings

260b17a

andreslucena requested changes May 4, 2022

View reviewed changes

docs/modules/install/pages/index.adoc Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

decidim-core/db/migrate/20220203121137_add_notifications_sending_frequency_to_users.rb Show resolved Hide resolved

ferblape dismissed their stale review via de005d9 May 5, 2022 03:26

ferblape and others added 4 commits May 5, 2022 05:26

Update CHANGELOG.md

de005d9

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

Update docs/modules/install/pages/index.adoc

8c397e0

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

Merge branch 'develop' into feature/mail-notifications-digest

ecd8806

Ruby linter

3301766

andreslucena reviewed May 11, 2022

View reviewed changes

docs/modules/install/pages/index.adoc Outdated Show resolved Hide resolved

Remove docs from installation page

e0b4005

ahukkanen requested changes May 13, 2022

View reviewed changes

config/i18n-tasks.yml Outdated Show resolved Hide resolved

decidim-core/spec/commands/decidim/update_notifications_settings_spec.rb Outdated Show resolved Hide resolved

ferblape and others added 5 commits May 16, 2022 07:23

Use symbols

7ed9886

Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi>

Merge branch 'develop' into feature/mail-notifications-digest

fa87fe2

Fix offenses

bd93f4f

Fix i18n

b787d69

Fix offenses

91c79ab

ahukkanen approved these changes May 16, 2022

View reviewed changes

ahukkanen requested a review from andreslucena May 16, 2022 15:42

andreslucena approved these changes May 17, 2022

View reviewed changes

andreslucena merged commit b6a7d56 into develop May 17, 2022

andreslucena deleted the feature/mail-notifications-digest branch May 17, 2022 08:19

andreslucena mentioned this pull request May 17, 2022

Send push notifications to client #8774

Merged

17 tasks

alecslupu added this to the 0.27.0 milestone Jul 14, 2023

fblupi mentioned this pull request Nov 16, 2023

Update email stylesheets and add it to the overrides spec AjuntamentdeBarcelona/decidim-barcelona#512

Merged

Uh oh!

Conversation

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

Uh oh!

🎩 What? Why?

📌 Related Issues

Testing

📋 Checklist

📷 Screenshots

Uh oh!

quinHD commented Feb 17, 2022

Uh oh!

andreslucena commented Feb 18, 2022

Uh oh!

andreslucena commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecslupu commented Mar 23, 2022

Uh oh!

ferblape left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ferblape commented Mar 23, 2022

Uh oh!

ferblape commented Mar 24, 2022

Uh oh!

Uh oh!

Uh oh!

andreslucena commented Apr 7, 2022

Uh oh!

andreslucena commented Apr 7, 2022

Uh oh!

ahukkanen commented May 4, 2022

Uh oh!

andreslucena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ferblape commented May 5, 2022

Uh oh!

Uh oh!

ahukkanen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahukkanen commented May 13, 2022

Uh oh!

ferblape commented May 16, 2022

Uh oh!

ahukkanen commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

quinHD commented Feb 16, 2022 •

edited by andreslucena

Loading

andreslucena commented Feb 18, 2022 •

edited

Loading