-
Notifications
You must be signed in to change notification settings - Fork 297
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
: Allow user to save messages for later
#9705
Communication
: Allow user to save messages for later
#9705
Conversation
…nel-jumping' into bugfix/communication/adjust-channel-jumping
…on/adjust-channel-jumping
…on/adjust-channel-jumping
…ion/save-message-for-later
…re/communication/save-message-for-later
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/webapp/app/shared/metis/post/post.component.ts (1)
Line range hint
1-300
: Consider enhancing saved posts state managementWhile the UI elements for saved posts are being added (bookmark and check icons), consider implementing:
- A service method to handle saved post status changes
- State management for real-time UI updates when post status changes
- Event emitters for saved post status changes
- Proper cleanup of subscriptions to prevent memory leaks
This will ensure a more robust implementation of the saved posts feature.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/ConversationMessagingService.java
(6 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/ConversationMessageResource.java
(1 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
(5 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts
(6 hunks)src/main/webapp/app/shared/metis/post/post.component.html
(8 hunks)src/main/webapp/app/shared/metis/post/post.component.ts
(5 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html
(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts
(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationMessageRepository.java
- src/main/java/de/tum/cit/aet/artemis/communication/web/ConversationMessageResource.java
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts
- src/main/webapp/app/shared/metis/post/post.component.html
- src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html
- src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts
- src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/ConversationMessagingService.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/main/webapp/app/shared/metis/post/post.component.ts (1)
🔇 Additional comments (7)
src/main/webapp/app/shared/metis/post/post.component.ts (3)
23-23
: LGTM: Icon imports and declarations are properly implemented
The new FontAwesome icons for bookmarks and checkmarks are correctly imported and declared as readonly properties, following Angular style guidelines.
Also applies to: 91-92
189-189
: LGTM: Lifecycle method changes ensure proper typing
The calls to assignPostingToPost()
are appropriately placed in both lifecycle hooks to ensure the posting object is properly typed after initialization and changes.
Also applies to: 201-201
295-300
: Referencing existing review comment for type casting improvements
A previous review comment already addresses the necessary improvements for this method, including enhanced null checking, deep copying, and error handling.
src/main/java/de/tum/cit/aet/artemis/communication/service/ConversationMessagingService.java (4)
43-43
: LGTM: Constructor dependency injection is properly implemented
The addition of SavedPostRepository
follows best practices by:
- Using constructor injection
- Maintaining single responsibility
- Following consistent parameter ordering
Also applies to: 82-85
159-159
: LGTM: Post preparation is correctly sequenced
The preparePostForBroadcast
call is properly placed before broadcasting to ensure the saved status is included in the notification.
347-347
: LGTM: Post preparation is correctly sequenced
The preparePostForBroadcast
call is properly placed before broadcasting to ensure the saved status is included in the update notification.
375-375
: LGTM: Post preparation is correctly sequenced
The preparePostForBroadcast
call is properly placed before broadcasting to ensure the saved status is included in the deletion notification.
Checklist
General
Server
Client
Motivation and Context
Currently, the user has no option to store messages for later reference. This leads to them having to use other tools than artemis to create reminders for certain tasks. In slack, this is done by the "Save message for later" feature.
Description
I added the option for the user to save messages for later. This is done by adding a new model "saved_post" which keeps track of which user stored which message, similar to a pivot table. The model has the following structure:
id - Primary key
user_id - The ID of the user that stored the message
post_id - The ID of the saved post
post_type - An identifier to tell the system if the stored post is an "answer_post" or a "post"
status - An identifier to tell the system in which status the post is. Is used to mark posts as "In Progress", "Completed" or "Archived".
completed_at - The date and time when a post was marked as completed or archived. May be used for cleanup.
In the client, I added a new tab to the communication modules sidebar called "Saved Messages". There, the user can view the stored messages. To store a message, I added a new icon next to the edit icon, which can be used to store a message.
For cleanup I added the SavedPostSchedule Service, which will delete archived/completed SavedPosts after 100 days. Additionally, it will try to clean up orphaned SavedPosts, since they are not connected via foreign key to the posts. (If someone for example manually deletes a post from the database)
Known Issues for followup PRs:
Addresses #9697
Addresses #9639
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
Performance Tests
Test Coverage
Client
Server
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests