Development: Autowire Spring Atlas Components based on Spring properties instead of Spring profiles
#10376
Conversation
github-actions
bot
added
tests
server
This reverts commit f46a008.
|
Warning Rate limit exceeded@ole-ve has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request transitions the Atlas configuration from a profile-based approach to a conditional mechanism. The change removes the Changes
Suggested labels
Suggested reviewers
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
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1)
1-1: 💡 Verification agent🧩 Analysis chain
Verify consistent application of configuration changes across the codebase.
Let's ensure all Atlas-related components have been updated consistently.
🏁 Script executed:
#!/bin/bash # Description: Verify that all Atlas-related components have been updated to use @Conditional instead of @Profile # Test 1: Check for any remaining @Profile annotations with PROFILE_ATLAS echo "Checking for remaining @Profile annotations with PROFILE_ATLAS..." rg -l "@Profile.*PROFILE_ATLAS" || echo "No remaining @Profile annotations found." # Test 2: Check for any remaining imports or usage of PROFILE_ATLAS constant echo -e "\nChecking for remaining PROFILE_ATLAS constant usage..." rg -l "PROFILE_ATLAS" || echo "No remaining PROFILE_ATLAS constant usage found." # Test 3: Verify that Atlas components use @Conditional(AtlasEnabled.class) echo -e "\nVerifying @Conditional usage in Atlas components..." rg -l "package.*atlas" | while read -r file; do if ! rg -q "@Conditional.*AtlasEnabled" "$file"; then echo "Warning: Atlas component missing @Conditional: $file" fi doneLength of output: 49082
Atlas Configuration Update Required
The verification confirms that while all
@Profileannotations and the usage ofPROFILE_ATLAShave been successfully removed, many Atlas-related components are missing the updated@Conditional(AtlasEnabled.class)annotation. For example:
src/main/java/de/tum/cit/aet/artemis/atlas/dto/ScienceEventDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/dto/LearningPathNavigationDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/dto/LinkedCourseCompetencyDTO.java- …and many others across the atlas packages (domain, DTOs, APIs, repositories, services, and tests).
Please update these files to consistently apply the new configuration using
@Conditional(AtlasEnabled.class).
🧹 Nitpick comments (12)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (1)
193-193: Consider enhancing the error message for better debugging.The exception change is good, but we could make the error message more helpful by including the current value of the property.
- var api = learningMetricsApi.orElseThrow(() -> new ApiConditionNotPresentException(LearningMetricsApi.class, ATLAS_ENABLED_PROPERTY_NAME)); + var api = learningMetricsApi.orElseThrow(() -> new ApiConditionNotPresentException(LearningMetricsApi.class, ATLAS_ENABLED_PROPERTY_NAME, "LearningMetricsApi is required but not available"));src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.java (1)
9-21: Add class-level documentation.Please add JavaDoc to explain the purpose of this class and its role in determining Atlas feature availability.
+/** + * Spring condition that determines if Atlas functionality is enabled based on application properties. + * This class is used in conjunction with @Conditional annotation to conditionally enable Atlas-related components. + */ public class AtlasEnabled implements Condition {src/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceSettingRepository.java (1)
12-14: Enhance repository documentation.The current documentation is minimal. Consider adding details about the repository's purpose and its relationship with the Atlas feature.
/** - * Spring Data repository for the ScienceSetting entity. + * Spring Data repository for the {@link ScienceSetting} entity. + * This repository manages science settings for Atlas users and is only available when Atlas is enabled. */src/main/java/de/tum/cit/aet/artemis/atlas/api/CourseCompetencyApi.java (1)
20-22: Consider adding a service layer for business logic.Direct repository access in the API layer might violate separation of concerns. Consider introducing a service layer to handle business logic and transaction management.
public class CourseCompetencyApi extends AbstractAtlasApi { - private final CourseCompetencyRepository courseCompetencyRepository; + private final CourseCompetencyService courseCompetencyService; - public CourseCompetencyApi(CourseCompetencyRepository courseCompetencyRepository) { - this.courseCompetencyRepository = courseCompetencyRepository; + public CourseCompetencyApi(CourseCompetencyService courseCompetencyService) { + this.courseCompetencyService = courseCompetencyService; } public void save(CourseCompetency courseCompetency) { - courseCompetencyRepository.save(courseCompetency); + courseCompetencyService.save(courseCompetency); }src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java (1)
9-11: Make class final and use constructor-based logger initialization.The class should be made final as it's not designed for inheritance. Also, prefer constructor-based initialization for the logger.
-public class ArtemisConfigHelper { - - private static final Logger log = LoggerFactory.getLogger(ArtemisConfigHelper.class); +public final class ArtemisConfigHelper { + + private final Logger log; + + public ArtemisConfigHelper() { + this.log = LoggerFactory.getLogger(ArtemisConfigHelper.class); + }src/main/java/de/tum/cit/aet/artemis/atlas/api/PrerequisitesApi.java (1)
11-19: LGTM! Clean implementation with proper dependency injection.The class follows best practices with constructor injection and proper inheritance structure.
Consider making the class final since it's not designed for inheritance:
-public class PrerequisitesApi extends AbstractAtlasApi { +public final class PrerequisitesApi extends AbstractAtlasApi {src/main/java/de/tum/cit/aet/artemis/atlas/api/ScienceEventApi.java (1)
12-20: LGTM! Clean implementation with proper dependency injection.The class follows best practices with constructor injection and proper inheritance structure.
Consider making the class final since it's not designed for inheritance:
-public class ScienceEventApi extends AbstractAtlasApi { +public final class ScienceEventApi extends AbstractAtlasApi {src/main/java/de/tum/cit/aet/artemis/core/config/ModuleFeatureInfoContributor.java (1)
31-34: Consider injecting ArtemisConfigHelper instead of direct instantiation.The direct instantiation of ArtemisConfigHelper in the constructor makes the class harder to test and violates dependency injection principles. Consider making ArtemisConfigHelper a Spring-managed bean and injecting it via constructor.
- private final ArtemisConfigHelper artemisConfigHelper; + private final ArtemisConfigHelper artemisConfigHelper; - public ModuleFeatureInfoContributor(Environment environment) { + public ModuleFeatureInfoContributor(Environment environment, ArtemisConfigHelper artemisConfigHelper) { this.environment = environment; - this.artemisConfigHelper = new ArtemisConfigHelper(); + this.artemisConfigHelper = artemisConfigHelper; }src/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceSettingsResource.java (1)
12-12: LGTM! Annotation change is correct, but error handling could be improved.The transition to conditional configuration is implemented correctly. However, consider enhancing error handling in the
saveScienceSettingsForCurrentUsermethod.Consider using a custom exception handler to provide more detailed error responses:
+@ExceptionHandler(BadRequestAlertException.class) +public ResponseEntity<ProblemDetail> handleBadRequestAlertException(BadRequestAlertException ex) { + ProblemDetail problem = ProblemDetail.forStatusAndDetail( + HttpStatus.BAD_REQUEST, + ex.getMessage() + ); + problem.setProperty("entityName", ex.getEntityName()); + problem.setProperty("errorKey", ex.getErrorKey()); + return ResponseEntity.of(problem).build(); +}Also applies to: 21-21, 34-34
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.java (1)
98-107: Consider optimizing the complex aggregation query.The query for
findAverageOfAllNonZeroStudentProgressByCompetencyIdperforms multiple joins and aggregations. Consider adding appropriate indexes to improve performance.Consider adding indexes on:
competency_progress(competency_id, progress)competency(course_id)user(student_group_name)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyMetricsRepository.java (1)
31-36: Consider query performance optimization.The repository contains complex queries with multiple joins and subqueries. Consider:
- Adding indexes for frequently queried columns
- Monitoring query performance in production
- Using query result caching for frequently accessed metrics
Consider adding indexes on:
competency(course_id, title)competency_jol(user_id, competency_id, judgement_time)Also applies to: 90-102
docs/admin/setup/production-setup-tips.rst (1)
76-76: Updated Service Start Command Line: Atlas Profile RemovedThe active profiles now omit the
atlasprofile, which aligns with the new configuration approach based on theartemis.atlas.enabledproperty. Please verify that all corresponding documentation references (in both production and related configuration files) have been updated consistently.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
.idea/runConfigurations/Artemis__Server_.xmlis excluded by!**/*.xml.idea/runConfigurations/Artemis__Server__LocalVC___Jenkins_.xmlis excluded by!**/*.xml.idea/runConfigurations/Artemis__Server__LocalVC___LocalCI_.xmlis excluded by!**/*.xml.idea/runConfigurations/Artemis__Server__LocalVC___LocalCI__Athena_.xmlis excluded by!**/*.xml.idea/runConfigurations/Artemis__Server__LocalVC___LocalCI__Theia_.xmlis excluded by!**/*.xml.idea/runConfigurations/Artemis__Server___Client_.xmlis excluded by!**/*.xmlsrc/main/resources/config/application-artemis.ymlis excluded by!**/*.yml
📒 Files selected for processing (84)
docs/admin/setup/aeolus.rst(1 hunks)docs/admin/setup/apollon.rst(1 hunks)docs/admin/setup/athena.rst(1 hunks)docs/admin/setup/distributed.rst(1 hunks)docs/admin/setup/iris.rst(1 hunks)docs/admin/setup/production-setup-tips.rst(1 hunks)docs/dev/setup/aeolus.rst(1 hunks)docs/dev/setup/integrated-code-lifecycle.rst(1 hunks)docs/dev/setup/jenkins-gitlab.rst(1 hunks)docs/dev/setup/jenkins-localvc.rst(1 hunks)docs/dev/setup/server.rst(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRelationApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/CourseCompetencyApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/LearnerProfileApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningMetricsApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningPathApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/PrerequisitesApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/ScienceEventApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyJolRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyLectureUnitLinkRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyMetricsRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRelationRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/KnowledgeAreaRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/PrerequisiteRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceEventRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceSettingRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/SourceRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/StandardizedCompetencyRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningMetricsService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/ScienceEventService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyRelationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/KnowledgeAreaService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNavigationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/CompetencyResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/MetricsResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/PrerequisiteResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceSettingsResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/StandardizedCompetencyResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/admin/AdminStandardizedCompetencyResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/ModuleFeatureInfoContributor.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/exception/ApiConditionNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/exception/ApiProfileNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportScienceEventService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(3 hunks)src/main/webapp/app/app.constants.ts(1 hunks)src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.ts(2 hunks)src/main/webapp/app/course/manage/course-update.component.ts(2 hunks)src/main/webapp/app/course/manage/overview/course-management-card.component.ts(2 hunks)src/main/webapp/app/overview/course-dashboard/course-dashboard.component.ts(2 hunks)src/main/webapp/app/overview/course-overview.component.ts(2 hunks)src/main/webapp/app/shared/layouts/navbar/navbar.component.ts(2 hunks)src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts(1 hunks)src/main/webapp/app/shared/layouts/profiles/profile.service.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationJenkinsGitlabTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java(1 hunks)src/test/javascript/spec/component/course/course-update.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/code-button.component.spec.ts(1 hunks)src/test/javascript/spec/component/shared/navbar.component.spec.ts(1 hunks)src/test/javascript/spec/service/profile.service.spec.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/dev/setup/jenkins-gitlab.rst
- src/main/java/de/tum/cit/aet/artemis/core/exception/ApiProfileNotPresentException.java
- src/test/javascript/spec/component/shared/code-button.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (4)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/layouts/profiles/profile-info.model.tssrc/main/webapp/app/app.constants.tssrc/main/webapp/app/shared/layouts/profiles/profile.service.tssrc/main/webapp/app/overview/course-overview.component.tssrc/main/webapp/app/overview/course-dashboard/course-dashboard.component.tssrc/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.tssrc/main/webapp/app/course/manage/overview/course-management-card.component.tssrc/main/webapp/app/course/manage/course-update.component.tssrc/main/webapp/app/shared/layouts/navbar/navbar.component.ts
`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/atlas/repository/CompetencyExerciseLinkRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceSettingRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.javasrc/main/java/de/tum/cit/aet/artemis/core/exception/ApiConditionNotPresentException.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/StandardizedCompetencyRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/LearningPathApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/KnowledgeAreaService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/ScienceEventApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceEventRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/MetricsResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRelationRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/PrerequisiteRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyJolRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRelationApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/LearningMetricsApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/LearningMetricsService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/SourceRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.javasrc/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportScienceEventService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyRelationService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/LearnerProfileApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/CompetencyResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyLectureUnitLinkRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/StandardizedCompetencyResource.javasrc/main/java/de/tum/cit/aet/artemis/core/config/ModuleFeatureInfoContributor.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNavigationService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceSettingsResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/admin/AdminStandardizedCompetencyResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/KnowledgeAreaRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/CourseCompetencyApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/ScienceEventService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/PrerequisitesApi.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/PrerequisiteResource.javasrc/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyMetricsRepository.javasrc/main/java/de/tum/cit/aet/artemis/core/config/Constants.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationJenkinsGitlabTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/profile.service.spec.tssrc/test/javascript/spec/component/shared/navbar.component.spec.tssrc/test/javascript/spec/component/course/course-update.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (89)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationJenkinsGitlabTest.java (1)
59-59: LGTM! The profile removal aligns with the PR objectives.The removal of the Atlas profile from
@ActiveProfilesis consistent with the transition from profile-based to property-based configuration for the Atlas feature.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (1)
3-3: LGTM! Import changes align with the new configuration strategy.The transition from profile-based to property-based configuration is well implemented through these import changes.
Also applies to: 27-27
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (2)
104-105: LGTM! Removal of PROFILE_ATLAS aligns with PR objectives.The removal of
PROFILE_ATLASfrom@ActiveProfilesis consistent with the PR's goal of transitioning from profile-based to property-based configuration for Atlas features.
179-180:✅ Verification successful
Verify Atlas-related test dependencies.
The class still contains Atlas-related mocks (e.g.,
CompetencyJolService). Let's verify if these mocks are still needed after the profile removal.
🏁 Script executed:
#!/bin/bash # Description: Check if CompetencyJolService is still used in tests after profile removal # Search for test classes that use CompetencyJolService rg -l "CompetencyJolService" --type java --glob "src/test/**/*Test.java" # Search for actual usage of the mock in test methods rg "competencyJolService\." --type java --glob "src/test/**/*Test.java"Length of output: 480
Atlas Mock Usage Confirmed
The grep results show thatCompetencyJolServiceis still invoked in tests (e.g., inPyrisEventSystemIntegrationTest), confirming that the Atlas-related test dependency is still in use. No changes are necessary regarding this mock at this time.src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.java (1)
17-20: LGTM! Clean implementation of Spring's Condition interface.The implementation correctly delegates to
ArtemisConfigHelperfor determining if Atlas is enabled.src/main/java/de/tum/cit/aet/artemis/atlas/repository/SourceRepository.java (1)
13-15: LGTM! Clean transition to conditional configuration.The repository is correctly configured with the new
@Conditional(AtlasEnabled.class)annotation, aligning with the PR's objective to move away from profile-based configuration.src/main/java/de/tum/cit/aet/artemis/atlas/repository/StandardizedCompetencyRepository.java (1)
13-15: LGTM! Clean transition to conditional configuration.The repository is correctly configured with the new
@Conditional(AtlasEnabled.class)annotation, maintaining consistency with the PR's objective.src/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceSettingRepository.java (1)
15-17: LGTM! Clean transition to conditional configuration.The repository is correctly configured with the new
@Conditional(AtlasEnabled.class)annotation, consistent with the PR's objective.src/main/java/de/tum/cit/aet/artemis/core/exception/ApiConditionNotPresentException.java (1)
9-18: LGTM! Well-structured exception class with clear error messaging.The implementation follows best practices:
- Clear and descriptive error message format
- Proper use of generics with bounded type parameter
- Good JavaDoc documentation
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyApi.java (1)
10-11: LGTM! Clean transition to conditional configuration.The change from profile to conditional configuration is well-implemented:
- Proper ordering of annotations
- Follows Spring's best practices for conditional bean registration
src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningMetricsApi.java (1)
10-11: LGTM! Consistent implementation of conditional configuration.The changes maintain consistency with other Atlas API classes and follow best practices:
- Proper annotation placement
- Clean separation of concerns
src/main/java/de/tum/cit/aet/artemis/atlas/api/CourseCompetencyApi.java (1)
10-11: LGTM! Annotation changes are consistent with the new configuration approach.The conditional configuration is properly implemented.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyExerciseLinkRepository.java (1)
14-24: LGTM! Clean transition to conditional configuration.The repository is well-structured with proper use of:
- Parameter annotation for safe SQL queries
- Text block for readable SQL
- Fetch join for performance optimization
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyLectureUnitLinkRepository.java (1)
3-3: LGTM! The changes align with the PR objectives.The transition from profile-based to conditional-based configuration is implemented correctly. The repository maintains proper Spring Data JPA practices.
Also applies to: 10-10, 14-14
src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningPathApi.java (1)
5-5: LGTM! Clean implementation following Spring best practices.The changes maintain proper dependency injection and validation while transitioning to conditional-based configuration.
Also applies to: 8-8, 14-14
src/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceEventRepository.java (1)
5-5: LGTM! Well-structured repository implementation.The changes maintain proper JPA practices while transitioning to conditional-based configuration. The SQL queries use proper parameter binding for security.
Also applies to: 12-12, 19-19
src/main/webapp/app/app.constants.ts (1)
30-30:❓ Verification inconclusive
Verify synchronization with backend constants.
Please ensure that the constant name
MODULE_FEATURE_ATLASmatches the corresponding constant in the Java backend'sConstants.javafile.
🏁 Script executed:
#!/bin/bash # Search for the corresponding constant in Constants.java rg -A 1 "MODULE_FEATURE_ATLAS" "src/main/java/**/Constants.java"Length of output: 136
Action Required: Verify Backend Constant Synchronization
The frontend constantMODULE_FEATURE_ATLASdefined insrc/main/webapp/app/app.constants.ts(line 30) is expected to match the corresponding constant on the Java backend. Our automated search forMODULE_FEATURE_ATLASinConstants.javadid not find any results, which may indicate one of the following:
- The backend constant might be located in a different file or under an alternative naming convention.
- The file path used in the search (
src/main/java/**/Constants.java) might not reflect the current repository structure.Please perform a manual verification to ensure that the backend constant exists and is synchronized with the frontend definition, or update the search paths accordingly if the location has changed.
src/main/java/de/tum/cit/aet/artemis/core/config/ModuleFeatureInfoContributor.java (1)
36-43: LGTM! Clear and concise implementation.The contribute method has a clear single responsibility and follows good practices:
- Uses ArrayList for mutable list operations
- Follows the builder pattern correctly
- Has clear and meaningful variable names
src/main/java/de/tum/cit/aet/artemis/atlas/api/LearnerProfileApi.java (1)
14-15: LGTM! Correct implementation of conditional configuration.The change from
@Profile(PROFILE_ATLAS)to@Conditional(AtlasEnabled.class)aligns with the PR objectives and maintains the same functionality while providing more flexibility in configuration.src/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceResource.java (1)
22-23:❓ Verification inconclusive
Verify the interaction between @conditional and @FeatureToggle annotations.
The combination of
@Conditional(AtlasEnabled.class)and@FeatureToggle(Feature.Science)creates two separate conditions for enabling this resource. Please verify that this is the intended behavior and that both conditions work together as expected.
🏁 Script executed:
#!/bin/bash # Search for other classes using both @Conditional(AtlasEnabled.class) and @FeatureToggle ast-grep --pattern $'@Conditional(AtlasEnabled.class) $$$ @FeatureToggle($_)'Length of output: 202
Dual-Annotation Condition Check
The production file (
src/main/java/de/tum/cit/aet/artemis/atlas/web/ScienceResource.java) correctly applies both@Conditional(AtlasEnabled.class)and@FeatureToggle(Feature.Science), which introduces two separate activation conditions for the resource. However, our initial automated search returned inconclusive results due to encoding issues in a test data file. Please manually confirm that this dual-condition setup is intentional and that the behavior in production matches the intended functionality.src/main/java/de/tum/cit/aet/artemis/atlas/web/MetricsResource.java (1)
18-20: LGTM! Clean implementation of conditional configuration.The change from
@Profile(PROFILE_ATLAS)to@Conditional(AtlasEnabled.class)is correctly implemented, maintaining the same functionality while providing more flexibility in configuration.src/main/java/de/tum/cit/aet/artemis/atlas/service/ScienceEventService.java (1)
5-5: LGTM! The configuration change aligns with the PR objective.The change from profile-based to property-based configuration is implemented correctly. The service maintains good practices such as constructor injection and proper documentation.
Also applies to: 10-10, 18-18
src/main/java/de/tum/cit/aet/artemis/atlas/repository/KnowledgeAreaRepository.java (1)
10-10: LGTM! The configuration change is properly implemented.The change from profile-based to property-based configuration is correct. The repository maintains good practices such as proper SQL query formatting and effective use of JPA features.
Also applies to: 16-16, 24-24
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRelationApi.java (1)
5-5: LGTM! The configuration change is consistent with other files.The change from profile-based to property-based configuration is implemented correctly. The class maintains good practices such as constructor injection.
Also applies to: 8-8, 16-16
src/main/java/de/tum/cit/aet/artemis/atlas/repository/PrerequisiteRepository.java (1)
7-7: LGTM! The configuration change is properly implemented.The change from profile-based to property-based configuration is correct. The repository maintains good practices such as proper JPQL query formatting and effective use of JPA features.
Also applies to: 12-12, 20-20
src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
8-8: LGTM! The new property follows TypeScript conventions.The addition of
activeModuleFeaturesaligns with the PR's objective of transitioning from profile-based to property-based feature management.src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRepository.java (2)
7-7: LGTM! Clean transition from profile-based to conditional configuration.The change from
@Profileto@Conditional(AtlasEnabled.class)aligns with the PR's objective of using property-based feature management.Also applies to: 10-10, 13-13, 21-21
25-36: Well-structured JPQL queries with proper JOIN FETCH statements.The queries follow best practices by:
- Using JOIN FETCH to avoid N+1 problems
- Properly parameterizing with @param
- Following consistent formatting
Also applies to: 38-45, 47-56
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java (2)
7-7: LGTM! Clean transition from profile-based to conditional configuration.The change from
@Profileto@Conditional(AtlasEnabled.class)aligns with the PR's objective of using property-based feature management.Also applies to: 10-10, 21-21
66-72: Well-designed asynchronous processing for course progress updates.The method follows best practices by:
- Processing courses independently
- Using async updates for each competency
- Avoiding blocking operations
src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportScienceEventService.java (2)
3-3: LGTM! Improved error handling with more specific exception.The changes enhance clarity by:
- Using the new property-based constant
- Providing better context with ApiConditionNotPresentException
Also applies to: 20-20, 45-45
57-71: Well-implemented CSV export with proper resource handling.The implementation follows best practices by:
- Using try-with-resources for proper resource cleanup
- Validating input data
- Following CSV format standards
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/KnowledgeAreaService.java (1)
5-5: LGTM! Clean transition to conditional configuration.The change from profile-based to conditional configuration is implemented correctly. The service maintains good practices with constructor injection and proper documentation.
Also applies to: 8-8, 17-17
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyJolRepository.java (1)
6-6: LGTM! Annotation change is properly implemented.The transition to conditional configuration is correct. The repository maintains good practices with properly parameterized queries and explicit column selection.
Also applies to: 11-11, 19-19
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java (1)
7-7: LGTM! Clean implementation of conditional configuration.The change to conditional configuration is implemented correctly. The repository demonstrates good practices with EntityGraph usage for performance optimization and proper error handling.
Also applies to: 15-15, 19-19
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.java (1)
8-8: LGTM! Clean transition from profile to conditional configuration.The change from
@Profile(PROFILE_ATLAS)to@Conditional(AtlasEnabled.class)aligns with the PR objectives and follows Spring best practices for feature toggling.Also applies to: 15-15, 21-21
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRelationRepository.java (2)
5-5: LGTM! Consistent configuration change.The change to
@Conditional(AtlasEnabled.class)maintains consistency with other repository configurations.Also applies to: 12-12, 20-20
81-98: Well-documented native query implementation.The recursive CTE implementation for finding matching competencies is well-documented and necessary since JPA doesn't support recursive queries. The use of native SQL is justified here.
src/main/java/de/tum/cit/aet/artemis/atlas/web/StandardizedCompetencyResource.java (2)
30-31: LGTM! Good combination of conditional and feature toggle.The use of both
@Conditional(AtlasEnabled.class)and@FeatureToggle(Feature.StandardizedCompetencies)provides fine-grained control over feature availability.
46-52: LGTM! Clean constructor injection implementation.The constructor follows Spring best practices by using constructor injection for all dependencies.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyMetricsRepository.java (1)
5-5: LGTM! Consistent configuration change.The change to
@Conditional(AtlasEnabled.class)maintains consistency with other Atlas components.Also applies to: 10-10, 21-21
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.java (1)
7-10: LGTM! The conditional configuration change looks good.The transition from profile-based to conditional configuration is well implemented. The service follows Spring best practices with constructor injection and has clear documentation.
Also applies to: 33-33
src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1)
32-32: LGTM! The module features integration looks good.The addition of
activeModuleFeaturesaligns with the transition from profile-based to module feature-based configuration.src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyJolService.java (1)
13-14: LGTM! The conditional configuration change looks good.The transition from profile-based to conditional configuration is well implemented. The service follows Spring best practices with proper error handling and logging.
Also applies to: 16-16, 32-32
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java (1)
8-9: LGTM! The conditional configuration change looks good.The transition from profile-based to conditional configuration is well implemented. The service follows Spring best practices with proper error handling.
Also applies to: 11-11, 38-38
src/main/webapp/app/course/manage/overview/course-management-card.component.ts (1)
2-2: LGTM! The changes align with the PR objectives.The transition from profile-based to module feature-based configuration is correctly implemented:
- Import statement updated to use
MODULE_FEATURE_ATLAS.- Feature check in
ngOnInitnow usesactiveModuleFeaturesinstead ofactiveProfiles.Also applies to: 122-122
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyRelationService.java (1)
10-10: LGTM! The changes align with the PR objectives.The transition from profile-based to conditional configuration is correctly implemented:
- Import statements updated to use
ConditionalandAtlasEnabled.- Service annotation updated to use
@Conditional(AtlasEnabled.class).Also applies to: 13-13, 25-25
src/main/java/de/tum/cit/aet/artemis/atlas/web/admin/AdminStandardizedCompetencyResource.java (1)
10-10: LGTM! The changes align with the PR objectives.The transition from profile-based to conditional configuration is correctly implemented:
- Import statements updated to use
ConditionalandAtlasEnabled.- Controller annotation updated to use
@Conditional(AtlasEnabled.class).Also applies to: 23-23, 39-39
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningMetricsService.java (1)
17-17: LGTM! The changes align with the PR objectives.The transition from profile-based to conditional configuration is correctly implemented:
- Import statements updated to use
ConditionalandAtlasEnabled.- Service annotation updated to use
@Conditional(AtlasEnabled.class).Also applies to: 20-20, 40-40
src/main/webapp/app/course/manage/course-management-tab-bar/course-management-tab-bar.component.ts (2)
34-34: LGTM! Import statement updated correctly.The import statement has been updated to include
MODULE_FEATURE_ATLASinstead ofPROFILE_ATLAS, aligning with the new feature management strategy.
133-133: LGTM! Feature check updated correctly.The Atlas feature check has been updated to use
activeModuleFeatures.includes(MODULE_FEATURE_ATLAS)instead of checkingactiveProfiles, aligning with the new property-based configuration approach.src/main/webapp/app/overview/course-dashboard/course-dashboard.component.ts (2)
18-18: LGTM! Import statement updated correctly.The import statement has been updated to include
MODULE_FEATURE_ATLASinstead ofPROFILE_ATLAS, aligning with the new feature management strategy.
97-97: LGTM! Feature check updated correctly.The Atlas feature check has been updated to use
activeModuleFeatures.includes(MODULE_FEATURE_ATLAS), aligning with the new property-based configuration approach.src/test/javascript/spec/service/profile.service.spec.ts (1)
132-132: LGTM! Test data updated correctly.The test objects have been updated to include the
activeModuleFeaturesarray, properly supporting the new feature management strategy.Also applies to: 157-157
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNavigationService.java (1)
7-7: LGTM! Service configuration updated correctly.The service has been updated to use
@Conditional(AtlasEnabled.class)instead of@Profile, aligning with the new property-based configuration approach. The imports have been updated accordingly.Also applies to: 10-10, 25-25
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java (1)
16-16: LGTM! The change to@Conditional(AtlasEnabled.class)improves configuration flexibility.The transition from profile-based to property-based configuration allows for more granular control over Atlas functionality. This change makes it easier for administrators to enable/disable features through configuration properties.
Also applies to: 22-22, 40-40
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java (1)
8-8: LGTM! The change to@Conditional(AtlasEnabled.class)enhances configuration management.The transition from profile-based to property-based configuration provides better control over Atlas repository instantiation. This change maintains consistency with the overall architectural improvements.
Also applies to: 15-15, 28-28
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java (1)
3-3: LGTM! The test configuration changes improve consistency with the new Atlas feature management.The transition from profile-based to property-based configuration in the test setup ensures proper testing of the new Atlas feature management. Setting
artemis.atlas.enabled=truedirectly in test properties provides clearer control over the feature's state during testing.Also applies to: 45-48
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
375-389: LGTM! The new constants provide clear configuration points for Atlas functionality.The addition of well-documented constants for Atlas feature management improves code maintainability and configuration clarity:
ACTIVE_MODULE_FEATURESfor InfoContributor integrationMODULE_FEATURE_ATLASfor consistent feature identificationATLAS_ENABLED_PROPERTY_NAMEfor property-based configurationsrc/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.java (1)
54-54: LGTM!The removal of
PROFILE_ATLASfrom@ActiveProfilesaligns with the PR objective of transitioning from profile-based to condition-based configuration for the Atlas feature.src/main/java/de/tum/cit/aet/artemis/atlas/web/CompetencyResource.java (1)
13-13: LGTM!The replacement of
@Profile(PROFILE_ATLAS)with@Conditional(AtlasEnabled.class)enhances configuration flexibility by allowing the Atlas feature to be enabled/disabled through properties rather than profiles.Also applies to: 24-24, 46-46
src/main/java/de/tum/cit/aet/artemis/atlas/web/PrerequisiteResource.java (1)
13-13: LGTM!The replacement of
@Profile(PROFILE_ATLAS)with@Conditional(AtlasEnabled.class)consistently applies the new condition-based configuration approach for the Atlas feature.Also applies to: 24-24, 50-50
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)
3-3: LGTM!The changes enhance error handling by:
- Using
ApiConditionNotPresentExceptionwhich better reflects the condition-based configuration approach.- Referencing
ATLAS_ENABLED_PROPERTY_NAMEin the error message to provide clearer context about the Atlas feature's availability.Also applies to: 39-39, 341-341
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java (1)
15-15: LGTM! The configuration change enhances flexibility.The replacement of profile-based configuration with
@Conditional(AtlasEnabled.class)maintains the same conditional behavior while providing more granular control. The change aligns well with the existing@FeatureToggleannotations and REST controller structure.Also applies to: 26-26, 59-59
src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java (1)
16-16: LGTM! Configuration change is consistent and well-integrated.The replacement of profile-based configuration maintains proper separation of concerns while working harmoniously with the existing security annotations and REST controller structure.
Also applies to: 29-29, 64-64
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1)
16-16: LGTM! Service layer configuration is properly updated.The configuration change maintains the service's conditional behavior while preserving the clean architecture and dependency injection pattern.
Also applies to: 20-20, 56-56
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1)
15-15: LGTM! Service configuration is correctly updated.The configuration change preserves the service's conditional behavior while maintaining the clean architecture and dependency injection pattern.
Also applies to: 19-19, 67-67
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java (1)
17-17: LGTM! Improved service activation with conditional check.The change from
@Profile(PROFILE_ATLAS)to@Conditional(AtlasEnabled.class)enhances configuration flexibility by allowing dynamic activation based on conditions rather than static profiles.Also applies to: 21-21, 24-24, 69-69
src/main/webapp/app/course/manage/course-update.component.ts (2)
12-12: LGTM! Updated imports to include module feature constant.The import statement now correctly includes
MODULE_FEATURE_ATLASfor feature-based configuration.
192-192: LGTM! Enhanced feature check using module features.The
atlasEnabledcheck now usesactiveModuleFeaturesinstead ofactiveProfiles, aligning with the new configuration approach.src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java (1)
17-17: LGTM! Improved service activation with conditional check.The change from
@Profile(PROFILE_ATLAS)to@Conditional(AtlasEnabled.class)enhances configuration flexibility by allowing dynamic activation based on conditions rather than static profiles.Also applies to: 23-23, 49-49
src/test/javascript/spec/component/shared/navbar.component.spec.ts (1)
88-88: LGTM! Added module features support in test profile.The addition of
activeModuleFeaturesarray toprofileInfoenables testing of module feature-based configuration.src/main/webapp/app/overview/course-overview.component.ts (2)
72-72: LGTM! Import statement updated correctly.The import statement has been updated to use
MODULE_FEATURE_ATLASinstead ofPROFILE_ATLAS, aligning with the PR's objective to transition from profile-based to property-based configuration.
259-259: LGTM! Atlas feature check updated correctly.The code now checks for the Atlas feature in
activeModuleFeaturesinstead ofactiveProfiles, which is consistent with the new configuration approach.src/main/webapp/app/shared/layouts/navbar/navbar.component.ts (2)
9-9: LGTM! Import statement updated correctly.The import statement has been updated to use
MODULE_FEATURE_ATLASinstead ofPROFILE_ATLAS, aligning with the PR's objective.
238-238: LGTM! Atlas feature check updated correctly.The code now checks for the Atlas feature in
activeModuleFeaturesinstead ofactiveProfiles, maintaining consistency with the new configuration approach.src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java (1)
24-24: LGTM! Service activation mechanism updated correctly.The service activation has been updated from profile-based (
@Profile) to property-based (@Conditional) configuration, which:
- Provides more flexibility in controlling the Atlas feature
- Aligns with the PR's objective to use Spring properties over profiles
Also applies to: 30-31, 55-55
src/test/javascript/spec/component/course/course-update.component.spec.ts (2)
45-45: LGTM! Import statement updated correctly.The import statement has been updated to use
MODULE_FEATURE_ATLASinstead ofPROFILE_ATLAS, aligning with the implementation changes.
170-170: LGTM! Test data updated correctly.The test data has been updated to use
activeModuleFeatureswithMODULE_FEATURE_ATLASinstead ofactiveProfileswithPROFILE_ATLAS, ensuring the tests reflect the new configuration approach.Also applies to: 954-954
docs/admin/setup/apollon.rst (1)
12-12: Command Activation Update: Remove Atlas ProfileThe updated Spring profile activation command now correctly excludes the
atlasprofile and lists only the necessary profiles. This aligns with the new configuration approach.docs/admin/setup/aeolus.rst (1)
12-12: Aeolus Profile Command UpdateThe command activation for Aeolus has been updated to exclude the
atlasprofile, thereby reflecting the shift in configuration management. Please verify that all related documentation parts consistently adopt this new approach.docs/admin/setup/athena.rst (1)
14-14: Athena Profile Command UpdateThe adjusted command for the Athena service now omits the
atlasprofile, which fits the new configuration strategy. Be sure that any mentions of the old profile configuration are updated accordingly.docs/admin/setup/iris.rst (1)
23-23: Iris Service Command UpdateThe profile activation command in this file has been revised to remove the
atlasprofile. This change ensures clearer configuration instructions and consistency with the overall strategy.docs/dev/setup/aeolus.rst (1)
80-80: Development Setup Command Update for AeolusThe command used to start Artemis with Aeolus in the development setup has been updated to remove the
atlasprofile. Please ensure that all deployment scripts and related documentation reflect this change for consistency across environments.docs/dev/setup/integrated-code-lifecycle.rst (1)
95-95: Integrated Code Lifecycle Setup Profiles UpdatedThe command-line snippet now lists the active profiles as
dev, localci, localvc, artemis, scheduling, buildagent, core, localwithout theatlasprofile. This correctly reflects the shift to the new conditional configuration mechanism.docs/dev/setup/server.rst (2)
219-219: Server Startup Profile UpdateThe startup command now uses
--spring.profiles.active=dev,jenkins,gitlab,artemis,scheduling, which no longer includes theatlasprofile. This update meets the PR objective and helps to reduce configuration overhead.
227-227: IntelliJ Run Configuration UpdateThe snippet for setting VM options (e.g., via
-Dspring.profiles.active=...) has been updated to excludeatlas. This ensures that both command-line and IDE-based startup configurations are consistent.docs/admin/setup/distributed.rst (1)
531-531: Distributed Setup for Core Nodes: Profile AdjustmentThe core node startup command has been updated to:
--spring.profiles.active=prod,core,ldap-only,localvc,localci,athena,scheduling,iris,ltiremoving the
atlasprofile. This change is consistent with the new configuration rule and should simplify instance management in a distributed setup.docs/dev/setup/jenkins-localvc.rst (1)
89-89: Jenkins and LocalVC Setup: Spring Profiles RefinedThe active profiles for starting Artemis in a Jenkins/LocalVC environment have been refined to:
--spring.profiles.active=dev,jenkins,localvc,artemis,schedulingwith the
atlasprofile removed. This update reflects the broader changes in how Atlas configuration is controlled and is in line with the overall PR objectives.
src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (1)
60-63: Consider adding a comment for future overrides.
This method returns an empty set by default. A brief comment or Javadoc explaining that subclasses may override to ignore specific classes would improve clarity.src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasNotPresentException.java (1)
12-20: Exception class is concise and aligned with best practices.
Creating a specific subclass (AtlasNotPresentException) for missing Atlas conditions adds clarity and improves error handling semantics. Consider adding aserialVersionUIDif long-term serialization is a concern.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/config/application-artemis.ymlis excluded by!**/*.yml
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportScienceEventService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/architecture/AtlasApiArchitectureTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java(1 hunks)src/test/javascript/spec/component/shared/navbar.component.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/javascript/spec/component/shared/navbar.component.spec.ts
- src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java
- src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportScienceEventService.java
- src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/atlas/architecture/AtlasApiArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
`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/atlas/config/AtlasNotPresentException.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: Analyse
🔇 Additional comments (7)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (3)
6-7: No issues with the new imports.
They are appropriate for handling sets and ArchUnit dependencies.Also applies to: 11-11
24-24: Descriptive test name.
Renaming the test method toshouldOnlyAccessApiDomainDtoAndAllowedException()is clearer and aligns well with the coverage goals for verifying allowed dependencies.
47-47: ArchUnit rule usage looks correct.
This rule ensures that any classes outside the specified module package must only have allowed dependencies. Good application of package-restriction best practices.src/test/java/de/tum/cit/aet/artemis/atlas/architecture/AtlasApiArchitectureTest.java (2)
3-5: Imports are properly introduced.
They bring in theSettype and the new exception class.
15-18: Local override for newly introduced exception.
OverridinggetIgnoredClasses()to includeAtlasNotPresentExceptionprovides the intended exception-handling mechanism for ArchUnit checks.src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasNotPresentException.java (2)
1-4: Package and static import structure look fine.
The usage ofATLAS_ENABLED_PROPERTY_NAMEclarifies the reason for this exception.
8-11: Well-written Javadoc.
The description properly indicates that this exception arises from missing properties for an optionally autowired API.
.../de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java (1)
276-289: LGTM! Consider a minor readability improvement.The helper method correctly implements the expanded functionality to check for both
@Profileand@Conditionalannotations.Consider this minor readability improvement:
- boolean hasProfileAnnotation = item.isAnnotatedWith(Profile.class); - boolean hasConditionalAnnotation = item.isAnnotatedWith(Conditional.class); - if (!(hasProfileAnnotation || hasConditionalAnnotation)) { - String message = String.format("Class %s is neither annotated with @Profile or @Conditional", item.getFullName()); + var hasProfileAnnotation = item.isAnnotatedWith(Profile.class); + var hasConditionalAnnotation = item.isAnnotatedWith(Conditional.class); + if (!(hasProfileAnnotation || hasConditionalAnnotation)) { + var message = String.format("Class %s is neither annotated with @Profile nor @Conditional", item.getFullName());src/test/java/de/tum/cit/aet/artemis/core/authorization/AuthorizationTestService.java (2)
108-115: Consider checking method-level conditional annotations.The implementation checks for class-level
@Conditionalannotations but misses method-level ones. This could lead to incomplete validation when methods are conditionally enabled/disabled independently of their class.Apply this diff to include method-level conditional checks:
private boolean isConditionalEndpoint(HandlerMethod handlerMethod) { var methodProfileAnnotation = handlerMethod.getMethod().getAnnotation(Profile.class); + var methodConditionalAnnotation = handlerMethod.getMethod().getAnnotation(Conditional.class); var classConditionalAnnotation = handlerMethod.getMethod().getDeclaringClass().getAnnotation(Conditional.class); var classProfileAnnotation = handlerMethod.getMethod().getDeclaringClass().getAnnotation(Profile.class); // No null-check required for classes because we have tests ensuring Profile annotations on classes - return (methodProfileAnnotation != null && isNonCoreProfile(methodProfileAnnotation)) || (classConditionalAnnotation != null && isConditional(classConditionalAnnotation)) + return (methodProfileAnnotation != null && isNonCoreProfile(methodProfileAnnotation)) + || (methodConditionalAnnotation != null && isConditional(methodConditionalAnnotation)) + || (classConditionalAnnotation != null && isConditional(classConditionalAnnotation)) || isNonCoreProfile(classProfileAnnotation); }
121-123: Add JavaDoc to document the method's purpose.The method would benefit from documentation explaining its purpose and parameters.
Add JavaDoc as follows:
+ /** + * Checks if a conditional annotation has any conditions specified. + * + * @param conditionalAnnotation The conditional annotation to check + * @return true if the annotation has conditions specified, false otherwise + */ private boolean isConditional(Conditional conditionalAnnotation) { return conditionalAnnotation.value().length != 0; }src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (2)
28-54: Consider simplifying the dependency validation logic.The current implementation could be made more maintainable by:
- Extracting the package validation logic into a separate method
- Using early returns to reduce nesting
Here's a suggested refactor:
ArchCondition<JavaClass> onlyAllowedDependencies = new ArchCondition<>("have only allowed dependencies") { @Override public void check(JavaClass origin, ConditionEvents events) { List<Dependency> targetsInModule = origin.getDirectDependenciesFromSelf().stream() .filter(dependency -> resideInAPackage(getModuleWithSubpackage()).test(dependency.getTargetClass())).toList(); for (Dependency dependency : targetsInModule) { JavaClass target = dependency.getTargetClass(); - if (resideOutsideOfPackage(getModuleWithSubpackage()).test(origin)) { // ToDo: Remove? - // target inside default-allowed packages (API, Domain, DTO) - boolean inDefaultAllowedPackage = resideInAnyPackage(getModuleApiSubpackage(), getModuleDomainSubpackage(), getModuleDtoSubpackage()).test(target); - if (inDefaultAllowedPackage) { - continue; - } - - // target explicitly ignored - boolean isIgnored = getIgnoredClasses().contains(target.reflect()); - if (!isIgnored) { - String message = String.format("%s depends on %s which is not in an allowed package or explicitly ignored", origin.getName(), target.getName()); - events.add(SimpleConditionEvent.violated(origin, message)); - } - } + if (!resideOutsideOfPackage(getModuleWithSubpackage()).test(origin)) { + continue; + } + + if (isAllowedDependency(target)) { + continue; + } + + String message = String.format("%s depends on %s which is not in an allowed package or explicitly ignored", + origin.getName(), target.getName()); + events.add(SimpleConditionEvent.violated(origin, message)); } } + + private boolean isAllowedDependency(JavaClass target) { + return resideInAnyPackage( + getModuleApiSubpackage(), + getModuleDomainSubpackage(), + getModuleDtoSubpackage() + ).test(target) || getIgnoredClasses().contains(target.reflect()); + } };
69-71: Add JavaDoc to document the purpose and expected usage.Since this is a protected method intended for extension, it should be documented to help implementers understand:
- Its purpose
- When to override it
- Expected return values
Add documentation:
+ /** + * Returns a set of classes that should be ignored during dependency validation. + * Override this method in subclasses to specify classes that are allowed to be + * accessed from outside the module, even if they don't reside in API, Domain, + * or DTO packages. + * + * @return A set of classes to be ignored during dependency validation + */ protected Set<Class<?>> getIgnoredClasses() { return Set.of(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/authorization/AuthorizationTestService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/core/authorization/AuthorizationTestService.javasrc/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.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: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java (1)
242-246: LGTM! The method name and implementation accurately reflect the expanded functionality.The changes correctly enforce that Spring components must be annotated with either
@Profileor@Conditional, aligning with the PR's objective to support both profile-based and conditional-based configuration.src/test/java/de/tum/cit/aet/artemis/core/authorization/AuthorizationTestService.java (2)
17-17: LGTM!The import is correctly placed and necessary for the new conditional check functionality.
103-107: LGTM!The documentation accurately reflects the enhanced functionality to check both profile and condition dependencies.
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (1)
38-38: Address or remove the TODO comment.The comment
// ToDo: Remove?suggests uncertainty about the condition. Please either:
- Remove the condition if it's unnecessary
- Document why it's needed if it should stay
- Create a tracking issue if more investigation is needed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (3)
27-27: Update method name to match implementation.The method name
shouldOnlyAccessApiDomainDtoshould be updated toshouldOnlyAccessApiDomainDtoAndAllowedExceptionto accurately reflect its functionality with the new ignored classes feature.- void shouldOnlyAccessApiDomainDto() { + void shouldOnlyAccessApiDomainDtoAndAllowedException() {
28-54: Consider breaking down the complex ArchCondition logic.The custom ArchCondition implementation is complex and could benefit from being broken down into smaller, more focused methods for better readability and maintainability.
Consider extracting the logic into helper methods:
ArchCondition<JavaClass> onlyAllowedDependencies = new ArchCondition<>("have only allowed dependencies") { @Override public void check(JavaClass origin, ConditionEvents events) { - List<Dependency> targetsInModule = origin.getDirectDependenciesFromSelf().stream() - .filter(dependency -> resideInAPackage(getModuleWithSubpackage()).test(dependency.getTargetClass())).toList(); + List<Dependency> targetsInModule = getTargetDependenciesInModule(origin); for (Dependency dependency : targetsInModule) { JavaClass target = dependency.getTargetClass(); if (resideOutsideOfPackage(getModuleWithSubpackage()).test(origin)) { - boolean inDefaultAllowedPackage = resideInAnyPackage(getModuleApiSubpackage(), getModuleDomainSubpackage(), getModuleDtoSubpackage()).test(target); - if (inDefaultAllowedPackage) { + if (isInDefaultAllowedPackage(target)) { continue; } - boolean isIgnored = getIgnoredClasses().contains(target.reflect()); - if (!isIgnored) { - String message = String.format("%s depends on %s which is not in an allowed package or explicitly ignored", origin.getName(), target.getName()); - events.add(SimpleConditionEvent.violated(origin, message)); + if (!isIgnoredClass(target)) { + addViolationEvent(origin, target, events); } } } } + private List<Dependency> getTargetDependenciesInModule(JavaClass origin) { + return origin.getDirectDependenciesFromSelf().stream() + .filter(dependency -> resideInAPackage(getModuleWithSubpackage()).test(dependency.getTargetClass())) + .toList(); + } + + private boolean isInDefaultAllowedPackage(JavaClass target) { + return resideInAnyPackage(getModuleApiSubpackage(), getModuleDomainSubpackage(), getModuleDtoSubpackage()) + .test(target); + } + + private boolean isIgnoredClass(JavaClass target) { + return getIgnoredClasses().contains(target.reflect()); + } + + private void addViolationEvent(JavaClass origin, JavaClass target, ConditionEvents events) { + String message = String.format("%s depends on %s which is not in an allowed package or explicitly ignored", + origin.getName(), target.getName()); + events.add(SimpleConditionEvent.violated(origin, message)); + } };
69-71: Add JavaDoc for the new method.Add documentation to explain the purpose of this method and how subclasses should use it.
+ /** + * Returns a set of classes that should be ignored during dependency validation. + * Subclasses can override this method to specify classes that are allowed to be + * accessed from outside the module's API, Domain, or DTO packages. + * + * @return a set of classes to be ignored during dependency validation + */ protected Set<Class<?>> getIgnoredClasses() { return Set.of(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyse
🔇 Additional comments (1)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (1)
38-38: Remove or clarify the TODO comment.The TODO comment suggests uncertainty about the condition. Either remove it if unnecessary or clarify the reasoning behind potentially removing this check.
ole-ve
changed the title
Development: Replace determining availability of atlas based on Spring properties over Spring profilesDevelopment: Autowire Spring Atlas Components based on Spring properties instead of Spring profiles
src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java
Show resolved
Hide resolved
.../de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
Outdated
Show resolved
Hide resolved
… by chained expression using the condition
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (2)
26-26: Update method name to match implementation.The method name
shouldOnlyAccessApiDomainDtodoesn't reflect that it also checks for ignored classes.- void shouldOnlyAccessApiDomainDto() { + void shouldOnlyAccessApiDomainDtoOrIgnoredClasses() {
66-68: Add JavaDoc for protected method.Since this is a protected method intended for subclass overrides, it should have JavaDoc explaining its purpose and contract.
+ /** + * Returns a set of classes that are allowed to be accessed from outside the API, Domain, and DTO packages. + * Subclasses can override this method to specify exceptions to the default architectural rules. + * + * @return Set of classes that are exempt from the package access restrictions + */ protected Set<Class<?>> getIgnoredClasses() { return Set.of(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/atlas/config/AtlasEnabled.java
- src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
🔇 Additional comments (1)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (1)
27-54: Well-structured implementation of architectural rule enforcement!The new implementation:
- Properly filters and checks dependencies
- Provides clear violation messages
- Follows ArchUnit best practices
|
Note: For some reason, some pipelines are not being run on the correct commit causing them to fail. From the logs, one can see that (for instance) the pipeline is ran for I was wondering because the pipelines are complaining about classes that are not part of this branch anymore. Maybe caused by cherry-picking a commit from another branch, but that is very strange IMO. However, some pipelines (like Bamboo Build) are picking up the correct commit and just working fine. |
Checklist
General
Server
Client
Motivation and Context
In the future, we want to have a default configuration that mainly includes all configurations by default (opt-out), and admins setting up artemis should be able to opt-out of specific features. Currently, this is done by defining the active profiles.
Description
AtlasEnabledinheriting Spring'sConditionartemis.atlas.enabled=true)AtlasEnabledusing@Conditional(AtlasEnabled.class)artemis.atlas.enabledis either true/false, but not undefined.application.ymlnext to the packaged jar which contains "artemis.atlas.enabled=false"Steps for Testing
Note: Some steps can only be tested locallyapplication-artemis.ymlExam Mode Testing
does not apply, atlas is not used for exams
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Exam Mode Test
does not apply, no relevant changes
Performance Tests
does not apply, no relevant changes
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
Refactor
Frontend
activeModuleFeaturesto profile info model.Tests