Email notifications should implement Drupal Queue operations

camilo.escobar → closed merge request !67

I can definitely see how this makes sense for sites with a large number of registrants.

How would you propose to execute the queue? With a cron? This will inevitably create a delay in the sending of some emails, is that acceptable?

The standard drupal cron is controllable by an admin in terms of how often it runs, but it is not granular and there are other admin tasks that run during cron, so there's no guarantee all jobs will execute each time cron runs.

There's ultimate cron available in the contrib space, but I am a little hesitant to add a dependency on it.

There's also the batch api but typically that happens client side, and not all the email sends happen after direct user interaction.

I'm just trying to think through the best approach.

tnx

@podarok @camilo.escobar could you provide some answers to owenbush's points raised in #4? perhaps you have discussed with him privately but as the owner of a site that uses this module, I would like to ensure that this major change will not have repercussions. My site has email notifications that can be sent out on the same day as an event and if they got stuck in the queue and not sent, that could be very bad

The changes in the MR imply that those notifications that by design are intended to be sent in bulk to multiple recipients, namely those corresponding to the keys below, are not sent immediately as soon as the triggering action occurs, but are added to a queue instead:

• registration_reminder
• series_modification_notification
• instance_modification_notification
• instance_deletion_notification
• series_deletion_notification

It means they don't go through recurring_events_registration_send_notification() immediately inside those foreach loops, but the Queue Worker will start processing them on the next cron run.

In those cases, the line
recurring_events_registration_send_notification($key, $registrant);
has been replaced by:
\Drupal::service('recurring_events_registration.notification_service')->addEmailNotificationToQueue($key, $registrant);

The new function addEmailNotificationToQueue($key, $registrant); is in charge of adding the notification to the queue. The function makes sure of invoking the hook hook_recurring_events_registration_send_notification_alter($send_email, $registrant) to allow modules to determine whether the notification should be sent (added to the queue) to the $registrant in question (the same as recurring_events_registration_send_notification($key, $registrant) does).
Also, the function introduces a new alter hook hook_ recurring_events_registration_message_params_alter($params, $registrant) to allow modules to add data to the $params array that is passed to the mail() function. Developers can use the data of the $registrant entity to set these params. Those $params can be used later as the $message['params'] in hook_mail_alter().

Actions that trigger only an email notification to a recipient (registrant) were left intact and continue to use the function recurring_events_registration_send_notification($key, $registrant), therefore the notification is sent out immediately. Namely:

• promotion_notification
• registration_notification
• waitlist_notification

These changes are my initial solution to overcome a big problem we were facing on our website: when an email event notification (e.g., instance_modification_notification) is triggered to a large number of registrants, the system may fail in those iterations and hit time and memory limits depending on the PHP configuration.

How long it takes for the queued notification list to be fully processed depends on three factors:

1. The number of notifications in the queue
2. How often Drupal cron runs
3. The number of seconds used by the queue worker on each cron run to process the items (it was set to 30)

This is acceptable in our case, since we have a large number of registrants in our events and we are aiming to run the Drupal cron every 5 or 10 minutes.

As I mentioned in the notes in the MR, there is a pending task:

Maybe provide a UI to configure if the system should use Queue Worker or not. People could have a checkbox to decide whether emails for notification types that are sent to multiple recipients should use the queue or send immediately.

@owenbush @podarok I think completing this to-do is important before merging the MR and releasing a new version, as not all the projects using the module may be interested in the queue implementation and may prefer to continue operating as usual: sending all emails immediately.

I know I owed you this technical background of the approach I implemented, and I was honestly expecting a MR review, some feedback and certain back-and-forth discussion before merging the MR. As I'm seeing this was already included in 2.0.0-rc8, I highly recommend to revert that release or to revert the MR and release a new version without those specific changes, since it is crucial that the to-do above is resolved, so as not to affect the normal operations of projects using the module.

@endless_wander, thanks for pointing out the importance of this case, please stick to a module version prior to 2.0.0-rc8, while I work in the above to-do.

I'll be working this weekend and create a new PR to resolve the pending item, so this solution, that can be very favorable in different cases, can be launched. This is what I will do:

1. Add a new configuration option in the Registrant Settings Form to enable/disable the queue of notifications
2. Have the option disabled by default, so that the websites continue operating as usual

@camilo.escobar your work and responsiveness on this is incredibly appreciated. thank you!

the guy definitely crazy )))
I appreciate
Thank you, merged and tagged

Automatically closed - issue fixed for 2 weeks with no activity.