119602: Improve types & docs

DSpace · Dec 11, 2024 · 010b2f9 · 010b2f9
1 parent cdec488
commit 010b2f9
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 51 deletions.
diff --git a/src/app/accessibility/accessibility-settings.service.spec.ts b/src/app/accessibility/accessibility-settings.service.spec.ts
@@ -1,6 +1,5 @@
 import {
   AccessibilitySettingsService,
-  AccessibilitySetting,
   AccessibilitySettings,
   ACCESSIBILITY_SETTINGS_METADATA_KEY,
   ACCESSIBILITY_COOKIE
@@ -41,17 +40,6 @@ describe('accessibilitySettingsService', () => {
     );
   });
 
-  describe('getALlAccessibilitySettingsKeys', () => {
-    it('should return an array containing all accessibility setting names', () => {
-      const settingNames: AccessibilitySetting[] = [
-        AccessibilitySetting.NotificationTimeOut,
-        AccessibilitySetting.LiveRegionTimeOut,
-      ];
-
-      expect(service.getAllAccessibilitySettingKeys()).toEqual(settingNames);
-    });
-  });
-
   describe('get', () => {
     it('should return the setting if it is set', () => {
       const settings: AccessibilitySettings = {
@@ -60,7 +48,7 @@ describe('accessibilitySettingsService', () => {
 
       service.getAll = jasmine.createSpy('getAll').and.returnValue(of(settings));
 
-      service.get(AccessibilitySetting.NotificationTimeOut, 'default').subscribe(value =>
+      service.get('notificationTimeOut', 'default').subscribe(value =>
         expect(value).toEqual('1000')
       );
     });
@@ -72,7 +60,7 @@ describe('accessibilitySettingsService', () => {
 
       service.getAll = jasmine.createSpy('getAll').and.returnValue(of(settings));
 
-      service.get(AccessibilitySetting.LiveRegionTimeOut, 'default').subscribe(value =>
+      service.get('liveRegionTimeOut', 'default').subscribe(value =>
         expect(value).toEqual('default')
       );
     });
@@ -82,23 +70,23 @@ describe('accessibilitySettingsService', () => {
     it('should return the setting as number if the value for the setting can be parsed to a number', () => {
       service.get = jasmine.createSpy('get').and.returnValue(of('1000'));
 
-      service.getAsNumber(AccessibilitySetting.NotificationTimeOut).subscribe(value =>
+      service.getAsNumber('notificationTimeOut').subscribe(value =>
         expect(value).toEqual(1000)
       );
     });
 
     it('should return the default value if no value is set for the setting', () => {
       service.get = jasmine.createSpy('get').and.returnValue(of(null));
 
-      service.getAsNumber(AccessibilitySetting.NotificationTimeOut, 123).subscribe(value =>
+      service.getAsNumber('notificationTimeOut', 123).subscribe(value =>
         expect(value).toEqual(123)
       );
     });
 
     it('should return the default value if the value for the setting can not be parsed to a number', () => {
       service.get = jasmine.createSpy('get').and.returnValue(of('text'));
 
-      service.getAsNumber(AccessibilitySetting.NotificationTimeOut, 123).subscribe(value =>
+      service.getAsNumber('notificationTimeOut', 123).subscribe(value =>
         expect(value).toEqual(123)
       );
     });
@@ -176,7 +164,7 @@ describe('accessibilitySettingsService', () => {
     it('should correctly update the chosen setting', () => {
       service.updateSettings = jasmine.createSpy('updateSettings');
 
-      service.set(AccessibilitySetting.LiveRegionTimeOut, '500');
+      service.set('liveRegionTimeOut', '500');
       expect(service.updateSettings).toHaveBeenCalledWith({ liveRegionTimeOut: '500' });
     });
   });
@@ -307,7 +295,7 @@ describe('accessibilitySettingsService', () => {
     });
 
     it('should set the settings in metadata', () => {
-      service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }).subscribe();
+      service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }).subscribe();
       expect(ePerson.setMetadata).toHaveBeenCalled();
     });
 
@@ -318,19 +306,19 @@ describe('accessibilitySettingsService', () => {
     });
 
     it('should create a patch with the metadata changes', () => {
-      service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }).subscribe();
+      service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }).subscribe();
       expect(ePersonService.createPatchFromCache).toHaveBeenCalled();
     });
 
     it('should send the patch request', () => {
-      service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }).subscribe();
+      service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }).subscribe();
       expect(ePersonService.patch).toHaveBeenCalled();
     });
 
     it('should emit true when the update succeeded', fakeAsync(() => {
       ePersonService.patch = jasmine.createSpy().and.returnValue(createSuccessfulRemoteDataObject$({}));
 
-      service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' })
+      service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' })
         .subscribe(value => {
         expect(value).toBeTrue();
       });
@@ -341,7 +329,7 @@ describe('accessibilitySettingsService', () => {
     it('should emit false when the update failed', fakeAsync(() => {
       ePersonService.patch = jasmine.createSpy().and.returnValue(createFailedRemoteDataObject$());
 
-      service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' })
+      service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' })
         .subscribe(value => {
         expect(value).toBeFalse();
       });
@@ -357,7 +345,7 @@ describe('accessibilitySettingsService', () => {
     });
 
     it('should store the settings in a cookie', () => {
-      service.setSettingsInCookie({ [AccessibilitySetting.LiveRegionTimeOut]: '500' });
+      service.setSettingsInCookie({ ['liveRegionTimeOut']: '500' });
       expect(cookieService.set).toHaveBeenCalled();
     });
 

diff --git a/src/app/accessibility/accessibility-settings.service.ts b/src/app/accessibility/accessibility-settings.service.ts
@@ -22,19 +22,21 @@ export const ACCESSIBILITY_COOKIE = 'dsAccessibilityCookie';
 export const ACCESSIBILITY_SETTINGS_METADATA_KEY = 'dspace.accessibility.settings';
 
 /**
- * Enum containing all possible accessibility settings.
- * When adding new settings, make sure to add the new setting to the accessibility-settings component.
+ * Type 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 enum AccessibilitySetting {
-  NotificationTimeOut = 'notificationTimeOut',
-  LiveRegionTimeOut = 'liveRegionTimeOut',
-}
+export type AccessibilitySetting = 'notificationTimeOut' | 'liveRegionTimeOut';
 
 /**
- * Type representing an object that contains accessibility settings values.
+ * Type representing an object that contains accessibility settings values for all accessibility settings.
  */
-export type AccessibilitySettings = { [key in AccessibilitySetting]?: string };
+export type FullAccessibilitySettings = { [key in AccessibilitySetting]: string };
+
+/**
+ * Type representing an object that contains accessibility settings values for some accessibility settings.
+ */
+export type AccessibilitySettings = Partial<FullAccessibilitySettings>;
 
 /**
  * The accessibility settings object format used by the accessibility-settings component form.
@@ -63,10 +65,6 @@ export class AccessibilitySettingsService {
   ) {
   }
 
-  getAllAccessibilitySettingKeys(): AccessibilitySetting[] {
-    return Object.entries(AccessibilitySetting).map(([_, val]) => val);
-  }
-
   /**
    * Get the stored value for the provided {@link AccessibilitySetting}. If the value does not exist or if it is empty,
    * the provided defaultValue is emitted instead.
@@ -238,9 +236,9 @@ export class AccessibilitySettingsService {
    */
   getPlaceholder(setting: AccessibilitySetting): string {
     switch (setting) {
-      case AccessibilitySetting.NotificationTimeOut:
+      case 'notificationTimeOut':
         return millisecondsToSeconds(environment.notifications.timeOut.toString());
-      case AccessibilitySetting.LiveRegionTimeOut:
+      case 'liveRegionTimeOut':
         return millisecondsToSeconds(environment.liveRegion.messageTimeOutDurationMs.toString());
       default:
         return '';
@@ -250,11 +248,11 @@ export class AccessibilitySettingsService {
   /**
    * Convert values in the provided accessibility settings object to values ready to be stored.
    */
-  convertFormValuesToStoredValues(settings: AccessibilitySettingsFormValues): AccessibilitySettings {
+  convertFormValuesToStoredValues(settings: AccessibilitySettingsFormValues): FullAccessibilitySettings {
     return {
-      'notificationTimeOut': settings.disableNotificationTimeOut ? '0'
+      notificationTimeOut: settings.disableNotificationTimeOut ? '0'
         : secondsToMilliseconds(settings.notificationTimeOut),
-      'liveRegionTimeOut': secondsToMilliseconds(settings.liveRegionTimeOut),
+      liveRegionTimeOut: secondsToMilliseconds(settings.liveRegionTimeOut),
     };
   }
 
@@ -271,6 +269,10 @@ export class AccessibilitySettingsService {
 
 }
 
+/**
+ * Converts a string representing seconds to a string representing milliseconds
+ * Returns null if the input could not be parsed to a float
+ */
 function secondsToMilliseconds(secondsStr: string): string {
   const seconds = parseFloat(secondsStr);
   if (isNaN(seconds)) {
@@ -280,6 +282,10 @@ function secondsToMilliseconds(secondsStr: string): string {
   }
 }
 
+/**
+ * Converts a string representing milliseconds to a string representing seconds
+ * Returns null if the input could not be parsed to a float
+ */
 function millisecondsToSeconds(millisecondsStr: string): string {
   const milliseconds = parseFloat(millisecondsStr);
   if (isNaN(milliseconds)) {

diff --git a/src/app/info/accessibility-settings/accessibility-settings.component.html b/src/app/info/accessibility-settings/accessibility-settings.component.html
@@ -30,7 +30,7 @@ <h2>{{ 'info.accessibility-settings.title' | translate }}</h2>
 
       <div class="col-sm-4">
         <input [type]="'number'" [id]="'notificationTimeOutInput'" class="form-control"
-               [placeholder]="getPlaceholder(AccessibilitySetting.NotificationTimeOut)"
+               [placeholder]="getPlaceholder('notificationTimeOut')"
                [readOnly]="formValues.disableNotificationTimeOut"
                [(ngModel)]="formValues.notificationTimeOut" [ngModelOptions]="{ standalone: true }"
                [attr.aria-describedby]="'notificationTimeOutHint'">
@@ -56,7 +56,7 @@ <h2>{{ 'info.accessibility-settings.title' | translate }}</h2>
 
       <div class="col-sm-4">
         <input [type]="'number'" [id]="'liveRegionTimeOutInput'" class="form-control"
-               [placeholder]="getPlaceholder(AccessibilitySetting.LiveRegionTimeOut)"
+               [placeholder]="getPlaceholder('liveRegionTimeOut')"
                [(ngModel)]="formValues.liveRegionTimeOut" [ngModelOptions]="{ standalone: true }"
                [attr.aria-describedby]="'liveRegionTimeOutHint'">
       </div>

diff --git a/src/app/info/accessibility-settings/accessibility-settings.component.ts b/src/app/info/accessibility-settings/accessibility-settings.component.ts
@@ -18,9 +18,6 @@ import { TranslateService } from '@ngx-translate/core';
 })
 export class AccessibilitySettingsComponent implements OnInit {
 
-  // Re-export for use in template
-  protected readonly AccessibilitySetting = AccessibilitySetting;
-
   protected formValues: AccessibilitySettingsFormValues;
 
   constructor(

diff --git a/src/app/shared/live-region/live-region.service.ts b/src/app/shared/live-region/live-region.service.ts
@@ -2,7 +2,7 @@ import { Injectable } from '@angular/core';
 import { BehaviorSubject, map, Observable, switchMap, take, timer } from 'rxjs';
 import { environment } from '../../../environments/environment';
 import { UUIDService } from '../../core/shared/uuid.service';
-import { AccessibilitySettingsService, AccessibilitySetting } from '../../accessibility/accessibility-settings.service';
+import { AccessibilitySettingsService } from '../../accessibility/accessibility-settings.service';
 
 export const MIN_MESSAGE_DURATION = 200;
 
@@ -130,7 +130,7 @@ export class LiveRegionService {
    */
   getConfiguredMessageTimeOutMs(): Observable<number> {
     return this.accessibilitySettingsService.getAsNumber(
-      AccessibilitySetting.LiveRegionTimeOut,
+      'liveRegionTimeOut',
       this.getMessageTimeOutMs(),
     ).pipe(map(timeOut => Math.max(timeOut, MIN_MESSAGE_DURATION)));
   }

diff --git a/src/app/shared/notifications/notifications-board/notifications-board.component.ts b/src/app/shared/notifications/notifications-board/notifications-board.component.ts
@@ -18,8 +18,7 @@ import { INotification } from '../models/notification.model';
 import { NotificationsState } from '../notifications.reducers';
 import { INotificationBoardOptions } from '../../../../config/notifications-config.interfaces';
 import {
-  AccessibilitySettingsService,
-  AccessibilitySetting
+  AccessibilitySettingsService
 } from '../../../accessibility/accessibility-settings.service';
 import cloneDeep from 'lodash/cloneDeep';
 import differenceWith from 'lodash/differenceWith';
@@ -98,7 +97,7 @@ export class NotificationsBoardComponent implements OnInit, OnDestroy {
 
       // It would be a bit better to handle the retrieval of configured settings in the NotificationsService.
       // Due to circular dependencies this is difficult to implement.
-      this.accessibilitySettingsService.getAsNumber(AccessibilitySetting.NotificationTimeOut, item.options.timeOut)
+      this.accessibilitySettingsService.getAsNumber('notificationTimeOut', item.options.timeOut)
         .pipe(take(1)).subscribe(timeOut => {
           if (timeOut < 0) {
             timeOut = 0;