Adaptive learning: Refactor competencies management page to signals
#9629
Conversation
github-actions
bot
added
tests
client
JohannesWt
changed the title
Adaptive Learning: Refactor competencies management page to signalsAdaptive learning: Refactor competencies management page to signals
JohannesWt
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces significant updates to 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/course/competencies/competency-management/competency-management.component.html (1)
42-43: LGTM! Proper signal-based input bindings for prerequisites table.
All input properties correctly use function calls for signal-based reactivity.
Consider extracting the common table configuration into a reusable interface or type to reduce duplication:
interface CompetencyTableConfig {
courseId: () => number;
courseCompetencies: () => CourseCompetency[];
competencyType: CourseCompetencyType;
standardizedCompetenciesEnabled: () => boolean;
}Then in the template, you could use a structural directive or component property to render tables based on configurations:
readonly tableConfigs: CompetencyTableConfig[] = [
{
courseId: this.courseId,
courseCompetencies: this.competencies,
competencyType: CourseCompetencyType.COMPETENCY,
standardizedCompetenciesEnabled: this.standardizedCompetenciesEnabled
},
{
courseId: this.courseId,
courseCompetencies: this.prerequisites,
competencyType: CourseCompetencyType.PREREQUISITE,
standardizedCompetenciesEnabled: this.standardizedCompetenciesEnabled
}
];Also applies to: 45-45
src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts (1)
176-194: Enhance error handling tests with message verification
While the error handling tests are good, they could be more specific by verifying the error message content:
it('should show alert when loading iris settings fails', async () => {
const errorSpy = jest.spyOn(alertService, 'error');
- getIrisSettingsSpy.mockRejectedValueOnce({});
+ const errorMessage = 'Failed to load IRIS settings';
+ getIrisSettingsSpy.mockRejectedValueOnce(new Error(errorMessage));
fixture.detectChanges();
await fixture.whenStable();
- expect(errorSpy).toHaveBeenCalledOnce();
+ expect(errorSpy).toHaveBeenCalledExactlyOnceWith('artemis.competencyManagement.error.loadIrisSettings');
});src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts (1)
117-118: Avoid unnecessary wrapping of values in new signals
Creating new signals with signal<number>(this.courseId()) when passing data to child components may be redundant since courseId is already a signal. Passing the existing signal or the value directly can simplify the code and avoid unnecessary nesting.
Adjust the code as follows:
-modalRef.componentInstance.courseId = signal<number>(this.courseId());
+modalRef.componentInstance.courseId = this.courseId;
-modalRef.componentInstance.courseCompetencies = signal<CourseCompetency[]>(this.courseCompetencies());
+modalRef.componentInstance.courseCompetencies = this.courseCompetencies;This ensures that the child components receive the current signals without creating additional layers.
Also applies to: 129-129
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/webapp/app/course/competencies/competency-management/competency-management.component.html (2 hunks)
- src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts (7 hunks)
- src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/course/competencies/competency-management/competency-management.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-management/competency-management.component.ts (1)
src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (6)
src/main/webapp/app/course/competencies/competency-management/competency-management.component.html (3)
11-12: LGTM! Proper usage of new Angular control flow syntax.
The @if directive is correctly used here, replacing the older *ngIf syntax as per guidelines. The function calls indicate proper signal usage.
27-33: LGTM! Proper loading state handling with accessibility support.
The loading spinner is correctly implemented with:
- New @if syntax
- Proper ARIA attributes for accessibility
- Signal-based loading state
35-36: LGTM! Proper signal-based input bindings for competency table.
All input properties correctly use function calls for signal-based reactivity.
Also applies to: 38-38
src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts (3)
34-35: LGTM: Proper mock service imports
The imports follow the best practices by using specific mock services instead of full module imports.
95-130: LGTM: Well-structured test setup
The test setup follows best practices with:
- Proper service injection
- Well-configured spies
- Clear test data structure
228-239: LGTM: Well-structured modal test
The test properly verifies the modal interaction and competency updates using signals. The assertions follow the coding guidelines for checking array length.
...pt/spec/component/competencies/competency-management/competency-management.component.spec.ts
Outdated
Show resolved
Hide resolved
| it('should set isLoading correctly', async () => { | ||
| const isLoadingSpy = jest.spyOn(component.isLoading, 'set'); | ||
|
|
||
| fixture.detectChanges(); | ||
| await fixture.whenStable(); | ||
|
|
||
| expect(isLoadingSpy).toHaveBeenNthCalledWith(1, true); | ||
| expect(isLoadingSpy).toHaveBeenNthCalledWith(2, false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider testing observable state instead of setter
The test is tightly coupled to implementation details by spying on the isLoading setter. Consider testing the observable state directly:
-it('should set isLoading correctly', async () => {
- const isLoadingSpy = jest.spyOn(component.isLoading, 'set');
+it('should handle loading state correctly', async () => {
+ const loadingStates: boolean[] = [];
+ component.isLoading().subscribe((state) => loadingStates.push(state));
fixture.detectChanges();
await fixture.whenStable();
- expect(isLoadingSpy).toHaveBeenNthCalledWith(1, true);
- expect(isLoadingSpy).toHaveBeenNthCalledWith(2, false);
+ expect(loadingStates).toEqual([true, false]);
});📝 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.
| it('should set isLoading correctly', async () => { | |
| const isLoadingSpy = jest.spyOn(component.isLoading, 'set'); | |
| fixture.detectChanges(); | |
| await fixture.whenStable(); | |
| expect(isLoadingSpy).toHaveBeenNthCalledWith(1, true); | |
| expect(isLoadingSpy).toHaveBeenNthCalledWith(2, false); | |
| }); | |
| it('should handle loading state correctly', async () => { | |
| const loadingStates: boolean[] = []; | |
| component.isLoading().subscribe((state) => loadingStates.push(state)); | |
| fixture.detectChanges(); | |
| await fixture.whenStable(); | |
| expect(loadingStates).toEqual([true, false]); | |
| }); |
...main/webapp/app/course/competencies/competency-management/competency-management.component.ts
Show resolved
Hide resolved
| const combinedCourseSettings = await firstValueFrom(this.irisSettingsService.getCombinedCourseSettings(this.courseId())); | ||
| this.irisCompetencyGenerationEnabled.set(combinedCourseSettings?.irisCompetencyGenerationSettings?.enabled ?? false); | ||
| } catch (error) { | ||
| this.alertService.error(error); |
There was a problem hiding this comment.
Provide user-friendly error messages in alerts
Passing the error object directly to this.alertService.error may not display meaningful information to the user. It could show as [object Object]. Extract a user-friendly message from the error object or provide a custom error message to enhance the user's understanding.
Update the error handling as follows:
-this.alertService.error(error);
+const errorMessage = error.message ?? 'An unexpected error occurred';
+this.alertService.error(errorMessage);Or use the AlertService's capability to handle error objects if available.
Also applies to: 105-105
...main/webapp/app/course/competencies/competency-management/competency-management.component.ts
Show resolved
Hide resolved
...main/webapp/app/course/competencies/competency-management/competency-management.component.ts
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
...pt/spec/component/competencies/competency-management/competency-management.component.spec.ts
Outdated
Show resolved
Hide resolved
zagemello
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
SindiBuklaji
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
kevinfischer4
added
deploy:artemis-test1
deploy:artemis-test2
and removed
deploy:artemis-test1
deploy:artemis-test2
labels
Checklist
General
Client
Motivation and Context
According to the new client coding guidelines Artemis should move to using signals step by step. This PR refactors the competency management page to using signals.
Description
This PR changes the current competency management page to use signals. Visually nothing changed.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Screenshots
unchanged
Summary by CodeRabbit
New Features
Bug Fixes
Tests