Problem/Motivation

A reviewer / manager might want to be notified when an item is ready for review.
Currently a reviewer to check the job item overview to see if there are pending items.

Proposed resolution

Trigger a mail when items are ready.
Avoid spamming when many items get ready.

CommentFileSizeAuthor
#3 notify_reviewer_about-2700789-3.patch3.4 KBedurenye

Comments

miro_dietiker created an issue. See original summary.

edurenye’s picture

edurenye commented
Assigned: Unassigned » edurenye
Status: Active » Needs review

First approach.
It works and send the mail.
I think we should somehow make the template configurable.
And I do not know how to avoid spamming when many items get ready.

edurenye’s picture

edurenye commented
StatusFileSize
new notify_reviewer_about-2700789-3.patch3.4 KB

Forgot the patch.

Status: Needs review » Needs work

The last submitted patch, 3: notify_reviewer_about-2700789-3.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Issue tags: +Needs tests

Some first feedback.

  1. +++ b/src/Entity/JobItem.php
@@ -1044,6 +1049,33 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
+    $message->setBody(\Drupal::service('renderer')->renderPlain($value));

    You are force setting the body with rendered values here.

    Instead i would expect to have a mail template with placeholders registered in courier allowing a user to modify the mail through the UI. Then we would only provide the tokens here and select that mail template. 

  2. +++ b/src/Entity/JobItem.php
@@ -1044,6 +1049,33 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
+      drupal_set_message(t('Message queued for delivery.'));
...
+      drupal_set_message(t('Failed to send message'), 'error');

    Sending a mail is an internal thing. We don't show a message. If things fail, you could only log a watchdog error.

    Also sending a mail only makes sense if the interacting user is not the owner - such as a service provides a translation. 

  3. +++ b/tmgmt.module
@@ -533,6 +533,12 @@ function tmgmt_theme() {
+        'item_label' => '',

    We also want to mention the job in the mail.

Also we need test coverage for this.
And as said, sending mails should be optional and we don't want to have TMGMT itself depend to courier. So we should make a separate module or check its presence. Also dunno about configurability of notifications: The site owner (or even each user?) should be able to enable / disable mail notifications. Is this configurability provided on courier level?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Eduard continued to work on this last week with more of a proof-of-concept implementation.
Architectural discussions are ongoing and important to clear before we can commit anything.

Please provide the link to the github repo here.

edurenye’s picture

edurenye commented

The module that extracts all the common functionality: https://github.com/edurenye/tmgmt_courier

The implementation for TMGMT: https://github.com/edurenye/tmgmt/pull/1

The implementation for another module (Monitoring): https://github.com/edurenye/monitoring/pull/1

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

I provided a bunch of feedback to the TMGMT implementation on github.

Most importantly, we need a default recipient set (as identity) for each message by the trigger origin.
And then i figured out some tokens are missing and more notifications make sense.

Finally, the common base module should provide some basic test helpers... so testing a mail workflow is easy.