-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Communication
: Add answer posts content to course wide search
#9638
base: develop
Are you sure you want to change the base?
Communication
: Add answer posts content to course wide search
#9638
Conversation
Communication
: Add answer posts content to course wide search
…course-wide-search
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (2)
63-66
: Consider indexing strategy for performance optimization.
The implementation correctly extends search to answer posts. However, the LEFT JOIN with OR condition might impact query performance on large datasets.
Consider adding the following database optimizations:
- Create a composite index on
answer_post(content, post_id)
to improve JOIN performance - Consider implementing a full-text search index if supported by your database for better search performance
Line range hint 42-121
: Consider caching strategy for frequently accessed posts.
The enhanced search functionality now requires additional database joins. To maintain performance as the system scales:
- Consider implementing result caching for frequently accessed posts
- Monitor query execution plans and response times after deployment
- Consider implementing pagination if not already present in the calling code
src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java (1)
524-542
: Enhance assertion specificity in the test method.
The test verifies search functionality for course-wide messages, but the assertions could be more specific:
Consider adding these assertions to better validate the search results:
List<Post> returnedPosts = request.getList("/api/courses/" + courseId + "/messages", HttpStatus.OK, Post.class, params);
-assertThat(returnedPosts).hasSize(existingCourseWideMessages.stream().filter(post -> post.getConversation().getId().equals(courseWideChannelId)
- && post.getAuthor().getLogin().equals(TEST_PREFIX + "student1") && (post.getContent().contains("Content") || answerHasContext(post.getAnswers(), "Content")))
- .toList().size());
+// Verify the size
+assertThat(returnedPosts).hasSize(expectedSize);
+// Verify each returned post matches the search criteria
+assertThat(returnedPosts).allMatch(post ->
+ post.getConversation().getId().equals(courseWideChannelId) &&
+ post.getAuthor().getLogin().equals(TEST_PREFIX + "student1") &&
+ (post.getContent().contains("Content") || answerHasContext(post.getAnswers(), "Content"))
+);
+// Verify posts are from course-wide channel
+assertThat(returnedPosts).allMatch(post ->
+ post.getConversation() instanceof Channel && ((Channel) post.getConversation()).getIsCourseWide()
+);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1)
Pattern src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java (1)
Pattern src/test/java/**/*.java
: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (2)
42-43
: LGTM! Documentation accurately reflects the enhanced search functionality.
The updated Javadoc clearly explains that both messages and answer posts are now included in the search results.
Line range hint 107-121
: Verify test coverage for answer post filtering.
The implementation correctly extends the own posts filter to include authored answer posts. However, we should ensure comprehensive test coverage.
Let's verify the test coverage:
✅ Verification successful
Let me search for test files related to this functionality.
Let me check the test implementation details to verify if the answer post filtering is properly tested.
Let me check the test implementation in AnswerMessageIntegrationTest since it seems more relevant to our verification.
Based on the test files analyzed, I can now generate the final response:
Test coverage for answer post filtering is comprehensive and verified
The implementation is well-tested through multiple test cases in both MessageIntegrationTest and AnswerMessageIntegrationTest:
- Tests verify filtering own posts with answer content search
- Tests cover answer post creation and association
- Integration tests validate the complete flow including:
- Filtering by own posts with answers
- Answer post content verification
- Answer post author verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering the getOwnSpecification method
rg -A 10 "getOwnSpecification.*filterToOwn.*userId" --type java
Length of output: 3300
Script:
#!/bin/bash
# Search for test files related to MessageSpecs and ConversationMessageRepository
fd -e java -g "*Test.java" | xargs rg -l "MessageSpecs|ConversationMessageRepository"
# Search for test cases that might test the own post filtering functionality
rg -A 10 "filterToOwn|getOwnSpecification" --type java --glob "*Test.java"
Length of output: 16722
Script:
#!/bin/bash
# Search for test setup and assertions related to answer posts in MessageIntegrationTest
rg -B 10 -A 10 "answerHasContext|getAnswers" src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java
# Look for test data setup in AbstractConversationTest
rg -B 5 -A 10 "existingCourseWideMessages|setupTestData" src/test/java/de/tum/cit/aet/artemis/communication/AbstractConversationTest.java
Length of output: 6945
Script:
#!/bin/bash
# Search for test setup and assertions specifically related to answer posts in AnswerMessageIntegrationTest
rg -B 15 -A 5 "student1\.getId\(\)\.equals\(post\.getAuthor\(\)\.getId\(\))" src/test/java/de/tum/cit/aet/artemis/communication/AnswerMessageIntegrationTest.java
# Look for test data setup related to answer posts
rg -B 5 -A 10 "setupAnswerPost|createAnswerPost" src/test/java/de/tum/cit/aet/artemis/communication/AnswerMessageIntegrationTest.java
Length of output: 9298
src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS3. I presume you meant messages in communication by "posts". Please correct me if I was testing the wrong thing. Everything works as described. One small thing though: After done searching for something then click on the Refresh button, I expect the origin view of the channel would show up with all the messages. But instead still my search result show up, it would only revert back to the original view after I click on another channel then go back, or by emptying the search bar and click on the search symbol (which happened to work in this case because I don't have any other public posts on other channel)
Untitled.video.-.Made.with.Clipchamp.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good apart from one difficult to understand part
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1)
119-122
: Consider extracting common join logic.
The implementation is correct, but there's potential code duplication with the join creation pattern seen in getSearchTextSpecification
. Consider extracting the common join logic into a reusable helper method.
Consider adding a helper method:
+private static Join<Post, AnswerPost> getAnswerPostJoin(Root<Post> root) {
+ return root.join(Post_.ANSWERS, JoinType.LEFT);
+}
public static Specification<Post> getOwnSpecification(boolean filterToOwn, Long userId) {
return ((root, query, criteriaBuilder) -> {
if (!filterToOwn) {
return null;
}
else {
- Join<Post, AnswerPost> answersJoin = root.join(Post_.ANSWERS, JoinType.LEFT);
+ Join<Post, AnswerPost> answersJoin = getAnswerPostJoin(root);
Predicate searchInAnswerContent = criteriaBuilder.equal(answersJoin.get(AnswerPost_.AUTHOR).get(User_.ID), userId);
Predicate isPostOwner = criteriaBuilder.equal(root.get(Post_.AUTHOR).get(User_.ID), userId);
return criteriaBuilder.or(isPostOwner, searchInAnswerContent);
}
});
}
This helper method could also be used in getSearchTextSpecification
and other methods that need to join with answer posts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (1)
Pattern src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java (2)
42-43
: LGTM! Documentation accurately reflects the new functionality.
The updated Javadoc clearly describes that both messages and answer posts are now included in the search functionality.
63-66
: Verify database performance impact.
The addition of LEFT JOINs in both specifications might affect query performance. Please ensure that:
- Appropriate indexes exist on the joined columns
- The query execution plan remains efficient
Also applies to: 119-122
✅ Verification successful
Performance impact is minimal, no database changes required
The LEFT JOIN on answers is safe because:
- The Post entity already eagerly loads answers (
@OneToMany(fetch = FetchType.EAGER)
) - The repository uses optimized query patterns:
- First fetches only IDs with specifications
- Then loads full entities with a single optimized query using
findByPostIdsWithEagerRelationships
- The existing eager loading query already includes answers with their relationships:
LEFT JOIN FETCH p.answers a LEFT JOIN FETCH a.reactions LEFT JOIN FETCH a.post LEFT JOIN FETCH a.author
The search specification change aligns with the existing query optimization strategy and won't degrade performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing database indexes and constraints
echo "Searching for database index definitions..."
fd -e sql -e java | xargs rg -l "CREATE.*INDEX|@Index"
echo "Searching for existing query performance tests..."
fd -e java | xargs rg -l "MessageSpecs.*getSearchTextSpecification|MessageSpecs.*getOwnSpecification.*performance"
Length of output: 476
Script:
#!/bin/bash
# Let's check the content of the repository file found and related entity classes
echo "Checking ConversationMessageRepository content..."
cat src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java
echo -e "\nChecking Post and AnswerPost entity definitions..."
fd "Post.java|AnswerPost.java" --type f --exec cat {}
echo -e "\nChecking for any existing JPA indexes in the codebase..."
rg "@Index|@Table\(.*indexes" -A 3
Length of output: 18504
src/main/java/de/tum/cit/aet/artemis/communication/repository/MessageSpecs.java
Show resolved
Hide resolved
|
|
|
|
|
|
|
…course-wide-search
Checklist
General
Server
Motivation and Context
Currently, the search only filters the content of the original post and also excludes private conversations a user migh have
Description
I removed the private channel check and added answer posts, where the original post does not match the content but an answer post matches, to the search result
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests