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
…mail-service' into chore/communication/use-dtos-in-mail-service
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
...de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (2)
17-19:⚠️ Potential issueUpdate test to use ActivationMailRecipientDTO.
The test currently uses the
Userentity directly, which doesn't align with the PR's objective of using DTOs for mail services.
7-7:⚠️ Potential issueReplace User import with ActivationMailRecipientDTO.
The PR aims to transition from using User entities to DTOs. Update the import to use ActivationMailRecipientDTO instead.
-import de.tum.cit.aet.artemis.core.domain.User; +import de.tum.cit.aet.artemis.communication.service.notifications.mails.dto.activation_mail.ActivationMailRecipientDTO;
🧹 Nitpick comments (6)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.java (2)
30-53: Consider making non-protected fields private.Fields that are only used within this class should be private to maintain encapsulation. This includes
templateEngine,markdownCustomLinkRendererService, andmarkdownCustomReferenceRendererService.Apply this diff:
- private SpringTemplateEngine templateEngine; + @Autowired + private final SpringTemplateEngine templateEngine; - private MarkdownCustomLinkRendererService markdownCustomLinkRendererService; + @Autowired + private final MarkdownCustomLinkRendererService markdownCustomLinkRendererService; - private MarkdownCustomReferenceRendererService markdownCustomReferenceRendererService; + @Autowired + private final MarkdownCustomReferenceRendererService markdownCustomReferenceRendererService;
58-62: Consider parameterizing the language key.The
createMinimalMailRecipientUsermethod hardcodes the language key to "de". Consider making it a parameter to support testing with different locales.Apply this diff:
- protected User createMinimalMailRecipientUser() { + protected User createMinimalMailRecipientUser(String langKey) { User recipient = new User(); - recipient.setLangKey("de"); + recipient.setLangKey(langKey); return recipient; }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java (2)
14-35: Enhance test method for better clarity and coverage.While the test follows good practices like AAA pattern and AssertJ assertions, consider these improvements:
- Rename method to describe the scenario without "test" prefix (e.g.,
verifyUserLoginsAreInjectedIntoTemplate).- Use more specific assertions to verify exact content and order.
- Use more descriptive variable names (e.g.,
adminRecipientinstead ofrecipient).- Add assertions for other template variables.
- void testThatVariablesAreInjectedIntoTheTemplate() { + void verifyUserLoginsAreInjectedIntoTemplate() { // Arrange: - User recipient = createMinimalMailRecipientUser(); - recipient.setLogin("test_login"); - recipient.setResetKey("test_reset_key"); + User adminRecipient = createMinimalMailRecipientUser(); + adminRecipient.setLogin("admin_login"); + adminRecipient.setResetKey("test_reset_key"); - String subject = createExpectedSubject(recipient, "email.successfulDataExportCreationsAdmin.title"); + String emailSubject = createExpectedSubject(adminRecipient, "email.successfulDataExportCreationsAdmin.title"); Set<DataExport> dataExports = createThreeDataExportsWithThreeDifferentUsers(); // Act: - mailService.sendSuccessfulDataExportsEmailToAdmin(recipient, dataExports); + mailService.sendSuccessfulDataExportsEmailToAdmin(adminRecipient, dataExports); // Assert: - String capturedContent = getGeneratedEmailTemplateText(subject); - assertThat(capturedContent).contains("test_subject_1"); - assertThat(capturedContent).contains("test_subject_2"); - assertThat(capturedContent).contains("test_subject_3"); + String emailContent = getGeneratedEmailTemplateText(emailSubject); + assertThat(emailContent) + .contains("Dear admin_login") + .contains("The following data exports have been created successfully:") + .satisfies(content -> + assertThat(content.lines()) + .filteredOn(line -> line.contains("test_subject")) + .containsExactlyInAnyOrder( + "test_subject_1", + "test_subject_2", + "test_subject_3" + ) + );
37-52: Improve helper method for better reusability and readability.Consider these improvements:
- Make the method more concise using streams.
- Use more meaningful test data.
- Consider moving to a test utility class for reuse.
- private static Set<DataExport> createThreeDataExportsWithThreeDifferentUsers() { - User dataExportSubjectUser1 = new User(); - User dataExportSubjectUser2 = new User(); - User dataExportSubjectUser3 = new User(); - dataExportSubjectUser1.setLogin("test_subject_1"); - dataExportSubjectUser2.setLogin("test_subject_2"); - dataExportSubjectUser3.setLogin("test_subject_3"); - DataExport dataExport1 = new DataExport(); - DataExport dataExport2 = new DataExport(); - DataExport dataExport3 = new DataExport(); - dataExport1.setUser(dataExportSubjectUser1); - dataExport2.setUser(dataExportSubjectUser2); - dataExport3.setUser(dataExportSubjectUser3); - - return Set.of(dataExport1, dataExport2, dataExport3); + private static Set<DataExport> createThreeDataExportsWithThreeDifferentUsers() { + return Stream.of("student", "tutor", "instructor") + .map(role -> { + User user = new User(); + user.setLogin("test_" + role); + DataExport dataExport = new DataExport(); + dataExport.setUser(user); + return dataExport; + }) + .collect(Collectors.toSet()); + }Consider moving this to a test utility class:
public final class DataExportTestUtil { private DataExportTestUtil() { // Prevent instantiation } public static Set<DataExport> createDataExportsWithUsers(String... userLogins) { return Arrays.stream(userLogins) .map(login -> { User user = new User(); user.setLogin(login); DataExport dataExport = new DataExport(); dataExport.setUser(user); return dataExport; }) .collect(Collectors.toSet()); } }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (2)
14-15: Make test method name more specific.The current method name
testThatVariablesAreInjectedIntoTheTemplateis too generic. Consider renaming it to be more specific about which variables are being tested.- void testThatVariablesAreInjectedIntoTheTemplate() { + void testThatLoginAndActivationKeyAreInjectedIntoActivationTemplate() {
27-28: Make assertions more specific.The current assertions only check if the values exist somewhere in the content. Consider using more specific assertions that verify where in the template these values should appear.
- assertThat(capturedContent).contains("test_student"); - assertThat(capturedContent).contains("test_key"); + assertThat(capturedContent) + .describedAs("Login should be present in the greeting section") + .contains("<span>Dear test_student</span>"); + assertThat(capturedContent) + .describedAs("Activation key should be present in the activation link") + .contains("key=test_key");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/WeeklySummaryMailTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java
- src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.java
- src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/WeeklySummaryMailTest.java
- src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.javasrc/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.javasrc/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.java (2)
1-28: LGTM! Well-structured test class.The class is well-organized with appropriate package structure, imports, and documentation. The package-private visibility is suitable for a test class.
73-77: Potential test flakiness in parallel execution.The
getGeneratedEmailTemplateTextmethod might cause flaky tests when running in parallel, as it uses a shared ArgumentCaptor.Consider adding a unique identifier (like the subject) to the verification to ensure test isolation:
protected String getGeneratedEmailTemplateText(String expectedSubject) { ArgumentCaptor<String> contentCaptor = ArgumentCaptor.forClass(String.class); - verify(mailSendingService).sendEmail(any(), eq(expectedSubject), contentCaptor.capture(), anyBoolean(), anyBoolean()); + verify(mailSendingService).sendEmail(any(), eq(expectedSubject), contentCaptor.capture(), anyBoolean(), anyBoolean()); return contentCaptor.getValue(); }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java (1)
1-12: LGTM!The class is well-structured with appropriate naming, minimal imports, and proper inheritance from
AbstractMailContentTest.src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (1)
9-9: LGTM!The class name is descriptive and follows test naming conventions. The inheritance from AbstractMailContentTest is appropriate for mail content testing.
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
…mail-service' into chore/communication/use-dtos-in-mail-service
xHadie
dismissed stale reviews from BBesrour, coderabbitai[bot], and ole-ve
via
b913cf9
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.java (3)
8-8:⚠️ Potential issueReplace User import with SAML2SetPasswordMailRecipientDTO.
The PR aims to transition from User entities to DTOs for mail recipients. Update the import to use SAML2SetPasswordMailRecipientDTO instead.
-import de.tum.cit.aet.artemis.core.domain.User; +import de.tum.cit.aet.artemis.communication.service.notifications.mails.dto.saml2_set_password_mail.SAML2SetPasswordMailRecipientDTO;
16-30:⚠️ Potential issueUpdate test to use SAML2SetPasswordMailRecipientDTO.
The test should use SAML2SetPasswordMailRecipientDTO instead of User entity to align with the PR's objective of using DTOs for mail recipients.
@Test void testThatVariablesAreInjectedIntoTheTemplate() { // Arrange: - User recipient = createMinimalMailRecipientUser(); - recipient.setLogin("test_login"); - recipient.setResetKey("test_reset_key"); + SAML2SetPasswordMailRecipientDTO recipient = new SAML2SetPasswordMailRecipientDTO( + "test_login", + "test_reset_key", + "en", + "[email protected]" + ); String subject = createExpectedSubject(recipient, "email.saml.title"); // Act: mailService.sendSAML2SetPasswordMail(recipient); // Assert: String capturedContent = getGeneratedEmailTemplateText(subject); assertThat(capturedContent).contains("test_login"); assertThat(capturedContent).contains("test_reset_key"); }
32-39:⚠️ Potential issueUpdate exception test to use SAML2SetPasswordMailRecipientDTO.
The test should use SAML2SetPasswordMailRecipientDTO instead of User entity to maintain consistency with the PR's objective.
@Test void testThatExceptionIsThrownWhenResetKeyIsMissing() { // Arrange: - User recipient = createMinimalMailRecipientUser(); + SAML2SetPasswordMailRecipientDTO recipient = new SAML2SetPasswordMailRecipientDTO( + "test_login", + null, + "en", + "[email protected]" + ); // Act and Assert: assertThatThrownBy(() -> mailService.sendSAML2SetPasswordMail(recipient)) .isInstanceOf(IllegalStateException.class) .hasMessage("Reset key is required"); }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (1)
35-36: 🛠️ Refactor suggestionUpdate test to use ActivationMailRecipientDTO.
The test should use ActivationMailRecipientDTO to maintain consistency with the service layer changes.
- User recipient = createMinimalMailRecipientUser(); - recipient.setLogin("test_student"); + ActivationMailRecipientDTO recipient = new ActivationMailRecipientDTO( + "test_student", + null, // Missing activation key to trigger exception + "en", + "[email protected]" + );src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (1)
22-27:⚠️ Potential issueAdd null check in factory method.
The factory method should validate both the user object and ensure resetKey is present for password reset functionality.
public static PasswordResetRecipientDTO of(User user) { + if (user == null) { + throw new IllegalArgumentException("User cannot be null"); + } if (user.getResetKey() == null) { throw new IllegalStateException("Reset key is required"); } return new PasswordResetRecipientDTO(user.getLangKey(), user.getEmail(), user.getLogin(), user.getResetKey()); }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java (1)
12-30: 🛠️ Refactor suggestionUpdate test to use PasswordResetRecipientDTO.
The test should use
PasswordResetRecipientDTOinstead of theUserentity to align with the PR's objective of using DTOs for non-notification mails.@Test void testThatVariablesAreInjectedIntoTheTemplate() { // Arrange: - User recipient = createMinimalMailRecipientUser(); - recipient.setLogin("test_login"); - recipient.setResetKey("test_reset_key"); + PasswordResetRecipientDTO recipient = new PasswordResetRecipientDTO( + "en", + "[email protected]", + "test_login", + "test_reset_key" + ); String subject = createExpectedSubject(recipient, "email.reset.title"); // Act: mailService.sendPasswordResetMail(recipient); // Assert: String capturedContent = getGeneratedEmailTemplateText(subject); assertThat(capturedContent).contains("test_login"); assertThat(capturedContent).contains("test_reset_key"); }
🧹 Nitpick comments (4)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java (3)
23-24: Fix typo in variable name.The variable name contains a typo:
dataExporSubjecttUsershould bedataExportSubjectUser.- User dataExporSubjecttUser = new User(); - dataExporSubjecttUser.setLogin("test_subject"); + User dataExportSubjectUser = new User(); + dataExportSubjectUser.setLogin("test_subject");
17-39: Enhance test coverage and data quality.While the test follows good practices with AAA pattern and clear assertions, consider these improvements:
- Use more realistic test data instead of test_* prefixes
- Add assertions for the complete HTML structure to ensure template integrity
- Consider testing different types of exceptions and their messages
41-70: Consider using parameterized tests for null checks.The null check tests follow good practices but could be more concise using JUnit 5's @ParameterizedTest feature. This would reduce code duplication while maintaining the same coverage.
Example refactor:
@ParameterizedTest @MethodSource("nullParameterProvider") void testExceptionsThrownForNullParameters(User recipient, DataExport dataExport, Exception exception, Class<? extends Exception> expectedExceptionType, String expectedMessage) { assertThatThrownBy(() -> mailService.sendDataExportFailedEmailToAdmin(recipient, dataExport, exception)) .isInstanceOf(expectedExceptionType) .hasMessage(expectedMessage); } private static Stream<Arguments> nullParameterProvider() { User recipient = createMinimalMailRecipientUser(); return Stream.of( Arguments.of(recipient, new DataExport(), null, IllegalArgumentException.class, "Parameters cannot be null"), Arguments.of(recipient, null, new Exception(), IllegalArgumentException.class, "Parameters cannot be null"), Arguments.of(recipient, new DataExport(), new Exception(), IllegalStateException.class, "DataExport user cannot be null") ); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (1)
14-21: Improve Javadoc documentation.The documentation needs the following improvements:
- Fix formatting in @param tag (remove extra indentation)
- Add @throws tag for IllegalArgumentException when user is null
/** * Factory method to create a {@link PasswordResetRecipientDTO} instance from a {@link User}. * * @param user The user object used to populate the DTO. - * The {@code resetKey} field in the {@code User}. + * The {@code resetKey} field in the {@code User}. * @return A new {@link PasswordResetRecipientDTO} containing the user's language key, email, login, and reset key. * @throws IllegalStateException if the {@code resetKey} field of {@code user} is {@code null}. + * @throws IllegalArgumentException if {@code user} is {@code null}. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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/notifications/NotificationMailRecipientDTO.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)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/WeeklySummaryMailTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/activation_mail/ActivationMailRecipientDTO.java
- src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/WeeklySummaryMailTest.java
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/saml2_set_password_mail/SAML2SetPasswordMailRecipientDTO.java
- src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/notifications/NotificationMailRecipientDTO.java
- src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java
🧰 Additional context used
📓 Path-based instructions (2)
`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.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.javasrc/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.javasrc/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.javasrc/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.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-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java (1)
1-11: LGTM! Well-structured test class setup.The class follows the coding guidelines with proper:
- Use of AssertJ's assertThat style assertions
- Clear and descriptive test class naming
- Appropriate test inheritance hierarchy
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (1)
18-20: Update test to use ActivationMailRecipientDTO.The test currently uses the User entity directly, which doesn't align with the PR's objective of using DTOs for mail services.
- User recipient = createMinimalMailRecipientUser(); - recipient.setLogin("test_student"); - recipient.setActivationKey("test_key"); + ActivationMailRecipientDTO recipient = new ActivationMailRecipientDTO( + "test_student", + "test_key", + "en", + "[email protected]" + );src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (1)
1-12: LGTM! Well-structured DTO declaration.The record follows Java naming conventions, implements the appropriate interface, and uses specific imports. The @JsonInclude annotation helps minimize the JSON payload.
...java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java
Show resolved
Hide resolved
...java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
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. Create a Text Exercise.
3. Wait a few minutes.
4. Verify that you receive an "Exercise Released" email notification.
Test Coverage
Summary by CodeRabbit