Skip to content
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

Merged
merged 59 commits into from
Nov 26, 2024

Conversation

PaRangger
Copy link
Contributor

@PaRangger PaRangger commented Nov 8, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

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:

  • The Post/Answer Post models currently use a "isSaved" transitive value. This is due to the post not being forwarded in a DTO in various communication APIs. To fix this, a dedicated refactor PR will be created as followup.
  • The saved_posts are currently not cached on client side. Due to the use case being that users will not save thousands of messages, rather 1-20, this has been handled as lower priority and will be resolved in a PR followup. However, to prevent "malicious" storing of a lot of messages, a cap of 100 stored messages was introduced.

Addresses #9697
Addresses #9639

Steps for Testing

Prerequisites:

  • 1 Student
  • 1 Course with communication enabled
  1. Log in to Artemis
  2. Navigate to course
  3. Navigate to the communication tab
  4. If there is none, write messages and answer to messages
  5. Locate the bookmark icon on the top right of a post
  6. Save some messages for later
  7. Go to the "In Progress" tab of saved messages
  8. Click on messages and look if they are displayed correctly
  9. Go back to the "In Progress" tab of saved messages
  10. Mark messages as "Completed" or "Archived" and verify they are in the corresponding tab

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Performance Tests

  • Test 1

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
account.model.ts 100%
conversation.model.ts 100%
post.model.ts 100%
posting.model.ts 100%
course-conversations.component.ts 90.72%
conversation-messages.component.ts 94.41%
posting-summary.component.ts 100%
saved-posts.component.ts 95.23%
course-overview.service.ts 92.8%
answer-post.component.ts 94.64%
metis.service.ts 86.64%
post.component.ts 95.45%
posting-content.components.ts 90.69%
answer-post-header.component.ts 83.33%
post-header.component.ts 100%
posting-header.directive.ts 100%
answer-post-reactions-bar.component.ts 92.1%
posting-reactions-bar.directive.ts 91.42%
posting.directive.ts 82.14%
saved-post.service.ts 100%
sidebar.component.ts 91.66%

Server

Class/File Line Coverage Confirmation (assert/expect)
AnswerMessageService.java 88%
ConversationMessagingService.java 93%
PostingService.java 98%
ReactionService.java 100%
SavedPostScheduleService.java 100%
SavedPostService.java 97%
ConversationMessageResource.java 92%
SavedPostResource.java 88%
User.java 90%
PlagiarismAnswerPostService.java 94%
PlagiarismPostService.java 88%

Screenshots

Bildschirmfoto 2024-11-08 um 10 08 14 Bildschirmfoto 2024-11-08 um 10 08 27 Bildschirmfoto 2024-11-08 um 10 08 35

output-3
output-1 2
output-2

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced the ability to save posts and manage their statuses, including options to archive or mark as completed.
    • Added functionality to retrieve saved posts based on user preferences and statuses.
    • Enhanced UI components to display saved post options and statuses dynamically.
  • Bug Fixes

    • Resolved issues related to the display of saved post statuses and improved error handling for invalid operations.
  • Documentation

    • Updated localization files to include new terms related to saved posts and their management.
  • Tests

    • Expanded test coverage for saved post functionalities, including unit and integration tests to validate new features.

cremertim and others added 24 commits October 28, 2024 11:55
…nel-jumping' into bugfix/communication/adjust-channel-jumping
@PaRangger PaRangger added feature server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. component:Communication communication Pull requests that affect the corresponding module labels Nov 8, 2024
Copy link

@coderabbitai coderabbitai bot left a 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 management

While the UI elements for saved posts are being added (bookmark and check icons), consider implementing:

  1. A service method to handle saved post status changes
  2. State management for real-time UI updates when post status changes
  3. Event emitters for saved post status changes
  4. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0a8076d and 238e171.

📒 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)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

🔇 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.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Nov 26, 2024
@krusche krusche added the maintainer-approved The feature maintainer has approved the PR label Nov 26, 2024
@krusche krusche merged commit 0851344 into develop Nov 26, 2024
95 of 102 checks passed
@krusche krusche deleted the feature/communication/save-message-for-later branch November 26, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) communication Pull requests that affect the corresponding module component:Communication core Pull requests that affect the corresponding module database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. feature maintainer-approved The feature maintainer has approved the PR plagiarism Pull requests that affect the corresponding module ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Merged