Communication: Do not send a notification for own answers in a thread
#10308
Conversation
PaRangger
added
bugfix
communication
github-actions
bot
added
the
server
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
WalkthroughThis pull request refines the notification logic in the application. In the main service, the filtering mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant Author as MessageAuthor
participant Service as SingleUserNotificationService
participant Users as InvolvedUsers
participant Notif as NotificationSystem
Author->>Service: Submit reply message
Service->>Users: Retrieve involved users
loop For each user in conversation
alt User ≠ Author
Service->>Notif: Dispatch notification to user
else
Service-->>Service: Skip notification
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/test/java/de/tum/cit/aet/artemis/communication/AnswerMessageIntegrationTest.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (1)
438-454: Consider enhancing the async implementation and separating concerns.While the core logic is correct, consider these improvements for better maintainability and reliability:
- Add error handling for async execution
- Extract user filtering logic to a separate method
- Use more descriptive variable names (e.g.,
conv→conversation)Here's a suggested refactor:
@Async public void notifyInvolvedUsersAboutNewMessageReply(Post post, SingleUserNotification notification, Set<User> mentionedUsers, AnswerPost savedAnswerMessage, User author) { + try { SecurityUtils.setAuthorizationObject(); // required for async - Set<User> usersInvolved = conversationMessageRepository.findUsersWhoRepliedInMessage(post.getId()); - // do not notify the author of the post if they are not part of the conversation (e.g. if they left or have been removed from the conversation) - if (conversationService.isMember(post.getConversation().getId(), post.getAuthor().getId())) { - usersInvolved.add(post.getAuthor()); - } + Set<User> usersToNotify = findUsersToNotify(post); filterAllowedRecipientsInMentionedUsers(mentionedUsers, post.getConversation()) .forEach(mentionedUser -> notifyUserAboutNewMessageReply(savedAnswerMessage, notification, mentionedUser, author, CONVERSATION_USER_MENTIONED)); - Conversation conv = conversationService.getConversationById(post.getConversation().getId()); - usersInvolved.stream().filter(userInvolved -> !mentionedUsers.contains(userInvolved) && !userInvolved.getId().equals(author.getId())).forEach(userInvolved -> { - notifyUserAboutNewMessageReply(savedAnswerMessage, notification, userInvolved, author, getAnswerMessageNotificationType(conv)); + Conversation conversation = conversationService.getConversationById(post.getConversation().getId()); + usersToNotify.stream() + .filter(userInvolved -> !mentionedUsers.contains(userInvolved) && !userInvolved.getId().equals(author.getId())) + .forEach(userInvolved -> { + notifyUserAboutNewMessageReply(savedAnswerMessage, notification, userInvolved, author, getAnswerMessageNotificationType(conversation)); }); + } catch (Exception e) { + // Log the error and potentially notify monitoring systems + log.error("Failed to send notifications for message reply", e); + } } +private Set<User> findUsersToNotify(Post post) { + Set<User> usersInvolved = conversationMessageRepository.findUsersWhoRepliedInMessage(post.getId()); + // do not notify the author of the post if they are not part of the conversation + // (e.g. if they left or have been removed from the conversation) + if (conversationService.isMember(post.getConversation().getId(), post.getAuthor().getId())) { + usersInvolved.add(post.getAuthor()); + } + return usersInvolved; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/AnswerMessageIntegrationTest.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/AnswerMessageIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: server-tests-postgres
- GitHub Check: server-tests-mysql
- GitHub Check: client-tests
- GitHub Check: Analyse
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
src/test/java/de/tum/cit/aet/artemis/communication/AnswerMessageIntegrationTest.java (5)
133-164: LGTM! Test methods simplified correctly.The removal of the
NotificationTypeparameter from test methods aligns with the PR's objective of refining notification handling. The methods remain focused and follow good testing practices.
166-171: LGTM! Good refactoring for reusability.The change improves code reusability by extracting channel creation logic into a separate helper method, following the DRY principle.
173-189: LGTM! Well-structured tests for notification behavior.The new test methods effectively validate the core functionality:
testSendNotificationWhenDifferentUserAnswersPost: Verifies notifications are sent for different userstestDoNotSendNotificationWhenSameUserAnswersPost: Verifies notifications are suppressed for same userThe tests are focused, well-named, and use appropriate mocking to verify the behavior.
191-197: LGTM! Helper method improved for better reusability.The modification to return the
Channelobject improves the method's reusability while maintaining its core functionality.
199-225: LGTM! Helper method enhanced to support notification testing.The modification to return the
AnswerPostobject enables proper verification in notification tests while maintaining the existing test coverage and assertions.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (1)
450-453: LGTM! The fix correctly prevents self-notifications.The implementation properly filters out the message author from receiving notifications about their own replies, while maintaining notifications for other involved users. This aligns with the existing self-notification prevention logic in the service.
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
krusche
changed the title
Notifications: Fix issue when answering in a threadCommunication: Do not send a notification for own answers in a thread
krusche
deleted the
bugfix/communication/fix-self-notification-on-answer
branch
Checklist
General
Server
Motivation and Context
Currently, when answering in a thread you will get a push notification for you own message. This issue exists on iOS as well as on Android.
Addresses #10225 and this issue in the android repository.
Description
I excluded the author from the list of users to notify when creating an answer post.
Steps for Testing
Prerequisites:
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
Manual Tests
Test Coverage
Server
Summary by CodeRabbit