src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (3)

71-71: Improve type safety and readability of the validation check.

While the stricter validation aligns with the PR objectives, the string comparison can be improved.

Consider this more idiomatic approach:

-        return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value != '' && this.fileName();
+        return this.statusChanges() === 'VALID' && !this.isFileTooBig() && Boolean(this.nameControl?.value?.trim()) && Boolean(this.fileName());

This change:

• Uses trim() to handle whitespace-only values
• Uses Boolean() for explicit type conversion
• Maintains null-safety with the optional chaining

---

Line range hint 82-95: Add file type validation and error handling.

The file change handler should validate the file type against the accepted extensions.

Consider adding these validations:

     onFileChange(event: Event): void {
         const input = event.target as HTMLInputElement;
         if (!input.files?.length) {
             return;
         }
         this.file = input.files[0];
+        const fileExtension = this.file.name.split('.').pop()?.toLowerCase();
+        if (!fileExtension || !ACCEPTED_FILE_EXTENSIONS_FILE_BROWSER.includes(`.${fileExtension}`)) {
+            // Reset the input
+            input.value = '';
+            this.file = undefined;
+            this.fileName.set(undefined);
+            return;
+        }
         this.fileName.set(this.file.name);
         // automatically set the name in case it is not yet specified
         if (this.form && (this.nameControl?.value == undefined || this.nameControl?.value == '')) {
             this.form.patchValue({
                 // without extension
                 name: this.file.name.replace(/\.[^/.]+$/, ''),
             });
         }
         this.isFileTooBig.set(this.file.size > MAX_FILE_SIZE);
     }

---

Line range hint 38-40: Enhance component with accessibility and cleanup.

Consider these improvements for better maintainability and accessibility:

1. Implement OnDestroy to clean up subscriptions:

export class AttachmentUnitFormComponent implements OnChanges, OnDestroy {
    private destroy$ = new Subject<void>();

    ngOnDestroy(): void {
        this.destroy$.next();
        this.destroy$.complete();
    }
}

2. Add ARIA attributes to the file input in the template:

<input
    #fileInput
    type="file"
    [attr.aria-label]="'attachment.fileInput' | translate"
    [attr.aria-invalid]="isFileTooBig()"
    [attr.aria-describedby]="isFileTooBig() ? 'file-size-error' : undefined"
/>

3. Consider extracting error messages to translation files for better i18n support.

src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.html (1)

108-130: Consider simplifying the tooltip logic.

The implementation successfully meets the PR objectives by disabling the submit button and showing appropriate tooltips. However, the tooltip logic could be improved:

1. Consider moving the tooltip condition to the component:

get tooltipText(): string | null {
  if (!this.fileInputTouched && this.nameControl?.invalid) {
    return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError') + 
           ' ' +
           this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError');
  }
  if (!this.fileInputTouched) {
    return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError');
  }
  if (this.nameControl?.invalid) {
    return this.translateService.instant('artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError');
  }
  return null;
}

2. Consider using ng-container instead of span to avoid extra DOM elements.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.

#!/bin/bash
# Check for potential spacing issues in translations
rg -g '*.json' '"artemisApp\.attachmentUnit\.createAttachmentUnit\.(nameRequiredValidationError|fileRequiredValidationError)"\s*:\s*"[^"]*(\s+$|^\s+)'

#!/bin/bash
# Let's first find the translation files and check the actual content of these specific keys
fd -e json | xargs rg -l "artemisApp.attachmentUnit.createAttachmentUnit"

# Then let's examine the actual translation strings for these specific validation messages
fd -e json | xargs rg "nameRequiredValidationError|fileRequiredValidationError"

src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (2)

73-73: Enhance form validation robustness

While the validation logic is comprehensive, consider these improvements:

1. Use strict comparison (!==) instead of loose comparison (!=) for empty string check
2. Optimize condition order for better short-circuiting by checking simpler conditions first

-return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value != '' && this.fileName();
+return this.nameControl?.value !== '' && this.fileName() && !this.isFileTooBig() && this.statusChanges() === 'VALID';

---

Line range hint 87-93: Improve file name handling robustness

Consider these improvements to the file name handling:

1. Use strict comparison (===) for undefined check
2. Extract the file name extraction logic into a separate method
3. Add a comment explaining the auto-naming behavior

-if (this.form && (this.nameControl?.value == undefined || this.nameControl?.value == '')) {
+// Auto-set the name from file name if not explicitly provided by user
+if (this.form && (this.nameControl?.value === undefined || this.nameControl?.value === '')) {
+    const fileNameWithoutExtension = this.extractFileNameWithoutExtension(this.file.name);
     this.form.patchValue({
-        // without extension
-        name: this.file.name.replace(/\.[^/.]+$/, ''),
+        name: fileNameWithoutExtension,
     });
 }

+private extractFileNameWithoutExtension(fileName: string): string {
+    return fileName.replace(/\.[^/.]+$/, '');
+}

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.

#!/bin/bash
# Verify that the component template uses these properties correctly
rg -l 'attachment-unit-form\.component\.html' | xargs cat

#!/bin/bash
# Find and show the template file content
fd -e html attachment-unit-form.component.html -x cat {}

src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts (3)

73-73: Verify the stricter form validation logic

The validation has been enhanced to ensure both name and file are provided before enabling submission. However, the empty string check nameControl?.value != '' should use strict equality for consistency with TypeScript best practices.

-        return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value != '' && this.fileName();
+        return this.statusChanges() === 'VALID' && !this.isFileTooBig() && this.nameControl?.value !== '' && this.fileName();

---

99-113: Consider adding type safety for translation keys

The tooltip implementation looks good with clear conditions and i18n support. However, consider using TypeScript's string literal types to ensure type safety for translation keys.

// Add at the top of the file
type TranslationKey =
  | 'artemisApp.attachmentUnit.createAttachmentUnit.nameAndFileRequiredValidationError'
  | 'artemisApp.attachmentUnit.createAttachmentUnit.fileRequiredValidationError'
  | 'artemisApp.attachmentUnit.createAttachmentUnit.nameRequiredValidationError';

// Then update the getter
get tooltipText(): string | null {
    const translate = (key: TranslationKey) => this.translateService.instant(key);
    // Rest of the implementation remains the same
}

---

Line range hint 82-92: Improve file handling robustness

The file handling logic could be improved in several ways:

1. Use strict equality checks
2. Extract the file extension regex to a constant
3. Add null check for form before accessing nameControl

     if (!input.files?.length) {
         return;
     }
     this.file = input.files[0];
     this.fileName.set(this.file.name);
     // automatically set the name in case it is not yet specified
-    if (this.form && (this.nameControl?.value == undefined || this.nameControl?.value == '')) {
+    const FILE_EXTENSION_REGEX = /\.[^/.]+$/;
+    if (this.form && this.nameControl && (this.nameControl.value === undefined || this.nameControl.value === '')) {
         this.form.patchValue({
             // without extension
-            name: this.file.name.replace(/\.[^/.]+$/, ''),
+            name: this.file.name.replace(FILE_EXTENSION_REGEX, ''),
         });
     }

src/main/webapp/i18n/de/lectureUnit.json (1)

176-176: Consider splitting the combined validation message

The translation follows the informal style guide correctly using "Du" form. However, combining two validation messages ("name required" and "file required") into one key might reduce maintainability and reusability.

Consider splitting into separate keys for better maintainability:

-"nameAndFileRequiredValidationError": "Der Name ist ein Pflichtfeld. Du musst eine Datei zum Uploaden auswählen.",
+"nameRequiredValidationError": "Der Name ist ein Pflichtfeld.",
+"fileRequiredValidationError": "Du musst eine Datei zum Uploaden auswählen."

This approach would:

1. Allow reusing individual messages elsewhere
2. Make it easier to maintain translations
3. Follow the single responsibility principle

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.

#!/bin/bash
# Description: Check if the German translation for the new validation message exists
rg "nameAndFileRequiredValidationError" "src/main/webapp/i18n/de/lectureUnit.json"