From dcd32c48c32d56e9c9827642734d353b6bf52d37 Mon Sep 17 00:00:00 2001
From: Andreas Awouters <andreas.awouters@atmire.com>
Date: Fri, 21 Feb 2025 12:53:44 +0100
Subject: [PATCH] 119602: Rework settings validation

---
 .../accessibility-settings.service.stub.ts    |  2 +-
 .../accessibility-settings.service.ts         | 25 ++++++++++++-------
 .../accessibility-settings.component.html     |  8 +++---
 .../accessibility-settings.component.ts       |  4 +--
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/src/app/accessibility/accessibility-settings.service.stub.ts b/src/app/accessibility/accessibility-settings.service.stub.ts
index 80b7de1fa3..1b8c534010 100644
--- a/src/app/accessibility/accessibility-settings.service.stub.ts
+++ b/src/app/accessibility/accessibility-settings.service.stub.ts
@@ -40,5 +40,5 @@ export class AccessibilitySettingsServiceStub {
 
   isValid = jasmine.createSpy('isValid').and.returnValue(true);
 
-  allValid = jasmine.createSpy('allValid').and.returnValue(true);
+  formValuesValid = jasmine.createSpy('allValid').and.returnValue(true);
 }
diff --git a/src/app/accessibility/accessibility-settings.service.ts b/src/app/accessibility/accessibility-settings.service.ts
index b97ecbe3ed..6db4c58977 100644
--- a/src/app/accessibility/accessibility-settings.service.ts
+++ b/src/app/accessibility/accessibility-settings.service.ts
@@ -24,11 +24,16 @@ export const ACCESSIBILITY_COOKIE = 'dsAccessibilityCookie';
 export const ACCESSIBILITY_SETTINGS_METADATA_KEY = 'dspace.accessibility.settings';
 
 /**
- * Type containing all possible accessibility settings.
+ * Array containing all possible accessibility settings.
  * When adding new settings, make sure to add the new setting to the accessibility-settings component form.
  * The converter methods to convert from stored format to form format (and vice-versa) need to be updated as well.
  */
-export type AccessibilitySetting = 'notificationTimeOut' | 'liveRegionTimeOut';
+export const accessibilitySettingKeys = ['notificationTimeOut', 'liveRegionTimeOut'] as const;
+
+/**
+ * Type representing the possible accessibility settings
+ */
+export type AccessibilitySetting = typeof accessibilitySettingKeys[number];
 
 /**
  * Type representing an object that contains accessibility settings values for all accessibility settings.
@@ -294,24 +299,26 @@ export class AccessibilitySettingsService {
   }
 
   /**
-   * Returns true if the provided value is a valid value for the provided AccessibilitySetting.
+   * Returns true if the provided AccessibilitySetting is valid in regard to the provided formValues.
    */
-  isValid(setting: AccessibilitySetting | string, value: string): boolean {
+  isValid(setting: AccessibilitySetting, formValues: AccessibilitySettingsFormValues): boolean {
     switch (setting) {
       case 'notificationTimeOut':
-        return hasNoValue(value) || parseFloat(value) > 0;
+        return formValues.notificationTimeOutEnabled ?
+          hasNoValue(formValues.notificationTimeOut) || parseFloat(formValues.notificationTimeOut) > 0 :
+          true;
       case 'liveRegionTimeOut':
-        return hasNoValue(value) || parseFloat(value) > 0;
+        return hasNoValue(formValues.liveRegionTimeOut) || parseFloat(formValues.liveRegionTimeOut) > 0;
       default:
         throw new Error(`Unhandled accessibility setting during validity check: ${setting}`);
     }
   }
 
   /**
-   * Returns true if all settings in the provided AccessibilitySettings object are valid
+   * Returns true if all settings in the provided AccessibilitySettingsFormValues object are valid
    */
-  allValid(settings: AccessibilitySettings) {
-    return Object.entries(settings).every(([setting, value], _) => this.isValid(setting, value));
+  formValuesValid(formValues: AccessibilitySettingsFormValues) {
+    return accessibilitySettingKeys.every(setting => this.isValid(setting, formValues));
   }
 }
 
diff --git a/src/app/info/accessibility-settings/accessibility-settings.component.html b/src/app/info/accessibility-settings/accessibility-settings.component.html
index 6a0094fc97..9b2eba493b 100644
--- a/src/app/info/accessibility-settings/accessibility-settings.component.html
+++ b/src/app/info/accessibility-settings/accessibility-settings.component.html
@@ -30,12 +30,12 @@
 
       <div class="col-sm-4">
         <input [type]="'number'" [id]="'notificationTimeOutInput'" class="form-control"
-               [ngClass]="{'is-invalid': !settingsService.isValid('notificationTimeOut', formValues.notificationTimeOut)}"
+               [ngClass]="{'is-invalid': !settingsService.isValid('notificationTimeOut', formValues)}"
                [min]="1"
                [readOnly]="!formValues.notificationTimeOutEnabled"
                [(ngModel)]="formValues.notificationTimeOut" [ngModelOptions]="{ standalone: true }"
                [attr.aria-describedby]="'notificationTimeOutHint'">
-        <div class="invalid-feedback" [ngClass]="{ 'd-block': !settingsService.isValid('notificationTimeOut', formValues.notificationTimeOut) }">
+        <div class="invalid-feedback" [ngClass]="{ 'd-block': !settingsService.isValid('notificationTimeOut', formValues) }">
           {{ 'info.accessibility-settings.notificationTimeOut.invalid' | translate }}
         </div>
       </div>
@@ -57,11 +57,11 @@
 
       <div class="col-sm-4">
         <input [type]="'number'" [id]="'liveRegionTimeOutInput'" class="form-control"
-               [ngClass]="{'is-invalid': !settingsService.isValid('liveRegionTimeOut', formValues.liveRegionTimeOut)}"
+               [ngClass]="{'is-invalid': !settingsService.isValid('liveRegionTimeOut', formValues)}"
                [min]="1"
                [(ngModel)]="formValues.liveRegionTimeOut" [ngModelOptions]="{ standalone: true }"
                [attr.aria-describedby]="'liveRegionTimeOutHint'">
-        <div class="invalid-feedback" [ngClass]="{ 'd-block': !settingsService.isValid('liveRegionTimeOut', formValues.liveRegionTimeOut) }">
+        <div class="invalid-feedback" [ngClass]="{ 'd-block': !settingsService.isValid('liveRegionTimeOut', formValues) }">
           {{ 'info.accessibility-settings.liveRegionTimeOut.invalid' | translate }}
         </div>
       </div>
diff --git a/src/app/info/accessibility-settings/accessibility-settings.component.ts b/src/app/info/accessibility-settings/accessibility-settings.component.ts
index fb1f614141..99453e2e29 100644
--- a/src/app/info/accessibility-settings/accessibility-settings.component.ts
+++ b/src/app/info/accessibility-settings/accessibility-settings.component.ts
@@ -61,9 +61,9 @@ export class AccessibilitySettingsComponent implements OnInit, OnDestroy {
    */
   saveSettings() {
     const formValues = this.formValues;
-    const convertedValues = this.settingsService.convertFormValuesToStoredValues(formValues);
 
-    if (this.settingsService.allValid(convertedValues)) {
+    if (this.settingsService.formValuesValid(formValues)) {
+      const convertedValues = this.settingsService.convertFormValuesToStoredValues(formValues);
       this.settingsService.setSettings(convertedValues).pipe(take(1)).subscribe(location => {
         if (location !== 'failed') {
           this.notificationsService.success(null, this.translateService.instant('info.accessibility-settings.save-notification.' + location));