Communication: Fix issue that allows saving same post multiple times
#10388
Conversation
PaRangger
added
small
bugfix
communication
github-actions
bot
added
the
server
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
WalkthroughThe pull request updates how saved posts are managed across the repository, service, and test layers. A new repository method is introduced to retrieve multiple saved posts based on user ID, post ID, and posting type, and existing methods now include parameter annotations. The service layer has been revised to handle lists of saved posts instead of a single instance, with corresponding updates in save, remove, and update operations. Test cases have been aligned with these changes to ensure consistency in returned values. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as SavedPostService
participant R as SavedPostRepository
participant DB as Database
U->>S: Initiate saved post action (save/remove/update)
S->>R: Call findSavedPostsByUserIdAndPostIdAndPostType()
R->>S: Return list of SavedPost(s)
alt List is empty (save operation)
S->>R: Save new post
R->>DB: Persist new saved post
DB-->>R: Acknowledgement
R-->>S: Confirmation
else List is non-empty (remove/update)
loop For each SavedPost
S->>R: Update/Delete SavedPost
R->>DB: Persist changes
DB-->>R: Acknowledgement
end
R-->>S: Confirmation
end
S->>U: Return operation result
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: 1
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/communication/repository/SavedPostRepository.java (1)
96-99: Consider adding an index to optimize the grouped query.The query effectively prevents duplicates by grouping saved posts. However, for optimal performance, consider adding a composite index on (user_id, status, completed_at).
CREATE INDEX idx_saved_post_user_status_completed ON saved_post (user_id, status, completed_at DESC);src/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java (5)
42-44: Rename variable to reflect multiple saved posts
The variableexistingSavedPostis aList<SavedPost>and should be pluralized (e.g.,existingSavedPosts) for clarity.- var existingSavedPost = this.getSavedPostForCurrentUser(post); - if (!existingSavedPost.isEmpty()) { + var existingSavedPosts = this.getSavedPostForCurrentUser(post); + if (!existingSavedPosts.isEmpty()) { return; }
59-63: Consider transactional or concurrency safeguards
While this logic is correct, concurrently calling this operation could lead to partial deletes or race conditions. You might want to ensure this method is annotated with a transactional boundary or otherwise handled to avoid concurrency pitfalls.
65-68: Consider bulk delete to simplify removal logic
Deleting posts one by one is valid but may be less efficient in certain database contexts. Implementing a bulk delete method in the repository could streamline the query and reduce overhead.
86-91: Unify repeated update logic for improved maintainability
Repeatedly updating eachSavedPostis correct but consider centralizing this logic (e.g., in a helper) or using a bulk operation if you anticipate large lists. This can improve performance and clarity while still ensuring cache invalidation, if applicable.
124-128: Rename method to clearly indicate list return type
The method namegetSavedPostForCurrentUserreturns a list. Renaming it togetSavedPostsForCurrentUsercould make the intent clearer.- private List<SavedPost> getSavedPostForCurrentUser(Posting post) { + private List<SavedPost> getSavedPostsForCurrentUser(Posting post) { ... }
📜 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/repository/SavedPostRepository.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java(4 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/repository/SavedPostRepository.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/repository/SavedPostRepository.java (2)
40-41: LGTM! Documentation improvement helps prevent misuse.The updated documentation clearly warns about potential multiple entries and guides developers to use this method only when a single entry is expected.
51-60: LGTM! New method effectively addresses duplicate posts issue.The method is well-documented and properly designed to handle multiple saved posts for the same user/post/type combination, directly addressing the PR's objective.
src/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java (1)
118-119: Documentation accurately reflects multiple rows
The updated doc comment clarifies that multiple saved posts may exist for the same posting. This improvement is helpful for maintainability.
src/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/SavedPostRepository.java (1)
83-84: Consider adding indexes to optimize the grouped query.The GROUP BY clause effectively prevents duplicates, but consider adding indexes on (user_id, post_id, post_type) to optimize query performance.
CREATE INDEX idx_saved_post_group ON saved_post (user_id, post_id, post_type);
📜 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/repository/SavedPostRepository.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/test_repository/SavedPostTestRepository.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`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/test_repository/SavedPostTestRepository.java
`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/SavedPostRepository.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: Analyse
🔇 Additional comments (4)
src/main/java/de/tum/cit/aet/artemis/communication/repository/SavedPostRepository.java (2)
38-47: LGTM! Well-documented method for handling duplicate posts.The new method effectively supports the PR's objective of managing duplicate saved posts by allowing retrieval of all saved posts for a specific user, post, and type combination.
86-86: LGTM! Clear parameter annotations.The @param annotations improve code clarity and follow SQL parameter annotation guidelines.
src/test/java/de/tum/cit/aet/artemis/communication/test_repository/SavedPostTestRepository.java (2)
6-7: LGTM!The added imports are necessary and properly organized.
10-12: LGTM!The interface is properly annotated and follows the test repository pattern.
src/test/java/de/tum/cit/aet/artemis/communication/test_repository/SavedPostTestRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java (2)
58-71: LGTM! Consider using batch delete for better performance.The method now correctly handles multiple saved posts. The comment about cache keys explains why posts are deleted one by one. However, if cache invalidation is not critical, consider using a batch delete operation for better performance.
79-92: LGTM! Consider using batch update for better performance.The method now correctly handles multiple saved posts and properly manages the completion time. The comment about cache keys explains why posts are updated one by one. However, if cache invalidation is not critical, consider using a batch update operation for better performance.
📜 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/SavedPostService.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/communication/service/SavedPostServiceTest.java(6 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/SavedPostService.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/service/SavedPostServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-tests
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/service/SavedPostService.java (2)
39-50: LGTM! The changes correctly prevent duplicate saved posts.The method now properly handles the case of duplicate posts by checking if any saved posts already exist for the current user and post combination.
117-129: LGTM! Clear documentation of edge cases.The method signature and documentation have been updated to clearly indicate that multiple saved posts may exist for the same posting in edge cases.
src/test/java/de/tum/cit/aet/artemis/communication/service/SavedPostServiceTest.java (1)
58-143: LGTM! Tests thoroughly cover the new functionality.The test methods have been properly updated to:
- Use the new repository method that returns lists
- Cover both empty and non-empty result cases
- Maintain existing assertions
Checklist
General
Server
Motivation and Context
In certain edge cases, the same post can be saved multiple times, resulting in duplicate entries in the saved posts list. This usually happens when clicking the bookmark button rapidly multiple times. Additionally, marking any of these duplicates as "completed" or "archived" can trigger a 500 error.
Addresses #10226
Description
I updated the server-side logic to group saved posts by user ID, post ID, and post type, ensuring that duplicate posts are not displayed. When updating or deleting, all occurrences of a saved post for a given user ID, post ID, and post type combination will be modified accordingly. This approach helps prevent error 500s and enhances clarity in the user interface.
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
Screenshots
Summary by CodeRabbit