Adaptive Learning: Fix prerequisites not shown in course enrolment
#9358
Conversation
WalkthroughThe changes in this pull request involve modifications to access control annotations 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range comments (1)
src/main/webapp/app/course/competencies/competency-card/competency-card.component.ts (1)
Line range hint
1-52: Overall: Code adheres to guidelines and best practicesThe
CompetencyCardComponentdemonstrates good adherence to Angular style guidelines and TypeScript best practices:
- Proper use of TypeScript features (e.g., strong typing, access modifiers).
- Consistent naming conventions (camelCase for methods and properties, PascalCase for types).
- Effective use of Angular decorators (
@Component,@Input).- Clear and concise method implementations.
The recent changes (new input property and method visibility change) integrate well with the existing code structure. The component appears to be part of a lazy-loaded module, which is good for performance.
To further improve the component:
- Consider adding JSDoc comments for the public API (i.e., input properties and public methods) to enhance documentation.
- If this component is used in templates with
*ngFor, consider implementingChangeDetectionStrategy.OnPushfor potential performance benefits.
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (7)
- src/main/java/de/tum/cit/aet/artemis/atlas/web/PrerequisiteResource.java (2 hunks)
- src/main/webapp/app/course/competencies/competency-card/competency-card.component.html (1 hunks)
- src/main/webapp/app/course/competencies/competency-card/competency-card.component.ts (1 hunks)
- src/main/webapp/app/overview/course-registration/course-registration-prerequisites-modal/course-prerequisites-modal.component.html (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/competency/CompetencyIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/competency/PrerequisiteIntegrationTest.java (1 hunks)
Additional context used
Path-based instructions (7)
src/main/java/de/tum/cit/aet/artemis/atlas/web/PrerequisiteResource.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/competencies/competency-card/competency-card.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/competencies/competency-card/competency-card.component.ts (1)
src/main/webapp/app/overview/course-registration/course-registration-prerequisites-modal/course-prerequisites-modal.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/test/java/de/tum/cit/aet/artemis/competency/AbstractCompetencyPrerequisiteIntegrationTest.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/java/de/tum/cit/aet/artemis/competency/CompetencyIntegrationTest.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/java/de/tum/cit/aet/artemis/competency/PrerequisiteIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
Learnings (3)
src/test/java/de/tum/cit/aet/artemis/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (1)
Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-02-23T00:03:06.365Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.src/test/java/de/tum/cit/aet/artemis/competency/CompetencyIntegrationTest.java (1)
Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-02-23T00:03:06.365Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.src/test/java/de/tum/cit/aet/artemis/competency/PrerequisiteIntegrationTest.java (1)
Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-02-23T00:03:06.365Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Additional comments not posted (11)
src/main/webapp/app/overview/course-registration/course-registration-prerequisites-modal/course-prerequisites-modal.component.html (2)
16-16: LGTM: New property aligns with PR objectivesThe addition of
[noProgressRings]="true"to the<jhi-competency-card>component is consistent with the PR objective of removing competency rings for students who are not yet enrolled. This change enhances the user experience by providing appropriate information based on the user's enrollment status.
Line range hint
7-18: Excellent use of new Angular syntaxThe template correctly uses the new Angular syntax
@ifand@forinstead of*ngIfand*ngFor. This adherence to the latest Angular best practices improves code readability and maintainability.src/main/webapp/app/course/competencies/competency-card/competency-card.component.ts (1)
20-21: LGTM: New input property added correctlyThe new
noProgressRingsinput property is well-implemented. It follows the camelCase naming convention, uses the@Input()decorator correctly, and has a default value set. This addition enhances the component's flexibility by allowing control over the display of progress rings.src/main/webapp/app/course/competencies/competency-card/competency-card.component.html (1)
61-65: LGTM! Conditional rendering implemented correctly.The changes effectively implement the conditional rendering of the competency rings component. This aligns well with the PR objective of removing competency rings for students who are not yet enrolled. The use of the new
@ifsyntax instead of*ngIfadheres to the provided coding guidelines for Angular templates.src/test/java/de/tum/cit/aet/artemis/competency/CompetencyIntegrationTest.java (2)
Line range hint
1-380: Overall structure adheres to coding guidelinesThe test class generally follows the coding guidelines for test files:
- Test methods have descriptive names.
- Tests are small and specific.
- JUnit 5 features like @ParameterizedTest are used.
- Authentication is properly mocked using @WithMockUser annotations.
For consistency, consider applying the @test annotation to all test methods, even those that override superclass methods. For example:
+ @Test @Override List<? extends CourseCompetency> getAllCall(long courseId, HttpStatus expectedStatus) throws Exception { return request.getList("/api/courses/" + courseId + "/competencies", expectedStatus, Competency.class); }This change would improve consistency and make it clear that these methods are also test cases.
Line range hint
1-380: Summary of review findings
- The main change in
shouldReturnCompetenciesForStudentOfCoursemethod has been addressed, with a suggestion to update the method name for clarity.- The overall structure of the test class adheres well to the coding guidelines for test files.
- A minor suggestion for consistency in applying @test annotations has been provided.
The changes align with the PR objectives of broadening access to course prerequisites and shifting from student-specific to course-level competency evaluations. The test structure supports these changes while maintaining good testing practices.
No further issues were identified. Once the suggested improvements are implemented, the code will be ready for the next stage of the review process.
src/test/java/de/tum/cit/aet/artemis/competency/PrerequisiteIntegrationTest.java (2)
110-110: LGTM! Improved test method name and implementation.The changes to this test method are well-aligned with our coding guidelines:
- The new method name
shouldReturnCompetenciesForCourseis more descriptive and accurately reflects the test's purpose, following our test_naming guideline.- The implementation leverages the parent class method, adhering to the util_service_factory_pattern guideline.
- The test remains small and specific, in line with our test_size guideline.
These changes improve the test's clarity and maintainability.
115-116: Improved test method name, but potential discrepancy in implementation.The changes to this test method align well with our coding guidelines:
- The new method name
shouldReturnCompetenciesForStudentNotInCourseis more descriptive and follows the "should" naming convention, adhering to our test_naming guideline.- The implementation leverages the parent class method, following the util_service_factory_pattern guideline.
- The test remains small and specific, in line with our test_size guideline.
However, there's a potential issue:
The method name suggests it's testing for a student not in the course, but the implementation is identical to the previous test for a student in the course. This might indicate a discrepancy between the test's intended purpose and its actual implementation.Could you please verify if the implementation correctly tests the scenario for a student not in the course? If not, consider updating either the method name or its implementation to ensure consistency between the test's name and its behavior.
src/main/java/de/tum/cit/aet/artemis/atlas/web/PrerequisiteResource.java (3)
42-42: LGTM: New import statement addedThe import statement for
@EnforceAtLeastStudentis correctly added. This import is necessary for the annotation change in thegetPrerequisitesWithProgressmethod.
Line range hint
1-385: Overall code quality is good, changes are minimal and focusedThe file adheres to the provided coding guidelines:
- Follows CamelCase naming convention
- Adheres to single responsibility principle
- Uses constructor injection for dependencies
- Follows RESTful principles
The changes made are minimal and focused on the PR objective of allowing all students to see prerequisites regardless of enrollment status. The overall code quality is good.
Line range hint
102-106: Verify prerequisiteService implementation for non-enrolled usersThe implementation of
getPrerequisitesWithProgresslooks correct. However, to ensure it fully meets the PR objective, we should verify that thefindPrerequisitesWithProgressForUserByCourseIdmethod inprerequisiteServicecorrectly handles the case where the user is not enrolled in the course.Let's check the implementation of
findPrerequisitesWithProgressForUserByCourseId:#!/bin/bash # Description: Check the implementation of findPrerequisitesWithProgressForUserByCourseId # Search for findPrerequisitesWithProgressForUserByCourseId method echo "Searching for findPrerequisitesWithProgressForUserByCourseId method:" rg --type java -A 10 "findPrerequisitesWithProgressForUserByCourseId"This will help us ensure that the service method correctly handles non-enrolled users, aligning with the PR objective.
|
|
||
| getIcon = getIcon; | ||
| protected readonly getIcon = getIcon; |
There was a problem hiding this comment.
Approved: Method visibility change enhances encapsulation
The change of getIcon to a protected, readonly property is a good improvement. It enhances encapsulation by restricting access and ensures immutability.
However, for consistency and clarity, consider the following minor suggestion:
Consider moving this line closer to other class properties for better organization. For example, you could place it right after the @Input properties:
@Input() noProgressRings = false;
protected readonly getIcon = getIcon;
constructor(public translateService: TranslateService) {}This arrangement would group all class properties together, improving readability.
| @@ -107,7 +107,7 @@ List<? extends CourseCompetency> getAllCall(long courseId, HttpStatus expectedSt | |||
| @Test | |||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | |||
| void shouldReturnCompetenciesForStudentOfCourse() throws Exception { | |||
| super.shouldReturnCompetenciesForStudentOfCourse(new Competency()); | |||
| super.shouldReturnCompetenciesForCourse(new Competency()); | |||
There was a problem hiding this comment.
Update method name to reflect new behavior
The method name shouldReturnCompetenciesForStudentOfCourse no longer accurately describes the test's behavior. The implementation now calls shouldReturnCompetenciesForCourse, which aligns with the PR objective of allowing all students to see course prerequisites regardless of enrollment status.
Consider updating the method name to better reflect its new purpose. For example:
- void shouldReturnCompetenciesForStudentOfCourse() throws Exception {
+ void shouldReturnCompetenciesForCourse() throws Exception {
super.shouldReturnCompetenciesForCourse(new Competency());
}This change would make the test method name consistent with its implementation and the overall PR objectives.
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.
| super.shouldReturnCompetenciesForCourse(new Competency()); | |
| void shouldReturnCompetenciesForCourse() throws Exception { | |
| super.shouldReturnCompetenciesForCourse(new Competency()); | |
| } |
| @@ -97,7 +98,7 @@ public PrerequisiteResource(CourseRepository courseRepository, AuthorizationChec | |||
| * @return the ResponseEntity with status 200 (OK) and with body the found prerequisites | |||
| */ | |||
| @GetMapping("courses/{courseId}/prerequisites") | |||
| @EnforceAtLeastStudentInCourse | |||
| @EnforceAtLeastStudent | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Multiple usages of @EnforceAtLeastStudentInCourse found
The annotation change to @EnforceAtLeastStudent was applied in PrerequisiteResource.java. However, the following files still use @EnforceAtLeastStudentInCourse, which may introduce security vulnerabilities if not updated:
- src/main/java/de/tum/cit/aet/artemis/iris/web/IrisCourseChatSessionResource.java
- src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java
- src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java
- src/main/java/de/tum/cit/aet/artemis/atlas/web/CompetencyResource.java
- src/main/java/de/tum/cit/aet/artemis/atlas/web/MetricsResource.java
- src/test/java/de/tum/cit/aet/artemis/aspects/resources/EnforceRoleInCourseResource.java
Please update all instances of @EnforceAtLeastStudentInCourse to @EnforceAtLeastStudent to ensure consistent access control across the codebase.
Analysis chain
Verify impact of broadened access control
The annotation change from @EnforceAtLeastStudentInCourse to @EnforceAtLeastStudent aligns with the PR objective of allowing all students to see prerequisites, regardless of their enrollment status. However, we should verify that this change doesn't introduce any security vulnerabilities.
Let's run a script to check for any other usages of @EnforceAtLeastStudentInCourse in the codebase:
This script will help us ensure that the change is consistent across the codebase and that there are no unintended side effects.
Let's run a corrected script to check for any other usages of @EnforceAtLeastStudentInCourse in the codebase:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of @EnforceAtLeastStudentInCourse
# Search for @EnforceAtLeastStudentInCourse usage
echo "Searching for @EnforceAtLeastStudentInCourse usage:"
rg --type java "@EnforceAtLeastStudentInCourse"
# Search for getPrerequisitesWithProgress method usage
echo "Searching for getPrerequisitesWithProgress method usage:"
rg --type java "getPrerequisitesWithProgress"
Length of output: 2516
Script:
#!/bin/bash
# Description: Check for other usages of @EnforceAtLeastStudentInCourse
# Search for @EnforceAtLeastStudentInCourse usage
echo "Searching for @EnforceAtLeastStudentInCourse usage:"
rg --type java "@EnforceAtLeastStudentInCourse"
# Search for getPrerequisitesWithProgress method usage
echo "Searching for getPrerequisitesWithProgress method usage:"
rg --type java "getPrerequisitesWithProgress"
Length of output: 2454
| @@ -227,7 +227,7 @@ void shouldReturnBadRequestForWrongCourse() throws Exception { | |||
| abstract List<? extends CourseCompetency> getAllCall(long courseId, HttpStatus expectedStatus) throws Exception; | |||
|
|
|||
| // Test | |||
| void shouldReturnCompetenciesForStudentOfCourse(CourseCompetency newCompetency) throws Exception { | |||
| void shouldReturnCompetenciesForCourse(CourseCompetency newCompetency) throws Exception { | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Old Method Name Still Referenced in Other Test Classes
The following test classes still contain references to shouldReturnCompetenciesForStudentOfCourse. Please update these method names to align with the new naming convention:
src/test/java/de/tum/cit/aet/artemis/competency/PrerequisiteIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/competency/CompetencyIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/competency/CourseCompetencyIntegrationTest.java
Analysis chain
LGTM! Verify impact on other test classes.
The method name change from shouldReturnCompetenciesForStudentOfCourse to shouldReturnCompetenciesForCourse aligns well with the PR objectives of allowing all students to see prerequisites regardless of enrollment status. This change shifts the focus from student-specific competencies to course-wide competencies.
To ensure this change doesn't affect other parts of the codebase, please run the following script:
If the script returns any results, please update those references accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any references to the old method name in other test classes
# Search for any references to the old method name
rg --type java "shouldReturnCompetenciesForStudentOfCourse" src/test/java
Length of output: 703
Checklist
General
Server
Client
Motivation and Context
Users got a not authorised exception when trying to see the prerequisites of a course before enrolling.
Description
Changed the authorisation to see prerequisites of a course to all students regardless if they are already enrolled in the course.
Removed the competency rings when viewing the prerequisites before enrolling in a course, since the student does not yet have progress in them.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Summary by CodeRabbit
New Features
noProgressRingsto the competency card, allowing for conditional rendering of progress rings based on user context.Bug Fixes
getPrerequisitesWithProgressmethod, broadening user roles that can access it.Tests