Communication: Allow users to mark all channels as read
#9994
Conversation
github-actions
bot
added
tests
server
cremertim
changed the title
Communication: Mark All Channels as Read
WalkthroughThis pull request introduces a feature that allows users to mark all channels associated with a specific course as read. The changes encompass backend services, resources, and frontend components, including new API endpoints, methods for managing read statuses, and UI elements for user interaction. Additionally, translations for the new functionality are provided to enhance the user experience across different languages. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)src/main/webapp/app/shared/sidebar/sidebar.component.ts (1)🔇 Additional comments (2)src/main/webapp/app/shared/sidebar/sidebar.component.ts (2)
The Also applies to: 65-65
Add validation and documentation to the method. The method should include validation checks and documentation to improve robustness and maintainability. Apply these improvements: + /**
+ * Marks all channels as read for the current course.
+ * This triggers the onMarkAllChannelsAsRead event which is handled by the parent component.
+ */
markAllMessagesAsChecked() {
+ // Validate that messaging is enabled and there are unread channels
+ if (!this.sidebarData?.messagingEnabled) {
+ return;
+ }
+
this.onMarkAllChannelsAsRead.emit();
}Additionally, consider adding a loading state to prevent multiple rapid clicks during the operation, given the previous race condition issues mentioned in the PR comments.
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: 9
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/overview/course-conversations/course-conversations.component.html (1)
33-33: Fix naming inconsistency in event bindingThe event handler method name
markAllChannelAsRead(singular) doesn't match the event nameonMarkAllChannelsAsRead(plural). Maintain consistency by using plural form throughout.-(onMarkAllChannelsAsRead)="markAllChannelAsRead()" +(onMarkAllChannelsAsRead)="markAllChannelsAsRead()"src/main/webapp/app/shared/sidebar/sidebar.component.html (1)
48-51: Add aria-label for accessibilityThe button should include an aria-label to improve accessibility for screen readers.
-<button (click)="markAllMessagesAsChecked()" class="p-2" ngbDropdownItem> +<button (click)="markAllMessagesAsChecked()" class="p-2" ngbDropdownItem + [attr.aria-label]="'artemisApp.courseOverview.sidebar.setMessagesAsRead' | artemisTranslate">src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
499-509: Consider enhancing error handling for edge cases.The implementation looks good and follows the established patterns in the codebase. However, consider adding explicit error handling for the case where no channels exist in the course.
@PutMapping("{courseId}/channels/mark-as-read") @EnforceAtLeastStudent public ResponseEntity<ChannelDTO> markAllChannelsOfCourseAsRead(@PathVariable Long courseId) { log.debug("REST request to mark all channels of course {} as read", courseId); var requestingUser = userRepository.getUserWithGroupsAndAuthorities(); var course = courseRepository.findByIdElseThrow(courseId); checkCommunicationEnabledElseThrow(course); authorizationCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.STUDENT, course, requestingUser); + var channels = channelRepository.findChannelsByCourseId(courseId); + if (channels.isEmpty()) { + return ResponseEntity.noContent().build(); + } channelService.markAllChannelsOfCourseAsRead(course, requestingUser); return ResponseEntity.ok().build(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java(1 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/course-conversations.component.ts(1 hunks)src/main/webapp/app/shared/metis/conversations/conversation.service.ts(1 hunks)src/main/webapp/app/shared/metis/metis-conversation.service.ts(1 hunks)src/main/webapp/app/shared/sidebar/sidebar.component.html(1 hunks)src/main/webapp/app/shared/sidebar/sidebar.component.ts(4 hunks)src/main/webapp/i18n/de/student-dashboard.json(1 hunks)src/main/webapp/i18n/en/student-dashboard.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java(4 hunks)src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/i18n/de/student-dashboard.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/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
src/main/webapp/app/shared/sidebar/sidebar.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/main/webapp/app/overview/course-conversations/course-conversations.component.ts (1)
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/services/metis-conversation.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}}
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (1)
Pattern 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/main/webapp/app/shared/sidebar/sidebar.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}}
🔇 Additional comments (7)
src/main/webapp/app/shared/sidebar/sidebar.component.ts (2)
31-31: LGTM! Output event declaration follows Angular patterns
The output event is correctly declared using the new output() syntax.
2-2: LGTM! Icon import is correctly organized
The faCheck icon is properly imported and grouped with other FontAwesome icons.
src/main/webapp/i18n/en/student-dashboard.json (1)
85-86: LGTM: Translation is clear and consistent.
The English translation is well-placed and follows the existing translation patterns.
src/main/webapp/i18n/de/student-dashboard.json (1)
85-86: LGTM: Translation follows German language guidelines.
The German translation correctly uses the informal form and maintains consistency with existing translations.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1)
447-455: LGTM! The implementation is efficient and handles edge cases.
The method correctly:
- Avoids unnecessary database updates when there are no unread messages
- Updates both the timestamp and unread count atomically
- Uses the existing
getOrCreateConversationParticipantmethod for participant management
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
583-589: LGTM! The test case is comprehensive.
The test effectively verifies:
- The correct invocation of
markAllChannelsAsRead - The subsequent refresh operations
- Uses proper Jest spying techniques for verification
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (1)
84-86: LGTM!
The field addition follows the established patterns and is properly annotated.
...ipt/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java
Show resolved
Hide resolved
src/main/webapp/app/overview/course-conversations/course-conversations.component.ts
Outdated
Show resolved
Hide resolved
sachmii
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
…ssages-to-read' into feature/communication/set-all-messages-to-read
sachmii
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
FelberMartin
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
ahbitaqu
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Communication: Mark All Channels as readCommunication: Allow users to mark all channels as read
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently, a user has to click at each channel to mark a channel as read. This becomes cumbersome during the lifecycle of a course
Description
I added abutton to resolve all channels via one click
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
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Summary by CodeRabbit
New Features
Bug Fixes
Tests