Development: Use signals in lecture add attachment form
#9656
Conversation
github-actions
bot
added
the
client
florian-glombik
added
code quality
lecture
.../lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts
Outdated
Show resolved
Hide resolved
florian-glombik
changed the title
Development: Use signals in lecture componentsDevelopment: Use signals in attachment unit form component
florian-glombik
changed the title
Development: Use signals in attachment unit form componentDevelopment: Use signals for lecture add attachment form
WalkthroughThe changes in this pull request primarily involve refactoring 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: 1
🧹 Outside diff range and nitpick comments (8)
src/main/webapp/app/shared/constants/file-extensions.constants.ts (1)
48-50: Add JSDoc documentation for consistency.The new constants should follow the same documentation pattern as other constants in this file for better maintainability.
Apply this diff to add documentation:
+/** + * A human-readable string of allowed file extensions, formatted with commas. + * Generated from UPLOAD_FILE_EXTENSIONS for display purposes. + */ export const ALLOWED_FILE_EXTENSIONS_HUMAN_READABLE = UPLOAD_FILE_EXTENSIONS.join(', '); +/** + * A string of file extensions prefixed with dots, joined with commas. + * Generated from UPLOAD_FILE_EXTENSIONS for use in HTML file input's accept attribute. + */ -// The list of file extensions for the "accept" attribute of the file input field export const ACCEPTED_FILE_EXTENSIONS_FILE_BROWSER = UPLOAD_FILE_EXTENSIONS.map((ext) => '.' + ext).join(',');src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html (1)
69-71: Consider converting fileInputTouched to a signal for consistency.While the change to use
fileName()signal is good, consider convertingfileInputTouchedto a signal as well for a more consistent reactive approach across the component's state management.-@if (!fileName() && fileInputTouched) { +@if (!fileName() && fileInputTouched()) {src/main/webapp/app/lecture/lecture-attachments.component.ts (4)
20-26: Consider moving icon declarations to a shared constants file.While the protected readonly declarations are good for encapsulation, these icons might be used across multiple components. Moving them to a shared constants file (e.g.,
icon.constants.ts) would improve reusability and maintain consistency.
Line range hint
64-74: Prevent memory leaks by properly managing subscriptions.The subscriptions in
loadAttachmentsand other methods aren't being cleaned up, which could lead to memory leaks. Consider using thetakeUntiloperator with a destroy subject.Example implementation:
export class LectureAttachmentsComponent implements OnInit, OnDestroy { private readonly destroy$ = new Subject<void>(); loadAttachments(): void { this.attachmentService .findAllByLectureId(this.lecture.id!) .pipe(takeUntil(this.destroy$)) .subscribe(/*...*/); } ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); this.dialogErrorSource.unsubscribe(); } }
Line range hint
146-152: Improve error handling to avoid exposing sensitive information.The error message from
HttpErrorResponseis directly exposed to the UI. Consider implementing a proper error handling service that maps error responses to user-friendly messages.Example implementation:
private handleFailedUpload(error: HttpErrorResponse): void { this.errorMessage = this.errorHandlingService.getErrorMessage(error); // ... rest of the code }
Line range hint
191-206: Consider using reactive forms and improve type safety.
- The attachment form could benefit from using reactive forms for better validation and state management.
- The event handler
setLectureAttachmentcould use a more specific type thanEvent.Example implementation:
import { FormBuilder, FormGroup, Validators } from '@angular/forms'; export class LectureAttachmentsComponent { attachmentForm: FormGroup; constructor(private fb: FormBuilder) { this.attachmentForm = this.fb.group({ name: ['', Validators.required], file: [null, Validators.required] }); } setLectureAttachment(event: InputEvent): void { const input = event.target as HTMLInputElement; // ... rest of the code } }src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (2)
85-85: Ensure user receives feedback when file is too largeWhen the selected file exceeds
MAX_FILE_SIZE,isFileTooBigis set totrue. Please confirm that appropriate user feedback is provided in the UI to inform the user.Also applies to: 93-93
114-124: Add validation check before form submissionCurrently,
submitFormemitsformSubmittedwithout checking if the form is valid. To prevent submission of invalid data, consider adding a validation check.Apply this diff to add the validation:
+ if (!this.isFormValid()) { + return; + } const formValue = this.form.value; const formProperties: FormProperties = { ...formValue }; const fileProperties: FileProperties = { file: this.file, fileName: this.fileName(), }; this.formSubmitted.emit({ formProperties, fileProperties, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/webapp/app/lecture/lecture-attachments.component.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts(4 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/shared/constants/file-extensions.constants.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/lecture/lecture-attachments.component.ts (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.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-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
src/main/webapp/app/shared/constants/file-extensions.constants.ts (1)
🔇 Additional comments (10)
src/main/webapp/app/shared/constants/file-extensions.constants.ts (1)
48-50: LGTM! The implementation is correct and follows best practices.
The constants are:
- Properly derived from the base
UPLOAD_FILE_EXTENSIONS - Correctly formatted for their respective use cases
- Maintain synchronization with server-side definitions
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html (2)
63-68: LGTM! Proper usage of new Angular syntax.
The change to isFileTooBig() method call and the use of @if directive follows the new Angular guidelines correctly.
Line range hint 108-116: LGTM! Good use of computed validation.
The change to isFormValid() suggests a computed property that likely encapsulates all form validation checks in one place, which is a good practice for maintainability. The use of @if directive is also correct per the new Angular guidelines.
src/main/webapp/app/lecture/lecture-attachments.component.ts (2)
3-3: LGTM: Import changes are well-organized.
The imports are properly organized and the new constants improve code maintainability by centralizing file extension configurations.
Also applies to: 11-11
27-28: LGTM: File extension constants are properly encapsulated.
The use of centralized constants and protected readonly modifiers aligns with Angular best practices.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (5)
1-8: Imports are correctly updated for new features
The imports have been updated to include signal, computed, inject, and the new constants. This aligns with the use of Angular's reactive programming model and improves code organization.
35-39: Class properties initialized with new constants
The class properties now use the new file extension constants, enhancing maintainability and readability.
55-57: State management enhanced with signals
Introducing signals for fileName and isFileTooBig improves the component's reactivity and state management.
58-66: Form initialization streamlined using dependency injection
Using inject(FormBuilder) and initializing the form inline simplifies the code and follows Angular best practices.
69-71: Review validation logic in isFormValid computed property
The isFormValid computed property uses the condition (this.statusChanges() === 'VALID' || this.fileName()) && !this.isFileTooBig();. The logical OR with this.fileName() might allow form submission when form controls are invalid but a file is selected. Please verify if this behavior is intended.
github-actions
bot
added
tests
and removed
lecture
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit-form.component.spec.ts (1)
167-168: Consider adding size-specific assertions.While the test correctly uses the signal getter
isFileTooBig()and follows the coding guidelines for boolean assertions withtoBeTrue, consider adding an explicit assertion to verify the exact file size being tested:expect(fakeFile.size).toBe(MAX_FILE_SIZE + 1);This would make the test more specific and easier to debug if the file size validation logic changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts(4 hunks)src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit-form.component.spec.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (1)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit-form.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/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit-form.component.spec.ts (4)
8-8: LGTM! Proper use of ng-mocks utilities.
The import statement correctly uses MockComponent, MockDirective, and MockPipe from ng-mocks, following best practices for Angular component testing.
72-73: LGTM! Proper assertion for signal value.
The test correctly uses the signal getter method fileName() instead of direct property access, following Angular's signals pattern. The assertion follows the coding guidelines using toEqual for object comparison.
91-91: LGTM! Proper signal setter usage in form submission test.
The test correctly uses the signal setter fileName.set() and follows the coding guidelines for spy assertions with toHaveBeenCalledOnce.
133-133: LGTM! Proper signal setter usage in invalid form test.
The test correctly uses the signal setter and follows the coding guidelines for negative spy assertions with not.toHaveBeenCalled.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (2)
138-138: Approval: Correct use of signals in form value setting
Setting the fileName signal using this.fileName.set(formData?.fileProperties?.fileName); ensures that the reactive property accurately reflects the current state of the file name.
93-93: Verify the correct implementation of file size validation
The isFileTooBig signal is set based on this.file.size > MAX_FILE_SIZE. Ensure that MAX_FILE_SIZE is correctly defined and that users receive appropriate feedback when their file exceeds the allowed size.
Run the following script to confirm the definition and usage of MAX_FILE_SIZE:
✅ Verification successful
File size validation is correctly implemented
The validation is properly implemented with MAX_FILE_SIZE being defined in src/main/webapp/app/shared/constants/input.constants.ts as 20MB (20 * 1024 * 1024 bytes). This constant is:
- Correctly imported and used in the attachment-unit-form component
- Consistent with the server-side configuration in
application.yml - Well-tested as evidenced by test files
- Used consistently across the application for file upload validations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that MAX_FILE_SIZE is properly defined and consistently used.
# Test: Search for the definition of MAX_FILE_SIZE.
# Expectation: MAX_FILE_SIZE should be defined in input.constants.ts or a similar constants file.
rg 'export const MAX_FILE_SIZE' src/ -t typescript
# Test: Search for all usages of MAX_FILE_SIZE to ensure consistent application.
rg 'MAX_FILE_SIZE' src/ -t typescript
Length of output: 160
Script:
#!/bin/bash
# Search for MAX_FILE_SIZE definition and usage without file type restriction
rg 'export const MAX_FILE_SIZE' src/
# Search for all occurrences of MAX_FILE_SIZE
rg 'MAX_FILE_SIZE' src/
# Find potential constant files that might contain MAX_FILE_SIZE
fd 'constant' src/
Length of output: 4950
.../lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts
Show resolved
Hide resolved
.../lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts
Show resolved
Hide resolved
florian-glombik
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
florian-glombik
changed the title
Development: Use signals for lecture add attachment formDevelopment: Use signals in lecture add attachment form
Feras797
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
florian-glombik
requested review from
HawKhiem,
Haoyuli2002,
Feras797 and
HanyangXu0508
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
BBesrour
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
HanyangXu0508
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
florian-glombik
added
the
lecture
Checklist
General
Client
Motivation and Context
This PR introduces signals and fixes code smells in components used for editing and updating lectures. 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
attachment-unit-form.component.ts(and thereby removed method calls in the template)Steps for Testing
Prerequisites:
There is a known issue in Chrome with file uploads, if it fails and something like "Snap, did not work" is displayed, please use a non-chromium browser (e.g. Firefox)
Attachment>Add attachmentto upload an attachment, verify the behavior is the same as on developUnits>Fileto create a File lecture unit, verify the behavior is the same as on developThis is a refactoring PR that does not add functionality, so the behavior should not change in comparison to the develop branch.
There is a known issue (guided mode + competency not set => internal server error) that will not be fixed in this PR #9656 (review), see #9656 (comment)
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
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
LectureAttachmentsComponentandAttachmentUnitFormComponent.Refactor
LectureAttachmentsComponentandAttachmentUnitFormComponentfor better maintainability.CreateExerciseUnitComponentfor improved code clarity.AttachmentUnitFormComponent.Documentation
AttachmentUnitFormComponentto align with new property access methods, improving maintainability.