Send optional notifications via email #474

pvannierop · 2024-07-05T08:22:14Z

Proposal

This PR implements the optional sending of notifications via email alongside push notifications. This feature is controlled by the new application property:

radar.notification.email.enabled=true/false

Implementation details

Optional email address field was added to the User object. When radar.notification.email.enabled=true and no email address provided at registration of new user, an warning is logged.
The message sending functions present in the NotificationSchedulingService and DataMessageSchedulingService were re-located to MessageTransmitter implementing classes. As such, the MessageScheduler schedules messages, and the MessageTransmitters send messages.
The MessageJob fired by Quartz handles one-or-more NotificationTransmitters and/or DataMessageTransmitters. These transmitters are conditionally created and injected by Spring.
Only the success/failure state of Firebase messages determine the success/failure state of the MessageJob. In effect, sending emails does not determine the retry or removal of scheduled jobs.
When radar.notification.email.enabled=true and there is no email provided for the User, this is logged as a warning, but no Exception is thrown.

request-info · 2024-07-05T08:22:24Z

We would appreciate it if you could provide us with more info about this issue/pr! It will help solve the problem faster.

…d for new user

yatharthranjan

Thanks for this. It mostly looks good to me. Please find a few comments below.

We should also have this configured in the protocol per assessment (or through the notification endpoint so the app can enable/disable it in the future). For now, I have pushed a quick version of this; please take a look and feel free to make any changes.

yatharthranjan · 2024-07-05T12:15:46Z

src/main/resources/application-prod.properties

@@ -40,6 +40,18 @@ spring.quartz.properties.auto-startup=true
 #spring.quartz.properties.org.quartz.jobStore.useProperties = true
 spring.quartz.properties.org.quartz.jobStore.driverDelegateClass=org.quartz.impl.jdbcjobstore.PostgreSQLDelegate

+# EMAIL SETTINGS
+# needed when notifications via email are enabled
+notification.email.enabled=true


please disable by default

Yes, indeed. Changed as per your suggestion.

yatharthranjan · 2024-07-05T12:15:52Z

src/main/resources/application-dev.properties

@@ -64,6 +64,14 @@ logging.pattern.console=%clr(%d{yyyy-MM-dd HH:mm:ss.SSS}){faint} %clr(%5p) %clr(
 # Firebase Cloud Messaging
 fcmserver.fcmsender=org.radarbase.fcm.downstream.AdminSdkFcmSender

+# EMAIL SETTINGS
+# needed when notifications via email are enabled
+notification.email.enabled=true


please disable by default

also, can you add radar.notification.email.enabled=true

yatharthranjan · 2024-07-05T12:19:54Z

src/main/java/org/radarbase/appserver/service/UserService.java

+            "The option to send notifications via email is enabled. " +
+                "Email Address is required for sending email notifications. " +
+                "Please provide a valid email address.");
+    }


I don't think this should be a requirement for adding a user, as this can be added later using the update endpoint. Moreover, I am adding the ability to customise email sending per assessment block in the protocol as I don't like enabling this for the whole deployment, so this can be disabled for some tasks/projects even when the environment property is enabled.

Perhaps a warn log should be enough

Ok, good point. I did not know about the update endpoint. I will log a warning instead.

yatharthranjan · 2024-07-05T14:42:51Z

src/main/java/org/radarbase/appserver/service/scheduler/quartz/MessageJob.java

+          // Note: at present there are FcmTransmitter and EmailNotificationTransmitter.
+          // Only the Fcm transmitter (sneakily) emits Exceptions caught by the JobExecutionException.
+          // As a result Fcm notifications are leading the job success/failure status.
+          notificationTransmitters.forEach(t -> t.send(notification));


Why not add another try/catch here (and for data one, too), so it can progress through the rest of the transmitters even if one fails? You can initialise a variable (List<Exception> exceptions) before the for loop and add all the exceptions to it during the processing without breaking the loop and then in finally block check if there were any exceptions during the processing and raise the throw new JobExecutionException(e);.

I reworked the error handling logic as per your suggestion. Can you confirm this solution is inline with you idea?

yatharthranjan · 2024-07-05T14:44:04Z

src/main/java/org/radarbase/appserver/service/transmitter/EmailNotificationTransmitter.java

+
+    private static SimpleMailMessage createEmailFromNotification(Notification notification) {
+        SimpleMailMessage message = new SimpleMailMessage();
+        message.setFrom(notification.getUser().getEmailAddress());


Should this be the from value specified in the spring properties?

Ouch, indeed. Fixed...

yatharthranjan · 2024-07-05T14:49:43Z

src/main/resources/application-dev.properties

+# EMAIL SETTINGS
+# needed when notifications via email are enabled
+notification.email.enabled=true
+[email protected]


similarly [email protected]

yatharthranjan · 2024-07-05T16:03:17Z

src/main/java/org/radarbase/appserver/service/scheduler/quartz/MessageJob.java

@@ -61,13 +60,13 @@ public class MessageJob implements Job {
  private final transient UserService userService;


i presume this is not needed in this class anymore

Indeed, I removed this reference.

Instead of throwing an exception.

yatharthranjan

Thanks for the changes. Looks nice—just a few minor final comments.

yatharthranjan · 2024-07-08T10:46:43Z

src/main/java/org/radarbase/appserver/service/transmitter/EmailNotificationTransmitter.java

                notification.getUser().getSubjectId());
            return;
        }
