Development: Introduce tutorialgroup module API
#10266
Conversation
github-actions
bot
added
tests
server
WalkthroughThis pull request refactors several classes to replace direct repository and service dependencies with optional API interfaces for tutorial group functionality. Constructor signatures, field declarations, and method logic across communication, core, and web layers have been updated to use Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Service/Resource
participant API as TutorialGroupApi (Optional)
participant Repo as Repository
Caller->>API: Check presence
alt API present
Caller->>API: Call method (e.g., findByTutorialGroupChannelId)
API->>Repo: Retrieve data
Repo-->>API: Return data
API-->>Caller: Return result
else API not present
Caller-->>Caller: Handle absence (throw ApiNotPresentException or skip)
end
sequenceDiagram
participant U as User
participant CR as ChannelResource
participant TCM as TutorialGroupChannelManagementApi
U->>CR: Request channel deletion
CR->>TCM: getTutorialGroupBelongingToChannel(channel)
alt API present
TCM-->>CR: TutorialGroup found
CR->>TCM: deleteTutorialGroupChannel(tutorialGroup)
TCM-->>CR: Channel deleted
else API not present
CR-->>U: Return error or skip deletion
end
CR-->>U: Send deletion response
Suggested labels
Suggested reviewers
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/tutorialgroup/api/TutorialGroupCommunicationApi.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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: 2
🧹 Nitpick comments (10)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupNotificationApi.java (1)
11-13: Consider using @service instead of @controller.Since this class is not handling web requests but rather providing a service facade for tutorial group notifications,
@Servicewould be more appropriate than@Controller.@Profile(PROFILE_CORE) -@Controller +@Service public class TutorialGroupNotificationApi extends AbstractTutorialGroupApi {src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupRegistrationApi.java (2)
15-17: Consider using@Serviceinstead of@Controller.This class appears to be a service facade rather than a REST controller as it doesn't handle HTTP requests directly. Using
@Servicewould better reflect its role in the architecture.@Profile(PROFILE_CORE) -@Controller +@Service public class TutorialGroupRegistrationApi extends AbstractTutorialGroupApi {
19-23: Add null check for injected dependency.While Spring handles dependency injection, it's a good practice to add explicit null checks to fail fast and provide clear error messages.
public TutorialGroupRegistrationApi(TutorialGroupRegistrationRepository tutorialGroupRegistrationRepository) { + if (tutorialGroupRegistrationRepository == null) { + throw new IllegalArgumentException("TutorialGroupRegistrationRepository must not be null"); + } this.tutorialGroupRegistrationRepository = tutorialGroupRegistrationRepository; }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/TutorialGroupNotificationService.java (1)
99-100: Consider handling invalid registrations.While this maps each registration to its student, consider skipping entries with null or invalid students to avoid potential NPEs or incomplete data.
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java (1)
30-48: Add JavaDoc documentation for public methods.Each public method should be documented with JavaDoc to describe its purpose, parameters, return values, and any exceptions it may throw.
Apply this diff to add JavaDoc:
+ /** + * Find all tutorial group IDs associated with notifications for a user. + * @param user The user to find notifications for + * @return Set of tutorial group IDs + */ public Set<Long> findAllForNotifications(User user) { return tutorialGroupService.findAllForNotifications(user); } + /** + * Count tutorial groups associated with a course. + * @param course The course to count tutorial groups for + * @return Number of tutorial groups + */ public Long countByCourse(Course course) { return tutorialGroupRepository.countByCourse(course); } + /** + * Find all tutorial groups for a course. + * @param courseId ID of the course + * @return Set of tutorial groups + */ public Set<TutorialGroup> findAllByCourseId(Long courseId) { return tutorialGroupRepository.findAllByCourseId(courseId); } + /** + * Delete a tutorial group by its ID. + * @param id ID of the tutorial group to delete + */ public void deleteById(Long id) { tutorialGroupRepository.deleteById(id); } + /** + * Find a tutorial group by its channel ID. + * @param channelId ID of the channel + * @return Optional containing the tutorial group if found + */ public Optional<TutorialGroup> findByTutorialGroupChannelId(Long channelId) { return tutorialGroupRepository.findByTutorialGroupChannelId(channelId); }src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupChannelManagementApi.java (1)
30-40: Add JavaDoc documentation for public methods.Each public method should be documented with JavaDoc to describe its purpose, parameters, and return values.
Apply this diff to add JavaDoc:
+ /** + * Get the tutorial group associated with a channel. + * @param channel The channel to find the tutorial group for + * @return Optional containing the tutorial group if found + */ public Optional<TutorialGroup> getTutorialGroupBelongingToChannel(Channel channel) { return tutorialGroupChannelManagementService.getTutorialGroupBelongingToChannel(channel); } + /** + * Delete the channel associated with a tutorial group. + * @param tutorialGroup The tutorial group whose channel should be deleted + */ public void deleteTutorialGroupChannel(TutorialGroup tutorialGroup) { tutorialGroupChannelManagementService.deleteTutorialGroupChannel(tutorialGroup); } + /** + * Handle timezone updates for a course. + * @param course The course whose timezone has been updated + */ public void onTimeZoneUpdate(Course course) { tutorialGroupsConfigurationService.onTimeZoneUpdate(course); }src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationDTOService.java (2)
138-144: Consider extracting tutorial group mapping logic to a private method.The tutorial group mapping logic is duplicated in both
convertChannelToDTOmethods. Consider extracting it to a private method to improve code reusability and maintainability.+private void setTutorialGroupInfo(ChannelDTO channelDTO, Channel channel) { + if (tutorialGroupApi.isPresent()) { + var tutorialGroup = tutorialGroupApi.get().findByTutorialGroupChannelId(channel.getId()); + tutorialGroup.ifPresent(tg -> { + channelDTO.setTutorialGroupId(tg.getId()); + channelDTO.setTutorialGroupTitle(tg.getTitle()); + }); + } +} public ChannelDTO convertChannelToDTO(User requestingUser, Channel channel) { // ... existing code ... - if (tutorialGroupApi.isPresent()) { - var tutorialGroup = tutorialGroupApi.get().findByTutorialGroupChannelId(channel.getId()); - tutorialGroup.ifPresent(tg -> { - channelDTO.setTutorialGroupId(tg.getId()); - channelDTO.setTutorialGroupTitle(tg.getTitle()); - }); - } + setTutorialGroupInfo(channelDTO, channel); return channelDTO; }
168-174: Duplicate code: Use the extracted method here as well.This is the same tutorial group mapping logic as in the other method. Use the extracted method here.
-if (tutorialGroupApi.isPresent()) { - var tutorialGroup = tutorialGroupApi.get().findByTutorialGroupChannelId(channel.getId()); - tutorialGroup.ifPresent(tg -> { - channelDTO.setTutorialGroupId(tg.getId()); - channelDTO.setTutorialGroupTitle(tg.getTitle()); - }); -} +setTutorialGroupInfo(channelDTO, channel);src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (1)
288-290: Consider improving error message clarity.The error message could be more descriptive about why the channel cannot be deleted. Consider mentioning that tutorial group channels must be managed through the tutorial group interface.
-throw new BadRequestAlertException("The channel belongs to tutorial group " + tutorialGroup.getTitle(), CHANNEL_ENTITY_NAME, "channel.tutorialGroup.mismatch"); +throw new BadRequestAlertException("Cannot delete channel: It belongs to tutorial group '" + tutorialGroup.getTitle() + "'. Tutorial group channels must be managed through the tutorial group interface.", CHANNEL_ENTITY_NAME, "channel.tutorialGroup.mismatch");src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
350-352: Consider extracting time zone update logic to a private method.The time zone update logic could be extracted to a private method to improve readability and maintainability.
+private void handleTimeZoneUpdate(Course course) { + if (tutorialGroupChannelManagementApi.isPresent()) { + tutorialGroupChannelManagementApi.get().onTimeZoneUpdate(course); + } +} -if (timeZoneChanged && tutorialGroupChannelManagementApi.isPresent()) { - tutorialGroupChannelManagementApi.get().onTimeZoneUpdate(result); +if (timeZoneChanged) { + handleTimeZoneUpdate(result); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationDTOService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/TutorialGroupNotificationService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/NotificationResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java(9 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/AbstractTutorialGroupApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupChannelManagementApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupNotificationApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupRegistrationApi.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/tutorialgroup/architecture/TutorialGroupApiArchitectureTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/AbstractTutorialGroupApi.java
🧰 Additional context used
📓 Path-based instructions (12)
src/test/java/de/tum/cit/aet/artemis/tutorialgroup/architecture/TutorialGroupApiArchitectureTest.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/tutorialgroup/api/TutorialGroupChannelManagementApi.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/core/exception/ApiNotPresentException.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/tutorialgroup/api/TutorialGroupRegistrationApi.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/ConversationDTOService.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/core/service/CourseService.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/core/web/CourseResource.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/NotificationResource.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/tutorialgroup/api/TutorialGroupApi.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/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupNotificationApi.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/TutorialGroupNotificationService.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
📓 Learnings (3)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupRegistrationApi.java (1)
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-11-12T12:51:58.050Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
src/main/java/de/tum/cit/aet/artemis/communication/web/NotificationResource.java (1)
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-11-12T12:51:58.050Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/TutorialGroupNotificationService.java (1)
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-11-12T12:51:58.050Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (29)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupNotificationApi.java (2)
1-10: LGTM! Package structure and imports are well-organized.The package structure follows Java naming conventions, and imports are specific without using wildcard imports.
15-19: LGTM! Field declaration and constructor follow best practices.The code follows best practices by:
- Using constructor injection
- Making the field private and final
- Following the principle of least access
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupRegistrationApi.java (1)
1-14: LGTM!Package declaration and imports are well-organized, following Java naming conventions and avoiding star imports.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/TutorialGroupNotificationService.java (5)
8-8: Nice introduction of Optional APIs and custom exception.Using
Optionalto represent potentially unavailable functionality and throwingApiNotPresentExceptionwhen absent is a clean approach to modular disablement.Also applies to: 20-22
31-31: Fields align well with the new API-based design.Declaring Optional fields for
tutorialGroupNotificationApiandtutorialGroupRegistrationApiensures graceful handling of disabled modules without polluting code with null checks.Also applies to: 33-33
41-45: Injection of Optionals in the constructor is consistent.This constructor neatly enforces the presence of either a valid API or throws an exception when it’s missing. All good here.
72-73: Safe fallback with orElseThrow.Efficiently terminates the flow if the notification API is unavailable. Ensure upstream callers are prepared to handle
ApiNotPresentException.
95-95: Same pattern for the registration API.Consistently applying the same fallback strategy for the registration API provides uniform handling. No issues noted.
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (5)
86-86: Addition of ApiNotPresentException and optional tutorial group APIs.Imports are well-structured and reflect the new API-based approach for tutorial groups. This is consistent with the modular design shift.
Also applies to: 119-121
135-135: Optional fields align with the disablement mechanism.These field declarations neatly accommodate conditional availability of tutorial group functionalities, maintaining clean boundaries.
Also applies to: 199-199, 211-211
234-234: Constructor injection of Optionals is coherent and maintainable.You've properly assigned the optional API fields within the constructor. This design fosters clarity regarding module presence.
Also applies to: 236-236, 269-269, 275-275, 276-276
372-377: Handling absent TutorialGroupApi during course retrieval.Falling back to zero tutorial groups if the API is missing is a simple, user-friendly fallback. This approach cleanly avoids error cascades.
546-546: Tutorial group deletion flow respects new modular APIs.Fetching the API with
orElseThrowensures the module is enabled before proceeding, and conditionally deletes channels and notifications to maintain data consistency.Also applies to: 548-548, 551-552
src/test/java/de/tum/cit/aet/artemis/tutorialgroup/architecture/TutorialGroupApiArchitectureTest.java (2)
1-5: New architecture test introduced.Extending the
AbstractModuleAccessArchitectureTestfor the tutorial group module is a solid approach to verify module boundaries and usage.
7-10: Module package resolved for tutorial group.Overriding
getModulePackagehelps ensure the architecture tests specifically target thetutorialgrouppackage.src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
6-18: LGTM! Well-structured exception class with clear error messaging.The implementation follows best practices with proper JavaDoc documentation and a clear, informative error message format.
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java (1)
25-28: LGTM! Constructor injection is properly implemented.The constructor follows Spring best practices for dependency injection.
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupChannelManagementApi.java (1)
24-28: LGTM! Constructor injection is properly implemented.The constructor follows Spring best practices for dependency injection.
src/main/java/de/tum/cit/aet/artemis/communication/web/NotificationResource.java (2)
57-65: LGTM! Optional dependency is properly handled.The constructor injection and field declaration follow Spring best practices.
82-88: LGTM! Proper handling of optional API presence.The code correctly handles the case when the tutorial group API is not present by falling back to an empty set.
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationDTOService.java (3)
34-34: LGTM!Clean import of the new API interface.
46-46: LGTM!Field declaration correctly uses Optional wrapper for the API dependency, following the module API pattern.
50-51: LGTM!Constructor properly handles the Optional API dependency injection.
Also applies to: 55-55
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ChannelResource.java (3)
57-57: LGTM!Clean import of the new API interface.
80-80: LGTM!Field declaration correctly uses Optional wrapper for the API dependency, following the module API pattern.
86-89: LGTM!Constructor properly handles the Optional API dependency injection.
Also applies to: 98-98
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (3)
125-125: LGTM!Clean import of the new API interface.
170-170: LGTM!Field declaration correctly uses Optional wrapper for the API dependency, following the module API pattern.
193-197: LGTM!Constructor properly handles the Optional API dependency injection.
Also applies to: 214-214
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupNotificationApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupRegistrationApi.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java (2)
22-29: Consider adding@Autowiredannotation for clarity.While constructor injection works without
@Autowiredin Spring, adding it makes the dependency injection more explicit. Also, the@Lazyannotation suggests a circular dependency that might need attention.- public TutorialGroupApi(TutorialGroupRepository tutorialGroupRepository, @Lazy TutorialGroupService tutorialGroupService) { + @Autowired + public TutorialGroupApi(TutorialGroupRepository tutorialGroupRepository, @Lazy TutorialGroupService tutorialGroupService) {
31-49: Consider adding transaction boundaries.The methods that modify data (e.g.,
deleteById) should be wrapped in transactions to ensure data consistency.+ @Transactional public void deleteById(Long id) { tutorialGroupRepository.deleteById(id); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/tutorialgroup/api/TutorialGroupApi.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java (1)
1-20: LGTM! Well-structured class with proper annotations and imports.The class follows Java naming conventions, uses specific imports, and is properly annotated for Spring framework integration.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationDTOService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupCommunicationApi.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupApi.java
- src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationDTOService.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/tutorialgroup/api/TutorialGroupCommunicationApi.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupCommunicationApi.java (3)
1-14: LGTM! Well-organized imports and package structure.The imports are specific and well-organized, following Java best practices.
19-23: LGTM! Clean dependency injection implementation.Good use of constructor injection and final field, following Spring best practices.
15-17: Consider using@Componentinstead of@Controller.Since this class acts as a facade for repository access rather than handling HTTP requests directly,
@Componentmight be more appropriate than@Controller.Please verify if this class needs to handle web requests. If not, consider changing the annotation:
-@Controller +@Component✅ Verification successful
I'm going to verify if this class is handling web requests by checking for any mapping annotations within it. We'll execute a shell script to search for common Spring HTTP mapping annotations (such as @RequestMapping, @GetMapping, @PostMapping, etc.) in the file.
🏁 Script executed:
#!/bin/bash # Search for Spring web mapping annotations in the file to determine if it's used for handling HTTP requests rg -n "@(RequestMapping|GetMapping|PostMapping|PutMapping|DeleteMapping)" src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupCommunicationApi.javaLength of output: 165
I'll generate a new script to display the first 100 lines of the file. This will let us manually inspect whether any HTTP request mapping annotations (such as @RequestMapping or @GetMapping) are present in any method declarations that might have been overlooked.
🏁 Script executed:
#!/bin/bash head -n 100 src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupCommunicationApi.javaLength of output: 1254
Annotation Choice Verification
The file doesn't contain any HTTP endpoint mapping annotations. Its sole public method doesn’t handle HTTP requests, confirming that the class is acting as a facade for repository access rather than exposing web endpoints. Based on this, replacing @controller with @component (or another appropriate stereotype) is advisable.
src/main/java/de/tum/cit/aet/artemis/tutorialgroup/api/TutorialGroupCommunicationApi.java
Outdated
Show resolved
Hide resolved
Checklist
General
Server
Motivation and Context
As part of the server modularization, we "facade" the service/repository declaration and method calls to module-external classes via a module API provided by the respective module.
Description
These changes add the module API for tutorialgroup functionality.
Out of scope: Adding a
PROFILE_TUTORIALGROUP(or similar) to disable the module on server start. This requires a lot of frontend changes and testing.Steps for Testing
Can only be test on TS3
Basically try to test all the functionality of tutorialgroup.
Exam Mode Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit