Merge pull request #1503 from atmire/fix-delete-item-with-empty-entity-type

Fix deleting an item with an empty or invalid entity type

Author: tdonohue

src/app/item-page/edit-item-page/item-delete/item-delete.component.spec.ts

@@ -3,7 +3,7 @@ import { ItemType } from '../../../core/shared/item-relationships/item-type.mode
 import { Relationship } from '../../../core/shared/item-relationships/relationship.model';
 import { Item } from '../../../core/shared/item.model';
 import { RouterStub } from '../../../shared/testing/router.stub';
-import { of as observableOf } from 'rxjs';
+import { of as observableOf, EMPTY } from 'rxjs';
 import { NotificationsServiceStub } from '../../../shared/testing/notifications-service.stub';
 import { CommonModule } from '@angular/common';
 import { FormsModule } from '@angular/forms';
@@ -24,6 +24,8 @@ import { RelationshipType } from '../../../core/shared/item-relationships/relati
 import { EntityTypeService } from '../../../core/data/entity-type.service';
 import { getItemEditRoute } from '../../item-page-routing-paths';
 import { createPaginatedList } from '../../../shared/testing/utils.test';
+import { RelationshipTypeService } from '../../../core/data/relationship-type.service';
+import { LinkService } from '../../../core/cache/builders/link.service';
 
 let comp: ItemDeleteComponent;
 let fixture: ComponentFixture<ItemDeleteComponent>;
@@ -40,6 +42,7 @@ let mockItemDataService: ItemDataService;
 let routeStub;
 let objectUpdatesServiceStub;
 let relationshipService;
+let linkService;
 let entityTypeService;
 let notificationsServiceStub;
 let typesSelection;
@@ -52,7 +55,12 @@ describe('ItemDeleteComponent', () => {
       uuid: 'fake-uuid',
       handle: 'fake/handle',
       lastModified: '2018',
-      isWithdrawn: true
+      isWithdrawn: true,
+      metadata: {
+        'dspace.entity.type': [
+          { value: 'Person' }
+        ]
+      }
     });
 
     itemType = Object.assign(new ItemType(), {
@@ -129,6 +137,12 @@ describe('ItemDeleteComponent', () => {
       }
     );
 
+    linkService = jasmine.createSpyObj('linkService',
+      {
+        resolveLinks: relationships[0],
+      }
+    );
+
     notificationsServiceStub = new NotificationsServiceStub();
 
     TestBed.configureTestingModule({
@@ -142,6 +156,8 @@ describe('ItemDeleteComponent', () => {
         { provide: ObjectUpdatesService, useValue: objectUpdatesServiceStub },
         { provide: RelationshipService, useValue: relationshipService },
         { provide: EntityTypeService, useValue: entityTypeService },
+        { provide: RelationshipTypeService, useValue: {} },
+        { provide: LinkService, useValue: linkService },
       ], schemas: [
         CUSTOM_ELEMENTS_SCHEMA
       ]
@@ -166,25 +182,45 @@ describe('ItemDeleteComponent', () => {
   });
 
   describe('performAction', () => {
-    it('should call delete function from the ItemDataService', () => {
-      spyOn(comp, 'notify');
-      comp.performAction();
-      expect(mockItemDataService.delete)
-        .toHaveBeenCalledWith(mockItem.id, types.filter((type) => typesSelection[type]).map((type) => type.id));
-      expect(comp.notify).toHaveBeenCalled();
+    describe(`when there are entitytypes`, () => {
+      it('should call delete function from the ItemDataService', () => {
+        spyOn(comp, 'notify');
+        comp.performAction();
+        expect(mockItemDataService.delete)
+          .toHaveBeenCalledWith(mockItem.id, types.filter((type) => typesSelection[type]).map((type) => type.id));
+        expect(comp.notify).toHaveBeenCalled();
+      });
+
+      it('should call delete function from the ItemDataService with empty types', () => {
+
+        spyOn(comp, 'notify');
+        jasmine.getEnv().allowRespy(true);
+        spyOn(entityTypeService, 'getEntityTypeRelationships').and.returnValue([]);
+        comp.ngOnInit();
+
+        comp.performAction();
+
+        expect(mockItemDataService.delete).toHaveBeenCalledWith(mockItem.id, []);
+        expect(comp.notify).toHaveBeenCalled();
+      });
     });
 
-    it('should call delete function from the ItemDataService with empty types', () => {
-
-      spyOn(comp, 'notify');
-      jasmine.getEnv().allowRespy(true);
-      spyOn(entityTypeService, 'getEntityTypeRelationships').and.returnValue([]);
-      comp.ngOnInit();
-
-      comp.performAction();
-
-      expect(mockItemDataService.delete).toHaveBeenCalledWith(mockItem.id, []);
-      expect(comp.notify).toHaveBeenCalled();
+    describe(`when there are no entity types`, () => {
+      beforeEach(() => {
+        (comp as any).entityTypeService = jasmine.createSpyObj('entityTypeService',
+          {
+            getEntityTypeByLabel: EMPTY,
+          }
+        );
+      });
+
+      it('should call delete function from the ItemDataService', () => {
+        spyOn(comp, 'notify');
+        comp.performAction();
+        expect(mockItemDataService.delete)
+          .toHaveBeenCalledWith(mockItem.id, types.filter((type) => typesSelection[type]).map((type) => type.id));
+        expect(comp.notify).toHaveBeenCalled();
+      });
     });
   });
   describe('notify', () => {

src/app/item-page/edit-item-page/item-delete/item-delete.component.ts

@@ -1,12 +1,14 @@
-import { Component, Input, OnInit } from '@angular/core';
-import {defaultIfEmpty, filter, map, switchMap, take} from 'rxjs/operators';
-import { AbstractSimpleItemActionComponent } from '../simple-item-action/abstract-simple-item-action.component';
+import { Component, Input, OnInit, OnDestroy } from '@angular/core';
+import { defaultIfEmpty, filter, map, switchMap, take } from 'rxjs/operators';
+import {
+  AbstractSimpleItemActionComponent
+} from '../simple-item-action/abstract-simple-item-action.component';
 import { NgbModal, NgbModalRef } from '@ng-bootstrap/ng-bootstrap';
 import {
   combineLatest as observableCombineLatest,
   combineLatest,
   Observable,
-  of as observableOf
+  of as observableOf, Subscription
 } from 'rxjs';
 import { RelationshipType } from '../../../core/shared/item-relationships/relationship-type.model';
 import { VirtualMetadata } from '../virtual-metadata/virtual-metadata.component';
@@ -32,6 +34,7 @@ import { followLink } from '../../../shared/utils/follow-link-config.model';
 import { getItemEditRoute } from '../../item-page-routing-paths';
 import { RemoteData } from '../../../core/data/remote-data';
 import { NoContent } from '../../../core/shared/NoContent.model';
+import { BehaviorSubject } from 'rxjs/internal/BehaviorSubject';
 
 @Component({
   selector: 'ds-item-delete',
@@ -42,7 +45,7 @@ import { NoContent } from '../../../core/shared/NoContent.model';
  */
 export class ItemDeleteComponent
   extends AbstractSimpleItemActionComponent
-  implements OnInit {
+  implements OnInit, OnDestroy {
 
   /**
    * The current url of this page
@@ -60,7 +63,7 @@ export class ItemDeleteComponent
    * A list of the relationship types for which this item has relations as an observable.
    * The list doesn't contain duplicates.
    */
-  types$: Observable<RelationshipType[]>;
+  types$: BehaviorSubject<RelationshipType[]> = new BehaviorSubject([]);
 
   /**
    * A map which stores the relationships of this item for each type as observable lists
@@ -84,6 +87,11 @@ export class ItemDeleteComponent
    */
   public modalRef: NgbModalRef;
 
+  /**
+   * Array to track all subscriptions and unsubscribe them onDestroy
+   */
+  private subs: Subscription[] = [];
+
   constructor(protected route: ActivatedRoute,
               protected router: Router,
               protected notificationsService: NotificationsService,
@@ -113,8 +121,8 @@ export class ItemDeleteComponent
     this.url = this.router.url;
 
     const label = this.item.firstMetadataValue('dspace.entity.type');
-    if (label !== undefined) {
-      this.types$ = this.entityTypeService.getEntityTypeByLabel(label).pipe(
+    if (isNotEmpty(label)) {
+      this.subs.push(this.entityTypeService.getEntityTypeByLabel(label).pipe(
         getFirstSucceededRemoteData(),
         getRemoteDataPayload(),
         switchMap((entityType) => this.entityTypeService.getEntityTypeRelationships(entityType.id)),
@@ -138,16 +146,14 @@ export class ItemDeleteComponent
             ),
           );
         })
-      );
-    } else {
-      this.types$ = observableOf([]);
+      ).subscribe((types: RelationshipType[]) => this.types$.next(types)));
     }
 
-    this.types$.pipe(
+    this.subs.push(this.types$.pipe(
       take(1),
     ).subscribe((types) =>
       this.objectUpdatesService.initialize(this.url, types, this.item.lastModified)
-    );
+    ));
   }
 
   /**
@@ -327,7 +333,7 @@ export class ItemDeleteComponent
    */
   performAction() {
 
-    this.types$.pipe(
+    this.subs.push(this.types$.pipe(
       switchMap((types) =>
         combineLatest(
           types.map((type) => this.isSelected(type))
@@ -339,13 +345,14 @@ export class ItemDeleteComponent
           map((selectedTypes) => selectedTypes.map((type) => type.id)),
         )
       ),
-    ).subscribe((types) => {
-      this.itemDataService.delete(this.item.id, types).pipe(getFirstCompletedRemoteData()).subscribe(
-        (rd: RemoteData<NoContent>) => {
-          this.notify(rd.hasSucceeded);
-        }
-      );
-    });
+      switchMap((types) =>
+        this.itemDataService.delete(this.item.id, types).pipe(getFirstCompletedRemoteData())
+      )
+    ).subscribe(
+      (rd: RemoteData<NoContent>) => {
+        this.notify(rd.hasSucceeded);
+      }
+    ));
   }
 
   /**
@@ -361,4 +368,14 @@ export class ItemDeleteComponent
       this.router.navigate([getItemEditRoute(this.item)]);
     }
   }
+
+  /**
+   * Unsubscribe from all subscriptions
+   */
+  ngOnDestroy(): void {
+    this.subs
+      .filter((sub) => hasValue(sub))
+      .forEach((sub) => sub.unsubscribe());
+  }
+
 }