Send multiple notifications by email by armandfardeau · Pull Request #5875 · decidim/decidim

armandfardeau · 2020-03-19T16:28:03Z

🎩 What? Why?

Batch email notifications

Batch email notifications are a way to send several email notifications at the same time.

This is useful for grouping notifications and not overloading your users emails.

Notifications are sent in groups, sorted from the most recent notifications.

Notifications that have not been sent after a certain time are not sent at all. This means that your users have already received plenty of them and will already have to visit the platform.

Deleting a notification cancels the future associated email.

This is based on 3 parameters:

batch_email_notifications_enabled: Enable batch email notifications and disable single email notification (default to false)
batch_email_notifications_expired: period of time after which a notification is considered to have expired (default to 1 week)
batch_email_notifications_max_length: Number of notifications to send in the same email (default to 5)

How to setup?

App setup

In config/initialize/decidim.rb

Set to desired value

config.batch_email_notifications_enabled = true
config.batch_email_notifications_expired = 1.week
config.batch_email_notifications_max_length = 5

Server setup

Batch email notification required to execute a rake task ("rake decidim:batch_email_notifications:send") at regular intervals.

Cron setup

Use crontab -e and enter the following lines:
```
0 */3 * * * cd {MY_APP} && {/path/to/bundle} exec rake decidim:batch_email_notifications:send
```
You could find the documentation about cron on cron man page
Heroku scheduler

You can find information on how to setup heroku scheduler on heroku documentation

Sidekiq scheduler

In config/sidekiq.yml

# sidekiq_scheduler.yml

batch_email_notifications:
  every: ['3h', first_in: '1m']
  class: Decidim::BatchEmailNotificationsGeneratorJob
  queue: scheduled
  description: 'This job executes batch email notifications'

Many thanks to @Quentinchampenois for his help and co-authorship

📋 Subtasks

📷 Screenshots (optional)

Demo

https://dcd-batch-notifications.osp.dev/

Sidekiq available connected as a system user at https://dcd-batch-notifications.osp.dev/sidekiq/
Letter opener available https://dcd-batch-notifications.osp.dev/letter_opener/

To test:

generate multiple notifications and mail, create proposals, debates, comments, etc...
Check mail in letter opener, you should have only "sent_now" priority ones.
Execute task "batch notification" in https://dcd-batch-notifications.osp.dev/sidekiq/recurring-jobs
Check mail in letter opener and check batch notification emails

tramuntanal · 2020-04-28T07:37:50Z

I've labeled the task as WIP, please, feel free to remove the label when the PR is ready :D

stale · 2020-07-18T06:27:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. @carolromero & @andreslucena feel free to chime in.

andreslucena · 2020-07-20T06:43:04Z

Hi @armandfardeau
Are you planning on doing this? For part of @decidim/product this is a very welcome and needed feature :D
For traceability, discussions on meta:

cc @virgile-dev

armandfardeau · 2020-07-30T10:15:51Z

Hi @andreslucena we are definitely planning to work on this, we will do a peer with @Quentinchampenois.

Please note that in the future version of this feature, multiple notifications per email will be the default setting and single-mode will be an option.

andreslucena · 2020-08-03T08:08:10Z

Do you have an estimated time on when you'll find time for working on this again? Long-lived PRs are a maintenance nightmare (too many conflicts).

Quentinchampenois · 2020-08-10T16:22:48Z

Do you have an estimated time on when you'll find time for working on this again? Long-lived PRs are a maintenance nightmare (too many conflicts).

Hi @andreslucena,

Sorry for the answer delay, I am planning on working on this PR this week

Quentinchampenois · 2020-08-20T12:23:34Z

Hi,

I worked on this PR this week. I have added a priority feature to decide if a notification must be sent directly or can be delayed and send with other notifications.

Now, the question is, what is the priority to set for each notification ? By default, a notification with undefined priority will have a low priority.

What's left to finish this PR is to come up with a list for high priority notification (ex: forgot password, you don't want that to go in a batch). I'll be making a list of these in this comment, don't hesitate to comment back if you feel we missed some important ones.

High priority notifications ideas:

When administrator exports
A proposal has been moderated
A proposal has been reported
User forgot his / her password

paulinebessoles · 2020-08-24T15:52:26Z

Good work @Quentinchampenois!

