Communication: Allow user to forward messages
#9794
Conversation
…e-message-view' into feature/communication/consecutive-message-view
…-view # Conflicts: # src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts # src/main/webapp/app/shared/metis/answer-post/answer-post.component.scss
There was a problem hiding this comment.
[Tested on TS2]
Functionalities work mostly as expected, except for two quirks while I was playing around.
-
After forwarding messages, I logged in to another user and the forwarded content was not rendered at first. After refreshing, they got rendered like normal. This also happened after my 2. point.
directly after logging in
after refreshing
-
When forwarding from a private channel to a public channel, the link for "view conversation" appears even if the user is not in the private channel, and clicking on it causes them to access a site they are not supposed to, and reloads them to the all public messages. Is this desired behaviour?
PS: after this happens, the forwarded content are again not rendered until you refresh. -
This is more of a sanity check, is it intended that messages from direct chats are allowed to be forwarded to public chats? I was thinking if this is suitable for the privacy of the users.
julian-wls
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
dismissed stale reviews from julian-wls, PaRangger, and HawKhiem
via
ccfc1b7
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
src/main/webapp/app/shared/metis/metis.service.ts (1)
785-825: Potential memory leak in createForwardedMessages.The forkJoin operation could lead to memory leaks if the component is destroyed before the operation completes. Consider using takeUntil or moving the subscription management to the component level.
Consider one of these approaches:
- Add takeUntil operator to handle component destruction:
private destroy$ = new Subject<void>(); // Add to the pipe chain takeUntil(this.destroy$) // Add to ngOnDestroy this.destroy$.next(); this.destroy$.complete();
- Or move subscription management to the component level and use the async pipe in the template.
src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts (10)
25-30: Check modular structure for the component.The component selector and metadata are properly defined. If you expect higher reusability, consider marking it as a standalone component or referencing it in a dedicated shared module for better modular design.
31-47: Use consistent naming for signals and properties.Your signals
channels,users, andpostToForwardfollow the new Angular reactivity model well. However, verify that all property names match the camelCase style recommended in the guidelines. For instance,courseIdvs._courseIdin other files. Standardizing naming conventions across the entire codebase can help maintain clarity.
48-56: Dropdown display flag and default states.The
showDropdownfield is a boolean that you toggle. You might consider resetting it tofalseon component initialization to ensure you don’t show the dropdown unexpectedly if some external condition sets it otherwise.
57-81: Leverage RxJS best practices and reduce side effects.Inside
ngOnInit, you pull data, build combined options, and filter them. If you foresee multiple side effects in the future, consider using a more reactive approach such as a single pipeline. This can help avoid duplication of filter logic and keep your code more organized.
83-87: Prefer Angular lifecycle hooks over setTimeout.Using
setTimeout(() => this.checkIfContentOverflows(), 0)ensures the DOM is stable, but you may also explorengAfterViewCheckedorChangeDetectorRef.afterViewInitto avoid potential race conditions or performance overhead from repeated setTimeout usage in large or complex components.
105-108: Prefer using a single event handler for search terms.
filterItemsis a thin wrapper aroundfilterOptions(). Consider consolidating the logic if no additional transformations are performed. This keeps the component minimal and direct.
110-142: Avoid redundant filtering logic for channels and users.
filterOptionsincludes separate logic for channels and users, plus a search to the server for user data if the search term length >= 3. Consider centralizing this logic to a single pipeline or function to reduce duplication. Also, handle possible concurrency (multiple typed queries) by using switchMap or distinctUntilChanged in your streams, if relevant.
214-217: Implement keyboard accessibility.
onInputFocussetsshowDropdown = true. Consider implementing keyboard navigation (arrow keys, tab/shift+tab) for selecting items in the dropdown to support accessibility best practices.
219-222: Consider a short delay or a click-away approach.
onInputBlurhides the dropdown immediately on blur. If your design calls for a short grace period, you could use a click-away approach or a slight delay to allow immediate re-click without closing.
224-228: Use a more direct approach to focusing.
focusInputcallsthis.renderer.selectRootElement(...). This can cause scrolling to the input if it’s not in view. If that’s intentional, it’s fine; otherwise, consider using@ViewChildto callnativeElement.focus()directly.src/main/webapp/app/shared/metis/post/post.component.ts (1)
83-84: Consider improving type definitions and immutability.The properties are well-structured, but could be enhanced:
-originalPostDetails: Post | AnswerPost | undefined = undefined; +readonly originalPostDetails: Posting | undefined = undefined; -isConsecutive = input<boolean>(false); -forwardedPosts = input<Post[]>([]); -forwardedAnswerPosts = input<AnswerPost[]>([]); -course: Course; +readonly isConsecutive = input<boolean>(false); +readonly forwardedPosts = input<Post[]>([]); +readonly forwardedAnswerPosts = input<AnswerPost[]>([]); +readonly course: Course;Also applies to: 97-99, 101-101
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
src/main/webapp/app/overview/course-conversations/course-conversations.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(2 hunks)src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts(1 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(2 hunks)src/main/webapp/app/shared/metis/metis-conversation.service.ts(1 hunks)src/main/webapp/app/shared/metis/metis.module.ts(3 hunks)src/main/webapp/app/shared/metis/metis.service.ts(4 hunks)src/main/webapp/app/shared/metis/post/post.component.html(3 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(8 hunks)src/main/webapp/app/shared/metis/posting.directive.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/webapp/app/shared/metis/posting.directive.ts
- src/main/webapp/app/shared/metis/metis.module.ts
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
- src/main/webapp/app/shared/metis/post/post.component.html
- src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts (1)
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/metis/post/post.component.ts (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
🔇 Additional comments (23)
src/main/webapp/app/shared/metis/metis.service.ts (5)
3-3: LGTM! Imports are well-organized.
The new imports are correctly structured and necessary for implementing the forwarded messages functionality.
Also applies to: 13-13, 37-39
67-67: LGTM! Dependency injection is properly implemented.
The ForwardedMessageService is correctly injected following Angular's dependency injection pattern.
796-820: LGTM! Robust error handling and caching logic.
The implementation correctly:
- Uses forkJoin for parallel execution
- Updates the cache only when necessary
- Handles errors appropriately
- Maintains consistency in the observable streams
753-756: 🛠️ Refactor suggestion
Add explicit return type and improve return statement.
The method lacks a return type annotation and uses an implicit return.
- getSourceAnswerPostsByIds(answerPostIds: number[]) {
- if (answerPostIds) return this.answerPostService.getSourceAnswerPostsByIds(this.courseId, answerPostIds);
- else return;
+ getSourceAnswerPostsByIds(answerPostIds: number[]): Observable<AnswerPost[]> | undefined {
+ return answerPostIds ? this.answerPostService.getSourceAnswerPostsByIds(this.courseId, answerPostIds) : undefined;
}Likely invalid or redundant comment.
728-734: 🛠️ Refactor suggestion
Add error handling for API failures.
The method should handle potential API errors to prevent unhandled exceptions.
getForwardedMessagesByIds(ids: number[], type: 'post' | 'answer'): Observable<HttpResponse<{ id: number; messages: ForwardedMessage[] }[]>> | undefined {
if (ids && ids.length > 0) {
- return this.forwardedMessageService.getForwardedMessages(ids, type);
+ return this.forwardedMessageService.getForwardedMessages(ids, type).pipe(
+ catchError(error => {
+ // Handle or transform the error as needed
+ return throwError(() => error);
+ })
+ );
} else {
return undefined;
}
}Likely invalid or redundant comment.
src/main/webapp/app/shared/metis/metis-conversation.service.ts (2)
242-244: Provide consistent naming conventions and align with existing create methods.
This new createGroupConversation method is clearly aligned with the existing pattern for creating different types of chats. Please ensure the naming convention is consistent across the entire codebase, and that all references to “group conversations” are updated accordingly in other Angular services or components if needed.
246-248: Validate single login parameter.
createDirectConversation currently accepts a single string parameter. Consider adding a null/undefined check or an empty string check, to avoid potential errors or unexpected results if no valid string is passed.
src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts (13)
1-17: Confirm compliance with Angular style guidelines.
The import statements and usage of new Angular features like inject, signal, and viewChild are aligned with the latest Angular standards. Just ensure that references like input, signal, and viewChild are intended to be lowercase, as they deviate from the traditional uppercase @Input, @ViewChild usage. If you are leveraging new stable APIs, it’s fine; otherwise, consider verifying your Angular version and nomenclature to ensure compatibility.
18-24: Interface declaration looks concise.
The CombinedOption interface is straightforward and typed properly. This helps maintain clarity when filtering and rendering channel/user search results.
89-95: Validate content measurement approach.
checkIfContentOverflows() calculates scrollHeight vs clientHeight. Confirm that this logic accounts for any dynamic styling or if the content includes images that load asynchronously. You might re-check in ngAfterViewChecked or upon image load events if needed.
97-99: Toggle for message detail is well-structured.
toggleShowFullForwardedMessage() cleanly maintains expanded/collapsed state for message content. Just ensure that the HTML template accounts for state transitions to provide a good user experience (e.g., transitions or animations).
101-103: Editor binding is straightforward.
updateField updates this.newPost.content properly. Ensure that any input validation (minimum length, safe HTML content) is done if your scenario requires it.
144-159: Mapping logic for combined filters is consistent.
updateCombinedOptions effectively merges two arrays (channels, users) into a single combined array. Looks correct for your use case. Be sure to handle potential duplicates if the code’s functionality ever requires distinct checks for user or channel with the same ID or name.
161-183: Prevent adding duplicates by verifying item presence.
The logic inside selectOption is robust: it checks if an item is already in the selected list. This helps prevent duplication. Consider showing an alert or toast if the user tries to select a channel or user that is already chosen, so they know why nothing happens.
185-191: Confirm usage pattern for removing channels.
removeSelectedChannel updates the selected list and sets focus back to the search input. This is a good UX pattern. Just ensure that the parent component or service is informed if the removal also requires backend updates.
193-199: Check if removal from selected users requires further cleanup.
Removing users from selectedUsers is straightforward. If you rely on a backend or parent component, make sure you handle any side effects of user removal, such as unsubscribing or revoking user membership for the forward operation.
201-208: Validate message content before ending the flow.
You’re directly closing the modal with selectedItems data. If empty messages are not allowed, incorporate a content validation step here. Otherwise, the user might forward an empty or invalid message.
210-212: Check the UI for selection prompts.
hasSelections returns true if there are any selected channels or users. Ensure the template properly disables or hides the send button when no selections exist to improve user clarity.
230-235: Validate dropdown closing logic for all user events.
You’re checking if the click target is outside of searchInput. Make sure you also handle cases like pressing the Escape key or overlay clicks. Additionally, watch for race conditions if the user interacts extremely fast.
237-238: Leverage enumerations for editor height.
Referencing MarkdownEditorHeight is helpful for clarity. In alignment with Angular style guidelines, ensure the usage of enumerations or typed constants if you extend additional heights or rely on other enumerated values in the future.
src/main/webapp/app/shared/metis/post/post.component.ts (3)
17-17: LGTM! Import statements are properly organized.
The new imports are correctly added and follow Angular's style guide.
Also applies to: 24-24, 26-26, 36-36, 37-37
111-111: LGTM! Proper null checking implemented.
The course initialization correctly implements null checking using the nullish coalescing operator.
330-332: LGTM! Event emitter implementation is correct.
The onTriggerNavigateToPost method correctly implements the event emission.
src/main/webapp/app/overview/course-conversations/course-conversations.component.html
Show resolved
Hide resolved
I used the existing logic for user selection. I face this issue while creating a direct conversation as well. So this issue is not related with this PR, but should be solved in another PR.
|
|
asliayk
requested review from
JerroyTan,
anian03,
HawKhiem,
PaRangger and
julian-wls
Checklist
General
Server
Client
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_messagetable has been added to the database with the following fields:postandanswer_posttable 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:
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
Screenshots
forwarded message view
forward message dialog
Summary by CodeRabbit
Summary by CodeRabbit
New Features
PostandAnswerPostclasses to track forwarded messages.ConversationMessagesComponentto handle forwarded messages and navigation.MetisConversationServicefor creating group and direct conversations.Bug Fixes
Documentation
Style
Tests
Chores