-        if (notification.isEmailEnabled()) {


I think you need this if statement for enabling email per assessment through the protocol.

Honestly, I do not understand what you did with the protocol. I cannot find the place where the NotificationProtocol is informed whether to use or not use email.

I have the feeling that this protocol-based mechanism is a parallel track next to the conditional bean creation of the EmailTranmitterService:

@Component @ConditionalOnProperty(value = "radar.notification.email.enabled", havingValue = "true") public class EmailNotificationTransmitter implements NotificationTransmitter {

And would this be not a more correct implementation if I understand your proposal well?

@Override public void send(Notification notification) throws EmailMessageTransmitException { if (notification.isEmailEnabled()) { if (notification.getUser().getEmailAddress() == null || notification.getUser().getEmailAddress().isBlank()) { log.warn("Could not transmit a notification via email because subject {} has no email address.", notification.getUser().getSubjectId()); return; } try { emailSender.send(createEmailFromNotification(notification)); } catch (Exception e) { log.error("Could not transmit a notification via email", e); throw new EmailMessageTransmitException("Could not transmit a notification via email", e); } } }

Note the isEmailEnabled check as first thing in the block?

I am fine with readding this line, but I also would like to make sure that I understand it and change the other code accordingly.

Honestly, I do not understand what you did with the protocol. I cannot find the place where the NotificationProtocol is informed whether to use or not use email.

Apologies, my bad. I should have included a rationale with the code. The information about whether to use email will come from the project protocol. This will be in the notifications key (e.g. here) in each assessment's protocol block.

This is to make the email configuration more granular than enabling it to be deployment-wide. This is because of these reasons -

Each project may not want this functionality. With protocols, this can be enabled/disabled per project.

Within a project, some assessments might be prioritised; a requirement could be that email should only be sent for these priority assessments; others can have disabled email and just have push notifications.

Multiple assessments could be scheduled simultaneously in the protocol with the same notification message and title. However, only one notification is usually required for these assessments (to avoid bombarding the user with superfluous notifications). This is achieved by the protocol's Notification block, where we can disable notifications for similar tasks and leave only one enabled (this was recently implemented in Notification scheduling updates #458). A similar arrangement should be made for email notifications.

We are moving task and protocol scheduling from the aRMT app to the AppServer, so if you are still using scheduling in the app, you might need a minor update there. This would involve reading the new email block in the protocols, and passing this info (emailEnabled) when adding notifications to the Appserver (the endpoint should support this as I have also updated the DTO).
Alternatively, you can enable the scheduling through the Appserver using a remote config property, which @mpgxvii can confirm. The Appsever automatically reads the protocol files and schedules tasks and notifications for all the users in the database.

Note the isEmailEnabled check as first thing in the block?

Yes, you are correct. That is a better approach. If the email is not enabled for this notification (signified by the assessment in the protocol), then we don't execute the email-sending code. You can log a debug message if it was not executed due to emailEnabled=false, which can help debug issues with email sending.

I hope this was clear, but if not, please feel free to Slack me, and we can discuss this further.

I reintroduced the notification.isEmailEnabled() conditional. We decided to keep both the notification-level and application-level configuration option to enable notifications via email.

yatharthranjan · 2024-07-08T10:51:47Z

src/main/java/org/radarbase/appserver/service/scheduler/quartz/MessageJob.java

+
+      // Here handle the exceptions that occurred while transmitting the message via the
+      // transmitters. At present, only the FcmTransmitter affects the job execution state.
+      if (exceptions.stream().anyMatch(e -> e instanceof FcmMessageTransmitException)) {


You can also keep the top-level try and catch (in case the error is somewhere outside the inner try/catch) and run this code in the finally block.

I reintroduced the top level try-catch.

PMD complains about throwing in a finally block. Now that we have the top-level try/catch, we don't really need the finally block, so I will remove it.

yatharthranjan · 2024-07-08T11:05:39Z

src/main/java/org/radarbase/appserver/service/transmitter/EmailNotificationTransmitter.java

+
+    @Override
+    public void send(Notification notification) throws EmailMessageTransmitException {
+        if (notification.getUser().getEmailAddress() == null || notification.getUser().getEmailAddress().isBlank()) {


you can move try to the top here

yatharthranjan · 2024-07-08T11:06:29Z

src/main/java/org/radarbase/appserver/service/transmitter/FcmTransmitter.java

+        try {
+            fcmSender.send(createMessageFromDataMessage(dataMessage));
+        }  catch (FirebaseMessagingException exc) {
+            handleFcmException(exc, dataMessage);


please raise FcmMessageTransmitException (encapsulate the FirebaseMessagingException exc) here too

I do not think I fully understand. You suggest that the FirebaseMessageingException will also resulti in a FcmMessageTransmitException? That is different from the orgiginal implementation AFAIKS:

In MessageJob:

} catch (FirebaseMessagingException exc) { log.error("Error occurred when sending downstream message.", exc); // TODO: update the data message status using event if (message != null) { handleErrorCode(exc.getErrorCode(), message); handleFCMErrorCode(exc.getMessagingErrorCode(), message); } } catch (Exception e) { throw new JobExecutionException(e); }

Note that FirebaseMessagingException does NOT result in the JobExecutionException.

Okay, agreed. Let's keep the original functionality for now. I was just thinking that if the notification was not sent successfully, that should maybe count towards the unsuccessful job so the notification event lifecycle can be updated with failure, but I think we can tackle it in another issue.

yatharthranjan · 2024-07-08T11:06:34Z

src/main/java/org/radarbase/appserver/service/transmitter/FcmTransmitter.java

+        try {
+            fcmSender.send(createMessageFromNotification(notification));
+        }  catch (FirebaseMessagingException exc) {
+            handleFcmException(exc, notification);


please raise FcmMessageTransmitException (encapsulate the FirebaseMessagingException exc) here too

yatharthranjan · 2024-07-08T11:09:59Z

src/main/java/org/radarbase/appserver/service/scheduler/quartz/MessageJob.java

      }
-    } catch (Exception e) {
-      throw new JobExecutionException(e);


you can keep still keep this try/catch.

This was re-introduced.

yatharthranjan

LGTM, thanks

pvannierop self-assigned this Jul 5, 2024

pvannierop changed the base branch from master to dev July 5, 2024 08:22

request-info bot added the needs-more-info label Jul 5, 2024

pvannierop added 3 commits July 5, 2024 10:25

Add emailAddress to user table

3767822

Impl. message transmitters

692f4c8

Throw error when notifications via email enabled and no email provide…

95c333c

…d for new user

pvannierop requested review from yatharthranjan and mpgxvii July 5, 2024 08:41

pvannierop force-pushed the notifications-via-email branch from f032f80 to 95c333c Compare July 5, 2024 08:41

pvannierop mentioned this pull request Jul 5, 2024

Impl. send-email endpoint #473

Closed

pvannierop and others added 6 commits July 5, 2024 11:15

Fix PMD errors

654b316

Add test SMTP server to application-dev.properties

1ae834f

Add EmailNotificationTransmitterTest

ccf73a7

Add comment on mail configuration

e87cece

feat: email notification per task in protocol

9f7249f

Fix: emails in reminders

e782888

yatharthranjan reviewed Jul 5, 2024

View reviewed changes

pvannierop added 5 commits July 8, 2024 10:24

Disable email notifications by default and rename properties

2913061

Log warnings when email address is missing

21230f5

Instead of throwing an exception.

Remove UserService import from MessageJob

f5d4c90

Change error handling in MessageJob

94f434f

Use correct 'from' email field

979edb5

pvannierop marked this pull request as ready for review July 8, 2024 08:31

pvannierop requested review from yatharthranjan and removed request for mpgxvii July 8, 2024 08:31

yatharthranjan reviewed Jul 8, 2024

View reviewed changes

pvannierop requested a review from yatharthranjan July 8, 2024 13:22

pvannierop force-pushed the notifications-via-email branch from 7661b94 to d5361c2 Compare July 8, 2024 14:53

pvannierop added 4 commits July 8, 2024 17:48

Restore notification.isEmailEnabled()

cc61090

Update exception handling by MessageJob

b64d89a

Move try to higher level

a2808c9

Clean whitespace

6565c4d

pvannierop force-pushed the notifications-via-email branch from d5361c2 to e516583 Compare July 8, 2024 15:49

Add MessageJobTest

eb9fcf9

pvannierop force-pushed the notifications-via-email branch from e516583 to eb9fcf9 Compare July 8, 2024 15:59

Fix: disable pmdTest and fix PMD errors

2343690

yatharthranjan approved these changes Jul 9, 2024

View reviewed changes

yatharthranjan merged commit 04e7edd into dev Jul 9, 2024
4 checks passed

yatharthranjan deleted the notifications-via-email branch July 9, 2024 15:55

mpgxvii mentioned this pull request Jul 19, 2024

Update protocol schema RADAR-base/RADAR-aRMT-protocols#199

Merged

		@@ -61,13 +60,13 @@ public class MessageJob implements Job {
		private final transient UserService userService;

Send optional notifications via email #474

Send optional notifications via email #474

Conversation

pvannierop commented Jul 5, 2024 • edited Loading

Proposal

Implementation details

request-info bot commented Jul 5, 2024

yatharthranjan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yatharthranjan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvannierop Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvannierop Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yatharthranjan left a comment

Choose a reason for hiding this comment

pvannierop commented Jul 5, 2024 •

edited

Loading

yatharthranjan left a comment •

edited

Loading

pvannierop Jul 8, 2024 •

edited

Loading

pvannierop Jul 8, 2024 •

edited

Loading