Communication: Allow user to forward messages

[asliayk]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

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

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 added multiple screenshots/screencasts of my UI changes.
• I translated all newly inserted strings into English and German.

Motivation and Context

Currently user cannot forward messages to other channels/direct-group chats.

(Closes #9066)

Description

The forward message feature has been added. Users can now navigate to the forward message dialog by clicking on the forward message option. They can select a random number of destinations and add content to the forwarded message (similar to Slack).

forwarded_message table has been added to the database with the following fields:

• id: id of forwarded message
• source_id: the id of the original message
• source_type: the type of the original message (post or answer)
• destination_post_id: the id of the destination post (the id of newly created post with the forwarded message)
• destination_answer_id: the id of the destination answer post (the id of newly created answer post with the forwarded message)

post and answer_post table have been updated with a new column named has_forwarded_messages.

Known Issue for a followup PR:
The server-side changes for forwarding multiple messages within a single message and forwarding a message as a reply to another message (inside an AnswerPost) are already in place, but the client-side implementation is not ready yet. To achieve this, a unique message link needs to be generated for each post/answer post, similar to the "Copy Link" feature in Slack. This will be completed in a follow-up PR.

Steps for Testing

Prerequisites:

• 1 Instructor/Student
• 1 Course with Communication enabled

1. Log in to Artemis.
2. Navigate to the Communication section of a course.
3. Select a random message to forward or create a new one. Click on the forward message option (available on its hover bar or in the dropdown menu when opened via right-click).
4. Add some content to the forwarded message and select random destinations to forward the message.
5. Go to the destination channels/user chats and observe the forwarded messages.

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 2

Test Coverage

Client

Class/File	Line Coverage	Confirmation (assert/expect)
answer-post.model.ts	100%	✅
forwarded-message.model.ts	91.66%	✅ ❌
post.model.ts	100%	✅
posting.model.ts	86.36%	✅ ❌
course-conversations.component.ts	91.3%	✅ ❌
forward-message-dialog.component.ts	95.79%	✅ ❌
conversation-messages.component.ts	95.53%	✅ ❌
answer-post.service.ts	100%	✅
answer-post.component.ts	96.29%	✅ ❌
forwarded-message.service.ts	100%	✅
forwarded-message.component.ts	90%	✅ ❌
metis-conversation.service.ts	83.47%	✅ ❌
metis.service.ts	88.47%	✅ ❌
post.service.ts	80%	✅ ❌
post.component.ts	97.08%	✅ ❌
posting-reactions-bar.component.ts	83.67%	✅ ❌
posting-thread.component.ts	100%	✅
posting.directive.ts	93.82%	✅ ❌

Server

Class/File	Line Coverage	Confirmation (assert/expect)
AnswerPost.java	85%	✅ ❌
ForwardedMessage.java	42%	✅ ❌
Post.java	91%	✅ ❌
ForwardedMessageDTO.java	100%	✅
ForwardedMessagesGroupDTO.java	100%	✅
ForwardedMessageRepository.java	not found (added)	❌
AnswerMessageService.java	88%	✅ ❌
ConversationMessagingService.java	92%	✅ ❌
AnswerMessageResource.java	90%	✅ ❌
ConversationMessageResource.java	88%	✅ ❌
ForwardedMessageResource.java	94%	✅ ❌

Screenshots

forwarded message view

forward message dialog

Summary by CodeRabbit

• New Features

  • Introduced message forwarding functionality, allowing users to forward posts and answers directly within conversations.
  • Enhanced the conversation interface with a new forward button, a dedicated dialog for composing and selecting recipients, and visual indicators linking to original messages.
  • Added new data transfer objects (DTOs) for managing forwarded messages and improved handling of forwarded posts and answers.
  • Updated localization to support the new forwarding functionality with relevant translations and tooltips.

• Bug Fixes

  • Resolved issues related to the retrieval of forwarded messages and ensured proper validation for user inputs in the forwarding dialog.

• Tests

  • Expanded test coverage for the ForwardedMessageService, PostService, and related components to ensure robust handling of forwarding functionality.

[HanyangXu0508]

tested on server 3 and worked as expected.

[simonbohnen]

Tested on TS3, works!

[Longppham]

On TS3: WORKS!

[sachmii]

Tested on TS2, works perfectly fine.
When I forward a message with no content to a user which I have not talked to yet, the chat does not appear in the "Direct messages" section. Only after forwarding a message with content added the chat appears and both forwarded messages as well. Not sure if this is intended.

[kevinfischer4]

Tested on TS2, works as expected 👍

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (2)

718-727: ⚠️ Potential issue

Fix incorrect expectations in ANSWER postingType test.

The test case for ANSWER postingType has incorrect expectations. It should check referencePostId for focusing and verify the conversation ID.

Apply this diff to fix the test:

 it('should set openThreadOnFocus = true if postingType is ANSWER', () => {
     const post = {
         id: 1,
         postingType: PostingType.ANSWER,
-        post: { id: 2 } as Post,
+        referencePostId: 2,
+        conversation: { id: 1 } as Conversation,
     } as Posting;

     component.onTriggerNavigateToPost(post);
+
     expect(component.openThreadOnFocus).toBeTrue();
+    expect(component.focusPostId).toBe(2);
+    expect(setActiveConversationSpy).toHaveBeenCalledWith(1);
 });

---

762-777: ⚠️ Potential issue

Fix incorrect expectations in combined logic test.

The test case for combined logic has incorrect expectations. The openThreadOnFocus should be true for ANSWER postingType.

Apply this diff to fix the test:

 it('should combine logic: set focusPostId and only set conversation if ID is present', () => {
     const post = {
         id: 10,
         referencePostId: 888,
         postingType: PostingType.ANSWER,
         conversation: {
             id: 444,
         },
     } as Posting;

     component.onTriggerNavigateToPost(post);

-    expect(component.focusPostId).toBe(10);
-    expect(component.openThreadOnFocus).toBeFalse();
+    expect(component.focusPostId).toBe(888);
+    expect(component.openThreadOnFocus).toBeTrue();
     expect(setActiveConversationSpy).toHaveBeenCalledWith(444);
 });

🧹 Nitpick comments (1)

src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)

700-707: Improve test suite setup.

The test suite setup can be optimized by moving common setup code to the beforeEach block.

Add spy setup to the beforeEach block:

 beforeEach(() => {
     fixture = TestBed.createComponent(CourseConversationsComponent);
     component = fixture.componentInstance;
+    setActiveConversationSpy = jest.spyOn(metisConversationService, 'setActiveConversation');
 });

+afterEach(() => {
+    jest.clearAllMocks();
+});

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d6fc76 and 565c285.

⛔ Files ignored due to path filters (1)

• src/main/resources/config/liquibase/master.xml is excluded by !**/*.xml

📒 Files selected for processing (2)

• src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html (1 hunks)
• src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html

🧰 Additional context used

📓 Path-based instructions (1)

`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...

src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

• src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts

⏰ 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: client-tests-selected
• GitHub Check: client-tests
• GitHub Check: server-style
• GitHub Check: server-tests
• GitHub Check: Analyse

🔇 Additional comments (1)

src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)

700-778: Test suite needs improvements for better coverage and consistency.

The test suite for onTriggerNavigateToPost has the same issues as mentioned in the past review comments:

1. Incorrect expectations for ANSWER type
2. Missing test cases for error handling
3. Repeated test setup

Please refer to the previous review comments for the suggested fixes.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/main/java/de/tum/cit/aet/artemis/communication/web/AnswerMessageResource.java (1)

109-141: LGTM! Well-implemented endpoint with proper validation and error handling.

The implementation follows REST best practices with appropriate security checks, input validation, and error handling.

Consider adding pagination and request limits.

To ensure scalability and prevent potential abuse:

1. Consider paginating the results for large lists of IDs
2. Add a maximum limit to the number of IDs that can be requested in a single call

Example improvement:

 @GetMapping("communication/courses/{courseId}/answer-messages-source-posts")
 @EnforceAtLeastStudentInCourse
-public ResponseEntity<List<AnswerPost>> getSourceAnswerPostsByIds(@PathVariable Long courseId, @RequestParam List<Long> answerPostIds) {
+public ResponseEntity<Page<AnswerPost>> getSourceAnswerPostsByIds(
+    @PathVariable Long courseId,
+    @RequestParam List<Long> answerPostIds,
+    @RequestParam(defaultValue = "0") int page,
+    @RequestParam(defaultValue = "20") int size) {
     log.debug("GET getSourceAnswerPostsByIds invoked for course {} with {} posts", courseId, answerPostIds.size());
     long start = System.nanoTime();
 
+    final int MAX_IDS = 100;
+    if (answerPostIds.size() > MAX_IDS) {
+        throw new BadRequestAlertException(
+            String.format("Cannot request more than %d posts at once", MAX_IDS),
+            answerMessageService.getEntityName(),
+            "tooManyIds"
+        );
+    }

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 565c285 and 382fee3.

📒 Files selected for processing (1)

• src/main/java/de/tum/cit/aet/artemis/communication/web/AnswerMessageResource.java (3 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/web/AnswerMessageResource.java

⏰ Context from checks skipped due to timeout of 90000ms (9)

• GitHub Check: Call Build Workflow / Build .war artifact
• GitHub Check: Call Build Workflow / Build and Push Docker Image
• GitHub Check: Codacy Static Code Analysis
• GitHub Check: client-tests-selected
• GitHub Check: client-style
• GitHub Check: server-tests
• GitHub Check: server-style
• GitHub Check: client-tests
• GitHub Check: Analyse

🔇 Additional comments (2)

src/main/java/de/tum/cit/aet/artemis/communication/web/AnswerMessageResource.java (2)

7-7: LGTM! Import statements follow best practices.

The new imports are specific, necessary, and well-organized, following Java coding guidelines.

Also applies to: 15-15, 21-21, 26-27, 29-30

---

42-50: LGTM! Constructor follows dependency injection best practices.

The constructor properly initializes new dependencies using constructor injection, and fields are marked as final for immutability.

[Cathy0123456789]

Tested on TS2. Works as described