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
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
Cathy0123456789
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS5.
- When deleting a pinned message, the count does not update until you reload the page
- When pinning a post, and a student being in the same channel at the same time, the change is reflected in the color change, but the count does not update for the student and when clicking the pinned-messages-button, the newly pinned message does not show up in the list as well for the student
asliayk
dismissed stale reviews from HawKhiem and coderabbitai[bot]
via
c39b0da
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
298-318: Consider adding error logging for better debugging.The error handling in
fetchAllPinnedPostssilently swallows errors. While resetting to an empty array is appropriate, logging the error would help with debugging.catchError((err) => { + console.error('Error fetching pinned posts:', err); this.pinnedPosts$.next([]); return of([]); }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/shared/metis/metis.service.ts(7 hunks)src/test/javascript/spec/service/metis/metis.service.spec.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/test/javascript/spec/service/metis/metis.service.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}}
🪛 Biome (1.9.4)
src/main/webapp/app/shared/metis/metis.service.ts
[error] 705-705: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 709-709: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 710-710: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/webapp/app/shared/metis/metis.service.ts (2)
51-51: LGTM! Well-structured observable pattern implementation.The
pinnedPosts$BehaviorSubject and its public getter method follow Angular best practices for state management.Also applies to: 89-91
771-782: LGTM! Well-documented and focused helper method.The
removeFromPinnedPostsmethod is well-documented and follows the single responsibility principle.src/test/javascript/spec/service/metis/metis.service.spec.ts (1)
Line range hint
163-790: LGTM! Comprehensive test coverage with clear test cases.The test suite thoroughly covers the pinned messages functionality:
- Tests WebSocket interactions for pinning/unpinning
- Verifies server fetching behavior
- Includes edge cases for post removal
- Uses proper test cleanup with subscription management
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (2)
661-669: LGTM! Consider improving test description.The test correctly verifies the toggle functionality. Consider making the test description more specific about the behavior being tested.
- it('should toggle the value of showOnlyPinned', () => { + it('should toggle pinned messages visibility between shown and hidden states', () => {
671-676: LGTM! Consider adding edge case tests.The test correctly verifies the pinned count update. Consider adding tests for edge cases.
Add these test cases for better coverage:
it('should handle negative pinnedCount values by setting to 0', () => { component.onPinnedCountChanged(-1); expect(component.pinnedCount).toBe(0); }); it('should handle undefined pinnedCount by setting to 0', () => { component.onPinnedCountChanged(undefined); expect(component.pinnedCount).toBe(0); });src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (1)
174-191: Enhance boolean assertions for better error messages.The change detection tests are well-structured but could benefit from more specific assertions.
Update the boolean assertions to follow the coding guidelines:
- expect(component.showPinnedMessages).toBeFalse(); + expect(component.showPinnedMessages).toBe(false); - expect(component.showPinnedMessages).toBeTrue(); + expect(component.showPinnedMessages).toBe(true);This change aligns with the coding guidelines which specify using
toBe(false)andtoBe(true)instead oftoBeFalse()andtoBeTrue()for boolean assertions.Also applies to: 193-210, 212-229
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/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 (5)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: Analyse
🔇 Additional comments (4)
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
678-692: LGTM! Tests follow Jest's expectation specificity guidelines.The tests correctly use Jest's
toBeTrue()andtoBeFalse()instead oftoBe(true)andtoBe(false)as recommended by the coding guidelines.src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (3)
29-30: LGTM!The new imports are appropriate for the added test functionality.
154-165: LGTM! Well-structured toggle visibility test.The test properly verifies the toggle behavior with clear assertions and spy usage.
167-172: LGTM! Event emission test follows best practices.The test correctly verifies the event emission using a spy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/shared/metis/metis.service.ts (1)
299-319: LGTM with a minor suggestion.The implementation is solid with proper error handling and state management. Consider adding a loading state to handle the async operation.
Add a loading state to improve the user experience:
+private pinnedPostsLoading$ = new BehaviorSubject<boolean>(false); + +getPinnedPostsLoading(): Observable<boolean> { + return this.pinnedPostsLoading$.asObservable(); +} public fetchAllPinnedPosts(conversationId: number): Observable<Post[]> { + this.pinnedPostsLoading$.next(true); const pinnedFilter: PostContextFilter = { courseId: this.courseId, conversationId: conversationId, postSortCriterion: PostSortCriterion.CREATION_DATE, sortingOrder: SortDirection.DESCENDING, pinnedOnly: true, pagingEnabled: false, }; return this.postService.getPosts(this.courseId, pinnedFilter).pipe( map((res: HttpResponse<Post[]>) => res.body ?? []), tap((pinnedPosts: Post[]) => { this.pinnedPosts$.next(pinnedPosts); + this.pinnedPostsLoading$.next(false); }), catchError((err) => { this.pinnedPosts$.next([]); + this.pinnedPostsLoading$.next(false); return of([]); }), ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/shared/metis/metis.service.ts(7 hunks)src/test/javascript/spec/service/metis/metis.service.spec.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/metis/metis.service.ts (1)
src/test/javascript/spec/service/metis/metis.service.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}}
🪛 Biome (1.9.4)
src/main/webapp/app/shared/metis/metis.service.ts
[error] 709-709: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 713-713: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 714-714: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/test/javascript/spec/service/metis/metis.service.spec.ts
[error] 819-821: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: client-tests-selected
- GitHub Check: Analyse
🔇 Additional comments (5)
src/main/webapp/app/shared/metis/metis.service.ts (4)
51-51: LGTM!The
pinnedPosts$BehaviorSubject is well-designed for managing the pinned posts state.
694-705: Wrap switch case declarations in blocks to prevent variable hoisting.The declarations inside the switch case could be hoisted to the outer scope. This is flagged by the static analysis tool.
case MetisPostAction.UPDATE: + { const indexToUpdate = this.cachedPosts.findIndex((post) => post.id === postDTO.post.id); if (indexToUpdate > -1) { postDTO.post.authorRole = this.cachedPosts[indexToUpdate].authorRole; postDTO.post.answers?.forEach((answer: AnswerPost) => { const cachedAnswer = this.cachedPosts[indexToUpdate].answers?.find((a) => a.id === answer.id); if (cachedAnswer) { answer.authorRole = cachedAnswer.authorRole; } }); this.cachedPosts[indexToUpdate] = postDTO.post; } if (postDTO.post.displayPriority === DisplayPriority.PINNED) { const currentPinnedPosts = this.pinnedPosts$.getValue(); const indexPinned = currentPinnedPosts.findIndex((pinnedPost) => pinnedPost.id === postDTO.post.id); if (indexPinned > -1) { currentPinnedPosts[indexPinned] = postDTO.post; this.pinnedPosts$.next([...currentPinnedPosts]); } else { this.pinnedPosts$.next([postDTO.post, ...currentPinnedPosts]); } } else { this.removeFromPinnedPosts(postDTO.post.id!); } this.addTags(postDTO.post.tags); + } break;
713-718: Wrap switch case declarations in blocks to prevent variable hoisting.The declarations inside the switch case could be hoisted to the outer scope. This is flagged by the static analysis tool.
case MetisPostAction.DELETE: + { const indexToDelete = this.cachedPosts.findIndex((post) => post.id === postDTO.post.id); if (indexToDelete > -1) { this.cachedPosts.splice(indexToDelete, 1); } const currentPinnedPosts = this.pinnedPosts$.getValue(); const isPinned = currentPinnedPosts.some((pinnedPost) => pinnedPost.id === postDTO.post.id); if (isPinned) { const updatedPinnedPosts = currentPinnedPosts.filter((pinnedPost) => pinnedPost.id !== postDTO.post.id); this.pinnedPosts$.next(updatedPinnedPosts); } + } break;🧰 Tools
🪛 Biome (1.9.4)
[error] 713-713: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 714-714: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
779-786: LGTM!The helper method is well-documented and efficiently implements the removal of posts from the pinned posts list.
src/test/javascript/spec/service/metis/metis.service.spec.ts (1)
163-825: LGTM! Comprehensive test coverage.The test suite thoroughly covers all aspects of the pinned messages functionality:
- Pinning/unpinning posts
- WebSocket interactions
- Edge cases and error scenarios
🧰 Tools
🪛 Biome (1.9.4)
[error] 819-821: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
|
I have also fixed the synchronization issues with pinned messages. Now, when a message is deleted or another user pins a message, the pinned count updates in real-time for other users without requiring a page refresh. |
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
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests