Adaptive learning: Add implementation for grade or bonus goal in learner profile
#10159
Conversation
…/adaptive-learning/learner-profile-graded-exercises
…/adaptive-learning/learner-profile-graded-exercises # Conflicts: # src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java
…/adaptive-learning/learner-profile-graded-exercises
…nto feature/adaptive-learning/learner-profile-graded-exercises
…/adaptive-learning/learner-profile-graded-exercises
…/adaptive-learning/learner-profile-graded-exercises
…/adaptive-learning/learner-profile-graded-exercises
…/adaptive-learning/learner-profile-graded-exercises
github-actions
bot
added
tests
server
|
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/atlas/service/learningpath/LearningPathService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThis pull request introduces several enhancements to the Artemis Atlas learning path and learner profile system. Key changes include the addition of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (17)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java (2)
605-619: UsePreferenceScaleenum for type safety and readability.In the
getExerciseOrderComparatormethod,aimForGradeOrBonusis an integer value derived fromCourseLearnerProfile. Using thePreferenceScaleenum directly enhances code clarity and prevents errors due to invalid values.Apply this diff to improve the code:
-private static Comparator<CompetencyExerciseLink> getExerciseOrderComparator(int aimForGradeOrBonus) { +private static Comparator<CompetencyExerciseLink> getExerciseOrderComparator(PreferenceScale aimForGradeOrBonus) { Comparator<CompetencyExerciseLink> exerciseComparator = Comparator.comparingDouble(exerciseLink -> (COMPETENCY_LINK_WEIGHT_TO_GRADE_AIM_RATIO * exerciseLink.getWeight()) - + aimForGradeOrBonus * getIncludeInOverallScoreWeight(exerciseLink.getExercise().getIncludedInOverallScore())); + + aimForGradeOrBonus.getValue() * getIncludeInOverallScoreWeight(exerciseLink.getExercise().getIncludedInOverallScore())); exerciseComparator = exerciseComparator.reversed(); exerciseComparator = exerciseComparator.thenComparing(exerciseLink -> exerciseLink.getExercise().getTitle()); return exerciseComparator; }Ensure that all calls to this method pass the
PreferenceScaleenum instead of an integer value.
628-639: Refactor to usePreferenceScalein predicate logic.In
getExerciseSelectionPredicate, using thePreferenceScaleenum directly improves readability and reduces the chance of errors from magic numbers.Apply this diff to enhance the code:
-private static Predicate<Exercise> getExerciseSelectionPredicate(int aimForGradeOrBonus, AtomicDouble remainingExercisePoints) { +private static Predicate<Exercise> getExerciseSelectionPredicate(PreferenceScale aimForGradeOrBonus, AtomicDouble remainingExercisePoints) { Predicate<Exercise> exercisePredicate = exercise -> remainingExercisePoints.getAndAdd(-exercise.getMaxPoints()) >= 0; - if (aimForGradeOrBonus == HIGH.getValue()) { + if (aimForGradeOrBonus == PreferenceScale.HIGH) { exercisePredicate = exercisePredicate .or(exercise -> exercise.getIncludedInOverallScore() == INCLUDED_COMPLETELY || exercise.getIncludedInOverallScore() == INCLUDED_AS_BONUS); } - else if (aimForGradeOrBonus == MEDIUM_HIGH.getValue()) { + else if (aimForGradeOrBonus == PreferenceScale.MEDIUM_HIGH) { exercisePredicate = exercisePredicate.or(exercise -> exercise.getIncludedInOverallScore() == INCLUDED_COMPLETELY); } return exercisePredicate; }src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearnerProfileRepository.java (1)
26-26: Remove unnecessary comment.The comment
// ok because of deleteis redundant and doesn't add value. The@Transactionalannotation is standard for delete operations.- @Transactional // ok because of delete + @Transactionalsrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java (1)
21-21: Remove unnecessary comment.The comment
// ok because of deleteis redundant and doesn't add value. The@Transactionalannotation is standard for delete operations.- @Transactional // ok because of delete + @Transactionalsrc/main/java/de/tum/cit/aet/artemis/atlas/service/profile/LearnerProfileService.java (1)
26-36: Enhance JavaDoc with more details.The JavaDoc could be more descriptive about the method's behavior, potential exceptions, and the bidirectional relationship setup.
/** * Create a learner profile for a user and saves it in the database * * @param user the user for which the profile is created + * @throws IllegalStateException if the user already has a profile + * @implNote This method sets up a bidirectional relationship between + * the user and the learner profile before saving */src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (1)
21-22: Consider bidirectional relationship management.The one-to-one relationship with User should include helper methods to manage both sides of the relationship.
Add this method to maintain the bidirectional relationship:
+ public void setUser(User user) { + if (user == null) { + if (this.user != null) { + this.user.setLearnerProfile(null); + } + } else { + user.setLearnerProfile(this); + } + this.user = user; + }src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java (1)
28-41: Document the meaning of preference scales.The numerical fields use a 0-5 scale, but the meaning of these values is not documented.
Add JavaDoc to explain the scale:
+ /** + * Indicates the learner's goal for grade or bonus achievement. + * Scale: 0 (not important) to 5 (highest priority) + */ @Column(name = "aim_for_grade_or_bonus") @Min(0) @Max(5) private int aimForGradeOrBonus;src/test/java/de/tum/cit/aet/artemis/core/test_repository/UserTestRepository.java (1)
62-63: Consider pagination for large result sets.The EntityGraph loads multiple collections which could result in large amounts of data being fetched. Consider adding a paginated version of this method.
Add a paginated version:
+ @EntityGraph(type = LOAD, attributePaths = { "learningPaths", "learnerProfile", "learnerProfile.courseLearnerProfiles" }) + Page<User> findAllWithLearningPathsAndLearnerProfile(Pageable pageable);src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java (1)
52-62: Optimize batch creation for better performance.The current implementation of
createCourseLearnerProfilescreates a newCourseLearnerProfileinstance for each user. For large sets of users, this could be optimized.Consider using batch processing:
public void createCourseLearnerProfiles(Course course, Set<User> users) { + // Process in batches of 100 to optimize memory usage and database operations + final int batchSize = 100; + List<CourseLearnerProfile> allProfiles = new ArrayList<>(); Set<CourseLearnerProfile> courseProfiles = users.stream().map(user -> { var courseProfile = new CourseLearnerProfile(); courseProfile.setCourse(course); courseProfile.setLearnerProfile(user.getLearnerProfile()); return courseProfile; - }).collect(Collectors.toSet()); + }).collect(Collectors.toList()); - courseLearnerProfileRepository.saveAll(courseProfiles); + for (int i = 0; i < courseProfiles.size(); i += batchSize) { + int end = Math.min(i + batchSize, courseProfiles.size()); + courseLearnerProfileRepository.saveAll(courseProfiles.subList(i, end)); + } }src/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.java (1)
68-70: Enhance test setup with profile verification.The setup creates learner profiles but doesn't verify their creation. Also, consider adding cleanup to prevent test data pollution.
Add verification and cleanup:
@BeforeEach void setup() { userUtilService.addUsers(TEST_PREFIX, 5, 1, 1, 1); learnerProfileUtilService.createLearnerProfilesForUsers(TEST_PREFIX); + // Verify profiles were created + assertThat(learnerProfileUtilService.getAllLearnerProfiles()) + .hasSize(8); // 5 users + 1 tutor + 1 editor + 1 instructor } +@AfterEach +void cleanup() { + learnerProfileUtilService.deleteAllLearnerProfiles(TEST_PREFIX); +}src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
548-580: Enhance navigation preferences test coverage.The test verifies completion states but doesn't validate how the preferences affect navigation behavior.
Consider adding assertions to verify that the preferences influence the navigation:
assertThat(result.predecessorLearningObject().completed()).isTrue(); assertThat(result.currentLearningObject().completed()).isFalse(); assertThat(result.successorLearningObject().completed()).isFalse(); +// Verify that preferences influence navigation +assertThat(result.currentLearningObject().type()) + .as("High grade preference should prioritize exercises") + .isEqualTo(aimForGradeOrBonus > 3 ? LearningPathNavigationObjectDTO.LearningObjectType.EXERCISE : LearningPathNavigationObjectDTO.LearningObjectType.LECTURE);src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (1)
414-415: Enhance learner profile verification.The learner profile verification only checks existence but not the profile's contents or structure.
Consider adding more detailed assertions:
-assertThat(learnerProfileRepository.findByUser(student)).isNotEmpty(); +var learnerProfile = learnerProfileRepository.findByUser(student); +assertThat(learnerProfile) + .isPresent() + .hasValueSatisfying(profile -> { + assertThat(profile.getUser()).isEqualTo(student); + assertThat(profile.getCourseLearnerProfiles()).isEmpty(); + });src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (3)
1058-1064: Avoid unnecessary database calls inaddUserToGroupmethodIn the
addUserToGroupmethod, you're fetching theCourseentity again from the database usingfindWithEagerCompetenciesAndPrerequisitesByIdElseThrow(course.getId()), even though you already have thecourseobject. This introduces an unnecessary database call and could impact performance.Consider using the existing
courseobject if it already contains the necessary data. If additional data is needed, adjust the initial retrieval of thecourseto include the required relationships.public void addUserToGroup(User user, String group, Course course) { userService.addUserToGroup(user, group); if (group.equals(course.getStudentGroupName()) && course.getLearningPathsEnabled()) { - Course courseWithCompetencies = courseRepository.findWithEagerCompetenciesAndPrerequisitesByIdElseThrow(course.getId()); - learnerProfileApi.createCourseLearnerProfile(course, user); - learningPathApi.generateLearningPathForUser(courseWithCompetencies, user); + learnerProfileApi.createCourseLearnerProfile(course, user); + learningPathApi.generateLearningPathForUser(course, user); } }
Line range hint
660-664: Optimize batch processing when registering multiple usersIn the
registerUsersForCourseGroupmethod, creating learner profiles and generating learning paths for each user individually might lead to performance issues when handling large numbers of users.Consider optimizing the process by batching database operations or processing users asynchronously to improve performance.
58-58: Remove unused imports and organize import statementsThere are import statements that might not be used in the class, and organizing imports improves code readability and maintenance.
Please review the imports and remove any that are unused. Ensure that the imports adhere to the project's conventions and coding guidelines.
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/PreferenceScale.java (2)
4-5: Correct typo in Javadoc: 'lickert-scale' should be 'Likert scale'In the Javadoc comment, 'lickert-scale' is misspelled.
Please correct the typo to improve documentation clarity.
/** - * Enum for the preferences as lickert-scale regarding settings in the (course) learner profile, see {@link CourseLearnerProfile} and {@link LearnerProfile}. + * Enum for the preferences as Likert scale regarding settings in the (course) learner profile, see {@link CourseLearnerProfile} and {@link LearnerProfile}. */
6-19: Provide a method to retrieve enum by valueCurrently, there's no method to get a
PreferenceScalebased on an integer value. Providing such a method can enhance usability and prevent duplicate code.Consider adding a static method to retrieve the enum constant by its associated value.
public static PreferenceScale fromValue(int value) { for (PreferenceScale scale : PreferenceScale.values()) { if (scale.getValue() == value) { return scale; } } throw new IllegalArgumentException("Invalid PreferenceScale value: " + value); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241107130000_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (27)
src/main/java/de/tum/cit/aet/artemis/atlas/api/LearnerProfileApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/PreferenceScale.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearnerProfileRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/LearnerProfileService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/domain/User.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java(9 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/UserScheduleService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/user/UserCreationService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(10 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java(9 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/core/test_repository/UserTestRepository.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationJenkinsGitlabTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (27)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationJenkinsGitlabTest.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/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.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/atlas/domain/profile/PreferenceScale.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.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/core/domain/User.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/atlas/repository/CourseLearnerProfileRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.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/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.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/atlas/repository/LearnerProfileRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/core/test_repository/UserTestRepository.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/atlas/service/profile/LearnerProfileService.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/UserScheduleService.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/atlas/service/profile/CourseLearnerProfileService.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/atlas/domain/profile/CourseLearnerProfile.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/atlas/web/LearningPathResource.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/user/UserCreationService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.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/atlas/service/learningpath/LearningPathService.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/user/UserService.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/core/repository/UserRepository.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/atlas/repository/LearningPathRepository.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/atlas/service/learningpath/LearningPathRecommendationService.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/atlas/domain/profile/LearnerProfile.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/atlas/api/LearnerProfileApi.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.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
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8030
File: src/main/java/de/tum/in/www1/artemis/service/CourseScoreCalculationService.java:21-49
Timestamp: 2024-11-12T12:52:03.805Z
Learning: The project consistently avoids using star-imports in Java files.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (35)
src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (1)
12-12: Well-organized additions for learner profile testing!The new fields and imports for learner profile functionality are correctly placed within their respective sections, maintaining the class's clear organization pattern. The additions follow the established conventions and will effectively support integration testing of the learner profile features.
Also applies to: 17-17, 92-93, 153-154
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationJenkinsGitlabTest.java (1)
11-11: LGTM! Well-structured integration of learner profile testing capabilities.The addition of
LearnerProfileUtilServicefollows the established patterns and enables comprehensive testing of learner profile features across programming exercises.Also applies to: 207-208
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (5)
181-182: LGTM! Clean dependency injection implementation.The addition of
LearnerProfileApifollows Spring's constructor injection pattern and immutability best practices.Also applies to: 201-201, 225-225
Line range hint
1243-1254: LGTM! Good simplification of method signature.The removal of the redundant
Roleparameter improves the method's design while maintaining all necessary functionality through thegroupparameter.
1184-1184: LGTM! Consistent with method signature update.The method call has been properly updated to match the simplified
addUserToCourseGroupsignature.
1199-1199: LGTM! Consistent with method signature update.The method call has been properly updated to match the simplified
addUserToCourseGroupsignature.
1215-1215: LGTM! Consistent with method signature update.The method calls have been properly updated to match the simplified
addUserToCourseGroupsignature.Also applies to: 1230-1230
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java (3)
39-44: LGTM! The eager loading implementation follows established patterns.The new method and its default implementation maintain consistency with other similar methods in the repository, appropriately using
@EntityGraphfor eager loading of the course relationship.
89-91: LGTM! The default method implementation is consistent.The method name and implementation maintain consistency with the repository's established patterns for handling missing entities.
73-87: Consider query optimization opportunities.While the query structure is clear, there are potential performance optimizations to consider:
- The condition
clp.course.id = l.course.idin the WHERE clause could create a cartesian product if multiple course learner profiles exist. Consider moving this to the JOIN clause.- Consider using INNER JOIN for mandatory relationships to improve query performance.
Here's the optimized query:
@Query(""" SELECT l FROM LearningPath l LEFT JOIN FETCH l.competencies c LEFT JOIN FETCH c.lectureUnitLinks lul LEFT JOIN FETCH lul.lectureUnit LEFT JOIN FETCH c.exerciseLinks el LEFT JOIN FETCH el.exercise - LEFT JOIN FETCH l.user u - LEFT JOIN FETCH u.learnerProfile lp - LEFT JOIN FETCH lp.courseLearnerProfiles clp + INNER JOIN FETCH l.user u + INNER JOIN FETCH u.learnerProfile lp + INNER JOIN FETCH lp.courseLearnerProfiles clp + ON clp.course.id = l.course.id - WHERE l.id = :learningPathId - AND clp.course.id = l.course.id + WHERE l.id = :learningPathId """)Let's verify the relationships to ensure INNER JOIN is safe:
src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.java (1)
705-705: LGTM! Consider applying the same setup to similar tests.The addition of learner profile creation before testing user addition aligns with the PR objectives and maintains test isolation.
Let's check if other similar test methods in this file should also create learner profiles:
✅ Verification successful
Verified: Learner profile creation is correctly placed
The addition of learner profile creation is appropriate for this test and no changes are needed in other tests because:
- The test inherits learnerProfileUtilService from the base class
- Similar tests already have access to this functionality through inheritance
- The setup follows the established pattern in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find similar test methods that might need learner profile setup # Look for test methods that deal with user management in courses # Search for test methods related to adding/removing users rg -A 5 'test.*((Add|Remove)(Student|Tutor|Instructor|Editor).*Course|EnrollInCourse)' src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.java # Search for test methods that use userUtilService rg -A 5 'userUtilService\.' src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.javaLength of output: 5828
Script:
#!/bin/bash # Check if courseTestService handles learner profiles rg -A 5 'createLearnerProfiles|LearnerProfile' 'src/test/**/CourseTestService.java' # Check for other test files that might need similar setup fd -e java -x rg -l 'class.*Test.*extends.*IntegrationTest' {} \; | xargs rg -l '(Add|Remove)(Student|Tutor|Instructor).*Course'Length of output: 294
Script:
#!/bin/bash # Find CourseTestService and check its content fd CourseTestService.java | xargs rg -A 5 'createLearnerProfiles|LearnerProfile' # Find similar test services and integration tests fd -e java | rg -l 'TestService|IntegrationTest' | xargs rg -l '(Student|Tutor|Instructor).*Course' # Check for learner profile setup in other integration tests fd -e java | rg -l 'IntegrationTest' | xargs rg -A 5 'learnerProfile|LearnerProfile'Length of output: 365
Script:
#!/bin/bash # Simple search for learner profile usage in test files rg -A 5 'learnerProfile|LearnerProfile' 'src/test' # Find similar integration test files rg -l 'class.*IntegrationTest' 'src/test' # Look for user management test patterns rg -A 5 '@Test.*((Add|Remove)(Student|Tutor|Instructor)|EnrollInCourse)' 'src/test'Length of output: 34835
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java (2)
597-603: Clarify the weighting logic ingetIncludeInOverallScoreWeight.The method
getIncludeInOverallScoreWeightassigns integer weights toIncludedInOverallScoreenum values. It's essential to ensure that these weights accurately reflect the intended prioritization of exercises. Consider adding comments or documentation to explain the rationale behind these specific weight assignments for better maintainability.
613-615: Verify the calculation logic in the exercise comparator.The formula used in the comparator combines multiple factors to prioritize exercises. Please review and ensure that the calculation accurately reflects the intended prioritization based on the learner's aim and exercise attributes. Adjust the weights or logic if necessary.
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/LearnerProfileService.java (1)
31-36: 🛠️ Refactor suggestionAdd transaction boundary and enhance error handling.
The
createProfilemethod should be transactional to ensure atomicity of the operation. Also, consider adding validation to check if the user already has a profile.+ @Transactional public void createProfile(User user) { + if (user.getLearnerProfile() != null) { + throw new IllegalStateException("User already has a learner profile"); + } var profile = new LearnerProfile(); profile.setUser(user); user.setLearnerProfile(profile); userRepository.save(user); }Likely invalid or redundant comment.
src/test/java/de/tum/cit/aet/artemis/core/test_repository/UserTestRepository.java (1)
79-84: Consider performance implications of LIKE prefix query.The
findAllByUserPrefixquery uses a LIKE operation which might perform poorly on large datasets without proper indexing.Run this script to check if an index exists:
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java (1)
17-82: 🛠️ Refactor suggestionAdd transaction management for database operations.
The service methods perform database operations but lack transaction management. This could lead to data inconsistency if any operation fails.
Add
@Transactionalannotations:+import org.springframework.transaction.annotation.Transactional; @Profile(PROFILE_CORE) @Service +@Transactional public class CourseLearnerProfileService {Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/core/domain/User.java (2)
216-219: LGTM! Well-structured JPA relationship.The one-to-one relationship with LearnerProfile is properly configured with:
- Lazy loading for performance optimization
- Appropriate cascade and orphan removal settings
- Proper JSON serialization handling
554-560: LGTM! Standard accessor methods.The getter and setter methods follow Java bean conventions.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java (4)
235-235: LGTM! Consistent service method update.Updated to fetch learner profile data along with competencies and learning objects.
251-251: LGTM! Consistent service method update.Updated to fetch learner profile data along with competencies and learning objects.
344-344: LGTM! Consistent service method update.Updated to fetch learner profile data along with competencies and learning objects.
367-367: LGTM! Enhanced user data retrieval.Updated to fetch user with learner profile data specific to the course context.
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (5)
94-95: LGTM! Clean dependency injection.Added CourseLearnerProfileService with proper constructor injection.
Also applies to: 99-100, 111-111
121-123: LGTM! Proper initialization of learner profiles.Ensures learner profiles are created for students when enabling learning paths.
134-137: LGTM! Consistent learner profile handling.Ensures learner profiles are created before generating learning paths.
139-145: LGTM! Well-documented method.Clear documentation of the method's purpose and parameters.
421-431: Ensure proper error handling for learner profile creation.The method attempts to create a course learner profile when not found, but there's no explicit error handling if the creation fails.
Consider adding try-catch block to handle potential exceptions during profile creation:
if (optionalLearningPath.isEmpty()) { LearningPath learningPathWithCourse = learningPathRepository.findWithEagerCourseByIdElseThrow(learningPathId); - courseLearnerProfileService.createCourseLearnerProfile(learningPathWithCourse.getCourse(), learningPathWithCourse.getUser()); - learningPath = learningPathRepository.findWithCompetenciesAndLectureUnitsAndExercisesAndLearnerProfileByIdElseThrow(learningPathId); + try { + courseLearnerProfileService.createCourseLearnerProfile(learningPathWithCourse.getCourse(), learningPathWithCourse.getUser()); + learningPath = learningPathRepository.findWithCompetenciesAndLectureUnitsAndExercisesAndLearnerProfileByIdElseThrow(learningPathId); + } catch (Exception e) { + log.error("Failed to create course learner profile for learning path {}: {}", learningPathId, e.getMessage()); + throw new RuntimeException("Failed to create course learner profile", e); + } }✅ Verification successful
Verified: Error handling is needed for profile creation
The implementation of
createCourseLearnerProfileuses repository operations that can throw exceptions (findByUserElseThrowandsave). Adding a try-catch block as suggested in the review comment is appropriate because:
- The service method is designed to propagate exceptions (uses methods with "ElseThrow" suffix)
- Multiple database operations could fail and need proper error handling
- It's a critical user-facing operation where graceful error handling improves user experience
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find implementation of createCourseLearnerProfile ast-grep --pattern 'createCourseLearnerProfile($$$) { $$$ }' # Look for similar error handling patterns rg "courseLearnerProfileService" -A 5 -B 5 # Find try-catch blocks in service layer fd "Service.java" --exec rg "try\s*\{.*?catch.*?\}" -ULength of output: 12577
Script:
#!/bin/bash # Find CourseLearnerProfileService implementation fd "CourseLearnerProfileService" --type f # Look for similar service methods with error handling rg "class.*Service.*implements" -A 10 # Find error handling patterns in service layer rg "@Service.*class" -A 20 -B 5Length of output: 68693
Script:
#!/bin/bash # Find implementation of createCourseLearnerProfile cat src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java # Look for error handling patterns in similar service methods rg "try\s*\{.*?catch.*?\}" -A 5 src/main/java/de/tum/cit/aet/artemis/atlas/service/Length of output: 3326
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java (2)
119-120: LGTM! Clean dependency injection.Added LearnerProfileApi with proper constructor injection.
Also applies to: 127-127, 142-142
476-478: 🛠️ Refactor suggestionConsider transaction boundary for profile deletion.
The profile deletion and user updates should be atomic to maintain data consistency.
Consider wrapping the operations in a transaction:
+@Transactional public void softDeleteUser(String login) { userRepository.findOneWithGroupsByLogin(login).ifPresent(user -> { participationVCSAccessTokenService.deleteAllByUserId(user.getId()); - learnerProfileApi.deleteProfile(user); - user.setDeleted(true); - user.setLearnerProfile(null); + try { + learnerProfileApi.deleteProfile(user); + user.setDeleted(true); + user.setLearnerProfile(null); + } catch (Exception e) { + log.error("Failed to delete learner profile for user {}: {}", user.getId(), e.getMessage()); + throw new RuntimeException("Failed to delete learner profile", e); + } anonymizeUser(user); log.warn("Soft Deleted User: {}", user); }); }Likely invalid or redundant comment.
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
86-86: Add verification for learner profile creation.The learner profile creation is added to the setup but there's no verification that the profiles were created successfully.
Add assertions to verify the learner profiles:
learnerProfileUtilService.createLearnerProfilesForUsers(TEST_PREFIX); +assertThat(learnerProfileRepository.findAll()).hasSize(NUMBER_OF_STUDENTS + NUMBER_OF_TUTORS + NUMBER_OF_EDITORS + NUMBER_OF_INSTRUCTORS);src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (1)
113-126: LGTM! Clean organization of repository dependencies.The repository dependencies are well-organized with each repository on its own line for clarity.
src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java (3)
102-112: LGTM! Well-structured JPQL query with efficient joins.The query is well-organized with appropriate LEFT JOINs to fetch the learner profile data efficiently.
193-194: LGTM! Efficient entity graph usage for eager loading.The method uses entity graph to efficiently load the related entities in a single query.
1025-1033: LGTM! Well-documented method with clear purpose.The method is well-documented and follows the repository's pattern for student retrieval methods.
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (1)
515-515: Handle potential exceptions when deleting learner profilesIn the
deletemethod, the call tolearnerProfileApi.deleteAllForCourse(course);might throw exceptions if an error occurs during the deletion process. It's important to handle these exceptions to ensure that the course deletion process is robust.Please ensure that any exceptions thrown by
learnerProfileApi.deleteAllForCourse(course);are properly handled, either by catching them and logging an appropriate message or by propagating them with additional context.
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/UserScheduleService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserCreationService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserCreationService.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java
Outdated
Show resolved
Hide resolved
…-graded-exercises # Conflicts: # src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java # src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java # src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java
github-actions
bot
removed
the
programming
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java (1)
906-917: Enhance error message for better debugging.While the implementation is solid, consider making the error message more specific by including the courseId in case of failure.
- return getValueElseThrow(findOneWithGroupsAndAuthoritiesAndLearnerProfileByLogin(currentUserLogin, courseId)); + return getValueElseThrow( + findOneWithGroupsAndAuthoritiesAndLearnerProfileByLogin(currentUserLogin, courseId), + String.format("User with login '%s' not found for course '%d'", currentUserLogin, courseId) + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.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
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java (1)
102-112: LGTM! Well-structured JPQL query.The query is well-optimized with appropriate LEFT JOINs and follows SQL best practices.
JohannesStoehr
changed the title
Adaptive learning: Add implementation for grade or bonus goal in learner profile
github-actions
bot
removed
the
database
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1)
421-421: Consider simplifying the method name.The current method name is quite long. Consider renaming it to something more concise while maintaining clarity, such as
findLearningPathWithDetailsById.- public LearningPath findWithCompetenciesAndReleasedLearningObjectsAndCompletedUsersAndLearnerProfileById(long learningPathId) { + public LearningPath findLearningPathWithDetailsById(long learningPathId) {Add a detailed JavaDoc to describe what "details" includes:
/** * Finds a learning path by ID with eagerly fetched: * - Competencies * - Released learning objects * - Completed users * - Learner profile * * @param learningPathId the ID of the learning path * @return the learning path with all related details * @throws EntityNotFoundException if the learning path is not found */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.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
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1)
135-135: LGTM! Good initialization of learner profiles.The addition ensures that learner profiles are created before generating learning paths, which is essential for the adaptive learning feature.
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
Show resolved
Hide resolved
Checklist
General
Server
Motivation and Context
Stacked on #9673
#8758
Description
Adapted scheduling of exercises in the learning path to respect the preferences regarding the inclusion of graded exercises.
This PR does not add any UI!
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Summary by CodeRabbit
New Features
Improvements
Technical Updates