Communication: Fix message search not showing results from some conversations
#10345
Conversation
github-actions
bot
added
tests
server
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
WalkthroughThe pull request refines the filtering logic for course-wide messages and adjusts UI rendering along with adding a new integration test. In the Changes
Possibly related PRs
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/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java (1)
760-784: Well-structured test covering the core functionality.The test effectively verifies that the message search includes results from both direct chats and non-course-wide channels, which is crucial for the PR's objective.
A few suggestions to enhance the test:
- Add negative test cases
- Test with empty search results
- Test with special characters in search text
@Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testFindCourseWideMessages_IncludesDirectChatsAndNonCourseWideChannels() throws Exception { + // Test with messages that should be found Post directPost = createPostWithOneToOneChat(TEST_PREFIX); directPost.setContent("SearchTestDirect"); request.postWithResponseBody("/api/courses/" + courseId + "/messages", directPost, Post.class, HttpStatus.CREATED); Channel nonCourseWideChannel = conversationUtilService.createPublicChannel(course, "group-chat-test"); conversationUtilService.addParticipantToConversation(nonCourseWideChannel, TEST_PREFIX + "student1"); conversationUtilService.addParticipantToConversation(nonCourseWideChannel, TEST_PREFIX + "student2"); Post groupPost = new Post(); groupPost.setAuthor(userTestRepository.findOneByLogin(TEST_PREFIX + "student1").orElseThrow()); groupPost.setConversation(nonCourseWideChannel); groupPost.setContent("SearchTestGroup"); request.postWithResponseBody("/api/courses/" + courseId + "/messages", groupPost, Post.class, HttpStatus.CREATED); + // Test with a message that should not be found + Post irrelevantPost = createPostWithOneToOneChat(TEST_PREFIX); + irrelevantPost.setContent("IrrelevantContent"); + request.postWithResponseBody("/api/courses/" + courseId + "/messages", irrelevantPost, Post.class, HttpStatus.CREATED); + PostContextFilterDTO filter = new PostContextFilterDTO(course.getId(), new long[] {}, null, null, "SearchTest", false, false, false, PostSortCriterion.ANSWER_COUNT, SortingOrder.DESCENDING); var student1 = userTestRepository.findOneByLogin(TEST_PREFIX + "student1").orElseThrow(); Page<Post> searchResults = conversationMessageRepository.findCourseWideMessages(filter, Pageable.unpaged(), student1.getId()); List<Post> resultPosts = searchResults.getContent(); assertThat(resultPosts).extracting(Post::getContent).contains("SearchTestDirect", "SearchTestGroup"); + assertThat(resultPosts).extracting(Post::getContent).doesNotContain("IrrelevantContent"); + + // Test with no results + filter = new PostContextFilterDTO(course.getId(), new long[] {}, null, null, "NonExistentText", false, false, false, PostSortCriterion.ANSWER_COUNT, + SortingOrder.DESCENDING); + searchResults = conversationMessageRepository.findCourseWideMessages(filter, Pageable.unpaged(), student1.getId()); + assertThat(searchResults.getContent()).isEmpty(); + + // Test with special characters + Post specialCharPost = createPostWithOneToOneChat(TEST_PREFIX); + specialCharPost.setContent("Search$Test#Special"); + request.postWithResponseBody("/api/courses/" + courseId + "/messages", specialCharPost, Post.class, HttpStatus.CREATED); + + filter = new PostContextFilterDTO(course.getId(), new long[] {}, null, null, "Search$Test#", false, false, false, PostSortCriterion.ANSWER_COUNT, + SortingOrder.DESCENDING); + searchResults = conversationMessageRepository.findCourseWideMessages(filter, Pageable.unpaged(), student1.getId()); + assertThat(searchResults.getContent()).extracting(Post::getContent).contains("Search$Test#Special"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java(1 hunks)src/main/webapp/app/shared/metis/post/post.component.html(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/metis/post/post.component.html
`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/repository/ConversationMessageRepository.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/MessageIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java (1)
89-89: LGTM! Type declaration improvement and simplified filtering.The change correctly removes redundant filtering and makes the type declaration more explicit, which improves code readability and allows message search to include results from all conversation types.
src/main/webapp/app/shared/metis/post/post.component.html (1)
45-45: LGTM! Improved UI by preventing empty channel references.The additional check
contextInformation.displayName !== ''prevents displaying empty channel references, which improves the user experience.
| SortingOrder.DESCENDING); | ||
|
|
||
| var student1 = userTestRepository.findOneByLogin(TEST_PREFIX + "student1").orElseThrow(); | ||
| Page<Post> searchResults = conversationMessageRepository.findCourseWideMessages(filter, Pageable.unpaged(), student1.getId()); |
There was a problem hiding this comment.
Since you save via a post request, would it also make sense to use a get request to get the actual searchResults, instead of simply using the repository?
| @@ -756,6 +757,31 @@ void testGetCourseWideMessagesWithPinnedOnly() throws Exception { | |||
| assertThat(allPosts).extracting(Post::getId).containsExactlyInAnyOrder(createdPinnedPost.getId(), createdUnpinnedPost.getId()); | |||
| } | |||
|
|
|||
| @Test | |||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | |||
| void testFindCourseWideMessages_IncludesDirectChatsAndNonCourseWideChannels() throws Exception { | |||
There was a problem hiding this comment.
Since you are testing if this works for directChats, and courseWideChannels, would it make sense to split this into two tests, for maintainability?
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
There was a problem hiding this comment.
This doesn't quite work as expected yet. I noticed two problems while testing on TS2:
Search results are inconsistent
Steps to reproduce:
- Log in on TS2 as artemis_test_user_16
- Go to the chat with me, Simon Martin Bohnen
- Search for 'Servus' in the main search bar
- Verify that the message from the DM with me shows up
- Go to the 'organization' channel
- Search for 'test'
- Search for 'servus'
- Verify that the message from my DM does NOT show up
It's not possible to go to the corresponding DM from the search results
There is a link to the channel if the message is from a channel, but there is no link if the message is from a DM. This might be a separate issue.
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
Checklist
General
Server
Motivation and Context
The message search, which is supposed to work for messages from all conversations, currently only works for channels marked as
isCourseWidein the database. It does not return results for non-course-wide channels, direct chats, or group chats.(Closes #10290)
Description
This PR fixes the issue by ensuring that message search retrieves results from all relevant conversations, including direct and group chats, as well as non-course-wide channels.
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
Screenshots
search message results for a direct chat and a non-course-wide channel
Summary by CodeRabbit