@carolromero and @andreslucena, what do you think about this list of high priority notifications (not going in a batch):

When administrator exports (decidim.export_mailer)
Welcome notification (decidim-core/app/events/decidim/welcome_notification_event.rb)
User forgot his / her password (devise.mailer.reset_password_instructions; devise.mailer.password_change)
User asked for an export of his data (decidim.export_mailer.data_portability_export)
User updated his profile (decidim-core/app/events/decidim/profile_updated_event.rb)
A content (proposal, meeting, comment, etc.) has been moderated
A content has been reported

Moreover, for Initiatives, I think there is a difference between mailer and events. How do you think we should deal with it?

tramuntanal · 2020-08-27T11:39:19Z

I would say that all admin and platform notifications should have high priority, on the other side notifications coming from participant interactions could have low priority.
But let's see what @decidim/product has to say on this

tramuntanal · 2020-08-27T11:42:24Z

Also, note that not all interactions that you mention would generate a notification. For example a "user forgot password" event simply generates an email, not a notification. I would leave this non notification emails out of scope.

carolromero · 2020-08-31T08:45:14Z

Hi everyone,
We agree with @tramuntanal regarding the notifications for the admins, which should be of high priority.
We now differentiate the notifications of the own activity from the activity we follow, and most notifications are due to the latter:

So we think that the digest should contain the notifications related to the activity I follow. As a participant I probably want to receive a direct notification if someone mentions me or interacts with some content that I have created.

armandfardeau · 2020-09-02T08:14:07Z

I would leave this non notification emails out of scope.

Yes that's the way we are doing it.

…tionnal

* increase batch notifications tasks specs * add priority to EmailNotification and Notification generators job * allow notification generator to receive priority * add priority to events * add priority to job notification generator for recipient * add priority to email notification generator * add priority to notification mailer * add priority to email notification generator job * update high_priority method in event publisher job * add rule rubocop * revert added specs in batch email notification

* Notification generators job harmonization * light clean batch_email_notifications * ensure notif are not sent for destroyed account * light clean of specs * fix redundant config in specs

armandfardeau · 2020-11-19T11:48:00Z

Hello @tramuntanal It was a long trip but it's finished. You can find the test instructions in the issue description.

* fix css loading in emails * update resource_text to safe_resource_text for event received template * Revert "update resource_text to safe_resource_text for event received template" This reverts commit a5ddf52. * change notifications format in email batch notifications * update translation and tests * light refactor emails scss

carolromero · 2021-01-13T08:54:28Z

Hi @armandfardeau I was going to review this but I get system errors when trying to access any participatory space. Would you please take a look at it? 🙏 Thanks

armandfardeau · 2021-01-13T10:58:41Z

@carolromero Hi, thank you for doing this acceptance test. Could you reproduce the error so that we can intercept it?

carolromero · 2021-01-13T13:11:33Z

@armandfardeau sure!

Go to https://dcd-batch-notifications.osp.dev/processes
Click on any process
See the error

armandfardeau · 2021-01-14T17:09:45Z

@armandfardeau sure!

Go to https://dcd-batch-notifications.osp.dev/processes

Click on any process

See the error

Odd, I can't see anything on the monitoring.

mrcasals · 2021-01-15T07:46:15Z

@armandfardeau I can see errors visiting this page:

https://dcd-batch-notifications.osp.dev/processes/nemo-aspernatur

armandfardeau · 2021-01-19T11:28:27Z

@armandfardeau I can see errors visiting this page:

https://dcd-batch-notifications.osp.dev/processes/nemo-aspernatur

Thanks, we caught it via Sentry. I can't see the same errors as you.

virgile-dev · 2021-01-21T18:53:11Z

Hey @armandfardeau @Quentinchampenois
With the help of Pauline, I did a little product do-over on the the email content itself.
Could you implement the changes 🙏 ?

virgile-dev · 2021-01-22T09:28:10Z

Also we were discussing with @Quentinchampenois yesterday about the max notification that can fit in the email by default.
Instead of 5 we were thinking of 10 to leave a little room for users that receive a lot of notification (like admins).

armandfardeau · 2021-01-22T11:08:23Z

Also we were discussing with @Quentinchampenois yesterday about the max notification that can fit in the email by default.
Instead of 5 we were thinking of 10 to leave a little room for users that receive a lot of notification (like admins).

Thanks for the feedback!

mrcasals · 2021-02-01T10:01:05Z

@armandfardeau is there any updates on this? I see some conflicts, check them out please! 😄

armandfardeau · 2021-03-22T12:35:26Z

Hello everyone, we talked about this P.R. at OSP and we made the decision to close it because we can't work on it properly. We may reopen another one in the future.

tramuntanal added the status: WIP label Apr 28, 2020

andreslucena changed the title ~~WIP: Feature/send multiple notifications by email~~ Send multiple notifications by email May 18, 2020

stale bot added the wontfix label Jul 18, 2020

stale bot removed the wontfix label Jul 20, 2020

armandfardeau added 14 commits September 16, 2020 15:42

add batch email notification

c5d8d33

Avoid sending an already sent notification

f453469

Execute batch email with a rails task and make email notifications op…

11d1a6d

…tionnal

Pluralize notifications

534c9f8

Don't execute job if not necessary

1c4acd6

Change from last scope

b67447f

Pluralize key

c2e118a

Add a view more link

f9d2312

Add icon and time ago to batch notifications mails

2af7a36

Fix offenses

f990065

Respect user consentement

40e8098

Add test event_publisher_job

a3a8a8d

Add test batch_email_notifications_generator

a85fde9

Add test batch_notifications_mailer

b1cbf75

Quentinchampenois and others added 10 commits November 13, 2020 17:32

Fix overriden sql limit

a89e9bc

Fix limit on query

93c6b87

Light specs enhancement for batch notifications (#26)

f72c4fe

* Notification generators job harmonization * light clean batch_email_notifications * ensure notif are not sent for destroyed account * light clean of specs * fix redundant config in specs

Merge branch 'develop' into feature/send-multiple-notifications-by-email

96ce1ef

Fix useless require

bc048df

Fix empty group

3b7b63c

Merge branch 'develop' into feature/send-multiple-notifications-by-email

2c39a81

Update doc

d9a6209

Fix offense

f5a04c6

armandfardeau marked this pull request as ready for review November 19, 2020 11:45

armandfardeau requested a review from tramuntanal November 19, 2020 11:45

Leusev removed the status: WIP label Dec 14, 2020

andreslucena assigned carolromero Jan 13, 2021

mrcasals added the status: WIP label Jan 26, 2021

armandfardeau closed this Mar 22, 2021

andreslucena mentioned this pull request Nov 4, 2021

Notifications digest #8486

Closed

9 tasks

Uh oh!

Conversation

armandfardeau commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

Batch email notifications

How to setup?

App setup

Server setup

📋 Subtasks

📷 Screenshots (optional)

Demo

Uh oh!

tramuntanal commented Apr 28, 2020

Uh oh!

stale bot commented Jul 18, 2020

Uh oh!

andreslucena commented Jul 20, 2020

Uh oh!

armandfardeau commented Jul 30, 2020

Uh oh!

andreslucena commented Aug 3, 2020

Uh oh!

Quentinchampenois commented Aug 10, 2020

Uh oh!

Quentinchampenois commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulinebessoles commented Aug 24, 2020

Uh oh!

tramuntanal commented Aug 27, 2020

Uh oh!

tramuntanal commented Aug 27, 2020

Uh oh!

carolromero commented Aug 31, 2020

Uh oh!

armandfardeau commented Sep 2, 2020

Uh oh!

armandfardeau commented Nov 19, 2020

Uh oh!

carolromero commented Jan 13, 2021

Uh oh!

armandfardeau commented Jan 13, 2021

Uh oh!

carolromero commented Jan 13, 2021

Uh oh!

armandfardeau commented Jan 14, 2021

Uh oh!

mrcasals commented Jan 15, 2021

Uh oh!

armandfardeau commented Jan 19, 2021

Uh oh!

virgile-dev commented Jan 21, 2021

Uh oh!

virgile-dev commented Jan 22, 2021

Uh oh!

armandfardeau commented Jan 22, 2021

Uh oh!

mrcasals commented Feb 1, 2021

Uh oh!

armandfardeau commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

armandfardeau commented Mar 19, 2020 •

edited

Loading

Quentinchampenois commented Aug 20, 2020 •

edited

Loading