Learning paths: Add explanation view for learning path users
#9392
Conversation
WalkthroughThe changes enhance the learning path management system by introducing user-specific methods in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
tests
server
JohannesWt
changed the title
Learning paths: Add explanation view for learning path users
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20240924125742_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (12)
- src/main/java/de/tum/cit/aet/artemis/atlas/domain/competency/LearningPath.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearningPathDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java (4 hunks)
- src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.html (2 hunks)
- src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (2 hunks)
- src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (1 hunks)
- src/main/webapp/app/course/learning-paths/services/learning-path-api.service.ts (3 hunks)
- src/main/webapp/app/entities/competency/learning-path.model.ts (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (26 hunks)
- src/test/javascript/spec/component/learning-paths/pages/learning-path-student-page.component.spec.ts (6 hunks)
- src/test/javascript/spec/service/learning-path/learning-path-api.service.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
src/main/java/de/tum/cit/aet/artemis/atlas/domain/competency/LearningPath.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_importssrc/main/java/de/tum/cit/aet/artemis/atlas/dto/LearningPathDTO.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_importssrc/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_importssrc/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_importssrc/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.html (1)
Pattern
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (1)
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (1)
src/main/webapp/app/course/learning-paths/services/learning-path-api.service.ts (1)
src/main/webapp/app/entities/competency/learning-path.model.ts (1)
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: truesrc/test/javascript/spec/component/learning-paths/pages/learning-path-student-page.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}src/test/javascript/spec/service/learning-path/learning-path-api.service.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🪛 Biome
src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts
[error] 42-42: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 42-42: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 72-72: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 73-73: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🪛 GitHub Check: client-compilation
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
[failure] 153-153:
Property 'Patch' does not exist on type 'typeof HttpMethod'.
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
[failure] 153-153:
Property 'Patch' does not exist on type 'typeof HttpMethod'.
🪛 GitHub Check: client-tests
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
[failure] 153-153:
Property 'Patch' does not exist on type 'typeof HttpMethod'.
🔇 Additional comments (38)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/LearningPathDTO.java (4)
1-6: LGTM: Package declaration and imports are well-structured.The package declaration follows the CamelCase naming convention, and imports are specific without using wildcard imports, adhering to the coding guidelines.
7-7: LGTM: Appropriate use of JsonInclude annotation.The @JsonInclude(JsonInclude.Include.NON_EMPTY) annotation ensures that only non-empty values are included in JSON serialization, adhering to the principle of minimal DTOs.
8-8: LGTM: Well-defined LearningPathDTO record.The LearningPathDTO record adheres to the coding guidelines:
- Uses Java records as recommended for DTOs.
- Follows CamelCase naming convention.
- Has a single responsibility of representing a LearningPath for data transfer.
- Uses primitive types as preferred.
- Appropriate access level (public) for a DTO.
10-12: LGTM: Well-implemented static factory method.The 'of' method adheres to the coding guidelines:
- Follows camelCase naming convention.
- Provides a clear way to create a DTO from the domain object.
- Uses static member reference as recommended.
- Follows the principle of minimal DTOs by transferring only necessary data.
src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.html (4)
2-2: LGTM! Correct usage of new Angular syntax.The change from
*ngIfto@ifaligns with the coding guidelines for .html files. The function name update fromisLearningPathIdLoading()toisLearningPathLoading()is consistent with the AI-generated summary and suggests a broader context for the loading state.
30-30: LGTM! Updated click event handler.The change from
generateLearningPath(this.courseId())tostartLearningPath()is consistent with the AI-generated summary. This update suggests a shift in functionality from generating to starting a learning path.Please verify the implementation of the new
startLearningPath()method in the component file. Ensure that it correctly handles the learning path initiation without requiring thecourseIdparameter.#!/bin/bash # Description: Check the implementation of startLearningPath() method # Test: Search for the startLearningPath method definition. Expect: Method implementation details. ast-grep --lang typescript --pattern 'startLearningPath() { $$$ }' src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts
33-33: LGTM! Updated button id.The change from
generate-learning-path-buttontostart-learning-path-buttonis consistent with the AI-generated summary and aligns with the updated functionality of starting rather than generating a learning path.Please verify that any associated test cases or selectors using this button id have been updated accordingly. Run the following script to check for any remaining occurrences of the old id:
#!/bin/bash # Description: Check for occurrences of the old button id # Test: Search for the old button id. Expect: No occurrences of 'generate-learning-path-button'. rg 'generate-learning-path-button'
8-9: LGTM! Correct usage of new Angular syntax and updated logic.The changes align with the coding guidelines and the AI-generated summary:
- Use of
@else ifinstead of*ngIfis correct.- The condition now checks
learningPath()and itsstartedByStudentproperty, which is consistent with the summary.learningPath()!.idis used instead oflearningPathId()!.Please verify the use of the non-null assertion operator (!) in
learningPath()!.id. Ensure thatlearningPath()cannot be null or undefined at this point to avoid potential runtime errors.src/main/java/de/tum/cit/aet/artemis/atlas/domain/competency/LearningPath.java (2)
98-104: LGTM! Getter and setter methods are well-implemented.The
isStartedByStudent()getter andsetStartedByStudent(boolean)setter methods are correctly implemented. They follow proper naming conventions for boolean properties and adhere to the single responsibility principle.
Line range hint
1-109: Overall, the changes look good and effectively implement the new feature.The introduction of the
startedByStudentfield, along with its getter and setter methods, and the update to thetoString()method, have been implemented correctly. These changes adhere to Java best practices and the existing code structure.A few minor suggestions have been made to further improve the code:
- Adding the
@NotNullannotation to thestartedByStudentfield for clarity.- Improving the readability of the
toString()method by breaking it into multiple lines.These changes effectively enhance the
LearningPathclass to track whether a student has started a learning path, which aligns with the PR objectives of introducing an explanation view related to learning paths.src/main/webapp/app/entities/competency/learning-path.model.ts (2)
Line range hint
1-40: Summary: New LearningPathDTO interface added successfully.The addition of the
LearningPathDTOinterface enhances the data structure related to learning paths. It complements existing interfaces and classes without disrupting the overall structure of the file. The new interface follows the established coding guidelines and naming conventions.To ensure the effectiveness of this change:
- Verify the usage of
LearningPathDTOacross the codebase as suggested in the previous comment.- Consider adding unit tests for any components or services that will use this new DTO.
- Update relevant documentation to reflect the addition of this new interface and its purpose in the learning path feature.
35-39: Verify usage of LearningPathDTO in the codebase.The new
LearningPathDTOinterface looks good and complements existing interfaces related to learning paths. To ensure its effectiveness and prevent potential dead code, let's verify its usage across the codebase.Run the following script to check for the usage of
LearningPathDTO:This script will help us confirm that
LearningPathDTOis being used in the codebase and identify any pending tasks related to its implementation.✅ Verification successful
LearningPathDTO is properly utilized across the codebase.
The
LearningPathDTOinterface is actively used in multiple TypeScript files, including services and components, ensuring it is integral to the current implementation. No TODO comments related toLearningPathDTOwere found, indicating no pending tasks or issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of LearningPathDTO in the codebase. # Test: Search for LearningPathDTO usage echo "Searching for LearningPathDTO usage:" rg --type typescript -g '!*.spec.ts' 'LearningPathDTO' # Test: Check for any TODO comments related to LearningPathDTO echo "Checking for TODO comments related to LearningPathDTO:" rg --type typescript -g '!*.spec.ts' 'TODO.*LearningPathDTO'Length of output: 389
Script:
#!/bin/bash # Description: Verify the usage of LearningPathDTO in the codebase. # Test: Search for LearningPathDTO usage in TypeScript files echo "Searching for LearningPathDTO usage in .ts and .tsx files:" rg -g '*.ts' -g '*.tsx' 'LearningPathDTO' # Test: Check for any TODO comments related to LearningPathDTO in TypeScript files echo "Checking for TODO comments related to LearningPathDTO in .ts and .tsx files:" rg -g '*.ts' -g '*.tsx' 'TODO.*LearningPathDTO'Length of output: 7873
src/test/javascript/spec/service/learning-path/learning-path-api.service.spec.ts (1)
Line range hint
1-105: Overall, good changes with room for improvement.The modifications to the test cases accurately reflect the changes made to the LearningPathApiService. The tests consistently use Jest and HttpTestingController, adhering to the provided coding guidelines.
For future improvements:
- Enhance test specificity by verifying response bodies and resolves/rejects behavior.
- Add error handling scenarios for each API call.
- Consider testing with specific inputs and outputs where applicable.
- Ensure full coverage of all possible scenarios for each method.
These enhancements will lead to a more robust and comprehensive test suite, improving the overall quality and reliability of the LearningPathApiService.
src/test/javascript/spec/component/learning-paths/pages/learning-path-student-page.component.spec.ts (9)
16-16: LGTM: Import statement for LearningPathDTO is correct.The import statement for LearningPathDTO is correctly added and follows the guideline of avoiding full module imports.
24-28: LGTM: LearningPathDTO mock object is well-defined.The learningPath constant is correctly defined as a LearningPathDTO object with the necessary properties for testing. This mock object will be useful for various test cases.
50-57: LGTM: TestBed configuration updated correctly.The LearningPathApiService mock in the TestBed configuration has been properly updated to include the necessary methods for working with LearningPathDTO. The use of jest.fn() for mocking is correct and follows the testing guidelines.
86-93: LGTM: "should get learning path" test case updated correctly.The test case has been properly updated to use the new getLearningPathForCurrentUser method and correctly expects the component's learningPath to equal the mock learningPath object. The use of toEqual for object comparison is appropriate and follows the expectation_specificity guideline.
Line range hint
96-111: LGTM: "should show navigation when learning path has been started" test case is well-structured.The test case has been updated to correctly set up a scenario where the learning path has been started. The use of mockResolvedValueOnce for the API call and the expectation to check for the presence of the navigation component are both appropriate.
124-125: LGTM: "should set isLoading correctly during learning path load" test case updated appropriately.The test case has been correctly updated to use the new getLearningPathForCurrentUser method. The use of a spy on the isLearningPathLoading setter is a good approach to verify the loading state changes. The expectations use specific matchers, following the expectation_specificity guideline.
134-147: LGTM: "should generate learning path on start when not found" test case is comprehensive and well-structured.The test case effectively simulates the scenario where a learning path needs to be generated when not found. The mocking of different API calls and the expectations are all appropriate and follow the expectation_specificity guideline. The test thoroughly checks the behavior of generating and starting a new learning path.
Line range hint
149-169: LGTM: Both "should set learning path to started" and "should set isLoading correctly during learning path start" test cases are well-implemented.These test cases effectively verify the behavior of starting a learning path and the corresponding loading state changes. The expectations use appropriate matchers and follow the expectation_specificity guideline. The tests provide good coverage for the learning path start functionality.
Line range hint
1-169: Overall, the changes to the LearningPathStudentPageComponent test file are well-implemented and improve test coverage.The updates consistently use the new LearningPathDTO structure and the revised API methods. All test cases have been appropriately modified to reflect these changes, maintaining good test coverage and adhering to the provided coding guidelines and best practices. The use of Jest and NgMocks is consistent throughout the file, and the expectations follow the specificity guidelines. These changes should effectively test the updated LearningPathStudentPageComponent functionality.
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (10)
39-39: LGTM: New import added for LearningPathDTOThe addition of the import statement for
LearningPathDTOis consistent with the changes made in the test methods. This import is necessary for the proper functioning of the updated tests.
125-127: LGTM: Constants updated for multiple student usersThe renaming of
STUDENT_OF_COURSEtoSTUDENT1_OF_COURSEand the addition ofSTUDENT2_OF_COURSEimprove clarity and support test scenarios involving multiple student users. These changes are consistent with the evolving requirements of the test suite.
164-164: LGTM: Updated student variable initializationThe change in the
setupTestScenariomethod to useSTUDENT1_OF_COURSEinstead ofSTUDENT_OF_COURSEmaintains consistency with the earlier constant renaming. This update ensures that the correct student constant is used for initialization in the setup method.
579-588: Updated GetLearningPath test classThe changes in this nested test class align with the earlier modifications to the API:
- The class and method names have been updated to reflect the focus on the entire LearningPath object rather than just the ID.
- The API endpoint has been changed to
/api/courses/{courseId}/learning-path/me, consistent with earlier changes.- The expected return type is now
LearningPathDTO.class, which is appropriate for the updated endpoint.The assertion logic has been updated to compare the result with
LearningPathDTO.of(learningPath), which is correct for the new return type.
592-599: LGTM: Updated test for non-existent learning pathThe changes in this test method are consistent with the earlier modifications to the API:
- The API endpoint has been updated to
/api/courses/{courseId}/learning-path/me.- The expected return type is now
LearningPathDTO.class, which aligns with the new endpoint behavior.These changes maintain consistency with the updated API design and improve the test coverage for the scenario where a learning path doesn't exist.
607-628: Updated GenerateLearningPath test classThe changes in this nested test class align with the earlier modifications to the API and improve RESTful adherence:
- The API endpoint has been updated to
/api/courses/{courseId}/learning-path.- The expected return type is now
LearningPathDTO.class, consistent with earlier changes.- The HTTP status for successful creation has been changed to CREATED (201), which is more appropriate for resource creation operations.
The assertion logic has been updated to check for a non-null response and verify the creation of a learning path in the repository, which is correct for the new behavior.
632-667: LGTM: New StartLearningPath test class addedThe addition of the
StartLearningPathnested class improves test coverage for the learning path starting functionality:
- It includes tests for different scenarios: attempting to start another user's learning path, trying to start an already started path, and successfully starting a path.
- The use of
STUDENT1_OF_COURSEandSTUDENT2_OF_COURSEis consistent with earlier changes.- The tests appropriately check for FORBIDDEN and BAD_REQUEST status codes in error scenarios.
- The successful case verifies both the HTTP status and the updated state of the learning path.
The API endpoint
/api/learning-path/{id}/startis used consistently across these tests.
673-673: LGTM: Consistent updates across test methodsThe changes in these test methods maintain consistency with earlier modifications:
- The use of
STUDENT1_OF_COURSEinstead ofSTUDENT_OF_COURSEaligns with the constant renaming.- Updates to API endpoints to use
/meare consistent with the new API design.These changes ensure that all test methods in the class adhere to the updated naming conventions and API structure.
Also applies to: 679-679, 691-691, 706-706, 731-731, 779-779, 802-802, 825-825, 835-835, 852-852, 886-886, 897-897
Line range hint
1-916: LGTM: Comprehensive updates to LearningPathIntegrationTestThe changes made to this test file are comprehensive and consistent:
- Constant renaming improves clarity for multiple student scenarios.
- API endpoint updates reflect changes in the application's design.
- New test classes and methods enhance coverage for learning path functionalities.
- Existing tests have been updated to maintain consistency with the new design.
These modifications align well with the provided AI-generated summary and significantly improve the test suite.
To ensure all changes work as expected, please run the entire test suite and verify that all tests pass successfully.
#!/bin/bash # Run the test suite for the LearningPathIntegrationTest ./gradlew test --tests "de.tum.cit.aet.artemis.atlas.learningpath.LearningPathIntegrationTest"
222-225: Updated test method for student accessThe changes in this test method are mostly consistent with earlier modifications:
- The mock user has been updated to use
STUDENT1_OF_COURSE, which is correct.- The API endpoint has been changed to
/api/courses/{courseId}/learning-path/me, reflecting an update in the API design.However, there's a change in the expected HTTP status from FORBIDDEN to BAD_REQUEST.
Please verify if this change in expected status is intentional and aligns with the current error handling logic for this endpoint.
src/main/webapp/app/course/learning-paths/services/learning-path-api.service.ts (4)
6-6: Import statement includes necessary DTOThe addition of
LearningPathDTOto the import statement is appropriate and necessary for the new methods.
18-20: New methodgetLearningPathForCurrentUseris well-implementedThe method correctly retrieves the learning path for the current user and adheres to the coding guidelines.
22-23: New methodstartLearningPathForCurrentUseris well-implementedThe method correctly initiates the learning path for the current user and follows the coding standards.
43-44: New methodgenerateLearningPathForCurrentUseris correctly implementedThe method properly generates a learning path for the current user and complies with the coding guidelines.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java (2)
20-20: Import of PatchMapping is appropriateThe import of
PatchMappingis necessary for the new PATCH endpoint.
32-32: Import of LearningPathDTO is necessaryThe import of
LearningPathDTOis required for returning learning path data.
src/main/java/de/tum/cit/aet/artemis/atlas/domain/competency/LearningPath.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/domain/competency/LearningPath.java
Show resolved
Hide resolved
src/test/javascript/spec/service/learning-path/learning-path-api.service.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/service/learning-path/learning-path-api.service.spec.ts
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/main/webapp/app/admin/metrics/metrics.model.ts (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (26 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/admin/metrics/metrics.model.ts (1)
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
🔇 Additional comments (12)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (12)
39-39: LGTM: Import statement added correctly.The new import statement for
LearningPathDTOis appropriate and likely necessary for the updated test methods.
125-127: LGTM: Constants updated for clarity and consistency.The renaming of
STUDENT_OF_COURSEtoSTUDENT1_OF_COURSEand the addition ofSTUDENT2_OF_COURSEimprove clarity and suggest that some tests now involve multiple students. The naming convention remains consistent with other constants in the file.
164-164: LGTM: Test setup updated correctly.The change in the
setupTestScenariomethod correctly uses the renamedSTUDENT1_OF_COURSEconstant, maintaining consistency with the earlier constant renaming.
579-588: LGTM: GetLearningPath test class updated correctly.The changes in the
GetLearningPathnested class are consistent with previous updates:
- The method renaming from
shouldReturnExistingIdtoshouldReturnExistingreflects a more general test case.- The
@WithMockUserannotation correctly uses the renamedSTUDENT1_OF_COURSEconstant.- The test method body has been updated to use the new
learning-path/meendpoint, maintaining consistency with the API changes.These modifications improve the test clarity and align with the observed API changes.
592-599: LGTM: Test method updated consistently.The changes in the
shouldReturnNotFoundIfNotExistsmethod are consistent with previous updates:
- The
@WithMockUserannotation correctly uses the renamedSTUDENT1_OF_COURSEconstant.- The test method body has been updated to use the new
learning-path/meendpoint.These modifications maintain consistency throughout the test class and align with the observed API changes.
Line range hint
691-701: LGTM: Test method updated consistently.The changes in the
testGetLearningPathNavigationmethod are appropriate:
- The
@WithMockUserannotation correctly uses the renamedSTUDENT1_OF_COURSEconstant, maintaining consistency with other tests.- The use of
learningPath.getId()instead oflearningPathIdsuggests that thelearningPathobject is now directly available in the test method, which can improve test readability and reduce the chance of errors.These modifications align with the overall updates in the test class and potentially improve the test's clarity.
Line range hint
706-730: LGTM: Test method updated consistently.The changes in the
testGetLearningPathNavigationEmptyCompetenciesmethod are appropriate and consistent with previous updates:
- The
@WithMockUserannotation correctly uses the renamedSTUDENT1_OF_COURSEconstant.- The method body has been updated to use
learningPath.getId()instead oflearningPathId, which is consistent with the changes in other test methods.These modifications maintain consistency throughout the test class and potentially improve the test's clarity by directly using the
learningPathobject.
Line range hint
731-889: LGTM: Consistent updates across multiple test methods.The changes across the remaining test methods demonstrate a consistent approach to updating the test class:
- All
@WithMockUserannotations now correctly use the renamedSTUDENT1_OF_COURSEconstant.- All methods have been updated to use
learningPath.getId()instead oflearningPathId.These systematic changes improve the overall consistency of the test class and align with the modifications observed in earlier methods. The use of
learningPath.getId()potentially enhances test readability and reduces the chance of errors by directly using thelearningPathobject.
897-897: LGTM: Helper method updated correctly.The change in the
createAndLinkTextExercisemethod correctly uses the renamedSTUDENT1_OF_COURSEconstant, maintaining consistency with the earlier constant renaming throughout the test class.
607-626: LGTM: GenerateLearningPath test class updated consistently.The changes in the
GenerateLearningPathnested class are consistent with previous updates:
- All
@WithMockUserannotations correctly use the renamedSTUDENT1_OF_COURSEconstant.- All test methods have been updated to use the new
learning-pathendpoint without an ID.These modifications maintain consistency throughout the test class and reflect the API changes for generating learning paths.
Please verify that the removal of the ID from the learning path generation endpoint aligns with the intended API design:
#!/bin/bash # Verify the new API endpoint usage for generating learning paths rg --type java 'courses/\$\{course\.getId\(\)\}/learning-path'
632-667: LGTM: New StartLearningPath test class added.The new
StartLearningPathnested class is a valuable addition to the test suite:
- It covers the functionality of starting a learning path, which appears to be a new feature.
- The test methods appropriately cover different scenarios:
- Attempting to start another user's path (expected: forbidden)
- Attempting to start an already started path (expected: bad request)
- Successfully starting a path
- The tests use appropriate assertions and HTTP status codes.
This addition improves the test coverage for the learning path functionality.
Please verify that the new start learning path functionality is correctly implemented in the corresponding service or controller:
✅ Verification successful
Verification Successful: Start Learning Path functionality is implemented and tested correctly.
The implemented endpoint
PATCH learning-path/{learningPathId}/startis present inLearningPathResource.java, and the corresponding test cases inLearningPathIntegrationTest.javaadequately cover the expected scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of the start learning path functionality rg --type java 'learning-path/.*?/start'Length of output: 950
Line range hint
222-226: LGTM: Test method updated to reflect API changes.The test method has been correctly updated to use the renamed
STUDENT1_OF_COURSEconstant and a new API endpoint. The change fromlearning-path-idtolearning-path/mesuggests a more RESTful approach.Please verify that these changes align with the intended API modifications:
✅ Verification successful
LGTM: Test method correctly references the existing API endpoint.
The
testAll_asStudentmethod appropriately uses theSTUDENT1_OF_COURSEconstant and the/api/courses/{courseId}/learning-path/meendpoint as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new API endpoint usage rg --type java 'learning-path/me'Length of output: 64
Script:
#!/bin/bash # Search for all occurrences of 'learning-path' in Java files rg --type java 'learning-path'Length of output: 16902
🧰 Tools
🪛 GitHub Check: Junit Results
[failure] 223-223: testAll_asStudent()
Junit test testAll_asStudent() failed java.lang.AssertionError: Response status expected:<400> but was:<405>
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
Show resolved
Hide resolved
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
Outdated
Show resolved
Hide resolved
...urse/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
Line range hint
1-924: Overall LGTM: Comprehensive updates to LearningPathIntegrationTestThe changes made to
LearningPathIntegrationTest.javaare comprehensive and improve the overall quality of the test suite:
- Constants have been renamed and added (
STUDENT1_OF_COURSE,STUDENT2_OF_COURSE) to support multiple student scenarios.- API endpoints have been consistently updated across all test methods, moving towards a more RESTful and user-centric approach (e.g.,
/learning-path/me).- A new nested test class
StartLearningPathhas been added to cover the functionality of starting a learning path.- All test methods have been updated to use the new constants and endpoints, maintaining consistency throughout the file.
These changes enhance the clarity, consistency, and coverage of the learning path integration tests. The new tests for starting a learning path will help ensure the robustness of this feature.
Consider extracting common setup code and utility methods to a base test class or helper methods to reduce duplication and improve maintainability of the test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (3 hunks)
- src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (2 hunks)
- src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (26 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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_importssrc/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (1)
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (1)
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
🪛 Biome
src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts
[error] 42-42: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 42-42: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 72-72: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 73-73: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (16)
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (3)
76-76: LGTM! Documentation improvement.The change in the return statement documentation improves clarity and aligns with the coding guidelines.
105-105: LGTM! Consistent documentation improvement.The change in the return statement documentation for the
postmethod is consistent with the improvement made to thegetmethod and aligns with the coding guidelines.
126-154:⚠️ Potential issueLGTM! Well-implemented
patchmethod with a potential enum issue.The new
patchmethod is well-implemented and consistent with the existing code structure. It adheres to the Angular style guide and coding guidelines, using generics, async/await, and maintaining consistent error handling.However, there's a potential issue with
HttpMethod.Patch:The
HttpMethod.Patchenum value might not exist in the currentHttpMethodtype. To resolve this, update theHttpMethodenum in thesrc/main/webapp/app/admin/metrics/metrics.model.tsfile as follows:export enum HttpMethod { Get = 'GET', Post = 'POST', Put = 'PUT', Delete = 'DELETE', Options = 'OPTIONS', Head = 'HEAD', Patch = 'PATCH' // Add this line }Run the following script to verify the
HttpMethodenum:After making this change, the
patchmethod should work correctly without any type errors.src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (7)
39-39: LGTM: New import added for LearningPathDTOThe addition of the import for
LearningPathDTOis appropriate, likely to support changes in the test methods that now use this DTO.
164-164: LGTM: Consistent use of renamed constantThe change from
STUDENT_OF_COURSEtoSTUDENT1_OF_COURSEin thesetupTestScenariomethod is consistent with the earlier constant renaming. This ensures that the setup method uses the correct identifier for the first student.
592-599: LGTM: Consistent updates in test methodThe changes in the
shouldReturnNotFoundIfNotExistsmethod are appropriate and consistent with earlier modifications:
- The variable name has been updated from
STUDENT_OF_COURSEtoSTUDENT1_OF_COURSE.- The API endpoint has been changed to use
/learning-path/me.These updates maintain consistency throughout the test class while preserving the original test logic.
607-626: LGTM: Consistent updates in GenerateLearningPath testsThe changes in the
GenerateLearningPathnested class are appropriate and consistent with earlier modifications:
- The
@WithMockUserannotations now correctly useSTUDENT1_OF_COURSE.- The API endpoints have been updated to use
/learning-path.These updates maintain consistency throughout the test class while preserving the original test logic.
Please verify that the new
/learning-pathendpoints are correctly implemented in the corresponding controller:#!/bin/bash # Verify the implementation of the new endpoints rg -g '*.java' -n '(GetMapping|PostMapping).*"/learning-path"'
Line range hint
673-897: LGTM: Consistent updates across remaining test methodsThe changes in the remaining test methods are appropriate and consistent with earlier modifications:
- All occurrences of
STUDENT_OF_COURSEhave been replaced withSTUDENT1_OF_COURSE.- API endpoints have been consistently updated across all methods.
These updates maintain consistency throughout the test class while preserving the original test logic for each method.
As a final check, please verify that all endpoints in the controller classes match the ones used in these tests:
#!/bin/bash # Verify all learning path related endpoints in controller classes rg -g '*Controller.java' -n '(GetMapping|PostMapping|PutMapping|PatchMapping|DeleteMapping).*"/learning-path'
579-588: LGTM: Improved test method and assertionsThe changes in the
GetLearningPathnested class are appropriate:
- The method renaming from
shouldReturnExistingIdtoshouldReturnExistingimproves clarity.- The API endpoint update to
/learning-path/meis consistent with earlier changes.- The assertion now uses
LearningPathDTO.of(learningPath), which is more precise.Please verify the implementation of
LearningPathDTO.of(LearningPath)method to ensure it correctly converts all necessary fields from LearningPath to LearningPathDTO.#!/bin/bash # Verify the implementation of LearningPathDTO.of method rg -g '*.java' -n 'public static LearningPathDTO of\(LearningPath'
222-225: LGTM: Updated mock user and API endpointThe changes in this test method are appropriate:
- The
@WithMockUserannotation now correctly usesSTUDENT1_OF_COURSE, consistent with earlier changes.- The API endpoint has been updated to use a more RESTful and user-centric approach with
/learning-path/me.Please verify that the new
/learning-path/meendpoint is correctly implemented in the corresponding controller and that it returns the expectedLearningPathDTO.src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (6)
2-2: Import statement correctly updated withLearningPathDTO.The inclusion of
LearningPathDTOin the import statement aligns with its usage in the component and is appropriate.
35-38: Services are properly injected using Angular'sinjectfunction.The injection of
LearningPathApiService,AlertService, andActivatedRouteis correctly implemented.
40-41: Signals are appropriately initialized.The signals
isLearningPathLoadingandlearningPathare correctly defined and initialized with their default values.
47-47: Effect function effectively loads the learning path.The
effectin the constructor properly callsloadLearningPathwith the currentcourseId, ensuring the learning path is loaded when the component initializes.
50-54:loadLearningPathmethod handles learning path retrieval correctly.The method correctly manages the loading state and asynchronously retrieves the learning path, updating the signal accordingly.
65-77:startLearningPathmethod starts the learning path as expected.The method checks for an existing learning path, generates one if necessary, and initiates it for the current user, updating the state appropriately.
🧰 Tools
🪛 Biome
[error] 72-72: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 73-73: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| private static final String STUDENT1_OF_COURSE = TEST_PREFIX + "student1"; | ||
|
|
||
| private static final String STUDENT2_OF_COURSE = TEST_PREFIX + "student2"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Improved clarity with student constants
The renaming of STUDENT_OF_COURSE to STUDENT1_OF_COURSE and the addition of STUDENT2_OF_COURSE improve the clarity of the tests by allowing distinct student identifiers. This change supports better test scenarios involving multiple students.
Consider adding a comment explaining the purpose of these constants for better code documentation.
| @Nested | ||
| class StartLearningPath { | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| course = learningPathUtilService.enableAndGenerateLearningPathsForCourse(course); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = STUDENT2_OF_COURSE, roles = "USER") | ||
| void shouldReturnForbiddenIfNotOwn() throws Exception { | ||
| final var student = userTestRepository.findOneByLogin(STUDENT1_OF_COURSE).orElseThrow(); | ||
| final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(course.getId(), student.getId()); | ||
| request.patch("/api/learning-path/" + learningPath.getId() + "/start", null, HttpStatus.FORBIDDEN); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = STUDENT1_OF_COURSE, roles = "USER") | ||
| void shouldReturnBadRequestIfAlreadyStarted() throws Exception { | ||
| final var student = userTestRepository.findOneByLogin(STUDENT1_OF_COURSE).orElseThrow(); | ||
| final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(course.getId(), student.getId()); | ||
| learningPath.setStartedByStudent(true); | ||
| learningPathRepository.save(learningPath); | ||
| request.patch("/api/learning-path/" + learningPath.getId() + "/start", null, HttpStatus.CONFLICT); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = STUDENT1_OF_COURSE, roles = "USER") | ||
| void shouldStartLearningPath() throws Exception { | ||
| final var student = userTestRepository.findOneByLogin(STUDENT1_OF_COURSE).orElseThrow(); | ||
| final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(course.getId(), student.getId()); | ||
| request.patch("/api/learning-path/" + learningPath.getId() + "/start", null, HttpStatus.NO_CONTENT); | ||
| final var updatedLearningPath = learningPathRepository.findByIdElseThrow(learningPath.getId()); | ||
| assertThat(updatedLearningPath.isStartedByStudent()).isTrue(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: New test class for StartLearningPath functionality
The addition of the StartLearningPath nested class is a valuable improvement to the test coverage:
- It tests the new functionality for starting a learning path.
- The tests cover multiple scenarios: forbidden access, already started paths, and successful start.
- The use of both
STUDENT1_OF_COURSEandSTUDENT2_OF_COURSEdemonstrates the value of the new constant.
These tests will help ensure the robustness of the learning path starting feature.
Consider adding a test case for when the learning path doesn't exist or when the course doesn't have learning paths enabled. This would further improve the test coverage.
| public LearningPathDTO getLearningPathForCurrentUser(long courseId) { | ||
| final var currentUser = userRepository.getUser(); | ||
| final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(courseId, currentUser.getId()); | ||
| return LearningPathDTO.of(learningPath); | ||
| } |
There was a problem hiding this comment.
Ensure User Enrollment Verification Before Accessing Learning Path
In the getLearningPathForCurrentUser method, it's important to verify that the current user is enrolled in the specified course before accessing the learning path. This prevents unauthorized access to learning paths of courses the user is not enrolled in.
Apply this diff to add the enrollment check:
public LearningPathDTO getLearningPathForCurrentUser(long courseId) {
final var currentUser = userRepository.getUser();
+ courseRepository.checkIfUserIsEnrolledInCourseElseThrow(currentUser, courseId);
final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(courseId, currentUser.getId());
return LearningPathDTO.of(learningPath);
}📝 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 LearningPathDTO getLearningPathForCurrentUser(long courseId) { | |
| final var currentUser = userRepository.getUser(); | |
| final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(courseId, currentUser.getId()); | |
| return LearningPathDTO.of(learningPath); | |
| } | |
| public LearningPathDTO getLearningPathForCurrentUser(long courseId) { | |
| final var currentUser = userRepository.getUser(); | |
| courseRepository.checkIfUserIsEnrolledInCourseElseThrow(currentUser, courseId); | |
| final var learningPath = learningPathRepository.findByCourseIdAndUserIdElseThrow(courseId, currentUser.getId()); | |
| return LearningPathDTO.of(learningPath); | |
| } |
| public LearningPathDTO generateLearningPathForCurrentUser(long courseId) { | ||
| final var currentUser = userRepository.getUser(); | ||
| final var course = courseRepository.findWithEagerCompetenciesAndPrerequisitesByIdElseThrow(courseId); | ||
| if (learningPathRepository.findByCourseIdAndUserId(courseId, currentUser.getId()).isPresent()) { | ||
| throw new ConflictException("Learning path already exists.", "LearningPath", "learningPathAlreadyExists"); | ||
| } | ||
| final var learningPath = generateLearningPathForUser(course, currentUser); | ||
| return LearningPathDTO.of(learningPath); | ||
| } |
There was a problem hiding this comment.
Add Enrollment Check Before Generating Learning Path
In the generateLearningPathForCurrentUser method, consider verifying that the current user is enrolled in the specified course. This ensures that only authorized users can generate learning paths for courses they are part of.
Apply this diff to include the enrollment check:
public LearningPathDTO generateLearningPathForCurrentUser(long courseId) {
final var currentUser = userRepository.getUser();
+ courseRepository.checkIfUserIsEnrolledInCourseElseThrow(currentUser, courseId);
final var course = courseRepository.findWithEagerCompetenciesAndPrerequisitesByIdElseThrow(courseId);
if (learningPathRepository.findByCourseIdAndUserId(courseId, currentUser.getId()).isPresent()) {
throw new ConflictException("Learning path already exists.", "LearningPath", "learningPathAlreadyExists");
}
final var learningPath = generateLearningPathForUser(course, currentUser);
return LearningPathDTO.of(learningPath);
}📝 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 LearningPathDTO generateLearningPathForCurrentUser(long courseId) { | |
| final var currentUser = userRepository.getUser(); | |
| final var course = courseRepository.findWithEagerCompetenciesAndPrerequisitesByIdElseThrow(courseId); | |
| if (learningPathRepository.findByCourseIdAndUserId(courseId, currentUser.getId()).isPresent()) { | |
| throw new ConflictException("Learning path already exists.", "LearningPath", "learningPathAlreadyExists"); | |
| } | |
| final var learningPath = generateLearningPathForUser(course, currentUser); | |
| return LearningPathDTO.of(learningPath); | |
| } | |
| public LearningPathDTO generateLearningPathForCurrentUser(long courseId) { | |
| final var currentUser = userRepository.getUser(); | |
| courseRepository.checkIfUserIsEnrolledInCourseElseThrow(currentUser, courseId); | |
| final var course = courseRepository.findWithEagerCompetenciesAndPrerequisitesByIdElseThrow(courseId); | |
| if (learningPathRepository.findByCourseIdAndUserId(courseId, currentUser.getId()).isPresent()) { | |
| throw new ConflictException("Learning path already exists.", "LearningPath", "learningPathAlreadyExists"); | |
| } | |
| final var learningPath = generateLearningPathForUser(course, currentUser); | |
| return LearningPathDTO.of(learningPath); | |
| } |
| public void startLearningPathForCurrentUser(long learningPathId) { | ||
| final var learningPath = learningPathRepository.findByIdElseThrow(learningPathId); | ||
| final var currentUser = userRepository.getUser(); | ||
| if (!learningPath.getUser().equals(currentUser)) { | ||
| throw new AccessForbiddenException("You are not allowed to start this learning path."); | ||
| } | ||
| else if (learningPath.isStartedByStudent()) { | ||
| throw new ConflictException("Learning path already started.", "LearningPath", "learningPathAlreadyStarted"); | ||
| } | ||
| learningPath.setStartedByStudent(true); | ||
| learningPathRepository.save(learningPath); | ||
| } |
There was a problem hiding this comment.
Verify User Enrollment Before Starting Learning Path
While you are checking if the learning path belongs to the current user in the startLearningPathForCurrentUser method, consider also verifying that the user is enrolled in the course associated with the learning path. This adds an extra layer of security to prevent unauthorized access.
Apply this diff to add the enrollment check:
public void startLearningPathForCurrentUser(long learningPathId) {
final var learningPath = learningPathRepository.findByIdElseThrow(learningPathId);
final var currentUser = userRepository.getUser();
if (!learningPath.getUser().equals(currentUser)) {
throw new AccessForbiddenException("You are not allowed to start this learning path.");
}
+ if (!courseRepository.isUserEnrolledInCourse(currentUser, learningPath.getCourse().getId())) {
+ throw new AccessForbiddenException("You are not enrolled in this course.");
+ }
else if (learningPath.isStartedByStudent()) {
throw new ConflictException("Learning path already started.", "LearningPath", "learningPathAlreadyStarted");
}
learningPath.setStartedByStudent(true);
learningPathRepository.save(learningPath);
}📝 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 startLearningPathForCurrentUser(long learningPathId) { | |
| final var learningPath = learningPathRepository.findByIdElseThrow(learningPathId); | |
| final var currentUser = userRepository.getUser(); | |
| if (!learningPath.getUser().equals(currentUser)) { | |
| throw new AccessForbiddenException("You are not allowed to start this learning path."); | |
| } | |
| else if (learningPath.isStartedByStudent()) { | |
| throw new ConflictException("Learning path already started.", "LearningPath", "learningPathAlreadyStarted"); | |
| } | |
| learningPath.setStartedByStudent(true); | |
| learningPathRepository.save(learningPath); | |
| } | |
| public void startLearningPathForCurrentUser(long learningPathId) { | |
| final var learningPath = learningPathRepository.findByIdElseThrow(learningPathId); | |
| final var currentUser = userRepository.getUser(); | |
| if (!learningPath.getUser().equals(currentUser)) { | |
| throw new AccessForbiddenException("You are not allowed to start this learning path."); | |
| } | |
| if (!courseRepository.isUserEnrolledInCourse(currentUser, learningPath.getCourse().getId())) { | |
| throw new AccessForbiddenException("You are not enrolled in this course."); | |
| } | |
| else if (learningPath.isStartedByStudent()) { | |
| throw new ConflictException("Learning path already started.", "LearningPath", "learningPathAlreadyStarted"); | |
| } | |
| learningPath.setStartedByStudent(true); | |
| learningPathRepository.save(learningPath); | |
| } |
| * flag indicating if a student started the learning path | ||
| */ | ||
| @Column(name = "started_by_student") | ||
| private boolean startedByStudent = false; |
There was a problem hiding this comment.
Should we move this to the future LearnerProfile, that is stored once per student? Or should they get the explanation in each course?
There was a problem hiding this comment.
Makes sense for the future. @N0W0RK can you add that point to your backlog?
Warning
Contains database migration
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently students get directly confronted with the learning path feature without an explanation. This PR introduces a simple explanation and enables students to start their learning path on their own.
Description
This PR adds a new column in the learning path table indicating if the student has started their learning path. Initially, the page shows a short description of how the learning path feature works. This message persists until the student starts the learning path manually.
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
Code Review
Manual Tests
Test Coverage
Client
Screenshots
Summary by CodeRabbit
Release Notes
New Features
LearningPathDTOfor enhanced data representation of learning paths.Improvements
Bug Fixes