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
Assessment against linked issues
Possibly related PRs
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
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
…-filter # Conflicts: # src/main/webapp/app/shared/metis/metis.service.ts
asliayk
dismissed stale reviews from sawys777, julian-wls, FelberMartin, simonbohnen, and coderabbitai[bot]
via
bca869f
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
There was a problem hiding this comment.
Thanks for fixing the merge conflict. Overall the changes look good to me 👍 However, I just noticed, that you didn't add a new server test to make sure the new filter pinnedOnly actually works correctly. I think it would be better to verify this with a test (all cases "null", "true", "false") to make sure it works correctly
asliayk
dismissed stale reviews from anian03 and coderabbitai[bot]
via
ab7437d
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java (1)
719-757: Good test implementation with room for minor improvements.The test implementation is well-structured with clear setup, action, and assertion phases. However, consider these improvements for better readability and maintainability:
Apply this diff to improve the test:
@Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void testGetCourseWideMessagesWithPinnedOnly() throws Exception { + // Setup: Create a channel and add instructor participant Channel channel = conversationUtilService.createCourseWideChannel(course, "channel-for-pinned-test", false); ConversationParticipant instructorParticipant = conversationUtilService.addParticipantToConversation(channel, TEST_PREFIX + "instructor1"); + // Create an unpinned post Post unpinnedPost = new Post(); unpinnedPost.setAuthor(instructorParticipant.getUser()); unpinnedPost.setConversation(channel); + unpinnedPost.setContent("This is an unpinned post"); Post createdUnpinnedPost = request.postWithResponseBody("/api/courses/" + courseId + "/messages", unpinnedPost, Post.class, HttpStatus.CREATED); + // Create a post and pin it Post pinnedPost = new Post(); pinnedPost.setAuthor(instructorParticipant.getUser()); pinnedPost.setConversation(channel); + pinnedPost.setContent("This is a pinned post"); Post createdPinnedPost = request.postWithResponseBody("/api/courses/" + courseId + "/messages", pinnedPost, Post.class, HttpStatus.CREATED); + // Pin the second post MultiValueMap<String, String> paramsPin = new LinkedMultiValueMap<>(); paramsPin.add("displayPriority", DisplayPriority.PINNED.toString()); Post updatedPinnedPost = request.putWithResponseBodyAndParams("/api/courses/" + courseId + "/messages/" + createdPinnedPost.getId() + "/display-priority", null, Post.class, HttpStatus.OK, paramsPin); assertThat(updatedPinnedPost.getDisplayPriority()).isEqualTo(DisplayPriority.PINNED); + // Test 1: Verify only pinned posts are returned when pinnedOnly=true MultiValueMap<String, String> params = new LinkedMultiValueMap<>(); params.add("courseWideChannelIds", channel.getId().toString()); params.add("pinnedOnly", "true"); - params.add("size", "10"); + params.add("size", String.valueOf(MAX_POSTS_PER_PAGE)); List<Post> pinnedPosts = request.getList("/api/courses/" + courseId + "/messages", HttpStatus.OK, Post.class, params); assertThat(pinnedPosts).hasSize(1); assertThat(pinnedPosts.get(0).getId()).isEqualTo(updatedPinnedPost.getId()); + // Test 2: Verify all posts are returned when pinnedOnly=false params.set("pinnedOnly", "false"); List<Post> allPosts = request.getList("/api/courses/" + courseId + "/messages", HttpStatus.OK, Post.class, params); assertThat(allPosts).hasSize(2); assertThat(allPosts).extracting(Post::getId).containsExactlyInAnyOrder(createdPinnedPost.getId(), createdUnpinnedPost.getId()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/communication/dto/PostContextFilterDTO.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/communication/dto/PostContextFilterDTO.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
|
I added a server test that covers both true and false cases, and I set the default value to false. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts (2)
395-405: Add error handling for the HTTP request.The
togglePin()method makes an HTTP request but doesn't handle potential errors. This could lead to an inconsistent UI state if the request fails.Add error handling:
togglePin() { if (this.canPin) { if (this.displayPriority === DisplayPriority.PINNED) { this.displayPriority = DisplayPriority.NONE; } else { this.displayPriority = DisplayPriority.PINNED; } (this.posting() as Post).displayPriority = this.displayPriority; - this.metisService.updatePostDisplayPriority((this.posting() as Posting).id!, this.displayPriority).subscribe(); + this.metisService.updatePostDisplayPriority((this.posting() as Posting).id!, this.displayPriority).subscribe({ + error: (error) => { + // Revert the UI state on error + this.displayPriority = this.displayPriority === DisplayPriority.PINNED ? DisplayPriority.NONE : DisplayPriority.PINNED; + (this.posting() as Post).displayPriority = this.displayPriority; + console.error('Failed to update pin status:', error); + } + }); } }
207-210: Add null check inresetTooltipsAndPriority.The method uses non-null assertion operator (
!) which could lead to runtime errors.private resetTooltipsAndPriority() { - this.displayPriority = (this.posting() as Post).displayPriority!; + const post = this.posting() as Post; + this.displayPriority = post.displayPriority ?? DisplayPriority.NONE; this.pinTooltip = this.getPinTooltip(); }
🧹 Nitpick comments (2)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts (2)
418-420: Consider removing redundant getter method.The
checkIfPinned()method simply returns thedisplayPriorityproperty without any additional logic. SincedisplayPriorityis already a class property, this getter method adds unnecessary complexity.Consider one of these alternatives:
- Remove the method and use the property directly
- If encapsulation is needed, use TypeScript's getter syntax:
- checkIfPinned(): DisplayPriority { - return this.displayPriority; - } + get isPinned(): boolean { + return this.displayPriority === DisplayPriority.PINNED; + }
191-205: Improve type safety insetCanPinmethod.The method uses type assertions and optional chaining extensively. Consider improving type safety:
setCanPin(currentConversation: ConversationDTO | undefined) { if (!currentConversation) { this.canPin = this.metisService.metisUserIsAtLeastTutorInCourse(); return; } if (isChannelDTO(currentConversation)) { - this.canPin = currentConversation.hasChannelModerationRights ?? false; + this.canPin = Boolean(currentConversation.hasChannelModerationRights); } else if (isGroupChatDTO(currentConversation)) { - this.canPin = currentConversation.creator?.id === this.accountService.userIdentity?.id; + const creatorId = currentConversation.creator?.id; + const userId = this.accountService.userIdentity?.id; + this.canPin = Boolean(creatorId && userId && creatorId === userId); } else if (isOneToOneChatDTO(currentConversation)) { this.canPin = true; } this.canPinOutput.emit(this.canPin); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: server-tests
- GitHub Check: Analyse
Checklist
General
Server
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.
(Closes #10177)
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