Adaptive learning: Add learner profile interface
#10440
Conversation
github-actions
bot
added
tests
server
N0W0RK
added
ready for review
tests
server
WalkthroughThis pull request introduces a set of new backend and frontend features for managing course learner profiles. On the backend, a new constant, DTO record, repository method, and REST controller endpoints are added. On the frontend, several Angular components, services, and models support an interactive learner profile interface that includes editable sliders, localization updates, and a new user settings route. Test suites for these new UI components have also been added to ensure correct behavior. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (25)
src/main/webapp/app/entities/learner-profile.model.ts (2)
1-7: Add JSDoc documentation to improve code clarity.The
CourseLearnerProfileDTOclass lacks documentation explaining its purpose and the meaning of each property. Adding JSDoc comments would enhance code readability and maintainability.+/** + * Data Transfer Object for Course Learner Profile. + * Represents a user's learning profile settings for a specific course. + */ export class CourseLearnerProfileDTO { + /** Unique identifier for the learner profile */ public id: number; + /** The associated course identifier */ public courseId: number; + /** Indicates the user's goal for grade or bonus in the course (value range: typically 0-100) */ public aimForGradeOrBonus: number; + /** Represents the user's planned time investment for the course (value range: typically 0-100) */ public timeInvestment: number; + /** Indicates how intensely the user plans to review course materials (value range: typically 0-100) */ public repetitionIntensity: number; }
1-7: Consider adding a constructor to initialize properties.Adding a constructor would help ensure proper initialization of the DTO and provide a cleaner way to create new instances.
export class CourseLearnerProfileDTO { public id: number; public courseId: number; public aimForGradeOrBonus: number; public timeInvestment: number; public repetitionIntensity: number; + + constructor( + id?: number, + courseId?: number, + aimForGradeOrBonus?: number, + timeInvestment?: number, + repetitionIntensity?: number + ) { + this.id = id ?? 0; + this.courseId = courseId ?? 0; + this.aimForGradeOrBonus = aimForGradeOrBonus ?? 0; + this.timeInvestment = timeInvestment ?? 0; + this.repetitionIntensity = repetitionIntensity ?? 0; + } }src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1)
22-49: Consider using @for instead of manually repeating navigation items.Per the provided coding guidelines,
@for(which replaces*ngFor) should be used instead of manually repeating similar elements. This would make the navigation bar more maintainable.You could refactor this section to use an array of navigation items in the component class and iterate through them in the template:
<section id="navigation-bar" class="list-group d-block pt-2"> <span class="list-group-item disabled fw-bold" jhiTranslate="artemisApp.userSettings.userSettings"></span> - <a class="list-group-item btn btn-outline-primary" routerLink="account" routerLinkActive="active" jhiTranslate="artemisApp.userSettings.accountInformation"></a> - <a class="list-group-item btn btn-outline-primary" routerLink="profile" routerLinkActive="active" jhiTranslate="artemisApp.userSettings.learnerProfile"></a> - <a - class="list-group-item btn btn-outline-primary" - routerLink="notifications" - routerLinkActive="active" - jhiTranslate="artemisApp.userSettings.notificationSettings" - ></a> - <a class="list-group-item btn btn-outline-primary" routerLink="science" routerLinkActive="active" jhiTranslate="artemisApp.userSettings.scienceSettings"></a> + @for (navItem of basicNavItems; track navItem.route) { + <a + class="list-group-item btn btn-outline-primary" + [routerLink]="navItem.route" + routerLinkActive="active" + [jhiTranslate]="navItem.translationKey"> + </a> + } @if (localVCEnabled) { <a class="list-group-item btn btn-outline-primary" routerLink="ssh" routerLinkActive="active" jhiTranslate="artemisApp.userSettings.sshSettings"></a> @if (isAtLeastTutor) {In the component:
basicNavItems = [ { route: 'account', translationKey: 'artemisApp.userSettings.accountInformation' }, { route: 'profile', translationKey: 'artemisApp.userSettings.learnerProfile' }, { route: 'notifications', translationKey: 'artemisApp.userSettings.notificationSettings' }, { route: 'science', translationKey: 'artemisApp.userSettings.scienceSettings' }, ];src/main/webapp/i18n/de/userSettings.json (1)
11-11: Consider using "Lernerprofil" instead of "Lerner Profil".In German, compound nouns are typically written as one word. The current translation "Lerner Profil" should be changed to "Lernerprofil" for better adherence to German language conventions.
- "learnerProfile": "Lerner Profil", + "learnerProfile": "Lernerprofil",src/main/webapp/app/shared/user-settings/learner-profile/learner-profile.component.ts (1)
8-8: Make standalone: true for better modularity.Consider adding
standalone: trueto the component decorator to follow modern Angular practices and improve modularity.selector: 'jhi-learner-profile', templateUrl: './learner-profile.component.html', styleUrls: ['../user-settings.scss'], - imports: [CourseLearnerProfileComponent], + standalone: true, + imports: [CourseLearnerProfileComponent],src/main/webapp/app/shared/editable-slider/edit-process.component.html (2)
3-3: Consider adding aria-labels for accessibility.The icons used for saving and aborting actions should have aria-labels to improve accessibility for screen readers.
- <span><fa-icon class="text-success mx-2" [icon]="faCheck" (click)="onSave()" /><fa-icon class="text-danger mx-2" [icon]="faXmark" (click)="onAbort()" /></span> + <span><fa-icon class="text-success mx-2" [icon]="faCheck" (click)="onSave()" aria-label="Save changes" /><fa-icon class="text-danger mx-2" [icon]="faXmark" (click)="onAbort()" aria-label="Cancel changes" /></span>
6-6: Add aria-label to edit icon for accessibility.The pencil icon should have an aria-label for better accessibility.
- <fa-icon class="mx-2" [icon]="faPencil" (click)="onEdit()" /> + <fa-icon class="mx-2" [icon]="faPencil" (click)="onEdit()" aria-label="Edit" />src/main/webapp/app/shared/editable-slider/editable-slider.component.html (1)
1-9: Appropriate implementation of reusable slider component.The component has a clean structure with responsive layout using Bootstrap grid system. I appreciate the separation of concerns with dedicated components for edit process and double slider functionality.
One consideration: The line lengths in the double slider element (line 8) are quite long. Consider splitting the attributes across multiple lines for improved readability.
- <jhi-double-slider [editStateTransition]="editStateTransition()" [currentValue]="currentValue()" [(initialValue)]="initialValue" [min]="min()" [max]="max()" /> + <jhi-double-slider + [editStateTransition]="editStateTransition()" + [currentValue]="currentValue()" + [(initialValue)]="initialValue" + [min]="min()" + [max]="max()" + />src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.html (2)
19-56: Component structure has a potential layout issue.The slider components are wrapped in a grid with
row-cols-2but there are three slider components. This may cause unexpected layout behavior where the third slider might not align properly.Either adjust the grid structure to accommodate three items per row:
- <div class="row row-cols-2 gx-5 align-items-end"> + <div class="row row-cols-3 gx-5 align-items-end">Or if you intended to have two components per row with the third on the next row, no changes are needed, but consider adding a comment explaining this intention.
44-44: Inconsistent use of alignment classes.Line 44 includes an
align-items-endclass on a column div, while other column divs don't have this class. This creates inconsistency in the layout. Thealign-items-endclass should be applied to the row element if it needs to apply to all columns.- <div class="col align-items-end"> + <div class="col">src/main/webapp/app/shared/editable-slider/edit-process.component.ts (1)
31-44: Simplify the switch statement to improve readability.The current implementation has redundant case clauses that could be simplified according to the static analysis. Since both
TrySaveandAborttransitions lead to the same behavior (settingediting = false), these cases can be combined.ngOnChanges(changes: SimpleChanges): void { if (changes.editStateTransition) { switch (changes.editStateTransition.currentValue) { case EditStateTransition.Edit: this.editing = true; break; - case EditStateTransition.TrySave: - case EditStateTransition.Abort: default: this.editing = false; break; } } }🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 38-38: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts (2)
11-13: Addawaitkeyword for consistency in async method.The
putUpdatedCourseLearnerProfilemethod is declared asasyncbut doesn't useawaitwhen calling the baseputmethod, which is inconsistent with thegetCourseLearnerProfilesForCurrentUsermethod. While it will still work, it's better to be consistent.async putUpdatedCourseLearnerProfile(courseLearnerProfile: CourseLearnerProfileDTO): Promise<CourseLearnerProfileDTO> { - return this.put<CourseLearnerProfileDTO>(`atlas/course-learner-profiles/${courseLearnerProfile.id}`, courseLearnerProfile); + return await this.put<CourseLearnerProfileDTO>(`atlas/course-learner-profiles/${courseLearnerProfile.id}`, courseLearnerProfile); }
1-14: Consider adding error handling for API calls.The service methods don't include error handling. In a production environment, it's important to handle potential API errors gracefully.
async getCourseLearnerProfilesForCurrentUser(): Promise<Record<number, CourseLearnerProfileDTO>> { - return await this.get<Record<number, CourseLearnerProfileDTO>>('atlas/course-learner-profiles'); + try { + return await this.get<Record<number, CourseLearnerProfileDTO>>('atlas/course-learner-profiles'); + } catch (error) { + console.error('Failed to fetch learner profiles:', error); + throw error; + } } async putUpdatedCourseLearnerProfile(courseLearnerProfile: CourseLearnerProfileDTO): Promise<CourseLearnerProfileDTO> { - return this.put<CourseLearnerProfileDTO>(`atlas/course-learner-profiles/${courseLearnerProfile.id}`, courseLearnerProfile); + try { + return await this.put<CourseLearnerProfileDTO>(`atlas/course-learner-profiles/${courseLearnerProfile.id}`, courseLearnerProfile); + } catch (error) { + console.error('Failed to update learner profile:', error); + throw error; + } }src/test/javascript/spec/component/learner-profile/component/double-slider.component.spec.ts (2)
42-45: Use toBeTrue instead of toBeFalse for boolean assertions.According to the coding guidelines, boolean assertions should use
toBeTrue/toBeFalseinstead oftoBeTruthy/toBeFalsy.it('should enable slider when editing', () => { component.onEdit(); - expect(component.currentSlider().nativeElement.disabled).toBeFalse(); + expect(component.currentSlider().nativeElement.disabled).toBeFalse(); });Note: The code is already using
toBeFalse()which aligns with the coding guidelines, so this is actually compliant. I initially thought it might be usingtoBeFalsy()which would have needed correction.
6-82: Consider adding accessibility tests.The component involves UI interactions that should be accessible to all users. Consider adding tests that verify keyboard accessibility and ARIA attributes.
You could add tests like:
it('should be keyboard accessible', () => { // Setup component in edit mode component.onEdit(); // Simulate keyboard interaction const event = new KeyboardEvent('keydown', { key: 'ArrowRight' }); component.currentSlider().nativeElement.dispatchEvent(event); // Check if slider value changes appropriately // This would depend on your slider implementation }); it('should have appropriate ARIA attributes', () => { // Check for aria-valuenow, aria-valuemin, aria-valuemax expect(component.currentSlider().nativeElement.getAttribute('aria-valuenow')).toBe(component.currentValue().toString()); expect(component.currentSlider().nativeElement.getAttribute('aria-valuemin')).toBe(component.min().toString()); expect(component.currentSlider().nativeElement.getAttribute('aria-valuemax')).toBe(component.max().toString()); });src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (5)
106-111: Remove duplicate await fixture.whenStable().There's a duplicate call to await fixture.whenStable() which is unnecessary and may cause delays in test execution.
fixture.detectChanges(); await fixture.whenStable(); - await fixture.whenStable(); fixture.detectChanges();
117-121: Consider adding a return type to the selectCourse function.Adding a return type to the utility function would improve code clarity and maintainability.
- function selectCourse(id: number) { + function selectCourse(id: number): void { Array.from(selector.options).forEach((opt) => { opt.selected = opt.value == String(id); }); }
137-145: Add type annotations to improve helper function readability.The setupUpdateTest function could benefit from explicit type annotations for better clarity, especially since it returns a complex type.
- function setupUpdateTest(course: number, updateFn: (value: number) => void, attribute: keyof CourseLearnerProfileDTO): CourseLearnerProfileDTO { + function setupUpdateTest( + course: number, + updateFn: (value: number) => void, + attribute: keyof CourseLearnerProfileDTO + ): CourseLearnerProfileDTO { let newVal = (profiles[course][attribute] + 1) % 5; let newProfile = profiles[course]; newProfile[attribute] = newVal; component.activeCourse = course; updateFn(newVal); return newProfile; }
147-153: Add return type to helper function for consistency.For consistency with other helper functions, consider adding a return type to validateUpdate.
- function validateUpdate(course: number, profile: CourseLearnerProfileDTO) { + function validateUpdate(course: number, profile: CourseLearnerProfileDTO): void { const req = httpTesting.expectOne(`api/learner-profiles/course-learner-profiles/${course}`, 'Request to put new Profile'); req.flush(profile); expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); expect(component.courseLearnerProfiles[course]).toEqual(profile); }
155-165: Add return type to validateError function for consistency.Similar to validateUpdate, add a return type to validateError for consistency.
- function validateError(course: number, profile: CourseLearnerProfileDTO) { + function validateError(course: number, profile: CourseLearnerProfileDTO): void { const req = httpTesting.expectOne(`api/learner-profiles/course-learner-profiles/${course}`, 'Request to put new Profile'); req.flush(errorBody, { headers: errorHeaders, status: 400, statusText: 'Bad Request', }); expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); expect(component.courseLearnerProfiles[course]).toEqual(profiles[course]); }src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts (1)
97-135: Consider refactoring repetitive update methods.The three update methods (updateAimForGradeOrBonus, updateTimeInvestment, updateRepetitionIntensity) have very similar logic. Consider refactoring them into a single generic method to reduce code duplication.
+ private updateProfileAttribute( + value: number, + attribute: keyof CourseLearnerProfileDTO, + stateSignal: WritableSignal<EditStateTransition> + ): void { + // Return if value is changed without user trying to save + if (!this.courseLearnerProfiles || stateSignal() != EditStateTransition.TrySave) { + return; + } + + // Try to update profile + const courseLearnerProfile = this.courseLearnerProfiles[this.activeCourse]; + courseLearnerProfile[attribute] = value; + + this.learnerProfileAPIService.putUpdatedCourseLearnerProfile(courseLearnerProfile).then( + (updatedProfile) => { + // Update profile with response from server + this.courseLearnerProfiles[this.activeCourse] = updatedProfile; + stateSignal.set(EditStateTransition.Saved); + }, + // Notify user of failure to update + (res: HttpErrorResponse) => this.onCourseLearnerProfileUpdateError(res, stateSignal) + ); + } + + updateAimForGradeOrBonus(value: number): void { + this.updateProfileAttribute(value, 'aimForGradeOrBonus', this.aimForGradeOrBonusState); + } + + updateTimeInvestment(value: number): void { + this.updateProfileAttribute(value, 'timeInvestment', this.timeInvestmentState); + } + + updateRepetitionIntensity(value: number): void { + this.updateProfileAttribute(value, 'repetitionIntensity', this.repetitionIntensityState); + }src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (4)
53-53: Typographical fix: "which" instead of "wich"
The doc comment at line 53 has a minor typographical error in the word "wich." It should be "which" for clarity and correctness.- * @return The ResponseEntity with status 200 (OK) and with body containing a map of DTOs, wich contain per course profile data. + * @return The ResponseEntity with status 200 (OK) and with body containing a map of DTOs, which contain per course profile data.
91-94: Typographical fix in error message key and text
"courseLEarnerProfileId" is misspelled (capital "L" rather than lowercase). To maintain consistency and clarity in your error messages, correct the spelling to "courseLearnerProfileId."- throw new BadRequestAlertException("Provided courseLEarnerProfileId does not match CourseLearnerProfile.", + throw new BadRequestAlertException("Provided courseLearnerProfileId does not match CourseLearnerProfile.",
60-62: Potential performance consideration for large datasets
When querying many CourseLearnerProfiles, loading all records at once (viafindAllByLogin) could be inefficient for users with many courses. If performance or memory usage becomes a concern, consider paginating or filtering based on active courses.
84-115: Concurrency caution for updates
When multiple clients update the same learner profile concurrently, the last writer wins. If you want to prevent race conditions or stale updates, consider an optimistic locking approach or a version field.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/dto/CourseLearnerProfileDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java(1 hunks)src/main/webapp/app/entities/learner-profile.model.ts(1 hunks)src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts(1 hunks)src/main/webapp/app/shared/editable-slider/double-slider.component.html(1 hunks)src/main/webapp/app/shared/editable-slider/double-slider.component.scss(1 hunks)src/main/webapp/app/shared/editable-slider/double-slider.component.ts(1 hunks)src/main/webapp/app/shared/editable-slider/edit-process.component.html(1 hunks)src/main/webapp/app/shared/editable-slider/edit-process.component.scss(1 hunks)src/main/webapp/app/shared/editable-slider/edit-process.component.ts(1 hunks)src/main/webapp/app/shared/editable-slider/editable-slider.component.html(1 hunks)src/main/webapp/app/shared/editable-slider/editable-slider.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.html(1 hunks)src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.scss(1 hunks)src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/learner-profile/learner-profile.component.html(1 hunks)src/main/webapp/app/shared/user-settings/learner-profile/learner-profile.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html(1 hunks)src/main/webapp/app/shared/user-settings/user-settings.route.ts(1 hunks)src/main/webapp/i18n/de/learnerProfile.json(1 hunks)src/main/webapp/i18n/de/userSettings.json(1 hunks)src/main/webapp/i18n/en/learnerProfile.json(1 hunks)src/main/webapp/i18n/en/userSettings.json(1 hunks)src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts(1 hunks)src/test/javascript/spec/component/learner-profile/component/double-slider.component.spec.ts(1 hunks)src/test/javascript/spec/component/learner-profile/component/edit-process.component.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/main/webapp/app/shared/user-settings/learner-profile/learner-profile.component.html
- src/main/java/de/tum/cit/aet/artemis/atlas/domain/profile/CourseLearnerProfile.java
- src/main/webapp/app/shared/editable-slider/double-slider.component.html
- src/main/webapp/i18n/de/learnerProfile.json
- src/main/webapp/app/shared/editable-slider/edit-process.component.scss
- src/main/webapp/i18n/en/learnerProfile.json
- src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.scss
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/entities/learner-profile.model.tssrc/main/webapp/app/shared/user-settings/learner-profile/learner-profile.component.tssrc/main/webapp/app/shared/user-settings/user-settings.route.tssrc/main/webapp/app/shared/editable-slider/editable-slider.component.tssrc/main/webapp/app/shared/editable-slider/double-slider.component.tssrc/main/webapp/app/learner-profile/service/learner-profile-api.service.tssrc/main/webapp/app/shared/editable-slider/edit-process.component.tssrc/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/userSettings.json
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
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/shared/user-settings/user-settings-container/user-settings-container.component.htmlsrc/main/webapp/app/shared/editable-slider/edit-process.component.htmlsrc/main/webapp/app/shared/editable-slider/editable-slider.component.htmlsrc/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.html
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CourseLearnerProfileDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.javasrc/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/component/learner-profile/component/double-slider.component.spec.tssrc/test/javascript/spec/component/learner-profile/component/edit-process.component.spec.tssrc/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
🪛 Biome (1.9.4)
src/main/webapp/app/shared/editable-slider/edit-process.component.ts
[error] 37-37: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 38-38: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🪛 GitHub Check: client-tests-selected
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
[failure] 4-4:
Cannot find module 'app/atlas/service/learner-profile-api.service' or its corresponding type declarations.
🪛 GitHub Check: client-tests
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
[failure] 4-4:
Cannot find module 'app/atlas/service/learner-profile-api.service' or its corresponding type declarations.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (45)
src/main/webapp/i18n/en/userSettings.json (1)
11-11: LGTM! Appropriate translation key added for the learner profile.The addition of the "learnerProfile" key is consistent with the established pattern in the localization file and properly aligns with the new Learner Profile feature being added to the user settings.
src/main/webapp/app/shared/user-settings/user-settings.route.ts (1)
28-34: LGTM! The route configuration follows Angular best practices.The implementation correctly uses lazy loading for the LearnerProfileComponent, which aligns with the coding guidelines. The route definition is properly structured with the appropriate path and page title.
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1)
25-25: LGTM! The navigation link follows consistent UI patterns.The new learner profile navigation link correctly implements the same styling and structure as other navigation links in the settings menu. It properly uses the Angular router directive and the translation pipe for localization.
src/main/webapp/app/shared/user-settings/learner-profile/learner-profile.component.ts (1)
1-10: Component implementation looks good.The component is correctly defined as a shell that hosts the CourseLearnerProfileComponent. The implementation follows Angular's best practices with proper component metadata configuration.
src/main/webapp/app/shared/editable-slider/editable-slider.component.ts (1)
1-19: Well-structured component with correct use of input() and model() APIs.The component is cleanly implemented using Angular's modern input() and model() APIs. All inputs are properly typed, and the component is correctly defined as standalone with its dependencies imported.
src/main/webapp/app/shared/editable-slider/edit-process.component.html (1)
1-8: Properly implemented HTML template with conditional styling.The template uses ngClass appropriately for conditional styling and maintains good structure for showing different icons based on the editing state.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseLearnerProfileRepository.java (1)
34-40: Well-structured query for retrieving learner profiles by login.The JPQL query is well-formed, respects SQL standards with uppercase keywords, and uses the proper
@Paramannotation for parameters. The method returns aSetwhich is an appropriate choice when duplicates aren't expected and uniqueness matters.src/main/java/de/tum/cit/aet/artemis/atlas/dto/CourseLearnerProfileDTO.java (1)
7-8: Good use of Java record for DTO with clear fields.The record definition follows the guidelines for DTOs with appropriate use of primitive types and meaningful field names in CamelCase.
src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.html (2)
3-8: Correctly implemented course selection with modern Angular syntax.The select element properly uses the new
@fordirective rather than the older*ngForas required by the coding guidelines. The track expression is correctly set tocourse.idfor optimal rendering performance.
12-17: Good use of legend for explaining slider states.The legend clearly explains the meaning of the different colored dots, enhancing user experience through proper visual feedback.
src/main/webapp/app/shared/editable-slider/edit-process.component.ts (3)
7-12: Good enum definition with clear state transitions.The
EditStateTransitionenum has well-defined states that reflect a complete editing workflow: initiating an edit, attempting to save, confirming a save, and aborting the operation.
21-24: Good use of Angular's modern signal-based APIs.The component properly uses the new signal-based APIs (
modelandinput) for reactive data binding, which is in line with current Angular best practices.
46-62: Good implementation of action methods with proper disabled state checks.The
onEdit,onSave, andonAbortmethods correctly check if the component is disabled before performing any action, which prevents unexpected state changes when the component should be inactive.src/main/webapp/app/learner-profile/service/learner-profile-api.service.ts (1)
7-9: Good implementation of profile retrieval method.The method correctly uses the
getmethod from the base service with proper typing to fetch course learner profiles.src/main/webapp/app/shared/editable-slider/double-slider.component.ts (2)
23-29: Good implementation of edit mode with proper class manipulation.The
onEditmethod correctly enables the slider, adds the editing class to the parent container, and updates the current and initial values from their respective signals.
48-65: Well-structured state management in ngOnChanges.The
ngOnChangesmethod correctly handles all possible state transitions by routing them to the appropriate handler methods. This is a clean approach to responding to external state changes.src/test/javascript/spec/component/learner-profile/component/double-slider.component.spec.ts (3)
16-27: Good test setup with proper component initialization.The test correctly configures the component with all required inputs using the TestBed and ComponentFixture APIs.
33-40: Well-structured initialization test with appropriate assertions.The test properly verifies that all component inputs are set correctly after initialization.
72-81: Comprehensive test for the full edit-save workflow.This test effectively verifies the complete edit-save workflow, ensuring that changes persist correctly after a successful save operation. The assertions are clear and cover both the model update and the UI state.
src/test/javascript/spec/component/learner-profile/component/edit-process.component.spec.ts (7)
1-10: Appropriate imports and setup for the EditProcessComponent test.The imports are correctly structured, including Angular's testing utilities and the component under test with its associated type.
11-18: Well-structured test environment setup.The beforeEach block correctly configures the TestBed and initializes the component with the necessary input.
20-22: Good test cleanup practice.Using jest.restoreAllMocks() in afterEach ensures that all mocks are restored after each test, preventing test pollution.
24-28: Complete initialization test.The test properly verifies component creation, initial state of editStateTransition, and the disabled state.
30-35: Well-structured async test for edit state transition.The test correctly verifies the state transition to Edit mode, including proper use of autoDetectChanges and awaiting stability.
37-42: Appropriate test for abort state transition.The test correctly verifies the state transition to Abort mode, following the same pattern as the previous test.
44-50: Consistent testing approach for save state transition.This test maintains the same pattern as the other state transition tests, ensuring consistency in test structure.
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (4)
167-179: Well-structured test setup for PUT requests.The test setup correctly modifies the edit state for all edit process components and ensures cleanup after tests.
180-190: Thorough testing of aimForGradeOrBonus updates.The tests cover both success and error cases for updating the aimForGradeOrBonus attribute.
192-202: Complete tests for timeInvestment updates.The tests appropriately validate both successful and failed updates to the timeInvestment attribute.
204-214: Comprehensive tests for repetitionIntensity updates.The tests thoroughly verify the behavior for both successful and failed updates to the repetitionIntensity attribute.
src/main/webapp/app/shared/editable-slider/double-slider.component.scss (7)
1-4: Appropriate base styling for slider control.The .sliders_control class provides a good foundational styling with proper positioning and spacing.
6-19: Cross-browser compatible styling for slider thumbs.The styles properly handle different browser-specific pseudo-elements for range inputs, ensuring consistent behavior across browsers.
20-28: Consistent primary color for current slider thumb.Using var(--primary) ensures the component adheres to the application's theme and remains consistent with design guidelines.
30-35: Improved usability for number inputs.Making the spin buttons fully visible improves the user experience by providing clear visual cues for incrementing/decrementing values.
37-44: Appropriate positioning and interaction handling for range inputs.The absolute positioning of range inputs and z-index management ensures proper layering of the UI elements.
46-48: Good visual separation for non-editing mode.Making the track background transparent when not in editing mode helps users distinguish between editable and non-editable states.
50-52: Clean UI by hiding unnecessary elements during editing.Hiding the initial slider during editing maintains a clean interface and prevents user confusion.
src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts (7)
1-12: Appropriate imports and dependencies.The component correctly imports the necessary Angular and application-specific modules needed for its functionality.
13-19: Well-structured component definition with proper imports.The component is correctly defined with appropriate selector, template, styles, and imported components.
20-24: Good use of dependency injection.Using Angular's inject function for dependency injection follows modern Angular practices.
38-42: Asynchronous initialization is properly implemented.The ngOnInit method correctly uses async/await syntax for loading profiles before loading courses.
64-75: Proper error handling implementation.The error handling method correctly extracts error information and displays appropriate alerts to the user.
77-95: Well-implemented update method with error handling.The updateAimForGradeOrBonus method includes proper state checking, updates the profile, and handles both success and error cases.
137-139: Efficient profile loading implementation.The loadProfiles method correctly uses async/await to retrieve all profiles in a single API call.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java (1)
71-75: Good practice: Field boundary checks
ThevalidateProfileFieldmethod is a clean way to ensure that fields remain within defined bounds. This helps guard against invalid data and keeps the logic DRY.
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CourseLearnerProfileDTO.java
Show resolved
Hide resolved
...javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
Show resolved
Hide resolved
...javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/user-settings/learner-profile/course-learner-profile.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (8)
42-45: Useconstfor immutable variables.The
errorHeadersvariable is declared withletbut is never reassigned. Useconstfor immutable variables to prevent accidental reassignment.- let errorHeaders = { + const errorHeaders = { 'x-artemisapp-error': 'error.courseLearnerProfileNotFound', 'x-artemisapp-params': 'courseLearnerProfile', };
108-109: Eliminate redundantwhenStable()calls.There are two consecutive calls to
fixture.whenStable()which is redundant. A single call is sufficient to wait for all async operations to complete.fixture.detectChanges(); await fixture.whenStable(); - await fixture.whenStable(); fixture.detectChanges();
125-126: Use consistent assertion methods.The test uses both
toStrictEqualandtoEqualfor object comparisons. It's better to consistently usetoStrictEqualfor more precise equality checks (including property types and object structure).expect(component).toBeTruthy(); expect(component.courses).toStrictEqual(courses); - expect(component.courseLearnerProfiles).toEqual(profiles); + expect(component.courseLearnerProfiles).toStrictEqual(profiles);
131-134: Use TypeScript's strong typing for event objects.The event object is created as a generic Event, but it would be more type-safe to use specific event types when available.
selectCourse(course); - let changeEvent = new Event('change', { bubbles: true, cancelable: false }); + const changeEvent = new Event('change', { bubbles: true, cancelable: false }) as Event; selector.dispatchEvent(changeEvent); expect(component.activeCourse).toBe(course);
137-141: Improve type safety in helper function.The
setupUpdateTestfunction lacks type annotations for its return value and some parameters. Adding these improves code readability and catches type errors early.- function setupUpdateTest(course: number, updateFn: (value: number) => void, attribute: keyof CourseLearnerProfileDTO): CourseLearnerProfileDTO { - let newVal = (profiles[course][attribute] + 1) % 5; - let newProfile = profiles[course]; + function setupUpdateTest(course: number, updateFn: (value: number) => void, attribute: keyof CourseLearnerProfileDTO): CourseLearnerProfileDTO { + const newVal = (profiles[course][attribute] + 1) % 5; + const newProfile: CourseLearnerProfileDTO = { ...profiles[course] }; newProfile[attribute] = newVal; component.activeCourse = course;
150-151: Use more specific spy assertions.The test checks if the spy was called but doesn't verify it was called exactly once. Using
toHaveBeenCalledTimes(1)ortoHaveBeenCalledExactlyOnceWithprovides stronger assertions.req.flush(profile); - expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); - expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledTimes(1); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledWith(profile); expect(component.courseLearnerProfiles[course]).toEqual(profile);
162-164: Use more specific spy assertions in error validation.Similar to the success case, use more specific assertions for spy calls in error validation.
}); - expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); - expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledTimes(1); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledWith(profile); expect(component.courseLearnerProfiles[course]).toEqual(profiles[course]);
180-184: Test cases use hardcoded course IDs.The test cases use a hardcoded course ID (1) while there are multiple courses defined. Consider using a more robust approach by iterating through available courses or using test.each for parameterized tests.
Instead of hardcoding course ID 1, you could use a more robust approach:
- it('should update aimForGradeOrBonus on successful request', () => { - let course = 1; - let newProfile = setupUpdateTest(course, component.updateAimForGradeOrBonus.bind(component), 'aimForGradeOrBonus'); - validateUpdate(course, newProfile); - }); + it.each([1, 2])('should update aimForGradeOrBonus on successful request for course %i', (courseId) => { + const newProfile = setupUpdateTest(courseId, component.updateAimForGradeOrBonus.bind(component), 'aimForGradeOrBonus'); + validateUpdate(courseId, newProfile); + });This pattern can be applied to all similar test cases for more comprehensive coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/component/learner-profile/component/course-learner-profile.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (1)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (1)
76-77: Remove duplicate AlertService provider.There are two different providers for AlertService which is redundant and may cause unexpected behavior.
providers: [ { provide: SessionStorageService, useClass: MockSyncStorage }, - MockProvider(AlertService), { provide: AlertService, useClass: MockAlertService }, { provide: TranslateService, useClass: MockTranslateService }, provideHttpClient(), provideHttpClientTesting(), ],
...javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (2)
75-77:⚠️ Potential issueRemove duplicate AlertService provider.
There are two providers for AlertService which can cause unexpected behavior. One should be removed.
providers: [ { provide: SessionStorageService, useClass: MockSyncStorage }, - MockProvider(AlertService), { provide: AlertService, useClass: MockAlertService }, { provide: TranslateService, useClass: MockTranslateService }, provideHttpClient(), provideHttpClientTesting(), ],
155-165: 🛠️ Refactor suggestionUpdate API endpoint path and verify AlertService error handling.
The API endpoint path should be consistent, and the error handling verification should check that AlertService is called with the error message.
function validateError(course: number, profile: CourseLearnerProfileDTO) { - const req = httpTesting.expectOne(`api/atlas/learner-profiles/course-learner-profiles/${course}`, 'Request to put new Profile'); + const req = httpTesting.expectOne(`api/learner-profiles/course-learner-profiles/${course}`, 'Request to put new Profile'); req.flush(errorBody, { headers: errorHeaders, status: 400, statusText: 'Bad Request', }); - expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledOnce(); - expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledWith(profile); expect(component.courseLearnerProfiles[course]).toEqual(profiles[course]); + const alertService = TestBed.inject(AlertService); + expect(alertService.error).toHaveBeenCalledWith(errorBody.message, errorBody.params); }
🧹 Nitpick comments (5)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (5)
108-110: Remove redundant await fixture.whenStable() call.Two consecutive calls to
fixture.whenStable()are unnecessary as this method returns a promise that resolves when all pending asynchronous activities are complete.await fixture.whenStable(); -await fixture.whenStable(); fixture.detectChanges();
123-125: Use toBeTrue() instead of toBeTruthy() for boolean assertions.According to coding guidelines, boolean assertions should use
toBeTrue()ortoBeFalse()instead oftoBeTruthy()for more specific and clearer expectations.it('should initialize', () => { - expect(component).toBeTruthy(); + expect(component).toBeTrue(); expect(component.courses).toStrictEqual(courses); expect(component.courseLearnerProfiles).toEqual(profiles); });
129-135: Use const instead of let for constants and improve boolean comparison.Variables that aren't reassigned should use
constinstead oflet. Also, the comparison in theselectCoursefunction should use===for strict equality.it('should select active course', () => { - let course = 1; + const course = 1; selectCourse(course); - let changeEvent = new Event('change', { bubbles: true, cancelable: false }); + const changeEvent = new Event('change', { bubbles: true, cancelable: false }); selector.dispatchEvent(changeEvent); expect(component.activeCourse).toBe(course); }); function selectCourse(id: number) { Array.from(selector.options).forEach((opt) => { - opt.selected = opt.value == String(id); + opt.selected = opt.value === String(id); }); }
167-215: Add test for edit mode cancellation.The test suite initializes edit mode and tests the update functionality, but there's no test to verify that cancelling an edit correctly reverts the values without making a PUT request.
Consider adding a test that:
- Enters edit mode
- Changes a value
- Triggers a cancel action
- Verifies that no PUT request is made
- Confirms the value is reverted to the original
it('should not update values when edit is cancelled', () => { const course = 1; component.activeCourse = course; const originalValue = profiles[course].aimForGradeOrBonus; const newValue = (originalValue + 1) % 5; // Enter edit mode editProcessComponents[0].editStateTransition.set(EditStateTransition.EnterEdit); fixture.detectChanges(); // Change value component.updateAimForGradeOrBonus(newValue); // Cancel edit editProcessComponents[0].editStateTransition.set(EditStateTransition.Abort); fixture.detectChanges(); // Verify no request made httpTesting.expectNone(`api/learner-profiles/course-learner-profiles/${course}`); expect(putUpdatedCourseLearnerProfileSpy).not.toHaveBeenCalled(); // Verify value reverted expect(component.courseLearnerProfiles[course].aimForGradeOrBonus).toBe(originalValue); });
180-184: Use const instead of let for constants in test cases.Variables that aren't reassigned should use
constinstead ofletin all test cases.it('should update aimForGradeOrBonus on successful request', () => { - let course = 1; - let newProfile = setupUpdateTest(course, component.updateAimForGradeOrBonus.bind(component), 'aimForGradeOrBonus'); + const course = 1; + const newProfile = setupUpdateTest(course, component.updateAimForGradeOrBonus.bind(component), 'aimForGradeOrBonus'); validateUpdate(course, newProfile); });Make similar changes in all other test cases.
Also applies to: 192-196, 204-208
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/component/learner-profile/component/course-learner-profile.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
...javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
Show resolved
Hide resolved
...javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
Outdated
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
| @RequestMapping("api/atlas/") | ||
| public class LearnerProfileResource { | ||
|
|
||
| private static final int MIN_PROFILE_VALUE = 1; |
There was a problem hiding this comment.
what are min and max profile values? Can they be named in a more descriptive way?
| } | ||
|
|
||
| async putUpdatedCourseLearnerProfile(courseLearnerProfile: CourseLearnerProfileDTO): Promise<CourseLearnerProfileDTO> { | ||
| return this.put<CourseLearnerProfileDTO>(`atlas/course-learner-profiles/${courseLearnerProfile.id}`, courseLearnerProfile); |
There was a problem hiding this comment.
no await here, then you can also remove the async
| pointer-events: all; | ||
| cursor: pointer; | ||
| } | ||
| input[type='range']#currentSlider::-webkit-slider-thumb { |
|
|
||
| loadCourses() { | ||
| this.courses = []; | ||
| //iterat eover each course ID in courseLearnerProfiles map to retrieve course title |
There was a problem hiding this comment.
| //iterat eover each course ID in courseLearnerProfiles map to retrieve course title | |
| //iterate over each course ID in courseLearnerProfiles map to retrieve course title |
helios-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test7.artemis.cit.tum.de
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (1)
108-109: Avoid redundantwhenStable()calls.There are two consecutive calls to
await fixture.whenStable()which is unnecessary. One call is sufficient to wait for all async operations to complete.fixture.detectChanges(); await fixture.whenStable(); - await fixture.whenStable(); fixture.detectChanges();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/component/learner-profile/component/course-learner-profile.component.spec.ts
🪛 GitHub Actions: Test
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts
[error] 125-125: Test failed: expect(component.courses).toStrictEqual(courses) - Expected array with courses but received an empty array.
[error] 134-134: Test failed: expect(component.activeCourse).toBe(course) - Expected: 1, Received: undefined.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/test/javascript/spec/component/learner-profile/component/course-learner-profile.component.spec.ts (4)
76-77: Remove duplicate AlertService provider.There are two different providers for AlertService which is redundant and may cause unexpected behavior in tests.
providers: [ { provide: SessionStorageService, useClass: MockSyncStorage }, - MockProvider(AlertService), { provide: AlertService, useClass: MockAlertService }, { provide: TranslateService, useClass: MockTranslateService }, provideHttpClient(), provideHttpClientTesting(), ],
138-141: Avoid mutating shared test data.The current implementation is modifying the shared
profilesobject directly, which could lead to side effects in other tests. Create a deep copy of the profile object before modifying it.function setupUpdateTest(course: number, updateFn: (value: number) => void, attribute: keyof CourseLearnerProfileDTO): CourseLearnerProfileDTO { let newVal = (profiles[course][attribute] + 1) % 5; - let newProfile = profiles[course]; + let newProfile = { ...profiles[course] }; newProfile[attribute] = newVal; component.activeCourse = course;
147-153: Update API endpoint path to match service import and improve assertions.The API endpoint path includes
/atlas/but the import doesn't reflect this structure. Also, adjust spy assertions according to coding guidelines.function validateUpdate(course: number, profile: CourseLearnerProfileDTO) { - const req = httpTesting.expectOne(`api/atlas/course-learner-profiles/${course}`, 'Request to put new Profile'); + const req = httpTesting.expectOne(`api/learner-profiles/course-learner-profiles/${course}`, 'Request to put new Profile'); req.flush(profile); - expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledOnce(); - expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledWith(profile); expect(component.courseLearnerProfiles[course]).toEqual(profile); }
155-165: Update error validation path and verify AlertService is called.Fix the API endpoint path and add validation for AlertService calls according to the coding guidelines.
function validateError(course: number, profile: CourseLearnerProfileDTO) { - const req = httpTesting.expectOne(`api/atlas/course-learner-profiles/${course}`, 'Request to put new Profile'); + const req = httpTesting.expectOne(`api/learner-profiles/course-learner-profiles/${course}`, 'Request to put new Profile'); req.flush(errorBody, { headers: errorHeaders, status: 400, statusText: 'Bad Request', }); - expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalled(); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledOnce(); - expect(putUpdatedCourseLearnerProfileSpy.mock.calls[0][0]).toEqual(profile); + expect(putUpdatedCourseLearnerProfileSpy).toHaveBeenCalledWith(profile); expect(component.courseLearnerProfiles[course]).toEqual(profiles[course]); + const alertService = TestBed.inject(AlertService); + expect(alertService.error).toHaveBeenCalledWith(errorBody.message, errorBody.params); }
| expect(component).toBeTruthy(); | ||
| expect(component.courses).toStrictEqual(courses); | ||
| expect(component.courseLearnerProfiles).toEqual(profiles); |
There was a problem hiding this comment.
Fix failing test - component.courses is empty.
The test is failing because component.courses is empty. The pipeline shows: "Expected array with courses but received an empty array." Ensure the component correctly initializes courses from the mocked service.
Verify how the component initializes the courses property. You might need to check the component implementation to ensure it properly loads courses from the courseManagerService during initialization.
🧰 Tools
🪛 GitHub Actions: Test
[error] 125-125: Test failed: expect(component.courses).toStrictEqual(courses) - Expected array with courses but received an empty array.
| let course = 1; | ||
| selectCourse(course); | ||
| let changeEvent = new Event('change', { bubbles: true, cancelable: false }); | ||
| selector.dispatchEvent(changeEvent); | ||
| expect(component.activeCourse).toBe(course); | ||
| }); |
There was a problem hiding this comment.
Fix failing test - activeCourse is undefined.
The test is failing because component.activeCourse remains undefined after selection. The pipeline shows: "Expected: 1, Received: undefined."
Check how the component handles the select change event. Make sure the component's method for handling course selection is being called and correctly updates the activeCourse property.
🧰 Tools
🪛 GitHub Actions: Test
[error] 134-134: Test failed: expect(component.activeCourse).toBe(course) - Expected: 1, Received: undefined.
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
#9673 added learner profiles and course specific learner profiles to Artemis. As they will impact the way Artemis interacts with the user, the user should have the ability to review and correct the profile.
Description
This PR adds a new menu entry in the user settings page. The user is presented with the values stored in his learner profile.
A new component, editable slider, was created that is able to show an initial and current state. It is further more able to enter an edit mode to set a new initial value if the user feels, the current state is incorrect.
The user is now able to select a course, review the lerner profile and adjust it according to their preferences.
Steps for Testing
Prerequisites:
Learner ProfileTestserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Client
Server
Screenshots
An example course learner profile.
Editing the profile.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests