Notifications: Create services for course wide notification system
#10439
Conversation
…ons/create-datamodel-for-course-specific-notification
PaRangger
added
feature
tests
server
github-actions
bot
added
database
WalkthroughThis pull request integrates comprehensive course notification functionality into the application. It updates existing services to include unseen notification counts and introduces new dependencies for push notifications. Moreover, a complete set of domain entities, DTOs, annotations, repositories, and service classes are added to support course notifications via multiple channels (push, email, webapp). New REST controllers and a large suite of tests (unit, architecture, integration) are also included, as well as configuration updates such as repository scanning and cache management. Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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: 17
🔭 Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/FirebasePushNotificationService.java (2)
44-52: 🛠️ Refactor suggestionEnsure CompletableFuture error handling
The code creates CompletableFuture instances for batch processing but doesn't handle potential errors that might occur during asynchronous execution.
Add error handling to the CompletableFuture operations:
@Override void sendNotificationRequestsToEndpoint(List<RelayNotificationRequest> requests, String relayBaseUrl) { // The relay server accepts at most 500 messages per batch List<List<RelayNotificationRequest>> batches = Lists.partition(requests, 500); - var futures = batches.stream().map(batch -> CompletableFuture.runAsync(() -> sendSpecificNotificationRequestsToEndpoint(batch, relayBaseUrl))).toList() - .toArray(CompletableFuture[]::new); - - CompletableFuture.allOf(futures); + var futures = batches.stream() + .map(batch -> CompletableFuture.runAsync(() -> sendSpecificNotificationRequestsToEndpoint(batch, relayBaseUrl)) + .exceptionally(ex -> { + log.error("Failed to send batch notification", ex); + return null; + })) + .toList() + .toArray(CompletableFuture[]::new); + + CompletableFuture.allOf(futures).exceptionally(ex -> { + log.error("Error occurred while sending notifications", ex); + return null; + }); }
51-51:⚠️ Potential issueCompletableFuture result is not being waited for or processed
The call to
CompletableFuture.allOf(futures)creates a new CompletableFuture but doesn't join/get the result, effectively making this asynchronous operation fire-and-forget without any confirmation of completion.Ensure the CompletableFuture is properly handled:
- CompletableFuture.allOf(futures); + CompletableFuture.allOf(futures).join(); // Wait for all futures to completeOr for non-blocking behavior:
- CompletableFuture.allOf(futures); + CompletableFuture.allOf(futures) + .thenAccept(v -> log.debug("All notification batches sent successfully")) + .exceptionally(ex -> { + log.error("Error sending notification batches", ex); + return null; + });
🧹 Nitpick comments (47)
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/IgnoreUserCourseNotificationSettingPreset.java (2)
9-10: Consider using a named constant instead of a magic number.The preset number "3" is hardcoded in the annotation, which could lead to maintenance issues if preset numbering changes in the future.
- @CourseNotificationSettingPreset(3) + @CourseNotificationSettingPreset(CourseNotificationSettingPresetConstants.IGNORE)This would require defining constants in a separate class (e.g.,
CourseNotificationSettingPresetConstants).
1-16: Add documentation to clarify the purpose of this preset.The class lacks documentation explaining its purpose. Adding a class-level Javadoc would help developers understand when and how to use this preset.
package de.tum.cit.aet.artemis.coursenotification.domain.setting_presets; import java.util.Map; import de.tum.cit.aet.artemis.coursenotification.annotations.CourseNotificationSettingPreset; import de.tum.cit.aet.artemis.coursenotification.domain.NotificationSettingOption; import de.tum.cit.aet.artemis.coursenotification.domain.notifications.NewPostNotification; +/** + * Preset that completely ignores notifications for new posts. + * Users with this preset will not receive any notifications (email, webapp, or push) + * when new posts are created. + */ @CourseNotificationSettingPreset(3) public class IgnoreUserCourseNotificationSettingPreset extends UserCourseNotificationSettingPreset {src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/DefaultUserCourseNotificationSettingPreset.java (1)
12-14: Consider extracting notification settings to constants for reusability.The preset map definition is currently duplicated in both preset classes. Consider extracting common notification settings to constants or a shared utility method to improve maintainability and follow the DRY principle.
public class DefaultUserCourseNotificationSettingPreset extends UserCourseNotificationSettingPreset { public DefaultUserCourseNotificationSettingPreset() { - presetMap = Map.of(NewPostNotification.class, Map.of(NotificationSettingOption.EMAIL, false, NotificationSettingOption.WEBAPP, true, NotificationSettingOption.PUSH, true)); + presetMap = Map.of(NewPostNotification.class, createStandardNotificationSettings()); } }And in the parent class:
protected static Map<NotificationSettingOption, Boolean> createStandardNotificationSettings() { return Map.of( NotificationSettingOption.EMAIL, false, NotificationSettingOption.WEBAPP, true, NotificationSettingOption.PUSH, true ); }src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/UserCourseNotificationStatusTestRepository.java (1)
13-14: Standardize parameter naming conventionThe parameter name uses snake_case (
courseNotification_id) rather than the camelCase convention recommended for Java.- UserCourseNotificationStatus findByCourseNotificationId(Long courseNotification_id); + UserCourseNotificationStatus findByCourseNotificationId(Long courseNotificationId);While Spring Data JPA will handle this correctly either way, maintaining consistent naming conventions improves code readability and maintainability.
src/test/java/de/tum/cit/aet/artemis/core/DatabaseQueryCountTest.java (3)
61-61: Document the additional database query for course notificationsThe test now expects 12 database calls instead of 11, reflecting the new course notification functionality. However, there's no documentation explaining this additional query.
}).hasBeenCalledTimes(12); // 1 DB call to get the user from the DB // 1 DB call to get all active courses // 1 DB call to load all exercises // 1 DB call to count the exams // 1 DB call to count the lectures // 1 DB call to get all individual student participations with submissions and results // 1 DB call to get all team student participations with submissions and results // 1 DB call to get all plagiarism cases // 1 DB call to get all grading scales // 1 DB call to get the active exams // 1 DB call to get the batch of a live quiz. No Batches of other quizzes are retrieved + // 1 DB call to count unseen course notifications for the userAdding this comment would help future developers understand why 12 queries are now expected.
73-76: Update comment to include course notificationsThis comment about FAQ-related query count should be updated to reflect that course notification queries are also part of the count.
var course = courses.getFirst(); - // potentially, we might get a course that has faqs disabled, in which case we would have 12 calls instead of 13 + // potentially, we might get a course that has faqs disabled, in which case we would have 12 calls instead of 13 + // these counts include the query for unseen course notificationsThis helps maintain test documentation accuracy as features evolve.
82-94: Add documentation for course notification query in the detailed query listThe list of database calls in the comments doesn't include the call for course notifications.
}).hasBeenCalledTimes(numberOfCounts); // 1 DB call to get the user from the DB // 1 DB call to get the course with lectures // 1 DB call to load all exercises with categories // 1 DB call to load all exams // 3 DB calls to load the numbers of competencies, prerequisites and tutorial groups // 1 DB call to get all individual student participations with submissions and results // 1 DB call to get all team student participations with submissions and results // 1 DB call to get all plagiarism cases // 1 DB call to get the grading scale // 1 DB call to get the batch of a live quiz. No Batches of other quizzes are retrieved // 1 DB call to get the faqs, if they are enabled + // 1 DB call to count unseen course notifications for the userAdding this comment completes the documentation of all expected database calls.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationBroadcastService.java (1)
8-17: Update documentation terminology for clarityThe class is documented as an "Interface" in the Javadoc comment, but it's implemented as an abstract class. While abstract classes can serve as interfaces conceptually, this terminology might be confusing.
/** - * Interface for services that broadcast course notifications to users. + * Abstract base class for services that broadcast course notifications to users. * * <p> * This interface defines the contract for notification services that can * send course-related notifications to a list of recipients. Implementations * might include different delivery methods such as email, push notifications, * or in-app notifications. * </p> */src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationDataDTO.java (1)
1-16: Well-structured DTO with backward compatibilityThe record is well-designed with a clear purpose and appropriate documentation explaining the transitional nature of some fields. The secondary constructor provides a convenient way to create instances with only the essential new field.
Given that most fields are intended to be removed once iOS and Android clients are updated, consider adding a TODO comment with a ticket reference to track this technical debt.
/** * This DTO represents the data that is encrypted and sent to the hermes service. All values except courseNotificationDTO * are part of the old notification api. As soon as iOS and Android are able to process the courseNotificationDTO by itself * we can remove the initial values in this record. + * TODO: Remove legacy fields once mobile clients are updated to use courseNotificationDTO directly */src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatusType.java (1)
19-22: Consider performance optimization for database key lookupThe current implementation uses streaming to find the matching enum value. While adequate for a small enum, for better performance you could consider using a static map for direct lookups:
- public static UserCourseNotificationStatusType fromDatabaseKey(short databaseKey) { - return Arrays.stream(UserCourseNotificationStatusType.values()).filter(type -> type.getDatabaseKey() == databaseKey).findFirst() - .orElseThrow(() -> new IllegalArgumentException("Unknown database key: " + databaseKey)); - } + private static final Map<Short, UserCourseNotificationStatusType> DB_KEY_MAP = Arrays.stream(values()) + .collect(Collectors.toMap(UserCourseNotificationStatusType::getDatabaseKey, Function.identity())); + + public static UserCourseNotificationStatusType fromDatabaseKey(short databaseKey) { + UserCourseNotificationStatusType type = DB_KEY_MAP.get(databaseKey); + if (type == null) { + throw new IllegalArgumentException("Unknown database key: " + databaseKey); + } + return type; + }This would provide O(1) lookup time instead of O(n), though the difference is negligible with only three enum values.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappService.java (1)
49-55: Consider using batch processing for large recipient listsThe current implementation sends messages to each user individually, which could become inefficient with a large number of recipients.
Consider implementing batch processing if recipient lists can be large:
@Async @Override protected void sendCourseNotification(CourseNotificationDTO courseNotification, List<User> recipients) { - recipients.forEach(user -> { - websocketMessagingService.sendMessageToUser(user.getLogin(), WEBSOCKET_TOPIC_PREFIX + courseNotification.courseId(), courseNotification); - }); + // Process in batches of 100 to avoid overwhelming the messaging system + List<List<User>> batches = Lists.partition(recipients, 100); + for (List<User> batch : batches) { + batch.forEach(user -> { + websocketMessagingService.sendMessageToUser(user.getLogin(), + WEBSOCKET_TOPIC_PREFIX + courseNotification.courseId(), + courseNotification); + }); + } }src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java (1)
67-84: Add assertion for total elements in pagination testWhile testing that the page size is respected is good, it would be better to also verify that the total count reflects all 20 elements created.
Add an assertion for the total elements:
request.performMvcRequest(MockMvcRequestBuilders.get("/api/coursenotification/course/" + course.getId() + "?page=0&size=" + pageSize)).andExpect(status().isOk()) - .andExpect(jsonPath("$.content", hasSize(pageSize))); + .andExpect(jsonPath("$.content", hasSize(pageSize))) + .andExpect(jsonPath("$.totalElements").value(20));src/main/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResource.java (1)
44-44: Potential large payload logging.
If the set of notification IDs is large, logging it at DEBUG level could clutter logs and potentially affect performance. Consider truncating or summarizing large collections in the logs.src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/UserCourseNotificationSettingPreset.java (1)
32-32: Consider making the map unmodifiable or private where possible.
If subclasses only populate but do not modify the map post-construction, you could benefit from an unmodifiable or immutable structure. This approach helps prevent accidental updates and improves maintainability.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyServiceTest.java (2)
21-21: Consider using a mocking approach for dependencies.
Currently, the service is being instantiated directly in the test class. If theCourseNotificationPushProxyServicehas external dependencies, consider mocking them or injecting through a factory or dependency injection pattern to maintain isolation and reduce coupling in the test.
31-32: Use of hard-coded placeholder IDs.
The repeated usage of literal values like"456"innotificationPlaceholders()is acceptable for a small test scenario. However, for larger suites, consider defining them as constants to ensure consistency and clarity across multiple tests.Also applies to: 60-60
src/test/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResourceIntegrationTest.java (1)
90-113: Batch update method is well-tested.
Creating multiple notifications and verifying each updated status thoroughly ensures the bulk update endpoint handles all items correctly. Consider adding performance checks in the future to confirm any DB query count thresholds, as recommended by “db_query_count_tests: track_performance.”src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingSpecification.java (1)
71-78: Constructor usage for keys.
This constructor sets all notification flags but omitscourseNotificationType. If this field is critical for the composite key, consider adding it as a parameter or providing an additional constructor to reduce the risk of having an uninitialized key.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryService.java (2)
33-70: Consider extracting reflection-based scanning from the constructor.Performing classpath scanning and reflection in the constructor can introduce unnecessary overhead during application startup and complicate debugging if scanning fails. For maintainability and clarity, consider refactoring this logic into a dedicated initialization or post-construct method.
public CourseNotificationSettingPresetRegistryService() { - // constructor performs scanning and instantiation + // consider moving scanning logic to an initialization method or @PostConstruct }
72-87: Validate the preset existence and behavior in unit tests.The
isPresetSettingEnabledmethod is an important part of the registry’s logic. Consider adding or expanding unit tests to ensure correct behavior when:
typeIdis not present.- The requested notification type or option is disabled.
Would you like me to propose a basic test class to verify these scenarios?
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatus.java (2)
145-149: Replace magic numbers with named constants in hashCode methodThe hashCode method uses magic numbers (31 and 17) without explanation. While these are common prime numbers for hash code implementations, using named constants would improve readability and maintainability.
@Override public int hashCode() { - int somePrimeNumber = 31; - int result = 17; + final int prime = 31; + int result = 17; result = somePrimeNumber * result + (courseNotification != null ? Objects.hash(courseNotification.getId()) : 0); result = somePrimeNumber * result + (user != null ? Objects.hash(user.getId()) : 0); return result;Similarly, update the variable name in the calculations:
- result = somePrimeNumber * result + (courseNotification != null ? Objects.hash(courseNotification.getId()) : 0); - result = somePrimeNumber * result + (user != null ? Objects.hash(user.getId()) : 0); + result = prime * result + (courseNotification != null ? Objects.hash(courseNotification.getId()) : 0); + result = prime * result + (user != null ? Objects.hash(user.getId()) : 0);
212-216: Maintain consistency between hashCode implementationsThe hashCode method in the inner class UserCourseNotificationStatusId uses the same magic numbers but should be consistent with the naming in the outer class. Additionally, update variable names to match the outer class for consistency.
@Override public int hashCode() { - int somePrimeNumber = 31; + final int prime = 31; int result = 17; - result = somePrimeNumber * result + (courseNotification != null ? Objects.hash(courseNotification) : 0); - result = somePrimeNumber * result + (user != null ? Objects.hash(user) : 0); + result = prime * result + (courseNotification != null ? Objects.hash(courseNotification) : 0); + result = prime * result + (user != null ? Objects.hash(user) : 0); return result; }src/test/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusServiceTest.java (1)
88-92: Consider extracting user creation to a utility class for reuseThe
createTestUserhelper method is useful but could potentially be moved to a shared test utility class if it's needed across multiple test classes. This would promote code reuse and consistency in test user creation.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushService.java (1)
48-52: Consider error handling for asynchronous operationsThe current implementation sends notifications to both Apple and Firebase services but doesn't handle potential exceptions that might occur. Consider adding try-catch blocks or using a more robust async execution mechanism to ensure failures in one service don't affect the other.
@Override protected void sendCourseNotification(CourseNotificationDTO courseNotification, List<User> recipients) { var recipientSet = new HashSet<>(recipients); - applePushNotificationService.sendCourseNotification(courseNotification, recipientSet); - firebasePushNotificationService.sendCourseNotification(courseNotification, recipientSet); + try { + applePushNotificationService.sendCourseNotification(courseNotification, recipientSet); + } catch (Exception e) { + // Log the exception but continue with Firebase notifications + log.error("Failed to send Apple push notifications", e); + } + + try { + firebasePushNotificationService.sendCourseNotification(courseNotification, recipientSet); + } catch (Exception e) { + log.error("Failed to send Firebase push notifications", e); + } }Note: This would require adding a logger field to the class:
private static final Logger log = LoggerFactory.getLogger(CourseNotificationPushService.class);src/main/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResource.java (1)
33-42: Fix documentation mismatch with actual endpoint pathThe JavaDoc comment mentions
coursenotification/courses/{courseId}but the actual endpoint path in the@GetMappingannotation iscourse/{courseId}. Update the documentation to match the actual implementation./** - * GET coursenotification/courses/{courseId}: get all non-archived course notifications for the current user + * GET coursenotification/course/{courseId}: get all non-archived course notifications for the current user * * @param courseId the ID of the course * @param pageable the pagination information * @return the ResponseEntity with status 200 (OK) and the list of course notifications in body */src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryService.java (1)
41-66: Consider using a dedicated initialization method or@PostConstructfor classpath scanning.Performing an extensive classpath scan in the constructor may be a performance concern, especially if the list of components grows large. A
@PostConstructmethod or a dedicated initialization routine can help keep the constructor lightweight and more testable.src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationRepository.java (1)
35-42: Use consistent uppercase SQL keywords for readability and alignment with coding guidelines.For clarity and standardization, consider using uppercase for
SELECT DISTINCT,FROM,JOIN, andWHEREkeywords in the query.@Query(""" - SELECT DISTINCT cn FROM CourseNotification cn - JOIN cn.userStatuses us - WHERE us.user.id = :userId - AND cn.course.id = :courseId - AND us.status <> 2 + SELECT DISTINCT cn + FROM CourseNotification cn + JOIN cn.userStatuses us + WHERE us.user.id = :userId + AND cn.course.id = :courseId + AND us.status <> 2 """)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailServiceTest.java (1)
61-67: Consider a configuration-based approach for the server URL.Relying on
ReflectionTestUtils.setFieldto injectserverUrlin tests can be brittle. Prefer reading the server URL from test configuration properties or environment variables, aligning with more standard test injection strategies.src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingPreset.java (2)
29-40: Consider using a primitive type forsettingPreset.Unless null is specifically needed for
settingPreset, switching fromShorttoshortaligns better with the "prefer_primitives" guideline and avoids potential unboxing issues.- private Short settingPreset; + private short settingPreset;
159-216: Optional suggestion: Use@Embeddableand@EmbeddedIdfor the composite key.While
@IdClassis valid, some teams prefer embedding the ID class for clarity and maintenance. Either approach is acceptable, but you might consider@Embeddablein the future for a more self-contained key structure.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupServiceTest.java (1)
87-105: Consider using a fixed clock for time-dependent tests.Relying on
ZonedDateTime.now()can cause edge-case failures around time boundaries. A fixed or mock clock ensures consistent and predictable test results.src/main/java/de/tum/cit/aet/artemis/core/dto/CourseForDashboardDTO.java (1)
10-23: Update documentation to include the new parameterThe Javadoc for the record doesn't include documentation for the new
courseNotificationCountparameter, which should be added for completeness.* @param quizScores the scores for just the quiz exercises in the course, including the max and reachable points and the scores of the currently logged in student * @param participationResults the relevant result for each participation. +* @param courseNotificationCount the count of unseen course notifications for the current user in this course */src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.java (1)
41-41: Consider making the test method more readableThe invocation of the private method using
ReflectionTestUtilswith a long Boolean unwrapping expression could be refactored for better readability.- boolean result = Boolean.TRUE.equals(ReflectionTestUtils.invokeMethod(settingPresetRegistry, "isPresetSettingEnabled", nonExistentPresetId, notificationType, option)); + // Extract method invocation result + Boolean methodResult = ReflectionTestUtils.invokeMethod(settingPresetRegistry, "isPresetSettingEnabled", nonExistentPresetId, notificationType, option); + // Safely handle potential null result + boolean result = Boolean.TRUE.equals(methodResult);src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappServiceTest.java (1)
31-31: Consider making the constant private or protectedThe
WEBSOCKET_TOPIC_PREFIXconstant is currently package-private but should be either private (if only used in this test class) or protected (if shared with subclasses).- private static final String WEBSOCKET_TOPIC_PREFIX = "/topic/coursenotification/"; + private static final String WEBSOCKET_TOPIC_PREFIX = "/topic/coursenotification/";src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.java (1)
139-139: Avoid unsafe type casting in mock setup.The current code uses an unsafe cast to
(Class)which bypasses type safety. Consider using a parameterized type to maintain type safety.-when(courseNotificationRegistryService.getNotificationClass(any())).thenReturn((Class) TestNotification.class); +when(courseNotificationRegistryService.getNotificationClass(any())).thenReturn(TestNotification.class);src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationStatusRepository.java (1)
63-64: Cache implementation looks good but consider TTL strategy.The cache configuration is appropriate, but for user notification counts that can change frequently, consider adding a time-to-live (TTL) value to prevent stale data.
You can add a TTL configuration in your cache configuration class:
@Bean public CacheManager cacheManager() { return new CaffeineCacheManager() .builder() .expireAfterWrite(5, TimeUnit.MINUTES) .build(); }src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.java (1)
79-81: Defensive coding for null array handling.The replaceNullPlaceholders method should check if the input array itself is null before attempting to stream it.
private String[] replaceNullPlaceholders(String[] notificationPlaceholders) { + if (notificationPlaceholders == null) { + return new String[0]; + } return Arrays.stream(notificationPlaceholders).map((s) -> s == null ? "" : s).toArray(String[]::new); }src/main/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusService.java (2)
25-33: Validate constructor arguments.
Currently, there's no null check on injected dependencies. Even though Spring typically guarantees non-null injection, consider an assertion or explicit check to safeguard against future extension or testing scenarios.public UserCourseNotificationStatusService(UserCourseNotificationStatusRepository userCourseNotificationStatusRepository, CourseNotificationCacheService courseNotificationCacheService) { + if (userCourseNotificationStatusRepository == null || courseNotificationCacheService == null) { + throw new IllegalArgumentException("Dependencies must not be null"); + } this.userCourseNotificationStatusRepository = userCourseNotificationStatusRepository; this.courseNotificationCacheService = courseNotificationCacheService; }
35-57: Avoid creating status entries for empty user sets.
If the incoming set of users is empty, we might do unnecessary logic. Consider an early return to optimize or clarify the flow.protected void batchCreateStatusForUsers(Set<User> users, long courseNotificationId, long courseId) { + if (users == null || users.isEmpty()) { + return; + } var courseNotification = new CourseNotification(); courseNotification.setId(courseNotificationId); ... }src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheService.java (3)
54-72: Excellent use of partial key invalidation.
Checking for null user IDs is important. This method is also an async boundary, so consider logging or tracing once the invalidation completes for better observability.
74-95: Potential performance concern withkeySet().
CallingkeySet()on large maps could cause a full iteration. Where possible, maintaining a separate index or a more direct approach may prove more scalable.
103-112: Silent failure in exception block.
CatchingClassCastException | NullPointerExceptionbut ignoring them might hide unexpected states or errors. Log at least a warning if repeated errors occur.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java (1)
40-64: Constructor with multiple dependencies.
While valid, consider verifying if each service is needed at runtime or if some can be injected lazily. Minimizing injected dependencies clarifies responsibilities.src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java (1)
35-39: Restrict field visibility to align with the least access principle.Currently,
courseIdandcreationDateare declared aspublic final. Consider using private or protected fields with getters to keep the API surface minimal and better encapsulated.- public final long courseId; - public final ZonedDateTime creationDate; + private final long courseId; + private final ZonedDateTime creationDate;src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java (1)
39-51: Use injected time or pass in a creation date to facilitate testing.Calling
ZonedDateTime.now()directly in constructors leads to non-deterministic values in tests. Consider injecting a clock or passing a creation date explicitly for more predictable test behavior and easier time-based validations.- super(courseId, ZonedDateTime.now()); + super(courseId, creationDate != null ? creationDate : ZonedDateTime.now());src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailService.java (1)
83-86: Provide a more robust fallback for missing locale keys.Currently, the code defaults to "en" if no language is set. You may want to confirm that “en” is a valid fallback for all use cases and properly localize error messages if the user’s locale is unsupported or incorrectly set.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java (1)
171-205: Good extraction of common logic to reduce duplication.The
encryptAndSendPushNotificationsmethod effectively encapsulates the common logic for encrypting and sending notifications, which reduces code duplication and improves maintainability.Consider adding more detailed Javadoc for this method, especially explaining what happens when the relay server URL is empty. The current comment doesn't fully describe the early return behavior.
/** * Encrypts and sends the data to the hermes service. * -* @param notificationData DTO to be sent via the channel the implementing service is responsible for -* @param userDeviceConfigurations devices to be contacted +* @param notificationData DTO to be sent via the channel the implementing service is responsible for +* @param userDeviceConfigurations devices to be contacted +* @return void - returns early if relay server URL is empty or if there's an error in encryption */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250214103211_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (68)
src/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/ApplePushNotificationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/FirebasePushNotificationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationDataDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/DatabaseConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/CourseForDashboardDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/annotations/CourseNotificationSettingPreset.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/annotations/CourseNotificationType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotification.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotificationParameter.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/NotificationSettingOption.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingPreset.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingSpecification.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatus.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatusType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotificationCategory.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/AllActivityUserCourseNotificationSettingPreset.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/DefaultUserCourseNotificationSettingPreset.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/IgnoreUserCourseNotificationSettingPreset.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/UserCourseNotificationSettingPreset.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/dto/CourseNotificationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/dto/CourseNotificationPageableDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/dto/UserCourseNotificationStatusUpdateRequestDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationParameterRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationSettingPresetRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationSettingSpecificationRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationStatusRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationBroadcastService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResource.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/core/DatabaseQueryCountTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationRepositoryArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationResourceArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationServiceArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/CourseNotificationParameterTestRepository.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/CourseNotificationTestRepository.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/UserCourseNotificationStatusTestRepository.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResourceIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/TestRepositoryConfiguration.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotificationCategory.java
- src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/NotificationSettingOption.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/core/config/DatabaseConfiguration.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/DefaultUserCourseNotificationSettingPreset.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/IgnoreUserCourseNotificationSettingPreset.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/AllActivityUserCourseNotificationSettingPreset.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/dto/CourseNotificationDTO.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/annotations/CourseNotificationSettingPreset.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/FirebasePushNotificationService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationParameterRepository.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatusType.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotificationParameter.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationBroadcastService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/UserCourseNotificationSettingPreset.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/dto/UserCourseNotificationStatusUpdateRequestDTO.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationRepository.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/dto/CourseNotificationPageableDTO.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationSettingSpecificationRepository.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResource.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationDataDTO.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/ApplePushNotificationService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationSettingPresetRepository.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingSpecification.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotification.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResource.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusService.javasrc/main/java/de/tum/cit/aet/artemis/core/dto/CourseForDashboardDTO.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/annotations/CourseNotificationType.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingPreset.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailService.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationStatusRepository.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatus.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/coursenotification/architecture/CourseNotificationCodeStyleArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationResourceArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationRepositoryArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationServiceArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/CourseNotificationParameterTestRepository.javasrc/test/java/de/tum/cit/aet/artemis/core/DatabaseQueryCountTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResourceIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/UserCourseNotificationStatusTestRepository.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/CourseNotificationTestRepository.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/TestRepositoryConfiguration.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (140)
src/test/java/de/tum/cit/aet/artemis/shared/TestRepositoryConfiguration.java (1)
19-24: Changes align well with the repository configuration pattern.The addition of the "de.tum.cit.aet.artemis.coursenotification.test_repository" package to the basePackages parameter follows the established pattern for repository scanning in the test configuration. The formatting is consistent with the existing style and maintains proper line breaks.
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/IgnoreUserCourseNotificationSettingPreset.java (2)
5-8: Appropriate imports for the notification preset functionality.The imports are clean, specific, and include only what's needed for this class's functionality, adhering to the "avoid_star_imports" guideline.
12-15:❓ Verification inconclusive
Confirm if this preset should only affect NewPostNotification.
The preset only configures settings for
NewPostNotificationwhile disabling all channels (EMAIL, WEBAPP, PUSH). If this is intentional, consider adding a comment explaining why only this notification type is affected. If other notification types should also be ignored, they should be added to the preset.
🏁 Script executed:
#!/bin/bash # Find all notification types that might need to be included in the preset echo "Searching for all notification classes that might need configuration in the preset..." find src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications -name "*.java" | sortLength of output: 598
Clarify Intentionality of NewPostNotification-Only Configuration
The preset currently disables all channels (EMAIL, WEBAPP, PUSH) for only
NewPostNotification. However, our investigation shows that there are additional notification classes (namely,CourseNotificationandCourseNotificationCategory). Please confirm whether this configuration is deliberately limited toNewPostNotification. If it is intentional, consider adding a clarifying comment explaining why the other notifications are not configured here. If not, please update the preset to include the relevant notification types.src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationCodeStyleArchitectureTest.java (1)
5-11: Test class follows the module testing pattern correctly.The architecture test class properly extends
AbstractModuleCodeStyleTestand correctly overrides thegetModulePackage()method to return the coursenotification module package. This follows the established pattern for module-specific architecture tests.src/main/java/de/tum/cit/aet/artemis/core/config/DatabaseConfiguration.java (2)
15-21: Repository configuration properly includes the new coursenotification module.The addition of "de.tum.cit.aet.artemis.coursenotification.repository" to the basePackages parameter is correctly formatted and follows the alphabetical ordering convention used in the existing configuration. This change ensures that JPA repositories for the new coursenotification module will be properly scanned and registered.
15-21:✅ Verification successful
Verify all repositories in the coursenotification package are discovered.
To ensure no repositories are missed by this configuration, it would be good to verify that all repository interfaces in the coursenotification package are properly discovered and registered by Spring.
🏁 Script executed:
#!/bin/bash # Find all repository interfaces in the coursenotification package echo "Searching for repository interfaces in the coursenotification package..." find src/main/java/de/tum/cit/aet/artemis/coursenotification/repository -name "*Repository.java" | sort # Check if these repositories follow the Spring Data JPA repository pattern echo -e "\nVerifying repository interfaces extend Spring Data repositories..." rg "interface \w+Repository(\s+extends|\s*<)" src/main/java/de/tum/cit/aet/artemis/coursenotification/repositoryLength of output: 2289
All coursenotification repositories are correctly discovered.
The verification script shows that all repository interfaces in thecoursenotificationpackage (namely,CourseNotificationParameterRepository,CourseNotificationRepository,UserCourseNotificationSettingPresetRepository,UserCourseNotificationSettingSpecificationRepository, andUserCourseNotificationStatusRepository) extendArtemisJpaRepositoryand are therefore properly registered by Spring according to the configuration in the@EnableJpaRepositoriesannotation.src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/CourseNotificationParameterTestRepository.java (1)
1-11: LGTM - Clean test repository interface definition.The test repository interface is correctly defined and follows best practices:
- It properly extends the main repository interface
- It uses appropriate Spring annotations (
@Repositoryand@Primary)- It maintains a clean structure with properly organized imports
This pattern allows for easy mocking/overriding in test scenarios.
src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/CourseNotificationTestRepository.java (1)
1-11: LGTM - Clean test repository interface definition.The test repository interface correctly:
- Extends the main CourseNotificationRepository
- Uses appropriate Spring annotations (
@Repositoryand@Primary)- Follows the same pattern as the CourseNotificationParameterTestRepository
This consistent approach across test repositories helps maintain a predictable testing structure.
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/DefaultUserCourseNotificationSettingPreset.java (1)
9-10: LGTM - Clear preset annotation with unique ID.The annotation with ID 1 clearly identifies this as the default preset in the preset hierarchy.
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/AllActivityUserCourseNotificationSettingPreset.java (2)
9-10: LGTM - Clear preset annotation with unique ID.The annotation with ID 2 clearly identifies this preset in the preset hierarchy.
12-14:❓ Verification inconclusive
Presets have identical notification settings - verify this is intentional.
The notification settings in this preset are identical to those in DefaultUserCourseNotificationSettingPreset. If these presets should have different behaviors, they should have different settings. Otherwise, consider using a single preset class or a factory method.
🏁 Script executed:
#!/bin/bash # Description: Check if these are the only notification classes in the system # to understand if more settings should be configured in AllActivityUserCourseNotificationSettingPreset echo "Finding all notification classes to verify if AllActivityUserCourseNotificationSettingPreset should have more notification types:" find src/main/java -name "*.java" -type f | grep -i notification | grep -v "NewPostNotification" | grep "domain/notifications" echo -e "\nChecking difference between presets:" diff --brief "src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/DefaultUserCourseNotificationSettingPreset.java" "src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/AllActivityUserCourseNotificationSettingPreset.java"Length of output: 1240
Action Required: Confirm Notification Preset Design
- Although the two preset files differ overall, the mapping for
NewPostNotification—with EMAIL disabled, WEBAPP and PUSH enabled—is identical to that inDefaultUserCourseNotificationSettingPreset.- The diff confirms that the files are not entirely the same, but the core notification settings remain unchanged.
- Please verify whether this duplication is intentional. If the identical settings are by design, consider consolidating them (e.g., via a common preset or factory method) to reduce redundancy. Otherwise, update one of the presets to reflect the intended behavioral differences.
src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationResourceArchitectureTest.java (1)
5-9: LGTM: Clean architecture test implementation.This architecture test class follows the established pattern for module resource architecture tests and correctly defines the module package for the course notification system.
src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationServiceArchitectureTest.java (1)
5-9: LGTM: Clean architecture test implementation.This architecture test class properly extends the abstract base class and correctly defines the module package for the course notification service architecture validation.
src/main/java/de/tum/cit/aet/artemis/coursenotification/annotations/CourseNotificationType.java (1)
8-25: LGTM: Well-designed annotation with clear documentation.The annotation is well-documented with proper Javadoc explaining its purpose and usage. It follows Java annotation best practices with appropriate retention policy and target type. The use of a primitive int for the value is efficient and aligns with the coding guidelines.
src/main/java/de/tum/cit/aet/artemis/coursenotification/dto/UserCourseNotificationStatusUpdateRequestDTO.java (1)
9-14: LGTM: Properly structured DTO using Java record.This DTO follows best practices by:
- Using a Java record for immutable data transfer
- Including only essential fields needed for the operation
- Using the @JsonInclude annotation to optimize serialization
- Having clear documentation with a descriptive comment
The record structure aligns with the PR objective of tracking notification statuses (seen/unseen/archived) for individual users.
src/test/java/de/tum/cit/aet/artemis/coursenotification/architecture/CourseNotificationRepositoryArchitectureTest.java (1)
5-10: Architecture test correctly extends the abstract module testThe architecture test follows good practices by extending the abstract base class and providing the specific module package. This ensures consistency in architecture testing across different modules.
src/main/java/de/tum/cit/aet/artemis/coursenotification/dto/CourseNotificationDTO.java (1)
14-17: Clean DTO implementation using Java recordsThis DTO record implementation follows the coding guidelines perfectly:
- Uses Java records as recommended
- Includes only the minimal necessary data
- Has a single, focused responsibility
- Properly implements Serializable for transport
- Uses JsonInclude annotation to optimize serialization
The parameters field using a Map<String, String> enables flexibility for different notification types while maintaining type safety for the core fields.
src/test/java/de/tum/cit/aet/artemis/coursenotification/test_repository/UserCourseNotificationStatusTestRepository.java (1)
9-13: Appropriate test repository setup with @primary annotationThe repository is correctly annotated with @repository and @primary to ensure it's used during tests instead of the actual implementation.
src/main/java/de/tum/cit/aet/artemis/coursenotification/annotations/CourseNotificationSettingPreset.java (1)
1-25: Well-structured annotation with clear documentationThe annotation is well-implemented with appropriate retention policy and target type. The documentation clearly explains its purpose and usage requirements.
src/main/java/de/tum/cit/aet/artemis/coursenotification/dto/CourseNotificationPageableDTO.java (1)
1-19: Clean and efficient DTO implementationThe record-based DTO follows best practices by:
- Using Java records for immutable data transfer
- Implementing Serializable
- Including JsonInclude annotation to optimize JSON serialization
- Providing a convenient static factory method
This implementation aligns perfectly with the coding guidelines requiring Java records for DTOs with minimal data and single responsibility.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationBroadcastService.java (1)
18-33: Well-designed abstraction for notification servicesThe abstract class provides a clear contract for services that need to broadcast notifications through different channels. The protected abstract method ensures proper encapsulation while forcing implementing classes to provide the delivery-specific logic.
src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationParameterRepository.java (3)
1-13: Appropriate package structure and importsThe repository is correctly placed in the repository package and imports the necessary classes. The use of static import for the profile constant enhances readability.
14-19: Well-documented repository interfaceThe repository interface is properly documented and correctly annotated with @repository and @Profile. It follows the Java naming conventions using CamelCase and extends the appropriate base repository.
21-22: Effective caching strategy for notification parametersGood implementation of caching with the @Cacheable annotation. The cache key is well-structured, and the "unless" condition prevents caching empty results, which optimizes memory usage.
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotification.java (5)
1-20: Appropriate imports and package structureThe imports are well-organized, and the class is placed in the correct package following the application's architecture.
21-47: Well-structured entity with proper JPA annotationsThe CourseNotification entity follows JPA best practices with:
- Appropriate table name
- Proper entity relations (ManyToOne and OneToMany)
- Correct fetch types (LAZY for Course reference)
- Cascade settings for related entities
- Well-initialized collections
Good use of @JsonInclude to optimize JSON serialization.
48-67: Clean constructors with good documentationThe constructors are well-implemented and documented with JavaDoc comments explaining their purpose and parameters.
68-175: Comprehensive getters and setters with proper documentationAll accessor methods are well-documented with JavaDoc comments, which enhances code readability and maintainability.
177-180: Concise toString() methodThe toString() method includes essential fields for debugging purposes, which is good practice. It properly references getId() from the parent class.
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheServiceTest.java (7)
1-34: Well-organized imports and proper test setupThe test class has all necessary imports for Mockito, JUnit 5, and AssertJ. Static imports for assertion and mockito methods improve readability.
35-55: Clean test setup with appropriate mockingGood use of MockitoExtension and properly initialized mock objects. The setUp method correctly initializes the service under test with the mocked dependencies.
57-78: Thorough testing of cache invalidation for single userThe test effectively verifies:
- Cache map retrieval
- Cache key deletion
- Specific key patterns for notifications and counts
Good use of Awaitility for handling asynchronous operations in the test.
80-105: Comprehensive test for multiple users scenarioThe test properly checks invalidation for multiple users, verifying that:
- All user-specific cache entries are deleted
- Both notification and count caches are cleared
- The correct number of deletions are performed
Good use of ArgumentCaptor to verify the exact keys being deleted.
107-128: Robust error handling testsThe tests properly validate:
- Exception throwing for users without IDs
- Handling of empty user sets
Good use of assertThatExceptionOfType to verify both the exception type and message.
130-167: Comprehensive testing with parameterized tests and exception handlingExcellent use of parameterized testing for different course IDs and explicit testing of exception handling during cache operations.
169-181: Well-designed test helper methodsThe helper methods are clean, reusable, and enhance test readability by encapsulating common test data creation logic.
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatusType.java (2)
1-8: Well-structured enumeration with clear status typesThe enum defines three distinct notification status types with appropriate database keys. The naming is clear and follows conventions with enum constants in uppercase.
9-17: Clean implementation of database key mappingGood design for storing and retrieving the database key associated with each enum value.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappService.java (2)
25-27: Appropriate service configurationThe class is properly annotated with
@Serviceand@Profile(PROFILE_CORE), ensuring it's only active in the core profile. The extension ofCourseNotificationBroadcastServicefollows good design principles by specializing the base functionality.
29-35: Well-implemented constructor injection and constant definitionThe service correctly uses constructor injection for the WebsocketMessagingService dependency, following the recommended dependency injection pattern. The constant WEBSOCKET_TOPIC_PREFIX is appropriately defined with private static final modifiers.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupService.java (2)
17-19: Appropriate service configurationThe class is properly annotated with
@Serviceand@Profile(PROFILE_SCHEDULING), ensuring it's only active in the scheduling profile, which is the correct approach for scheduled tasks.
27-30: Well-implemented constructor injectionThe service correctly uses constructor injection for dependencies, following the recommended dependency injection pattern.
src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java (5)
24-33: Well-structured test class setupThe test class properly extends
AbstractSpringIntegrationIndependentTestand injects the required repositories. The test prefix constant is a good practice for isolating test data.
38-42: Well-implemented test setupThe
setUpmethod correctly initializes the test data before each test, creating a clean state for each test method.
44-58: Test ensures notifications are correctly retrievedThis test properly validates that the endpoint returns the expected notification when one exists in the database. The assertions check both the status code and the content of the response.
60-65: Test properly verifies empty result handlingThis test correctly verifies that the endpoint returns an empty result when no notifications exist. Good edge case testing.
24-84:✅ Verification successful
Missing test for updating notification status
According to the AI summary, there should be tests for updating notification statuses, but they aren't included in this file. This is a critical functionality that should be tested.
Verify if there are tests for updating notification status elsewhere:
🏁 Script executed:
#!/bin/bash # Search for tests that verify notification status updates echo "Searching for tests related to notification status updates..." rg -A 3 "updateNotificationStatus|shouldUpdateNotificationStatus" --glob "src/test/java/**/*.java"Length of output: 962
Test for updating notification status is already covered elsewhere
The tests for updating notification statuses are implemented in the file
src/test/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResourceIntegrationTest.java(see the methodshouldUpdateNotificationStatusWhenUpdateIsCalled()).
Given that these tests exist and clearly separate concerns from the notifications retrieval tests inCourseNotificationResourceIntegrationTest.java, no additional update tests are required in the latter file.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/FirebasePushNotificationService.java (2)
21-21: Appropriate import for new dependencyThe import for
CourseNotificationPushProxyServiceis correctly added to support the new constructor parameter.
38-41: Constructor updated to support course notificationsThe constructor has been properly updated to include the
CourseNotificationPushProxyServiceand passes it to the superclass constructor. This change enables integration with the course-specific notification system.src/main/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResource.java (3)
1-3: No issues with package and import statements.
The chosen package path is consistent with the application's package structure, and no wildcard imports are used.
24-28: Constructor injection aligns with best practices.
Using final fields and constructor injection enforces immutability and clearly defines dependencies per the coding guidelines.
46-48: Ensure proper authorization checks for the course.
Currently, the code retrieves the current user and updates notifications for the specified course. Consider verifying that the user is authorized to update notifications in that course (e.g., enrolled in the course or has the correct privileges).src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/setting_presets/UserCourseNotificationSettingPreset.java (3)
1-2: Package and basic setup look good.
No star imports and consistent naming follow the guidelines.
42-48: Method implementation appears correct.
TheisEnabledapproach is concise, properly checks the map, and returns default false when not configured. Adheres to single responsibility.
56-60: CamelCase identifier generation is straightforward.
This method meets the KISS principle and correctly transforms the class name to a readable identifier for potential translations.src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotificationParameter.java (3)
20-23: Entity configuration looks correct.
Table naming and annotations conform to JPA best practices, and JSON inclusion is minimal.
31-33: Verify the length of 'param_key'.
A length of 20 might be restrictive if future features require longer parameter names. Confirm that 20 characters is sufficient for all intended use cases.
160-216: Composite key class is logically consistent.
TheCourseNotificationParameterIddesign is coherent and meets JPA requirements for composite keys.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyServiceTest.java (2)
23-42: Test method is clear and descriptive.
The naming of the test (shouldTransformNewMessageNotificationCorrectly) and the use of structured assertions with AssertJ provide clarity. This aligns well with the guideline for having “small_specific” test methods and improves readability.
64-74: Parameterized test coverage is valid.
Testing unsupported types using parameterized inputs is beneficial for coverage. This enforces robust behavior when encountering unexpected notification types.src/test/java/de/tum/cit/aet/artemis/coursenotification/web/UserCourseNotificationStatusResourceIntegrationTest.java (2)
27-27: Integration testing approach is appropriate.
Using a full integration test with database interactions is suitable here, as these tests verify the HTTP endpoint behavior and database updates, aligning with the “resource” or “controller” test scenario.
49-65: Sufficient assertion of status updates.
The test checks both the HTTP response status code and ensures the underlying database record is updated toSEEN. This strategy provides a thorough verification of functionality.src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingSpecification.java (2)
21-23: Entity and table structure are cohesive.
The class uses JPA annotations effectively, including a composite primary key definition and table mapping that aligns with the requirement for user-specific notification settings.
252-313: Composite key class is well-defined.
Defining a static class to handle the composite key is a standard approach in JPA, ensuring clarity in how user, course, and notification type form a unique key.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryService.java (2)
1-16: Overall import structure looks good.No wildcard imports are used, and CamelCase naming is followed for the class name.
22-32: Class-level construction and field usage are well-organized.The class is properly annotated with
@Profile(PROFILE_CORE)and@Service, adhering to standard Spring conventions. The creation of aLoggerinstance and thepresetsmap are consistent with the project's coding style.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushServiceTest.java (5)
24-25: Proper use of JUnit 5 extension and Mockito.Declaring
@ExtendWith(MockitoExtension.class)correctly integrates Mockito with JUnit 5. No issues found here.
35-39: Constructor usage in setup is clear.Injecting mocks in
CourseNotificationPushServiceTest#setUpensures a clean test environment for each test. This approach follows good testing practices.
40-50: Tests are appropriately verifying the interaction with push services.The test confirms that both services receive the notification with the correct set of recipients, which is a solid usage of Mockito's
verify(...).
52-62: Good coverage for empty recipient lists.Ensuring that the logic still calls both push services with an empty set helps maintain consistent behavior across all scenarios.
64-78: Helper methods enhance readability.Private factory methods (
createTestNotificationandcreateTestRecipients) keep the tests short and focused. This is consistent with keeping test methods small and specific.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/ApplePushNotificationService.java (1)
34-35: Constructor injection aligns with best practices.The addition of
CourseNotificationPushProxyServiceto the constructor is consistent with dependency injection principles, reducing coupling and improving maintainability.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusServiceTest.java (3)
40-58: Well-structured test with clear assertions and verificationThis test method effectively verifies the batch creation functionality with clear assertions that check both the correct status creation and the cache invalidation. The code follows good practices by using argument captors to verify the saved objects.
60-72: Good use of parameterized testingUsing
@ParameterizedTestwith@EnumSourceis an excellent approach to test all possible status types without duplicating test code. This ensures comprehensive test coverage of the functionality for all enum values.
74-86: Testing edge case with empty collectionExcellent approach to test the edge case with an empty notification list. This ensures the service handles this scenario gracefully and still performs the expected repository call and cache invalidation.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushService.java (2)
34-37: Well-implemented constructor with dependency injectionThe constructor follows best practices by using constructor injection instead of field injection, which makes the dependencies explicit and promotes testability.
16-25: Excellent comprehensive JavaDocThe class-level JavaDoc is well-written, clearly explaining the purpose and behavior of the service. It provides good context about the notification delivery mechanism and platforms supported.
src/main/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResource.java (1)
28-31: Good use of constructor injectionThe class correctly uses constructor injection for its dependencies, which is the recommended approach in Spring applications. This makes the dependencies explicit and improves testability.
src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/CourseNotificationRepository.java (1)
1-2: Interface naming and package structure look good.This file follows the recommended naming conventions and properly extends the base repository. No issues found here.
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailServiceTest.java (2)
69-90: Ensure stability of time-bound tests with Awaitility.Using a 2-second upper limit might cause flakiness in slower environments. Validate whether this duration is sufficient or consider higher timeouts or a more deterministic approach to reduce test flakiness.
210-229: Parameterized tests are well-structured.Your CSV-based parameterization for testing different notification templates is a concise approach that boosts test coverage and readability.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.java (1)
26-42: Good use of constructor injection.This approach follows the recommended practice of constructor-based dependency injection for better testability and clarity.
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupServiceTest.java (2)
42-52: Test naming and structure look good.The descriptive method name and use of
assertThatalign well with the coding guidelines for tests.
64-76: Well-structured parameterized test.This approach efficiently verifies multiple scenarios, maintaining small, specific tests.
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseForDashboardDTO.java (2)
25-26: Good enhancement of the DTO with notification count fieldThe addition of the
courseNotificationCountfield to theCourseForDashboardDTOrecord is well-implemented and aligns with the PR's objective to integrate course notification functionality.
28-31: Well-designed backward compatibility approachThe secondary constructor that defaults
courseNotificationCountto0Lis an excellent approach for maintaining backward compatibility with existing code that doesn't provide notification counts.This implementation allows for:
- Gradual adoption of the new notification feature
- Reduced risk when rolling out changes
- Clean integration with existing code
src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationSettingPresetRepository.java (3)
19-21: Well-structured repository with appropriate cachingThe repository is properly configured with caching annotations using a dedicated cache name, which aligns with performance optimization best practices.
31-32: Good use of key-based caching for performance optimizationThe
@Cacheableannotation with a compound key that includes both user ID and course ID is an excellent approach for targeted caching, which reduces database load and improves response times.
41-44: Correctly implemented cache eviction with transactionsThe override of the
savemethod properly evicts the cache entry when a setting is saved, ensuring cache consistency. The@Transactionalannotation is correctly applied to this write operation.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.java (3)
25-33: Well-structured test setup with appropriate annotationsThe test class is properly structured with Mockito extension and a clean setup method that initializes the service under test before each test execution.
35-44: Good test for non-existent preset handlingThis test correctly verifies that the service returns
falsewhen a preset ID doesn't exist, which is an important edge case to validate.
97-118: Well-implemented test helper classThe
TestNotificationinner class is well-designed with appropriate overrides and implementation of the required methods, providing a concrete test implementation.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationWebappServiceTest.java (4)
23-36: Well-structured test class with clear setupThe test class follows best practices with proper extension configuration, mocking of dependencies, and a clean setup method.
38-48: Comprehensive test verifying notification delivery to multiple recipientsThis test effectively validates that the service correctly sends notifications to each recipient when multiple users are provided, with proper verification of the expected behavior.
50-58: Good edge case testing for empty recipient listThe test correctly verifies that no messages are sent when the recipient list is empty, which is an important edge case to validate.
71-80: Well-designed helper methods for test data creationThe helper methods for creating test users and notifications are well-implemented and promote code reuse across test methods.
src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationSettingSpecificationRepository.java (3)
20-23: Repository properly configured with appropriate annotations.The repository interface is correctly annotated with
@Profile,@Repository, and@CacheConfig, following the project's architectural patterns for data access components.
33-34: Well-designed cacheable query method.The method is appropriately annotated with
@Cacheableand uses a descriptive cache key pattern that includes both user and course IDs to ensure proper cache isolation.
53-56: Same null safety concern in delete method's cache eviction.Similar to the save method, the delete operation's cache eviction key might cause a NullPointerException if the user or course is null.
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.java (4)
42-43: Test class follows JUnit 5 best practices.The test class is properly configured with MockitoExtension for efficient mock creation and handling.
74-78: Proper test setup with constructor-based dependency injection.The setup method follows best practices by initializing the service under test with all required dependencies via constructor injection.
80-101: Comprehensive test for multi-channel notification distribution.This test effectively verifies that notifications are correctly routed to appropriate channels based on user preferences. The use of ArgumentCaptor to verify the exact set of notified users is a good practice.
190-224: Well-structured test notification implementation.The TestNotification inner class is well-designed with clear implementation of required methods and flexibility in channel configuration through constructors.
src/main/java/de/tum/cit/aet/artemis/coursenotification/repository/UserCourseNotificationStatusRepository.java (1)
38-47: Well-implemented bulk update operation.The method uses JPQL with parameter binding for safe query execution and is properly annotated with @Modifying and @transactional for database modifications.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.java (1)
17-29: Good documentation of temporary nature of the service.The class documentation clearly explains the purpose of this service as a temporary compatibility layer, which helps future developers understand when it might be removed.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/UserCourseNotificationStatusService.java (2)
1-23: Good overall structure and compliant naming.
This service cleanly follows Single Responsibility principles and is annotated correctly for Spring usage. Constructor-based dependency injection aligns with best practices.
59-73: Consider transactional boundaries.
This update method modifies multiple user notification statuses and invalidates caches but does not explicitly ensure atomicity. Consider annotating with@Transactionalor verifying the repository method if a transaction is already handled elsewhere.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCacheService.java (2)
1-27: Solid approach for caching.
The class is well-organized and leverages Spring and Hazelcast effectively. Keep an eye on concurrency aspects with@Asyncmethods.
46-52: Validate performance implications of clearing the entire cache.
Clearing the entire cache might be expensive if the map is large. Ensure the use case justifies this blanket invalidation.src/test/java/de/tum/cit/aet/artemis/communication/notifications/service/push_notifications/AppleFirebasePushNotificationServiceTest.java (3)
37-37: Import looks valid.
ImportingCourseNotificationPushProxyServiceis consistent with the new push proxy service usage. No immediate issues.
54-55: Appropriate usage of mocks.
Mocking the newCourseNotificationPushProxyServiceensures the test remains isolated from the actual push proxy logic.
84-85: Constructor changes appear correctly integrated.
The new signature withcourseNotificationPushProxyServiceis properly reflected in the test. Consider adding coverage to ensure the push proxy service is called under certain conditions if needed.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java (3)
1-38: Comprehensive service annotation and profile scoping.
The class-level Javadoc is thorough, explaining the service's usage. Keep monitoring the profile usage to confirm it's activated only in relevant environments.
94-136: Reflection-based instantiation.
Be aware of performance overhead and possible security implications when using reflection. If possible, consider a more direct approach or a basic factory pattern for instantiating domain objects.
138-152: Clear mapping logic for parameters.
This private helper method is straightforward and aligns with the KISS principle. Keep an eye on potential collisions if keys can be duplicated.src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java (1)
66-109: Validate incoming parameter data and consider potential security implications.Relying on reflection to set protected fields from external parameters can introduce significant risks if the source data is not strictly controlled or validated. This approach can also become error-prone if new field types are later added but not included in the parse logic.
Would you like me to generate a script to search for usages of this constructor to confirm data validation?
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java (1)
15-16: Ensure unique notification type identifiers.You have used
@CourseNotificationType(1). Confirm that no other notification class duplicates the same integer ID within the system, or it may cause conflicts when persisting notifications to the database.src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationEmailService.java (1)
80-115: Clarify asynchronous sending strategy.Although the Javadoc mentions asynchronous email sending, the current loop executes sequentially. Double-check if
mailSendingServicehandles asynchronous processing internally. If you intend to parallelize sending, consider using an asynchronous executor or parallel streams to avoid blocking.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/push_notifications/PushNotificationService.java (4)
49-50: Appropriate imports added for course notification integration.The imports for
CourseNotificationDTOandCourseNotificationPushProxyServiceclearly indicate the integration of course notification capabilities into the push notification service.
66-70: Good use of dependency injection for the new service.The
CourseNotificationPushProxyServiceis properly injected using constructor injection, following the project's dependency injection pattern. This is preferable to field injection.
137-151: Improved notification handling with input validation.The refactored method now includes proper validation by checking if
userDeviceConfigurationsis empty before proceeding with notification sending. The code is also more concise by directly checking notification type and moving the common encryption logic to a separate method.
153-169: Well-implemented new method for course notifications.The
sendCourseNotificationmethod follows the same pattern as the existing notification method, maintaining consistency in the codebase. It properly validates input and delegates to the common encryption method.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingServiceTest.java (8)
29-45: Well-structured test class with proper mock setup.The test class follows JUnit 5 and Mockito best practices, clearly defining the service under test and its dependencies. The
@ExtendWith(MockitoExtension.class)annotation ensures proper mock initialization.
46-50: Good setup method initializing the service under test.The setup method properly initializes the
CourseNotificationSettingServicewith all required dependencies, ensuring each test starts with a clean service instance.
52-85: Well-written test for custom notification settings.This test thoroughly validates the filtering of recipients based on custom settings. It properly sets up test data, mocks repository responses, and verifies the expected outcome with clear assertions.
87-109: Good test coverage for preset notification settings.The test correctly verifies the filtering logic when using preset settings. It properly sets up different presets for different users and checks that only users with enabled preset settings are included in the filtered list.
111-136: Thorough test of edge case when no recipients match filter.This test ensures that the service returns an empty list when no recipients match the filter criteria, which is an important edge case to verify.
138-160: Good test for the case when notification specification is not found.This test verifies another important edge case: when a user's notification specification doesn't match the notification type. The test confirms that an empty list is returned in this scenario.
162-167: Useful helper method for creating test users.The
createTestUsermethod improves code readability by encapsulating user creation logic, reducing duplication across test methods.
169-202: Well-implemented test notification class.The
TestNotificationinner class provides a concrete implementation for testing purposes, implementing all required methods from the abstractCourseNotificationclass. The implementation follows the single responsibility principle.src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.java (6)
22-30: Well-structured test class with proper initialization.The class follows JUnit 5 best practices, with a clear structure and initialization of the service under test in the setup method.
32-39: Good test for handling unknown notification class.This test correctly verifies that the service returns null when an unknown notification type ID is requested, validating the error handling behavior.
41-48: Thorough test for handling unknown notification identifier.The test properly verifies that requesting an identifier for an unknown notification class returns null, covering another important error case.
50-60: Use of ReflectionTestUtils to test private methods.While the test itself is well-structured, it relies on ReflectionTestUtils to access private methods, which can be fragile if the implementation changes.
This approach works but consider if these methods should be package-private instead of private to facilitate testing without reflection. Alternatively, you could test the public API that uses these private methods.
74-86: Well-designed helper methods for test data creation.The helper methods improve test readability by encapsulating the creation of test notification classes, following good testing practices.
88-109: Good use of annotation for test notification class.The
TestNotificationclass is properly annotated with@CourseNotificationTypeand extendsCourseNotification, providing a realistic test case for the registry service.src/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.java (4)
36-36: Appropriate import for notification integration.The import for
UserCourseNotificationStatusRepositoryclearly indicates the integration of course notification status tracking into the course score calculation service.
70-70: Good use of final field for repository dependency.The
userCourseNotificationStatusRepositoryfield is properly declared as private and final, following good encapsulation and immutability practices.
73-79: Proper constructor injection for the new dependency.The constructor correctly initializes the new repository dependency, following the project's dependency injection pattern.
275-275: Enhanced dashboard DTO with notification count.The method now includes the count of unseen notifications in the
CourseForDashboardDTO, utilizing the repository to fetch this information for the specific user and course.
...aet/artemis/coursenotification/architecture/CourseNotificationCodeStyleArchitectureTest.java
Show resolved
Hide resolved
...java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationCleanupService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotificationParameter.java
Show resolved
Hide resolved
...um/cit/aet/artemis/coursenotification/domain/UserCourseNotificationSettingSpecification.java
Show resolved
Hide resolved
...main/java/de/tum/cit/aet/artemis/coursenotification/domain/UserCourseNotificationStatus.java
Show resolved
Hide resolved
...um/cit/aet/artemis/coursenotification/repository/UserCourseNotificationStatusRepository.java
Show resolved
Hide resolved
...va/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java
Show resolved
Hide resolved
.../java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java
Outdated
Show resolved
Hide resolved
...java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotification.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java (1)
66-92:⚠️ Potential issueAdd transaction boundary and handle partial delivery scenarios.
The notification sending process involves multiple database operations across channels, but lacks transaction boundaries.
Add a transactional boundary and consider tracking success/failure for each channel:
+@Transactional public void sendCourseNotification(CourseNotification courseNotification, List<User> recipients) { var supportedChannels = courseNotification.getSupportedChannels(); var setOfNotifiedUsers = new HashSet<User>(); + var deliveryResults = new HashMap<NotificationSettingOption, Boolean>(); var courseNotificationEntityId = createCourseNotification(courseNotification); for (var supportedChannel : supportedChannels) { var service = serviceMap.get(supportedChannel); if (service == null) { + deliveryResults.put(supportedChannel, false); continue; } var filteredRecipients = courseNotificationSettingService.filterRecipientsBy(courseNotification, recipients, supportedChannel); - service.sendCourseNotification(convertToCourseNotificationDTO(courseNotification), filteredRecipients); + try { + service.sendCourseNotification(convertToCourseNotificationDTO(courseNotification), filteredRecipients); + deliveryResults.put(supportedChannel, true); + } catch (Exception e) { + log.error("Failed to deliver notification through channel {}: {}", supportedChannel, e.getMessage(), e); + deliveryResults.put(supportedChannel, false); + } // We keep track of the notified users so that we only create notification status entries for them setOfNotifiedUsers.addAll(filteredRecipients); } + // Only create status entries if at least one channel was successful + if (deliveryResults.containsValue(true)) { userCourseNotificationStatusService.batchCreateStatusForUsers(setOfNotifiedUsers, courseNotificationEntityId, courseNotification.courseId); + } else if (!deliveryResults.isEmpty()) { + log.warn("Failed to deliver notification through any channel, not creating status entries"); + } }
🧹 Nitpick comments (9)
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.java (3)
32-39: Direct usage of ReflectionTestUtils exposes implementation details.While the test effectively validates the behavior when an unknown notification type is requested, using ReflectionTestUtils to invoke private methods creates tight coupling to implementation details. Consider testing through public API methods instead.
- Class<? extends CourseNotification> result = ReflectionTestUtils.invokeMethod(courseNotificationRegistryService, "getNotificationClass", unknownTypeId); + // Access through public API methods instead, such as: + // Class<? extends CourseNotification> result = courseNotificationRegistryService.findNotificationClassByTypeId(unknownTypeId);
41-48: Test validates correct behavior for unknown notification identifier.The test correctly verifies that null is returned when requesting an unknown notification identifier. However, it also uses ReflectionTestUtils to access private methods.
50-60: Test properly validates the mapping between type ID and notification class.The test effectively sets up the internal state and verifies the expected behavior. Consider moving the creation of test data into the helper methods to reduce duplication and improve maintainability.
@Test void shouldReturnCorrectClassWhenRequestingKnownTypeId() { Short knownTypeId = (short) 1; - Class<? extends CourseNotification> expectedClass = createTestNotificationClass(); - Map<Short, Class<? extends CourseNotification>> notificationTypes = Map.of(knownTypeId, expectedClass); + Class<? extends CourseNotification> expectedClass = createTestNotificationClass(); + Map<Short, Class<? extends CourseNotification>> notificationTypes = createNotificationTypesMap(knownTypeId, expectedClass); ReflectionTestUtils.setField(courseNotificationRegistryService, "notificationTypes", notificationTypes); Class<? extends CourseNotification> result = ReflectionTestUtils.invokeMethod(courseNotificationRegistryService, "getNotificationClass", knownTypeId); assertThat(result).isEqualTo(expectedClass); } + /** + * Helper method to create a notification types map + */ + private Map<Short, Class<? extends CourseNotification>> createNotificationTypesMap(Short typeId, Class<? extends CourseNotification> notificationClass) { + return Map.of(typeId, notificationClass); + }src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java (2)
71-100: Good testing strategy with mock service, consider verifying the method call.Your approach of using a no-op service implementation is appropriate for testing storage and retrieval in isolation.
Consider using a spy or adding a simple counter to verify the service method was actually called:
CourseNotificationBroadcastService noopService = new CourseNotificationBroadcastService() { + private int callCount = 0; @Override protected void sendCourseNotification(CourseNotificationDTO courseNotification, List<User> recipients) { - // Do nothing + callCount++; } + + public int getCallCount() { + return callCount; + } }; // Later after service call +// Assert the service was actually called +assertTrue(((CourseNotificationBroadcastService)mockServiceMap.get(NotificationSettingOption.WEBAPP)).getCallCount() > 0);
109-126: Optimize test performance for bulk operations.Creating 20 notifications in a loop with individual save operations could impact test performance.
Consider reducing the number of test entities or implementing batch saving if the repository supports it:
-for (int i = 0; i < 20; i++) { +// Reduce test data size to minimum needed to verify pagination +for (int i = 0; i < pageSize + 1; i++) { var courseNotification = new CourseNotification(course, (short) 1, ZonedDateTime.now(), ZonedDateTime.now()); - - courseNotificationRepository.save(courseNotification); - var userCourseNotificationStatus = new UserCourseNotificationStatus(courseNotification, user, UserCourseNotificationStatusType.UNSEEN); - - userCourseNotificationStatusTestRepository.save(userCourseNotificationStatus); + courseNotification.setUserCourseNotificationStatus(Set.of(userCourseNotificationStatus)); + notifications.add(courseNotification); } +// Save all notifications in a single batch +courseNotificationRepository.saveAll(notifications);src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.java (1)
54-62: Add error logging for missing user presets.When no preset is found, the code falls back to default settings, but this situation isn't logged, which might mask configuration issues.
Add logging when no preset exists for a user:
if (preset == null) { + log.debug("No notification preset found for user {} and course {}. Using default preset.", + recipient.getId(), notification.courseId); // Run query on default preset if none are present return this.courseNotificationSettingPresetRegistryService.isPresetSettingEnabled(1, notification.getClass(), filterFor); }src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java (3)
73-116: Improve error handling in reflection code.The current exception handling wraps original exceptions in RuntimeExceptions, which may obscure important error details.
Enhance exception handling:
try { // Type conversion logic... } catch (IllegalAccessException | NumberFormatException e) { - throw new RuntimeException("Error assigning value to field: " + field.getName(), e); + log.error("Failed to parse parameter value for field {}: {}", field.getName(), e.getMessage()); + // Either rethrow with more context or just continue to next field + throw new IllegalStateException("Failed to parse parameter '" + field.getName() + + "' with value '" + value + "'. See cause for details.", e); }
126-153: Consider always rebuilding parameter map.The check for non-empty parameters could cause issues if the map is externally modified.
Remove the early return to ensure the map is always consistent:
private void initializeParameterMap() { - if (!parameters.isEmpty()) { - return; - } + // Clear existing parameters to ensure we build a fresh map + parameters.clear(); for (Field field : this.getClass().getDeclaredFields()) { // Field processing logic... } parameters.put("courseTitle", courseTitle); parameters.put("courseIconUrl", courseIconUrl); }
49-55: Use initial capacity for HashMap when size is predictable.The parameters HashMap is created without an initial capacity, which may cause rehashing if many parameters are added.
Specify initial capacity based on expected field count:
-this.parameters = new HashMap<>(); +// Allocate capacity based on typical field count plus a buffer +this.parameters = new HashMap<>(10);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java
- src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingServiceTest.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/coursenotification/service/CourseNotificationSettingService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.javasrc/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.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/coursenotification/web/CourseNotificationResourceIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: Analyse
🔇 Additional comments (16)
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingPresetRegistryServiceTest.java (6)
1-34: Well structured test class setup with proper imports and initialization.The test class is correctly set up with appropriate imports, MockitoExtension for JUnit 5, and a clean BeforeEach method to initialize the service under test. The use of static imports for assertions and mockito methods follows good practices.
35-44: Good test for the non-existent preset scenario.This test correctly verifies the behavior when a preset ID doesn't exist in the registry. The test uses clear descriptive naming that follows the "should_when" pattern, which aligns with the test_naming guidelines. The test is also appropriately sized and focused on a single scenario.
A few observations:
- The test uses ReflectionTestUtils to invoke a likely private method, which is an appropriate approach for unit testing
- The assertion uses AssertJ's assertThat as required by the guidelines
- The Boolean.TRUE.equals wrapper is used to handle potential null return values safely
46-61: Well-designed test for the enabled setting scenario with proper mocking.This test effectively validates that the method returns true when a preset exists and the setting is enabled. The test follows good practices by:
- Using descriptive naming
- Setting up test data with fixed values
- Using static mocks as recommended in the guidelines
- Properly injecting the mock into the service using ReflectionTestUtils
- Making specific assertions with AssertJ
63-78: Good test coverage for the disabled setting scenario.This test complements the previous one by verifying the opposite case - when a preset exists but the setting is disabled. The test structure mirrors the previous test with appropriate changes to the mock behavior, which promotes consistency in the test suite.
80-95: Excellent parameter verification test.This test verifies that the correct parameters are passed to the preset's isEnabled method, which is a crucial aspect of the service's behavior. Unlike the previous test that used any() matchers, this one correctly uses the actual parameters to ensure they're properly passed through.
97-118: Well-implemented test notification class with all required overrides.The TestNotification static inner class provides a concrete implementation of CourseNotification for testing purposes. The implementation:
- Is properly annotated with @CourseNotificationType
- Extends the abstract CourseNotification class
- Implements all required methods with reasonable test values
- Returns fixed data as recommended in the guidelines
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationRegistryServiceTest.java (5)
1-16: Import organization looks good.The imports are well organized and properly grouped. The static imports for assertions and mockito come first, followed by Java standard libraries and then framework-specific imports.
22-31: Test class setup follows good practices.The class is properly annotated with MockitoExtension and has a clear setup method. The service initialization in the @beforeeach method follows the recommended approach for test setup.
62-72: Test correctly validates the reverse mapping functionality.The test for retrieving the correct identifier when provided with a known class is well-implemented. Similar to the previous test, consider using helper methods to improve maintainability.
74-86: Helper methods improve test readability.The helper methods are well-documented and provide clear separation of concerns for creating test and mock notification classes. This follows good testing practices.
88-109: Test notification class is well-implemented with all required overrides.The TestNotification class properly extends CourseNotification and provides implementations for all required methods. The @CourseNotificationType annotation with ID 1 matches the test data used in the test methods.
src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java (2)
55-69:❓ Verification inconclusive
Ensure notification type mapping is consistent.
The test creates a notification with type (short) 1 and then expects "newPostNotification" in the response, but this mapping is implicit. Consider adding a more explicit verification.
Run this to check the consistency of notification type mapping:
🏁 Script executed:
#!/bin/bash # Search for the definition of notification type 1 ast-grep --pattern 'class $_ { $$$@CourseNotificationType($val)$$$ }' | grep -B2 -A2 'CourseNotificationType(1)'Length of output: 238
NOTICE: Verify Notification Type Mapping Consistency
The test case creates a notification with type
(short) 1and then expects"newPostNotification"in the API response. The automated check for an explicit mapping (via the provided shell script) did not yield a definitive result—likely due to non-UTF8 characters in a file withinsrc/test/resources. Please manually verify that the mapping for notification type1indeed corresponds to"newPostNotification"in the codebase. It is recommended to either update the test to explicitly assert the mapping or add documentation/comments to clarify this implicit mapping.
32-54:❓ Verification inconclusive
Test setup looks good but consider additional validation.
The test class setup follows good practices with proper initialization and dependency injection. The test prefix constant helps maintain test data isolation.
Consider verifying that your test user is properly enrolled in the course to ensure realistic test conditions:
🏁 Script executed:
#!/bin/bash # Check if test user enrollment is properly handled in similar test classes rg -A 3 "userUtilService.createAndSave.*\n.*course" --type java src/testLength of output: 266
Action Required: Verify Test User Enrollment
The test setup is well-organized—with proper dependency injection and initialization—but it’s unclear whether the test user is actually enrolled in the course to simulate realistic conditions. While the original suggestion to verify enrollment is still valid, our initial regex-based check using newline matching encountered issues. Please re-run the verification with multiline mode enabled or manually check if other similar test classes include an explicit user enrollment step. For example, you might run:
#!/bin/bash # Re-verify test user enrollment with multiline mode enabled rg -U -A 3 'userUtilService\.createAndSaveUser\s*\(.*\);\n\s*courseUtilService\.createCourse\s*\(' --type java src/testIf no output is returned, please confirm manually whether the enrollment logic (or a similar validation) is incorporated in your test flows.
- File:
src/test/java/de/tum/cit/aet/artemis/coursenotification/web/CourseNotificationResourceIntegrationTest.java- Concern: Ensure that the created test user is properly enrolled in the course to better simulate production-like behavior in tests.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.java (1)
24-42: Good use of constructor injection and service organization.The service follows best practices by using constructor injection for dependencies and clearly defining its responsibility for notification settings management.
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java (1)
36-64: Good service implementation with clear responsibility.The service is well-structured with proper dependency injection and clear separation of responsibilities.
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotification.java (1)
34-56: Good class design with clear abstract API.The abstract class provides a solid foundation for notification types with appropriate field encapsulation and clear constructors.
...java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationSettingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java (1)
55-65: Consider reducing constructor parameters.The constructor injects many dependencies, hinting at potential role overlap in this class. Consolidating or refactoring some of these responsibilities may improve readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java(1 hunks)
🧰 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/coursenotification/service/CourseNotificationService.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: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationService.java (3)
74-93: Revisit partial success scenario and transaction boundary.This code can cause a mismatch between stored notifications and broadcast success in case of channel failures. Consider wrapping logic in a transactional boundary or adding robust fallback/error handling to prevent partial successes.
123-135: Enhance reflection error handling and consider caching constructors.Returning null for all reflection-related failures can lead to missed notifications or null entries downstream. Additionally, repeated reflection may impact performance for larger notification volumes. You could cache the constructor handle and handle errors more gracefully, as previously suggested.
[duplicate_comment, refactor_suggestion_good_to_have]
190-203: Batch saving of parameters looks great.This addresses past feedback regarding efficiency in persisting many parameters at once. Good job implementing the batching logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.java (5)
122-124: Avoid raw type casting in generic argumentsThe raw type cast
(Class) Iterable.classcan lead to type safety issues. Consider using a parameterized type instead.-ArgumentCaptor<Iterable<CourseNotificationParameter>> captor = ArgumentCaptor.forClass((Class) Iterable.class); +ArgumentCaptor<Iterable<CourseNotificationParameter>> captor = ArgumentCaptor.forClass(Iterable.class);
141-141: Avoid raw type casting for class referencesSimilar to the previous issue, using raw type casts like
(Class) TestNotification.classcan lead to type safety warnings.-when(courseNotificationRegistryService.getNotificationClass(any())).thenReturn((Class) TestNotification.class); +when(courseNotificationRegistryService.getNotificationClass(any())).thenReturn(TestNotification.class);
160-160: Consider testing through public APIs instead of reflection when possibleUsing
ReflectionTestUtilsto test private methods is sometimes necessary, but it can make tests more fragile to implementation changes. Where possible, consider testing the behavior through public methods.
174-176: Extract test constants for improved maintainabilityConsider extracting repeated values like the course ID
123Land parameter keys into constants to make the tests more maintainable.+ private static final long TEST_COURSE_ID = 123L; + private static final Map<String, String> TEST_PARAMETERS = Map.of("key1", "value1", "key2", "value2"); private TestNotification createTestNotification(NotificationSettingOption... supportedChannels) { - return new TestNotification(123L, ZonedDateTime.now(), Map.of("key1", "value1", "key2", "value2"), supportedChannels); + return new TestNotification(TEST_COURSE_ID, ZonedDateTime.now(), TEST_PARAMETERS, supportedChannels); }
1-228: Consider adding error scenario testsThe current tests cover the happy path scenarios well, but there are no tests for error handling. Consider adding tests that verify the service's behavior when dependencies throw exceptions or when invalid data is provided.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.java(1 hunks)
🧰 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/coursenotification/service/CourseNotificationServiceTest.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/test/java/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationServiceTest.java (6)
42-78: Well-structured test setup with appropriate mocking!You've correctly used
@ExtendWith(MockitoExtension.class)and properly set up all dependencies with@Mockannotations, following the static mocks strategy. The service initialization insetUp()is clean and follows the dependency injection pattern.
80-101: Good test coverage for multi-channel notification delivery!This test effectively verifies that notifications are sent to the appropriate channels based on user settings and that the right recipients are filtered for each channel. The use of
ArgumentCaptorfor verification is excellent.
127-151: Test methods follow best practices with descriptive names and appropriate assertionsThe test methods have descriptive names that clearly indicate what's being tested, and they use
assertThat()with specific assertions as required by the guidelines. Each test is focused on a specific aspect of the service's functionality.Also applies to: 153-165
192-226: Well-designed test class implementationThe
TestNotificationinner class is well-structured with clear implementations of the required methods. The inclusion of a specific constructor comment for test case reference is great for maintainability.
80-228: Tests comply well with coding guidelinesYour tests follow the required guidelines well:
- Descriptive test names
- Small, focused tests
- Fixed test data
- JUnit 5 features
- Proper use of assertThat with specific assertions
- No direct DB access (using mocks instead)
- Static mocks strategy with Mockito
148-148:✅ Verification successful
Verify compatibility with Java version for
getFirst()The method
getFirst()was introduced in more recent Java versions. If you're using an older Java version, consider usingget(0)instead for better compatibility.
🏁 Script executed:
#!/bin/bash # Check Java version used in the project java -version 2>&1 | head -1 # Look for other uses of getFirst() vs get(0) in the codebase rg -A 1 -B 1 'getFirst\(\)' --type java rg -A 1 -B 1 'get\(0\)' --type javaLength of output: 66417
Java 17 usage confirmed – no changes required
The project is using OpenJDK 17 (as shown by the version output) and there are many instances of
getFirst()throughout the codebase. This widespread usage confirms thatgetFirst()is fully supported in the current environment, so there’s no need to switch toget(0).
.../tum/cit/aet/artemis/coursenotification/domain/notifications/CourseNotificationCategory.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/coursenotification/domain/CourseNotification.java
Show resolved
Hide resolved
...va/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.java
Show resolved
Hide resolved
...java/de/tum/cit/aet/artemis/coursenotification/domain/notifications/NewPostNotification.java
Show resolved
Hide resolved
...va/de/tum/cit/aet/artemis/coursenotification/service/CourseNotificationPushProxyService.java
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
Note: To help with understanding the design choices for the notification system, it may be useful to consult the Figma Designs.
Checklist
General
Server
Motivation and Context
The current implementation of the Artemis notification system distributes notifications globally across all courses. Though this approach provides basic functionality, it presents some user experience limitations. Users frequently encounter difficulty in rapidly determining the specific course context for each notification.
Furthermore, the system employs a single datetime column in the database to track notification viewing status. This architecture creates limitations in monitoring which specific notifications have been viewed by individual users, resulting in an incomplete view of user engagement with important communications.
Here a short summary on the current system:
Storage and Delivery: Notifications are stored as single entries in the database. User eligibility for notifications is determined through complex logical queries that rely on the user role, course assignments, tutorial group memberships, columns like the
jhi_users.hide_notifications_untiltimestamp (used to suppress older notifications for newly registered users), etc.Read Status Tracking: The system tracks notification views through the
jhi_users.last_notification_readcolumn, which updates when a user accesses their notifications. This provides a basic "all or nothing" approach to read status tracking.Some submodules implement similar functionality within their models (e.g.,
conversation_participants.last_read) to enable more targeted read status tracking.Limitations:
Key Challenges:
High Coupling: The notification system is tightly coupled with other systems, relying on external objects, services, and models, which impedes maintainability and comprehension
Query Performance: The complex query that determines notification targeting is resource-intensive and slow
Read Status Granularity: The system cannot effectively track which specific notifications have been viewed by which users
Settings Reliability: Notification preferences are inconsistently applied, with users receiving notifications despite disabling relevant settings. This stems partly from the design choice of client-side (rather than server-side) notification filtering
Visual Disambiguation: Notifications lack visual indicators to help users quickly identify their associated course
Description
This pull request implements the foundation of the new course-specific notification system on server side. This will be followed up by a foundational client side PR, and lastly by multiple PRs implementing every notification one-by-one.
The redesigned notification system addresses the key challenges through strategic decoupling from other Artemis components. By limiting public interfaces, I aimed for a more maintainable architecture that offers improved functionality while reducing system dependencies.
Sending: For notification distribution, I have implemented a type-based channel declaration approach. The streamlined interface
courseNotificationService.sendCourseNotification(new XYZNotification(), recipientList)processes notifications (such as NewPostNotification), filters recipients based on their preferences, and distributes through appropriate channels as specified by the notification type itself. The consolidated approach eliminates the need for service-specific channel implementations, allowing developers to utilize a single, well-defined interface for comprehensive notification distribution without managing individual channel communications.Recieving: The new reception model now tracks individual notification entries per user, eliminating complex logical queries in favor of direct database relationships. This granular approach enables precise tracking of notification delivery while implementing caching strategies to minimize database traffic. All queries utilize indexing for optimized performance, and database writes are executed as batch operations to reduce table locking by various write commands. Notifications can be retrieved through the simplified interface
courseNotificationService.getCourseNotifications(page, courseId, userId). Additionally, on the course overview we attach the count of unread notifications to each course DTO so that we can display the number per-course.Tracking: The enhanced tracking capabilities allow notifications to be marked as "Seen," "Unseen," or "Archived," giving users greater control over their notification feed and improving interface organization.
Settings: Notification preferences have been redesigned around server-defined presets, implemented as auto-discovered classes that appear in user settings. For users requiring more specific control, granular configuration options are available at the notification type and channel level. Since filtering occurs server-side, clients can reliably display all received notifications without additional filtering logic.
Cleanup: To ensure database efficiency, notifications include a deletion date determined by notification-type intervals. An automated service executes during off-peak hours to remove expired notifications, preventing database bloat while maintaining organized user feeds.
Creating new notifications: The system simplifies notification extension through a straightforward model creation process in the notifications directory. New notifications become available immediately upon interface implementation and annotation, with streamlined localization based on camelCase class naming conventions. The NewPostNotification has been implemented as a reference example and for server testing.
The underlying data structure centers on the "course_notification" table, with notification types stored as TinyInt values and scheduled cleanup managed via the "deletion_date" column. Notification metadata is stored as key-value pairs in the "course_notification_parameter" table, replacing the previous JSON storage approach. User interaction status is tracked in the "user_course_notification_status" table, progressing from "Unseen" to "Seen" and optionally "Archived."
User preference presets are maintained in the "user_course_notification_settings_preset" table, with custom configurations stored in the "user_course_notification_setting_specification" table when needed. All entries maintain course relationships through "course_id" references, ensuring proper organizational context.
The ERM looks like this:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Test Coverage
Server
Summary by CodeRabbit
New Features
Bug Fixes
Tests