71429: Feedback 2020-06-23

src/app/+admin/admin-access-control/epeople-registry/eperson-form/eperson-form.component.ts

@@ -24,7 +24,7 @@ import { NotificationsService } from '../../../../shared/notifications/notificat
 import { PaginationComponentOptions } from '../../../../shared/pagination/pagination-component-options.model';
 import { AuthService } from '../../../../core/auth/auth.service';
 import { AuthorizationDataService } from '../../../../core/data/feature-authorization/authorization-data.service';
-import { FeatureType } from '../../../../core/data/feature-authorization/feature-type';
+import { FeatureID } from '../../../../core/data/feature-authorization/feature-id';
 
 @Component({
   selector: 'ds-eperson-form',
@@ -245,7 +245,7 @@ export class EPersonFormComponent implements OnInit, OnDestroy {
         });
       }));
       this.canImpersonate$ = this.epersonService.getActiveEPerson().pipe(
-        switchMap((eperson) => this.authorizationService.isAuthenticated(FeatureType.LoginOnBehalfOf, hasValue(eperson) ? eperson.self : undefined))
+        switchMap((eperson) => this.authorizationService.isAuthenticated(FeatureID.LoginOnBehalfOf, hasValue(eperson) ? eperson.self : undefined))
       );
     });
   }

src/app/+admin/admin-sidebar/admin-sidebar.component.ts

@@ -19,7 +19,7 @@ import { MenuComponent } from '../../shared/menu/menu.component';
 import { MenuService } from '../../shared/menu/menu.service';
 import { CSSVariableService } from '../../shared/sass-helper/sass-helper.service';
 import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
-import { FeatureType } from '../../core/data/feature-authorization/feature-type';
+import { FeatureID } from '../../core/data/feature-authorization/feature-id';
 
 /**
  * Component representing the admin sidebar
@@ -73,32 +73,31 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
    * Set and calculate all initial values of the instance variables
    */
   ngOnInit(): void {
-    this.authorizationService.isAuthenticated(FeatureType.AdministratorOf).subscribe((authorized) => {
+    this.createMenu();
-      this.createMenu(authorized);
+    this.createSiteAdministratorMenuSections();
-      super.ngOnInit();
+    super.ngOnInit();
-      this.sidebarWidth = this.variableService.getVariable('sidebarItemsWidth');
+    this.sidebarWidth = this.variableService.getVariable('sidebarItemsWidth');
-      this.authService.isAuthenticated()
+    this.authService.isAuthenticated()
-        .subscribe((loggedIn: boolean) => {
+      .subscribe((loggedIn: boolean) => {
-          if (loggedIn) {
+        if (loggedIn) {
-            this.menuService.showMenu(this.menuID);
+          this.menuService.showMenu(this.menuID);
-          }
+        }
-        });
+      });
-      this.menuCollapsed.pipe(first())
+    this.menuCollapsed.pipe(first())
-        .subscribe((collapsed: boolean) => {
+      .subscribe((collapsed: boolean) => {
-          this.sidebarOpen = !collapsed;
+        this.sidebarOpen = !collapsed;
-          this.sidebarClosed = collapsed;
+        this.sidebarClosed = collapsed;
-        });
+      });
-      this.sidebarExpanded = combineLatestObservable(this.menuCollapsed, this.menuPreviewCollapsed)
+    this.sidebarExpanded = combineLatestObservable(this.menuCollapsed, this.menuPreviewCollapsed)
-        .pipe(
+      .pipe(
-          map(([collapsed, previewCollapsed]) => (!collapsed || !previewCollapsed))
+        map(([collapsed, previewCollapsed]) => (!collapsed || !previewCollapsed))
-        );
+      );
-    });
   }
 
   /**
    * Initialize all menu sections and items for this menu
    */
-  createMenu(isAdministratorOfSite: boolean) {
+  createMenu() {
     const menuList = [
       /* News */
       {
@@ -310,99 +309,6 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
         } as LinkMenuItemModel,
       },
 
-      /* Access Control */
-      {
-        id: 'access_control',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.TEXT,
-          text: 'menu.section.access_control'
-        } as TextMenuItemModel,
-        icon: 'key',
-        index: 4
-      },
-      {
-        id: 'access_control_people',
-        parentID: 'access_control',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.access_control_people',
-          link: '/admin/access-control/epeople'
-        } as LinkMenuItemModel,
-      },
-      {
-        id: 'access_control_groups',
-        parentID: 'access_control',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.access_control_groups',
-          link: '/admin/access-control/groups'
-        } as LinkMenuItemModel,
-      },
-      {
-        id: 'access_control_authorizations',
-        parentID: 'access_control',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.access_control_authorizations',
-          link: ''
-        } as LinkMenuItemModel,
-      },
-      /*  Admin Search */
-      {
-        id: 'admin_search',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.admin_search',
-          link: '/admin/search'
-        } as LinkMenuItemModel,
-        icon: 'search',
-        index: 5
-      },
-      /*  Registries */
-      {
-        id: 'registries',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.TEXT,
-          text: 'menu.section.registries'
-        } as TextMenuItemModel,
-        icon: 'list',
-        index: 6
-      },
-      {
-        id: 'registries_metadata',
-        parentID: 'registries',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.registries_metadata',
-          link: 'admin/registries/metadata'
-        } as LinkMenuItemModel,
-      },
-      {
-        id: 'registries_format',
-        parentID: 'registries',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.registries_format',
-          link: 'admin/registries/bitstream-formats'
-        } as LinkMenuItemModel,
-      },
-
       /* Curation tasks */
       {
         id: 'curation_tasks',
@@ -444,24 +350,130 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
         icon: 'cogs',
         index: 9
       },
-      /* Workflow */
-      {
-        id: 'workflow',
-        active: false,
-        visible: isAdministratorOfSite,
-        model: {
-          type: MenuItemType.LINK,
-          text: 'menu.section.workflow',
-          link: '/admin/workflow'
-        } as LinkMenuItemModel,
-        icon: 'user-check',
-        index: 10
-      },
     ];
     menuList.forEach((menuSection) => this.menuService.addSection(this.menuID, Object.assign(menuSection, {
       shouldPersistOnRouteChange: true
     })));
+  }
 
