Development: Migrate exam exercises client code to signals
#10329
Conversation
github-actions
bot
added
tests
client
coolchock
changed the title
General: Migrate exam exercises module to signals
WalkthroughThis pull request refactors multiple Angular components and test files related to exam submissions and exercise management. The changes primarily replace Angular decorator-based property declarations with functional calls (using helpers like Changes
Sequence Diagram(s)sequenceDiagram
participant ST as StudentExamTimelineComponent
participant FU as FileUploadComponent
participant SS as studentSubmission
ST->>SS: update(() => currentSubmission)
Note right of SS: Performs encapsulated update logic
SS-->>ST: Returns updated submission state
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
src/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts (1)
8-8: 🛠️ Refactor suggestionConsider mocking dependencies instead of importing full test module.
The current setup imports the entire
ArtemisTestModule, which violates our coding guideline to avoid full module imports. Consider creating a minimal test module with only the required dependencies.Replace the full module import with a minimal test configuration:
-import { ArtemisTestModule } from '../../../../test.module'; +import { MockModule } from 'ng-mocks'; +import { CommonModule } from '@angular/common'; TestBed.configureTestingModule({ - imports: [ArtemisTestModule], + imports: [MockModule(CommonModule)], + declarations: [ + TextExamSubmissionComponent, + MockComponent(ExerciseSaveButtonComponent) + ], + providers: [ + // Add only required providers + ] })Also applies to: 23-25
src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts (1)
40-46: 🛠️ Refactor suggestionAdd null check for exercises array.
The code should handle potential null values more safely.
- this.studentExam().exercises?.forEach((exercise) => { + const exercises = this.studentExam()?.exercises; + if (!exercises) { + return; + } + exercises.forEach((exercise) => { const item = new ExamExerciseOverviewItem(); item.exercise = exercise; item.icon = faHourglassHalf; this.examExerciseOverviewItems.push(item); });src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts (1)
155-172:⚠️ Potential issueAdd cleanup for Apollon editor subscription.
The Apollon editor's nextRender subscription might cause memory leaks if not properly cleaned up.
+ private apollonEditorSubscription?: Promise<void>; private async updateViewFromSubmissionVersion() { if (this.submissionVersion?.content) { let model = this.submissionVersion.content.substring(0, this.submissionVersion.content.indexOf('; Explanation:')); - await this.modelingEditor()!.apollonEditor!.nextRender; + this.apollonEditorSubscription = this.modelingEditor()!.apollonEditor!.nextRender; + await this.apollonEditorSubscription; model = model.replace('Model: ', ''); this.umlModel = JSON.parse(model); // ... } } + ngOnDestroy(): void { + if (this.apollonEditorSubscription) { + this.apollonEditorSubscription = undefined; + } + super.ngOnDestroy(); + }
🧹 Nitpick comments (29)
src/test/javascript/spec/component/exam/participate/exercises/file-upload-exam-submission.component.spec.ts (1)
179-179: LGTM! Consider grouping related test cases.The changes consistently apply the new property access pattern using function calls and proper input setting. However, the test organization could be improved.
Consider grouping related test cases into nested describe blocks for better organization:
describe('FileUploadExamSubmissionComponent', () => { + describe('File Validation', () => { + // Move file size and type validation tests here + it('Too big file can not be submitted', ...); + it('Incorrect file type can not be submitted', ...); + }); + + describe('File Upload', () => { + // Move file upload and saving related tests here + describe('saveUploadedFile', ...); + }); });Also applies to: 183-183, 197-197, 203-203, 221-222, 231-231, 250-251, 260-260, 292-292, 295-295
src/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts (2)
37-134: Enhance test coverage with error cases and boundary testing.While the current test cases cover the basic functionality well, consider adding:
- Error cases (e.g., invalid input handling)
- Boundary cases (e.g., maximum text length, special characters)
- Edge cases (e.g., null/undefined submissions)
Example test cases to add:
it('should handle null submission gracefully', () => { fixture.componentRef.setInput('exercise', exercise); fixture.componentRef.setInput('studentSubmission', null); fixture.detectChanges(); expect(component.answer).toBe(''); }); it('should handle maximum text length', () => { fixture.componentRef.setInput('exercise', exercise); const maxLengthText = 'a'.repeat(10000); // Adjust based on actual max length textSubmission.text = maxLengthText; fixture.componentRef.setInput('studentSubmission', textSubmission); fixture.detectChanges(); expect(component.answer).toBe(maxLengthText); });
71-71: Standardize assertion methods according to guidelines.Update assertions to consistently use the recommended methods:
- Use
toBeTrue()/toBeFalse()for boolean checks- Use
toHaveBeenCalledExactlyOnceWith()for spy callsApply these changes:
-expect(component.hasUnsavedChanges()).toBeTrue(); +expect(component.hasUnsavedChanges()).toBeTrue(); -expect(component.studentSubmission().isSynced).toBeFalse(); +expect(component.studentSubmission().isSynced).toBeFalse(); -expect(saveExerciseSpy).toHaveBeenCalledOnce(); +expect(saveExerciseSpy).toHaveBeenCalledExactlyOnceWith();Also applies to: 114-114, 133-133
src/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.component.spec.ts (2)
47-47: Consider using more descriptive test data.While initializing
answerOptionsas an empty array is good practice, consider:
- Adding test answer options to validate more scenarios.
- Using a more descriptive text for
shortAnswerQuestionthat reflects its purpose in the test.- multipleChoiceQuestion.answerOptions = []; + multipleChoiceQuestion.answerOptions = [ + { id: 101, text: 'Test Option 1' } as AnswerOption, + { id: 102, text: 'Test Option 2' } as AnswerOption, + ]; - shortAnswerQuestion.text = 'Short answer question text'; + shortAnswerQuestion.text = 'What is the capital of France?';Also applies to: 52-52
213-247: Reduce test setup duplication.The setup code for
quizExerciseand component inputs is duplicated across test cases. Consider moving common setup to thebeforeEachblock.beforeEach(() => { + const quizExercise = new QuizExercise(new Course(), undefined); + quizExercise.quizQuestions = [dragAndDropQuestion]; + const quizConfiguration: QuizConfiguration = { + quizQuestions: [multipleChoiceQuestion, dragAndDropQuestion, shortAnswerQuestion] + }; + return TestBed.configureTestingModule({ // ... existing setup ... }) .compileComponents() .then(() => { fixture = TestBed.createComponent(QuizExamSubmissionComponent); component = fixture.componentInstance; quizService = TestBed.inject(ArtemisQuizService); + fixture.componentRef.setInput('exercise', quizExercise); + fixture.componentRef.setInput('quizConfiguration', quizConfiguration); }); });src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts (6)
49-49: Consider safe DOM interaction with ElementRef.
Directly interacting with the DOM throughElementRefcan raise maintainability and security concerns (e.g., XSS). Whenever possible, prefer Angular abstractions likeRenderer2for DOM manipulations.
74-74: Avoid repeated calls tothis.exercise()in performance-critical code.
Callingthis.exercise()multiple times can create overhead if it triggers re-computation or re-render. If performance becomes a concern, consider storing the result in a local variable.
120-123: Ensure exercise is always present.
These lines returnthis.exercise().idor the exercise object. Confirm that any parent or test code properly initializes the input signals. A defensive check might guard againstundefined.
142-142: Combine conditions for clarity.
The expression(this.studentSubmission().isSynced && this.studentSubmission().filePath) || (this.examTimeline() && ...)can be refactored for readability if it becomes more complex in the future.
160-160: Handle large file uploads gracefully.
Make sure the service call handles network or server errors properly. AlthoughonError()is present, consider additional user feedback if the file is especially large or the upload lags.
163-165: Avoid repetitive property access.
SettingfilePath,isSynced, andsubmittedon the submission is correct. For performance or readability reasons, consolidate updates if these fields must be changed together often.src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts (4)
90-91: Favor local caching if frequent re-evaluation.
Repeated calls tothis.exercise()can cause re-invocations if it’s a signal. If performance is critical, store a local reference. Otherwise, this is a concise approach.
120-121: Check for participation consistency when merging objects.
Spreadingthis.studentParticipation()into a new object is correct. Just ensure the source participation truly matches the exercise associated.
139-145: Automatic submission synchronization logic.
UpdatingsubmittedandisSyncedwhencommitStatechanges is acceptable, but watch out for any race conditions if there's parallel polling or server updates. Logically, this looks consistent.
167-168: Auto-save on update is a good approach.
this.codeEditorContainer().actions.onSave();is a neat way to ensure continuity. Consider adding user feedback (e.g., a small message) to clarify that a save and not a commit occurred.src/main/webapp/app/exam/participate/exercises/exam-submission.component.ts (1)
32-33: Validate boolean default inputs.
Usingreadonly = input(false);andexamTimeline = input(false);sets a default offalse. Confirm external bindings pass boolean values to avoid accidental type coercions.src/test/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.ts (1)
59-61: Improve type safety in optional chaining.The optional chaining could be strengthened with type guards.
- const studentExamValue = comp.studentExam?.(); // Optional chaining to handle potential undefined. - - const exerciseWithParticipations = studentExamValue?.exercises?.find((ex) => ex.studentParticipations && ex.studentParticipations.length > 0); + const studentExamValue = comp.studentExam(); + if (!studentExamValue) { + return; + } + const exerciseWithParticipations = studentExamValue.exercises?.find((ex) => ex.studentParticipations && ex.studentParticipations.length > 0);src/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.ts (1)
60-61: Improve test readability with local variables.Multiple accesses to
studentParticipation().submissions![0]could be simplified using a local variable.+ const submission = component.studentParticipation().submissions![0]; - expect(component.studentParticipation().submissions![0].submitted).toBeTrue(); - expect(component.studentParticipation().submissions![0].isSynced).toBeTrue(); + expect(submission.submitted).toBeTrue(); + expect(submission.isSynced).toBeTrue();Also applies to: 68-68, 71-71, 78-78
src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts (1)
69-69: Address the TODO comment about extracting common logic.The TODO comment indicates a need to extract common logic for calculating exercise status.
Would you like me to help create a separate service or utility class to handle the exercise status calculation logic?
src/test/javascript/spec/component/exam/participate/exam-exercise-update-highlighter.component.spec.ts (1)
91-99: Enhance test coverage for programming exercise case.The test for programming exercises could be more comprehensive by verifying the behavior when toggling the problem statement.
it('should not highlight differences for programming exercise', () => { // For programming exercises, the highlighting of differences is handled in the programming-exercise-instruction.component.ts. // Therefore, the highlighting of differences is not called and updatedProblemStatementWithHighlightedDifferencesHTML // and updatedProblemStatementHTML remain undefined const highlightDifferencesSpy = jest.spyOn(component, 'highlightProblemStatementDifferences'); expect(highlightDifferencesSpy).not.toHaveBeenCalled(); expect(component.updatedProblemStatementWithHighlightedDifferencesHTML).toBeUndefined(); expect(component.updatedProblemStatementHTML).toBeUndefined(); + + // Verify that toggling doesn't affect programming exercises + component.toggleHighlightedProblemStatement(new MouseEvent('click')); + expect(component.updatedProblemStatementWithHighlightedDifferencesHTML).toBeUndefined(); + expect(component.updatedProblemStatementHTML).toBeUndefined(); });src/test/javascript/spec/component/exam/participate/exercises/modeling-exam-submission.component.spec.ts (2)
196-219: Add test cases for error scenarios.The test suite should include cases for invalid JSON and Apollon editor errors.
it('should handle invalid JSON in submission version', async () => { jest.spyOn(comp, 'modelingEditor').mockReturnValue({ apollonEditor: { nextRender: Promise.resolve() } as unknown as ApollonEditor, } as unknown as ModelingEditorComponent); const submissionVersion = { content: 'Model: invalid-json; Explanation: explanation', } as unknown as SubmissionVersion; await comp.setSubmissionVersion(submissionVersion); expect(comp.umlModel).toBeUndefined(); expect(comp.explanationText).toBe('explanation'); }); it('should handle Apollon editor errors', async () => { jest.spyOn(comp, 'modelingEditor').mockReturnValue({ apollonEditor: { nextRender: Promise.reject(new Error('Apollon error')) } as unknown as ApollonEditor, } as unknown as ModelingEditorComponent); const submissionVersion = { content: 'Model: {}; Explanation: explanation', } as unknown as SubmissionVersion; await expect(comp.setSubmissionVersion(submissionVersion)).rejects.toThrow('Apollon error'); });
152-165: Add assertions for error handling in updateSubmissionFromView test.The test for updateSubmissionFromView should verify error handling behavior.
describe('updateSubmissionFromView', () => { it('should set submission model to new model from modeling editor', () => { fixture.detectChanges(); const modelingEditor = fixture.debugElement.query(By.directive(ModelingEditorComponent)).componentInstance; const newModel = { newModel: true }; const currentModelStub = jest.spyOn(modelingEditor, 'getCurrentModel').mockReturnValue(newModel); const explanationText = 'New explanation text'; comp.explanationText = explanationText; comp.updateSubmissionFromView(); expect(comp.studentSubmission().model).toEqual(JSON.stringify(newModel)); expect(currentModelStub).toHaveBeenCalledTimes(2); expect(comp.studentSubmission().explanationText).toEqual(explanationText); }); + + it('should handle JSON.stringify errors', () => { + fixture.detectChanges(); + const modelingEditor = fixture.debugElement.query(By.directive(ModelingEditorComponent)).componentInstance; + const circularRef: any = {}; + circularRef.self = circularRef; + jest.spyOn(modelingEditor, 'getCurrentModel').mockReturnValue(circularRef); + + const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); + comp.updateSubmissionFromView(); + + expect(consoleSpy).toHaveBeenCalledWith('Failed to stringify model:', expect.any(Error)); + expect(comp.studentSubmission().model).toBeUndefined(); + }); });src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.html (2)
8-9: Dynamic Attribute Bindings for Points and Bonus
The updated bindings for[jhiTranslate]and[translateValues]now correctly callexercise(). Note that becauseexercise()is invoked several times in this small expression, if the function becomes non-trivial in the future, consider caching its result to reduce redundant evaluations.
36-37: Correct Usage of Read-Only and Disabled Bindings
Using[readOnly]="readonly() || !studentSubmission()"and[disabled]="readonly() || !studentSubmission()"is correct. As with other function calls in templates, ensure that these are lightweight to avoid performance issues during change detection.src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.html (1)
7-8: Updated Attribute Bindings for Bonus and Points
Using function calls within[jhiTranslate]and[translateValues]is consistent. As with similar bindings in other files, consider the potential impact of multiple invocations if the functions grow complex.src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.html (2)
6-7: Updated Translation and Points Binding
The attribute bindings for bonus points and maximum points now useexercise()correctly. As with other files, be mindful of multiple function calls possibly affecting performance if the compute logic changes later.
16-29: Online Editor Block with Dynamic Access
The block starting with@if (exercise().allowOnlineEditor) {now wraps the online code editor container. Key bindings such as[participation]="studentParticipation()"and[course]="getCourseFromExercise(exercise())"correctly use function calls. Ensure thatgetCourseFromExercise()is efficient since it is called during change detection cycles.src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.html (2)
20-20: Iterating Over Quiz Questions for Navigation
The use of@for (question of quizConfiguration().quizQuestions; track question; let i = $index)is correctly updated. Be cautious if the function call toquizConfiguration()becomes expensive when iterated multiple times.
78-88: Drag and Drop and Short Answer Question Bindings
The<jhi-drag-and-drop-question>and<jhi-short-answer-question>components are updated with appropriate bindings (e.g.[clickDisabled]="readonly()"). The changes maintain the overall data-driven style; a TODO remains regarding map vs. array consistency, which should be addressed separately.
📜 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/**/*.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/text/text-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/exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.tssrc/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.tssrc/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts
`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/participate/exam-exercise-overview-page.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.tssrc/test/javascript/spec/component/exam/manage/student-exams/student-exam-timeline.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/modeling-exam-submission.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.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-update-highlighter.component.spec.tssrc/test/javascript/spec/component/exam/participate/exercises/text-exam-submission.component.spec.ts
`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/programming/programming-exam-submission.component.htmlsrc/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.htmlsrc/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.html
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (65)
src/test/javascript/spec/component/exam/participate/exercises/file-upload-exam-submission.component.spec.ts (2)
42-43: LGTM! Improved component initialization.The changes improve the test setup by:
- Using
fixture.componentRef.setInput()for setting inputs, which is the recommended approach- Simplifying the component initialization code
Also applies to: 59-64
77-77: LGTM! Consistent property access pattern.The changes maintain consistency by:
- Using
fixture.componentRef.setInput()for setting inputs- Using function calls (e.g.,
exercise()) instead of direct property accessAlso applies to: 85-85, 97-99
src/test/javascript/spec/component/exam/participate/exercises/quiz-exam-submission.component.spec.ts (3)
29-30: LGTM!The imports are correctly added from their respective paths.
82-84: LGTM! Proper use ofsetInputfor component inputs.The change to use
fixture.componentRef.setInput()aligns with Angular's testing best practices for setting component inputs.Also applies to: 97-99, 140-142, 164-166, 175-177, 220-222, 238-240
132-132:❓ Verification inconclusive
Verify the change in accessing
studentSubmission.The code now accesses
studentSubmissionas a function, which suggests a change in the component's implementation (possibly using signals or getters). Ensure this change is consistent with the component's implementation.Also applies to: 207-208
🏁 Script executed:
#!/bin/bash # Search for the studentSubmission property/method definition in the component ast-grep --pattern 'class QuizExamSubmissionComponent { $$$ studentSubmission$_($$$) { $$$ } $$$ }'Length of output: 113
Verify the Implementation of
studentSubmissionThe test expectation now calls
component.studentSubmission()instead of accessing a property directly. This indicates that the component may have been refactored to exposestudentSubmissionas a method (or via a getter/signal), rather than as a direct property. However, our initial automated search usingast-grepdid not reveal a concrete definition forstudentSubmissionin the component, so it’s unclear whether this change was intentional or if the component implementation needs to be adjusted accordingly.Action Items:
- Manual Verification Required:
Please double-check the implementation ofQuizExamSubmissionComponent(and specifically the definition ofstudentSubmission) to confirm whether it is now implemented as a method (or getter) returning an object that contains theisSyncedproperty.- Review Consistency:
Verify that the changes seen in the test file (lines 132 and 207–208) consistently match the component’s implementation. IfstudentSubmissionwas intended to be a function or getter, ensure that its usage across the codebase reflects this decision.src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts (7)
51-52: Validate required signals usage.
Usingmodel.required<FileUploadSubmission>()andinput.required<FileUploadExercise>()is consistent with the signal-based approach. Ensure that your upstream code always provides these required properties, or handle the case where they might be undefined.
96-96: Check for null or undefined before using non-null assertions.
this.exercise().filePattern!assumesfilePatternis never null or undefined. To improve robustness, validate or provide a fallback forfilePatternbefore splitting.
103-103: Prevent accidental state stomping onisSynced.
this.studentSubmission().isSynced = false;is fine if this is the intended logic. Just double-check that togglingisSyncedhere aligns well with other synchronization logic in the codebase.
[approve]
116-116: Validate due date logic for consistency.
Ensure thedueDatecomparison is correct, especially if there's any time zone or daylight saving logic. Usingdayjsis good, but confirm that you handle time offsets consistently across different locales.
127-127: Verify mutable state changes.
return !this.studentSubmission().isSynced!;depends on calling a function that may cause re-check every time. Verify that any subsequent code also respects the unsaved-changes state to prevent user confusion.
131-131: Consistency in retrieval pattern.
return this.studentSubmission();is consistent with the new function-based approach. No immediate issues, just ensure that the code reading this is updated accordingly.
145-145: Check for safe array splitting.
this.studentSubmission()!.filePath!.split('/')uses multiple non-null assertions. Always confirm the presence offilePathin suspicious edge cases (e.g., empty string).src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.ts (7)
59-65: Ensure correct usage of required signals.
The shifted approach usingviewChild.required(...)andinput.required(...)aligns with your functional property-binding style. Check that each property is defined at runtime; if optional usage is ever needed, consider a non-required approach or default values.
77-80: Guard against empty submissions array.
When returningstudentParticipation.submissions[0], ensure your logic (or upstream code) can handle the case wheresubmissionsis unexpectedly empty.
86-87: No concerns: retrieving exercise id.
Returningthis.exercise().idlooks straightforward. Confirm the ID is always set for exam exercises.
112-112: Review re-initialization on activate.
Callingthis.instructions().updateMarkdown();ononActivate()is good for re-rendering the instructions, but verify if this might cause unnecessary repeated updates in certain navigation flows.
128-129: Prevent overwriting local state if already set.
this.submissionCount = this.studentParticipation().submissionCount ?? this.submissionCount;is safe, but confirm that reassigning an existingsubmissionCountdoesn’t conflict with server responses or UI updates.
152-155: Handle unsynced references carefully.
MarkingstudentParticipation.submissions[0].isSynced = false;ensures the submission status is updated, but verify if the UI or timeline for exam updates is also reflecting this change properly.
159-163: Respect offline vs. online editor conditions.
The conditional check forallowOfflineIdeis correct. Just confirm that returningfalseisn’t overshadowing other potential unsaved changes in the code editor.src/main/webapp/app/exam/participate/exercises/exam-submission.component.ts (1)
4-4: Transition to function-based inputs is consistent with your refactor.
inputfrom@angular/coreis recognized in your environment. Verify that code referencing legacy@Input()usage is fully removed or updated across all files.
[approve]src/test/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.ts (1)
15-15: LGTM! Test setup improvements.The initialization of
studentExamis now properly separated and follows best practices for test organization.Also applies to: 28-42
src/test/javascript/spec/component/exam/participate/exercises/programming-exam-submission.component.spec.ts (1)
47-48: LGTM! Consistent use of setInput.The changes properly use
fixture.componentRef.setInputfor setting component inputs, following Angular's best practices.Also applies to: 65-66, 75-76, 86-86
src/main/webapp/app/exam/participate/exercises/exam-exercise-update-highlighter/exam-exercise-update-highlighter.component.ts (1)
26-28: LGTM! Proper use of new input/output syntax.The component correctly uses the new Angular input/output syntax with proper typing.
src/main/webapp/app/exam/participate/exercises/exercise-overview-page/exam-exercise-overview-page.component.ts (1)
24-32: LGTM! Proper dependency injection and input/output setup.The component correctly uses dependency injection for ChangeDetectorRef and properly implements the new input/output syntax.
src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.ts (1)
46-47: LGTM! Input migration looks good.The migration from @input decorators to input.required() is correctly implemented, maintaining type safety and required status.
Also applies to: 47-48
src/test/javascript/spec/component/exam/participate/exam-exercise-update-highlighter.component.spec.ts (1)
19-42: LGTM! Test setup is well structured.The test setup is properly organized with beforeEach and uses the recommended fixture.componentRef.setInput approach.
src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts (1)
44-44: LGTM! ViewChild migration looks good.The migration from @ViewChild decorator to viewChild.required is correctly implemented.
src/test/javascript/spec/component/exam/manage/student-exams/student-exam-timeline.component.spec.ts (1)
166-168: LGTM! Mock implementation aligns with the new functional approach.The addition of the
studentSubmissionobject with anupdatemethod in the mock correctly reflects the component's transition to a more functional style.src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts (2)
61-65: LGTM! ViewChildren decorators correctly replaced with viewChildren function.The transition from @ViewChildren decorators to viewChildren function calls follows Angular's new signal-based approach.
68-71: LGTM! Input decorators correctly replaced with input function.The transition from @input decorators to input function calls is well-implemented, with proper use of required inputs where necessary.
src/main/webapp/app/exam/manage/student-exams/student-exam-timeline/student-exam-timeline.component.ts (1)
163-163: LGTM! Correctly updated to use functional update approach.The change from direct assignment to using the
updatemethod is consistent with the signal-based approach and maintains reactivity.src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.html (2)
1-1: LGTM! Correctly using new Angular control flow syntax.The template properly uses the new
@ifsyntax instead of*ngIf, following the latest Angular guidelines.Also applies to: 26-26
5-5: LGTM! Property access correctly updated to function calls.The template consistently updates all property access to function calls (e.g.,
exercise(),studentSubmission(),examTimeline()), aligning with the signal-based approach.Also applies to: 8-9, 12-13, 16-16, 20-22, 30-32, 48-49
src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.html (7)
1-1: Consistent Function Invocation in Conditional Directive
The change from property access to invokingexercise()in the@ifblock is correctly implemented to ensure dynamic evaluation.
5-5: Proper Binding for Exercise Group Title
Using the function call with safe navigation ({{ exercise().exerciseGroup?.title }}) is correct and aligns with the new dynamic access pattern.
12-13: Conditional Rendering for Score Badge
The condition usingexercise().includedInOverallScoreand the subsequent badge component inclusion is implemented correctly.
16-16: Updated Submission Binding
The<jhi-exercise-save-button>now correctly receives its submission data viastudentSubmission(). This change is consistent with other dynamic accesses in the template.
20-20: Exam Timeline Binding Updated
The[examTimeline]input now usesexamTimeline(), ensuring that the value is evaluated at run time. Verify that the returned value’s type meets the component’s requirement.
22-22: Dynamic Exercise Title in Left Panel
The expression{{ examTimeline() ? exercise().title : ('artemisApp.exam.yourSolution' | artemisTranslate) }}properly utilizes the updated function call syntax forexercise().
50-51: Proper Invocation for Update Highlighter Component
The conditional block and the binding, usingexercise()to pass the current exercise to<jhi-exam-exercise-update-highlighter>, is correctly updated.src/main/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.html (11)
1-1: Dynamic Function Call in Conditional for File Upload
The change to@if (exercise()) {ensures that the condition checks the evaluated object, reflecting the latest state.
4-4: Correct Display of Exercise Group Title
The interpolation now correctly callsexercise()to retrieve the exercise group title.
11-12: Conditional Badge Rendering for Overall Score
The directive now uses@if (exercise().includedInOverallScore !== IncludedInOverallScore.INCLUDED_COMPLETELY)along with the badge component correctly.
16-16: Exam Timeline in Resizeable Container
The<jhi-resizeable-container>now receives its timeline data viaexamTimeline(), which is consistent with the new style.
18-18: Dynamic Title in Left Panel
The expression for the exercise title correctly usesexamTimeline() ? exercise().title : ...ensuring dynamic resolution.
21-21: Comprehensive Conditional Check in File Input Section
The@ifcondition now includes multiple function calls (exercise() && studentSubmission() && !readonly()) and is correctly updated to reflect the new accessor style.
39-39: For-Loop with File Pattern Parsing
Using@for (extension of exercise().filePattern!.split(','); track extension)is correctly updated. The non-null assertion onfilePatternshould be double-checked elsewhere to ensure it never unexpectedly becomes null.
50-50: Conditional Handling of Submitted File Details
The condition@if (submittedFileName && studentSubmission()?.filePath)now safely handles potentially undefined values with optional chaining—this is a solid defensive change.
53-53: Download File Action with Non-null Assertion
The download link callsdownloadFile(studentSubmission()!.filePath!)using the non-null assertions. Ensure that these assertions are justified by the component’s logic so that they do not lead to runtime errors.
61-61: Conditional Display When No File is Submitted
The directive@if (!submittedFileName && examTimeline())appropriately reflects the updated data access pattern.
73-74: Conditional Rendering for Problem Statement Update
The block that renders the<jhi-exam-exercise-update-highlighter>now properly invokesexercise(). This is consistent with the refactored style and ensures up-to-date data is passed.src/main/webapp/app/exam/participate/exercises/programming/programming-exam-submission.component.html (8)
3-4: Dynamic Binding for Exercise Group in Programming Component
Changing the interpolation to{{ exercise().exerciseGroup?.title }}ensures that the current exercise state is always used.
10-10: Conditional Badge Display in Programming Submission
The updated@if (exercise().includedInOverallScore !== IncludedInOverallScore.INCLUDED_COMPLETELY)statement matches the refactored access method.
31-34: Offline IDE Action Handling Within Editor Title Actions
The conditional@if (exercise().allowOfflineIde) {and subsequent rendering of<jhi-exercise-details-student-actions>are consistent with the new accessor style.
41-47: Updating Result Component Bindings in Editor Toolbar
In the editor toolbar, the@if (studentParticipation())block now correctly passes data to<jhi-updating-result>with function calls. This ensures that the display of ungraded results and progress bar information is always up-to-date.
53-58: Trigger Build Button with Updated Exercise and Participation Data
The<jhi-programming-exercise-student-trigger-build-button>receives itsexerciseandparticipationusing function calls. This is implemented consistently with the online editor section.
63-64: Programming Exercise Instructions Binding
The<jhi-programming-exercise-instructions>component now correctly obtains bothexercise()andstudentParticipation()data. This ensures that editor-side instructions reflect the current state.
72-78: Offline IDE Block for Programming Component
The block beginning with@if (!exercise().allowOnlineEditor && exercise().allowOfflineIde) {handles the offline IDE scenario. All components inside—such as student actions, submission policy status, and repository lock notices—correctly use function calls. Verify that these two scenarios (online vs. offline) are mutually exclusive and that the template logic is thoroughly tested.
95-96: Reusing Instructions Component in Offline Mode
The conditional and binding for<jhi-programming-exercise-instructions>in the offline IDE section are consistent with previous usage.src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.html (6)
4-4: Dynamic Binding for Quiz Exercise Group
The interpolation{{ quizConfiguration().exerciseGroup?.title }}correctly updates the reference to use the new function invocation style.
6-7: Translation and Points Binding for Quiz
The updated binding[translateValues]="{ points: quizConfiguration().maxPoints }"and the conditional block for overall score badge now correctly callquizConfiguration().
11-11: Updated Save Button for Quiz Submission
The<jhi-exercise-save-button>uses[submission]="studentSubmission()"correctly, maintaining consistency with other exercise types.
16-16: Conditional Rendering of Quiz Navigation
The condition@if (quizConfiguration().quizQuestions && !examTimeline())ensures that quiz navigation is displayed only when quiz questions are available and the exam timeline is not active.
60-64: Rendering Quiz Questions for Detailed View
The block starting with@if (quizConfiguration().quizQuestions) {followed by a loop to render individual quiz questions now properly passes the entire quiz questions array via[quizQuestions]="quizConfiguration().quizQuestions". Additionally,[clickDisabled]="readonly()"is correctly set.
72-75: Multiple Choice Question Component Bindings
Within the for-loop for quiz questions, the<jhi-multiple-choice-question>component receives the quiz questions array and the disabled state via function calls. This is a correct and consistent update.
...est/javascript/spec/component/exam/participate/exam-exercise-overview-page.component.spec.ts
Show resolved
Hide resolved
...ate/exercises/exam-exercise-update-highlighter/exam-exercise-update-highlighter.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exam/participate/exercises/text/text-exam-submission.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exam/participate/exercises/modeling/modeling-exam-submission.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exam/participate/exercises/quiz/quiz-exam-submission.component.ts
Show resolved
Hide resolved
coolchock
dismissed stale reviews from ole-ve, HawKhiem, and coderabbitai[bot]
via
12e658a
...n/webapp/app/exam/participate/exercises/file-upload/file-upload-exam-submission.component.ts
Show resolved
Hide resolved
krusche
changed the title
General: Migrate exam exercises module to signalsDevelopment: Migrate exam exercises client code to signals
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
coolchock
added
the
maintainer-approved
Checklist
General
Client
Description
This PR updates the
exam-exercisesmodule to use SignalsSteps for Testing
Prerequisites:
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Summary by CodeRabbit