General: Migrate exercise group module to signals, inject and standalone
#9891
Conversation
github-actions
bot
added
tests
client
coolchock
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces a refactor of several Angular components and services related to exercise management. It replaces constructor-based dependency injection with Angular's Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 8
🧹 Outside diff range and nitpick comments (29)
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.html (1)
1-3: Consider optimizing multiple exercise() calls.The
exercise()function is called twice in quick succession. Consider storing the result in a variable to avoid potential redundant computations.Here's a suggested optimization:
-@if (exercise().type === exerciseType.FILE_UPLOAD) { - {{ exercise().filePattern || '' }} +@if ((exerciseData = exercise()).type === exerciseType.FILE_UPLOAD) { + {{ exerciseData.filePattern || '' }} }Note: This assumes you have declared
exerciseDataas a property in your component. If not, you might need to add it:exerciseData?: FileUploadExercise;src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.html (1)
2-2: Consider enhancing the display text for better UX.The current implementation only shows a number without context. Consider making it more descriptive for better user experience.
- {{ exercise().quizQuestions?.length || 0 }} + <span>Questions: {{ exercise().quizQuestions?.length || 0 }}</span>src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.html (1)
1-3: Consider adding null safety checksThe current implementation might be vulnerable to null/undefined values. Consider adding safety checks:
-@if (exercise().type === exerciseType.MODELING) { +@if (exercise() && exercise().type === exerciseType.MODELING) { {{ 'artemisApp.DiagramType.' + exercise().diagramType | artemisTranslate }} }Additionally, you might want to consider using the nullish coalescing operator for the diagram type:
- {{ 'artemisApp.DiagramType.' + exercise().diagramType | artemisTranslate }} + {{ 'artemisApp.DiagramType.' + (exercise().diagramType ?? 'UNKNOWN') | artemisTranslate }}src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.ts (1)
Line range hint
6-10: Consider migrating to a standalone component.Since this PR aims to modernize the architecture, consider converting this component to a standalone component. This would:
- Reduce bundle size through better tree-shaking
- Make dependencies explicit
- Align with Angular's modern architectural practices
Example migration:
@Component({ selector: 'jhi-quiz-exercise-group-cell', templateUrl: './quiz-exercise-group-cell.component.html', styles: [':host{display: contents}'], + standalone: true, + imports: [] // Add required imports here })src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.ts (1)
Line range hint
6-10: Consider adding component documentation.While the component is well-structured, adding a brief JSDoc comment describing its purpose and usage would improve maintainability.
Add documentation above the @component decorator:
+/** + * Renders a file upload exercise within an exercise group cell. + * Used in the exam management interface to display and manage file upload exercises. + */ @Component({ selector: 'jhi-file-upload-exercise-group-cell', templateUrl: './file-upload-exercise-group-cell.component.html', styles: [':host{display: contents}'], })src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.spec.ts (1)
19-36: Enhance test coverage and readability.While the happy path is well tested, consider these improvements:
- Add error case testing
- Make the test description more specific
- Use constants for test data
- Add explicit response handling verification
Here's an improved version:
- it('should set additional parameters correctly in delete', () => { + const TEST_DATA = { + courseId: 1, + examId: 2, + exerciseGroupId: 3, + }; + + it('should send DELETE request with correct parameters when deleting exercise group', () => { const courseId = 1; const examId = 2; const exerciseGroupId = 3; const deleteStudentReposBuildPlans = true; const deleteBaseReposBuildPlans = false; - service.delete(courseId, examId, exerciseGroupId, deleteStudentReposBuildPlans, deleteBaseReposBuildPlans).subscribe(); + service + .delete(courseId, examId, exerciseGroupId, deleteStudentReposBuildPlans, deleteBaseReposBuildPlans) + .subscribe({ + complete: () => expect(true).toBe(true), + }); const req = httpMock.expectOne( `api/courses/${courseId}/exams/${examId}/exercise-groups/${exerciseGroupId}?deleteStudentReposBuildPlans=true&deleteBaseReposBuildPlans=false`, ); expect(req.request.method).toBe('DELETE'); expect(req.request.params.get('deleteStudentReposBuildPlans')).toBe('true'); expect(req.request.params.get('deleteBaseReposBuildPlans')).toBe('false'); req.flush(null); }); + + it('should handle error when delete request fails', () => { + const errorMessage = 'Delete failed'; + + service + .delete(TEST_DATA.courseId, TEST_DATA.examId, TEST_DATA.exerciseGroupId, true, false) + .subscribe({ + error: (error) => expect(error.message).toBe(errorMessage), + }); + + const req = httpMock.expectOne( + `api/courses/${TEST_DATA.courseId}/exams/${TEST_DATA.examId}/exercise-groups/${TEST_DATA.exerciseGroupId}?deleteStudentReposBuildPlans=true&deleteBaseReposBuildPlans=false`, + ); + + req.flush( + { message: errorMessage }, + { status: 404, statusText: 'Not Found' } + ); + });src/test/javascript/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts (1)
Line range hint
8-46: Enhance test coverage with additional scenariosThe current test suite covers basic functionality but would benefit from additional test cases:
- Edge cases (0 questions, undefined exercise)
- Error handling scenarios
- Component lifecycle tests
Consider adding these test cases:
it('should handle undefined exercise gracefully', () => { fixture.componentRef.setInput('exercise', undefined); fixture.detectChanges(); expect(fixture.nativeElement.querySelector('.question-count')).toBeNull(); }); it('should handle quiz with no questions', () => { const exercise = new QuizExercise({ id: 1, title: 'Empty Quiz', type: ExerciseType.QUIZ, quizQuestions: [], maxPoints: 0, duration: 300 }); fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); expect(fixture.nativeElement.querySelector('.question-count').textContent).toBe('0'); }); it('should update display when exercise input changes', () => { const exercise = new QuizExercise({ id: 1, type: ExerciseType.QUIZ, quizQuestions: [{ id: 1 }] }); fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); expect(fixture.nativeElement.querySelector('.question-count').textContent).toBe('1'); exercise.quizQuestions.push({ id: 2 }); fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); expect(fixture.nativeElement.querySelector('.question-count').textContent).toBe('2'); });src/test/javascript/spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.component.spec.ts (2)
Line range hint
25-33: Improve test robustness and type safetySeveral improvements can be made to enhance the test quality:
- Avoid using
as anytype casting- Create a complete exercise object
- Use more specific expectations
Consider this refactor:
- const exercise: FileUploadExercise = { - id: 1, - type: ExerciseType.FILE_UPLOAD, - filePattern: '*.pdf', - } as any as FileUploadExercise; + const exercise = new FileUploadExercise(); + exercise.id = 1; + exercise.type = ExerciseType.FILE_UPLOAD; + exercise.filePattern = '*.pdf'; + exercise.title = 'Test Exercise'; fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); - expect(fixture.nativeElement.textContent).toContain(exercise.filePattern); + expect(fixture.nativeElement.textContent.trim()).toBe(exercise.filePattern);
Line range hint
8-45: Consider adding more test cases for better coverageThe test suite would benefit from additional test cases to cover:
- Undefined/null exercise input
- Empty file pattern
- Invalid file pattern
- Component initialization state
Would you like me to provide example implementations for these additional test cases?
src/test/javascript/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.component.spec.ts (3)
Line range hint
1-23: Refactor test setup to use NgMocks and avoid full module importsThe current setup imports the full
ArtemisTestModulewhich violates our coding guidelines. Consider refactoring to use NgMocks and mock only the required dependencies.Here's the suggested implementation:
-import { ArtemisTestModule } from '../../../../test.module'; +import { MockBuilder, MockRender } from '@ng-mocks'; +import { ModelingExerciseGroupCellComponent } from 'app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component'; describe('Modeling Exercise Group Cell Component', () => { let fixture: ComponentFixture<ModelingExerciseGroupCellComponent>; beforeEach(() => { - TestBed.configureTestingModule({ - imports: [ArtemisTestModule], - declarations: [ModelingExerciseGroupCellComponent, TranslatePipeMock], - providers: [], - }) - .compileComponents() - .then(() => { - fixture = TestBed.createComponent(ModelingExerciseGroupCellComponent); - }); + return MockBuilder(ModelingExerciseGroupCellComponent) + .mock(TranslatePipeMock) + .then(() => { + fixture = MockRender(ModelingExerciseGroupCellComponent); + }); });
Line range hint
26-34: Improve test case robustness and type safetyThe test case has several areas for improvement:
- Avoid
as anytype casting- Use more specific expectations
- Avoid string concatenation in expectations
Here's the suggested implementation:
it('should display diagram type', () => { - const exercise: ModelingExercise = { + const exercise = new ModelingExercise({ id: 1, type: ExerciseType.MODELING, diagramType: UMLDiagramType.ClassDiagram, - } as any as ModelingExercise; + }); fixture.componentRef.setInput('exercise', exercise); fixture.detectChanges(); - expect(fixture.nativeElement.textContent).toContain('artemisApp.DiagramType.' + exercise.diagramType); + const expectedText = `artemisApp.DiagramType.${UMLDiagramType.ClassDiagram}`; + expect(fixture.nativeElement.textContent).toContainEqual(expectedText); });
Line range hint
1-46: Good migration tosetInput, but test suite needs modernizationThe changes to use
fixture.componentRef.setInputare a step in the right direction. However, the test suite would benefit from a broader modernization effort to align with current best practices and coding guidelines.Consider:
- Using NgMocks for better isolation
- Implementing stronger typing
- Using more specific expectations
- Adding test cases for edge cases and error scenarios
src/main/webapp/app/exam/manage/exercise-groups/exercise-group-update.component.ts (3)
Line range hint
1-16: Consider enhancing type safety for route parametersThe component looks well-structured, but we could improve type safety for route parameters using ParamMap type.
- import { ActivatedRoute, Router } from '@angular/router'; + import { ActivatedRoute, ParamMap, Router } from '@angular/router';
Line range hint
34-57: Enhance type safety and error handlingSeveral improvements could make the code more robust:
- Replace type assertions with proper null checks
- Add parameter validation
- Implement more specific error handling
ngOnInit(): void { - this.courseId = Number(this.route.snapshot.paramMap.get('courseId')); + const courseId = this.route.snapshot.paramMap.get('courseId'); + if (!courseId) { + this.alertService.error('error.courseIdMissing'); + this.previousState(); + return; + } + this.courseId = Number(courseId); } save() { this.isSaving = true; + if (!this.exam?.id) { + this.onSaveError(new HttpErrorResponse({ + error: 'Exam ID is required', + status: 400 + })); + return; + } if (this.exerciseGroup.id !== undefined) { - this.subscribeToSaveResponse(this.exerciseGroupService.update(this.courseId, this.exam.id!, this.exerciseGroup)); + this.subscribeToSaveResponse(this.exerciseGroupService.update(this.courseId, this.exam.id, this.exerciseGroup)); } else { this.exerciseGroup.exam = this.exam; - this.subscribeToSaveResponse(this.exerciseGroupService.create(this.courseId, this.exam.id!, this.exerciseGroup)); + this.subscribeToSaveResponse(this.exerciseGroupService.create(this.courseId, this.exam.id, this.exerciseGroup)); } }
Line range hint
71-74: Improve error handling specificityThe current error handling could be more informative for users and easier to debug.
private onSaveError(error: HttpErrorResponse) { - onError(this.alertService, error); + const errorMessage = error.status === 400 + ? 'exerciseGroup.error.validation' + : error.status === 403 + ? 'exerciseGroup.error.forbidden' + : 'exerciseGroup.error.unknown'; + this.alertService.error(errorMessage, error.message, { param: this.exerciseGroup.id }); this.isSaving = false; }src/main/webapp/app/exam/manage/exercise-groups/exercise-group.service.ts (3)
13-13: Consider making the resource URL configurableThe hardcoded API URL could make it challenging to switch between different environments (development, staging, production). Consider moving this to an environment configuration file.
- public resourceUrl = 'api/courses'; + public resourceUrl = environment.apiUrl + '/courses';
Line range hint
20-24: Consider enhancing error handling and memory managementWhile the HTTP methods are well-implemented, consider these improvements:
- Add error handling using catchError operator
- Add request timeout using timeoutWith operator
- Implement proper cleanup using take/takeUntil operators when subscribing
Example implementation for the create method:
create(courseId: number, examId: number, exerciseGroup: ExerciseGroup): Observable<EntityResponseType> { return this.http.post<ExerciseGroup>( `${this.resourceUrl}/${courseId}/exams/${examId}/exercise-groups`, exerciseGroup, { observe: 'response' } ).pipe( timeoutWith(30000, throwError(() => new Error('Request timed out'))), catchError(error => { console.error('Error creating exercise group:', error); return throwError(() => error); }) ); }Also applies to: 29-33, 38-41, 49-58, 64-67
Line range hint
6-7: Enhance type safety with more specific response typesConsider creating more specific response types that include potential error states and null checks.
type ApiError = { message: string; status: number; }; type EntityResponse<T> = HttpResponse<T | null>; type EntityResponseType = EntityResponse<ExerciseGroup>; type EntityArrayResponseType = EntityResponse<ExerciseGroup[]>;src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts (2)
1-1: Consider migrating to standalone componentSince the PR objectives include migration to standalone components, consider updating this component to use the standalone architecture. This would involve:
- Adding
standalone: trueto the component decorator- Moving imports to the component's imports array
- Removing the component from its NgModule declarations
41-50: Consider extracting URL creation logicThe buildPlanUrl creation logic is duplicated for both solution and template participations. Consider extracting this into a helper method to improve maintainability and reduce duplication.
+ private updateParticipationBuildPlanUrl(participation: any, projectKey: string, urlTemplate: string): void { + if (participation?.buildPlanId) { + participation.buildPlanUrl = createBuildPlanUrl(urlTemplate, projectKey, participation.buildPlanId); + } + } ngOnInit(): void { this.profileService.getProfileInfo().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; - if (solutionParticipation?.buildPlanId) { - solutionParticipation.buildPlanUrl = createBuildPlanUrl(profileInfo.buildPlanURLTemplate, projectKey, solutionParticipation.buildPlanId); - } - const templateParticipation = this.exercise()?.templateParticipation; - if (templateParticipation?.buildPlanId) { - templateParticipation.buildPlanUrl = createBuildPlanUrl(profileInfo.buildPlanURLTemplate, projectKey, templateParticipation.buildPlanId); - } + this.updateParticipationBuildPlanUrl(this.exercise()?.solutionParticipation, projectKey, profileInfo.buildPlanURLTemplate); + this.updateParticipationBuildPlanUrl(this.exercise()?.templateParticipation, projectKey, profileInfo.buildPlanURLTemplate); } }); }src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html (1)
10-20: Consider DRY principle for repository link handlingThe template and solution repository sections have very similar code structures. Consider extracting this pattern into a reusable template.
+ <!-- Create a new ng-template --> + <ng-template #repositoryLink let-type="type" let-uri="uri"> + @if (!localVCEnabled) { + <span> + <a [href]="uri" target="_blank">{{type}}</a> + </span> + } @else { + <a [routerLink]="" (click)="downloadRepository(type)"> + <fa-icon [icon]="faDownload" /> {{type}} + </a> + } + </ng-template> + <!-- Use it in both places --> + <ng-container + *ngTemplateOutlet="repositoryLink; + context: { + type: 'TEMPLATE', + uri: exercise().templateParticipation?.repositoryUri + }"> + </ng-container>Also applies to: 30-38
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group-update.component.spec.ts (1)
Line range hint
69-124: Consider adding tests for edge casesWhile the existing test cases cover the main scenarios well, consider adding tests for:
- Validation error scenarios
- Component initialization failures
- Edge cases in exercise group data
This would provide more comprehensive test coverage for the component.
Example test case structure:
it('should handle initialization errors', fakeAsync(() => { const error = new Error('Initialization failed'); route.data = throwError(() => error); fixture = TestBed.createComponent(ExerciseGroupUpdateComponent); fixture.detectChanges(); tick(); expect(alertServiceStub).toHaveBeenCalledOnce(); }));src/test/javascript/spec/component/exam/manage/exercise-groups/programming-exercise-group-cell.component.spec.ts (3)
64-64: Good use of setInput, but consider improving test module setup.While the change to use
setInputis correct, there are some improvements that could be made to the test setup:
- Consider using
MockModuleinstead of importing the fullArtemisTestModule- Explicitly mock HTTP calls in
ProgrammingExerciseServiceExample refactor:
// Replace full module import with mock TestBed.configureTestingModule({ imports: [NgMocks.mockModule(ArtemisTestModule)], // ... rest of the configuration }); // Explicitly mock HTTP calls mockProgrammingExerciseService = { exportInstructorRepository: jest.fn().mockReturnValue(of(new HttpResponse<Blob>())), // ... other methods };
Line range hint
77-91: Enhance test assertions for better specificity.While the input setting is correct, the assertions could be more specific according to our testing guidelines:
- Use
toBeTrue()/toBeFalse()for boolean checks- Use
toEqualfor exact matches- Consider using
toHaveText()for more precise text content matchingExample improvements:
// For the short name test expect(div).not.toBeNull(); expect(div.nativeElement).toHaveText(exercise.shortName); // For the repository URL test expect(span).toBeDefined(); expect(span.nativeElement).toHaveText('Template'); expect(span.nativeElement.getAttribute('href')).toEqual(exercise.templateParticipation!.repositoryUri);
Line range hint
94-107: Improve editor mode flags test structure and translations.The test could be improved in several ways:
- Use translation service properly for text expectations
- Reduce duplication in assertions
- Make text content checks more precise
Suggested refactor:
it('should display editor mode flags', () => { const translateService = TestBed.inject(TranslateService); fixture.componentRef.setInput('displayEditorModus', true); fixture.detectChanges(); const testCases = [ { selector: 'div:first-child', mode: 'offlineIde', value: exercise.allowOfflineIde }, { selector: 'div:nth-child(2)', mode: 'onlineEditor', value: exercise.allowOnlineEditor }, { selector: 'div:nth-child(3)', mode: 'onlineIde', value: exercise.allowOnlineIde } ]; testCases.forEach(({ selector, mode, value }) => { const div = fixture.debugElement.query(By.css(`div > div > ${selector}`)); const expectedText = translateService.instant(`artemisApp.programmingExercise.${mode}`) + translateService.instant(`artemisApp.exercise.${value ? 'yes' : 'no'}`); expect(div).not.toBeNull(); expect(div.nativeElement).toHaveText(expectedText); }); });src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts (4)
43-51: Consider adding readonly modifier to injected servicesWhile the migration to
inject()function is good, consider addingreadonlymodifier to prevent accidental reassignment of these service references.- private route = inject(ActivatedRoute); + private readonly route = inject(ActivatedRoute); - private exerciseGroupService = inject(ExerciseGroupService); + private readonly exerciseGroupService = inject(ExerciseGroupService); - exerciseService = inject(ExerciseService); + readonly exerciseService = inject(ExerciseService);
Line range hint
89-102: Add subscription management to prevent memory leaksThe
forkJoinsubscription inngOnInitshould be managed to prevent memory leaks. Consider using thetakeUntilDestroyedoperator or implementingOnDestroy.+ private readonly destroy$ = new Subject<void>(); + ngOnInit(): void { this.courseId = Number(this.route.snapshot.paramMap.get('courseId')); this.examId = Number(this.route.snapshot.paramMap.get('examId')); // Only take action when a response was received for both requests forkJoin([this.loadExerciseGroups(), this.loadLatestIndividualEndDateOfExam()]).subscribe({ next: ([examRes, examInfoDTO]) => { this.exam = examRes.body!; this.exerciseGroups = this.exam.exerciseGroups; this.course = this.exam.course!; this.latestIndividualEndDate = examInfoDTO ? examInfoDTO.body!.latestIndividualEndDate : undefined; this.setupExerciseGroupToExerciseTypesDict(); }, error: (res: HttpErrorResponse) => onError(this.alertService, res), - }); + }).pipe(takeUntil(this.destroy$)); + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }
Line range hint
89-102: Consider migrating to Angular signals for better state managementAs mentioned in the PR objectives about migrating to signals, this component's state management could benefit from using signals for
exerciseGroups,exam, and other state properties.Example implementation:
export class ExerciseGroupsComponent implements OnInit { private readonly exerciseGroups = signal<ExerciseGroup[]>([]); private readonly exam = signal<Exam | null>(null); // Computed values readonly hasExerciseGroups = computed(() => this.exerciseGroups().length > 0); ngOnInit(): void { forkJoin([/*...*/]).subscribe({ next: ([examRes, examInfoDTO]) => { this.exam.set(examRes.body!); this.exerciseGroups.set(examRes.body!.exerciseGroups); // ... } }); } }
Line range hint
196-208: Improve error handling in deleteExerciseGroupThe error handling in
deleteExerciseGroupupdates UI state before the server request completes. Consider moving the state updates to the success callback.deleteExerciseGroup(exerciseGroupId: number, event: { [key: string]: boolean }) { this.exerciseGroupService.delete(this.courseId, this.examId, exerciseGroupId, event.deleteStudentReposBuildPlans, event.deleteBaseReposBuildPlans).subscribe({ next: () => { + this.exerciseGroups = this.exerciseGroups!.filter((exerciseGroup) => exerciseGroup.id !== exerciseGroupId); + this.exerciseGroupToExerciseTypesDict.delete(exerciseGroupId); this.eventManager.broadcast({ name: 'exerciseGroupOverviewModification', content: 'Deleted an exercise group', }); this.dialogErrorSource.next(''); - this.exerciseGroups = this.exerciseGroups!.filter((exerciseGroup) => exerciseGroup.id !== exerciseGroupId); - this.exerciseGroupToExerciseTypesDict.delete(exerciseGroupId); }, error: (error: HttpErrorResponse) => this.dialogErrorSource.next(error.message), }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
src/main/webapp/app/exam/manage/exercise-groups/exercise-group-update.component.ts(2 hunks)src/main/webapp/app/exam/manage/exercise-groups/exercise-group.service.ts(2 hunks)src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts(2 hunks)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/manage/exercise-groups/exercise-group-update.component.spec.ts(2 hunks)src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.spec.ts(1 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)
🧰 Additional context used
📓 Path-based instructions (17)
src/main/webapp/app/exam/manage/exercise-groups/exercise-group-update.component.ts (1)
src/main/webapp/app/exam/manage/exercise-groups/exercise-group.service.ts (1)
src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts (1)
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.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/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.ts (1)
src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.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/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.ts (1)
src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.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/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts (1)
src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.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/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.ts (1)
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group-update.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}}
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.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}}
src/test/javascript/spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.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}}
src/test/javascript/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.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}}
src/test/javascript/spec/component/exam/manage/exercise-groups/programming-exercise-group-cell.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}}
src/test/javascript/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.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 (24)
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.html (1)
1-1: LGTM! Correct usage of new Angular control flow syntax.
The @if syntax follows the new Angular control flow guidelines, replacing the older *ngIf directive.
src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.html (1)
1-1: LGTM! Correct usage of new @if syntax.
The code correctly uses the new Angular @if syntax as per guidelines, replacing the older *ngIf directive.
src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.html (1)
1-1: LGTM! Correctly using new @if syntax
The code follows the new Angular syntax guidelines by using @if instead of *ngIf.
src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell/quiz-exercise-group-cell.component.ts (2)
1-1: LGTM! Good migration to the new input API.
The change from Input decorator to the new input function aligns with Angular's modern practices for handling component inputs.
12-12: Well-structured input declaration with type safety.
The migration to input.required<QuizExercise>() brings several benefits:
- Ensures type safety with explicit
QuizExercisetype - Makes the input requirement explicit at compile time
- Reduces boilerplate compared to the traditional decorator approach
Let's verify the template usage of this input:
✅ Verification successful
Input usage verified and correctly implemented in template
The template correctly uses the new input syntax by calling exercise() as a function in both instances:
- Condition check:
exercise().type === exerciseType.QUIZ - Property access:
exercise().quizQuestions?.length
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the template correctly uses the new input syntax
# Expect: Template should call exercise() as a function rather than accessing it as a property
rg -t html "exercise\(" "src/main/webapp/app/exam/manage/exercise-groups/quiz-exercise-cell"
Length of output: 400
src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/modeling-exercise-group-cell.component.ts (2)
1-1: LGTM! Modern dependency injection approach
The change from @Input decorator to the new input function aligns with Angular's modern dependency injection patterns.
12-12: LGTM! Verify template updates
The new input.required<ModelingExercise>() implementation provides better type safety and removes the need for explicit type casting.
Let's verify that all template usages have been updated:
✅ Verification successful
Let me check the template file specifically to verify if it's using the new exercise() syntax.
Template updates are correctly implemented
The template is properly using the new exercise() signal syntax, and the remaining modelingExercise references found are just translation keys in a different component, which is expected and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the template file has been updated to use exercise() instead of modelingExercise
# and ensure no other files are still referencing modelingExercise
# Check the component's template file
rg -A 2 "modelingExercise" src/main/webapp/app/exam/manage/exercise-groups/modeling-exercise-cell/
# Check for any remaining references in other files
rg "modelingExercise" src/main/webapp/app/exam/manage/exercise-groups/
Length of output: 789
Script:
#!/bin/bash
# Check the component's template file specifically
fd modeling-exercise-group-cell.component.html -t f --exec cat {}
Length of output: 201
src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.ts (2)
12-12: Verify template updates for the new input syntax.
The migration to input.required looks good, but ensure that the corresponding template file has been updated to use exercise() instead of direct property access.
Let's verify the template updates:
✅ Verification successful
Template is correctly using the new input syntax ✅
The template is already using the correct exercise() function call syntax in both places:
exercise().typeexercise().filePattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the template has been properly updated
# Expected: Template should use exercise() function calls
# Check the template file for any remaining direct property access
rg "fileUploadExercise" "src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.html"
Length of output: 144
Script:
#!/bin/bash
# Let's first check if the template file exists and see its content
fd "file-upload-exercise-group-cell.component.html" --type f --exec cat {}
# As a fallback, let's check for any references to the exercise property in the template
rg "exercise" "src/main/webapp/app/exam/manage/exercise-groups/file-upload-exercise-cell/file-upload-exercise-group-cell.component.html"
Length of output: 397
1-1: LGTM! Verify other components in the exercise group module.
The migration from Input decorator to input function aligns with Angular's new input system using signals, which can improve performance through better change detection.
Let's verify consistent migration across related components:
✅ Verification successful
Migration to input() signal is consistently applied across exercise group components
The verification shows that all exercise cell components in the exercise groups module (file-upload, modeling, programming, and quiz) have been properly migrated to use the new input() signal from @angular/core. Let's check the remaining components to ensure complete migration:
Migration to input() signal is correctly implemented
Based on the verification results, all exercise cell components (file-upload, modeling, programming, and quiz) have been properly migrated to use the new input() signal from @angular/core. No legacy @Input decorators were found in the module, confirming consistent implementation of the signals architecture migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other exercise group components still use the old Input decorator
# Expected: No components should use the old @Input decorator
# Search for old @Input usage in exercise group components
rg -l "@Input\(\)" "src/main/webapp/app/exam/manage/exercise-groups"
Length of output: 68
Script:
#!/bin/bash
# Let's try a broader search to find any Input usage, including variations
rg -i "input.*from.*@angular/core|@Input|input\(" "src/main/webapp/app/exam/manage/exercise-groups" -A 2
Length of output: 3171
Script:
#!/bin/bash
# Let's also check for component files that might need migration
fd ".component.ts$" "src/main/webapp/app/exam/manage/exercise-groups"
Length of output: 685
Script:
#!/bin/bash
# Check the specific pattern of input usage from @angular/core
ast-grep --pattern 'import { $$$, input } from "@angular/core"'
Length of output: 65
Script:
#!/bin/bash
# Check if exercise-group-update and exercise-groups components still use @Input
rg -i "@Input|input.*from.*@angular/core" "src/main/webapp/app/exam/manage/exercise-groups/exercise-group" -A 2
Length of output: 214
Script:
#!/bin/bash
# Check the remaining components with correct paths
rg -i "@Input|input.*from.*@angular/core" "src/main/webapp/app/exam/manage/exercise-groups/exercise-group.*\.component\.ts" -A 2
Length of output: 250
Script:
#!/bin/bash
# Double check for any remaining @Input decorators in the module
rg "@Input" "src/main/webapp/app/exam/manage/exercise-groups/" --type ts
Length of output: 70
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.spec.ts (1)
1-4: LGTM! Imports follow best practices.
The imports are specific and include the necessary testing utilities for HTTP testing in Angular.
src/main/webapp/app/exam/manage/exercise-groups/exercise-group-update.component.ts (1)
17-20: 🛠️ Refactor suggestion
Verify subscription cleanup to prevent memory leaks
The inject pattern is correctly implemented. However, since we're subscribing to route.data in ngOnInit, we should ensure proper cleanup.
Consider implementing OnDestroy and managing subscriptions:
-export class ExerciseGroupUpdateComponent implements OnInit {
+export class ExerciseGroupUpdateComponent implements OnInit, OnDestroy {
+ private destroy$ = new Subject<void>();
+
private route = inject(ActivatedRoute);
private router = inject(Router);
private exerciseGroupService = inject(ExerciseGroupService);
private alertService = inject(AlertService);
ngOnInit(): void {
this.courseId = Number(this.route.snapshot.paramMap.get('courseId'));
- this.route.data.subscribe(({ exam, exerciseGroup }) => {
+ this.route.data.pipe(
+ takeUntil(this.destroy$)
+ ).subscribe(({ exam, exerciseGroup }) => {
this.exam = exam;
this.exerciseGroup = exerciseGroup;
});
}
+
+ ngOnDestroy(): void {
+ this.destroy$.next();
+ this.destroy$.complete();
+ }src/main/webapp/app/exam/manage/exercise-groups/exercise-group.service.ts (1)
1-2: LGTM! Good modernization of dependency injection
The transition from constructor-based DI to the inject() function follows modern Angular best practices and reduces boilerplate code. The removal of unused Router dependency also helps maintain a clean codebase.
Also applies to: 11-11
src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts (3)
19-21: LGTM! Clean migration to inject()
The migration from constructor injection to the new inject() function follows Angular's modern best practices and reduces boilerplate code.
28-32: LGTM! Proper input() usage
The migration from @input() decorators to input() function is well implemented with:
- Correct default values for boolean flags
- Proper type safety using input.required for the exercise property
Line range hint 63-71: LGTM! Well-documented and properly implemented
The downloadRepository method:
- Correctly uses the new exercise() accessor
- Includes proper error handling
- Has clear documentation about repository type limitations
src/main/webapp/app/exam/manage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html (3)
1-6: LGTM! Proper usage of new Angular syntax
The changes correctly implement the new @if syntax and handle null safety appropriately.
Line range hint 84-101: LGTM! Well-structured editor modes section
The section properly handles different IDE modes with appropriate translations and feature flags.
21-27: Verify status component integration
The programming exercise instructor status component is used with different participations. Let's verify its integration.
Also applies to: 39-45
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the component's integration:
Programming Exercise Instructor Status component is properly integrated
The component is consistently used across the codebase with the same input pattern for different participation types:
- In programming exercise management view for template and solution participations
- In programming test status details
- In exam exercise groups for template and solution participations (current usage)
All usages properly provide the required inputs: participationType, participation, and exercise.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the component definition and its usage
ast-grep --pattern 'class ProgrammingExerciseInstructorStatus {
$$$
}'
# Check for other usages of the component
rg "programming-exercise-instructor-status" -A 5
Length of output: 10035
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group-update.component.spec.ts (3)
1-2: LGTM! Modern HTTP testing configuration
The switch to provideHttpClient() and provideHttpClientTesting() follows Angular's recommended testing practices, providing better control over HTTP testing and aligning with the modernization objectives.
Also applies to: 50-51
Line range hint 47-58: LGTM! Well-structured test module configuration
The test module setup follows best practices with:
- Proper component and directive mocking
- Clear separation of concerns
- Avoiding NO_ERRORS_SCHEMA as per guidelines
Line range hint 1-124: Verify signal testing coverage
Given that this PR involves migrating to signals, please ensure that signal-related functionality is properly tested.
src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts (3)
Line range hint 1-32: LGTM! Well-organized imports following Angular best practices
The imports are properly organized and follow tree-shaking friendly patterns.
Line range hint 34-42: LGTM! Component follows Angular architectural patterns
The component structure follows Angular best practices with proper separation of concerns.
45-45: Review public access modifier for exerciseService
The exerciseService is marked as public while other services are private. Please verify if this service needs to be accessed from the template, otherwise consider making it private for better encapsulation.
✅ Verification successful
Keep exerciseService public as it's used in the template
The exerciseService needs to remain public since it's being accessed directly in the template to call isIncludedInScore() method on an exercise.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if exerciseService is used in the template
rg "exerciseService\." src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.html
Length of output: 219
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-group.service.spec.ts
Show resolved
Hide resolved
...script/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts
Show resolved
Hide resolved
...script/spec/component/exam/manage/exercise-groups/quiz-exercise-group-cell.component.spec.ts
Show resolved
Hide resolved
...spec/component/exam/manage/exercise-groups/file-upload-exercise-group-cell.component.spec.ts
Show resolved
Hide resolved
...pt/spec/component/exam/manage/exercise-groups/modeling-exercise-group-cell.component.spec.ts
Show resolved
Hide resolved
...anage/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.ts
Show resolved
Hide resolved
...age/exercise-groups/programming-exercise-cell/programming-exercise-group-cell.component.html
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.component.spec.ts (4)
51-54: Consider using NgMocks consistently for all mocksWhile the current setup works, consider using NgMocks consistently for all mocked dependencies to align with the coding guidelines:
TestBed.configureTestingModule({ imports: [ArtemisTestModule, ExerciseGroupsComponent], - providers: [{ provide: ActivatedRoute, useValue: route }, { provide: Router, useClass: MockRouter }, MockProvider(AlertService)], + providers: [ + { provide: ActivatedRoute, useValue: route }, + MockProvider(Router, { + navigate: jest.fn().mockReturnValue(Promise.resolve(true)), + }), + MockProvider(AlertService), + ], }) - .overrideProvider(NgbModal, { useValue: new MockNgbModalService() }) + .overrideProvider(NgbModal, { useFactory: () => MockProvider(NgbModal) })
Line range hint
82-95: Enhance HTTP response expectations in loadExerciseGroups testThe test could be more specific in its expectations and better handle HTTP responses according to the coding guidelines.
it('loads the exercise groups', fakeAsync(() => { const mockResponse = new HttpResponse<Exam>({ body: exam }); - jest.spyOn(examManagementService, 'find').mockReturnValue(of(mockResponse)); + const findSpy = jest.spyOn(examManagementService, 'find').mockReturnValue(of(mockResponse)); comp.loadExerciseGroups().subscribe((response) => { expect(response.body).not.toBeNull(); - expect(response.body!.id).toBe(exam.id); + expect(response).toEqual( + expect.objectContaining({ + body: expect.objectContaining({ + id: exam.id, + course: expect.objectContaining({ + id: course.id, + }), + }), + }), + ); }); tick(); + expect(findSpy).toHaveBeenCalledExactlyOnceWith(exam.id); }));
Line range hint
165-189: Improve test organization for exercise icon testsThe individual icon tests could be consolidated using Jest's test.each for better maintainability.
-it('returns the exercise icon type quiz', () => { - const icon = faCheckDouble; - const exercise = { type: ExerciseType.QUIZ } as Exercise; - expect(comp.exerciseIcon(exercise)).toBe(icon); -}); - -it('returns the exercise icon type file upload', () => { - const icon = faFileUpload; - const exercise = { type: ExerciseType.FILE_UPLOAD } as Exercise; - expect(comp.exerciseIcon(exercise)).toBe(icon); -}); -// ... other icon tests +it.each([ + [ExerciseType.QUIZ, faCheckDouble], + [ExerciseType.FILE_UPLOAD, faFileUpload], + [ExerciseType.MODELING, faProjectDiagram], + [ExerciseType.PROGRAMMING, faKeyboard], + [ExerciseType.TEXT, faFont], +])('returns the correct icon for exercise type %s', (exerciseType, expectedIcon) => { + const exercise = { type: exerciseType } as Exercise; + expect(comp.exerciseIcon(exercise)).toBe(expectedIcon); +});
Line range hint
191-234: Consolidate import modal navigation testsThe two similar test blocks for import modal navigation could be combined for better maintainability.
-it.each([[ExerciseType.PROGRAMMING], [ExerciseType.TEXT], /* ... */])( - 'opens the import modal and navigates to import page', - fakeAsync((exerciseType: ExerciseType) => { - const mockReturnValue = { - result: Promise.resolve({ id: 1 } as Exercise), - componentInstance: {}, - } as NgbModalRef; - // ... test implementation - }), -); - -it.each([[ExerciseType.PROGRAMMING], [ExerciseType.TEXT], /* ... */])( - 'opens the import modal and navigates to import from file page', - fakeAsync((exerciseType: ExerciseType) => { - const mockReturnValue = { - result: Promise.resolve({ id: undefined } as Exercise), - componentInstance: {}, - } as NgbModalRef; - // ... test implementation - }), -); +it.each([ + [true, 1, '/course-management/%d/exams/%d/exercise-groups/%d/%s-exercises/import/%d'], + [false, undefined, '/course-management/%d/exams/%d/exercise-groups/%d/%s-exercises/import-from-file'] +])('opens the import modal and navigates correctly when exercise id is %s', + fakeAsync((hasId: boolean, exerciseId: number | undefined, expectedPath: string) => { + const exerciseTypes = [ + ExerciseType.PROGRAMMING, + ExerciseType.TEXT, + ExerciseType.MODELING, + ExerciseType.QUIZ, + ExerciseType.FILE_UPLOAD + ]; + + for (const exerciseType of exerciseTypes) { + const mockReturnValue = { + result: Promise.resolve({ id: exerciseId } as Exercise), + componentInstance: {}, + } as NgbModalRef; + + const modalSpy = jest.spyOn(modalService, 'open').mockReturnValue(mockReturnValue); + const routerSpy = jest.spyOn(router, 'navigate'); + + comp.openImportModal(groups[0], exerciseType); + tick(); + + expect(modalSpy).toHaveBeenCalledExactlyOnceWith(expect.anything(), expect.anything()); + expect(routerSpy).toHaveBeenCalledExactlyOnceWith( + hasId + ? [expectedPath.replace(/%d|%s/g, (m) => m === '%s' ? exerciseType : m === '%d' ? [456, 123, 0, exerciseId][i++] : '')] + : [expectedPath.replace(/%d|%s/g, (m) => m === '%s' ? exerciseType : m === '%d' ? [456, 123, 0][i++] : '')], + hasId ? undefined : expect.anything() + ); + + jest.clearAllMocks(); + } + }) +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.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 (1)
src/test/javascript/spec/component/exam/manage/exercise-groups/exercise-groups.component.spec.ts (1)
Line range hint 1-286: Overall test implementation looks good with some suggested improvements
The test file successfully adapts to the standalone component architecture while maintaining comprehensive test coverage. The implementation follows good testing practices with proper use of Jest features and clear expectations.
The suggested improvements above would enhance:
- Consistency in mocking approach using NgMocks
- Specificity in HTTP response expectations
- Test organization and maintainability
- Code duplication reduction
These changes are not critical but would align better with the testing best practices and make the tests more maintainable.
coolchock
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
coolchock
dismissed stale reviews from sawys777 and coderabbitai[bot]
via
00e3db1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/exam/manage/exam-management.module.ts (1)
Line range hint
89-90: Address the TODO comment about modularizationThe TODO comment suggests creating a dedicated exercise module with exam routes for better modularization. This aligns well with the current PR's objective of improving the architecture. Would you like assistance in:
- Creating a new exercise module
- Moving the relevant routes and components
- Setting up lazy loading for the new module
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/exam/manage/exam-management.module.ts(1 hunks)src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exam/manage/exam-management.module.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/exam/manage/exam-management.module.ts (2)
143-143: LGTM: Export of ExamExerciseRowButtonsComponent
The addition of ExamExerciseRowButtonsComponent to the exports array is appropriate as it makes the component available for other modules to use. This change aligns with the module's responsibility of providing exam management functionality.
Line range hint 1-143: Consider improving module organization and lazy loading
While the changes align with migrating to standalone components, there are opportunities to improve the module's organization:
- The module imports many feature modules that could be candidates for lazy loading to improve initial load time.
- Consider grouping related imports (e.g., material modules, shared modules, feature modules) for better organization.
Let's verify the potential impact of these imports on bundle size:
Checklist
General
Client
Description
This pull request migrates exercise group module to signals, inject and standalone
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
Summary by CodeRabbit
Release Notes
New Features
injectfunction for improved code clarity and maintainability.standalonecomponents for better modularity in the application structure.ExamExerciseRowButtonsComponentto the exports of the exam management module.Bug Fixes
Tests