-
Notifications
You must be signed in to change notification settings - Fork 301
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 forward messages
#9794
base: develop
Are you sure you want to change the base?
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
after resolving merge conflicts and newly added client tests, this PR is ready for review again :) here are the performance test results: This Locust script tests the forward message workflow under load. Each user:
These steps mimic typical user activity, allowing us to see how the system handles multiple forwarding actions simultaneously. From the attached results, we can observe stable response times and low error rates, indicating that the application can effectively manage concurrent forwarding requests without significant performance issues. |
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.
Just tested it on TS3, works mostly as expected from a technical side. I have two remarks:
- In dark mode, the forward autocompletion hint row highlighting is quite bright, and makes it hard to read the row that the user currently is hovering over
- When searching for users, it looks like we currently only search by loginName. This leads to confusion when I already see the "Test User 12" in the list when typing "test", but then the user disappears again when I type "test user" into the search field. Would it be possible to search users by their full name?
20250117-1421-36.5808733.mp4
The second issue you mentioned has already been noted. However, I used the preexisting endpoint for user selection, which is also used in the "Create Direct Chat" and "Group Chat" options. In those options, users cannot search by name either; they can only search using the login name. While this is an issue that needs to be addressed, it is not directly related to this PR. I’m not entirely sure which specific color you are referring to in the first issue. I tried to align it with the existing input sections to make it consistent. |
Ah I remember having the same problem on the Android app 😅. However I just checked for creating a DM chat, and there searching by username seems to work. Maybe there is already a fix in place?
The color I am refering to, is the bright white of the row item backgrounds, that highlights the list item that the mouse hovers over (see screenshot below). I am not sure what other inputs use this color for highlighting as well, but eg also the selection for the DM chat creation uses a much more pleasant color imo (see blue highlighted row in screenshot above). |
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: 2
🧹 Nitpick comments (3)
src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts (3)
5-11
: Consolidate monaco-editor action imports.Consider consolidating the monaco-editor action imports for better maintainability.
Apply this diff:
-import { BoldAction } from 'app/shared/monaco-editor/model/actions/bold.action'; -import { ItalicAction } from 'app/shared/monaco-editor/model/actions/italic.action'; -import { UnderlineAction } from 'app/shared/monaco-editor/model/actions/underline.action'; -import { QuoteAction } from 'app/shared/monaco-editor/model/actions/quote.action'; -import { CodeAction } from 'app/shared/monaco-editor/model/actions/code.action'; -import { CodeBlockAction } from 'app/shared/monaco-editor/model/actions/code-block.action'; -import { UrlAction } from 'app/shared/monaco-editor/model/actions/url.action'; +import { + BoldAction, + ItalicAction, + UnderlineAction, + QuoteAction, + CodeAction, + CodeBlockAction, + UrlAction, +} from 'app/shared/monaco-editor/model/actions';
41-43
: Simplify signal type declarations.The union with empty array is unnecessary as the type already includes empty arrays.
Apply this diff:
- channels = signal<ChannelDTO[] | []>([]); - users = signal<UserPublicInfoDTO[] | []>([]); - postToForward = signal<Post | undefined>(undefined); + channels = signal<ChannelDTO[]>([]); + users = signal<UserPublicInfoDTO[]>([]); + postToForward = signal<Post | undefined>(undefined);
169-191
: Add selection limits for channels and users.Implement maximum selection limits to prevent performance issues and improve user experience.
Apply this diff:
+ private readonly MAX_SELECTIONS = 10; + selectOption(option: CombinedOption): void { + if (this.selectedChannels.length + this.selectedUsers.length >= this.MAX_SELECTIONS) { + // Optionally show error message to user + return; + } + if (option.type === 'channel') {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ 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/dialogs/forward-message-dialog/forward-message-dialog.component.scss
(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.scss
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
🔇 Additional comments (4)
src/main/webapp/app/overview/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts (4)
33-39
: Convert to standalone component.As suggested in the past review, this component should be converted to a standalone component for better modularity and tree-shaking.
Apply this diff:
@Component({ selector: 'jhi-forward-message-dialog', templateUrl: './forward-message-dialog.component.html', styleUrls: ['./forward-message-dialog.component.scss'], + standalone: true, imports: [ArtemisTranslatePipe, ProfilePictureComponent, NgClass, PostingContentComponent, MarkdownEditorMonacoComponent, ArtemisSharedModule], providers: [MetisService, LinkPreviewService, LinkifyService], })
54-55
: Use viewChild with signal syntax.As suggested in the past review, use the signal syntax for viewChild declarations.
Apply this diff:
- searchInput = viewChild<ElementRef>('searchInput'); - messageContent = viewChild<ElementRef>('messageContent'); + searchInput = viewChild.required<ElementRef>('searchInput'); + messageContent = viewChild.required<ElementRef>('messageContent');
111-113
: Add content validation in updateField.The method should validate the content before updating.
Apply this diff:
updateField(content: string): void { + if (!content || !content.trim()) { + return; + } this.newPost.content = content; }
209-216
: Add validation in send method.The send method should validate selections and content before closing the modal.
Apply this diff:
send(): void { + if (!this.hasSelections()) { + // Optionally show error message to user + return; + } + + if (!this.newPost.content?.trim()) { + // Optionally show error message to user + return; + } + const selectedItems = { channels: this.selectedChannels, users: this.selectedUsers, messageContent: this.newPost.content, }; this.activeModal.close(selectedItems); }
...view/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts
Show resolved
Hide resolved
...view/course-conversations/dialogs/forward-message-dialog/forward-message-dialog.component.ts
Show resolved
Hide resolved
ah I see the color issue now, thank you for noticing this. I think I have solved it now. However, the user selection does not work for me with usernames on TS3 for direct chat user selection. In my PR, I kept the user selection logic unchanged, as it is in the current implementation. Since the existing logic already has this issue, it also affects this PR. Therefore, I think this issue is unrelated to this PR and should be addressed in a separate PR. Screen.Recording.2025-01-17.at.17.08.02.mov |
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.
The highlight color is much better now, thanks for adjusting!
Regarding the search by username issue, I accidentality tested the user search in the DM dialog on TS2, but you are right, on TS3 that searching by username does not work there.
Good to merge from my side :D
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.
Code seems good to me, just 1 remark in the translations
"addMessage": "Eine Nachricht hinzufügen", | ||
"searchBar": "Nach Name oder Kanal suchen", | ||
"viewConversation": "Unterhaltung anzeigen", | ||
"forwardedFrom": "Forwarded from", |
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.
Are you intentionally using a english string? Should be "forwardedFrom": "Weitergeleitet von",
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.
Tested on TS3. Reapprove
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_message
table has been added to the database with the following fields:post
andanswer_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:
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
forwarded message view
forward message dialog
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Post
andAnswerPost
classes to track forwarded messages.ConversationMessagesComponent
to handle forwarded messages and navigation.MetisConversationService
for creating group and direct conversations.Bug Fixes
Documentation
Style
Tests
Chores