Communication: Add send as direct message option for replies
#10420
Conversation
asliayk
added
feature
tests
client
github-actions
bot
added
server
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
WalkthroughThe changes update serialization configurations in backend domain models, add a new field to store an original answer identifier, and propagate these modifications through related model classes. The UI components are updated with conditional logic for handling direct messaging, thread replies, and active conversation management. Additionally, new CSS classes improve layout styling and i18n keys are added for new user messages. The tests are extended to cover the new behaviors in the messaging and post components. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as MessageReplyInlineInputComponent
participant S as MetisService
participant A as AnswerPost
U->>M: Toggle direct message option
M->>M: toggleSendAsDirectMessage()
U->>M: Submit form (createPosting)
alt sendAsDirectMessage true
M->>M: mapAnswerPostToPost(AnswerPost)
M->>S: createAnswerPost$ observable
S-->>M: AnswerPost data
M->>M: finalizeCreation()
M-->>U: Emit created AnswerPost (onCreate event)
else sendAsDirectMessage false
M->>S: create Posting normally
S-->>M: Posting data
M-->>U: Emit created AnswerPost (onCreate event)
end
sequenceDiagram
participant U as User
participant C as CourseConversationsComponent
participant D as ChangeDetector
U->>C: Navigate to a post with conversation ID
alt Active Conversation equals selected ID
C->>C: Set activeConversation to undefined
C->>C: setTimeout(() => set activeConversation to ID)
C->>D: Trigger detectChanges()
else
C->>C: Set activeConversation to the new ID
C->>D: Trigger detectChanges()
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
638-647: Effective solution for handling same-conversation navigation.The conditional logic ensures proper refreshing when navigating to the same conversation by temporarily setting it to undefined before resetting it. This is a common pattern to force re-renders in Angular applications.
Consider using Angular's change detection API directly for more precise control:
- if (this.activeConversation && this.activeConversation.id === id) { - this.metisConversationService.setActiveConversation(undefined); - setTimeout(() => { - this.metisConversationService.setActiveConversation(id); - this.changeDetector.detectChanges(); - }, 0); - } else { - this.metisConversationService.setActiveConversation(id); - this.changeDetector.detectChanges(); - } + if (this.activeConversation && this.activeConversation.id === id) { + // Mark component for check and trigger change detection cycle + this.changeDetector.markForCheck(); + } + this.metisConversationService.setActiveConversation(id); + this.changeDetector.detectChanges();src/main/webapp/app/shared/metis/post/post.component.html (1)
32-43: Good implementation of the reply indicator.The implementation provides clear visual feedback that the post is a reply to another conversation thread. The clickable link to the original content helps maintain context and aids navigation between related conversations.
Small styling suggestion:
- <div class="message-container post-content-padding text-muted fs-smaller mt-1"> + <div class="message-container post-content-padding text-muted fs-smaller mt-1 mb-1">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250227143115_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (17)
src/main/java/de/tum/cit/aet/artemis/communication/domain/AnswerPost.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/Post.java(2 hunks)src/main/webapp/app/entities/metis/post.model.ts(1 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(1 hunks)src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.html(2 hunks)src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts(5 hunks)src/main/webapp/app/shared/metis/post/post.component.html(3 hunks)src/main/webapp/app/shared/metis/post/post.component.scss(1 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(5 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts(2 hunks)src/main/webapp/i18n/de/conversation.json(1 hunks)src/main/webapp/i18n/de/metis.json(2 hunks)src/main/webapp/i18n/en/conversation.json(1 hunks)src/main/webapp/i18n/en/metis.json(2 hunks)src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
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/posting-reactions-bar/posting-reactions-bar.component.htmlsrc/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.htmlsrc/main/webapp/app/shared/metis/post/post.component.html
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/conversation.jsonsrc/main/webapp/i18n/de/metis.json
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/entities/metis/post.model.tssrc/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.tssrc/main/webapp/app/overview/course-conversations/course-conversations.component.tssrc/main/webapp/app/shared/metis/post/post.component.tssrc/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts
`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/shared/metis/post/post.component.spec.tssrc/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts
`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/domain/AnswerPost.javasrc/main/java/de/tum/cit/aet/artemis/communication/domain/Post.java
🧠 Learnings (1)
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts:179-181
Timestamp: 2024-11-12T12:51:58.049Z
Learning: In `src/test/javascript/spec/integration/code-editor/code-editor-container.integration.spec.ts`, `ResizeObserver` is mocked within the `beforeEach` block.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (32)
src/main/webapp/i18n/de/conversation.json (1)
22-22: LGTM! The translation uses the correct informal style.The German translation correctly uses the informal style (du-form) as required by our coding guidelines. This key properly supports the new "send as direct message" functionality.
src/main/webapp/i18n/en/conversation.json (1)
22-22: LGTM! Translation string correctly added.The English translation for the new "send as direct message" feature is clear and concise.
src/main/webapp/i18n/en/metis.json (2)
146-146: LGTM! Clear and concise label for thread navigation.The new translation key provides a clear instruction for users to navigate to the thread view.
156-157: LGTM! Properly structured JSON addition with correct comma placement.The new translation for thread reply indication and the added comma maintain proper JSON formatting.
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.html (1)
3-3: Logic addition handles original answers and thread views correctly.The conditional logic now checks for the absence of an original answer ID and ensures the component is not in a thread sidebar before rendering the reply button. This prevents redundant reply options in thread contexts.
src/main/webapp/app/entities/metis/post.model.ts (1)
13-14: Model properties support direct message functionality.The added properties enhance the Post model:
isConsecutivehelps with UI display logic for consecutive messagesoriginalAnswerIdimplements the core functionality by storing the ID of the original answer when a reply is sent as both an AnswerPost and a PostThis aligns perfectly with the PR objective of enabling replies to be sent as both types of messages.
src/main/webapp/app/shared/metis/post/post.component.scss (2)
109-114: Well-structured flex container implementation.The flex-wrapper provides a clean way to handle the new layout requirements for the thread reply feature.
116-127: Good text handling for thread replies.These classes appropriately handle text overflow for the reply indicators, preventing layout issues when text is too long. The combination of
.replied-textfor fixed content and.one-linefor expandable content with ellipsis works well for responsive design.src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts (2)
116-116: Property added to support direct message replies.The
originalAnswerIdproperty correctly follows TypeScript conventions and Angular style guidelines.
166-166: Properly initializes originalAnswerId from posting.The implementation correctly checks if the posting is a post type before assigning the originalAnswerId property.
src/main/webapp/i18n/de/metis.json (2)
146-146: Thread navigation text added correctly.The German translation uses informal style ("Thread ansehen") as required by the coding guidelines.
156-157: Thread reply message and JSON formatting fixed.Added proper comma after the "saved" entry and included the new reply notification message in informal German style.
src/main/java/de/tum/cit/aet/artemis/communication/domain/AnswerPost.java (1)
42-42: Added 'content' to JSON serialization properties.The addition of "content" to the JsonIncludeProperties annotation is necessary to support the new functionality of showing original answer content in direct message replies.
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
33-34: Good setup for HTTP client testing and ResizeObserver mock!The addition of HTTP client testing providers and the ResizeObserver mock implementation are appropriate for testing components that interact with HTTP services and need to handle resize events. This follows best practices for Angular testing.
Also applies to: 54-55, 86-87, 121-123
src/main/java/de/tum/cit/aet/artemis/communication/domain/Post.java (2)
107-108: Well-structured database column addition.The new
originalAnswerIdfield is correctly annotated with@Columnand uses the appropriate naming convention for the database column.
253-259: Clean accessor methods implementation.Standard getter and setter methods following Java conventions for the new
originalAnswerIdfield.src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.html (1)
15-52: Good implementation of the direct message checkbox feature.The changes improve the layout using flexbox utilities and add the new "Send as Direct Message" checkbox functionality. The code follows the modern Angular practices by using
@iffor conditional rendering and proper translation directives for i18n.src/main/webapp/app/shared/metis/post/post.component.html (2)
18-18: Enhanced conditional rendering logic.The condition has been updated to display the post header when the post is not consecutive or when it's a reply (has originalAnswerId). This improves the user interface by ensuring proper context is shown for all types of posts.
209-221: Appropriate conditional behavior for thread navigation.The dropdown now checks if the post is an original post or a reply, and adapts the action accordingly:
- For original posts: Opens reply thread
- For posts that are replies (with originalAnswerId): Navigates to the original thread
This creates a consistent user experience and prevents confusion when interacting with different types of posts.
src/main/webapp/app/shared/metis/post/post.component.ts (3)
43-43: Properly imported and injected AnswerPostService.The service is correctly imported and injected using the modern inject pattern.
Also applies to: 79-79
132-132: Added property to track original answer post.The optional property is correctly typed and follows camelCase naming convention.
225-225: Properly initialized originalAnswer in ngOnInit.The method is called at the appropriate lifecycle hook to ensure the data is loaded when the component initializes.
src/main/webapp/app/shared/metis/message/message-reply-inline-input/message-reply-inline-input.component.ts (6)
1-1: Updated imports to support new functionality.Added necessary imports for the signal API and Post model.
Also applies to: 12-12
30-30: Added signal for tracking direct message preference.Using Angular's signal API for reactive state management is a good practice.
51-53: Clean implementation of toggle functionality.The method correctly toggles the signal state using the modern signal API.
73-93: Well-structured implementation for dual posting functionality.The implementation correctly handles creating both an AnswerPost and a regular Post when sendAsDirectMessage is true. The observable chaining is properly structured with appropriate error handling.
95-99: Good code reuse pattern with finalizeCreation method.Extracted common logic into a separate method, improving maintainability and readability.
101-107: Properly implemented mapAnswerPostToPost conversion.The method correctly maps the necessary fields from AnswerPost to Post, ensuring that the originalAnswerId reference is maintained.
src/test/javascript/spec/component/shared/metis/message-reply-inline-input/message-reply-inline-input.component.spec.ts (4)
10-10: Updated imports to support new tests.Added necessary imports for rxjs operators and the AnswerPost model.
Also applies to: 16-16
131-137: Well-implemented test for toggle functionality.The test properly verifies the initial state and the effect of toggling the sendAsDirectMessage signal.
139-144: Good coverage for resetFormGroup functionality.The test verifies both the posting.content update and the form control value update.
146-171: Comprehensive test for direct message creation.The test properly:
- Sets up the component state with the required data
- Mocks the service responses
- Verifies the correct parameters are passed to createPost
- Checks that the onCreate event is emitted with the right data
- Confirms that loading state is properly reset
Great job ensuring thorough test coverage for this critical feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/communication/domain/Posting.java (1)
128-134: Consider using 'is' prefix for boolean getters.While the code functions correctly, Java conventions typically recommend using the 'is' prefix rather than 'get' for boolean property getters. Consider renaming
getHasForwardedMessages()toisHasForwardedMessages()for better compliance with Java bean naming conventions.-public boolean getHasForwardedMessages() { +public boolean isHasForwardedMessages() { return hasForwardedMessages; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/communication/domain/AnswerPost.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/Post.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/Posting.java(2 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/de/tum/cit/aet/artemis/communication/domain/AnswerPost.java
- src/main/java/de/tum/cit/aet/artemis/communication/domain/Post.java
- src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts
🧰 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/domain/Posting.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-compilation
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/domain/Posting.java (3)
63-64: Field addition addresses cross-cutting functionality.Adding the
hasForwardedMessagesfield to thePostingbase class is a good refactoring decision. This eliminates duplication by moving a common field from bothPostandAnswerPostsubclasses to their shared parent, following the DRY (Don't Repeat Yourself) principle.
63-134: Feature connection to PR objectives.This change supports the PR objective of sending replies as both an
AnswerPostand a regularPost. ThehasForwardedMessagesfield will track when a message has been duplicated across post types, enabling the "Also send as direct message" functionality described in the PR objectives.
63-134:❓ Verification inconclusive
Verify database migration for schema changes.
This change introduces a new column at the
Postinglevel that was previously in child entities. Ensure there's a corresponding database migration script to handle this schema change correctly, especially concerning existing data.
🏁 Script executed:
#!/bin/bash # Check for migration files that handle this schema change rg -l "has_forwarded_messages" --type-add 'sql:*.sql' --type sql src/main/resources/config/liquibase/Length of output: 101
Action Required: Verify Database Migration for New Column on Posting
The new
has_forwarded_messagescolumn in thePostingentity must have an associated migration to handle existing data. Our initial search in the Liquibase migration files did not return any scripts referencing this column. Please manually verify that a database migration script exists to update the schema accordingly and that existing records are appropriately handled.
| @@ -26,6 +27,8 @@ export class MessageReplyInlineInputComponent extends PostingCreateEditDirective | |||
|
|
|||
| @Output() valueChange = new EventEmitter<void>(); | |||
There was a problem hiding this comment.
Can you also adapt this here, to use input... ?
| @@ -113,6 +113,7 @@ export class PostingReactionsBarComponent<T extends Posting> implements OnInit, | |||
| canPin = false; | |||
| channels: ChannelDTO[] = []; | |||
| users: UserPublicInfoDTO[] = []; | |||
| originalAnswerId: number | undefined; | |||
There was a problem hiding this comment.
you can use this here:
| originalAnswerId: number | undefined; | |
| originalAnswerId?: number; |
does it even need to be undefined here?
| @if (editType !== EditType.UPDATE) { | ||
| <div class="direct-message-checkbox"> | ||
| <input type="checkbox" id="directMessageCheckbox" [checked]="sendAsDirectMessage()" (change)="toggleSendAsDirectMessage()" /> | ||
| <label for="directMessageCheckbox" jhiTranslate="artemisApp.conversationsLayout.sendAsDirectMessage"></label> | ||
| </div> | ||
| } | ||
|
|
||
| <div class="d-flex gap-2"> | ||
| @if (editType === EditType.UPDATE) { |
There was a problem hiding this comment.
I think here it would possibly make sense to only use the first condition and then use @else
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
There was a problem hiding this comment.
Tested on TS5. Really cool new feature 😄 Everything works great, except one thing: When I "send as direct message" in a thread that has a formatted message, it will show the link to that message unfromatted. I think generally that would be okay, just the markdown should not be shown.
Edit: But I would also think its okay if we do not even show the message content at all and just make the "thread" in "replied to a thread" a link.
Checklist
General
Server
Client
Motivation and Context
Currently, when replying to a message, the response is always created as an
AnswerPost. However, there is no option to post the reply as a regularPostinstead. This limitation is not always ideal in scenarios where the reply is relevant to the entire conversation rather than just being tied to the original message.Description
This PR introduces "Also send as direct message" option when writing a reply.
AnswerPostand as a regularPostwithin the same conversation.To achieve this feature, an
original_answer_idcolumn has been added to thePosttable. This column stores the IDs of answers that have been sent both as anAnswerPostand as a regularPost, allowing seamless navigation between the post and its corresponding thread.Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Screenshots
reply input with checkbox
navigation link to the thread
Summary by CodeRabbit
New Features
Style/UI Enhancements
Localization
Tests