Adaptive learning: Add LearnerProfile

[N0W0RK]

Warning

This PR includes a migration! Do not deploy on testservers

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

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 pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
• I documented the Java code using JavaDoc style.

Motivation and Context

Based on #9448

Description

Added LearnerProfile and CourseLearnerProfile. Each user has a LearnerProfile and each user in an active (current semester and not a test course) course with learning paths has automatically a course learner profile.
The CourseLearnerProfile is also created when a user enrolls in a course with learning paths or learning paths get enabled. The profiles are deleted if the user unenrolls or is deleted.

This PR does not add any UI!

Steps for Testing

🚨 🚨 Local setup to verify database entries 🚨 🚨
Course with enabled learning paths and some students

1. Log in to Artemis
2. Check that the migration works and all existing users have a corresponding entry in the learner profile table
3. Check that the migration works and all existing users in the course with learning paths have a corresponding entry in the course learner profile table
4. Create a new user and check that they also get a new learner profile in the data base
5. Delete the course and check that the corresponding course learner profiles are deleted as well
6. Delete a user and check that the corresponding learner profile course learner profiles are deleted as well

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

Performance Tests

• Test 1
• Test 2

Test Coverage

Summary by CodeRabbit

• New Features

  • Introduced CourseLearnerProfile and LearnerProfile classes for enhanced profile management.
  • Added functionality to create and delete course learner profiles associated with users.
  • Updated user management to include learner profile creation during user registration and deletion.
  • Enhanced course management to automatically create learner profiles when users are added to courses.
  • Added methods for retrieving users with their associated learner profiles.
  • Implemented a utility service for creating learner profiles for multiple users in tests.

• Bug Fixes

  • Corrected method signatures and ensured proper integration of learner profiles in user and course management.

• Tests

  • Updated integration tests to validate the presence of learner profiles during user and course operations.

[coderabbitai]

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (3)

src/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.java (1)

13-18: Consider using constructor injection instead of field injection

While the code works, constructor injection is preferred over field injection as it:

• Makes dependencies explicit
• Ensures required dependencies are provided
• Facilitates easier testing

 @Service
 public class LearnerProfileUtilService {
-    @Autowired
     private UserTestRepository userTestRepository;
+
+    public LearnerProfileUtilService(UserTestRepository userTestRepository) {
+        this.userTestRepository = userTestRepository;
+    }

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

215-216: Remove unused field.

The faqRepository field appears to be unused in the provided code. Consider removing it if it's not needed.

---

1051-1061: Consider fetching course with competencies earlier.

The course is fetched with competencies only after checking if learning paths are enabled. Consider moving the fetch earlier to avoid potential redundant database calls if the course is needed elsewhere.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 59a65e3 and f824403.

⛔ Files ignored due to path filters (1)

• src/main/resources/config/liquibase/changelog/20241107130000_changelog.xml is excluded by !**/*.xml

📒 Files selected for processing (6)

• src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (9 hunks)
• src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (2 hunks)
• src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (2 hunks)
• src/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.java (1 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)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
• src/test/java/de/tum/cit/aet/artemis/programming/CourseGitlabJenkinsIntegrationTest.java

🧰 Additional context used

📓 Path-based instructions (4)

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/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/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

🔇 Additional comments (9)

src/test/java/de/tum/cit/aet/artemis/atlas/profile/util/LearnerProfileUtilService.java (2)

1-12: LGTM: Package structure and imports are well-organized

The class is appropriately placed in the test utility package, and imports are properly organized.

---

19-26: Add DB query count tracking as per guidelines

According to the coding guidelines, test classes should track database query counts for performance monitoring.

Consider adding:

1. @SqlTestUtil.CountQueries annotation
2. Assertions for the expected number of queries

src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (2)

12-12: LGTM! Import statement follows the established pattern.

The import is correctly placed within the atlas-specific imports section, maintaining the existing organization.

---

149-151: LGTM! Field declaration follows the established patterns.

The learnerProfileUtilService field is:

• Correctly placed in the Util Services section
• Properly annotated with @Autowired
• Consistent with other utility service declarations

Please ensure that all test classes extending AbstractAtlasIntegrationTest properly handle this new dependency in their test scenarios.

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

11-11: LGTM! Changes follow established patterns.

The addition of LearnerProfileUtilService is consistent with the existing structure:

• Import follows alphabetical order
• Field is properly placed in the "External Util Services" section
• Follows the same autowiring pattern as other utility services

Also applies to: 211-212

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

217-217: LGTM: Clean dependency injection implementation.

The CourseLearnerProfileService integration follows proper constructor injection pattern and maintains the class's single responsibility principle.

Also applies to: 232-233, 277-277

---

512-512: LGTM: Proper cleanup of learner profiles.

The deletion of course learner profiles is correctly handled during course deletion.

---

623-625: Refactor duplicated learner profile creation logic.

The code for creating learner profiles and generating learning paths is duplicated. Consider extracting this into a helper method as suggested in previous reviews.

Also applies to: 657-660

---

674-674: LGTM: Proper cleanup during unenrollment.

The deletion of course learner profile during user unenrollment is correctly implemented.

[AjayvirS]

Tested with local setup, CRUD works!

After creating learning path through client:

After deleting course through client:

[az108]

Code 👍

[EneaGore]

code lgtm.

[florian-glombik]

Why do we have an empty constructor for new CourseLearnerProfile(); - I do not think that this makes sense and can only lead to wrong usage of the class 🤔

[florian-glombik]

Approve code

I had a look at the migration but did not write many changelogs recently, so my review is unfortunately not the most relevant on the changelog 😅

[florian-glombik]

Re-approve code after merge

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

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

133-136: Add detailed logging for better observability.

Consider adding more detailed logging to track the number of students for which learning paths will be generated.

    public void generateLearningPaths(@NotNull Course course) {
+       log.debug("Fetching students with learner profiles for course (id={})", course.getId());
        Set<User> students = userRepository.getStudentsWithLearnerProfile(course);
+       log.debug("Found {} students with learner profiles for course (id={})", students.size(), course.getId());
        generateLearningPaths(course, students);
    }

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

416-417: Enhance assertion specificity for learner profile check.

While the assertion correctly verifies the existence of a learner profile, it could be more specific about what it's checking.

Consider enhancing the assertion to verify specific properties of the learner profile:

-        assertThat(learnerProfileRepository.findByUser(student)).isNotEmpty();
+        var learnerProfile = learnerProfileRepository.findByUser(student);
+        assertThat(learnerProfile)
+            .isPresent()
+            .hasValueSatisfying(profile -> {
+                assertThat(profile.getUser()).isEqualTo(student);
+                // Add more specific assertions about the learner profile properties
+            });

src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java (1)

Line range hint 127-142: Refactor constructor to improve maintainability

The UserService constructor has a large number of parameters (16), which can affect readability and maintainability. Consider refactoring by grouping related services into composite objects, using a builder pattern, or leveraging a dependency injection framework to manage dependencies more effectively.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f824403 and 389f18b.

⛔ Files ignored due to path filters (1)

• src/main/resources/config/liquibase/master.xml is excluded by !**/*.xml

📒 Files selected for processing (4)

• src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (5 hunks)
• src/main/java/de/tum/cit/aet/artemis/core/domain/User.java (4 hunks)
• src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java (4 hunks)
• src/test/java/de/tum/cit/aet/artemis/core/user/util/UserTestService.java (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main/java/de/tum/cit/aet/artemis/core/domain/User.java

🧰 Additional context used

📓 Path-based instructions (3)

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/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

🔇 Additional comments (5)

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

Line range hint 94-111: LGTM! Constructor injection properly implemented.

The addition of CourseLearnerProfileService follows Spring best practices with constructor injection and final field declaration.

---

120-123: ⚠️ Potential issue

Add transaction management for database operations.

The method performs multiple database operations (enabling paths, creating profiles, generating paths) without transaction management. This could lead to inconsistent state if any operation fails.

Apply this diff to add transaction management:

+   @Transactional
    public void enableLearningPathsForCourse(@NotNull Course course) {
        course.setLearningPathsEnabled(true);
+       try {
            Set<User> students = userRepository.getStudentsWithLearnerProfile(course);
            courseLearnerProfileService.createCourseLearnerProfiles(course, students);
            generateLearningPaths(course, students);
            courseRepository.save(course);
            log.debug("Enabled learning paths for course (id={})", course.getId());
+       } catch (Exception e) {
+           log.error("Failed to enable learning paths for course (id={})", course.getId(), e);
+           throw new RuntimeException("Failed to enable learning paths", e);
+       }
    }

---

138-144: ⚠️ Potential issue

Add input validation for students parameter.

The method assumes that the provided students belong to the course and have learner profiles, but doesn't validate these assumptions.

Apply this diff to add validation:

    public void generateLearningPaths(@NotNull Course course, Set<User> students) {
+       if (students == null || students.isEmpty()) {
+           log.warn("No students provided for learning path generation in course (id={})", course.getId());
+           return;
+       }
+       // Validate that all students belong to the course and have profiles
+       var invalidStudents = students.stream()
+           .filter(student -> !student.getGroups().contains(course.getStudentGroupName()))
+           .collect(Collectors.toSet());
+       if (!invalidStudents.isEmpty()) {
+           throw new IllegalArgumentException("Some students are not enrolled in the course");
+       }
        students.forEach(student -> generateLearningPathForUser(course, student));
        log.debug("Successfully created learning paths for all {} students in course (id={})", students.size(), course.getId());
    }

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

114-128: LGTM: Repository dependencies properly declared.

The new repository dependencies are correctly autowired and follow the project's naming conventions.

src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java (1)

476-476: Add error handling when deleting the learner profile

To ensure that the user deletion process continues even if deleting the learner profile fails, consider wrapping the learnerProfileService.deleteProfile(user) call in a try-catch block and logging any exceptions.

Apply this diff to implement the error handling:

- 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
+ }

[AjayvirS]

re-approve trivial changes

[EneaGore]

re-approve after merge conflicts