General: Migrate exercise group module to signals
#9924
Conversation
github-actions
bot
added
the
client
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
coolchock
force-pushed
the
chore/exam-exercises-client-migration
branch
from
ea55e4d to
d1ff991
Compare
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
# Conflicts: # src/main/webapp/app/exam/manage/exam-management.module.ts # src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/programming-exam-diff/programming-exercise-exam-diff.component.ts # src/main/webapp/app/exam/participate/exam-participation.module.ts # src/main/webapp/app/exam/participate/exercises/exam-exercise-update-highlighter/exam-exercise-update-highlighter.component.ts # src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts # src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts # src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts # src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts # src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts # src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.ts # src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instructions-render.module.ts # src/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.ts # src/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.component.spec.ts # src/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts
WalkthroughThis pull request refactors several Angular components and their corresponding test suites. The changes transition from decorator-based property declarations (using Changes
Sequence Diagram(s)sequenceDiagram
participant TIMELINE as StudentExamTimelineComponent
participant FILE_UPLOAD as fileUploadComponent
participant SUBMISSION as studentSubmission
TIMELINE->>FILE_UPLOAD: updateFileUploadExerciseView()
FILE_UPLOAD->>SUBMISSION: studentSubmission.update(() => currentSubmission)
SUBMISSION-->>FILE_UPLOAD: Returns updated submission
Possibly related PRs
Suggested labels
✨ Finishing Touches
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: 2
🔭 Outside diff range comments (3)
src/main/webapp/app/exam/participate/exercises/exam-exercise-update-highlighter/exam-exercise-update-highlighter.component.ts (1)
66-80: 🛠️ Refactor suggestionImprove error handling in updateExerciseProblemStatementById.
The current implementation could be more robust in handling undefined values and type checking.
updateExerciseProblemStatementById(exerciseId: number, updatedProblemStatement: string) { - if (updatedProblemStatement != undefined && exerciseId === this.exercise().id) { + if (!exerciseId || !this.exercise()?.id) { + return; + } + + if (updatedProblemStatement && exerciseId === this.exercise().id) { this.outdatedProblemStatement = this.exercise().problemStatement!; this.updatedProblemStatement = updatedProblemStatement; this.exercise().problemStatement = updatedProblemStatement;src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts (1)
75-96: 🛠️ Refactor suggestionRefactor setExerciseIconStatus for better maintainability.
The method has multiple responsibilities and could be split into smaller, more focused methods.
+private getExerciseStatus(submission: Submission | undefined): ExerciseButtonStatus { + if (!submission) { + return ExerciseButtonStatus.Synced; + } + if (submission.submitted && submission.isSynced) { + return ExerciseButtonStatus.SyncedSaved; + } + return submission.isSynced ? ExerciseButtonStatus.Synced : ExerciseButtonStatus.NotSynced; +} +private getIconForStatus(status: ExerciseButtonStatus): IconDefinition { + switch (status) { + case ExerciseButtonStatus.SyncedSaved: + return facSaveSuccess; + case ExerciseButtonStatus.NotSynced: + return facSaveWarning; + default: + return faHourglassHalf; + } +} setExerciseIconStatus(item: ExamExerciseOverviewItem): ExerciseButtonStatus { const submission = ExamParticipationService.getSubmissionForExercise(item.exercise); - // start with exercise not started icon - item.icon = faHourglassHalf; - if (!submission) { - return ExerciseButtonStatus.Synced; - } - if (submission.submitted && submission.isSynced) { - item.icon = facSaveSuccess; - return ExerciseButtonStatus.SyncedSaved; - } - if (submission.isSynced) { - return ExerciseButtonStatus.Synced; - } else { - item.icon = facSaveWarning; - return ExerciseButtonStatus.NotSynced; - } + const status = this.getExerciseStatus(submission); + item.icon = this.getIconForStatus(status); + return status; }src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.ts (1)
57-57:⚠️ Potential issuePrevent potential memory leak from unsubscribed Subject.
The textEditorInput Subject is not being unsubscribed, which could lead to memory leaks.
+private destroy$ = new Subject<void>(); ngOnInit(): void { // show submission answers in UI this.problemStatementHtml = htmlForMarkdown(this.exercise()?.problemStatement); this.updateViewFromSubmission(); + + // Setup text editor input subscription + this.textEditorInput + .pipe(takeUntil(this.destroy$)) + .subscribe(/* handle text editor input */); } +ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); +}Also applies to: 65-69
🧹 Nitpick comments (21)
src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts (6)
96-96: Guard against null or empty file pattern.
this.exercise().filePattern!uses a non-null assertion. IffilePatternis unexpectedly empty or null,split(',')can break. It might be safer to check before splitting or provide a fallback, e.g.filePattern ?? ''.- const allowedFileExtensions = this.exercise().filePattern!.split(','); + const filePattern = this.exercise()?.filePattern ?? ''; + const allowedFileExtensions = filePattern.split(',');
103-103: Consider referencing studentSubmission once in this method.Calling
this.studentSubmission()repeatedly can lead to confusion if it does more than retrieve a property. For clarity and performance, consider storing it in a local variable if the method or logic expands further.
116-116: Verify dueDate check logic.This expression repeatedly calls
this.exercise(). Although short, storing the exercise in a local variable and reusing it could improve readability and reduce repeated function calls.
127-131: Revisit frequentstudentSubmission()calls.Similar to the exercise approach, repeated calls to a function-based property might be less clear than storing it locally, especially if the logic inside
studentSubmission()evolves.
142-145: ConsolidatestudentSubmission()usage and null assertions.This block calls
this.studentSubmission()multiple times, each with a non-null assertion (!). If the submission is guaranteed to exist, consider storing it first and then using that local reference to avoid repeated!.
160-165: Unsubscribe from the observable or use async pipe.When using
subscribe, it’s recommended to manage subscriptions (e.g., viatakeUntilorngOnDestroy) to avoid potential memory leaks, especially in exam-scale usage. Also confirm concurrency safety if multiple submissions are triggered simultaneously.src/main/webapp/app/exam/participate/exercises/exam-submission.component.ts (1)
32-33: Add explicit type annotations for read-only and examTimeline.Using
input(false)is fine, but consider making the intent clearer with explicit types (e.g.,boolean) and ensuring that any default values or external bindings still reflect the correct type.src/test/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.ts (2)
53-53: Avoid using non-null assertion for exercises if possible.
studentExam.exercises![0]can throw ifexercisesis undefined. While it’s safe in this test setup, you might skip the!if further logic ensures exercises is always populated.
59-61: Confirm function call usage forcomp.studentExam?.().
comp.studentExamis defined as a function-like accessor rather than a standard property. This can be confusing to future maintainers. Ensure your project consistently uses or documents this pattern.src/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.ts (2)
46-62: Enhance test readability with beforeEach setup.Consider moving the common setup code to beforeEach to reduce duplication and improve test maintainability.
beforeEach(() => { TestBed.configureTestingModule({ imports: [ArtemisTestModule], }) .compileComponents() .then(() => { fixture = TestBed.createComponent(ProgrammingExamSubmissionComponent); component = fixture.componentInstance; + const studentParticipation = newParticipation(); + fixture.componentRef.setInput('studentParticipation', studentParticipation); }); }); it('should change state on commit', () => { - const studentParticipation = newParticipation(); - fixture.componentRef.setInput('studentParticipation', studentParticipation);
60-61: Use more specific assertions for better test clarity.The current assertions could be more specific about what they're testing.
- expect(component.studentParticipation().submissions![0].submitted).toBeTrue(); - expect(component.studentParticipation().submissions![0].isSynced).toBeTrue(); + expect(component.studentParticipation().submissions![0]).toContainEntries([ + ['submitted', true], + ['isSynced', true] + ]);src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts (1)
69-70: Address TODO comment about extracting common logic.The TODO comment indicates a need to extract common logic for exercise status calculation.
Would you like me to help create a new utility service to handle the common logic for exercise status and icon calculation? This would help avoid side effects and improve code reusability.
src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts (2)
77-81: Refactor suggestion for student participation handling.Consider extracting the student participation access logic into a private method to avoid repetition and improve maintainability.
+private getFirstSubmission(): Submission | undefined { + const studentParticipation = this.studentParticipation(); + return studentParticipation?.submissions?.[0]; +} getSubmission(): Submission | undefined { - const studentParticipation = this.studentParticipation(); - - if (studentParticipation?.submissions && studentParticipation.submissions.length > 0) { - return studentParticipation.submissions[0]; - } + return this.getFirstSubmission(); return undefined; }
138-149: Refactor suggestion for commit state handling.The commit state logic could be simplified by using the extracted method for accessing the first submission.
onCommitStateChange(commitState: CommitState): void { - const studentParticipation = this.studentParticipation(); - if (studentParticipation?.submissions && studentParticipation.submissions.length > 0) { - const firstSubmission = studentParticipation.submissions[0]; + const firstSubmission = this.getFirstSubmission(); + if (firstSubmission) { if (commitState === CommitState.CLEAN && this.hasSubmittedOnce) { firstSubmission.submitted = true; firstSubmission.isSynced = true; } else if (commitState !== CommitState.UNDEFINED && !this.hasSubmittedOnce) { this.hasSubmittedOnce = true; } } }src/test/javascript/spec/component/exam/participate/exercises/modeling-exam-submission.component.spec.ts (2)
28-34: Improve test data initialization.The mock submission's model is hardcoded with a specific version. Consider parameterizing the version to make tests more maintainable.
-mockSubmission = { explanationText: 'Test Explanation', model: JSON.stringify({ version: '2.0.0', model: true }) } as ModelingSubmission; +const MOCK_MODEL_VERSION = '2.0.0'; +mockSubmission = { + explanationText: 'Test Explanation', + model: JSON.stringify({ version: MOCK_MODEL_VERSION, model: true }) +} as ModelingSubmission;
196-219: Improve test readability with test data builders.The test for submission version changes contains complex test data. Consider extracting the test data setup into a builder pattern for better maintainability.
class UMLModelBuilder { private model: Partial<UMLModel> = { version: '3.0.0', type: 'ClassDiagram', size: { width: 220, height: 420 }, interactive: { elements: {}, relationships: {} }, elements: {}, relationships: {}, assessments: {}, }; build(): UMLModel { return this.model as UMLModel; } } // Usage in test const parsedModel = new UMLModelBuilder().build();src/test/javascript/spec/component/exam/participate/exercises/file-upload-exam-submission.component.spec.ts (1)
212-239: Add test coverage for file size validation.The test for file size validation only checks error handling. Consider adding test cases for:
- Files at the size limit
- Edge cases around the size limit
it('should accept file at size limit', fakeAsync(() => { const submissionFile = new File([''], 'example.png'); Object.defineProperty(submissionFile, 'size', { value: MAX_SUBMISSION_FILE_SIZE, writable: false }); // ... rest of the test }));src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts (1)
238-292: Consider using a strategy pattern for answer type handling.The
updateSubmissionFromViewmethod contains similar logic for different answer types. Consider extracting this into a strategy pattern to improve maintainability.interface AnswerStrategy { createSubmittedAnswer(question: any, data: any): any; } class MultipleChoiceStrategy implements AnswerStrategy { createSubmittedAnswer(question: any, options: AnswerOption[]) { const answer = new MultipleChoiceSubmittedAnswer(); answer.quizQuestion = question; answer.selectedOptions = options; return answer; } } // Usage in component private answerStrategies = new Map<QuizQuestionType, AnswerStrategy>([ [QuizQuestionType.MULTIPLE_CHOICE, new MultipleChoiceStrategy()], // ... other strategies ]);src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.html (1)
5-5: Ensure function calls in template are optimized.Multiple function calls in template bindings could impact performance. Consider caching values or using computed signals:
-{{ exercise().exerciseGroup?.title }} +{{ exerciseGroupTitle() }} -[diagramType]="exercise().diagramType!" +[diagramType]="exerciseDiagramType()"Also applies to: 8-9, 20-22, 32-32
src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.html (2)
36-37: Optimize form control bindings.The template uses multiple function calls in form control bindings which could trigger unnecessary re-evaluations:
-[readOnly]="readonly() || !studentSubmission()" -[disabled]="readonly() || !studentSubmission()" +[readOnly]="isReadOnlyOrNoSubmission()" +[disabled]="isReadOnlyOrNoSubmission()"Consider adding a computed signal in the component to combine these conditions.
50-52: Ensure consistent error handling for exercise updates.The template should handle potential errors when updating the problem statement:
<jhi-exam-exercise-update-highlighter [exercise]="exercise()" + (error)="handleUpdateError($event)" (problemStatementUpdateEvent)="updateProblemStatement($event)" />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts(1 hunks)src/main/webapp/app/exam/participate/exercises/exam-exercise-update-highlighter/exam-exercise-update-highlighter.component.ts(3 hunks)src/main/webapp/app/exam/participate/exercises/exam-submission.component.ts(2 hunks)src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts(2 hunks)src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.html(4 hunks)src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts(7 hunks)src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.html(2 hunks)src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts(6 hunks)src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.html(1 hunks)src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts(5 hunks)src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.html(5 hunks)src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts(10 hunks)src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.html(3 hunks)src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.ts(5 hunks)src/test/javascript/spec/component/exam/manage/student-exams/student-exam-timeline.component.spec.ts(2 hunks)src/test/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.ts(4 hunks)src/test/javascript/spec/component/exam/participate/exam-exercise-update-highlighter.component.spec.ts(5 hunks)src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts(0 hunks)src/test/javascript/spec/component/exam/participate/exercises/file-upload-exam-submission.component.spec.ts(11 hunks)src/test/javascript/spec/component/exam/participate/exercises/modeling-exam-submission.component.spec.ts(5 hunks)src/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.ts(2 hunks)src/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.component.spec.ts(11 hunks)src/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts(6 hunks)
💤 Files with no reviewable changes (1)
- src/test/javascript/spec/component/exam/participate/exam-participation.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/participate/exercises/file-upload/file-upload-exam-submission.component.htmlsrc/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.htmlsrc/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.htmlsrc/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.htmlsrc/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.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/student-exams/student-exam-timeline.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/file-upload-exam-submission.component.spec.tssrc/test/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.tssrc/test/javascript/spec/component/exam/participate/exam-exercise-update-highlighter.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/modeling-exam-submission.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.tssrc/main/webapp/app/exam/participate/exercises/exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/exam-exercise-update-highlighter/exam-exercise-update-highlighter.component.tssrc/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.tssrc/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (39)
src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts (3)
49-52: Validate the custom property binding approach.You’ve switched from the standard
@ViewChildand@Inputdecorators toviewChild(),model.required(), andinput.required(). While this can be a valid approach, ensure that these are recognized and processed correctly by the Angular compiler, and that they integrate seamlessly with Angular’s lifecycle (particularly for child references likefileInput). It’s good practice to verify usage in your templates and corresponding tests to confirm these new property accessors work as expected.
74-74: Handle potential null fields in exercise.Here,
this.exercise()?.problemStatementis accessed safely, but ensure that any other code paths referencingexercise()similarly handle the possibility of it returningundefined(for instance, if the input wasn’t bound).
120-123: Returnexercise()safely.Both methods directly call
this.exercise(). Ifexerciseis truly required, that’s fine, but ensure an upstream check prevents null references. Otherwise, consider optional chaining or a fallback.src/main/webapp/app/exam/participate/exercises/exam-submission.component.ts (1)
4-4: Check for alignment with Angular best practices.Using
inputfrom a custom source (instead of the native@Input) requires consistency in the rest of your codebase. Ensure that Angular’s change detection picks these properties up as intended, especially if they’re used in templates.src/test/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.ts (2)
15-29: Initialize and verifystudentExamwith explicit checks.The new approach properly constructs a
StudentExamand populates exercises. Ensure tests robustly validate scenarios wherestudentExam.exercisescould be empty or null, especially if the logic referencesexercises!.
42-42: Leverage type-safe input binding.Calling
fixture.componentRef.setInput('studentExam', studentExam)is appropriate. Consider verifying that no property name mismatches occur if the code is refactored, as these dynamic string references won’t get compile-time checks.src/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts (2)
38-40: LGTM! Improved test setup using recommended Angular patterns.The change from direct property assignment to
fixture.componentRef.setInputfollows Angular's best practices for setting component inputs in tests.
67-67:❓ Verification inconclusive
Verify the change in property access pattern.
The code now accesses
studentSubmissionas a method call (e.g.,component.studentSubmission().isSynced). This change suggests that the component's property has been converted to a function.Also applies to: 82-82, 114-114
🏁 Script executed:
#!/bin/bash # Description: Verify the property access pattern in the component ast-grep --pattern 'studentSubmission = input.required<TextSubmission>()'Length of output: 75
Action Required: Confirm
studentSubmissionFunction ChangeThe test file now accesses
studentSubmissionas a method call (e.g.,component.studentSubmission().isSynced) instead of a property access. Please manually verify that this conversion—from a property to a function returning an object with theisSyncedproperty—has been correctly and consistently applied at the affected locations (lines 67, 82, and 114 ofsrc/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts).Since the automated
ast-grepsearch did not yield any output, additional manual verification is advised to ensure that this change is intentional and that there are no unintended side effects.src/test/javascript/spec/component/exam/participate/exam-exercise-update-highlighter.component.spec.ts (2)
14-15: LGTM! Improved test isolation.Converting constant declarations to
letand moving initialization intobeforeEachensures each test starts with a fresh state.Also applies to: 19-25
36-37:❓ Verification inconclusive
Verify the exercise property access pattern.
The code now accesses
exerciseas a method call (e.g.,component.exercise().id). This change suggests that the component's property has been converted to a function.Also applies to: 84-85
🏁 Script executed:
#!/bin/bash # Description: Verify the property access pattern in the component ast-grep --pattern 'exercise = input.required<Exercise>()'Length of output: 60
Action: Manually Verify the Updated Exercise Access Pattern
It appears that the automated check using the provided ast-grep command did not return any output. This inconclusive result means we can’t definitively confirm that the conversion of the exercise property to a function (e.g., using
component.exercise().id) was applied consistently.
- Please manually verify that the component’s
exerciseproperty is intentionally converted to a function throughout the codebase.- Confirm that the initialization (such as
exercise = input.required<Exercise>()) and its subsequent usage in lines 36–37 (and similarly in lines 84–85) reflect this change.src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts (2)
44-44: LGTM! Improved type safety with required inputs.The transition from decorators to
input.required<T>()andviewChild.required()enhances type safety by explicitly marking required dependencies.Also applies to: 47-47, 50-50, 55-56
108-108:❓ Verification inconclusive
Verify null safety in method calls.
The code now uses optional chaining with method calls (e.g.,
modelingEditor()?). Ensure this pattern is consistently applied across all method calls to prevent null reference errors.Also applies to: 111-111, 161-161
🏁 Script executed:
#!/bin/bash # Description: Check for consistent use of optional chaining with method calls rg -A 1 '\(\)\?' src/main/webapp/app/exam/participate/exercises/modeling/Length of output: 172
Action Required: Verify Consistent Use of Optional Chaining
Please manually confirm that in file
src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts
(optional chaining is used for method calls on potentially null objects). The pattern should resemble:if (!this.modelingEditor()?.getCurrentModel()) { // … }Key points to verify:
- Ensure that all calls to
modelingEditor()(and similar methods) use optional chaining (i.e.,this.modelingEditor()?.getCurrentModel()) to safeguard against null reference errors.- Specifically, review the implementations around lines 108, 111, and 161 for consistent application of this pattern.
- Confirm that the file path and naming align with repository standards, as our initial search did not locate the expected directory.
src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts (1)
59-60: LGTM! Improved type safety with required inputs.The transition to
input.required<T>()andviewChild.required()enhances type safety by explicitly marking required dependencies.Also applies to: 63-65
src/test/javascript/spec/component/exam/participate/exercises/modeling-exam-submission.component.spec.ts (2)
57-60: LGTM! Good practice adding cleanup.The
afterEachblock properly cleans up the fixture and restores mocks, preventing test pollution.
75-82:❓ Verification inconclusive
Verify test coverage for bonus points calculation.
The test cases for max score and bonus points only verify the translation values. Consider adding test cases to verify the actual calculation logic.
Also applies to: 85-97
🏁 Script executed:
#!/bin/bash # Search for bonus points calculation logic in the component ast-grep --pattern 'bonusPoints = $value'Length of output: 43
Review Note: Bonus Points Calculation Test Coverage
- The current tests only verify that the translation values for max points and bonus points are set (i.e. via the directive) without checking the underlying computation.
- Our initial search for explicit bonus points assignment (using the pattern
'bonusPoints = $value') did not yield any results, so it isn’t clear that bonus points calculation logic is being directly exercised.- Please verify manually whether the bonus points calculation is implemented elsewhere (in the business logic of the component or a related service). If it is, consider adding targeted tests to ensure the calculation behaves as expected.
- This consideration applies to both the code segment at lines 75–82 and the related section at lines 85–97.
src/test/javascript/spec/component/exam/participate/exercises/file-upload-exam-submission.component.spec.ts (1)
42-44: LGTM! Good practice using setInput.Using
fixture.componentRef.setInput()is the recommended way to set component inputs in Angular tests.src/test/javascript/spec/component/exam/manage/student-exams/student-exam-timeline.component.spec.ts (1)
166-168: LGTM! Good practice adding update method.The addition of the
updatemethod tostudentSubmissionfollows the pattern of encapsulating state changes.src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts (1)
1-1: LGTM! Good practice using new Angular syntax.The component correctly uses Angular's new input/output syntax and viewChildren function calls, improving type safety and reducing decorator usage.
Also applies to: 61-71
src/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.component.spec.ts (3)
47-47: Initialize required properties to prevent undefined errors.Good practice to initialize
answerOptionsandtextproperties to prevent potential undefined errors during tests.Also applies to: 52-52
132-132: Update test assertions to match new function call syntax.The test has been updated to use function call syntax (
studentSubmission()), which aligns with the changes in the component. Ensure all assertions are updated consistently.Also applies to: 207-208
82-84:❓ Verification inconclusive
Verify test setup follows Angular's new input() syntax.
The test has been updated to use
fixture.componentRef.setInput()for setting component inputs, which aligns with Angular's new input syntax. However, ensure that the component itself has been migrated to use the new input syntax:Also applies to: 97-99, 140-142, 164-165, 214-222
🏁 Script executed:
#!/bin/bash # Description: Verify component uses new input syntax # Search for @Input decorators in the component rg "@Input\(\)" "src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts" # Search for input() usage in the component rg "input\(" "src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts"Length of output: 456
Action Required: Verify Component File Location and Input Syntax
It appears that the file path provided in the shell script (
src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts) could not be found. Please verify manually that the component file exists at the expected location or update the path accordingly. Additionally, confirm that the component has been migrated to use Angular’s newinput()syntax as referenced in the test setup.
- Action Items:
- Manually check that the quiz exam submission component file exists and is located at the correct path.
- Confirm that the component’s implementation aligns with Angular’s new input binding syntax (i.e., using the
input()method or the appropriate decorators).- Ensure that similar adjustments are applied for the locations noted (lines 97–99, 140–142, 164–165, 214–222).
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts (1)
163-163:❓ Verification inconclusive
Verify the update callback pattern is used consistently.
The change to use
updatecallback for settingstudentSubmissionaligns with the Signal-based approach. However, ensure this pattern is used consistently across all submission types:
🏁 Script executed:
#!/bin/bash # Description: Check for consistent usage of update callback pattern # Search for direct assignments to studentSubmission rg "studentSubmission\s*=" "src/main/webapp/app/exam/**/*.ts" # Search for update callback usage rg "studentSubmission.*update\(" "src/main/webapp/app/exam/**/*.ts"Length of output: 279
Action Required: Confirm Consistent Usage of the Update Callback Pattern
The change in using the
updatecallback to setstudentSubmissioninstudent-exam-timeline.component.tsadheres to the Signal-based approach. However, our earlier automated search using the path"src/main/webapp/app/exam/**/*.ts"returned errors due to a potential mismatch in directory structure. Before finalizing, please verify manually (or update the search path) that:
- No direct assignments to
studentSubmission(i.e., using=) exist anywhere else in the codebase.- The update callback pattern is consistently applied for all submission types.
Once confirmed, this change can be accepted.
src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.html (1)
1-1: Verify consistent usage of new Angular control flow syntax.The template correctly uses the new
@ifsyntax instead of*ngIf. This aligns with Angular's latest best practices.Also applies to: 12-14, 26-26, 48-50
src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.html (6)
1-13: Consistent Functional Access in Header Section
The header correctly switches from direct property access to function calls—using constructs likeexercise()—and applies the new@ifsyntax. The translated text and score badge condition are both handled consistently.
16-19: Left Panel: Dynamic Title and Container Usage
The<jhi-resizeable-container>now acceptsexamTimeline()as a function call. The inline expression for displaying the exercise title (with a fallback translated text) is clear and consistent.
21-29: Conditional Display for File Submission Input
The conditional block using@if (isActive && !result && exercise() && studentSubmission() && !readonly()) { … }ensures the file upload form is only rendered when all required conditions (e.g. valid exercise and submission state) are met. This updated check using function calls is appropriate.
50-54: File Download Section with Safe Navigation
The updated block that conditionally renders a download link (usingsubmittedFileNameandstudentSubmission()?.filePath) and employs non-null assertions (e.g.studentSubmission()!.filePath!) is both clear and safe.
61-65: Display When No File Has Been Submitted
The block@if (!submittedFileName && examTimeline()) { … }correctly renders a message indicating that no file was submitted. The use of function calls aligns with the new functional style.
73-79: Right Panel: Problem Statement Update Highlighter
The conditional block renders the<jhi-exam-exercise-update-highlighter>component only whenexercise()is available. Passingexercise()to the component ensures a dynamic update of the problem statement.src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.html (7)
1-13: Header Section with Functional Data Retrieval
The header now usesexercise()to obtain properties (e.g.exerciseGroup?.title,bonusPoints,maxPoints, andincludedInOverallScore) and conditionally renders the score badge. This refactor is consistent and clear.
16-26: Online Code Editor Block: Container and Participation Binding
The online editor block starts with@if (exercise().allowOnlineEditor) { <div> <jhi-code-editor-container … [participation]="studentParticipation()" … [course]="getCourseFromExercise(exercise())" …> … </jhi-code-editor-container> </div> }This consistent use of function calls to get
exercise()andstudentParticipation()guarantees that the component receives the most up-to-date data and complies with the project’s functional style.
31-35: Online Editor: Offline IDE Conditional Subsection
Within the container, the condition@if (exercise().allowOfflineIde) { … }ensures that student actions (rendered via
<jhi-exercise-details-student-actions>) are shown only when the offline IDE option is enabled. The usage is straightforward and matches the new coding style.
41-52: Result Update and Participation in Toolbar
The section that conditionally renders the<jhi-updating-result>component usesstudentParticipation()as expected. This ensures that dynamic updates (progress bar, badges, etc.) are based on the current participation state.
53-58: Trigger Build Button with Functional Bindings
The<jhi-programming-exercise-student-trigger-build-button>receives both[exercise]and[participation]via their respective function calls and sets the button size. This consistent pattern improves maintainability.
60-67: Programming Exercise Instructions with Editor Sidebar
The instructions component correctly binds inputs throughexercise()andstudentParticipation(), ensuring that any updates or instruction unavailability events are handled seamlessly.
72-99: Offline IDE Block for Alternative Student Actions
The offline IDE block (triggered when!exercise().allowOnlineEditor && exercise().allowOfflineIde) conditionally renders a secondary interface. Function calls forexercise(),courseId(), andstudentParticipation()maintain consistency. The nested conditionals for repository status and result updating are clear and well structured.src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.html (3)
1-12: Header Section with Quiz Configuration Transformation
The header now retrieves data usingquizConfiguration()(e.g. forexerciseGroup?.title,maxPoints, andincludedInOverallScore), and the save button correctly receivesstudentSubmission()via function invocation. This refactor is consistent and improves dynamic data handling.
15-44: Quiz Navigation: Conditional Rendering and Looping
The navigation block uses@if (quizConfiguration().quizQuestions && !examTimeline()) { … }to conditionally render the quiz navigation UI. The use of
@for (question of quizConfiguration().quizQuestions; track question; let i = $index)ensures that the list of quiz questions is iterated over efficiently.
45-89: Quiz Questions Details: Iterative Rendering with Conditional Disabling
Within the details block, each question is rendered based on its type (MULTIPLE_CHOICE, DRAG_AND_DROP, SHORT_ANSWER). Notable updates include:
- Passing
[quizQuestions]="quizConfiguration().quizQuestions"to the multiple-choice component (line 57).- Consistently using
[clickDisabled]="readonly()"for each question type to control interactivity (lines 59, 71, and 83).These changes standardize the UI behavior across quiz question types. Additionally, the presence of TODO comments regarding “Map vs Array consistency” serves as a reminder for potential future improvements.
| updateViewFromSubmission(): void { | ||
| if (this.studentSubmission.text) { | ||
| this.answer = this.studentSubmission.text; | ||
| } else { | ||
| this.answer = ''; | ||
| } | ||
| this.answer = this.studentSubmission().text ?? ''; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in updateViewFromSubmission.
The current implementation could be more type-safe when handling potentially undefined values.
updateViewFromSubmission(): void {
- this.answer = this.studentSubmission().text ?? '';
+ const submission = this.studentSubmission();
+ if (!submission) {
+ this.answer = '';
+ return;
+ }
+ this.answer = submission.text ?? '';
}📝 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.
| updateViewFromSubmission(): void { | |
| if (this.studentSubmission.text) { | |
| this.answer = this.studentSubmission.text; | |
| } else { | |
| this.answer = ''; | |
| } | |
| this.answer = this.studentSubmission().text ?? ''; | |
| } | |
| updateViewFromSubmission(): void { | |
| const submission = this.studentSubmission(); | |
| if (!submission) { | |
| this.answer = ''; | |
| return; | |
| } | |
| this.answer = submission.text ?? ''; | |
| } |
| initQuiz() { | ||
| // randomize order | ||
| // in the exam timeline, we do not want to randomize the order as this makes it difficult to view the changes between submissions. | ||
| if (!this.examTimeline) { | ||
| this.quizService.randomizeOrder(this.quizConfiguration.quizQuestions, this.quizConfiguration.randomizeQuestionOrder); | ||
| if (!this.examTimeline()) { | ||
| this.quizService.randomizeOrder(this.quizConfiguration().quizQuestions, this.quizConfiguration().randomizeQuestionOrder); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify randomization behavior in exam timeline.
The randomization is skipped in exam timeline mode, but there's no test coverage for this behavior.
🏁 Script executed:
#!/bin/bash
# Search for test coverage of randomization in exam timeline
rg -A 5 "randomizeOrder.*examTimeline"Length of output: 40
I'll run an additional script to search for any test references to the "examTimeline" and "randomizeOrder" behavior in test file(s):
🏁 Script executed:
#!/bin/bash
# Searching test files for references to examTimeline
rg -n "examTimeline" --type ts
# Searching test files for invocations of randomizeOrder (including 'randomizeOrder(')
rg -n "randomizeOrder(" --type tsLength of output: 296
Attention: Missing Test Coverage for Randomization Behavior in Exam Timeline Mode
The logic skipping randomization in exam timeline mode appears correct; however, our searches indicate there are no tests verifying this behavior. Please add unit tests that confirm:
- When
examTimeline()returnstrue,quizService.randomizeOrder()is not invoked. - When
examTimeline()returnsfalse,quizService.randomizeOrder()is triggered as expected.
This will help ensure that future changes do not inadvertently break the intended behavior.
coolchock
changed the title
General: Migrate exam exercises module to signals, inject and standaloneGeneral: Migrate exercise group module 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