Development: Restructure non-notification mails to use data transfer objects
#10219
Conversation
… mail content generated from thymeleaf templates
…adjust thymeleaf templates and add tests
…nd illegal annotation
github-actions
bot
added
tests
server
xHadie
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
xHadie
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
xHadie
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
WalkthroughThe changes refactor the email notification functionality in the application. The updates modify method signatures in email-sending services to use specialized Data Transfer Objects (DTOs) instead of the User entity. A new DTO interface and multiple implementations have been added to encapsulate recipient data. In addition, email templates have been updated to reference the new DTO properties, and corresponding tests have been adjusted or added to validate that the correct variables are passed and rendered in the emails. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MS as MailService
participant MTS as MailSendingService
participant TE as TemplateEngine
U->>MS: Request email action (e.g., activation)
Note over MS: Convert User to proper DTO\n(e.g., ActivationMailRecipientDTO)
MS->>MTS: Call sendEmail(DTO, subject, content, flags)
MTS->>TE: Render email template using DTO properties
TE-->>MTS: Return rendered email content
MTS-->>MS: Confirm email sending
MS-->>U: Email sent (logged or notified)
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/activation_mail/ActivationMailRecipientDTO.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185|
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…um/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…um/cit/aet/artemis/communication/service/notifications/mails/dto/notifications/NotificationMailRecipientDTO.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…um/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…um/cit/aet/artemis/communication/service/notifications/mails/dto/activation_mail/ActivationMailRecipientDTO.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
helios-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
…mail-service' into chore/communication/use-dtos-in-mail-service
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java (1)
27-35:⚠️ Potential issueAdd validation and sanitize exception message.
The current implementation has potential issues:
- No sanitization of exception messages
- Raw exception message might contain sensitive information
Apply this diff to implement message sanitization:
public static DataExportFailedContentDTO of(Exception exception, DataExport dataExport) { if (exception == null || dataExport == null) { throw new IllegalArgumentException("Parameters cannot be null"); } if (dataExport.getUser() == null) { throw new IllegalStateException("DataExport user cannot be null"); } + // Sanitize exception message to prevent sensitive information leakage + String sanitizedMessage = sanitizeExceptionMessage(exception.getMessage()); - return new DataExportFailedContentDTO(exception.getMessage(), dataExport.getUser().getLogin()); + return new DataExportFailedContentDTO(sanitizedMessage, dataExport.getUser().getLogin()); } + private static String sanitizeExceptionMessage(String message) { + if (message == null) { + return "Unknown error"; + } + // Remove potential sensitive information + return message.replaceAll("(?i)(password|secret|key|token|credentials)=.*?(&|$)", "$1=*****$2") + .replaceAll("(?i)(jdbc|ftp|sftp|smtp)://[^\\s]*", "[REDACTED_URL]") + .replaceAll("\\d{4}[- ]?\\d{4}[- ]?\\d{4}[- ]?\\d{4}", "[REDACTED_CARD_NUMBER]"); + }
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (1)
8-12: LGTM! Well-structured DTO using Java record.The implementation follows best practices:
- Uses Java record for immutable data structure
- Implements the DTO interface contract
- Includes JSON serialization optimization
Consider enhancing the class-level JavaDoc to document the purpose of each field:
/** * DTO for the password reset mail recipient + * + * @param langKey The language key for localization + * @param email The recipient's email address + * @param login The recipient's login identifier + * @param resetKey The password reset token */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/activation_mail/ActivationMailRecipientDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/saml2_set_password_mail/SAML2SetPasswordMailRecipientDTO.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/activation_mail/ActivationMailRecipientDTO.java
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/saml2_set_password_mail/SAML2SetPasswordMailRecipientDTO.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (6)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (2)
1-7: LGTM! Package structure and imports are well-organized.The package structure follows Java conventions, and imports are specific without any star imports.
14-31: LGTM! Factory method includes proper validation.The implementation includes appropriate null checks and validation as previously suggested, with clear error messages and well-documented exceptions.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java (4)
1-6: LGTM!Package structure follows naming conventions and imports are properly organized without wildcards.
7-13: LGTM!Documentation is clear and complete. The @JsonInclude annotation helps minimize the DTO payload.
14-14: LGTM!Appropriate use of Java record for an immutable DTO with minimal data fields.
16-26: LGTM!Factory method documentation is thorough and clearly describes parameters, return value, and potential exceptions.
| } | ||
|
|
||
| private void prepareTemplateAndSendEmail(User admin, String templateName, String titleKey, Context context) { | ||
| private void prepareTemplateAndSendEmail(IMailRecipientUserDTO recipient, String templateName, String titleKey, Context context) { |
There was a problem hiding this comment.
recipientAdmin for consistency (also for other occurrences)
| /** | ||
| * Retrieve the content of the interpreted thymeleaf template, which represents the mail content. | ||
| */ | ||
| protected String getGeneratedEmailTemplateText() { |
There was a problem hiding this comment.
I could image that this might introduce some flakyness to server tests by running two instances of getGeneratedEmailTemplateText in parallel. Could we maybe parameterize the method with something unique, like the subject for instance?
Checklist
General
Server
Motivation and Context
The usage of DTOs prevents the leakage of potentially sensitive information, makes use cases clearer, and results in more efficient data transfer.
In the context of the mail service, it allows developers to easily see what information is actually being transferred in a mail.
Furthermore, this PR introduces a straightforward testing strategy for HTML mail content generated by thymeleaf.
This ensures that the shift to DTOs within this PR did not break the interpretation and send logic of the mail messages. In addition, it allows us to test that DTO content is really being used within the mail content, making editing the scopes of DTOs and sending methods more convenient.
During development, it revealed, for instance, that the
dataExportFailedAdminEmail.htmltemplate used the recipient username instead of the username connected to the data export.In case this PR gets accepted, we can apply the same approach to the notification-based mails and also refactor the underlying bulk of thymeleaf fragments.
Implements a part of #9524
Description
Architecture:
AbstractMailContentTest, which allows testing HTML content generated by thymeleaf viagetGeneratedEmailTemplateTextfor the correct rendering of textIMailRecipientUserDTO, which contains the minimal set of required attributes a mail receiver needs to have to be compatible withMailSendingService.Individual mail templates will implement this interface in a dedicated recipient record, which may be extended with additional attributesMailSendingService::sendMailto useIMailRecipientUserDTOinstead ofUserMailService::createBaseContext,MailService::sendEmailFromTemplate,MailService::prepareTemplateAndSendEmailandMailService::prepareTemplateAndSendEmailWithArgumentInSubjectto useIMailRecipientUserDTOinstead ofUser[Use case] Activation mail:
ActivationMailRecipientDTOand the accompanyingActivationMailTestand adjustedMailService::sendActivationEmailto use the DTOactivationEmail.html[Use case] Password reset mail:
[Use case] SAML2 password reset mail:
[Use case] Data export failed mail:
DataExportFailedContentDTO, which contains only information that is relevant to the templateDataExportFailedMailAdminRecipientDTOand the accompanyingActivationMailTestand adjustedMailService::sendActivationEmailto use the DTOsMailService::sendDataExportFailedEmailForAdmindataExportFailedAdminEmail.html[Use case] Data export successful mail:
DataExportSuccessfulContentsDTOandDataExportSuccessfulContentDTO, which contain only information that is relevant to the template.DataExportSuccessfulMailAdminRecipientDTOand the accompanyingDataExportSuccessfulMailTestand adjustedMailService::sendSuccessfulDataExportsEmailToAdminto use the dtoMailService::sendSuccessfulDataExportsEmailToAdminsuccessfulDataExportsAdminEmail.html[Use case] Notifications mails:
NotificationMailRecipientDTOand adjustedMailService::sendNotificationAdditional Improvements:
MailService::sendSuccessfulDataExportsEmailToAdminfrompublictoprivateand adjustedDataExportScheduleServiceTestto use the parent method with an equal nameMailService::sendDataExportFailedEmailForAdminfrompublictoprivateMailService::sendEmailFromTemplatefrompublictoprivatedataExportFailedAdminEmail.htmlIdentified issues:
creationEmail.htmlis not being usedMailServiceneeds refactoring and is doing too much. Some of the private methods are relevant only for single-use cases. In my opinion, it would make sense to extract individual mail construction into their own classes and letMailServicedelegate the constructed mails toMailSendingService.Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
2. Click "Forgot Password?".
3. Enter your registered email.
4. Click [Reset Password].
5. Verify that you receive a password reset email and check its content for accuracy.
2. Click on Privacy in the footer.
3. Click Request Data Export.
4. Enter your geXXYYY username.
5. Verify that you receive an email (this may take a few days).
2. Wait a few minutes.
3. Verify that you receive an "Exercise Released" email notification.
Test Coverage
Summary by CodeRabbit