Notify the user via email about granted project access #365

sven1103 · 2023-09-08T08:12:00Z

After a user has been granted access to a project, they will now get
notified with an email.

The email also contains a link to the project to simplify access for the user.

…as-been-granted-access-to-a-project-with-a-link-to-it

Steffengreiner

Heya @sven1103 ! Thanks for the awesome implementation.
However i have some questions:
1.) The email generation code looks very simliar to the code provided in the UserContactService, is there a reason for a distinction here?
2.) I was unable to get an email if i granted project access to my user accounts or after project creation (@uni-tuebingen, @qbic.uni-tuebingen.de). Do i need to set additional environment variables or parameters to trigger the email sending?
I checked Jobrunner and found the following error:

org.jobrunr.scheduling.exceptions.JobClassNotFoundException:
life.qbic.authorization.application.policy.directive.InformUserAboutGrantedAccess.notifyUser(java.lang.String,java.lang.String,java.lang.String)

Steffengreiner · 2023-09-11T08:21:04Z

authorization/src/main/java/life/qbic/authorization/application/AppContextProvider.java

@@ -0,0 +1,21 @@
+package life.qbic.authorization.application;


Since then navigation is based on URL and the user authorization is not checked in the contextprovider i'd recommend moving the interface from the authorization package to the more general life.qbic.datamanager package

Dependency Inversion Principle. My only concern here was to introduce lose coupling so the auth module is not depending on the app UI module.

Steffengreiner · 2023-09-11T08:30:15Z

.../java/life/qbic/authorization/application/policy/directive/InformUserAboutGrantedAccess.java

+  private final EmailService emailService;
+
+  private final JobScheduler jobScheduler;
+  private final UserRepository userRepository;


You could think about having all the email sending logic be handled by the UserContactService class. If there is a reason for the distinction then i'd recommend renaming the UserContactService class to make the distinction more clear (e.g. UserAccountContactService)

I think this would be a strong violation of the Single Responsibility Pattern. I think it is not of the user contact service concern to know what it is about, but to deliver a message to a user with a given id. Imo, the service should care about finding the user's contact email address and sending the mail and only this. The content comes from the domain, where business logic policy demand to inform the user about an event.

Steffengreiner · 2023-09-11T08:32:29Z

authorization/src/main/java/life/qbic/authorization/application/AppContextProvider.java

