Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send optional notifications via email #474

Merged
merged 20 commits into from
Jul 9, 2024
Merged

Send optional notifications via email #474

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
3767822
Add emailAddress to user table
pvannierop Jul 4, 2024
692f4c8
Impl. message transmitters
pvannierop Jul 2, 2024
95c333c
Throw error when notifications via email enabled and no email provide…
pvannierop Jul 4, 2024
654b316
Fix PMD errors
pvannierop Jul 5, 2024
1ae834f
Add test SMTP server to application-dev.properties
pvannierop Jul 5, 2024
ccf73a7
Add EmailNotificationTransmitterTest
pvannierop Jul 5, 2024
e87cece
Add comment on mail configuration
pvannierop Jul 5, 2024
9f7249f
feat: email notification per task in protocol
yatharthranjan Jul 5, 2024
e782888
Fix: emails in reminders
yatharthranjan Jul 5, 2024
2913061
Disable email notifications by default and rename properties
pvannierop Jul 8, 2024
21230f5
Log warnings when email address is missing
pvannierop Jul 8, 2024
f5d4c90
Remove UserService import from MessageJob
pvannierop Jul 8, 2024
94f434f
Change error handling in MessageJob
pvannierop Jul 8, 2024
979edb5
Use correct 'from' email field
pvannierop Jul 8, 2024
cc61090
Restore notification.isEmailEnabled()
pvannierop Jul 8, 2024
b64d89a
Update exception handling by MessageJob
pvannierop Jul 8, 2024
a2808c9
Move try to higher level
pvannierop Jul 8, 2024
6565c4d
Clean whitespace
pvannierop Jul 8, 2024
eb9fcf9
Add MessageJobTest
pvannierop Jul 8, 2024
2343690
Fix: disable pmdTest and fix PMD errors
yatharthranjan Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/main/java/org/radarbase/appserver/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,9 @@ public FcmUserDto saveUserInProject(FcmUserDto userDto) {
}

if (sendEmailNotifications && (userDto.getEmailAddress() == null || userDto.getEmailAddress().isEmpty())) {
throw new InvalidUserDetailsException(
"The option to send notifications via email is enabled. " +
"Email Address is required for sending email notifications. " +
"Please provide a valid email address.");
log.warn("No email address was provided for new subject '{}'. The option to send notifications via email " +
"('radar.notification.email.enabled') will not work for this subject. Consider to provide a valid email " +
"address for subject '{}'.", userDto.getSubjectId());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a warn log should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


User newUser = userConverter.dtoToEntity(userDto).setProject(project);
Expand Down
18 changes: 7 additions & 11 deletions src/main/java/org/radarbase/appserver/service/transmitter/EmailNotificationTransmitter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,16 @@ public EmailNotificationTransmitter(
@Override
public void send(Notification notification) {
if (notification.getUser().getEmailAddress() == null || notification.getUser().getEmailAddress().isBlank()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can move try to the top here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

log.error("Could not transmit a notification via email because subject {} has no email address.",
log.warn("Could not transmit a notification via email because subject {} has no email address.",
notification.getUser().getSubjectId());
return;
}
if (notification.isEmailEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pvannierop pvannierop Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -

  1. Each project may not want this functionality. With protocols, this can be enabled/disabled per project.
  2. 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.
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
emailSender.send(createEmailFromNotification(notification));
} catch (Exception e) {
// Note: the EmailNotificationTransmitter does not emit Exceptions caught by the JobExecutionException.
// As a result, email notifications are not influencing the job success/failure status.
log.error("Could not transmit a notification via email", e);
}
} else {
log.debug("Email notification is not enabled for: {}", notification.toString());
try {
emailSender.send(createEmailFromNotification(notification));
} catch (Exception e) {
// Note: the EmailNotificationTransmitter does not emit Exceptions caught by the JobExecutionException.
// As a result, email notifications are not influencing the job success/failure status.
log.error("Could not transmit a notification via email", e);
}
}

Expand Down