Communication: Add shortcut to direct chats via usernames in conversation detail dialog
#10277
Conversation
asliayk
added
tests
client
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
WalkthroughThis pull request introduces modifications to the conversation detail dialog and related components. It adds a condition in the dialog’s HTML to only show the members tab for group conversations. New output properties and methods are implemented to emit events when a username is clicked. Corresponding changes in styling and test cases support these functional improvements. Additionally, the conversation header component now subscribes to these events to dismiss the modal and trigger one-to-one chat creation. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MR as MemberRow
participant MM as MembersComponent
participant DD as DetailDialog
participant CH as Header
U->>MR: Click on username
MR->>MM: Emit onUserNameClicked(userId)
MM->>DD: Forward click event
DD->>CH: Emit userNameClicked(userId)
CH->>CH: Dismiss modal and initiate one-to-one chat
Possibly related PRs
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: 2
🧹 Nitpick comments (3)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (3)
169-198: Improve test readability and maintainability.The test case effectively verifies the username click functionality, but could benefit from some improvements:
- it('should dismiss modal and call createOneToOneChatWithId when userNameClicked is emitted', fakeAsync(() => { + it('should create one-to-one chat and dismiss modal when username is clicked', fakeAsync(() => { const fakeUserNameClicked$ = new Subject<number>(); const fakeModalRef: NgbModalRef = { componentInstance: { - course: undefined, - activeConversation: undefined, - selectedTab: undefined, + course, + activeConversation, + selectedTab: ConversationDetailTabs.INFO, initialize: jest.fn(), userNameClicked: fakeUserNameClicked$, },
249-271: Enhance test case with specific expectations.While the test verifies the emission of
onUpdateSidebar, it could be more specific about the expected behavior.- expect(onUpdateSidebarSpy).toHaveBeenCalled(); + expect(onUpdateSidebarSpy).toHaveBeenCalledExactlyOnceWith();
273-278: Enhance test case with specific expectations.Similar to the previous test, this one could be more specific about the expected behavior.
- expect(collapseSearchSpy).toHaveBeenCalled(); + expect(collapseSearchSpy).toHaveBeenCalledExactlyOnceWith();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html(2 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts(3 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts(4 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts(4 hunks)src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.tssrc/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.tssrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.tssrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
`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/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.htmlsrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.htmlsrc/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html
`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/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.tssrc/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.tssrc/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts
🪛 GitHub Check: client-style
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
[warning] 305-305:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 306-306:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (18)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (4)
13-13: LGTM! Clean import organization and mock service setup.The imports are well-organized, and the mock service is properly configured using dependency injection.
Also applies to: 29-31, 70-70
200-222: LGTM! Robust error handling test.The test case properly verifies that the component gracefully handles modal instances without the
userNameClickedproperty.
224-247: LGTM! Comprehensive one-to-one chat behavior test.The test case effectively verifies that the info tab is always selected for one-to-one chats, aligning with the PR objective to streamline the UI for direct messages.
303-304: LGTM! Proper tab selection logic.The modification to
detailDialogTestcorrectly implements the requirement to always show the info tab for one-to-one chats.src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (1)
38-38: LGTM! Clean implementation of user click handling.The implementation follows Angular's style guide and uses the new signal-based output syntax.
Also applies to: 77-79
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)
131-136: LGTM! Well-structured test case.The test follows Jest guidelines and uses proper matchers to verify event emission.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts (1)
42-42: LGTM! Clean implementation of user click event propagation.The implementation follows Angular's style guide and uses signal-based outputs.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (1)
54-54: LGTM! Clean implementation of user click event emission.The output property follows Angular's style guide.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (3)
284-294: LGTM! Test coverage for self-click prevention.The test correctly verifies that clicking one's own username doesn't emit an event.
296-306: LGTM! Test coverage for username click event.The test properly verifies that clicking another user's name emits their ID.
338-358: Well-structured parameterized tests for user roles!Excellent use of parameterized testing to verify icon and tooltip combinations for different user roles.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss (1)
19-34: LGTM! Clear visual feedback for interactive elements.Good use of cursor styles and transitions to indicate clickable elements. The
.no-hoverclass effectively prevents interaction styling for the current user's name.src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html (1)
16-24: LGTM! Clean implementation of clickable username.Good use of
ngClassfor conditional styling and proper event binding for the click handler.src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html (2)
40-50: LGTM! Improved UX by hiding members tab in one-to-one chats.Good implementation of conditional rendering to reduce UI clutter in direct chats.
67-72: LGTM! Clean event binding for username clicks.Proper implementation of the userNameClicked event binding in the conversation members component.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.html (1)
61-61: Ensure Event Binding is Correctly Configured for Username Clicks.The addition of
(onUserNameClicked)="userNameClicked.emit($event)"on the<li>element meets the PR objective by enabling clickable usernames that redirect to the appropriate direct chat. Please verify that the correspondinguserNameClickedoutput property is declared and handled appropriately in the component’s TypeScript file.src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts (2)
125-127: LGTM! Tab selection for one-to-one chats.The implementation correctly forces the INFO tab for one-to-one chats, aligning with the PR objective to remove the members tab from these conversations.
29-34: LGTM! Code style and structure.The component follows Angular style guidelines and coding requirements:
- Proper use of dependency injection with
inject()- Consistent code style with arrow functions and curly braces
- Well-organized component structure
Also applies to: 35-153
...ialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
Outdated
Show resolved
Hide resolved
...pp/overview/course-conversations/layout/conversation-header/conversation-header.component.ts
Outdated
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
...e-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html
Outdated
Show resolved
Hide resolved
...log/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html
Outdated
Show resolved
Hide resolved
...log/tabs/conversation-members/conversation-member-row/conversation-member-row.component.scss
Outdated
Show resolved
Hide resolved
...ialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
Outdated
Show resolved
Hide resolved
...pp/overview/course-conversations/layout/conversation-header/conversation-header.component.ts
Outdated
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (3)
169-201: Test case for username click handling looks good, but could be more specific with expectations.The test case properly verifies the modal dismissal and chat creation, but could be more specific with the expectation for
createOneToOneChatWithId.Update the expectation to be more specific:
- expect(createChatSpy).toHaveBeenCalledWith(testUserId); + expect(createChatSpy).toHaveBeenCalledExactlyOnceWith(testUserId);
203-225: Test case for modal without userNameClicked property looks good, but could be more specific with expectations.The test case properly verifies that the chat creation is not called when the modal instance lacks the userNameClicked property.
Update the expectation to be more specific:
- expect(createChatSpy).not.toHaveBeenCalled(); + expect(createChatSpy).toHaveBeenCalledTimes(0);
252-274: Test case for sidebar update looks good, but could be more specific with expectations.The test case properly verifies that the sidebar is updated when the conversation detail dialog is closed.
Update the expectation to be more specific:
- expect(onUpdateSidebarSpy).toHaveBeenCalledOnce(); + expect(onUpdateSidebarSpy).toHaveBeenCalledExactlyOnceWith();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts(5 hunks)
🧰 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/layout/conversation-header/conversation-header.component.spec.ts
⏰ 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: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (4)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (4)
13-13: LGTM! Import changes and mock provider updates look good.The changes properly import required dependencies and update the mock provider configuration.
Also applies to: 29-31, 70-70
227-250: LGTM! Test case for one-to-one chat info tab looks good.The test case properly verifies that the info tab is always opened for one-to-one chats, regardless of the requested tab.
276-281: Test case for search bar toggle looks good, but could be more specific with expectations.The test case properly verifies that the search bar toggle emits the collapse event.
Update the expectation to be more specific:
- expect(collapseSearchSpy).toHaveBeenCalledOnce(); + expect(collapseSearchSpy).toHaveBeenCalledExactlyOnceWith();
305-307: LGTM! Modified detailDialogTest function looks good.The function now properly handles the tab selection based on the conversation type.
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
…-from-overview-dialog # Conflicts: # src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts # src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts
asliayk
dismissed stale reviews from simonbohnen, flbrgit, sachmii, ole-ve, and coderabbitai[bot]
via
20e77b7
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (1)
55-61: Add null safety checks for conversation members.While the logic is correct, consider adding null safety checks for the members array:
if (this.activeConversation) { const conversation = getAsOneToOneChatDTO(this.activeConversation); if (conversation) { this.isOneToOneChat = true; - this.otherUser = conversation.members?.find((user) => !user.isRequestingUser); + this.otherUser = conversation.members?.[0] + ? conversation.members.find((user) => !user.isRequestingUser) ?? undefined + : undefined; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html(3 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts(5 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html
- src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts
⏰ 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: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: Analyse
🔇 Additional comments (4)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.ts (4)
1-1: LGTM! Import changes align with new functionality.The imports follow the Angular style guide and support the new features for handling one-to-one chats and user profiles.
Also applies to: 6-6, 17-18
29-38: LGTM! Component metadata follows standalone pattern.The imports array is properly structured and includes all necessary dependencies.
48-51: LGTM! Properties follow Angular conventions.The new properties are well-typed and follow Angular's naming conventions:
isOneToOneChatfor state trackingotherUserfor storing the conversation partneruserNameClickedoutput for event emission
100-102: LGTM! Event handler follows Angular patterns.The method correctly emits the userId through the output property.
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
krusche
deleted the
feature/communication/add-user-navigation-from-overview-dialog
branch
krusche
added
ready to merge
maintainer-approved
Checklist
General
Client
Motivation and Context
Description
ConversationMemberRowComponent.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
profile picture on header & no members tab for direct chats in conversation detail dialog
clickable user names on dialog
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Style
Bug Fixes