+  /**
+   * Create menu sections dependent on whether or not the current user is a site administrator
+   */
+  createSiteAdministratorMenuSections() {
+    this.authorizationService.isAuthenticated(FeatureID.AdministratorOf).subscribe((authorized) => {
+      const menuList = [
+        /* Access Control */
+        {
+          id: 'access_control',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.TEXT,
+            text: 'menu.section.access_control'
+          } as TextMenuItemModel,
+          icon: 'key',
+          index: 4
+        },
+        {
+          id: 'access_control_people',
+          parentID: 'access_control',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.access_control_people',
+            link: '/admin/access-control/epeople'
+          } as LinkMenuItemModel,
+        },
+        {
+          id: 'access_control_groups',
+          parentID: 'access_control',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.access_control_groups',
+            link: '/admin/access-control/groups'
+          } as LinkMenuItemModel,
+        },
+        {
+          id: 'access_control_authorizations',
+          parentID: 'access_control',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.access_control_authorizations',
+            link: ''
+          } as LinkMenuItemModel,
+        },
+        /*  Admin Search */
+        {
+          id: 'admin_search',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.admin_search',
+            link: '/admin/search'
+          } as LinkMenuItemModel,
+          icon: 'search',
+          index: 5
+        },
+        /*  Registries */
+        {
+          id: 'registries',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.TEXT,
+            text: 'menu.section.registries'
+          } as TextMenuItemModel,
+          icon: 'list',
+          index: 6
+        },
+        {
+          id: 'registries_metadata',
+          parentID: 'registries',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.registries_metadata',
+            link: 'admin/registries/metadata'
+          } as LinkMenuItemModel,
+        },
+        {
+          id: 'registries_format',
+          parentID: 'registries',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.registries_format',
+            link: 'admin/registries/bitstream-formats'
+          } as LinkMenuItemModel,
+        },
+
+        /* Workflow */
+        {
+          id: 'workflow',
+          active: false,
+          visible: authorized,
+          model: {
+            type: MenuItemType.LINK,
+            text: 'menu.section.workflow',
+            link: '/admin/workflow'
+          } as LinkMenuItemModel,
+          icon: 'user-check',
+          index: 10
+        },
+      ];
+
+      menuList.forEach((menuSection) => this.menuService.addSection(this.menuID, Object.assign(menuSection, {
+        shouldPersistOnRouteChange: true
+      })));
+    });
   }
 
   /**

src/app/core/data/feature-authorization/authorization-data.service.spec.ts

@@ -5,7 +5,7 @@ import { Site } from '../../shared/site.model';
 import { EPerson } from '../../eperson/models/eperson.model';
 import { of as observableOf } from 'rxjs';
 import { FindListOptions } from '../request.models';
-import { FeatureType } from './feature-type';
+import { FeatureID } from './feature-id';
 import { hasValue } from '../../../shared/empty.util';
 import { RequestParam } from '../../cache/models/request-param.model';
 import { Authorization } from '../../shared/authorization.model';
@@ -51,7 +51,7 @@ describe('AuthorizationDataService', () => {
     const objectUrl = 'fake-object-url';
     const ePersonUuid = 'fake-eperson-uuid';
 
-    function createExpected(objectUrl: string, ePersonUuid?: string, featureId?: FeatureType): FindListOptions {
+    function createExpected(objectUrl: string, ePersonUuid?: string, featureId?: FeatureID): FindListOptions {
       const searchParams = [new RequestParam('uri', objectUrl)];
       if (hasValue(featureId)) {
         searchParams.push(new RequestParam('feature', featureId));
@@ -74,31 +74,31 @@ describe('AuthorizationDataService', () => {
 
     describe('when no arguments except for a feature are provided and a user is authenticated', () => {
       beforeEach(() => {
-        service.searchByObject(FeatureType.LoginOnBehalfOf).subscribe();
+        service.searchByObject(FeatureID.LoginOnBehalfOf).subscribe();
       });
 
       it('should call searchBy with the site\'s url, authenticated user\'s uuid and the feature', () => {
-        expect(service.searchBy).toHaveBeenCalledWith('object', createExpected(site.self, ePerson.uuid, FeatureType.LoginOnBehalfOf));
+        expect(service.searchBy).toHaveBeenCalledWith('object', createExpected(site.self, ePerson.uuid, FeatureID.LoginOnBehalfOf));
       });
     });
 
     describe('when a feature and object url are provided, but no user uuid and a user is authenticated', () => {
       beforeEach(() => {
-        service.searchByObject(FeatureType.LoginOnBehalfOf, objectUrl).subscribe();
+        service.searchByObject(FeatureID.LoginOnBehalfOf, objectUrl).subscribe();
       });
 
       it('should call searchBy with the object\'s url, authenticated user\'s uuid and the feature', () => {
-        expect(service.searchBy).toHaveBeenCalledWith('object', createExpected(objectUrl, ePerson.uuid, FeatureType.LoginOnBehalfOf));
+        expect(service.searchBy).toHaveBeenCalledWith('object', createExpected(objectUrl, ePerson.uuid, FeatureID.LoginOnBehalfOf));
       });
     });
 
     describe('when all arguments are provided', () => {
       beforeEach(() => {
-        service.searchByObject(FeatureType.LoginOnBehalfOf, objectUrl, ePersonUuid).subscribe();
+        service.searchByObject(FeatureID.LoginOnBehalfOf, objectUrl, ePersonUuid).subscribe();
       });
 
       it('should call searchBy with the object\'s url, user\'s uuid and the feature', () => {
-        expect(service.searchBy).toHaveBeenCalledWith('object', createExpected(objectUrl, ePersonUuid, FeatureType.LoginOnBehalfOf));
+        expect(service.searchBy).toHaveBeenCalledWith('object', createExpected(objectUrl, ePersonUuid, FeatureID.LoginOnBehalfOf));
       });
     });
 

src/app/core/data/feature-authorization/authorization-data.service.ts

@@ -25,7 +25,7 @@ import { hasValue, isNotEmpty } from '../../../shared/empty.util';
 import { RequestParam } from '../../cache/models/request-param.model';
 import { AuthorizationSearchParams } from './authorization-search-params';
 import { addAuthenticatedUserUuidIfEmpty, addSiteObjectUrlIfEmpty } from './authorization-utils';
-import { FeatureType } from './feature-type';
+import { FeatureID } from './feature-id';
 
 /**
  * A service to retrieve {@link Authorization}s from the REST API
@@ -59,7 +59,7 @@ export class AuthorizationDataService extends DataService<Authorization> {
    *                      If not provided, the UUID of the currently authenticated {@link EPerson} will be used.
    * @param featureId     ID of the {@link Feature} to check {@link Authorization} for
    */
-  isAuthenticated(featureId?: FeatureType, objectUrl?: string, ePersonUuid?: string): Observable<boolean> {
+  isAuthenticated(featureId?: FeatureID, objectUrl?: string, ePersonUuid?: string): Observable<boolean> {
     return this.searchByObject(featureId, objectUrl, ePersonUuid).pipe(
       map((authorizationRD) => (authorizationRD.statusCode !== 401 && hasValue(authorizationRD.payload) && isNotEmpty(authorizationRD.payload.page)))
     );
@@ -76,7 +76,7 @@ export class AuthorizationDataService extends DataService<Authorization> {
    * @param options       {@link FindListOptions} to provide pagination and/or additional arguments
    * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved
    */
-  searchByObject(featureId?: FeatureType, objectUrl?: string, ePersonUuid?: string, options: FindListOptions = {}, ...linksToFollow: Array<FollowLinkConfig<Authorization>>): Observable<RemoteData<PaginatedList<Authorization>>> {
+  searchByObject(featureId?: FeatureID, objectUrl?: string, ePersonUuid?: string, options: FindListOptions = {}, ...linksToFollow: Array<FollowLinkConfig<Authorization>>): Observable<RemoteData<PaginatedList<Authorization>>> {
     return observableOf(new AuthorizationSearchParams(objectUrl, ePersonUuid, featureId)).pipe(
       addSiteObjectUrlIfEmpty(this.siteService),
       addAuthenticatedUserUuidIfEmpty(this.authService),
@@ -101,14 +101,12 @@ export class AuthorizationDataService extends DataService<Authorization> {
     return hrefObs.pipe(
       find((href: string) => hasValue(href)),
       tap((href: string) => {
-          this.requestService.removeByHrefSubstring(href);
           const request = new FindListRequest(this.requestService.generateRequestId(), href, options);
 
           this.requestService.configure(request);
         }
       ),
       switchMap((href) => this.requestService.getByHref(href)),
-      skipWhile((requestEntry) => hasValue(requestEntry) && requestEntry.completed),
       switchMap((href) =>
         this.rdbService.buildList<Authorization>(hrefObs, ...linksToFollow) as Observable<RemoteData<PaginatedList<Authorization>>>
       )
@@ -122,7 +120,7 @@ export class AuthorizationDataService extends DataService<Authorization> {
    * @param ePersonUuid Optional parameter value to add to {@link RequestParam} "eperson"
    * @param featureId   Optional parameter value to add to {@link RequestParam} "feature"
    */
-  private createSearchOptions(objectUrl: string, options: FindListOptions = {}, ePersonUuid?: string, featureId?: FeatureType): FindListOptions {
+  private createSearchOptions(objectUrl: string, options: FindListOptions = {}, ePersonUuid?: string, featureId?: FeatureID): FindListOptions {
     let params = [];
     if (isNotEmpty(options.searchParams)) {
       params = [...options.searchParams];

src/app/core/data/feature-authorization/authorization-search-params.ts

@@ -1,11 +1,14 @@
-import { FeatureType } from './feature-type';
+import { FeatureID } from './feature-id';
 
+/**
+ * Search parameters for retrieving authorizations from the REST API
+ */
 export class AuthorizationSearchParams {
   objectUrl: string;
   ePersonUuid: string;
-  featureId: FeatureType;
+  featureId: FeatureID;
 
-  constructor(objectUrl?: string, ePersonUuid?: string, featureId?: FeatureType) {
+  constructor(objectUrl?: string, ePersonUuid?: string, featureId?: FeatureID) {
     this.objectUrl = objectUrl;
     this.ePersonUuid = ePersonUuid;
     this.featureId = featureId;

src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.spec.ts

@@ -1,6 +1,6 @@
 import { FeatureAuthorizationGuard } from './feature-authorization.guard';
 import { AuthorizationDataService } from '../authorization-data.service';
-import { FeatureType } from '../feature-type';
+import { FeatureID } from '../feature-id';
 import { of as observableOf } from 'rxjs';
 
 /**
@@ -9,14 +9,14 @@ import { of as observableOf } from 'rxjs';
  */
 class FeatureAuthorizationGuardImpl extends FeatureAuthorizationGuard {
   constructor(protected authorizationService: AuthorizationDataService,
-              protected featureType: FeatureType,
+              protected featureId: FeatureID,
               protected objectUrl: string,
               protected ePersonUuid: string) {
     super(authorizationService);
   }
 
-  getFeatureType(): FeatureType {
+  getFeatureID(): FeatureID {
-    return this.featureType;
+    return this.featureId;
   }
 
   getObjectUrl(): string {
@@ -32,19 +32,19 @@ describe('FeatureAuthorizationGuard', () => {
   let guard: FeatureAuthorizationGuard;
   let authorizationService: AuthorizationDataService;
 
-  let featureType: FeatureType;
+  let featureId: FeatureID;
   let objectUrl: string;
   let ePersonUuid: string;
 
   function init() {
-    featureType = FeatureType.LoginOnBehalfOf;
+    featureId = FeatureID.LoginOnBehalfOf;
     objectUrl = 'fake-object-url';
     ePersonUuid = 'fake-eperson-uuid';
 
     authorizationService = jasmine.createSpyObj('authorizationService', {
       isAuthenticated: observableOf(true)
     });
-    guard = new FeatureAuthorizationGuardImpl(authorizationService, featureType, objectUrl, ePersonUuid);
+    guard = new FeatureAuthorizationGuardImpl(authorizationService, featureId, objectUrl, ePersonUuid);
   }
 
   beforeEach(() => {
@@ -54,14 +54,14 @@ describe('FeatureAuthorizationGuard', () => {
   describe('canActivate', () => {
     it('should call authorizationService.isAuthenticated with the appropriate arguments', () => {
       guard.canActivate(undefined, undefined).subscribe();
-      expect(authorizationService.isAuthenticated).toHaveBeenCalledWith(featureType, objectUrl, ePersonUuid);
+      expect(authorizationService.isAuthenticated).toHaveBeenCalledWith(featureId, objectUrl, ePersonUuid);
     });
   });
 
   describe('canLoad', () => {
     it('should call authorizationService.isAuthenticated with the appropriate arguments', () => {
       guard.canLoad(undefined, undefined).subscribe();
-      expect(authorizationService.isAuthenticated).toHaveBeenCalledWith(featureType, objectUrl, ePersonUuid);
+      expect(authorizationService.isAuthenticated).toHaveBeenCalledWith(featureId, objectUrl, ePersonUuid);
     });
   });
 });

src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.ts

@@ -1,6 +1,6 @@
 import { ActivatedRouteSnapshot, CanActivate, CanLoad, Route, RouterStateSnapshot, UrlSegment } from '@angular/router';
 import { AuthorizationDataService } from '../authorization-data.service';
-import { FeatureType } from '../feature-type';
+import { FeatureID } from '../feature-id';
 import { Observable } from 'rxjs/internal/Observable';
 
 /**
@@ -16,23 +16,21 @@ export abstract class FeatureAuthorizationGuard implements CanActivate, CanLoad
    * True when user has authorization rights for the feature and object provided
    */
   canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean> {
-    return this.authorizationService.isAuthenticated(this.getFeatureType(), this.getObjectUrl(), this.getEPersonUuid());
+    return this.authorizationService.isAuthenticated(this.getFeatureID(), this.getObjectUrl(), this.getEPersonUuid());
   }
 
   /**
    * True when user has authorization rights for the feature and object provided
    */
   canLoad(route: Route, segments: UrlSegment[]): Observable<boolean> {
-    return this.authorizationService.isAuthenticated(this.getFeatureType(), this.getObjectUrl(), this.getEPersonUuid());
+    return this.authorizationService.isAuthenticated(this.getFeatureID(), this.getObjectUrl(), this.getEPersonUuid());
   }
 
   /**
    * The type of feature to check authorization for
    * Override this method to define a feature
    */
-  getFeatureType(): FeatureType {
+  abstract getFeatureID(): FeatureID;
-    return undefined;
-  }
 
   /**
    * The URL of the object to check if the user has authorized rights for

src/app/core/data/feature-authorization/feature-authorization-guard/site-administrator.guard.ts

@@ -1,6 +1,6 @@
 import { Injectable } from '@angular/core';
 import { FeatureAuthorizationGuard } from './feature-authorization.guard';
-import { FeatureType } from '../feature-type';
+import { FeatureID } from '../feature-id';
 import { AuthorizationDataService } from '../authorization-data.service';
 
 /**
@@ -18,7 +18,7 @@ export class SiteAdministratorGuard extends FeatureAuthorizationGuard {
   /**
    * Check administrator authorization rights
    */
-  getFeatureType(): FeatureType {
+  getFeatureID(): FeatureID {
-    return FeatureType.AdministratorOf;
+    return FeatureID.AdministratorOf;
   }
 }

src/app/core/data/feature-authorization/feature-id.ts

@@ -1,7 +1,7 @@
 /**
- * Enum object for all possible {@link Feature} types
+ * Enum object for all possible {@link Feature} IDs
  */
-export enum FeatureType {
+export enum FeatureID {
   LoginOnBehalfOf = 'loginOnBehalfOf',
   AdministratorOf = 'administratorOf'
 }

src/app/core/shared/feature.model.ts

@@ -1,7 +1,8 @@
 import { typedObject } from '../cache/builders/build-decorators';
-import { autoserialize, inheritSerialization } from 'cerialize';
+import { autoserialize, deserialize, inheritSerialization } from 'cerialize';
 import { FEATURE } from './feature.resource-type';
 import { DSpaceObject } from './dspace-object.model';
+import { HALLink } from './hal-link.model';
 
 /**
  * Class representing a DSpace Feature
@@ -28,4 +29,9 @@ export class Feature extends DSpaceObject {
    */
   @autoserialize
   resourcetypes: string[];
+
+  @deserialize
+  _links: {
+    self: HALLink;
+  };
 }