Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adaptive learning: Add implementation for grade or bonus goal in learner profile #9699

Open
wants to merge 19 commits into
base: feature/adaptive-learning/learner-profile
Choose a base branch
from

Conversation

JohannesStoehr
Copy link
Contributor

@JohannesStoehr JohannesStoehr commented Nov 7, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I documented the Java code using JavaDoc style.

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:

  • 1 Instructor
  • 1 Student
  1. Log in to Artemis
  2. Create or find a course with a few competencies and linked exercises and lecture units
  3. Participate as student in the learning path and check that no errors occur

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Summary by CodeRabbit

  • New Features

    • Introduced CourseLearnerProfile and LearnerProfile entities for managing learner profiles in relation to courses.
    • Added methods for creating and deleting course learner profiles.
    • Enhanced learning path recommendations by incorporating learner profiles into exercise scheduling.
    • Updated user management to include learner profile creation and deletion upon user actions.
  • Bug Fixes

    • Improved data retrieval methods to ensure learner profile information is included in learning path navigation.
  • Documentation

    • Added Javadoc comments for new repository methods related to user and learner profile retrieval.
  • Tests

    • Updated integration tests to include setup for learner profiles in test scenarios.

@JohannesStoehr JohannesStoehr requested a review from a team as a code owner November 7, 2024 15:33
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. atlas Pull requests that affect the corresponding module core Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module labels Nov 7, 2024
@JohannesStoehr JohannesStoehr changed the base branch from develop to feature/adaptive-learning/learner-profile November 7, 2024 15:33
Copy link

coderabbitai bot commented Nov 7, 2024

Walkthrough

This pull request introduces several new classes and modifications to existing ones, primarily focusing on the management of learner profiles and their relationships with courses in a learning management system. Key additions include the CourseLearnerProfile and LearnerProfile entities, along with their respective repositories and services. The changes enhance the functionality of various services and resources to better accommodate learner-specific data, allowing for more personalized learning experiences.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java Added CourseLearnerProfile entity with fields for learner profile, course, aim for grade or bonus, time investment, and repetition intensity.
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java Added LearnerProfile entity with a relationship to User and a collection of CourseLearnerProfile.
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/PreferenceScale.java Introduced PreferenceScale enum with constants for user preferences.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java Created a repository interface for CourseLearnerProfile with methods for deleting profiles by course and user.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearnerProfileRepository.java Created a repository interface for LearnerProfile with methods to find and delete profiles by user.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java Modified methods to include learner profile data in learning path retrieval.
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java Enhanced recommendation logic to incorporate CourseLearnerProfile.
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java Updated to use CourseLearnerProfileService for generating learning paths.
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java Introduced service for managing course learner profiles with methods for creation and deletion.
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/LearnerProfileService.java Created service for managing learner profiles with methods for creation and deletion.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java Updated methods to include learner profile data in learning path navigation.
src/main/java/de/tum/cit/aet/artemis/core/domain/User.java Added a LearnerProfile relationship to the User entity.
src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java Added methods to retrieve users with their learner profiles.
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java Integrated CourseLearnerProfileService for user enrollment and group management.
src/main/java/de/tum/cit/aet/artemis/core/service/UserScheduleService.java Updated to manage learner profiles upon user deletion.
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserCreationService.java Modified to create learner profiles upon user registration.
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java Integrated learner profile management into the user soft deletion process.
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java Updated to manage learner profiles during course updates and user group management.
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java Added setup for creating learner profiles in tests.
src/test/java/de/tum/cit/aet/artemis/core/test_repository/UserTestRepository.java Introduced method for retrieving users by login prefix.
src/test/java/de/tum/cit/aet/artemis/core/user/util/UserUtilService.java Added functionality to create learner profiles for users based on a prefix.
src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.java Added test for adding users to courses, including learner profile creation.

Possibly related issues

Possibly related PRs

Suggested labels

tests, server, enhancement, client, ready to merge, atlas

Suggested reviewers

  • MaximilianAnzinger
  • pzdr7
  • az108
  • florian-glombik
  • sarpsahinalp
  • EneaGore

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 28

🧹 Outside diff range and nitpick comments (32)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearnerProfileRepository.java (3)

20-20: Consider adding @query annotation for findByUser.

For better control over the generated SQL and potential performance optimization, consider explicitly defining the query.

+    @Query("SELECT lp FROM LearnerProfile lp WHERE lp.user = :user")
     Optional<LearnerProfile> findByUser(User user);

26-28: Improve @transactional documentation and consider adding @query.

  1. The comment "ok because of delete" is not informative. Consider removing it or providing better documentation.
  2. Consider adding an explicit @query for better performance and clarity.
-    @Transactional // ok because of delete
+    @Transactional
+    @Query("DELETE FROM LearnerProfile lp WHERE lp.user = :user")
     @Modifying
     void deleteByUser(User user);

20-28: Add JavaDoc documentation for repository methods.

Consider adding documentation to describe the purpose, parameters, and return values of each method.

+    /**
+     * Find a learner profile by its associated user.
+     *
+     * @param user the user to search for
+     * @return an Optional containing the learner profile if found
+     */
     Optional<LearnerProfile> findByUser(User user);

+    /**
+     * Find a learner profile by user, throwing an exception if not found.
+     *
+     * @param user the user to search for
+     * @return the learner profile
+     * @throws EntityNotFoundException if no profile exists for the user
+     */
     default LearnerProfile findByUserElseThrow(User user)

+    /**
+     * Delete a learner profile associated with the given user.
+     *
+     * @param user the user whose profile should be deleted
+     */
     void deleteByUser(User user);
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/LearnerProfileService.java (2)

13-15: Consider adding class-level documentation.

While the class structure and annotations are correct, adding Javadoc would improve code maintainability by documenting the service's purpose and responsibilities.

Add documentation like:

+/**
+ * Service for managing learner profiles.
+ * Handles the creation and deletion of learner profiles and their associated course profiles.
+ */
 @Profile(PROFILE_CORE)
 @Service
 public class LearnerProfileService {

17-24: Consider transaction management for repository operations.

While the constructor injection is correctly implemented, the service might benefit from transaction management for database operations.

Consider adding transaction management:

 @Profile(PROFILE_CORE)
 @Service
+@Transactional(readOnly = true)
 public class LearnerProfileService {
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java (2)

21-27: Improve the transaction annotation documentation.

While the implementation is correct, consider replacing the comment "ok because of delete" with more informative documentation explaining why the transaction is necessary and safe in this context.

-    @Transactional // ok because of delete
+    @Transactional // Required for modifying queries, safe here as it's a simple delete operation

33-39: Maintain consistent documentation improvement.

The implementation is correct, but for consistency, improve the transaction annotation documentation similar to the other methods.

-    @Transactional // ok because of delete
+    @Transactional // Required for modifying queries, safe here as it's a simple delete operation
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (1)

22-28: Consider adding an index for the user_id foreign key.

While the relationships are well-defined with appropriate fetch strategies, adding an index on the user_id column would optimize queries, especially for user lookups.

Add the following index to improve query performance:

@Table(name = "learner_profile", indexes = {
    @Index(name = "idx_learner_profile_user_id", columnList = "user_id")
})
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java (4)

14-16: Add class-level documentation.

Consider adding Javadoc to describe the purpose of this entity, its relationships, and the meaning of its fields. This will improve code maintainability and help other developers understand the domain model better.

+/**
+ * Entity representing a learner's profile specific to a course.
+ * This entity captures course-specific learning preferences and goals,
+ * including grade/bonus targets, time investment, and repetition intensity.
+ */
 @Entity
 @Table(name = "course_learner_profile")
 public class CourseLearnerProfile extends DomainObject {

18-24: Enhance relationship definitions with additional constraints.

Consider adding the following JPA annotations to better define the relationships:

 @ManyToOne
-@JoinColumn(name = "learner_profile_id")
+@JoinColumn(name = "learner_profile_id", nullable = false,
+    foreignKey = @ForeignKey(name = "fk_course_learner_profile_learner"))
 private LearnerProfile learnerProfile;

 @OneToOne(fetch = FetchType.LAZY)
-@JoinColumn(name = "course_id")
+@JoinColumn(name = "course_id", nullable = false, unique = true,
+    foreignKey = @ForeignKey(name = "fk_course_learner_profile_course"))
 private Course course;

These changes will:

  • Enforce referential integrity with proper foreign key constraints
  • Ensure required relationships are not null
  • Prevent duplicate course assignments

35-73: Add parameter validation to setters.

Consider adding validation to prevent invalid values:

+import java.util.Objects;

     public void setLearnerProfile(LearnerProfile learnerProfile) {
+        Objects.requireNonNull(learnerProfile, "learnerProfile must not be null");
         this.learnerProfile = learnerProfile;
     }

     public void setCourse(Course course) {
+        Objects.requireNonNull(course, "course must not be null");
         this.course = course;
     }

     public void setAimForGradeOrBonus(int aimForGradeOrBonus) {
+        if (aimForGradeOrBonus < 0 || aimForGradeOrBonus > 100) {
+            throw new IllegalArgumentException("aimForGradeOrBonus must be between 0 and 100");
+        }
         this.aimForGradeOrBonus = aimForGradeOrBonus;
     }

     public void setTimeInvestment(int timeInvestment) {
+        if (timeInvestment < 0 || timeInvestment > 168) {
+            throw new IllegalArgumentException("timeInvestment must be between 0 and 168 hours");
+        }
         this.timeInvestment = timeInvestment;
     }

     public void setRepetitionIntensity(int repetitionIntensity) {
+        if (repetitionIntensity < 1 || repetitionIntensity > 5) {
+            throw new IllegalArgumentException("repetitionIntensity must be between 1 and 5");
+        }
         this.repetitionIntensity = repetitionIntensity;
     }

14-16: Add database indexes for better query performance.

Consider adding indexes to improve query performance:

 @Entity
-@Table(name = "course_learner_profile")
+@Table(name = "course_learner_profile",
+    indexes = {
+        @Index(name = "idx_course_learner_profile_learner",
+               columnList = "learner_profile_id"),
+        @Index(name = "idx_course_learner_profile_course",
+               columnList = "course_id")
+    })
 public class CourseLearnerProfile extends DomainObject {
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserCreationService.java (1)

Line range hint 1-379: Consider splitting user creation and profile management responsibilities.

The UserCreationService is taking on additional responsibilities with profile management. This could lead to maintenance issues as the profile feature grows.

Consider creating a separate UserProfileOrchestrationService to handle the coordination between user creation and profile management, following the Single Responsibility Principle.

src/main/java/de/tum/cit/aet/artemis/core/domain/User.java (2)

229-231: Consider marking field as @nullable and reviewing cascade options.

The relationship definition looks good overall, but consider these improvements:

  1. Add @Nullable annotation to the learnerProfile field since a user might not have a profile initially
  2. Review if CascadeType.ALL is necessary - consider using more specific cascade types (e.g., PERSIST, MERGE) if not all operations need to cascade
    @OneToOne(mappedBy = "user", fetch = FetchType.LAZY, cascade = CascadeType.ALL, orphanRemoval = true)
    @JsonIgnoreProperties("user")
-   private LearnerProfile learnerProfile;
+   @Nullable
+   private LearnerProfile learnerProfile;

576-582: Add documentation and null-safety annotations to getter/setter methods.

The methods should include:

  1. JavaDoc documentation explaining the purpose and behavior
  2. Null-safety annotations for better API understanding
  3. Consider adding validation in the setter method
+   /**
+    * Gets the learner profile associated with this user.
+    * @return the learner profile, or null if not set
+    */
+   @Nullable
    public LearnerProfile getLearnerProfile() {
        return learnerProfile;
    }

+   /**
+    * Sets the learner profile for this user.
+    * @param learnerProfile the learner profile to set
+    */
    public void setLearnerProfile(@Nullable LearnerProfile learnerProfile) {
        this.learnerProfile = learnerProfile;
    }
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java (3)

Line range hint 344-350: Consider extracting the transformation logic for better readability.

While the implementation is correct, the stream operations could be more readable by extracting the transformation logic into a separate method.

Consider refactoring like this:

-        List<CompetencyNameDTO> competencyNames = recommendationState.recommendedOrderOfCompetencies().stream()
-                .map(competencyId -> recommendationState.competencyIdMap().get(competencyId)).map(CompetencyNameDTO::of).toList();
+        List<CompetencyNameDTO> competencyNames = recommendationState.recommendedOrderOfCompetencies().stream()
+                .map(this::mapToCompetencyNameDTO)
+                .toList();
+    }
+
+    private CompetencyNameDTO mapToCompetencyNameDTO(Long competencyId) {
+        return CompetencyNameDTO.of(recommendationState.competencyIdMap().get(competencyId));

367-367: Add error handling for course ID retrieval.

While the implementation correctly retrieves the user with learner profile, it should handle potential null course scenarios.

Consider adding null checks:

-        final var user = userRepository.getUserWithGroupsAndAuthoritiesAndLearnerProfile(learningPath.getCourse().getId());
+        final var course = learningPath.getCourse();
+        if (course == null) {
+            throw new IllegalStateException("Learning path must be associated with a course");
+        }
+        final var user = userRepository.getUserWithGroupsAndAuthoritiesAndLearnerProfile(course.getId());

Line range hint 235-367: Consider documenting the learner profile integration pattern.

The integration of learner profiles across multiple endpoints follows a consistent pattern. Consider documenting this pattern in the class-level JavaDoc to help future maintainers understand the design decision.

src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (2)

138-143: Improve method documentation for the new overloaded method.

The JavaDoc comment is identical to the original method and doesn't describe the additional students parameter.

Update the documentation:

    /**
-     * Generate learning paths for all students enrolled in the course
+     * Generate learning paths for a specific set of students in the course
     *
     * @param course course the learning paths are created for
+     * @param students set of students for whom learning paths should be generated
     */
    public void generateLearningPaths(@NotNull Course course, Set<User> students) {

419-420: Consider optimizing data fetching strategy.

The method findWithCompetenciesAndReleasedLearningObjectsAndCompletedUsersAndLearnerProfileById performs multiple separate queries to fetch related data. This could lead to N+1 query issues with large datasets.

Consider:

  1. Using batch fetching for competencies and their related entities
  2. Implementing a custom query using SQL joins to fetch all required data in a single query
  3. Adding appropriate database indexes for frequently accessed columns

Example optimization for competency progress fetching:

-   Map<Long, CompetencyProgress> competencyProgresses = competencyProgressRepository
-       .findAllByCompetencyIdsAndUserId(competencyIds, userId).stream()
-       .collect(Collectors.toMap(progress -> progress.getCompetency().getId(), cp -> cp));
+   @Query("""
+       SELECT cp FROM CompetencyProgress cp
+       JOIN FETCH cp.competency c
+       WHERE c.id IN :competencyIds AND cp.user.id = :userId
+       """)
+   List<CompetencyProgress> findAllByCompetencyIdsAndUserIdWithFetch(
+       @Param("competencyIds") Set<Long> competencyIds,
+       @Param("userId") Long userId);
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (3)

Line range hint 447-476: Add documentation for test utility methods.

The utility methods createAndLinkTextExercise and createAndLinkTextUnit would benefit from JavaDoc comments explaining their purpose, parameters, and return values.

Apply this documentation:

+    /**
+     * Creates a text exercise and links it to the given competency.
+     * @param competency The competency to link the exercise to
+     * @param withAssessment If true, creates a student score of 100%
+     * @return The created and linked text exercise
+     */
     private TextExercise createAndLinkTextExercise(Competency competency, boolean withAssessment) {

+    /**
+     * Creates a text unit, adds it to the lecture, and links it to the given competency.
+     * @param student The student to set completion status for
+     * @param competency The competency to link the unit to
+     * @param completed If true, marks the unit as completed for the student
+     * @return The created and linked text unit
+     */
     private TextUnit createAndLinkTextUnit(User student, Competency competency, boolean completed) {

Line range hint 35-76: Consider extracting test data constants to a dedicated class.

The test class contains numerous test data constants. Consider extracting these to a dedicated test data configuration class to improve maintainability and reusability.

Example refactor:

public class LearningPathTestData {
    public static final String TEST_PREFIX = "learningpathintegration";
    public static final int NUMBER_OF_STUDENTS = 5;
    public static final String STUDENT1_OF_COURSE = TEST_PREFIX + "student1";
    // ... other constants
}

Line range hint 324-325: Address TODO comment about duplicate learning objects.

The TODO comment indicates a known issue where learning objects connected to multiple competencies appear multiple times in the learning path. This should be tracked and addressed.

Would you like me to create a GitHub issue to track this improvement task?

src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.java (1)

705-705: Consider improving test method organization.

While the addition of learner profile creation is necessary for the new functionality, consider the following improvements:

  1. Rename the test method to reflect that it's testing both learner profile setup and course user management, e.g., testAddStudentOrTutorOrInstructorToCourseWithLearnerProfile.
  2. Consider splitting this into two separate test methods:
    • One focusing on learner profile setup
    • Another focusing on course user management with the learner profile as a prerequisite

This would improve test clarity and maintain the single responsibility principle for tests.

src/main/java/de/tum/cit/aet/artemis/core/repository/UserRepository.java (2)

102-112: Consider adding an index on the login column for better query performance.

The query looks well-structured and uses proper JOIN FETCH statements to avoid N+1 query problems. However, since this query filters by login, consider adding an index on the login column if not already present.


916-926: Consider enhancing the error message for better debugging.

The implementation is solid, but consider enhancing the error message in getValueElseThrow to include both the login and courseId for better debugging when the user is not found.

-        return getValueElseThrow(findOneWithGroupsAndAuthoritiesAndLearnerProfileByLogin(currentUserLogin, courseId));
+        return getValueElseThrow(
+            findOneWithGroupsAndAuthoritiesAndLearnerProfileByLogin(currentUserLogin, courseId),
+            String.format("User not found with login '%s' and courseId '%d'", currentUserLogin, courseId)
+        );
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)

1276-1283: Improve null safety in addUserToCourseGroup method

The method could benefit from additional null checks and validation.

Consider adding parameter validation:

 @NotNull
 public ResponseEntity<Void> addUserToCourseGroup(String userLogin, User instructorOrAdmin, Course course, String group) {
+    if (userLogin == null || instructorOrAdmin == null || course == null || group == null) {
+        throw new IllegalArgumentException("Required parameters cannot be null");
+    }
+
     if (authCheckService.isAtLeastInstructorInCourse(course, instructorOrAdmin)) {
         Optional<User> userToAddToGroup = userRepository.findOneWithGroupsAndAuthoritiesByLogin(userLogin);
         if (userToAddToGroup.isEmpty()) {
             throw new EntityNotFoundException("User", userLogin);
         }
         User user = userToAddToGroup.get();
         courseService.addUserToGroup(user, group, course);
         return ResponseEntity.ok().body(null);
     } else {
         throw new AccessForbiddenException();
     }
 }
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java (1)

604-615: Clarify comparator behavior for low preference

In getExerciseOrderComparator, when aimForGradeOrBonus is low, the comparator returns Comparator.comparing(ignored -> 0), which maintains the original order. Consider adding a comment to explain this choice for better code clarity.

src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (2)

1056-1060: Optimize database call by reusing existing course object

In addUserToGroup, you fetch courseWithCompetencies from the database, which may be unnecessary if the current course object already contains the required competencies and prerequisites. This additional call can affect performance.

Consider reusing the existing course object or updating it to include the needed data to eliminate the extra database query.


512-512: Consider checking if learning paths are enabled before deleting course learner profiles

Deleting all course learner profiles when learning paths are disabled may be unnecessary and could introduce overhead.

Optionally, add a condition:

+if (course.getLearningPathsEnabled()) {
    courseLearnerProfileService.deleteAllForCourse(course);
+}
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java (2)

66-79: Optimize query to improve performance due to multiple LEFT JOIN FETCH clauses

Using multiple LEFT JOIN FETCH clauses can lead to performance issues by generating large result sets and causing Cartesian product effects. Consider the following options to optimize the query:

  • Use @EntityGraph Annotation: Replace the JPQL query with an @EntityGraph to specify the fetching strategy declaratively.
  • Fetch Associations Lazily: Modify the entity mappings to use lazy fetching where appropriate and load associations on demand.
  • Split the Query: Break down the query into multiple simpler queries to reduce complexity and improve performance.

66-82: Consider using @EntityGraph for consistency and maintainability

Other repository methods utilize the @EntityGraph annotation to manage fetching strategies. For consistency and easier maintenance, consider converting the JPQL query into a method annotated with @EntityGraph.

Example:

@EntityGraph(type = LOAD, attributePaths = {
    "competencies",
    "competencies.lectureUnitLinks.lectureUnit",
    "competencies.exerciseLinks.exercise",
    "user",
    "user.learnerProfile",
    "user.learnerProfile.courseLearnerProfiles"
})
Optional<LearningPath> findWithCompetenciesAndLectureUnitsAndExercisesAndLearnerProfileById(long learningPathId);

Ensure that the entity paths match the desired associations to be fetched.

🛑 Comments failed to post (28)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearnerProfileRepository.java (1)

22-24: 🛠️ Refactor suggestion

Enhance error handling in findByUserElseThrow.

The method should provide a meaningful exception message when no profile is found.

     default LearnerProfile findByUserElseThrow(User user) {
-        return getValueElseThrow(findByUser(user));
+        return findByUser(user)
+            .orElseThrow(() -> new EntityNotFoundException(
+                String.format("LearnerProfile not found for user: %s", user.getLogin())
+            ));
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    default LearnerProfile findByUserElseThrow(User user) {
        return findByUser(user)
            .orElseThrow(() -> new EntityNotFoundException(
                String.format("LearnerProfile not found for user: %s", user.getLogin())
            ));
    }
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/LearnerProfileService.java (2)

26-30: 🛠️ Refactor suggestion

Enhance createProfile method with validation and error handling.

The method should validate input, handle potential errors, and include logging for better observability.

Consider these improvements:

+    @Transactional
     public LearnerProfile createProfile(User user) {
+        if (user == null) {
+            throw new IllegalArgumentException("User cannot be null");
+        }
+        if (learnerProfileRepository.existsByUser(user)) {
+            throw new IllegalStateException("Profile already exists for user: " + user.getId());
+        }
         var profile = new LearnerProfile();
         profile.setUser(user);
-        return learnerProfileRepository.save(profile);
+        var savedProfile = learnerProfileRepository.save(profile);
+        log.debug("Created learner profile for user: {}", user.getId());
+        return savedProfile;
     }

Committable suggestion skipped: line range outside the PR's diff.


32-35: ⚠️ Potential issue

Improve deleteProfile method with proper transaction management and error handling.

The current implementation might face referential integrity issues and lacks proper error handling.

Consider these improvements:

+    @Transactional
     public void deleteProfile(User user) {
+        if (user == null) {
+            throw new IllegalArgumentException("User cannot be null");
+        }
+        var profile = learnerProfileRepository.findByUser(user)
+            .orElseThrow(() -> new IllegalStateException("Profile not found for user: " + user.getId()));
+
+        log.debug("Deleting course profiles for user: {}", user.getId());
         courseLearnerProfileRepository.deleteAllByUser(user);
+
+        log.debug("Deleting learner profile for user: {}", user.getId());
         learnerProfileRepository.deleteByUser(user);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    @Transactional
    public void deleteProfile(User user) {
        if (user == null) {
            throw new IllegalArgumentException("User cannot be null");
        }
        var profile = learnerProfileRepository.findByUser(user)
            .orElseThrow(() -> new IllegalStateException("Profile not found for user: " + user.getId()));

        log.debug("Deleting course profiles for user: {}", user.getId());
        courseLearnerProfileRepository.deleteAllByUser(user);

        log.debug("Deleting learner profile for user: {}", user.getId());
        learnerProfileRepository.deleteByUser(user);
    }
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java (1)

29-31: ⚠️ Potential issue

Fix parameter name typo and improve documentation.

There are two issues to address:

  1. The parameter name contains a typo: "couese" should be "course"
  2. The transaction comment could be more informative
-    @Transactional // ok because of delete
-    void deleteAllByCourse(Course couese);
+    @Transactional // Required for modifying queries, safe here as it's a simple delete operation
+    void deleteAllByCourse(Course course);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    @Transactional // Required for modifying queries, safe here as it's a simple delete operation
    @Modifying
    void deleteAllByCourse(Course course);
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/LearnerProfile.java (2)

29-35: 🛠️ Refactor suggestion

Add bidirectional relationship management.

The User relationship methods should maintain both sides of the relationship to prevent inconsistencies.

Consider this implementation:

 public void setUser(User user) {
+    if (this.user != null) {
+        this.user.setLearnerProfile(null);
+    }
     this.user = user;
+    if (user != null) {
+        user.setLearnerProfile(this);
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.


37-55: 🛠️ Refactor suggestion

Enhance collection management with defensive programming.

The collection management methods should:

  1. Include null checks
  2. Maintain bidirectional relationships
  3. Use defensive copying in getters

Consider these improvements:

 public Set<CourseLearnerProfile> getCourseLearnerProfiles() {
-    return this.courseLearnerProfiles;
+    return new HashSet<>(this.courseLearnerProfiles);
 }

 public boolean addCourseLearnerProfile(CourseLearnerProfile courseLearnerProfile) {
+    if (courseLearnerProfile == null) {
+        return false;
+    }
+    courseLearnerProfile.setLearnerProfile(this);
     return this.courseLearnerProfiles.add(courseLearnerProfile);
 }

 public boolean removeCourseLearnerProfile(CourseLearnerProfile courseLearnerProfile) {
+    if (courseLearnerProfile == null) {
+        return false;
+    }
+    courseLearnerProfile.setLearnerProfile(null);
     return this.courseLearnerProfiles.remove(courseLearnerProfile);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public void setCourseLearnerProfiles(Set<CourseLearnerProfile> courseLearnerProfiles) {
        this.courseLearnerProfiles = courseLearnerProfiles;
    }

    public Set<CourseLearnerProfile> getCourseLearnerProfiles() {
        return new HashSet<>(this.courseLearnerProfiles);
    }

    public boolean addCourseLearnerProfile(CourseLearnerProfile courseLearnerProfile) {
        if (courseLearnerProfile == null) {
            return false;
        }
        courseLearnerProfile.setLearnerProfile(this);
        return this.courseLearnerProfiles.add(courseLearnerProfile);
    }

    public boolean addAllCourseLearnerProfiles(Collection<? extends CourseLearnerProfile> courseLearnerProfiles) {
        return this.courseLearnerProfiles.addAll(courseLearnerProfiles);
    }

    public boolean removeCourseLearnerProfile(CourseLearnerProfile courseLearnerProfile) {
        if (courseLearnerProfile == null) {
            return false;
        }
        courseLearnerProfile.setLearnerProfile(null);
        return this.courseLearnerProfiles.remove(courseLearnerProfile);
    }
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java (1)

26-33: 🛠️ Refactor suggestion

Add field validation and documentation.

The integer fields need validation and documentation to ensure data integrity:

+import jakarta.validation.constraints.Min;
+import jakarta.validation.constraints.Max;

+    /**
+     * The target grade or bonus points the learner aims to achieve.
+     * Valid range: 0-100 (percentage)
+     */
     @Column(name = "aim_for_grade_or_bonus")
+    @Min(0) @Max(100)
     private int aimForGradeOrBonus;

+    /**
+     * Weekly time investment in hours.
+     * Valid range: 0-168 (max hours per week)
+     */
     @Column(name = "time_investment")
+    @Min(0) @Max(168)
     private int timeInvestment;

+    /**
+     * Repetition intensity on a scale of 1-5.
+     * 1: minimal, 5: intensive
+     */
     @Column(name = "repetition_intensity")
+    @Min(1) @Max(5)
     private int repetitionIntensity;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    import jakarta.validation.constraints.Min;
    import jakarta.validation.constraints.Max;

    /**
     * The target grade or bonus points the learner aims to achieve.
     * Valid range: 0-100 (percentage)
     */
    @Column(name = "aim_for_grade_or_bonus")
    @Min(0) @Max(100)
    private int aimForGradeOrBonus;

    /**
     * Weekly time investment in hours.
     * Valid range: 0-168 (max hours per week)
     */
    @Column(name = "time_investment")
    @Min(0) @Max(168)
    private int timeInvestment;

    /**
     * Repetition intensity on a scale of 1-5.
     * 1: minimal, 5: intensive
     */
    @Column(name = "repetition_intensity")
    @Min(1) @Max(5)
    private int repetitionIntensity;
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java (7)

25-29: ⚠️ Potential issue

Remove unused constructor parameter learnerProfileService.

The constructor accepts LearnerProfileService but never uses it. This violates the YAGNI principle.

-    public CourseLearnerProfileService(CourseLearnerProfileRepository courseLearnerProfileRepository, LearnerProfileService learnerProfileService,
-            LearnerProfileRepository learnerProfileRepository) {
+    public CourseLearnerProfileService(CourseLearnerProfileRepository courseLearnerProfileRepository,
+            LearnerProfileRepository learnerProfileRepository) {
         this.courseLearnerProfileRepository = courseLearnerProfileRepository;
         this.learnerProfileRepository = learnerProfileRepository;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public CourseLearnerProfileService(CourseLearnerProfileRepository courseLearnerProfileRepository,
            LearnerProfileRepository learnerProfileRepository) {
        this.courseLearnerProfileRepository = courseLearnerProfileRepository;
        this.learnerProfileRepository = learnerProfileRepository;
    }

17-19: 🛠️ Refactor suggestion

Add class-level documentation and @transactional.

The service class should have:

  1. JavaDoc explaining its purpose and responsibilities
  2. Transaction management for database operations
 @Profile(PROFILE_CORE)
 @Service
+@Transactional
+/**
+ * Service for managing course learner profiles.
+ * Handles the creation, deletion and management of profiles that track a learner's progress within a specific course.
+ */
 public class CourseLearnerProfileService {

Committable suggestion skipped: line range outside the PR's diff.


57-59: 🛠️ Refactor suggestion

Add validation and consider batch deletion for large datasets.

The bulk delete method needs improvements:

  1. Add input validation
  2. Add transaction boundary
  3. Consider batch deletion for large datasets
+    /**
+     * Deletes all course learner profiles for a given course.
+     *
+     * @param course the course, must not be null
+     * @throws IllegalArgumentException if course is null
+     */
+    @Transactional
     public void deleteAllForCourse(Course course) {
+        if (course == null) {
+            throw new IllegalArgumentException("Course must not be null");
+        }
+        // TODO: Consider implementing batch deletion for large datasets
         courseLearnerProfileRepository.deleteAllByCourse(course);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Deletes all course learner profiles for a given course.
     *
     * @param course the course, must not be null
     * @throws IllegalArgumentException if course is null
     */
    @Transactional
    public void deleteAllForCourse(Course course) {
        if (course == null) {
            throw new IllegalArgumentException("Course must not be null");
        }
        // TODO: Consider implementing batch deletion for large datasets
        courseLearnerProfileRepository.deleteAllByCourse(course);
    }

41-51: ⚠️ Potential issue

Fix N+1 query issue and add validation.

The current implementation has several issues:

  1. Potential N+1 query problem when accessing learner profiles
  2. Missing input validation
  3. Direct entity access instead of using repository
  4. No duplicate handling
+    /**
+     * Bulk creates course learner profiles for multiple users.
+     *
+     * @param course the course, must not be null
+     * @param users the set of users, must not be null or empty
+     * @throws IllegalArgumentException if course or users is null/empty
+     */
+    @Transactional
     public void createCourseLearnerProfiles(Course course, Set<User> users) {
+        if (course == null || users == null || users.isEmpty()) {
+            throw new IllegalArgumentException("Course and users must not be null/empty");
+        }
+
+        // Fetch all learner profiles in one query
+        var learnerProfiles = learnerProfileRepository.findAllByUserIn(users);
+        var userProfileMap = learnerProfiles.stream()
+            .collect(Collectors.toMap(profile -> profile.getUser().getId(), profile -> profile));
+
         Set<CourseLearnerProfile> courseProfiles = users.stream().map(user -> {
+            if (!userProfileMap.containsKey(user.getId())) {
+                throw new IllegalStateException("Learner profile not found for user: " + user.getId());
+            }
             var courseProfile = new CourseLearnerProfile();
             courseProfile.setCourse(course);
-            courseProfile.setLearnerProfile(user.getLearnerProfile());
+            courseProfile.setLearnerProfile(userProfileMap.get(user.getId()));

Committable suggestion skipped: line range outside the PR's diff.


31-39: ⚠️ Potential issue

Add input validation and duplicate handling.

The method needs several improvements:

  1. Validate input parameters
  2. Handle potential duplicate profiles
  3. Add method-level transaction annotation
+    /**
+     * Creates a course learner profile for a given course and user.
+     *
+     * @param course the course, must not be null
+     * @param user the user, must not be null
+     * @throws IllegalArgumentException if course or user is null
+     */
+    @Transactional
     public void createCourseLearnerProfile(Course course, User user) {
+        if (course == null || user == null) {
+            throw new IllegalArgumentException("Course and user must not be null");
+        }
+        
+        if (courseLearnerProfileRepository.existsByCourseAndUser(course, user)) {
+            throw new IllegalStateException("Course learner profile already exists");
+        }
+
         var courseProfile = new CourseLearnerProfile();
         courseProfile.setCourse(course);

Committable suggestion skipped: line range outside the PR's diff.


53-55: 🛠️ Refactor suggestion

Add input validation and transaction boundary.

The delete method needs input validation and transaction management.

+    /**
+     * Deletes a course learner profile for a given course and user.
+     *
+     * @param course the course, must not be null
+     * @param user the user, must not be null
+     * @throws IllegalArgumentException if course or user is null
+     */
+    @Transactional
     public void deleteCourseLearnerProfile(Course course, User user) {
+        if (course == null || user == null) {
+            throw new IllegalArgumentException("Course and user must not be null");
+        }
         courseLearnerProfileRepository.deleteByCourseAndUser(course, user);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Deletes a course learner profile for a given course and user.
     *
     * @param course the course, must not be null
     * @param user the user, must not be null
     * @throws IllegalArgumentException if course or user is null
     */
    @Transactional
    public void deleteCourseLearnerProfile(Course course, User user) {
        if (course == null || user == null) {
            throw new IllegalArgumentException("Course and user must not be null");
        }
        courseLearnerProfileRepository.deleteByCourseAndUser(course, user);
    }

21-23: 💡 Codebase verification

Based on the verification results, I can now provide the final response:

Repository methods need to be added

The following required methods are missing from the repository interfaces:

  • findAllByUserIn in LearnerProfileRepository
  • existsByCourseAndUser in CourseLearnerProfileRepository
🔗 Analysis chain

Verify repository method declarations.

Let's verify that all required repository methods are properly declared in the interfaces.


Let me try a different approach to verify the repository interfaces using rg instead of ast-grep.


Let me check for one more method that wasn't visible in the first 10 lines.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for repository interface declarations
echo "Searching for repository interfaces and their method declarations..."

# Check CourseLearnerProfileRepository
echo "Checking CourseLearnerProfileRepository..."
ast-grep --pattern 'interface CourseLearnerProfileRepository {
  $$$
  deleteByCourseAndUser($$$);
  $$$
  deleteAllByCourse($$$);
  $$$
  existsByCourseAndUser($$$);
  $$$
}'

# Check LearnerProfileRepository
echo "Checking LearnerProfileRepository..."
ast-grep --pattern 'interface LearnerProfileRepository {
  $$$
  findByUserElseThrow($$$);
  $$$
  findAllByUserIn($$$);
  $$$
}'

Length of output: 882


Script:

#!/bin/bash
# Search for repository interface declarations and their methods
echo "Searching for CourseLearnerProfileRepository..."
rg -A 10 "interface CourseLearnerProfileRepository" --type java

echo -e "\nSearching for LearnerProfileRepository..."
rg -A 10 "interface LearnerProfileRepository" --type java

Length of output: 3067


Script:

#!/bin/bash
# Search for specific methods in repositories
echo "Searching for findAllByUserIn in LearnerProfileRepository..."
rg "findAllByUserIn" --type java

echo -e "\nSearching for deleteAllByCourse and existsByCourseAndUser in CourseLearnerProfileRepository..."
rg "deleteAllByCourse|existsByCourseAndUser" --type java

Length of output: 1552

src/test/java/de/tum/cit/aet/artemis/core/test_repository/UserTestRepository.java (1)

79-84: 🛠️ Refactor suggestion

Consider adding pagination and case sensitivity control.

The new query method has potential performance implications:

  1. Returns unbounded results without pagination
  2. Doesn't specify case sensitivity for the LIKE operation

Consider this alternative implementation:

-    @Query("""
-            SELECT user
-            FROM User user
-            WHERE user.login LIKE CONCAT(:userPrefix, '%')
-            """)
-    Set<User> findAllByUserPrefix(String userPrefix);
+    @Query("""
+            SELECT user
+            FROM User user
+            WHERE LOWER(user.login) LIKE LOWER(CONCAT(:userPrefix, '%'))
+            """)
+    Page<User> findAllByUserPrefix(String userPrefix, Pageable pageable);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    @Query("""
            SELECT user
            FROM User user
            WHERE LOWER(user.login) LIKE LOWER(CONCAT(:userPrefix, '%'))
            """)
    Page<User> findAllByUserPrefix(String userPrefix, Pageable pageable);
src/main/java/de/tum/cit/aet/artemis/core/service/UserScheduleService.java (1)

119-123: 🛠️ Refactor suggestion

Consider improving error handling and transaction management.

The current implementation has potential issues:

  1. If profile deletion fails after the user is deleted and flushed, it could leave orphaned profiles
  2. The operations are not wrapped in a transaction, risking partial completion

Consider refactoring to:

  1. Add error handling
  2. Ensure atomic operations
  3. Delete profile before flushing changes
 private void deleteUser(User user) {
+    try {
         userRepository.delete(user);
         clearUserCaches(user);
+        learnerProfileService.deleteProfile(user);
         userRepository.flush();
-        learnerProfileService.deleteProfile(user);
+    } catch (Exception e) {
+        log.error("Failed to delete user and associated profile", e);
+        throw new RuntimeException("User deletion failed", e);
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        try {
            userRepository.delete(user);
            clearUserCaches(user);
            learnerProfileService.deleteProfile(user);
            userRepository.flush();
        } catch (Exception e) {
            log.error("Failed to delete user and associated profile", e);
            throw new RuntimeException("User deletion failed", e);
        }
    }
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserCreationService.java (2)

197-204: ⚠️ Potential issue

Fix inconsistent return value and consider method refactoring.

The method has several issues:

  1. Returns user instead of savedUser, potentially losing database-generated values
  2. Method is growing large and handling multiple responsibilities

Fix the return value:

-        return user;
+        return savedUser;

Consider extracting the post-save operations into a separate method to improve readability and maintainability:

private User handlePostSaveOperations(User savedUser, String password, Set<String> groups) {
    optionalVcsUserManagementService.ifPresent(service -> 
        service.createVcsUser(savedUser, password));
    optionalCIUserManagementService.ifPresent(service -> 
        service.createUser(savedUser, password));
    addUserToGroupsInternal(savedUser, groups);
    learnerProfileService.createProfile(savedUser);
    return savedUser;
}

149-150: 🛠️ Refactor suggestion

Consider adding error handling for profile creation.

While the changes maintain functionality, there are a few concerns:

  1. If profile creation fails, the user will be created but left without a profile
  2. No explicit transaction boundary for the two operations

Consider wrapping both operations in a transaction and adding error handling:

-        newUser = saveUser(newUser);
-        learnerProfileService.createProfile(newUser);
+        try {
+            newUser = saveUser(newUser);
+            learnerProfileService.createProfile(newUser);
+        } catch (Exception e) {
+            log.error("Failed to create user profile for user {}: {}", newUser.getLogin(), e.getMessage());
+            throw new RuntimeException("Failed to complete user creation", e);
+        }

Committable suggestion skipped: line range outside the PR's diff.

src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1)

120-123: ⚠️ Potential issue

Consider adding transaction management for data consistency.

The method performs multiple database operations (profile creation and learning path generation) without explicit transaction management. This could lead to inconsistent states if any operation fails.

Consider wrapping the operations in a transaction:

+   @Transactional
    public void enableLearningPathsForCourse(@NotNull Course course) {
        course.setLearningPathsEnabled(true);
        var students = userRepository.getStudentsWithLearnerProfile(course);
        courseLearnerProfileService.createCourseLearnerProfiles(course, students);
        generateLearningPaths(course, students);
        courseRepository.save(course);
        log.debug("Enabled learning paths for course (id={})", course.getId());
    }

Committable suggestion skipped: line range outside the PR's diff.

src/test/java/de/tum/cit/aet/artemis/core/user/util/UserUtilService.java (1)

549-556: 🛠️ Refactor suggestion

Add documentation, validation, and error handling.

The method needs several improvements to meet the test utility standards:

  1. Add JavaDoc documentation to describe the method's purpose, parameters, and behavior
  2. Add validation for the userPrefix parameter
  3. Add error handling for database operations
  4. Add assertions to verify the operation's success

Here's the suggested implementation:

+    /**
+     * Creates and saves learner profiles for all users whose login starts with the given prefix.
+     * This method is used to set up test data for integration tests.
+     *
+     * @param userPrefix The prefix to filter users by (must not be null)
+     * @throws IllegalArgumentException if userPrefix is null
+     */
     public void createLearnerProfilesForUsers(String userPrefix) {
+        assertThat(userPrefix).as("userPrefix must not be null").isNotNull();
+
         Set<LearnerProfile> learnerProfiles = userTestRepository.findAllByUserPrefix(userPrefix).stream().map(user -> {
             LearnerProfile learnerProfile = new LearnerProfile();
             learnerProfile.setUser(user);
             return learnerProfile;
         }).collect(Collectors.toSet());
-        learnerProfileRepository.saveAll(learnerProfiles);
+        Set<LearnerProfile> savedProfiles = learnerProfileRepository.saveAll(learnerProfiles);
+        assertThat(savedProfiles).as("All learner profiles should be saved successfully")
+            .hasSameSizeAs(learnerProfiles);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Creates and saves learner profiles for all users whose login starts with the given prefix.
     * This method is used to set up test data for integration tests.
     *
     * @param userPrefix The prefix to filter users by (must not be null)
     * @throws IllegalArgumentException if userPrefix is null
     */
    public void createLearnerProfilesForUsers(String userPrefix) {
        assertThat(userPrefix).as("userPrefix must not be null").isNotNull();

        Set<LearnerProfile> learnerProfiles = userTestRepository.findAllByUserPrefix(userPrefix).stream().map(user -> {
            LearnerProfile learnerProfile = new LearnerProfile();
            learnerProfile.setUser(user);
            return learnerProfile;
        }).collect(Collectors.toSet());
        Set<LearnerProfile> savedProfiles = learnerProfileRepository.saveAll(learnerProfiles);
        assertThat(savedProfiles).as("All learner profiles should be saved successfully")
            .hasSameSizeAs(learnerProfiles);
    }
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java (1)

471-471: 🛠️ Refactor suggestion

Consider adding error handling for profile deletion.

The profile deletion operation could fail and prevent user deletion. Consider wrapping it in a try-catch block to ensure the user deletion process continues even if profile deletion fails.

-            learnerProfileService.deleteProfile(user);
+            try {
+                learnerProfileService.deleteProfile(user);
+            } catch (Exception e) {
+                log.error("Failed to delete learner profile for user {}: {}", user.getLogin(), e.getMessage());
+                // Continue with user deletion despite profile deletion failure
+            }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

            try {
                learnerProfileService.deleteProfile(user);
            } catch (Exception e) {
                log.error("Failed to delete learner profile for user {}: {}", user.getLogin(), e.getMessage());
                // Continue with user deletion despite profile deletion failure
            }
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathRecommendationService.java (3)

586-591: ⚠️ Potential issue

Correct logical error in exercise selection to prevent negative remainingExercisePoints

The predicate remainingExercisePoints.getAndAdd(-exercise.getMaxPoints()) >= 0 may include exercises even when remainingExercisePoints are insufficient, causing it to become negative. This could lead to incorrect scheduling of exercises.

Apply this diff to fix the predicate logic:

-Predicate<Exercise> exercisePredicate = exercise -> remainingExercisePoints.getAndAdd(-exercise.getMaxPoints()) >= 0;
+Predicate<Exercise> exercisePredicate = exercise -> {
+    double remaining = remainingExercisePoints.get();
+    if (remaining >= exercise.getMaxPoints()) {
+        remainingExercisePoints.addAndGet(-exercise.getMaxPoints());
+        return true;
+    } else {
+        return false;
+    }
+};

Committable suggestion skipped: line range outside the PR's diff.


486-488: ⚠️ Potential issue

Handle potential null values for LearnerProfile to avoid NullPointerException

The code assumes that user.getLearnerProfile() returns a non-null value. If getLearnerProfile() can return null, this will cause a NullPointerException when calling learnerProfile.getCourseLearnerProfiles(). Please add a null check for learnerProfile.

Apply this diff to handle the potential null value:

var learnerProfile = user.getLearnerProfile();
+if (learnerProfile == null) {
+    // Initialize learnerProfile or handle the null case appropriately
+    learnerProfile = new LearnerProfile(user);
+}
var courseLearnerProfile = learnerProfile.getCourseLearnerProfiles().stream().findFirst().orElse(new CourseLearnerProfile());

Committable suggestion skipped: line range outside the PR's diff.


624-635: ⚠️ Potential issue

Handle potential unexpected values in aimForGradeOrBonus

The methods getExerciseOrderComparator and getExerciseSelectionPredicate rely on aimForGradeOrBonus matching specific enum values. Ensure that aimForGradeOrBonus is validated or handle unexpected values to prevent potential bugs.

Apply this diff to include validation:

 private static Predicate<Exercise> getExerciseSelectionPredicate(int aimForGradeOrBonus, AtomicDouble remainingExercisePoints) {
+    if (aimForGradeOrBonus != LOW.getValue() && aimForGradeOrBonus != MEDIUM_HIGH.getValue() && aimForGradeOrBonus != HIGH.getValue()) {
+        throw new IllegalArgumentException("Invalid aimForGradeOrBonus value: " + aimForGradeOrBonus);
+    }
     Predicate<Exercise> exercisePredicate = exercise -> {
         // existing logic
     };
     // rest of the method
 }

Committable suggestion skipped: line range outside the PR's diff.

src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (2)

1072-1074: ⚠️ Potential issue

Add missing check for course.getLearningPathsEnabled() in removeUserFromGroup

Currently, the method deletes the course learner profile without verifying if learning paths are enabled for the course. This could lead to unintended behavior or errors when learning paths are not used.

Apply this diff to include the check:

 if (group.equals(course.getStudentGroupName())) {
+    if (course.getLearningPathsEnabled()) {
         courseLearnerProfileService.deleteCourseLearnerProfile(course, user);
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        if (group.equals(course.getStudentGroupName())) {
            if (course.getLearningPathsEnabled()) {
                courseLearnerProfileService.deleteCourseLearnerProfile(course, user);
            }
        }

674-674: ⚠️ Potential issue

Verify if learning paths are enabled before deleting learner profiles in unenrollUserForCourseOrThrow

When unenrolling a user, the method deletes the course learner profile without checking if learning paths are enabled. This may cause issues if learning paths are not enabled for the course.

Consider adding the following check:

+if (course.getLearningPathsEnabled()) {
    courseLearnerProfileService.deleteCourseLearnerProfile(course, user);
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        if (course.getLearningPathsEnabled()) {
            courseLearnerProfileService.deleteCourseLearnerProfile(course, user);
        }
src/main/java/de/tum/cit/aet/artemis/atlas/repository/LearningPathRepository.java (2)

77-77: ⚠️ Potential issue

Fix WHERE clause to correctly filter by LearningPath ID

The query filters by lp.id = :learningPathId, which uses the LearnerProfile ID. However, the parameter learningPathId represents the ID of the LearningPath. This mismatch will result in incorrect data retrieval. Update the WHERE clause to filter by l.id = :learningPathId.

Apply this diff to correct the WHERE clause:

-            WHERE lp.id = :learningPathId
+            WHERE l.id = :learningPathId
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

            WHERE l.id = :learningPathId

76-76: ⚠️ Potential issue

Invalid use of LEFT JOIN FETCH with ON clause in JPQL

JPQL does not support ON clauses with JOIN FETCH. The line LEFT JOIN FETCH lp.courseLearnerProfiles clp ON clp.course.id = l.course.id is invalid and will cause a runtime exception. Consider removing the FETCH keyword or restructuring the query.

Apply this diff to correct the query:

-            LEFT JOIN FETCH lp.courseLearnerProfiles clp ON clp.course.id = l.course.id
+            LEFT JOIN lp.courseLearnerProfiles clp
+            WHERE clp.course.id = l.course.id

Adjust the query logic to ensure the necessary associations are fetched appropriately.

Committable suggestion skipped: line range outside the PR's diff.

@github-actions github-actions bot removed tests database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. programming Pull requests that affect the corresponding module labels Nov 7, 2024
…/adaptive-learning/learner-profile-graded-exercises

# Conflicts:
#	src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java
@JohannesStoehr JohannesStoehr marked this pull request as draft November 11, 2024 15:15
@github-actions github-actions bot added the tests label Nov 11, 2024
Copy link
Contributor

@EneaGore EneaGore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code lgtm for this Pr, on TS5 the same issue which isnt caused by this PR as mentioned above, the exercises loads if you press the compentency drop down though, also the exercises do not start with a server error.

…/adaptive-learning/learner-profile-graded-exercises

# Conflicts:
#	src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
#	src/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.java
EneaGore
EneaGore approved these changes Nov 28, 2024
Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS5, everything works fine. The issue mentioned above is not related to this PR.

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapprove

Copy link

@kevinfischer4 kevinfischer4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS4. Everything worked fine for me. (The issue mentioned above doesn't belong to this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atlas Pull requests that affect the corresponding module core Pull requests that affect the corresponding module lock:artemis-test4 ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Work In Progress
Development

Successfully merging this pull request may close these issues.

8 participants