Communication: Fix an issue for answers in unnamed group chats
#10424
Conversation
FelberMartin
added
server
WalkthroughThe change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant AMS as AnswerMessageService
participant CS as ConversationService
AMS ->> CS: loadConversationWithParticipantsIfGroupChat(conversationId, ...)
CS -->> AMS: Returns conversation with participants if group chat
Suggested labels
Suggested reviewers
✨ 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/AnswerMessageService.java (1)
92-94: Be cautious withorElsevsorElseGetin the conversation loading logicThe implementation uses
orElse()which will always evaluate theloadConversationWithParticipantsIfGroupChat()method regardless of whether the first method returns a value. This could lead to unnecessary database operations.Consider using
orElseGet()instead:- var conversation = conversationService.isMemberOrCreateForCourseWideElseThrow(conversationId, author, Optional.empty()) - .orElse(conversationService.loadConversationWithParticipantsIfGroupChat(conversationId)); + var conversation = conversationService.isMemberOrCreateForCourseWideElseThrow(conversationId, author, Optional.empty()) + .orElseGet(() -> conversationService.loadConversationWithParticipantsIfGroupChat(conversationId));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/AnswerMessageService.java(2 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/communication/service/AnswerMessageService.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/AnswerMessageService.java (1)
64-64: Clear removal of unnecessary dependencyThe removal of the
ConversationRepositoryparameter from the constructor aligns with dependency injection best practices by eliminating unused dependencies. This change maintains the single responsibility principle while reducing the complexity of the service's initialization.
FelberMartin
changed the title
Communication: Fix error for answer in group chat
src/main/java/de/tum/cit/aet/artemis/communication/service/AnswerMessageService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/service/AnswerMessageService.java (3)
64-64: Consider reducing constructor dependencies for cleaner design.
Removing theConversationRepositorydependency is consistent with delegating conversation logic toConversationService. However, the constructor parameter list remains quite large. Consider consolidating dependencies or employing a design pattern (e.g., a service aggregator) to reduce complexity.
93-93: Clarify the rationale behind group-chat participant loading.
This comment is helpful but could be slightly expanded to explain the dependency on participants for generating conversation titles, making future maintenance more intuitive.
94-95: Consider encapsulating fallback logic in a single method.
ChainingisMemberOrCreateForCourseWideElseThrow(...).orElse(loadConversationWithParticipantsIfGroupChat(...))is functionally correct but slightly cumbersome. You might unify both calls in a dedicated service method for improved readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/AnswerMessageService.java(2 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/communication/service/AnswerMessageService.java
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyse
- GitHub Check: client-style
- GitHub Check: server-tests
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/AnswerMessageService.java (1)
92-92: Verify non-null conversation object.
answerMessage.getPost().getConversation()might be null if a conversation is not set on the post, causing potential runtime errors when invoking.getId().
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
krusche
changed the title
Communication: Fix error for answer in group chatCommunication: Fix an issue for answers in unnamed group chat
krusche
changed the title
Communication: Fix an issue for answers in unnamed group chatCommunication: Fix an issue for answers in unnamed group chats
Checklist
General
Server
Motivation and Context
There has been an internal server error for creating answer posts in a group chat without a name.
This PR closes #10423
Description
Similar to the creation of a new base post, we use the
conversationService.loadConversationWithParticipantsIfGroupChat()function to get the participants for a group chat (the participants are needed to construct the conversation title for the notifications.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
Summary by CodeRabbit