+ */
+public interface AppContextProvider {
+
+  /**


This looks similiar to the PasswortResetLinkSupplier class maybe this could be streamlined?

Yes exactly, however this was my first effort to refactor it. The reset link supplier and confirmation link supplier would be also implemented in the app module and the interface extended accordingly.

Steffengreiner · 2023-09-11T08:34:23Z

.../java/life/qbic/authorization/application/policy/directive/InformUserAboutGrantedAccess.java

+  private String composeMessage(String projectId, User recipient, String projectTitle) {
+    return String.format("""
+        Dear %s,
+
+        you have been granted access to project:
+
+        '%s'
+
+        Please click the link below to access the project after login:
+
+        %s
+
+        Need help? Contact us for further questions at [email protected]
+
+        Best regards,\
+        The QBiC team
+        """, recipient.fullName(), projectTitle, appContextProvider.urlToProject(projectId));
+  }


We have a EmailFactory class which i'd expect to provide the content of the email messages. Is there a reason for the seperation here?

Different domain.

Steffengreiner · 2023-09-11T08:43:45Z

domain-concept/src/main/java/life/qbic/domain/concepts/communication/EmailService.java

@@ -3,4 +3,6 @@
 public interface EmailService {

  void send(Email email);
+
+  void send(String recipient, String recipientFullName, String subject, String message);


IMO this is unnecessary since we can just generate and provide the email record. (Even better if that's handled by the UserContactService similiar to the password reset email generation)

The issue is, that Email is introduced as domain concept but with a technical low level detail: mime type. To not break existing functionality, I decided to override the method with simple String values.

Steffengreiner · 2023-09-11T08:44:44Z

.../java/life/qbic/authorization/application/policy/directive/InformUserAboutGrantedAccess.java

+    emailService.send(recipient.emailAddress().get(), recipient.fullName().get(),
+        "Project access granted", composeMessage(projectId, recipient, projectTitle));


IMO we can just generate and provide the email record here

Suggested change

emailService.send(recipient.emailAddress().get(), recipient.fullName().get(),

"Project access granted", composeMessage(projectId, recipient, projectTitle));

new Email(composeMessage(projectId, recipient, projectTitle), "Project access granted",

"[email protected]",

new Recipient(recipient.emailAddress().get(), recipient.fullName().get()),

"text/plain"));

Steffengreiner · 2023-09-11T08:46:31Z

...handler/src/main/java/life/qbic/newshandler/usermanagement/email/EmailSubmissionService.java

+  @Override
+  public void send(String recipientAddress, String recipientFullName, String subject, String message) {
+    var email = new Email(message, subject, "[email protected]",
+        new Recipient(recipientAddress, recipientFullName),  "text/plain");
+    sendPlainEmail(email).ifSuccessOrElse(
+            successResponse -> reportSuccess(successResponse, email),
+            failureResponse -> reportFailure(failureResponse, email)
+    );
+  }


Unnecessary if we stick to the Email DTO which enables us to avoid redundancy here :)

Suggested change

@Override

public void send(String recipientAddress, String recipientFullName, String subject, String message) {

var email = new Email(message, subject, "[email protected]",

new Recipient(recipientAddress, recipientFullName), "text/plain");

sendPlainEmail(email).ifSuccessOrElse(

successResponse -> reportSuccess(successResponse, email),

failureResponse -> reportFailure(failureResponse, email)

);

}

Steffengreiner · 2023-09-11T08:47:22Z

...main/java/life/qbic/projectmanagement/domain/project/service/event/ProjectAccessGranted.java

+/**
+ * <b><class short description - 1 Line!></b>
+ *
+ * <p><More detailed description - When to use, what it solves, etc.></p>
+ *
+ * @since <version tag>
+ */


Steffengreiner · 2023-09-11T08:49:52Z

vaadinfrontend/src/main/java/life/qbic/datamanager/DataManagerContextProvider.java

+ * @since 1.0.0
+ */
+@Component
+public class DataManagerContextProvider implements AppContextProvider {


NIT: Should the DataManagerContextProvider always provide a project endpoint? 🤔
Also the functionality looks really simliar to previous URL generation classes like e.g. the PasswordResetLinkSupplier class

sonarcloud · 2023-09-11T11:44:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

Steffengreiner

LGTM 🚀

sven1103 added 2 commits September 6, 2023 17:39

First draft implementation

02a0f5b

Refactored version with email submission

3cb0df1

github-actions bot added the feature label Sep 8, 2023

sven1103 marked this pull request as ready for review September 8, 2023 08:12

sven1103 requested a review from a team as a code owner September 8, 2023 08:12

sven1103 changed the base branch from main to development September 8, 2023 08:13

Merge branch 'development' into feature/dm-976-notify-the-user-that-h…

215ca4e

…as-been-granted-access-to-a-project-with-a-link-to-it

sven1103 changed the title ~~Notify the user after they have been granted project access~~ Notify the user via email about granted project access Sep 8, 2023

Steffengreiner requested changes Sep 11, 2023

View reviewed changes

Provide JDs

fbf18c2

Steffengreiner approved these changes Sep 11, 2023

View reviewed changes

sven1103 merged commit 40b7fa7 into development Sep 11, 2023
5 checks passed

sven1103 deleted the feature/dm-976-notify-the-user-that-has-been-granted-access-to-a-project-with-a-link-to-it branch September 11, 2023 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify the user via email about granted project access #365

Notify the user via email about granted project access #365

sven1103 commented Sep 8, 2023

Steffengreiner left a comment •

edited

Loading

Steffengreiner Sep 11, 2023

sven1103 Sep 11, 2023

Steffengreiner Sep 11, 2023

sven1103 Sep 11, 2023

Steffengreiner Sep 11, 2023

sven1103 Sep 11, 2023

Steffengreiner Sep 11, 2023

sven1103 Sep 11, 2023

Steffengreiner Sep 11, 2023

sven1103 Sep 11, 2023

Steffengreiner Sep 11, 2023

Steffengreiner Sep 11, 2023

Steffengreiner Sep 11, 2023

Steffengreiner Sep 11, 2023

sonarcloud bot commented Sep 11, 2023

Steffengreiner left a comment

		@@ -0,0 +1,21 @@
		package life.qbic.authorization.application;

		emailService.send(recipient.emailAddress().get(), recipient.fullName().get(),
		"Project access granted", composeMessage(projectId, recipient, projectTitle));

Notify the user via email about granted project access #365

Notify the user via email about granted project access #365

Conversation

sven1103 commented Sep 8, 2023

Steffengreiner 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

sonarcloud bot commented Sep 11, 2023

Steffengreiner left a comment

Choose a reason for hiding this comment

Steffengreiner left a comment •

edited

Loading