Lectures: Improve and refactor lecture attachment client validation
#9893
Conversation
github-actions
bot
added
tests
client
florian-glombik
changed the title
Lectures: Improve and refactor lecture attachment validationLectures: Improve and refactor lecture attachment client validation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
src/main/webapp/app/lecture/lecture-attachments.component.html (2)
55-55: Consider using class binding instead of ngClassFor better performance, consider replacing ngClass with class binding when there's only one class to toggle:
-<tr [ngClass]="{ 'edit-overlay-container': attachmentToBeUpdatedOrCreated()?.id === attachment?.id }"> +<tr [class.edit-overlay-container]="attachmentToBeUpdatedOrCreated()?.id === attachment?.id">
142-143: Consider adding validation error messagesThe form fields use formControlName for validation, but there are no visible error messages to guide users when validation fails. Consider adding error messages for better user experience.
<input type="text" class="form-control" id="attachmentName" name="attachmentName" formControlName="attachmentName" /> +<div class="invalid-feedback" *ngIf="form.get('attachmentName')?.invalid && form.get('attachmentName')?.touched"> + <div *ngIf="form.get('attachmentName')?.errors?.['required']">Name is required</div> +</div>src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts (3)
151-162: Enhance file upload test assertionsWhile the test correctly verifies the basic file upload flow, consider adding assertions to verify the form's validity state after setting values.
Add these assertions:
comp.form.patchValue({ attachmentName: 'Test File Name', releaseDate: dayjs(), }); fixture.detectChanges(); +expect(comp.form.valid).toBeTrue(); +expect(comp.isFormValid()).toBeTrue(); expect(uploadAttachmentButton.nativeElement.disabled).toBeFalse();
221-227: Add error state validation assertionsThe error handling test should verify that the form remains in an invalid state after the error occurs.
Add these assertions:
expect(comp.attachmentToBeUpdatedOrCreated()).toEqual(attachment); expect(comp.attachmentFile()).toBeUndefined(); expect(comp.erroredFile).toEqual(file); expect(comp.errorMessage).toBe(errorMessage); expect(comp.fileInput.nativeElement.value).toBe(''); +expect(comp.isFormValid()).toBeFalse();
Line range hint
1-419: Consider adding test documentationWhile the test cases are well-structured, adding descriptive comments for each test group would improve maintainability. Consider using Jest's
describeblocks to group related tests and document the testing strategy for:
- Form validation scenarios
- File upload workflows
- Error handling cases
Example structure:
describe('Form Validation', () => { // Group form validation related tests }); describe('File Upload Workflow', () => { // Group file upload related tests }); describe('Error Handling', () => { // Group error handling related tests });src/main/webapp/app/lecture/lecture-attachments.component.ts (3)
45-48: Prefer constructor injection overinject()for dependenciesIn components, it is recommended to use constructor injection for dependencies instead of the
inject()function. This enhances readability and consistency across the codebase. Consider injectingActivatedRoute,AttachmentService,LectureService,FileService, andFormBuildervia the constructor.Apply this diff to implement constructor injection:
- private readonly activatedRoute = inject(ActivatedRoute); - private readonly attachmentService = inject(AttachmentService); - private readonly lectureService = inject(LectureService); - private readonly fileService = inject(FileService); - private readonly formBuilder = inject(FormBuilder); + constructor( + private readonly activatedRoute: ActivatedRoute, + private readonly attachmentService: AttachmentService, + private readonly lectureService: LectureService, + private readonly fileService: FileService, + private readonly formBuilder: FormBuilder + ) { + effect( + () => { + this.notificationText = undefined; + this.routeDataSubscription?.unsubscribe(); + this.routeDataSubscription = this.activatedRoute.parent!.data.subscribe(({ lecture }) => { + if (this.lectureId()) { + this.lectureService.findWithDetails(this.lectureId()!).subscribe((lectureResponse: HttpResponse<Lecture>) => { + this.lecture.set(lectureResponse.body!); + this.loadAttachments(); + }); + } else { + this.lecture.set(lecture); + this.loadAttachments(); + } + }); + }, + { allowSignalWrites: true }, + ); + }
178-185: Simplify form resetting by callingthis.form.reset()without parametersIn the
clearFormValues()method, you are resetting the form and explicitly setting each control toundefined. Since you intend to reset all form controls to their initial state, you can simplify this by callingthis.form.reset()without parameters.Apply this diff to simplify the form resetting:
private clearFormValues(): void { - this.form.reset({ - attachmentName: undefined, - attachmentFileName: undefined, - releaseDate: undefined, - notificationText: undefined, - }); + this.form.reset(); }
207-209: Reset file input value safelyIn the
editAttachment()method, you check iffileInputexists before resetting its value. Ensure consistent null checks across your methods to prevent potential errors. Review other methods wherefileInputis accessed without null checks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/webapp/app/lecture/lecture-attachments.component.html(9 hunks)src/main/webapp/app/lecture/lecture-attachments.component.scss(1 hunks)src/main/webapp/app/lecture/lecture-attachments.component.ts(8 hunks)src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lecture/lecture-attachments.component.scss
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/lecture/lecture-attachments.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/lecture/lecture-attachments.component.ts (1)
src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (6)
src/main/webapp/app/lecture/lecture-attachments.component.html (3)
64-70: LGTM! Well-implemented download status indication
The download status handling with spinner animation provides good user feedback, and the implementation follows Angular best practices.
Line range hint 93-116: LGTM! Well-structured action buttons with proper state management
The implementation includes:
- Proper disabled state management
- Confirmation dialog for delete action
- Accessibility support
166-168: LGTM! Well-implemented file input validation
The file input validation includes:
- Visual feedback for invalid files
- Proper spacing with CSS
- Clear state management
src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts (1)
12-12: LGTM: Test setup properly configured for reactive forms
The test module setup correctly incorporates ReactiveFormsModule and properly mocks the OwlDateTime modules using MockModule, which aligns with the migration to reactive forms.
Also applies to: 18-18, 24-24, 89-97
src/main/webapp/app/lecture/lecture-attachments.component.ts (2)
138-139: Verify manual incrementation of the version property
You are manually incrementing the version property of attachmentToBeUpdatedOrCreated. Ensure that this aligns with your application's versioning strategy and that version conflicts are properly managed. If the server also handles versioning, manually incrementing it on the client side might lead to inconsistencies.
66-69:
Correct Subject cleanup in ngOnDestroy()
In the ngOnDestroy() method, you are calling unsubscribe() on dialogErrorSource, which is a Subject. Subjects do not have an unsubscribe() method. Instead, you should call complete() to properly terminate the Subject.
Apply this diff to fix the cleanup logic:
- this.dialogErrorSource.unsubscribe();
+ this.dialogErrorSource.complete();Likely invalid or redundant comment.
src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/lecture/lecture-attachments.component.ts (4)
69-74: Consider adding more specific validatorsThe form controls could benefit from more specific validators:
attachmentNamecould have a max length validatorreleaseDateshould have a validator to ensure it's not in the pastform: FormGroup = this.formBuilder.group({ - attachmentName: [undefined as string | undefined, [Validators.required]], + attachmentName: [undefined as string | undefined, [Validators.required, Validators.maxLength(255)]], attachmentFileName: [undefined as string | undefined], - releaseDate: [undefined as dayjs.Dayjs | undefined], + releaseDate: [undefined as dayjs.Dayjs | undefined, [this.futureDateValidator]], notificationText: [undefined as string | undefined], });
76-81: Simplify form validation logicThe validation logic using computed signals is complex and tightly coupled with the date picker component. Consider extracting the validation logic into a separate method for better maintainability.
-isFormValid = computed(() => this.statusChanges() === 'VALID' && this.isFileSelectionValid() && this.datePickerComponent()?.isValid()); +private validateDatePicker = computed(() => this.datePickerComponent()?.isValid() ?? false); +isFormValid = computed(() => { + return this.statusChanges() === 'VALID' + && this.isFileSelectionValid() + && this.validateDatePicker(); +});
135-142: Simplify attachment save logicThe save logic contains multiple null checks and property assignments. Consider extracting the attachment preparation logic into a separate method.
+private prepareAttachmentForSave(): void { + if (!this.attachmentToBeUpdatedOrCreated()) return; + + const attachment = this.attachmentToBeUpdatedOrCreated()!; + attachment.version!++; + attachment.uploadDate = dayjs(); + attachment.name = this.form.value.attachmentName ?? undefined; + attachment.releaseDate = this.form.value.releaseDate ?? undefined; +} saveAttachment(): void { - if (!this.attachmentToBeUpdatedOrCreated()) { - return; - } - this.attachmentToBeUpdatedOrCreated()!.version!++; - this.attachmentToBeUpdatedOrCreated()!.uploadDate = dayjs(); - this.attachmentToBeUpdatedOrCreated()!.name = this.form.value.attachmentName ?? undefined; - this.attachmentToBeUpdatedOrCreated()!.releaseDate = this.form.value.releaseDate ?? undefined; + this.prepareAttachmentForSave(); this.notificationText = this.form.value.notificationText ?? undefined;
286-289: Improve filename regex patternThe current regex pattern for extracting file extension could be more robust to handle edge cases like multiple dots in filename.
-private determineAttachmentNameBasedOnFileName(fileName: string): string { - const FILE_EXTENSION_REGEX = /\.[^/.]+$/; - return fileName.replace(FILE_EXTENSION_REGEX, ''); +private determineAttachmentNameBasedOnFileName(fileName: string): string { + const lastDotIndex = fileName.lastIndexOf('.'); + return lastDotIndex === -1 ? fileName : fileName.substring(0, lastDotIndex); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/lecture/lecture-attachments.component.html(9 hunks)src/main/webapp/app/lecture/lecture-attachments.component.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/lecture/lecture-attachments.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/lecture/lecture-attachments.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/lecture/lecture-attachments.component.ts (2)
Line range hint 1-22: LGTM! Clean imports and well-structured interface
The new imports and interface definition align well with the transition to reactive forms. The LectureAttachmentFormData interface is well-structured with appropriate optional properties.
196-198:
Add null check for fileInput access
This is a duplicate of a previous review comment about adding a null check for fileInput.
-this.fileInput.nativeElement.value = '';
+if (this.fileInput?.nativeElement) {
+ this.fileInput.nativeElement.value = '';
+}
Checklist
General
Client
Motivation and Context
This PR extends the current client validation in the lecture attachments add and edit view, converts the the validation to a signal based solution and fixes code smells.
This is a preparation for Lectures: Add status bar to lecture creation and edit mode #9655 and aims to keep #9655 at a manageable size of changes.
Description
formControlNameinstead ofngModelas this approach appears to be deprecated since Angular6isFormValidcheck instead of getter (= bad performance)Steps for Testing
Prerequisites:
Except for extended validation, the behavior should not differ from develop.
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
Performance Review
Code Review
Manual Tests
Screenshots
old version (with extra column for link)
new version
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests