Development: Migrate exercise group client code to signals
#10332
Conversation
…ups-client-migration
# Conflicts: # src/main/webapp/app/exam/manage/exercise-groups/exercise-group-update.component.ts # src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts # src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.ts # src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts # src/main/webapp/app/exercises/shared/exam-exercise-row-buttons/exam-exercise-row-buttons.component.ts # src/test/javascript/spec/component/exam/manage/exam-exercise-row-buttons.component.spec.ts # src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group-update.component.spec.ts # src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.spec.ts # src/test/javascript/spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.component.spec.ts # src/test/javascript/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.component.spec.ts # src/test/javascript/spec/component/exam/manage/exercise-groups/programming-exercise-group-cell.component.spec.ts # src/test/javascript/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts
coolchock
changed the title
General: Migrate exercise group module to signals
github-actions
bot
added
tests
client
WalkthroughThis PR refactors several Angular exercise group cell components and their corresponding tests. The changes replace direct property accesses and setter methods with calls to an Changes
Sequence Diagram(s)sequenceDiagram
participant Angular
participant Component
participant Template
Angular->>Component: Initialize component
Angular->>Component: Bind input using input.required<T>()
Component->>Template: Expose exercise() for data access
Template->>Component: Call exercise() to retrieve properties (e.g., type, filePattern)
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
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: 1
🔭 Outside diff range comments (3)
src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts (2)
41-59: 🛠️ Refactor suggestionPrevent memory leaks and improve code readability.
The profile service subscription should be cleaned up on component destruction, and the nested optional chaining could be simplified.
Apply these improvements:
+ private destroy$ = new Subject<void>(); ngOnInit(): void { - this.profileService.getProfileInfo().subscribe((profileInfo) => { + this.profileService.getProfileInfo().pipe( + takeUntil(this.destroy$) + ).subscribe((profileInfo) => { this.localVCEnabled = profileInfo.activeProfiles.includes(PROFILE_LOCALVC); this.onlineIdeEnabled = profileInfo.activeProfiles.includes(PROFILE_THEIA); - const projectKey = this.exercise()?.projectKey; - if (projectKey) { - const solutionParticipation = this.exercise()?.solutionParticipation; + const exercise = this.exercise(); + if (exercise?.projectKey) { + const { solutionParticipation, templateParticipation } = exercise; if (solutionParticipation?.buildPlanId) { - solutionParticipation.buildPlanUrl = createBuildPlanUrl(profileInfo.buildPlanURLTemplate, projectKey, solutionParticipation.buildPlanId); + solutionParticipation.buildPlanUrl = createBuildPlanUrl( + profileInfo.buildPlanURLTemplate, + exercise.projectKey, + solutionParticipation.buildPlanId + ); } - const templateParticipation = this.exercise()?.templateParticipation; if (templateParticipation?.buildPlanId) { - templateParticipation.buildPlanUrl = createBuildPlanUrl(profileInfo.buildPlanURLTemplate, projectKey, templateParticipation.buildPlanId); + templateParticipation.buildPlanUrl = createBuildPlanUrl( + profileInfo.buildPlanURLTemplate, + exercise.projectKey, + templateParticipation.buildPlanId + ); } } }); } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }
61-77: 🛠️ Refactor suggestionAdd error handling and prevent memory leaks in HTTP call.
The HTTP subscription should be cleaned up and error cases should be handled.
Apply these improvements:
downloadRepository(repositoryType: ProgrammingExerciseInstructorRepositoryType): void { const programmingExerciseId = this.exercise()?.id; if (programmingExerciseId) { - this.programmingExerciseService.exportInstructorRepository(programmingExerciseId, repositoryType, undefined).subscribe((response: HttpResponse<Blob>) => { - downloadZipFileFromResponse(response); - this.alertService.success('artemisApp.programmingExercise.export.successMessageRepos'); - }); + this.programmingExerciseService.exportInstructorRepository(programmingExerciseId, repositoryType, undefined).pipe( + take(1) + ).subscribe({ + next: (response: HttpResponse<Blob>) => { + downloadZipFileFromResponse(response); + this.alertService.success('artemisApp.programmingExercise.export.successMessageRepos'); + }, + error: (error) => { + this.alertService.error('artemisApp.programmingExercise.export.errorMessageRepos', error.message); + } + }); } }src/test/javascript/spec/component/exam/manage/exercise-groups/programming-exercise-group-cell.component.spec.ts (1)
103-114: 🛠️ Refactor suggestionAdd error handling test for repository download.
The download repository test only covers the success case. Add tests for error scenarios.
Add error handling test:
it('should handle repository download error', () => { // GIVEN const error = new Error('Download failed'); const exportRepositoryStub = jest.spyOn(mockProgrammingExerciseService, 'exportInstructorRepository') .mockReturnValue(throwError(() => error)); const alertErrorStub = jest.spyOn(mockAlertService, 'error'); // WHEN comp.downloadRepository('TEMPLATE'); // THEN expect(exportRepositoryStub).toHaveBeenCalledOnce(); expect(alertErrorStub).toHaveBeenCalledOnce(); expect(alertErrorStub).toHaveBeenCalledWith( 'artemisApp.programmingExercise.export.errorMessageRepos', error.message ); });
🧹 Nitpick comments (11)
src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.ts (1)
1-1: LGTM! Migration to required inputs improves type safety.The changes align with Angular's new input APIs, providing better type safety and simpler code structure by removing the need for private backing fields and setter methods.
Consider adding a comment explaining that this component is standalone, as it's not immediately obvious from the current code. This helps other developers understand the component's architecture.
Also applies to: 12-12
src/test/javascript/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts (2)
17-27: Improve test assertions using recommended patterns.The test assertions could be more specific as per guidelines.
Apply this diff to improve the assertions:
it('should display number of quiz questions', () => { const exercise: QuizExercise = { id: 1, type: ExerciseType.QUIZ, quizQuestions: [{}, {}], } as any as QuizExercise; fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); - expect(fixture.nativeElement.textContent).toContain('2'); + expect(fixture.nativeElement.textContent.includes('2')).toBeTrue(); });
29-39: Improve negative test case assertions.The negative test case could be more specific.
Apply this diff to improve the assertions:
it('should not display anything for other exercise types', () => { const exercise: QuizExercise = { id: 1, type: ExerciseType.TEXT, quizQuestions: [{}, {}], } as any as QuizExercise; fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); - expect(fixture.nativeElement.textContent).toBe(''); + expect(fixture.nativeElement.textContent.trim()).toBe(''); });src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts (1)
1-38: LGTM! Modern Angular patterns well implemented.The migration to standalone components and signal-based inputs is well executed. The component properly declares its dependencies and uses type-safe inputs.
Consider grouping related inputs together and adding JSDoc comments for better documentation:
+ /** Display-related configuration inputs */ displayShortName = input(false); displayRepositoryUri = input(false); displayTemplateUrls = input(false); displayEditorModus = input(false); + /** Core exercise data input */ exercise = input.required<ProgrammingExercise>();src/test/javascript/spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.component.spec.ts (1)
1-40: LGTM! Tests follow modern Angular patterns.The migration to using
setInputfor component inputs is well implemented. However, consider adding error case tests.Add test for error handling:
it('should handle invalid file pattern gracefully', () => { const exercise: FileUploadExercise = { id: 1, type: ExerciseType.FILE_UPLOAD, filePattern: undefined, } as any as FileUploadExercise; fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); expect(fixture.nativeElement.textContent).toBe(''); });src/test/javascript/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.component.spec.ts (1)
1-42: LGTM! Tests follow modern Angular patterns.The migration to using
setInputand proper test module setup is well implemented. However, consider adding more edge case tests.Add tests for edge cases:
it('should handle undefined diagram type gracefully', () => { const exercise: ModelingExercise = { id: 1, type: ExerciseType.MODELING, diagramType: undefined, } as any as ModelingExercise; fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); expect(fixture.nativeElement.textContent).toBe(''); }); it('should handle null exercise gracefully', () => { fixture.componentRef.setInput('exercise', null); fixture.detectChanges(); expect(fixture.nativeElement.textContent).toBe(''); });src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html (5)
1-2: Top-Level Conditional Checks and Function Calls:
The refactored conditionals now callexercise()anddisplayShortName()instead of directly referencing a component variable. This promotes consistency across the template. If these functions are more than light getters, consider caching their results in a local template variable to avoid redundant calls during change detection cycles.
7-24: Template Participation Repository Block:
The block handling the template participation details consistently replaces direct property accesses with function calls (e.g.,exercise().templateParticipation?.repositoryUri). The use of optional chaining and the non-null assertion in[participation]="exercise().templateParticipation!"appears safe provided the conditional check (line 10) guarantees the presence of the property. This results in cleaner, more maintainable code.
30-44: Solution Participation Block:
The changes for the solution participation section mirror those in the template block. Using expressions likeexercise().solutionParticipation?.repositoryUriand[participation]="exercise().solutionParticipation!"ensures consistency and safe access. Just be sure that the non-null assertions are indeed safe given the preceding conditions.
60-83: Template URLs Block:
This block appropriately displays the build plan links or IDs for template and solution participations based on the availability of a build plan ID. The use of non-null assertions (e.g.,exercise().templateParticipation!) is acceptable here as long as the conditions guarantee that these properties exist.
84-101: Editor Mode Block:
The rendering logic for offline and online editor modes is now driven by thedisplayEditorModus()function, with subsequent data accessed viaexercise(). Conditional translations forallowOfflineIde,allowOnlineEditor, andallowOnlineIdeare handled cleanly. As with earlier sections, evaluate whether repeatedly invokingexercise()might benefit from caching if any performance issues arise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.html(1 hunks)src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.ts(2 hunks)src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.html(1 hunks)src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.ts(2 hunks)src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html(2 hunks)src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts(3 hunks)src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.html(1 hunks)src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.ts(2 hunks)src/test/javascript/spec/component/exam-exercise-row-buttons/exam-exercise-row-buttons.component.spec.ts(1 hunks)src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.component.spec.ts(4 hunks)src/test/javascript/spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.component.spec.ts(1 hunks)src/test/javascript/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.component.spec.ts(1 hunks)src/test/javascript/spec/component/exam/manage/exercise-groups/programming-exercise-group-cell.component.spec.ts(3 hunks)src/test/javascript/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/javascript/spec/component/exam-exercise-row-buttons/exam-exercise-row-buttons.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
`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/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.htmlsrc/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.htmlsrc/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.htmlsrc/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html
`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/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.tssrc/test/javascript/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.component.spec.tssrc/test/javascript/spec/component/exam/manage/exercise-groups/programming-exercise-group-cell.component.spec.tssrc/test/javascript/spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.component.spec.tssrc/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.tssrc/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.tssrc/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.tssrc/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.ts (1)
1-1: LGTM! Consistent implementation with other components.The changes maintain consistency with the quiz component's implementation, following the same pattern for required inputs.
Also applies to: 12-12
src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.ts (1)
1-1: LGTM! Well-structured standalone component.The changes maintain consistency with other components while properly declaring dependencies through imports.
Also applies to: 14-14
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.component.spec.ts (1)
51-59: Great improvements to the test setup!The changes enhance the test configuration by:
- Using standalone components in the imports array
- Leveraging
MockProviderfor better mocking- Using
overrideProviderfor dependency injection- Using
fixture.debugElement.injector.getfor service retrievalAlso applies to: 68-68
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.html (1)
1-2: Excellent use of Angular's new control flow syntax!The changes demonstrate best practices by:
- Using
@ifinstead of*ngIf- Using function calls for data access
- Providing a fallback with the
||operatorsrc/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.html (1)
1-2: Great use of safe navigation operators!The changes demonstrate best practices by:
- Using
@ifinstead of*ngIf- Using optional chaining (
?.) for safe property access- Providing a fallback with the
||operatorsrc/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.html (1)
1-2: Great use of Angular's new control flow syntax and translation pipe!The changes demonstrate best practices by:
- Using
@ifinstead of*ngIf- Using function calls for data access
- Using the
artemisTranslatepipe for internationalizationsrc/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html (2)
4-4: Interpolation Binding for Short Name:
Using the interpolation expression{{ exercise().shortName || '' }}correctly provides a fallback in caseshortNameis null or undefined.
48-57: Test Repository URI Block:
The conditional checking forexercise().testRepositoryUriis straightforward and effectively controls the display of the test repository link. This section adheres to the new approach for fetching exercise details.
...script/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts
Show resolved
Hide resolved
# Conflicts: # src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts
coolchock
dismissed stale reviews from HawKhiem and coderabbitai[bot]
via
dc044d4
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
krusche
changed the title
General: Migrate exercise group module to signalsDevelopment: Migrate exercise group client code to signals
Checklist
General
Client
Description
This pull request migrates exercise group module to signals, inject and standalone
Steps for Testing
Prerequisites:
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Summary by CodeRabbit