Communication: Add pinned messages filter
#10173
Conversation
asliayk
added
feature
tests
client
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces functionality for managing pinned messages in course conversations. The changes span multiple components and files, focusing on enhancing the user interface for displaying and interacting with pinned messages. Key modifications include adding new properties and methods to handle pinned message visibility, implementing filtering mechanisms, and updating the UI to show pinned message counts and toggle options. Changes
Sequence DiagramsequenceDiagram
participant User
participant ConversationHeader
participant ConversationMessages
participant CourseConversations
User->>ConversationHeader: Click "Show Pinned" button
ConversationHeader->>CourseConversations: Emit togglePinnedMessage event
CourseConversations->>ConversationMessages: Set showOnlyPinned = true
ConversationMessages->>ConversationMessages: Filter posts to show only pinned
ConversationMessages->>CourseConversations: Emit pinnedCount
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
|
Feras797
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
Line range hint
324-338: Improve type safety for post processing.The post processing logic uses 'any' type for creationDateDayjs, which bypasses TypeScript's type checking. Consider adding proper typing:
+interface PostWithDayjs extends Post { + creationDateDayjs?: dayjs.Dayjs; +} -setPosts(): void { +setPosts(): void { if (this.content) { this.previousScrollDistanceFromTop = this.content.nativeElement.scrollHeight - this.content.nativeElement.scrollTop; } this.applyPinnedMessageFilter(); - this.posts = this.posts + this.posts = (this.posts as PostWithDayjs[]) .slice() .reverse() .map((post) => { - (post as any).creationDateDayjs = post.creationDate ? dayjs(post.creationDate) : undefined; + post.creationDateDayjs = post.creationDate ? dayjs(post.creationDate) : undefined; return post; }); this.groupPosts(); }src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
Line range hint
1-90: *Migrate remaining ngIf directives to @if syntax.Per the coding guidelines, @if syntax should be used instead of *ngIf. Please update the remaining *ngIf directives:
- <div class="input-group mb-2 rounded-3 p-2 me-2 module-bg" [hidden]="!isCodeOfConductAccepted"> + @if (isCodeOfConductAccepted) { + <div class="input-group mb-2 rounded-3 p-2 me-2 module-bg">
🧹 Nitpick comments (4)
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html (1)
52-74: Well-structured implementation of the pinned messages filter!The implementation follows best practices with:
- Proper conditional rendering
- Clear visual feedback with emojis
- Internationalized text with number interpolation
Consider adding aria-label for better accessibility
Add aria-label to the button to improve screen reader support.
- <button type="button" (click)="togglePinnedMessages()" class="btn btn-sm btn-outline-secondary"> + <button + type="button" + (click)="togglePinnedMessages()" + class="btn btn-sm btn-outline-secondary" + [attr.aria-label]="showPinnedMessages ? 'Hide pinned messages' : 'Show pinned messages'">src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
147-155: Consider optimizing array operations.While the implementation is functionally correct, creating new arrays on each filter operation could impact performance with large datasets. Consider these optimizations:
- Cache the pinned posts count to avoid recalculating it
- Use a getter for filtered posts instead of maintaining a separate array
Example implementation:
-applyPinnedMessageFilter(): void { - if (this.showOnlyPinned()) { - this.posts = this.allPosts.filter((post) => post.displayPriority === DisplayPriority.PINNED); - } else { - this.posts = [...this.allPosts]; - } - const pinnedCount = this.allPosts.filter((post) => post.displayPriority === DisplayPriority.PINNED).length; - this.pinnedCount.emit(pinnedCount); -} +private cachedPinnedCount = 0; + +private updatePinnedCount(): void { + this.cachedPinnedCount = this.allPosts.filter((post) => post.displayPriority === DisplayPriority.PINNED).length; + this.pinnedCount.emit(this.cachedPinnedCount); +} + +get filteredPosts(): Post[] { + return this.showOnlyPinned() + ? this.allPosts.filter((post) => post.displayPriority === DisplayPriority.PINNED) + : this.allPosts; +} + +applyPinnedMessageFilter(): void { + this.updatePinnedCount(); + this.posts = this.filteredPosts; +}src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
165-166: Consider using a more TypeScript-idiomatic initialization.The pinnedCount property initialization can be more concise:
-showOnlyPinned = false; -pinnedCount: number = 0; +showOnlyPinned = false; +pinnedCount = 0;src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
67-68: Consider adding an aria-label for accessibility.The implementation looks good! To enhance accessibility, consider adding an aria-label to indicate when pinned messages are being filtered.
- [showOnlyPinned]="showOnlyPinned" + [showOnlyPinned]="showOnlyPinned" + [attr.aria-label]="showOnlyPinned ? 'Showing only pinned messages' : 'Showing all messages'"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
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/channels-create-dialog/channel-form/channel-form.component.ts(2 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.scss(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts(4 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts(8 hunks)src/main/webapp/i18n/de/metis.json(1 hunks)src/main/webapp/i18n/en/metis.json(1 hunks)src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.scss
🧰 Additional context used
📓 Path-based instructions (10)
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.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/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (1)
Pattern 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/main/webapp/i18n/de/metis.json (1)
Pattern 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/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/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1)
Pattern 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/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.ts (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts (1)
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
Pattern 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}}
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channel-form/channel-form.component.ts (1)
60-60: LGTM! The getter method name has been corrected.The typo in the getter method name has been fixed from
isisCourseWideChannelControltoisCourseWideChannelControl, which aligns with the PR objectives.Also applies to: 110-110
src/main/webapp/app/overview/course-conversations/layout/conversation-header/conversation-header.component.ts (4)
28-29: LGTM! New imports and component metadata are properly configured.The imports for
EmojiComponentandArtemisTranslatePipeare correctly added and included in the component's metadata.Also applies to: 35-35
42-43: LGTM! Properties are properly initialized using the new input/output syntax.The properties are correctly initialized:
pinnedMessageCountas an input with a default value of 0togglePinnedMessageas an output event emitter
66-66: LGTM! State management and dependency injection are properly configured.The component correctly:
- Initializes the
showPinnedMessagesstate- Injects the
ChangeDetectorReffor managing view updatesAlso applies to: 69-69
81-85: LGTM! The toggle method is properly implemented.The
togglePinnedMessagesmethod correctly:
- Emits the toggle event
- Updates the local state
- Triggers change detection
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (2)
154-165: LGTM! The visibility toggle test is comprehensive.The test case properly verifies:
- Initial state is false
- State changes after first toggle
- State reverts after second toggle
167-172: LGTM! The event emission test is properly implemented.The test case correctly verifies that the
togglePinnedMessageevent is emitted when the toggle method is called.src/main/webapp/i18n/en/metis.json (1)
14-16: LGTM! Clear and consistent translations.The translations are well-structured with proper handling of singular/plural cases.
src/main/webapp/i18n/de/metis.json (1)
14-16: LGTM! Translations follow German language guidelines.The translations correctly use:
- Informal style (dutzen) as required
- Proper singular/plural forms
- Consistent terminology with existing translations
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
70-70: LGTM! Properties are well-defined and follow Angular patterns.The new properties are correctly typed and follow Angular's signal-based input/output pattern. The naming is clear and follows conventions.
Also applies to: 95-96, 116-116
141-145: LGTM! OnChanges implementation is correct.The ngOnChanges implementation correctly handles changes to showOnlyPinned and avoids unnecessary setPosts calls on first change.
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1)
300-380: LGTM! Test coverage is comprehensive.The test cases thoroughly cover the new pinned messages functionality:
- Filtering of pinned posts
- Showing all posts
- Pinned count emission
- Change detection handling
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
208-214: LGTM! Methods are concise and focused.The togglePinnedView and onPinnedCountChanged methods are well-implemented and follow the single responsibility principle.
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
661-676: LGTM! Test cases are well-structured.The test cases effectively verify:
- Toggle behavior of showOnlyPinned
- Update behavior of pinnedCount
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
54-59: LGTM! Clean implementation of pinned message header functionality.The new bindings for toggling pinned messages and displaying their count are well-integrated into the existing header component.
SindiBuklaji
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
added
deployment-error
github-actions
bot
removed
the
deployment-error
vinceclifford
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
zagemello
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
added
deployment-error
Longppham
added
deploy:artemis-test2
and removed
deployment-error
Longppham
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
As discussed in the communication subgroup, displaying pinned messages at the top of the chat history was not the best approach. This is because, in conversations that have been open for a long time and contain many messages, pinned messages become practically invisible to the user, requiring them to scroll all the way to the top. Users need a separate/specific section to easily access pinned messages, similar to how it is implemented in Slack.
Description
isisCourseWideChannelControlhas been corrected toisCourseWideChannelControl.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
Code Review
Manual Tests
Test Coverage
Client
Screenshots
pinned messages button
applying pinned messages filter
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests