Adaptive learning: Fix learner profile settings
#11324
Conversation
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
WalkthroughAdds a duplicate-check in course learner profile creation. Migrates SegmentedToggleComponent to Angular signals (inputs/models), updates its template, styles, and tests accordingly. Adjusts learner profile feedback layout. Updates i18n messages for feedback range validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Service as CourseLearnerProfileService
participant Repo as CourseLearnerProfileRepository
participant LPRepo as LearnerProfileRepository
User->>Service: createCourseLearnerProfile(user, course)
Service->>Repo: findByLoginAndCourse(user.login, course)
alt Profile exists
Repo-->>Service: Optional(existing)
Service-->>User: return existing
else No profile
Repo-->>Service: Optional.empty
Service->>LPRepo: load/create LearnerProfile
Service->>Repo: save(new CourseLearnerProfile)
Repo-->>Service: persisted
Service-->>User: return new profile
end
sequenceDiagram
autonumber
actor User
participant UI as SegmentedToggleComponent
Note over UI: Signals-based API<br/>options(), selected(), maxLines
User->>UI: Click option (value)
UI->>UI: select(value) → selected.set(value)
UI-->>User: Rerender with selected() active
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.spec.ts (1)
46-53: Fix selector: querying non-existent 'jhi-button' makes the test vacuously passTemplate renders native
<button>s. This test will always pass even if buttons exist.- const options = compiled.querySelectorAll('jhi-button'); + const options = compiled.querySelectorAll('button');
🧹 Nitpick comments (10)
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.scss (1)
22-30: Make line clamp configurable to honor maxLines inputRight now the clamp is hardcoded to 2 in CSS. Wire it via a CSS variable so the new maxLines input takes effect.
Apply this diff:
.btn-group .btn .label { - display: -webkit-box; - -webkit-box-orient: vertical; - overflow: hidden; - text-overflow: ellipsis; - white-space: normal; /* allow wrap */ - line-height: 1.2; - -webkit-line-clamp: 2; /* default clamp to 2 lines */ + display: -webkit-box; + -webkit-box-orient: vertical; + overflow: hidden; + text-overflow: ellipsis; + white-space: normal; /* allow wrap */ + line-height: 1.2; + /* Drive clamp via CSS var with sensible fallback */ + -webkit-line-clamp: var(--st-line-clamp, 2); + line-clamp: var(--st-line-clamp, 2); + /* Prevent overflow with long unbreakable strings */ + overflow-wrap: anywhere; }Additionally (outside this file), set the variable in the template where the label is rendered:
<!-- segmented-toggle.component.html --> <span class="label" [style.--st-line-clamp]="maxLines()">{{ option.label }}</span>src/main/webapp/app/core/user/settings/learner-profile/feedback-learner-profile/feedback-learner-profile.component.html (2)
23-23: Optionally pass the new value for clarityIf you use the emitted value, pass $event; otherwise consider removing the handler and reacting via an effect on the bound property.
Apply this diff:
- <jhi-segmented-toggle class="mt-auto" [options]="feedbackDetailOptions" [(selected)]="feedbackDetail" (selectedChange)="onToggleChange()" /> + <jhi-segmented-toggle class="mt-auto" [options]="feedbackDetailOptions" [(selected)]="feedbackDetail" (selectedChange)="onToggleChange($event)" />Remember to update the TS signature accordingly.
28-28: Mirror the event handling choice on the second toggleKeep consistency with the first toggle.
Apply this diff:
- <jhi-segmented-toggle class="mt-auto" [options]="feedbackFormalityOptions" [(selected)]="feedbackFormality" (selectedChange)="onToggleChange()" /> + <jhi-segmented-toggle class="mt-auto" [options]="feedbackFormalityOptions" [(selected)]="feedbackFormality" (selectedChange)="onToggleChange($event)" />src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.html (3)
4-4: Improve a11y semantics for a single-select segmented controlModel the group as a radiogroup and each button as a radio for proper screen reader semantics and focus handling. At minimum, expose state via ARIA.
Apply within this line:
- <button type="button" class="btn" [ngClass]="selected() === option.value ? 'btn-primary' : 'btn-outline-secondary'" (click)="select(option.value)"> + <button + type="button" + class="btn" + [attr.role]="'radio'" + [attr.aria-checked]="selected() === option.value" + [ngClass]="selected() === option.value ? 'btn-primary' : 'btn-outline-secondary'" + (click)="select(option.value)">And outside the changed hunk (container div), set the group role:
<div class="btn-group w-100" role="radiogroup">
3-3: Verify track expression uniqueness
track option.valueassumesvalueis stable and unique. IfTcan be non-primitive or duplicated, updates may misbehave. Confirm all uses pass unique primitive values; otherwise considertrack $indexor expose a stable id.
5-5: Reconsider using title for the same visible label
titleduplicates visible text and often isn't announced; it can also create hover-only tooltips. If labels can wrap (per SCSS), consider droppingtitleor only setting it when truncation occurs.- <span class="label" [attr.title]="option.label">{{ option.label }}</span> + <span class="label">{{ option.label }}</span>src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.spec.ts (2)
40-44: Rename test: it no longer verifies an event emissionThe spec name references
selectedChangebut no event is emitted or asserted. Rename to reflect current behavior.- it('should emit selectedChange event when an option is selected', () => { + it('should update selected when select() is called', () => {
55-66: Add a DOM interaction test to cover click behaviorCurrently selection changes are only tested via direct method calls. Add a click-based test to exercise the template binding.
it('should update selection on button click', () => { fixture.componentRef.setInput('options', mockOptions); fixture.detectChanges(); const compiled = fixture.nativeElement as HTMLElement; const buttons = compiled.querySelectorAll('button'); (buttons[1] as HTMLButtonElement).click(); // click "Option 2" fixture.detectChanges(); expect(component.selected()).toBe(2); expect(compiled.querySelector('.btn-primary')?.textContent?.trim()).toBe('Option 2'); });src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java (2)
39-44: Method now “returns existing” — consider renaming for clarity.
Consider renamingcreateCourseLearnerProfiletofindOrCreateCourseLearnerProfileto reflect idempotent semantics. If renaming is out-of-scope, the updated JavaDoc is fine.
85-100: Avoid O(n^2) scans; pre-index learner profiles and use map lookup.
The innerstream().filter(...).findFirst()is repeated per user. Build a map once and perform O(1) lookups.Apply this diff within the block:
- Set<CourseLearnerProfile> courseProfiles = users.stream().map(user -> courseLearnerProfileRepository.findByLoginAndCourse(user.getLogin(), course).orElseGet(() -> { - - CourseLearnerProfile courseProfile = new CourseLearnerProfile(); - courseProfile.setCourse(course); - LearnerProfile learnerProfile = learnerProfiles.stream().filter(profile -> profile.getUser().equals(user)).findFirst() - .orElseThrow(() -> new IllegalStateException("Learner profile for user " + user.getLogin() + " not found")); - - courseProfile.setLearnerProfile(learnerProfile); - - // Initialize values in the middle of Likert scale - courseProfile.setAimForGradeOrBonus(3); - courseProfile.setRepetitionIntensity(3); - courseProfile.setTimeInvestment(3); - - return courseProfile; - })).collect(Collectors.toSet()); + Set<CourseLearnerProfile> courseProfiles = users.stream().map(user -> + courseLearnerProfileRepository.findByLoginAndCourse(user.getLogin(), course).orElseGet(() -> { + CourseLearnerProfile courseProfile = new CourseLearnerProfile(); + courseProfile.setCourse(course); + LearnerProfile learnerProfile = learnerProfileMap.get(user); + if (learnerProfile == null) { + throw new IllegalStateException("Learner profile for user " + user.getLogin() + " not found"); + } + courseProfile.setLearnerProfile(learnerProfile); + // Initialize values in the middle of Likert scale + courseProfile.setAimForGradeOrBonus(3); + courseProfile.setRepetitionIntensity(3); + courseProfile.setTimeInvestment(3); + return courseProfile; + }) + ).collect(Collectors.toSet());Add before Line 85 (outside selected lines):
// imports: java.util.Map; java.util.function.Function; Map<User, LearnerProfile> learnerProfileMap = learnerProfiles.stream().collect(Collectors.toMap(LearnerProfile::getUser, Function.identity()));Optional (bigger win): prefetch existing course profiles to eliminate per-user SELECTs if repository supports it:
var existingByLogin = courseLearnerProfileRepository .findAllByCourseAndLoginIn(course, users.stream().map(User::getLogin).toList()) .stream().collect(Collectors.toMap(clp -> clp.getLearnerProfile().getUser().getLogin(), Function.identity()));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java(2 hunks)src/main/webapp/app/core/user/settings/learner-profile/feedback-learner-profile/feedback-learner-profile.component.html(1 hunks)src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.html(1 hunks)src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.scss(1 hunks)src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.spec.ts(3 hunks)src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts(2 hunks)src/main/webapp/i18n/de/learnerProfile.json(1 hunks)src/main/webapp/i18n/en/learnerProfile.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/**/*.html
⚙️ CodeRabbit configuration file
@if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
Files:
src/main/webapp/app/core/user/settings/learner-profile/feedback-learner-profile/feedback-learner-profile.component.htmlsrc/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.html
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.spec.tssrc/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
src/main/webapp/i18n/de/**/*.json
⚙️ CodeRabbit configuration file
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".
Files:
src/main/webapp/i18n/de/learnerProfile.json
🧠 Learnings (8)
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java
📚 Learning: 2024-10-10T11:42:23.069Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9443
File: src/test/javascript/spec/component/hestia/git-diff-report/git-diff-modal.component.spec.ts:55-60
Timestamp: 2024-10-10T11:42:23.069Z
Learning: In `git-diff-report-modal.component.spec.ts`, using `fakeAsync` and `tick` does not work for handling asynchronous operations in the tests; alternative methods are needed.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.spec.ts
📚 Learning: 2024-10-20T21:59:11.630Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:9-9
Timestamp: 2024-10-20T21:59:11.630Z
Learning: In Angular templates within the Artemis project (e.g., `src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html`), properties like `selectedFile()`, `readOnlyManualFeedback()`, `highlightDifferences()`, and `course()` are signals. It is appropriate to call these signals directly in the template.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.htmlsrc/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
📚 Learning: 2025-01-18T08:49:30.693Z
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#10147
File: src/main/webapp/app/shared/components/code-button/code-button.component.ts:1-1
Timestamp: 2025-01-18T08:49:30.693Z
Learning: In Angular 19+ projects, component inputs should use the new input() function from angular/core instead of the Input() decorator, as it's part of the new signals architecture that improves performance and type safety.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
📚 Learning: 2024-10-20T22:04:46.906Z
Learnt from: pzdr7
PR: ls1intum/Artemis#9505
File: src/main/webapp/app/exercises/programming/shared/code-editor/monaco/code-editor-monaco.component.html:36-36
Timestamp: 2024-10-20T22:04:46.906Z
Learning: In Angular templates, methods that return Signals (e.g., `feedbackSuggestionsForSelectedFile()` in `code-editor-monaco.component.ts`) can be used directly without performance issues because Signals are optimized for efficient change detection.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
📚 Learning: 2025-03-26T12:29:47.826Z
Learnt from: coolchock
PR: ls1intum/Artemis#10456
File: src/main/webapp/app/exam/overview/summary/exercises/quiz-exam-summary/quiz-exam-summary.component.ts:68-75
Timestamp: 2025-03-26T12:29:47.826Z
Learning: Components with inputs defined using `input.required<T>()` in Angular don't need optional chaining when accessing the properties of the input value, as these inputs are guaranteed to have a value when the component is properly used.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
📚 Learning: 2025-04-22T10:19:41.546Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#10714
File: src/main/webapp/app/quiz/manage/create-buttons/quiz-exercise-create-buttons.component.ts:11-15
Timestamp: 2025-04-22T10:19:41.546Z
Learning: In Angular 19, standalone components are the default, so components with an `imports` array don't need to explicitly declare `standalone: true` property in the component decorator.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
📚 Learning: 2025-05-27T15:39:30.592Z
Learnt from: eylulnc
PR: ls1intum/Artemis#10910
File: src/main/webapp/app/core/user/settings/global-notifications-settings/global-notifications-settings.component.ts:101-101
Timestamp: 2025-05-27T15:39:30.592Z
Learning: In Angular components, constants defined at module level (even if exported) are not accessible in templates. To use constants in templates for conditional rendering or comparisons, they must be assigned to a protected or public property of the component class, like `protected readonly CONSTANT_NAME = CONSTANT_NAME;`.
Applied to files:
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
src/main/webapp/i18n/en/learnerProfile.json (1)
49-51: Aligns message with 3-option scale — LGTMThe updated range (“1 and 3”) matches the three-level toggles.
Please confirm client-side and server-side validation both enforce 1–3 for feedback preferences to avoid mismatched error messages.
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.scss (1)
8-20: Equal-width, vertically stretched buttons — LGTMThe flex layout ensures consistent sizing and alignment across options.
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.ts (2)
11-18: Signals migration — LGTMClean switch to input()/model(); API supports [(selected)] and selectedChange as expected.
14-14: Ensure maxLines affects renderingThe input exists but only takes effect if the template binds it (e.g., via a CSS var). See SCSS comment; please connect it in the HTML.
Would you like me to open a follow-up patch wiring maxLines in the template?
src/main/webapp/i18n/de/learnerProfile.json (1)
49-51: Deutsch: informell und korrekt — LGTMText uses “du/dein” and matches the 1–3 scale.
src/main/webapp/app/core/user/settings/learner-profile/feedback-learner-profile/feedback-learner-profile.component.html (1)
18-29: Layout adjustments for bottom-aligned toggles — LGTMResponsive row/column setup with d-flex + mt-auto reads well and matches the new segmented-toggle styling.
src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.html (1)
3-5: Signals + @for migration looks correctUsing
@for (... of options(); track option.value)and reading signals viaselected()aligns with the Angular signals pattern used in Artemis templates. Thetype="button"prevents unintended form submits. Looks good.src/main/webapp/app/shared/segmented-toggle/segmented-toggle.component.spec.ts (3)
25-32: Good: inputs via setInput() and signal accessInitializing with
fixture.componentRef.setInput('options', ...)and asserting viacomponent.options()matches the new API.
35-38: Good: signal write/read path covered
component.selected.set(2)thencomponent.selected()verifies signal plumbing.
68-77: Good: visual state assertion via Bootstrap classAsserting
.btn-primaryon the active option is appropriate given the template'sngClass.src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java (1)
3-3: LGTM: Optional import is appropriate.
src/main/java/de/tum/cit/aet/artemis/atlas/service/profile/CourseLearnerProfileService.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
| @Output() selectedChange = new EventEmitter<T>(); | ||
| options = input<{ label: string; value: T }[]>([]); | ||
| selected = model<T>(); | ||
| maxLines = input(2); |
There was a problem hiding this comment.
Is this input used anywhere? if not, maybe can be deleted
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
maximiliansoelch
had a problem deploying
to
https://artemis-test1.artemis.cit.tum.de
GitHub Actions
Failure
Checklist
General
Server
Client
Motivation and Context
Learner profile interface had some small flaws regarding UI/UX, and was raising an internal service error due to a duplicate object generation trial on CourseLearnerProfileResource
Description
I fixed the problems on the client side and also the CourseLearnerProfileResource
Steps for Testing
Prerequisites:
Testserver 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
Test Coverage
Screenshots
Summary by CodeRabbit
Style
Bug Fixes