Development: Restructure non-notification mails to use data transfer objects
#9858
Conversation
… mail content generated from thymeleaf templates
…adjust thymeleaf templates and add tests
github-actions
bot
added
tests
server
…nd illegal annotation
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Outdated
Show resolved
Hide resolved
WalkthroughThe pull request introduces significant changes to the email handling system within the application. Key modifications include the refactoring of the Changes
Possibly related PRs
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.6.0)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailSendingService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/IMailRecipientUserDTO.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
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}
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Outside diff range and nitpick comments (45)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/IMailRecipientUserDTO.java (2)
3-5: Enhance interface documentation.The current documentation could be more comprehensive to better explain:
- The purpose and responsibility of this DTO
- Contract requirements for implementations
- Nullability constraints
- Usage examples
Consider expanding the documentation:
/** - * Minimal information a recipient of an email needs to provide. + * Data Transfer Object (DTO) interface defining the minimal information required for email recipients. + * This interface is designed to prevent sensitive information leakage by exposing only necessary + * recipient details for email communication. + * + * Implementations must ensure that neither langKey nor email return null values. + * + * @see ActivationMailRecipientDTO + * @see DataExportFailedMailAdminRecipientDTO */
6-6: Consider removing the 'I' prefix from the interface name.While prefixing interfaces with 'I' is a convention in some languages (e.g., C#), it's not a common practice in modern Java. The name should focus on what the interface represents rather than that it's an interface.
Consider renaming to
MailRecipientUserDTO:-public interface IMailRecipientUserDTO { +public interface MailRecipientUserDTO {src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentDTO.java (1)
5-9: Consider enhancing the documentation.While the current documentation is clear, it could be more descriptive about:
- The DTO's role in the mail service
- When this DTO is used in the workflow
- The relationship with the mail template
/** * DTO for the content of a data export successful mail. + * This DTO encapsulates the necessary information for rendering the data export + * successful notification email template. It is used by the MailService when + * notifying administrators about successful data exports. * * @param userLogin the login of the user who requested the data export */src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java (2)
6-8: Enhance class documentationWhile the current documentation provides a basic description, it would be beneficial to include:
- Purpose and use cases of this DTO
- Description of each field
- Example usage with the factory method
/** * DTO for the weekly summary mail recipient. + * + * This DTO encapsulates the essential user information needed for sending weekly summary emails, + * preventing unnecessary exposure of sensitive user data. + * + * @param langKey The language preference key of the recipient + * @param email The email address of the recipient + * @param name The display name of the recipient + * + * @see IMailRecipientUserDTO */
9-9: Consider adding validation constraintsThe record fields, especially the email, could benefit from validation annotations to ensure data integrity.
+import jakarta.validation.constraints.Email; +import jakarta.validation.constraints.NotNull; -public record WeeklySummaryMailRecipientDTO(String langKey, String email, String name) implements IMailRecipientUserDTO { +public record WeeklySummaryMailRecipientDTO( + @NotNull String langKey, + @NotNull @Email String email, + @NotNull String name) implements IMailRecipientUserDTO {src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/notifications/NotificationMailRecipientDTO.java (1)
6-8: Enhance documentation for better clarity.Consider expanding the documentation to include:
- Purpose and usage of each field
- Relationship with
IMailRecipientUserDTO- Typical usage scenarios
/** - * DTO for the recipient of a notification mail. + * Data Transfer Object (DTO) representing a recipient of a notification mail. + * This record implements IMailRecipientUserDTO to provide essential user information + * required for sending notification emails. + * + * @param langKey The language key for localization + * @param email The recipient's email address + * @param name The recipient's display name + * @param login The recipient's login identifier */src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (1)
6-9: Enhance class documentation for better maintainability.While the implementation is correct, consider enhancing the documentation to improve maintainability:
/** * DTO for the password reset mail recipient + * + * @param langKey The language key for localization + * @param email The recipient's email address + * @param login The user's login identifier + * @param resetKey The password reset token + * @since 5.0.0 */src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/activation_mail/ActivationMailRecipientDTO.java (2)
6-8: Enhance documentation for better clarity and maintainability.The current documentation is minimal. Consider adding:
- Purpose and usage context of this DTO
- Description of each field and its constraints
- Documentation for the factory method including any preconditions
/** - * DTO for the recipient of the activation mail + * Data Transfer Object (DTO) representing a recipient of an activation email. + * This DTO encapsulates the minimal required information needed for sending activation emails, + * preventing exposure of sensitive user information. + * + * @param langKey The language key for email localization + * @param email The recipient's email address + * @param login The user's login identifier + * @param activationKey The key required for account activation */
1-14: Well-structured DTO implementation aligned with PR objectives.The implementation successfully:
- Encapsulates only necessary recipient data
- Prevents sensitive information leakage
- Uses modern Java features (records)
- Follows single responsibility principle
Consider documenting the rationale for choosing these specific fields in a separate architecture decision record (ADR) to help maintain the design decisions for future reference.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulMailAdminRecipientDTO.java (2)
11-13: Consider making the factory method package-private.Following the principle of least access, consider restricting the visibility of the factory method if it's only used within the same package. This helps prevent unintended usage from other packages and maintains better encapsulation.
- public static DataExportSuccessfulMailAdminRecipientDTO of(User user) { + static DataExportSuccessfulMailAdminRecipientDTO of(User user) {
6-8: Enhance class documentation.While the current documentation is clear, it would be beneficial to add more details about the purpose and usage of this DTO, particularly its role in preventing sensitive information leakage.
/** * DTO for the admin recipient of the data export successful mail notification. + * This DTO ensures that only necessary user information (language key and email) + * is passed to the mail service, preventing sensitive user data leakage. */src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedMailAdminRecipientDTO.java (2)
6-8: Consider enhancing the documentation.While the current documentation is clear, it could be more comprehensive by including:
- The purpose and context of this DTO in data export failure scenarios
- Description of the fields (langKey, email, login) and their significance
- Usage examples or scenarios
/** - * DTO for the recipient of the data export failed mail for the admin + * Data Transfer Object representing an admin recipient for data export failure notifications. + * + * This DTO encapsulates the minimal required information needed to send notifications + * to administrators when a data export operation fails: + * - langKey: The administrator's preferred language for the notification + * - email: The administrator's email address for receiving the notification + * - login: The administrator's login identifier + * + * @see IMailRecipientUserDTO */
11-13: Consider adding null checks and reducing access level.The factory method could be more robust with the following improvements:
- Add null validation to prevent NullPointerException
- Consider making the method package-private if it's only used within the package
- public static DataExportFailedMailAdminRecipientDTO of(User user) { + static DataExportFailedMailAdminRecipientDTO of(User user) { + if (user == null) { + throw new IllegalArgumentException("User cannot be null"); + } return new DataExportFailedMailAdminRecipientDTO(user.getLangKey(), user.getEmail(), user.getLogin()); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/saml2_set_password_mail/SAML2SetPasswordMailRecipientDTO.java (3)
6-8: Enhance class documentation for better clarity.Consider adding more comprehensive documentation that includes:
- The purpose and role of this DTO in the SAML2 password reset flow
- Description of each field and its significance
- Usage context or examples
/** - * DTO for the recipient of the SAML2SetPasswordMail + * Data Transfer Object representing a recipient for SAML2 password reset emails. + * + * @param langKey The language key for email localization + * @param email The recipient's email address + * @param login The recipient's login username + * @param resetKey The password reset key for verification + * + * This DTO encapsulates only the necessary user information required for sending + * SAML2 password reset emails, preventing exposure of sensitive user data. */
9-9: Consider adding field validation.While the record implementation is clean and follows best practices, consider adding validation for the fields, especially for
resetKeywhich are critical for the password reset functionality.public record SAML2SetPasswordMailRecipientDTO(String langKey, String email, String login, String resetKey) implements IMailRecipientUserDTO { + public SAML2SetPasswordMailRecipientDTO { + if (email == null || email.isBlank()) { + throw new IllegalArgumentException("Email cannot be null or blank"); + } + if (resetKey == null || resetKey.isBlank()) { + throw new IllegalArgumentException("Reset key cannot be null or blank"); + } + }
1-14: Implementation aligns well with PR objectives.The DTO implementation successfully achieves the goal of preventing sensitive information leakage by:
- Encapsulating only the necessary data for SAML2 password reset emails
- Using immutable records for data transfer
- Providing a clear interface for mail recipient data
Consider creating a common base class or utility for validation logic as more mail-specific DTOs are added to maintain consistency across the codebase.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java (1)
5-10: Consider enhancing the documentation.While the current documentation is clear, consider adding:
@sincetag to track when this DTO was introduced- A brief usage example in the class-level documentation
/** * DTO for the content of a data export failed mail + * + * @since 5.0.0 + * + * Example usage: + * <pre> + * var dto = DataExportFailedContentDTO.of(exception, dataExport); + * mailService.sendDataExportFailedEmail(dto); + * </pre> * * @param reason The reason why the data export failed * @param exportUsername The username of the user who initiated the data export */src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentsDTO.java (2)
8-11: Consider enhancing the documentation.While the documentation is clear, it could be more comprehensive by including:
- @param description for the contents field
- Usage examples or context
- Relationship with DataExportSuccessfulContentDTO
/** * DTO for the contents of a data export successful mail notification. + * + * @param contents Set of individual data export content DTOs representing successful exports + * @see DataExportSuccessfulContentDTO + * @see DataExport */
13-16: Consider adding input validation and defensive copying.While the implementation is clean and efficient, consider these safety improvements:
public static DataExportSuccessfulContentsDTO of(Set<DataExport> exportSet) { + Objects.requireNonNull(exportSet, "exportSet must not be null"); Set<DataExportSuccessfulContentDTO> contents = exportSet.stream() .map(DataExportSuccessfulContentDTO::of) - .collect(Collectors.toSet()); + .collect(Collectors.toUnmodifiableSet()); return new DataExportSuccessfulContentsDTO(contents); }This change:
- Adds null check for the input parameter
- Returns an unmodifiable set to prevent external modifications
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (2)
11-14: Enhance test documentation for better clarity.While the current documentation describes the basic purpose, it could be more comprehensive by including:
- Expected behavior of the template injection
- Preconditions and test setup details
- Specific verification steps
Consider expanding the documentation like this:
/** - * Test that the variables injected in the template are used in the generated HTML content. + * Verifies that user-specific variables (login and activation key) are correctly injected + * into the activation email template and appear in the generated HTML content. + * + * Test steps: + * 1. Creates a minimal user with specific login and activation key + * 2. Triggers activation email sending + * 3. Verifies that the generated content contains the injected values */
15-28: Strengthen test assertions and consider edge cases.The test follows good practices with AAA pattern and fixed test data. However, consider these improvements:
- Make assertions more specific about where these values should appear in the template
- Add edge cases for special characters in login/activation key
- Verify the complete email structure (subject, recipients, etc.)
Consider enhancing the test like this:
@Test void testThatVariablesAreInjectedIntoTheTemplate() { // Arrange: User recipient = createMinimalMailRecipientUser(); recipient.setLogin("test_student"); recipient.setActivationKey("test_key"); // Act: mailService.sendActivationEmail(recipient); // Assert: String capturedContent = getGeneratedEmailTemplateText(); - assertThat(capturedContent).contains("test_student"); - assertThat(capturedContent).contains("test_key"); + assertThat(capturedContent) + .contains("<span>" + recipient.getLogin() + "</span>") + .contains("activation-key=" + recipient.getActivationKey()); } + + @Test + void testActivationEmailWithSpecialCharacters() { + // Test with special characters in login/key + User recipient = createMinimalMailRecipientUser(); + recipient.setLogin("test@student"); + recipient.setActivationKey("test/key+special"); + + mailService.sendActivationEmail(recipient); + + String capturedContent = getGeneratedEmailTemplateText(); + assertThat(capturedContent) + .contains(recipient.getLogin()) + .contains(recipient.getActivationKey()); + }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java (1)
24-28: Enhance test coverage with additional assertionsWhile the current assertions verify the presence of login and reset key, consider adding these improvements:
- Verify the correct email template is being used
- Assert that sensitive User information is not leaked in the email content
Add these assertions:
String capturedContent = getGeneratedEmailTemplateText(); assertThat(capturedContent).contains("test_login"); assertThat(capturedContent).contains("test_reset_key"); +assertThat(getTemplateName()).isEqualTo("passwordReset"); +// Verify no sensitive information is leaked +assertThat(capturedContent).doesNotContain("password", "authorities", "groups");src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.java (2)
11-14: Enhance test documentation and coverageThe test documentation could be more specific about:
- The expected template being used
- The specific HTML elements being verified
- Any security considerations for the reset key in the email content
Consider expanding the documentation:
/** * Test that the variables injected in the template are used in the generated HTML content. + * Template: saml2-set-password.html + * Verifies: + * - User login is properly displayed in the email + * - Reset key is securely embedded in the reset link */
24-28: Add template path verification and HTML structure assertionsThe test should verify that the correct template is being used and that the variables are inserted in the proper HTML structure.
Consider adding these assertions:
// Assert: String capturedContent = getGeneratedEmailTemplateText(); + assertThat(getUsedTemplatePath()) + .as("Should use the SAML2 set password template") + .isEqualTo("mail/saml2-set-password"); + + // Verify proper HTML structure + assertThat(capturedContent) + .contains("<html", "</html>") + .matches(".*<a[^>]*href=\"[^\"]*test_reset_key[^\"]*\".*"); + assertThat(capturedContent).contains("test_login"); assertThat(capturedContent).contains("test_reset_key");src/main/resources/templates/mail/dataExportFailedAdminEmail.html (1)
Line range hint
4-4: Update title attribute to use DTOThe title attribute still uses
${user.login}while the body has been updated to use the DTO. This inconsistency should be fixed to maintain the PR's objective of preventing direct User object exposure.Apply this diff to fix the inconsistency:
- <title th:text="#{email.dataExportFailedAdmin.title(${user.login})}">Data export for one of the users failed</title> + <title th:text="#{email.dataExportFailedAdmin.title(${dataExportFailedContent.exportUsername})}">Data export for one of the users failed</title>src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java (4)
15-16: Add @DisplayName for better test reporting.Consider adding a @DisplayName annotation to provide a more readable description in test reports.
@Test +@DisplayName("Should inject all variables into the data export failed email template") void testThatVariablesAreInjectedIntoTheTemplate() {
21-22: Fix typo in variable name.The variable name contains a typo: 'dataExporSubjecttUser' should be 'dataExportSubjectUser'.
- User dataExporSubjecttUser = new User(); - dataExporSubjecttUser.setLogin("test_subject"); + User dataExportSubjectUser = new User(); + dataExportSubjectUser.setLogin("test_subject");
29-36: Enhance test verification.The test should verify that the email was actually sent and validate the complete email context, not just the content.
Consider adding these verifications:
// Act: mailService.sendDataExportFailedEmailToAdmin(recipient, dataExport, reason); // Assert: String capturedContent = getGeneratedEmailTemplateText(); assertThat(capturedContent).contains("test_login"); assertThat(capturedContent).contains("test_subject"); assertThat(capturedContent).contains("test_reason"); + // Verify email was sent + verify(mailService).sendEmail( + argThat(dto -> dto.getEmail().equals(recipient.getEmail())), + any(), + any() + );
17-28: Consider using test utilities for data creation.The test data creation could be more robust and maintainable using dedicated test utilities or builder patterns.
Consider refactoring to use test utilities:
// Arrange: - User recipient = createMinimalMailRecipientUser(); - recipient.setLogin("test_login"); - - User dataExporSubjecttUser = new User(); - dataExporSubjecttUser.setLogin("test_subject"); - - DataExport dataExport = new DataExport(); - dataExport.setUser(dataExporSubjecttUser); - - Exception reason = new Exception("test_reason"); + User recipient = UserTestBuilder.withLogin("test_login").build(); + User dataExportSubjectUser = UserTestBuilder.withLogin("test_subject").build(); + DataExport dataExport = DataExportTestBuilder + .withUser(dataExportSubjectUser) + .build(); + Exception reason = new TestException("test_reason");src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailContentDTO.java (1)
9-15: Consider adding null validation for exerciseSummaries.While the record implementation is clean and follows DTO guidelines, consider adding validation to ensure
exerciseSummariesis not null. This can be done through a compact constructor.public record WeeklySummaryMailContentDTO(boolean hasNewExercises, Set<WeeklyExerciseSummaryDTO> exerciseSummaries) { + public WeeklySummaryMailContentDTO { + exerciseSummaries = exerciseSummaries == null ? Set.of() : exerciseSummaries; + }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java (2)
18-33: Enhance test coverage with additional scenarios.The current test only verifies the happy path. Consider adding test cases for:
- Empty data exports set
- Single data export
- Data exports with null user references
12-51: Enhance test class with JUnit 5 features.Consider utilizing more JUnit 5 features to improve test organization and readability:
- Add
@DisplayNamefor better test descriptions- Use
@Nestedclasses to group related test scenarios- Consider using
@ParameterizedTestfor testing multiple data exports scenariosExample:
@DisplayName("Data Export Successful Mail Tests") class DataExportSuccessfulMailTest extends AbstractMailContentTest { @Nested @DisplayName("Template variable injection") class TemplateVariableInjection { @Test @DisplayName("should include all user logins in generated content") void testThatVariablesAreInjectedIntoTheTemplate() { // existing test code } } }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklyExerciseSummaryDTO.java (2)
17-17: Consider adding input validation for the record fields.While the record structure is clean and follows the DTO guidelines, consider adding validation to ensure data integrity:
- Prevent negative values for
maxPointsandbonusPoints- Ensure
titleandtypeare not nullYou could implement validation using a custom constructor:
public record WeeklyExerciseSummaryDTO(String title, String type, String difficulty, String releaseDate, String dueDate, double maxPoints, double bonusPoints) { + public WeeklyExerciseSummaryDTO { + if (title == null || type == null) { + throw new IllegalArgumentException("Title and type must not be null"); + } + if (maxPoints < 0 || bonusPoints < 0) { + throw new IllegalArgumentException("Points must not be negative"); + } + }
1-40: Well-structured DTO implementation with clear separation of concerns.The implementation follows good architectural practices:
- Uses Java record for immutable data transfer
- Provides a clear factory method for object creation
- Maintains separation of concerns by focusing solely on exercise summary data
- Follows single responsibility principle
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.java (2)
20-22: Enhance class documentationWhile the documentation describes the purpose, it could be more comprehensive. Consider adding:
- Examples of how to extend this class
- Description of the key dependencies
- Common usage patterns
/** - * Abstract class for testing mail content and the correct usage of DTO attributes in the mail content. + * Abstract base class for testing email template content and DTO attribute integration. + * + * This class provides the foundation for testing email templates with Thymeleaf and verifying + * that DTOs are correctly used in the generated content. It handles common setup like mocking + * the MailSendingService and providing utility methods for content verification. + * + * @see MailService + * @see MailSendingService */
39-42: Add null checks for required dependenciesWhile the setup is clean, consider adding null checks to fail fast if any required dependency is missing.
@BeforeEach void setup() { + if (messageSource == null || templateEngine == null || timeService == null || mailSendingService == null) { + throw new IllegalStateException("Required dependencies not initialized"); + } mailService = new MailService(messageSource, templateEngine, timeService, mailSendingService); }src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/WeeklySummaryMailTest.java (2)
62-73: Enhance assertion specificity and test data organizationThe assertions could be more specific and the test data creation could be optimized:
- The difficulty level assertion
containsAnyOf("Schwer", "Mittel", "Leicht")doesn't verify that each exercise has the correct difficulty level.- The exercise creation logic could be extracted to improve readability.
Consider these improvements:
+ private Exercise createExercise(String title, DifficultyLevel difficulty, + ZonedDateTime releaseDate, ZonedDateTime dueDate, + Double bonusPoints, Double maxPoints, Class<? extends Exercise> exerciseClass) { + Exercise exercise = exerciseClass.getDeclaredConstructor().newInstance(); + exercise.setTitle(title); + exercise.setDifficulty(difficulty); + exercise.setReleaseDate(releaseDate); + exercise.setDueDate(dueDate); + exercise.setBonusPoints(bonusPoints); + exercise.setMaxPoints(maxPoints); + return exercise; + } // In test method: - assertThat(capturedContent).containsAnyOf("Schwer", "Mittel", "Leicht"); + // Verify specific difficulty for each exercise + switch(e.getDifficulty()) { + case EASY -> assertThat(capturedContent).contains("Leicht"); + case MEDIUM -> assertThat(capturedContent).contains("Mittel"); + case HARD -> assertThat(capturedContent).contains("Schwer"); + }
31-54: Consider using test data builders for exercise creationThe current approach of creating and configuring exercises is verbose and repetitive. Consider implementing a builder pattern for test data creation to improve maintainability and readability.
Example implementation:
@Builder public class ExerciseTestDataBuilder { private String title; private DifficultyLevel difficulty; private ZonedDateTime releaseDate; private ZonedDateTime dueDate; private Double bonusPoints; private Double maxPoints; private Class<? extends Exercise> exerciseClass; public Exercise build() { Exercise exercise = exerciseClass.getDeclaredConstructor().newInstance(); exercise.setTitle(title); exercise.setDifficulty(difficulty); exercise.setReleaseDate(releaseDate); exercise.setDueDate(dueDate); exercise.setBonusPoints(bonusPoints); exercise.setMaxPoints(maxPoints); return exercise; } } // Usage in test: Exercise easyExercise = ExerciseTestDataBuilder.builder() .title("easy_exercise") .difficulty(DifficultyLevel.EASY) .releaseDate(releaseDateTime) .dueDate(dueDateTime) .bonusPoints(0.0) .maxPoints(101.0) .exerciseClass(TextExercise.class) .build();src/main/resources/templates/mail/weeklySummary.html (2)
27-27: Track the TODO comment dependency.The comment indicates a pending refactoring that depends on another PR. This should be tracked to ensure it's not forgotten.
Would you like me to create a GitHub issue to track this TODO and link it to the other PR?
Line range hint
30-42: Consider simplifying difficulty display logic.The nested switch statement for difficulty levels, while functional, could be simplified using a message key pattern.
Consider refactoring to use a dynamic message key:
- <th:block th:switch="${exerciseSummary.difficulty}"> - <th:block th:case="'EASY'"> - <span th:utext="#{email.notification.title.exercise.information.difficulty(#{email.notification.aux.difficulty.easy})} "/> - </th:block> - <th:block th:case="'MEDIUM'"> - <span th:utext="#{email.notification.title.exercise.information.difficulty(#{email.notification.aux.difficulty.medium})} "/> - </th:block> - <th:block th:case="'HARD'"> - <span th:utext="#{email.notification.title.exercise.information.difficulty(#{email.notification.aux.difficulty.hard})} "/> - </th:block> - </th:block> + <span th:with="difficultyKey='email.notification.aux.difficulty.' + ${#strings.toLowerCase(exerciseSummary.difficulty)}" + th:utext="#{email.notification.title.exercise.information.difficulty(#{__${difficultyKey}__})}"/>src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java (2)
128-128: Consider adding assertion for DTO conversion.While the test correctly uses the new DTO, it might be valuable to verify that the DTO properly carries the essential user information needed for email sending.
Consider adding:
+ var recipientDto = ActivationMailRecipientDTO.of(student1); + assertThat(recipientDto.getEmail()).isEqualTo(student1.getEmail()); + assertThat(recipientDto.getLangKey()).isEqualTo(student1.getLangKey()); mailSendingService.sendEmail(ActivationMailRecipientDTO.of(student1), subject, content, false, true);
Line range hint
127-139: Consider enhancing test coverage and descriptiveness.While the tests are well-structured, consider these improvements to better align with our testing guidelines:
- Make test names more descriptive:
- void testSendEmail() + void shouldSuccessfullySendEmailWhenUsingValidDTO() - void testNoMailSendExceptionThrown() + void shouldSuppressMailSendExceptionWhenSendingFails()
- Add parameterized tests for different scenarios:
@ParameterizedTest @MethodSource("provideUserScenarios") void shouldHandleVariousUserScenarios(User user, boolean expectedSuccess) { // Test implementation } static Stream<Arguments> provideUserScenarios() { return Stream.of( Arguments.of(userWithAllFields(), true), Arguments.of(userWithoutLangKey(), true), Arguments.of(userWithoutEmail(), false) ); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (3)
424-424: Externalize the email subject for localization supportThe subject
"Weekly Summary"is hardcoded, which doesn't support localization. Consider externalizing it usingmessageSourceto facilitate translations.Apply this change to use localized messages:
- String subject = "Weekly Summary"; + String subject = messageSource.getMessage("email.weeklySummary.title", null, context.getLocale());Don't forget to add the key
email.weeklySummary.titleto your localization files.
185-186: Ensure consistency in parameter usageThe parameters passed to
sendDataExportFailedEmailForAdminmix DTOs and domain objects. For consistency and clarity, consider convertingDataExportandExceptionto a dedicated DTO before passing them.Refactor the method call:
- sendDataExportFailedEmailForAdmin(DataExportFailedMailAdminRecipientDTO.of(admin), "mail/dataExportFailedAdminEmail", "email.dataExportFailedAdmin.title", - DataExportFailedContentDTO.of(reason, dataExport)); + DataExportFailedContentDTO exportFailContentDTO = DataExportFailedContentDTO.of(reason, dataExport); + sendDataExportFailedEmailForAdmin(DataExportFailedMailAdminRecipientDTO.of(admin), "mail/dataExportFailedAdminEmail", "email.dataExportFailedAdmin.title", + exportFailContentDTO);This enhances readability and aligns with the use of DTOs throughout the service.
170-180: Consider refactoring to accept DTOs directlyMethods like
sendActivationEmail,sendPasswordResetMail, andsendSAML2SetPasswordMailacceptUserobjects but immediately convert them to DTOs. For consistency and to follow the single-responsibility principle, consider accepting the DTOs directly.Refactor the method signatures:
- public void sendActivationEmail(User user) { + public void sendActivationEmail(ActivationMailRecipientDTO recipient) { log.debug("Sending activation email to '{}'", recipient.getEmail()); sendEmailFromTemplate(recipient, "mail/activationEmail", "email.activation.title"); } - public void sendPasswordResetMail(User user) { + public void sendPasswordResetMail(PasswordResetRecipientDTO recipient) { log.debug("Sending password reset email to '{}'", recipient.getEmail()); sendEmailFromTemplate(recipient, "mail/passwordResetEmail", "email.reset.title"); } - public void sendSAML2SetPasswordMail(User user) { + public void sendSAML2SetPasswordMail(SAML2SetPasswordMailRecipientDTO recipient) { log.debug("Sending SAML2 set password email to '{}'", recipient.getEmail()); sendEmailFromTemplate(recipient, "mail/samlSetPasswordEmail", "email.saml.title"); }This change promotes consistency and separates concerns more effectively.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (28)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailSendingService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/IMailRecipientUserDTO.java(1 hunks)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/data_export_failed_mail/DataExportFailedMailAdminRecipientDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulMailAdminRecipientDTO.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/WeeklyExerciseSummaryDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailContentDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java(1 hunks)src/main/resources/templates/mail/dataExportFailedAdminEmail.html(1 hunks)src/main/resources/templates/mail/notification/fragments.html(1 hunks)src/main/resources/templates/mail/successfulDataExportsAdminEmail.html(1 hunks)src/main/resources/templates/mail/weeklySummary.html(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java(3 hunks)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)src/test/java/de/tum/cit/aet/artemis/core/service/DataExportScheduleServiceTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (24)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailSendingService.java (1)
Pattern 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/MailService.java (1)
Pattern 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/IMailRecipientUserDTO.java (1)
Pattern 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/activation_mail/ActivationMailRecipientDTO.java (1)
Pattern 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/data_export_failed_mail/DataExportFailedContentDTO.java (1)
Pattern 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/data_export_failed_mail/DataExportFailedMailAdminRecipientDTO.java (1)
Pattern 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/data_export_successful_mail/DataExportSuccessfulContentDTO.java (1)
Pattern 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/data_export_successful_mail/DataExportSuccessfulContentsDTO.java (1)
Pattern 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/data_export_successful_mail/DataExportSuccessfulMailAdminRecipientDTO.java (1)
Pattern 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/notifications/NotificationMailRecipientDTO.java (1)
Pattern 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 (1)
Pattern 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/saml2_set_password_mail/SAML2SetPasswordMailRecipientDTO.java (1)
Pattern 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/weekly_summary_mail/WeeklyExerciseSummaryDTO.java (1)
Pattern 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/weekly_summary_mail/WeeklySummaryMailContentDTO.java (1)
Pattern 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/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java (1)
Pattern 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/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java (1)
Pattern 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/AbstractMailContentTest.java (1)
Pattern 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.java (1)
Pattern 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/DataExportFailedMailTest.java (1)
Pattern 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/DataExportSuccessfulMailTest.java (1)
Pattern 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.java (1)
Pattern 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/SAML2SetPasswordMailTest.java (1)
Pattern 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/WeeklySummaryMailTest.java (1)
Pattern 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/core/service/DataExportScheduleServiceTest.java (1)
Pattern 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
🔇 Additional comments (39)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentDTO.java (2)
1-4: LGTM! Clean package structure and imports.
The package structure follows domain-driven design principles, and imports are specific without using wildcards.
10-10: LGTM! Well-structured DTO declaration.
The record declaration follows best practices:
- Uses Java record for immutable data transfer
- Contains only the essential data needed for the use case
- Clear and descriptive field naming
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java (1)
1-14: LGTM! The overall structure follows best practices
The implementation:
- Uses Java records appropriately for DTOs
- Follows single responsibility principle
- Minimizes data exposure
- Uses proper naming conventions
- Avoids star imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/notifications/NotificationMailRecipientDTO.java (2)
1-5: LGTM! Clean and minimal imports.
The imports are specific and well-organized, following Java best practices by avoiding star imports.
9-9: Verify field requirements and consider field ordering.
The record implementation is clean, but consider:
- Reordering fields to group interface-required fields first (
langKey,email). - Verifying if all fields are necessary for notification emails to maintain minimal DTO principle.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the field requirements and ordering in NotificationMailRecipientDTO.
Field order and requirements are appropriate for notification emails.
The current field order (langKey, email, name, login) is already optimal because:
- The interface
IMailRecipientUserDTOonly requireslangKeyandemailfields, which are already placed first - All fields are actively used in the codebase:
langKeyandemailare required by the interface for mail deliverynameandloginare used for user identification in notifications, as seen in theof(User user)factory method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of notification recipient fields in templates
rg -A 5 "notification.*\.(name|login)" "src/main/resources/templates/"
# Search for interface definition to understand required fields
ast-grep --pattern 'interface IMailRecipientUserDTO {
$$$
}'
Length of output: 259
Script:
#!/bin/bash
# Search for interface definition with a more specific path
fd -e java "IMailRecipientUserDTO" --exec ast-grep --pattern 'interface IMailRecipientUserDTO {
$$$
}'
# Search for usage of notification recipient fields in mail templates with .html extension
fd -e html . src/main/resources/templates/ --exec rg -l "notification.*\.(name|login|email|langKey)" {}
# Search for usages of NotificationMailRecipientDTO to understand field requirements
rg -A 5 "NotificationMailRecipientDTO"
Length of output: 3355
Script:
#!/bin/bash
# Search for IMailRecipientUserDTO interface in a different way
rg -A 10 "interface IMailRecipientUserDTO"
# Look for other DTOs implementing the same interface to understand common patterns
ast-grep --pattern 'record $_ implements IMailRecipientUserDTO'
# Search for User class to understand field usage
rg -A 5 "getLangKey|getEmail|getName|getLogin" --type java
Length of output: 133092
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/password_reset_mail/PasswordResetRecipientDTO.java (1)
1-5: LGTM! Clean package structure and imports.
The package naming and import statements follow Java best practices with no unnecessary imports.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulMailAdminRecipientDTO.java (2)
9-9: LGTM! Good use of Java record for DTO implementation.
The record implementation aligns well with the coding guidelines, providing an immutable data carrier with minimal boilerplate.
1-14: Verify consistent DTO pattern usage across mail services.
Let's ensure this DTO implementation pattern is consistently followed across other mail notification DTOs.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the verification is complete. The results show consistent implementation patterns across all mail DTOs:
DTO implementation follows consistent patterns across mail services
The implementation is verified to follow the established patterns across the codebase:
- All mail recipient DTOs consistently implement
IMailRecipientUserDTO - Factory method pattern
of(User user)is consistently used across all DTOs:- ActivationMailRecipientDTO
- DataExportFailedMailAdminRecipientDTO
- DataExportSuccessfulMailAdminRecipientDTO
- NotificationMailRecipientDTO
- PasswordResetRecipientDTO
- SAML2SetPasswordMailRecipientDTO
- WeeklySummaryMailRecipientDTO
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation patterns across mail DTOs
# Check for other DTOs implementing IMailRecipientUserDTO
echo "Checking for other mail recipient DTOs:"
ast-grep --pattern 'record $RECORD_NAME($_) implements IMailRecipientUserDTO { $$$ }'
# Check for consistent factory method pattern
echo -e "\nChecking for consistent factory method pattern:"
ast-grep --pattern 'public static $RECORD_NAME of(User user) { $$$ }'
# Check for potential direct User object usage in mail services
echo -e "\nChecking for potential direct User object usage in mail services:"
rg "User\s+\w+\s*[,\)]" --type java src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/
Length of output: 7023
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedMailAdminRecipientDTO.java (2)
1-5: LGTM! Package and imports are well-structured.
The package naming follows conventions, and imports are specific without any wildcards.
9-9: LGTM! Record declaration follows DTO best practices.
The implementation:
- Uses Java record as recommended
- Contains only the necessary fields
- Follows single responsibility principle
- Properly implements the IMailRecipientUserDTO interface
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/saml2_set_password_mail/SAML2SetPasswordMailRecipientDTO.java (1)
1-5: LGTM! Package structure and imports are well-organized.
The package naming and import statements follow best practices with specific imports and clear organization.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_failed_mail/DataExportFailedContentDTO.java (2)
1-4: LGTM! Package and imports are well-structured.
The package naming and import statement follow best practices and coding guidelines.
11-11: LGTM! Record declaration follows DTO best practices.
The use of a Java record with minimal fields aligns perfectly with the coding guidelines for DTOs.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentsDTO.java (2)
1-7: LGTM! Clean and well-organized imports.
Package structure and imports follow best practices with specific imports and clear organization.
11-11: Verify data encapsulation in DataExportSuccessfulContentDTO.
To ensure alignment with PR objectives regarding sensitive information protection:
✅ Verification successful
Let me gather more information about the DataExportSuccessfulContentDTO to verify its implementation.
DTO properly encapsulates user data with minimal exposure
The DataExportSuccessfulContentDTO implementation follows secure data handling practices by:
- Only exposing the user login, which is a non-sensitive identifier
- Using a record class for immutability
- Providing a controlled factory method that safely maps from the domain model
- Not exposing any sensitive information from the DataExport entity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check DataExportSuccessfulContentDTO implementation for proper data encapsulation
# Find and display the related DTO implementation
ast-grep --pattern 'record DataExportSuccessfulContentDTO {
$$$
}'
# Check for any potential sensitive fields in DataExport that might be exposed
rg -A 5 "class DataExport"
Length of output: 17127
Script:
#!/bin/bash
# Search for DataExportSuccessfulContentDTO implementation
rg -l "DataExportSuccessfulContentDTO"
# Display content of files containing DataExportSuccessfulContentDTO
fd "DataExportSuccessfulContentDTO.java" --exec cat {}
Length of output: 927
src/main/resources/templates/mail/successfulDataExportsAdminEmail.html (2)
13-14: LGTM! Good refactoring to use DTOs
The changes improve encapsulation by using DTOs instead of exposing the internal User object structure. The new property access pattern (dataExportContent.userLogin) is more direct and maintainable than the previous chained calls.
13-14: Verify template context setup
Let's verify that the template context is properly set up with the new DTO structure.
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/ActivationMailTest.java (2)
1-9: LGTM! Clean class structure with proper inheritance.
The class follows good practices with minimal imports and clear organization.
17-17: Verify the helper method implementation.
The test uses createMinimalMailRecipientUser() from the parent class. Let's ensure this helper method is properly implemented.
✅ Verification successful
Helper method is properly implemented in the parent class
The createMinimalMailRecipientUser() method is correctly implemented in AbstractMailContentTest.java (lines 47-51). It creates a minimal user with a German language key, which is used as a base setup across multiple mail-related test classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the helper method implementation
ast-grep --pattern 'protected User createMinimalMailRecipientUser() {
$$$
}'
# Alternatively, search for any occurrence of the method
rg -A 5 "createMinimalMailRecipientUser"
Length of output: 7035
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/PasswordResetMailTest.java (2)
1-9: LGTM! Class structure and imports are well organized.
The test class follows good practices with:
- Clear naming convention
- Appropriate test class inheritance
- Minimal necessary imports
17-17: Verify the implementation of createMinimalMailRecipientUser()
Let's ensure the helper method in the parent class properly creates a minimal user object without unnecessary data.
✅ Verification successful
Helper method implementation is minimal and appropriate
The createMinimalMailRecipientUser() method in AbstractMailContentTest creates a user with only the language key set to "de", which is indeed a minimal implementation suitable for mail-related tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of createMinimalMailRecipientUser
ast-grep --pattern 'protected $_ createMinimalMailRecipientUser() {
$$$
}'
Length of output: 911
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/SAML2SetPasswordMailTest.java (1)
1-9: Verify AbstractMailContentTest configuration for DTO testing
Since this PR is part of a transition to DTOs, we should ensure that the parent class AbstractMailContentTest is properly configured to handle both entity and DTO-based mail testing.
src/main/resources/templates/mail/dataExportFailedAdminEmail.html (1)
13-16: LGTM! Proper use of DTOs for data transfer
The changes properly implement the DTO pattern by:
- Using
dataExportFailedContent.exportUsernameinstead of direct user access - Accessing the error reason through the DTO with
dataExportFailedContent.reason
These changes align well with the PR's objective of preventing sensitive information leakage and improving data transfer structure.
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportFailedMailTest.java (1)
1-10: LGTM! Clean class structure and imports.
The class follows proper naming conventions and has well-organized imports.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailContentDTO.java (1)
1-8: LGTM! Clean imports and package structure.
The imports are specific and minimal, following the Java guidelines.
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/DataExportSuccessfulMailTest.java (1)
1-12: LGTM! Class structure follows best practices.
The class follows proper test naming conventions and uses appropriate testing frameworks (JUnit 5, AssertJ).
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/mails/dto/weekly_summary_mail/WeeklyExerciseSummaryDTO.java (1)
1-5: LGTM! Clean imports and package structure.
The package naming and imports follow best practices with no wildcard imports.
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/mail/AbstractMailContentTest.java (1)
23-38: LGTM! Well-structured test class setup
The class follows best practices with:
- Proper use of static mocks
- Clear dependency injection
- Appropriate field visibility modifiers
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailSendingService.java (3)
19-19: LGTM! Import follows best practices.
The specific import of IMailRecipientUserDTO aligns with the coding guidelines and supports the DTO-based restructuring.
50-50: Verify all callers of sendEmail have been updated.
The signature change from User to IMailRecipientUserDTO looks good and aligns with the DTO restructuring goal.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the concern. All callers have been properly updated to use DTOs:
All callers of sendEmail have been properly updated to use DTOs
The verification shows that:
- Test cases use
ActivationMailRecipientDTO.of() MailServiceuses appropriate DTOs for different scenarios:NotificationMailRecipientDTO.of()WeeklySummaryMailRecipientDTO.of()
- No instances of direct
Userusage withsendEmailwere found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of sendEmail to ensure they've been updated to use DTOs
# Search for method calls to sendEmail
rg -A 2 "sendEmail\(" --type java
# Search for any remaining references to User in conjunction with sendEmail
rg -A 2 "sendEmail.*User" --type java
Length of output: 4596
57-57: Verify IMailRecipientUserDTO implementation.
The change to recipient.email() suggests the use of Java records. Let's verify the DTO implementation follows the guidelines.
✅ Verification successful
DTO implementation correctly follows Java record pattern
The verification confirms that:
- The
IMailRecipientUserDTOinterface is properly implemented by multiple record classes - All implementations use Java records syntax consistently
- The change from
getEmail()toemail()is correct as it aligns with Java record's canonical accessor pattern - Each implementation provides the required email field as part of the record's components
The codebase shows a well-structured approach using Java records for DTOs, making the change to recipient.email() both valid and consistent with the codebase's design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DTO implementation follows guidelines
# Find the DTO interface definition
ast-grep --pattern 'interface IMailRecipientUserDTO {
$$$
}'
# Find any implementations to verify they're using records
ast-grep --pattern 'record $_($$) implements IMailRecipientUserDTO {
$$$
}'
Length of output: 392
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find the DTO
# Find the interface definition
rg -l "IMailRecipientUserDTO"
# Once we find the files, we can examine their content
rg "IMailRecipientUserDTO" -A 5 -B 5
Length of output: 34375
src/main/resources/templates/mail/weeklySummary.html (3)
16-19: LGTM! Clean conditional logic using DTO.
The refactoring to use weeklySummaryContent.hasNewExercises instead of checking collection emptiness directly is a good practice. It encapsulates the logic within the DTO and maintains clean template code.
43-51: LGTM! Clean handling of optional fields.
The code properly handles optional fields with conditional blocks and uses appropriate message keys for internationalization. The DTO property access is clean and consistent.
Line range hint 1-67: Verify template variable injection.
Let's ensure that all required variables are properly injected into the template by the service layer.
✅ Verification successful
Let me gather more information about the template variable injection.
Let me gather more information about the WeeklySummaryContent class and its properties.
Let me try a different approach to find the relevant files and properties.
Based on the gathered information, I can now provide a final response about the template variable injection.
Template variables are properly injected and handled.
The code shows that all required variables are correctly injected into the template:
- The
MailServiceproperly sets up the context with:BASE_URLfor server URLUSERviaWeeklySummaryMailRecipientDTOWEEKLY_SUMMARY_CONTENTviaWeeklySummaryMailContentDTO
- The
WeeklySummaryMailContentDTOcontains all properties used in the template:hasNewExercisesfor conditional renderingexerciseSummariesfor exercise details
- Message properties are defined for all text content used in the template
- Unit tests verify the variable injection and content rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for template variable injection in the service layer
# Search for template name usage
rg -A 10 "weeklySummary" --type java
# Search for variables being set
ast-grep --pattern 'setVariable($name, $value)' --lang java
Length of output: 2259
Script:
#!/bin/bash
# Search for the context setup for weekly summary email
rg -B 5 -A 15 "weeklySummary.*context" --type java
# Search for WeeklySummaryContent class definition
ast-grep --pattern 'class WeeklySummaryContent {
$$$
}'
# Search for any other potential template variable setters
rg -B 2 -A 5 "Context.*set" --type java
Length of output: 41580
Script:
#!/bin/bash
# Search for WeeklySummaryContent class and its properties
rg -B 5 -A 15 "class.*WeeklySummaryContent" --type java
# Search for WeeklySummaryMailContentDTO
rg -B 5 -A 15 "class.*WeeklySummaryMailContentDTO" --type java
# Search for template message properties
rg "email\.weekly\.summary\." --type properties
Length of output: 207
Script:
#!/bin/bash
# Search for WeeklySummaryContent and related classes
rg -B 5 -A 15 "WeeklySummary" --type java
# Search for message properties files and their content
fd "messages.*properties" --type f --exec cat {} \;
# Search for template variables used in the HTML
rg "\${weeklySummaryContent\." --type html
Length of output: 59134
src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/MailServiceTest.java (2)
34-34: LGTM: Import addition is correct and well-placed.
The new import for ActivationMailRecipientDTO is appropriately placed and aligns with the refactoring to use DTOs instead of direct User objects.
138-138: LGTM: Exception handling test properly updated.
The test correctly verifies exception handling with the new DTO structure while maintaining clear assertions using the recommended assertThat style.
src/main/resources/templates/mail/notification/fragments.html (2)
193-193: LGTM! Change aligns with DTO transition.
The change from method call to property access aligns with the PR's objective of using DTOs instead of User entities. This helps prevent sensitive information leakage by explicitly exposing only the required properties through DTOs.
193-193: Verify template consistency across the codebase.
Let's ensure this property access pattern is consistently applied across all mail templates.
✅ Verification successful
Property access pattern is consistently applied across mail templates
The verification shows that property access patterns are consistently used throughout the mail templates. All user-related properties (user.name, user.login, user.email, user.resetKey, user.activationKey) follow the same direct property access pattern without method calls. There are no instances of getName() or similar method calls in any of the mail templates, confirming that the current change aligns with the existing codebase conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining method calls to getName() in mail templates
# and verify consistent property access pattern
# Search for getName() calls in mail templates
echo "Checking for getName() calls in mail templates..."
rg -g '*.html' 'user\.getName\(\)' 'src/main/resources/templates/mail'
# Search for user.name property access in mail templates
echo "Checking for user.name property access in mail templates..."
rg -g '*.html' 'user\.name' 'src/main/resources/templates/mail'
# Search for other potential User method calls that might need DTO properties
echo "Checking for other potential User method calls..."
rg -g '*.html' 'user\.[a-zA-Z]+\(\)' 'src/main/resources/templates/mail'
Length of output: 687
Script:
#!/bin/bash
# Let's check for all user-related references in mail templates to ensure consistency
echo "Checking for all user-related references in mail templates..."
rg -g '*.html' '\$\{user\.' 'src/main/resources/templates/mail'
# Also check for any other potential user references without ${} syntax
echo "Checking for other user references..."
rg -g '*.html' 'th:text=".*user' 'src/main/resources/templates/mail'
# Check for any other potential property access patterns in templates
echo "Checking for other property access patterns..."
rg -g '*.html' '\$\{[a-zA-Z]+\.[a-zA-Z]+\}' 'src/main/resources/templates/mail'
Length of output: 8441
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (1)
421-421:
Add null check for user.getLangKey() in locale creation
To ensure robustness, verify that user.getLangKey() is not null when creating the Locale. Default to "en" if necessary.
Here is the suggested fix:
- Locale locale = Locale.forLanguageTag(user.getLangKey());
+ String langKey = user.getLangKey() != null ? user.getLangKey() : "en";
+ Locale locale = Locale.forLanguageTag(langKey);Likely invalid or redundant comment.
...tum/cit/aet/artemis/communication/service/notifications/mails/dto/IMailRecipientUserDTO.java
Show resolved
Hide resolved
...tum/cit/aet/artemis/communication/service/notifications/mails/dto/IMailRecipientUserDTO.java
Show resolved
Hide resolved
...vice/notifications/mails/dto/data_export_successful_mail/DataExportSuccessfulContentDTO.java
Show resolved
Hide resolved
...ation/service/notifications/mails/dto/weekly_summary_mail/WeeklySummaryMailRecipientDTO.java
Show resolved
Hide resolved
...ommunication/service/notifications/mails/dto/notifications/NotificationMailRecipientDTO.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java
Show resolved
Hide resolved
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:
AbstractMailContentTestwhich allows testing HTML content generated by thymeleaf viagetGeneratedEmailTemplateTextfor the correct rendering of textIMailRecipientUserDTOwhich 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:
DataExportFailedContentDTOwhich contains only information that is relevant to the templateDataExportFailedMailAdminRecipientDTOand the accompanyingActivationMailTestand adjustedMailService::sendActivationEmailto use the DTOsMailService::sendDataExportFailedEmailForAdmindataExportFailedAdminEmail.html[Use case] Data export successful mail:
DataExportSuccessfulContentsDTOandDataExportSuccessfulContentDTOwhich 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.Steps for Testing [To be extended after code reviews]
Prerequisites:
FOREACH IN [
Activation mail,Password reset mail,SAML2 set password mail,Data export failed mail,Data export successful mail,Weekly summary mail,Some notification mail] DO: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
Summary by CodeRabbit
New Features
Bug Fixes
Tests