Communication: Fix performance issues in mark all read feature
#10083
Conversation
krusche
changed the title
Communication: Fix performance issues with mark all read featureCommunication: Fix performance issues in mark all read feature
github-actions
bot
added
tests
server
krusche
added
database
WalkthroughThis pull request introduces comprehensive modifications to the communication and conversation-related components in the Artemis application. The changes primarily focus on refactoring boolean properties in channel and conversation classes, converting them from 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 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: 1
🧹 Nitpick comments (13)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1)
320-320: Async TODO Reminder.
The comment indicates that this broadcast method should eventually be asynchronous. Consider using@Asyncif feasible, or another asynchronous approach, to prevent potential blocking calls.Would you like me to draft an asynchronous refactor for this method?
src/main/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.java (1)
94-106: Check subquery performance.
The subquery can be less efficient compared to an equivalent join or a two-step query process. Consider verifying that it meets performance needs or investigate using joins if query execution times become a concern.src/main/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.java (1)
42-43: Use primitive booleans for consistency and clarity.The comment notes a future transition to primitive booleans. Converting
Booleantobooleanavoids null-check overhead per the coding guidelines (prefer_primitives).src/main/webapp/app/shared/metis/conversations/conversation.service.ts (1)
181-181: Switch from PUT to POST
ChangingmarkAllChannelsAsReadto a POST request might be more semantically correct if the existing endpoint modifies state without an idempotent guarantee. Ensure that corresponding server-side logic also aligns with REST best practices.src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.java (2)
57-57: Use direct boolean checks without extra variable if clarity isn’t improved.Here, you assign
channel.getIsArchived()to a local variable before using it only once. If the variable name does not aid readability, you could use the call directly to reduce clutter.
75-76: Leverage direct boolean checks consistently.Similar to line 57, consider using direct checks instead of creating local boolean variables if they do not significantly improve legibility. This helps keep methods concise and more readable.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java (2)
302-310: Replace multiple early returns with a clear conditional structure.You have multiple single-line
return;statements (lines 304, 308) that can make the flow less straightforward to read. Consolidating them in a clear conditional block can improve readability. For instance:if (channelName == null || channel == null) { return; } updateChannelName(channel, channelName);
426-426: Enforce stricter validation or reject ambiguous prefixes.You forbid user-generated channels to start with '$', but consider if other special prefixes might also be disallowed or need further checks.
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
228-228: Prefer passing the requesting user as a parameter.You're retrieving the user again via
userRepository.getUserWithGroupsAndAuthorities(). Since you already haverequestingUserin scope, consider reusing that object instead.- var createdChannel = channelService.createChannel(course, channelDTO.toChannel(), Optional.of(userRepository.getUserWithGroupsAndAuthorities())); + var createdChannel = channelService.createChannel(course, channelDTO.toChannel(), Optional.of(requestingUser));src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.java (1)
45-46: Prefer constructor injection over field injection in tests.Using
@Autowiredon fields is a common pattern in tests, but constructor injection can improve test clarity and facilitate mocking if needed. However, this is more of a stylistic preference for many teams.Also applies to: 48-49, 51-52, 54-55, 57-58
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (2)
980-982: Minor improvement to inline comment wording.Line 980’s comment could be more precise grammatically (e.g., “there exist at least two channels with unread messages”). Functionally, these lines correctly create the required channels for unread message testing.
- // ensure there exist at least two channel with unread messages in the course + // ensure there are at least two channels with unread messages in the course
996-997: Safe handling of Optional result.When accessing
conversationParticipant.get(), ensure that you have confirmed theOptionalis present (e.g., “.orElseThrow()”). This practice prevents potentialNoSuchElementExceptionif no participant is found.- var conversationParticipant = conversationParticipantRepository.findConversationParticipantByConversationIdAndUserId(channel.getId(), instructor1.getId()); - await().untilAsserted(() -> assertThat(conversationParticipant.get().getUnreadMessagesCount()).isZero()); + var optionalParticipant = conversationParticipantRepository.findConversationParticipantByConversationIdAndUserId(channel.getId(), instructor1.getId()); + await().untilAsserted(() -> { + assertThat(optionalParticipant) + .isPresent() + .map(cp -> cp.getUnreadMessagesCount()) + .contains(0L); + });src/test/java/de/tum/cit/aet/artemis/communication/test_repository/ConversationTestRepository.java (1)
15-20: Consider adding pagination or limits for performance.While this repository method appears valid for testing, returning all conversations by course ID may become inefficient if a course has a large number of conversations. Adding support for pagination or limits could enhance performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/main/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java(3 hunks)src/main/webapp/app/entities/metis/conversation/channel.model.ts(2 hunks)src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-detail-channel-modal.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts(2 hunks)src/main/webapp/app/shared/metis/conversations/channel.service.ts(3 hunks)src/main/webapp/app/shared/metis/conversations/conversation.service.ts(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/test_repository/ConversationTestRepository.java(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts(2 hunks)src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-detail-channel-modal.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (21)
src/test/java/de/tum/cit/aet/artemis/communication/test_repository/ConversationTestRepository.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/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-detail-channel-modal.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.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/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.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/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts (1)
src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-detail-channel-modal.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/dialogs/channels-create-dialog/channels-create-dialog.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/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/metis/conversations/conversation.service.ts (1)
src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.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/ConversationIntegrationTest.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/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.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/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/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.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/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/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/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (1)
src/main/webapp/app/entities/metis/conversation/channel.model.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.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/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.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
🔇 Additional comments (65)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (5)
17-18: Imports for SLF4J Logger are correctly added.
This aligns with standard logging practices and is consistent with typical usage patterns in Spring-based services.
49-49: TimeLogUtil import recognized.
Using TimeLogUtil for performance measurement is appropriate. Keep in mind that logging long-running operations may be beneficial for debugging, but confirm that debug logs won’t clutter production logs.
55-55: Logger declared with standard SLF4J patterns.
A static final logger is the recommended approach for thread-safe and consistent logging output.
431-460: Efficient multiple-updates approach looks good.
The two-step strategy (bulk updating existing participants, then creating new participants for course-wide channels) is a great way to reduce database calls, which aligns with the PR objective of improving performance. However, please verify concurrency aspects:
- Ensure partial updates do not lead to inconsistent states if another parallel call modifies the same participants.
- Confirm or document the transactional boundaries for these operations.
477-477: Documentation now matches set return type.
Updating the Javadoc to reflect that the method returns a set resolves any confusion about the returned collection type.
src/main/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.java (1)
17-17: Import addition is appropriate.
No issues found with adding this import for the Channel entity.
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts (1)
38-39: Good use of nullish coalescing for boolean defaults.
Using ?? false ensures that the properties are always assigned valid boolean values.
src/main/webapp/app/entities/metis/conversation/channel.model.ts (2)
22-25: Ensures consistent default false for Channel booleans.
Defining default values for these properties avoids potential null or undefined references.
45-50: Proper introduction of default boolean values in the DTO.
Providing defaults here further aligns the client model with the server-side changes, minimizing null checks.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-detail-channel-modal.component.ts (1)
43-44: Robust assignment with fallback to false.
Using logical OR (|| false) ensures these fields hold a defined boolean value for safer usage.
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts (2)
10-10: Updated import for ChannelDTO is valid.
Adapting to use ChannelDTO in tests aligns with the new model structure.
84-87: Validation of form data is appropriate.
Ensuring expectedChannel is built from a ChannelDTO instance helps maintain consistency with production code. The usage of the non-null assertion operator (!) is acceptable here but confirm that formData is always defined at this point.
src/main/webapp/app/shared/metis/conversations/channel.service.ts (3)
7-7: Import usage looks good.
This import is correctly referencing a utility method, complying with the Angular style guidelines on code reuse.
53-57: Properly converting server dates in create/update.
Using the convertDateFromServer method ensures consistent date handling and reduces potential date-parsing errors.
101-114: Date conversion logic is well-structured.
Encapsulating date conversion in convertServerDates and calling it in convertDateFromServer aligns with the single-responsibility principle. These arrow functions also conform to Angular style guidelines (arrow_funcs, open_braces_same_line, etc.).
src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-detail-channel-modal.component.spec.ts (1)
74-80: Consistent initialization of channel properties.
Setting default boolean values to false in tests helps clarify expected behavior and avoids null or undefined issues. Looks good.
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.java (3)
6-6: Necessary import added.
The addition of List is required for the newly introduced method signatures. No issues here.
64-74: Efficient batch update with async.
This updateMultipleLastReadAsync method improves performance for bulk updates. Make sure to confirm that concurrent updates remain consistent with your transactional settings.
77-84: Query method naming is clear and consistent.
The method findConversationIdsByUserIdAndCourseId expressly matches the query usage. This naming convention is consistent with standard Spring Data patterns.
src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.java (19)
18-18: Use of primitive boolean with default value
Switching from Boolean to boolean with a default of false improves code safety by avoiding null checks. Good job adhering to the coding guideline of preferring primitives when possible.
20-20: Consistent booleans across the DTO
Having isAnnouncementChannel as a primitive boolean is consistent with isPublic. This unifies the data model and reduces potential null-pointer issues.
22-22: Clarity of default states
Initializing isArchived to false explicitly clarifies the default state for new channels. Well done.
24-24: Default course-wide channel setting
Setting isCourseWide to false clearly communicates that not all channels are course-wide. This is logical and consistent with typical use.
30-30: Meaningful documentation
The comment explains that hasChannelModerationRights is derived from the user’s course role. Great job clarifying the difference from being an actual channel moderator.
36-36: Aligned property nomenclature
isChannelModerator aligns with the existing naming scheme, maintaining clarity.
102-102: Getter name matches property
getIsPublic() accurately reflects the property name, minimizing confusion.
106-106: Setting booleans
setIsPublic is consistent with the naming style; the code is succinct and clear.
110-110: Getter usage
getIsAnnouncementChannel() fosters consistent naming.
114-114: Setter consistency
setIsAnnouncementChannel properly updates isAnnouncementChannel. Straightforward and correct.
118-118: Getter for isArchived
getIsArchived() is logically named, matching the field.
122-122: Setter for archived
setIsArchived properly sets isArchived.
126-126: Getter naming
getHasChannelModerationRights() indicates the user has some channel-level privileges, making this straightforward.
130-130: Channel moderation rights
Using boolean ensures no need for null checks on hasChannelModerationRights.
134-134: Getter for isChannelModerator
Follows the established naming pattern.
138-138: Setter for isChannelModerator
Aligns perfectly with the field name.
166-166: Getter for isCourseWide
No issues spotted; good naming consistency.
170-170: Setter for isCourseWide
Clear, consistent property management.
200-210: DTO-to-entity conversion
The toChannel() method provides a clean, single responsibility approach for mapping ChannelDTO to Channel. Good job separating concerns of conversion from the rest of the DTO logic.
src/main/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.java (10)
60-60: Primitive boolean field
Using boolean for isPublic with a default false is consistent with channel states. This helps avoid unnecessary null checks.
67-67: Boolean property for announcements
isAnnouncementChannel as a primitive type removes ambiguity for channels that are explicitly or implicitly not announcement channels.
75-75: Archived flag
Using a primitive boolean for isArchived with documentation clarifies the lifecycle state of a channel.
101-101: Constructor signature changes
The constructor now requires primitive booleans for isPublic, isAnnouncementChannel, and isArchived. This fosters consistent usage across the codebase.
138-138: Getter alignment
getIsPublic() is consistent with the naming approach in ChannelDTO.
142-142: Setter alignment
setIsPublic(boolean isPublic) sets the property straightforwardly.
155-155: Getter for archived
Reflective of the underlying field; no issues.
159-159: Setter for archived
Property assignment is clear, matching the field structure.
163-163: Getter for announcement
Matches other boolean getters, ensuring consistency.
167-167: Setter for announcement
No concerns; naming aligns well with the field.
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (2)
5-5: Selective imports
Removing unnecessary imports or retaining only what’s needed (e.g., ConversationDTO) improves clarity and reduces overhead. Good practice.
14-14: Efficient date conversion
convertDateFromServer is used consistently. The separate function fosters smaller, focused methods as recommended by Angular style guides.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (5)
33-33: Required input property
Using input.required<ConversationDTO>() is a solid approach, ensuring that activeConversation is never undefined. This enforces reliable usage within the component.
93-93: Effective participant check
this.canBeRemovedFromConversation = !this.isCurrentUser && this.canRemoveUsersFromConversation(...) is straightforward. It ensures you can only remove others if you have permissions.
95-96: Local variable for clarity
Extracting channelDTO from this.activeConversation() clarifies property usage. This aligns with the DRY principle.
97-97: Moderator role enabling
this.canBeGrantedChannelModeratorRole = ... && !this.conversationMember()?.isChannelModerator; effectively prevents reassigning an existing mod. Good logic.
99-99: Avoid revoking channel ownership
this.canBeRevokedChannelModeratorRole = ... && !this.isCreator; ensures the channel creator remains a moderator. This is a neat guard clause to prevent unexpected role changes.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java (3)
260-260: Verify usage of the new void return type.
Previously, this method returned a Channel. Now it's void. Ensure no downstream code relies on the returned object. Otherwise, consider returning the created channel if it’s necessary.
289-289: Check for consistency in exam channel creation.
Similar to the lecture channel creation at line 260, this method also changed from returning a Channel to returning void. Confirm that the calling code doesn’t require the created Channel object.
430-430: Ensure consistent usage of ChannelDTO#toChannel().
Applying channelDTO.toChannel() here avoids duplication, aligning with the new approach. Verify that all channel properties within channelDTO are properly set before conversion.
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (4)
89-89: Constructor injection looks good; verify interface usage.
Using storage and service classes in the constructor fosters clarity. Please confirm that TutorialGroupChannelManagementService usage is consistent with the rest of the code (e.g., ChannelService, etc.).
224-224: Restrict channels starting with certain prefixes.
Same as in ChannelService line 426, ensure consistency across front-end and back-end checks to avoid partial validations.
489-494: Changing to POST is semantically appropriate for an action.
Using POST to mark resources as read is consistent with an action-driven endpoint. This is a good alignment with RESTful conventions.
502-502: Validate performance for marking all conversations as read.
Be mindful of callback or query complexity. Marking all conversations as read in a large course might be heavy. Test with large data sets to ensure performance remains acceptable.
src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.java (1)
210-210: Clear assertion ensures only channels are returned.
This line succinctly verifies that each item is a ChannelDTO. Good approach to ensure type correctness in test logic.
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (2)
5-5: Approved import for handling asynchronous operations in tests.
The static import of await from Awaitility is appropriate for managing and verifying asynchronous conditions in integration tests.
992-993: Ensure idempotent test setup or teardown.
Line 992 fetches a user from the repository, and line 993 triggers a POST to a custom endpoint for marking channels as read. This pattern is fine for test coverage, but verify that the test environment is left cleanly so multiple test runs won't cause side effects.
...e/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java
Show resolved
Hide resolved
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.8.0)src/main/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
WalkthroughThis pull request introduces comprehensive modifications to the conversation and channel management system in the Artemis application. The changes primarily focus on refactoring Boolean properties in various classes, converting Changes
Sequence DiagramsequenceDiagram
participant Client
participant ChannelResource
participant ChannelService
participant ChannelRepository
Client->>ChannelResource: Create Channel Request
ChannelResource->>ChannelService: createChannel(channelDTO)
ChannelService->>ChannelRepository: save(channel)
ChannelRepository-->>ChannelService: savedChannel
ChannelService-->>ChannelResource: channelDTO
ChannelResource-->>Client: ResponseEntity<ChannelDTO>
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 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (18)
src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.java (5)
45-46: Prefer using constructor injection over field injection.
While field injection works, constructor injection is usually recommended for mandatory dependencies in Spring-based applications because it facilitates testing, enforces immutability, and is aligned with best practices. If the project guidelines specifically allow or encourage field injection for test classes, then this can be acceptable.
48-49: Prefer using constructor injection over field injection.
For consistency and best practices, consider using constructor injection here as well.
51-52: Prefer using constructor injection over field injection.
Constructor injection generally yields better testability and clarity regarding mandatory dependencies.
54-55: Prefer using constructor injection over field injection.
Stick to a single injection style across the codebase for improved maintainability and clarity.
57-58: Prefer using constructor injection over field injection.
Unifying the injection strategy helps keep test classes consistent and easily testable.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (1)
394-394: Add the@Asyncannotation to fulfill the TODO commentThe method is flagged with a
TODOnote indicating it should run asynchronously. Consider adding the@Asyncannotation and ensuring thread safety where necessary to avoid potential concurrency issues.- // TODO: this should be Async + @Async // Making the method asynchronous public void notifyClientAboutConversationCreationOrDeletion(Conversation conversation, User user, User responsibleUser, NotificationType notificationType) { notifyRecipientWithNotificationType(new ConversationNotificationSubject(conversation, user, responsibleUser), notificationType, null, null); }src/main/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.java (1)
94-106: Consider rewriting the subselect with a JOIN
Using a direct JOIN instead of a subselect may improve performance and maintainability.src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1)
431-431: Minor grammar improvement in Javadoc.
Consider "Mark all conversations of a user" instead of the singular form.src/main/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.java (1)
42-43: Consider converting Booleans to boolean
The existing TODO suggests usingbooleanwhere a default false state is desired. Converting these fields can remove null checks and improve consistency with the rest of the codebase.Would you like guidance on systematically replacing these fields and updating corresponding code references?
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.java (1)
77-84: Ensure correct usage of conversation IDs
ThefindConversationIdsByUserIdAndCourseIdmethod might return a large result set for large courses. Verify that the consuming code handles this efficiently. Consider pagination or chunked processing if results can be very large.src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.java (2)
9-9: Consider converting to a Java record
The TODO comment suggests refactoring this DTO into a record. Doing so can reduce boilerplate and improve immutability.
102-138: Getter/Setter naming convention
The usage ofgetIsPublic,getIsArchived, etc., is acceptable, but consider aligning them with typical Java naming conventions such asisPublic()/setPublic()orisAnnouncementChannel()/setAnnouncementChannel(). This is more idiomatic and consistent.- public boolean getIsPublic() { + public boolean isPublic() { return isPublic; }src/main/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.java (1)
Line range hint
138-167: Consistent naming for boolean methods
As withChannelDTO, consider renaminggetIsPublic()toisPublic(), etc. This keeps naming uniform across the codebase.- public boolean getIsPublic() { + public boolean isPublic() { return isPublic; }src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.java (1)
75-76: Split archived vs announcement logic for clarity.Lines 75-76 rely on two booleans (
isAnnouncementChannel,isArchivedChannel). Consider grouping them for readability:if (channel.getIsArchived()) { // ... } else if (channel.getIsAnnouncementChannel()) { // ... }This ensures a more straightforward branching structure.
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (3)
224-224: Prevent potentially confusing channel names.This
$-starting name restriction is consistent with other channel creation checks. Confirm that front-end validations or user instructions clarify this restriction to avoid user confusion.
228-228: Double-check concurrency on the.toChannel()call.Line 228 calls
channelDTO.toChannel()while also injecting the requesting user. Consider concurrency or immutability concerns if multiple threads attempt concurrent channel creation with the same DTO.
489-502: POST endpoint improvement for marking channels as read.Switching from PUT to POST is more semantically correct if marking channels as read is an action. Confirm that any front-end calls and client code are updated to use POST.
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (1)
980-982: Create multiple unread channels for robust test coverage
Having at least two channels ensures that the “mark all read” feature is checked in a scenario covering multiple conversations. Ensure that you also consider edge cases such as when there is only one unread channel or none at all.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/main/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java(3 hunks)src/main/webapp/app/entities/metis/conversation/channel.model.ts(2 hunks)src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-detail-channel-modal.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts(2 hunks)src/main/webapp/app/shared/metis/conversations/channel.service.ts(3 hunks)src/main/webapp/app/shared/metis/conversations/conversation.service.ts(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/test_repository/ConversationTestRepository.java(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts(2 hunks)src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-detail-channel-modal.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (21)
src/test/java/de/tum/cit/aet/artemis/communication/test_repository/ConversationTestRepository.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/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.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/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.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/programming-exercise/feedback-analysis/modals/feedback-detail-channel-modal.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/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.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/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.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/conversations/conversation.service.ts (1)
src/main/webapp/app/shared/metis/conversations/channel.service.ts (1)
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/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.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/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts (1)
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/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (1)
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-detail-channel-modal.component.ts (1)
src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.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/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/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.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/entities/metis/conversation/channel.model.ts (1)
src/main/java/de/tum/cit/aet/artemis/communication/dto/ConversationDTO.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/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.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/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
🔇 Additional comments (40)
src/test/java/de/tum/cit/aet/artemis/communication/ConversationIntegrationTest.java (1)
210-210: Assertion approach is sound.
This succinctly verifies that all returned items are instances of ChannelDTO.
src/main/java/de/tum/cit/aet/artemis/communication/repository/conversation/ConversationRepository.java (1)
17-17: No issues with the new import.
No functional changes introduced.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (7)
17-18: Logger import is correct.
No issues found.
49-49: No issues with TimeLogUtil import.
Ensures consistent time-logging capabilities.
55-56: Using a dedicated logger.
This approach is recommended for better log organization and performance tracking.
437-443: Efficient bulk update approach.
This is a good use of a single query to update all conversation participants.
444-446: Debug logging.
Capturing timing information here will help diagnose performance issues.
447-460: Creating new participants for course-wide channels.
This second pass ensures users aren’t missed for channels they haven’t accessed yet.
477-477: Clear documentation.
The Javadoc properly describes the returned set of users.
src/test/java/de/tum/cit/aet/artemis/communication/test_repository/ConversationTestRepository.java (1)
15-20: Method name and Javadoc.
The method is clearly documented and follows a consistent naming convention.
src/main/webapp/app/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.ts (1)
38-39: Nullish coalescing usage.
Good approach to ensure a default value of false for both properties.
src/main/webapp/app/entities/metis/conversation/channel.model.ts (2)
22-25: Default boolean values in Channel.
Setting these to false by default removes ambiguity and simplifies usage.
45-50: ChannelDTO property initialization.
Initializing these fields to false aligns with the domain model and ensures type safety.
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-create-dialog/channels-create-dialog.component.spec.ts (2)
10-10: Use of ChannelDTO import is appropriate
Good job switching from Channel to ChannelDTO. This is consistent with the new DTO-based approach and helps ensure correct type usage in the tests.
84-87: Confirm the non-null assertion
Using formData.isPublic! is acceptable in tests if you are sure isPublic is never null. As an alternative, consider safely handling null or undefined to improve test robustness.
src/main/webapp/app/shared/metis/conversations/channel.service.ts (4)
7-7: Import for date conversion looks good
This new import for convertDateFromServer helps streamline date handling in the service. Ensure thorough testing around time zones or locale-specific pitfalls.
53-53: Properly mapping the post response with date conversion
Chaining .pipe(map(this.convertDateFromServer)) ensures post-creation responses have correct date conversions. This aligns with best practices for handling server date fields.
57-57: Consistent date conversion on update
Applying the same .pipe(map(this.convertDateFromServer)) pattern on updates ensures uniformity and avoids stale or mis-converted date fields.
101-114: Centralized date conversion
Introducing convertDateFromServer and convertServerDates within this service is a clean, maintainable approach that reinforces single responsibility.
src/test/javascript/spec/component/programming-exercise/feedback-analysis/modals/feedback-detail-channel-modal.component.spec.ts (1)
74-80: Explicitly setting default false states
Setting these boolean fields to false by default aligns with the new DTO convention and removes ambiguity. Validate that you do not overwrite user choices unintentionally.
src/main/java/de/tum/cit/aet/artemis/communication/repository/ConversationParticipantRepository.java (1)
64-74: Validate asynchronous update logic
The addition of the updateMultipleLastReadAsync method provides a bulk update for multiple conversations. Ensure that other code paths calling this method handle potential race conditions in asynchronous workloads (e.g., user concurrency scenarios).
src/main/java/de/tum/cit/aet/artemis/communication/dto/ChannelDTO.java (2)
18-36: Primitive booleans with defaults
Converting from Boolean to primitive boolean with default values is consistent with the recommended guidelines. This approach prevents null pointer issues and improves clarity.
199-210: Ensure alignment between ChannelDTO and Channel
The toChannel() method correctly sets the updated fields, including the new primitive booleans. Confirm that additional fields (e.g., tutorialGroupId, tutorialGroupTitle) do not need to be transferred to the domain model or handled in a future refactoring.
src/main/java/de/tum/cit/aet/artemis/communication/domain/conversation/Channel.java (2)
60-75: Primitive fields and default values
Replacing Boolean with boolean aligns with the best practice of avoiding wrappers where null is not a valid state. The default false assignments eliminate potential null references.
101-101: Compact constructor
The constructor now supports primitive booleans without checks for null. This is more streamlined and follows Java best practices. Ensure that all callers of this constructor pass correct boolean values and that relevant validations are performed, if needed.
src/main/webapp/app/shared/metis/conversations/conversation.service.ts (3)
5-5: Ensuring type definitions
Make sure the ConversationDTO import is used effectively; if referencing external types, verify that TypeScript type definitions are consistent to prevent runtime issues.
14-14: Efficient date conversion
Using convertDateFromServer from app/utils/date.utils is good for centralized date handling. Confirm that all date-like fields in ConversationDTO are consistently converted.
181-181: HTTP method switch from PUT to POST
Switching to POST for markAllChannelsAsRead can simplify the endpoint usage. However, confirm the semantic correctness if the action is idempotent. Typically, PUT is used when an update is fully known. POST may be appropriate if the request is more of an action without an existing resource.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (2)
33-33: Ensure input usage is well documented and validated.
The switch to input.required<ConversationDTO>() enforces that activeConversation must be provided. Confirm that parent components supplying activeConversation always pass a valid object, and consider including defensive checks or relevant error messages for debugging.
[approve]
93-99: Align removal and role-granting logic for channel creators.
Within the channel logic block:
- Line 96 tightens removal eligibility by checking
!this.isCreator && !channelDTO.isCourseWide. This is good but ensure it remains consistent with policies in the backend (e.g., channel creators not removable). - Line 99 similarly excludes channel creators from losing moderator privileges. Confirm that these rules match channel-creator invariants across the system for consistency.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/auth/ChannelAuthorizationService.java (1)
57-57: Primitive usage improves clarity, but confirm default values.
The direct check if (isArchivedChannel) {...} is clearer than null checks. Just be sure that isArchived always accurately defaults to false upon creation, matching the domain constraints.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ChannelService.java (4)
260-260: Method no longer returns a Channel object.
Converting from Channel createLectureChannel(...) to void createLectureChannel(...) is valid if the caller doesn’t need the return object. Verify any callers do not inadvertently rely on the previously returned channel.
289-289: Method no longer returns a Channel object.
Similarly, ensure that removing the return value from createExamChannel(...) does not break existing usage. Confirm that exam is always updated with the new channel name as needed.
302-310: Adjust lecture channel updates carefully.
The lines within updateLectureChannel (302, 304, 308, 310) show early returns if conditions aren’t met (e.g., channelName == null, channel == null). While this is concise, ensure it doesn’t skip logic that must occur, e.g., further validations or state updates, in edge cases.
[approve]
426-430: Validate user-defined channel names thoroughly.
At line 426, we disallow $ as a starting character. Consider systematically validating all user-created channel names and confirm that name-based restrictions match the rest of the codebase (e.g., CHANNEL_NAME_REGEX).
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
89-89: Removing StudentParticipationRepository from the constructor is a good simplification.
Ensure no remaining references in the class or downstream calls require it.
src/test/java/de/tum/cit/aet/artemis/communication/ChannelIntegrationTest.java (3)
5-5: Good use of Awaitility for asynchronous testing
This import enables explicit waiting for asynchronous operations to complete, thereby improving reliability of the test when verifying state changes in the database.
992-993: Confirm endpoint behavior with multiple scenarios
The POST request to mark-as-read works as expected here, but consider adding negative or error scenarios to further validate error handling and concurrency aspects.
996-997: Asynchronously validate unread message count
Using await().untilAsserted is appropriate to handle eventual consistency in the database. Confirm that the default polling intervals are suitable for the build environment to avoid flaky tests.
src/main/webapp/app/exercises/programming/manage/grading/feedback-analysis/Modal/feedback-detail-channel-modal.component.ts (1)
43-44: Ensure default boolean values for form entries
Setting isPublic and isAnnouncementChannel to false by default prevents accidental null or undefined states in the ChannelDTO. This aligns well with the shift to primitive boolean usage across the application.
cremertim
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I realised a weird behaviour when writing new messages. Your own message is marked as a new, not read message. I haven't found a reliable way to reproduce this issue so far. Sometimes it happens, and sometime it does not happen
unknown_2024.12.27-16.38_1_clip_1.mp4
There was a problem hiding this comment.
tested on ts2 and works as expected.
however, I encountered a UI issue related with this feature:
When I use Artemis in German, the options in the "+" dropdown overlap with other UI elements and become partially invisible. This happens because the string 'Alle Kanäle als gelesen markieren' is long; the issue disappears when replaced with a shorter string. I think this can be solved in a follow-up PR
sawys777
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS4, I've posted messages as User 1 and User 2 in the communication channels of Practical Course: Interactive Learning SS24, everything works as expected except for one time which I have to sign out and then sign in again to notice the intended changes.
krusche
dismissed stale reviews from sawys777, asliayk, cremertim, and coderabbitai[bot]
via
66cd503
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/test/javascript/spec/component/shared/sidebar/conversation-options.component.spec.ts (1)
34-37: Consider removing unnecessary type assertions or adjusting the function signature.While explicitly casting these objects to
ChannelDTOensures type clarity, it may indicate that the signature ofgenerateExampleChannelDTOcould be made more explicit. If the helper function reliably returns aChannelDTO, updating the function signature could remove the need to cast here. Please verify whether these assertions are necessary.src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts (1)
37-37: Consider returning typed objects directly from the generator functions.Although casting is a convenient shortcut, it may be cleaner to have the
generateExampleGroupChatDTOandgenerateExampleChannelDTOfunctions directly return properly typed objects if feasible. This would avoid the need for explicit casts in your test code.Here’s a potential diff for refining the generator functions, if you have access to them:
- export function generateExampleGroupChatDTO(overrides: Partial<GroupChatDTO>): ConversationDTO { ... } + export function generateExampleGroupChatDTO(overrides: Partial<GroupChatDTO>): GroupChatDTO { ... } - export function generateExampleChannelDTO(overrides: Partial<ChannelDTO>): ConversationDTO { ... } + export function generateExampleChannelDTO(overrides: Partial<ChannelDTO>): ChannelDTO { ... }src/test/javascript/spec/service/notification.service.spec.ts (1)
518-520: Consider streamlining test data creation for maintainability.You are manually casting the conversation and answers to
ChannelandUserobjects in lines 518–520. While this helps enforce type correctness in tests, consider using a dedicated factory or builder pattern for generating test data. This approach would make your tests clearer and more resilient to future changes in theChannelorUsermodels.src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts (3)
64-64: Consider omitting redundant casts.The
as ChannelDTOcast may be redundant if thegenerateExampleChannelDTOfunction already returns aChannelDTO.- channel = generateExampleChannelDTO({ id: 3 } as ChannelDTO); + channel = generateExampleChannelDTO({ id: 3 });
199-199: Avoid redundant casts.Same reasoning as line 64. The cast to
ChannelDTOmight be unnecessary ifgenerateExampleChannelDTOalready returns the correct type.- const newChannel = generateExampleChannelDTO({ id: 99 } as ChannelDTO); + const newChannel = generateExampleChannelDTO({ id: 99 });
425-425: Accessing private fields can lead to brittle tests.Tests that depend on private fields (
metisConversationService['conversationsOfUser']) may become fragile if the internal structure changes. Consider using an accessor or public method that returns the relevant data.src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
48-53: Consider removing redundant casts for clarityThese casts to
OneToOneChatDTO,GroupChatDTO, andChannelDTOmay not be necessary if the helper functions already return properly typed DTOs. Avoiding superfluous casts can make the tests clearer and more maintainable.-const examples: (ConversationDTO | undefined)[] = [ - undefined, - generateOneToOneChatDTO({} as OneToOneChatDTO), - generateExampleGroupChatDTO({} as GroupChatDTO), - generateExampleChannelDTO({} as ChannelDTO), -]; +const examples: (ConversationDTO | undefined)[] = [ + undefined, + generateOneToOneChatDTO({}), + generateExampleGroupChatDTO({}), + generateExampleChannelDTO({}), +];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-overview-dialog/channel-item/channel-item.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-overview-dialog/channels-overview-dialog.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/add-users-form/conversation-add-users-form.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.spec.ts(2 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(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.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(1 hunks)src/test/javascript/spec/component/overview/course-conversations/services/channel.service.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/services/conversation-permissions.util.spec.ts(10 hunks)src/test/javascript/spec/component/overview/course-conversations/services/conversation.service.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts(5 hunks)src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts(2 hunks)src/test/javascript/spec/component/shared/sidebar/conversation-options.component.spec.ts(1 hunks)src/test/javascript/spec/service/notification.service.spec.ts(1 hunks)tsconfig.spec.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.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/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.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/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.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/dialogs/channels-overview-dialog/channel-item/channel-item.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/dialogs/conversation-detail-dialog/conversation-detail-dialog.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/programming-exercise/feedback-analysis/feedback-analysis.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/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/service/notification.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/test/javascript/spec/component/overview/course-conversations/services/channel.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/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}}
src/test/javascript/spec/component/overview/course-conversations/services/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/test/javascript/spec/component/overview/course-conversations/services/conversation-permissions.util.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/shared/sidebar/conversation-options.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/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.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/dialogs/channels-overview-dialog/channels-overview-dialog.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/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/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/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/add-users-form/conversation-add-users-form.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/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.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 (49)
tsconfig.spec.json (1)
5-5: LGTM! Verify jest-extended installation.
The addition of jest-extended types will enhance testing capabilities by providing additional matchers, which aligns well with the PR's focus on comprehensive testing.
Let's verify the jest-extended package is properly configured:
✅ Verification successful
Jest-extended is properly configured and installed
The verification confirms:
- jest-extended v4.0.2 is correctly listed in package.json
- The package is properly imported in the test setup file at
src/test/javascript/spec/jest-test-setup.ts
Let's verify the actual import statement to ensure it's correctly configured:
Jest-extended setup is complete and correct
The verification confirms that jest-extended is:
- Properly installed (v4.0.2 in package.json)
- Correctly imported in the test setup file using the recommended import statement
- The
typeRootsconfiguration in tsconfig.spec.json correctly points to its type definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify jest-extended package installation and configuration
# Check if jest-extended is listed in package.json
echo "Checking package.json for jest-extended..."
cat package.json | jq '.dependencies["jest-extended"] // .devDependencies["jest-extended"]'
# Check if jest-extended is properly imported in test setup
echo "Checking test setup file for jest-extended import..."
rg -l "jest-extended" "src/test/javascript/spec/jest-test-setup.ts"
Length of output: 421
Script:
#!/bin/bash
# Check the actual import statement in the test setup file
echo "Checking jest-extended import statement..."
rg "jest-extended" "src/test/javascript/spec/jest-test-setup.ts" -A 1
Length of output: 236
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/add-users-form/conversation-add-users-form.component.spec.ts (5)
11-11: Good use of strongly-typed imports.
Importing GroupChatDTO enforces type safety in your tests, ensuring that the test data matches the expected contract.
14-14: Consistent approach with ChannelDTO.
Using an explicit import for ChannelDTO and isChannelDTO clarifies your intentions regarding channel-based structures in the tests.
19-19: Examples array now references typed DTO objects.
Explicitly casting test data to GroupChatDTO and ChannelDTO ensures the code’s correctness and maintainability by surfacing potential type mismatches early. Looks good.
168-168: Appropriate usage of async/await.
Switching the clickSubmitButton to an async function aligns with modern best practices in Angular test suites and improves readability over promise chaining.
175-183: Clear and specific Jest expectations.
Your test logic correctly verifies that submitForm and the emit event are triggered only when expectSubmitEvent is true, following best practices for granular test coverage. The usage of toHaveBeenCalledOnce() matches the recommended approach for verifying single calls. Nicely done!
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (2)
20-20: Good practice importing the strongly typed model.
Importing ChannelDTO and getAsChannelDTO together helps maintain consistency across the test file, aligning it with the new type usage and improving type safety throughout the codebase.
27-28: Explicit casting ensures proper type-checking.
Casting to ChannelDTO enforces type constraints and helps reveal potential inconsistencies or missing properties, especially if future changes alter the ChannelDTO structure. This approach reduces null-related issues and streamlines test setup.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-add-users-dialog/conversation-add-users-dialog.component.spec.ts (1)
19-20: Good addition for clearer type safety.
Importing both ChannelDTO and GroupChatDTO—along with the corresponding type-check functions—helps ensure that your test suite handles conversation types more explicitly. This improves test clarity and maintainability.
src/test/javascript/spec/component/programming-exercise/feedback-analysis/feedback-analysis.service.spec.ts (2)
6-6: Good use of a specific DTO import.
This direct import helps maintain type safety without resorting to excessive module imports. This follows the guideline to avoid full module imports.
156-156: Appropriate explicit typing for the channel DTO mock.
Casting the object to ChannelDTO ensures the mock accurately reflects the expected structure, fostering more robust tests.
src/test/javascript/spec/component/overview/course-conversations/services/conversation-permissions.util.spec.ts (17)
22-22: Good use of type assertions to enforce correctness
Your addition of as ChannelDTO ensures stricter typing for generateExampleChannelDTO({ isMember: true }). This helps avoid subtle typing issues during refactors.
29-29: Consistent approach to boolean checks
Using .toBeFalse() aligns with the recommended Jest-specific boolean check from the coding guidelines.
33-33: Maintaining test readability
The concise test name combined with a direct check (.toBeFalse()) keeps the test understandable and consistent with naming conventions.
37-37: Ensuring type safety with explicit channel properties
Adding as ChannelDTO clarifies the channel's shape, strengthening type checks for properties like isAnnouncementChannel.
41-43: Good job adhering to test clarity
Breaking assertions into multiple lines improves readability. The test logic is clear and concise.
49-49: Effective use of multiple properties in type assertions
Including hasChannelModerationRights in the struct clarifies that the user has enhanced privileges, ensuring the tested scenario is explicit.
56-56: Excellent test coverage for membership checks
Verifying membership behavior ensures robust validation of user-based channel permissions.
75-75: Properly clarifying authorized channel state
Using isArchived: false in combination with hasChannelModerationRights: true explicitly covers the scenario where the user can add new members.
101-101: Comprehensive scenario handling
Your addition of isPublic: false ensures realistic coverage of private channels in removal scenarios.
133-133: Detail-oriented approach for moderator states
Ensuring that the channel has no moderator privileges sets up boundary conditions for negative tests, increasing reliability.
143-143: Thorough test scenario coverage
Including a channel where the creator is also a channel moderator tests combined privileges in deletion scenarios.
154-154: Refined typing for tutorial group channels
Specifying tutorialGroupId and tutorialGroupTitle ensures that tutorial-group-specific logic remains statically verifiable.
161-161: Ensuring correct handling of channel moderation rights
Explicitly verifying hasChannelModerationRights: true helps confirm that granting moderator status is restricted to authorized users.
173-173: Reliable test for revoking accessible roles
The property hasChannelModerationRights: true sets a clear condition for validating role revocation.
185-185: Straightforward test for channel archival
Using hasChannelModerationRights: true is consistent with archival permission checks. Excellent short, clear attribute usage.
197-197: Readability and maintainability in test data
Naming channelThatCanBeChanged is self-explanatory and indicates the channel’s modifiability.
219-219: Explicitly typed all fields for channel membership
Finalizing the channel object with as ChannelDTO preserves type consistency, ensuring the object’s properties align with the domain model.
src/test/javascript/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts (2)
15-15: Imports look consistent and aligned with the updated model.
305-305: Check the default boolean properties in generated channel DTO.
Ensure that any new boolean fields introduced in the ChannelDTO model default to false (or the intended default) when using generateExampleChannelDTO.
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-overview-dialog/channel-item/channel-item.component.spec.ts (3)
6-6: New import is consistent and well-placed.
Importing ChannelDTO ensures strong typing and clarifies the intention of the test code.
14-14: Explicitly typing the channel object is a good practice.
Casting to ChannelDTO helps ensure the intended properties are available during testing.
57-57: Good coverage of non-member behavior.
Ensuring that the test includes a scenario where isMember = false verifies the UI elements’ condition.
src/test/javascript/spec/component/overview/course-conversations/services/conversation.service.spec.ts (2)
5-5: Importing ChannelDTO strengthens type safety.
This ensures that test objects conform to the domain model’s structure.
151-151: Ensuring correct type usage for channel test objects.
Casting to ChannelDTO explicitly clarifies the structure for better readability and maintainability.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (2)
15-15: New import for ChannelDTO looks correct.
This import aligns with the type safety improvements and ensures that tests referencing ChannelDTO remain consistent.
69-69: Typecasting generateExampleChannelDTO enhances consistency.
Casting to ChannelDTO helps maintain strict typing across the tests, matching the updated model definitions.
src/test/javascript/spec/component/overview/course-conversations/services/channel.service.spec.ts (1)
34-34: Casting elemDefault to ChannelDTO ensures type safety.
This approach matches the transition to primitive boolean fields in ChannelDTO and eliminates potential null checks.
src/test/javascript/spec/component/overview/course-conversations/dialogs/channels-overview-dialog/channels-overview-dialog.component.spec.ts (3)
26-27: Updated input and output properties are clear and consistent.
Using typed ChannelDTO and a well-defined ChannelAction event emitter promotes readability and reduces type mismatches.
31-34: Explicit type assertions on example channels improve reliability.
Casting each channel as ChannelDTO ensures strict typing and consistent test logic across multiple channel subtypes.
77-78: Constructing test channels with explicit casts is consistent.
These lines align with the rest of the codebase’s type-safe approach and avoid any hidden type conversion errors.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.spec.ts (2)
14-14: Importing ChannelDTO supports typed conversation references.
This import complements the updated model definitions and encourages type-safe usage throughout the component tests.
38-38: Applying type casting to generateExampleChannelDTO maintains consistency.
This ensures strict typing for channel-related conversations, mirroring the broader changes across the codebase.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts (1)
23-23: Casting inside test example array is acceptable
Casting the empty object to ChannelDTO is valid to ensure type-checking in the test context.
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-header/conversation-header.component.spec.ts (1)
35-38: Type assertions for channel objects
Using ChannelDTO assertions ensures clarity within the test data, preventing type mismatches.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.spec.ts (2)
23-23: Type assertion for test example
This addition preserves type safety for the mock channel data.
39-39: Consistent type assertion for updated channel data
Again, casting the object to ChannelDTO clarifies the shape of the updated data in tests.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (1)
45-46: Confirming type alignment for channel test objects
Casting these objects to ChannelDTO references ensures correct usage in the test setup.
src/test/javascript/spec/component/overview/course-conversations/course-conversations.component.spec.ts (1)
4-4: All good here!
The new import of OneToOneChatDTO is valid and properly referenced in the code below.
...ipt/spec/component/overview/course-conversations/services/metis-conversation.service.spec.ts
Show resolved
Hide resolved
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
The mark all read was implemented with a poor performance
Description
This PR improves the performance significantly
Steps for Testing
Prerequisites:
Communication: Allow users to mark all channels as read #9994Testserver 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
Performance Tests
Test Coverage
no changes
Screenshots
no changes
Summary by CodeRabbit
New Features
ChannelandChannelDTO, ensuring they default tofalse.ConversationDTOfor enhanced conversation management.Bug Fixes
Refactor
ChannelDTOdirectly.Documentation
Tests
ChannelDTO.