94207: Extract 'wait for invalidation' pattern into a new method

Author: ybnd

src/app/core/cache/builders/remote-data-build.service.spec.ts

@@ -1,4 +1,4 @@
-import { createSuccessfulRemoteDataObject } from '../../../shared/remote-data.utils';
+import { createFailedRemoteDataObject, createPendingRemoteDataObject, createSuccessfulRemoteDataObject } from '../../../shared/remote-data.utils';
 import { buildPaginatedList, PaginatedList } from '../../data/paginated-list.model';
 import { Item } from '../../shared/item.model';
 import { PageInfo } from '../../shared/page-info.model';
@@ -19,6 +19,8 @@ import { HALLink } from '../../shared/hal-link.model';
 import { RequestEntryState } from '../../data/request-entry-state.model';
 import { RequestEntry } from '../../data/request-entry.model';
 import { cold } from 'jasmine-marbles';
+import { TestScheduler } from 'rxjs/testing';
+import { fakeAsync, tick } from '@angular/core/testing';
 
 describe('RemoteDataBuildService', () => {
   let service: RemoteDataBuildService;
@@ -763,4 +765,95 @@ describe('RemoteDataBuildService', () => {
       });
     });
   });
+
+  describe('buildFromRequestUUIDAndAwait', () => {
+    let testScheduler;
+
+    let callback: jasmine.Spy;
+    let buildFromRequestUUIDSpy;
+
+    const BOOLEAN = { t: true, f: false };
+
+    const MOCK_PENDING_RD = createPendingRemoteDataObject();
+    const MOCK_SUCCEEDED_RD = createSuccessfulRemoteDataObject({});
+    const MOCK_FAILED_RD = createFailedRemoteDataObject('failed');
+
+    const RDs = {
+      p: MOCK_PENDING_RD,
+      s: MOCK_SUCCEEDED_RD,
+      f: MOCK_FAILED_RD,
+    };
+
+
+    beforeEach(() => {
+      testScheduler = new TestScheduler((actual, expected) => {
+        expect(actual).toEqual(expected);
+      });
+
+      callback = jasmine.createSpy('callback');
+      callback.and.returnValue(observableOf(undefined));
+      buildFromRequestUUIDSpy = spyOn(service, 'buildFromRequestUUID').and.callThrough();
+    });
+
+    it('should patch through href & followLinks to buildFromRequestUUID', () => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+      service.buildFromRequestUUIDAndAwait('some-href', callback, ...linksToFollow);
+      expect(buildFromRequestUUIDSpy).toHaveBeenCalledWith('some-href', ...linksToFollow);
+    });
+
+    it('should trigger the callback on successful RD', (done) => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback).subscribe(rd => {
+        expect(rd).toBe(MOCK_SUCCEEDED_RD);
+        expect(callback).toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should trigger the callback on successful RD even if nothing subscribes to the returned Observable', fakeAsync(() => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback);
+      tick();
+
+      expect(callback).toHaveBeenCalled();
+    }));
+
+    it('should not trigger the callback on pending RD', (done) => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_PENDING_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback).subscribe(rd => {
+        expect(rd).toBe(MOCK_PENDING_RD);
+        expect(callback).not.toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should not trigger the callback on failed RD', (done) => {
+      buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_FAILED_RD));
+
+      service.buildFromRequestUUIDAndAwait('some-href', callback).subscribe(rd => {
+        expect(rd).toBe(MOCK_FAILED_RD);
+        expect(callback).not.toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it('should only emit after the callback is done', () => {
+      testScheduler.run(({ cold: tsCold, expectObservable }) => {
+        buildFromRequestUUIDSpy.and.returnValue(
+          tsCold('-p----s', RDs)
+        );
+        callback.and.returnValue(
+          tsCold('      --t', BOOLEAN)
+        );
+
+        const done$ = service.buildFromRequestUUIDAndAwait('some-href', callback);
+        expectObservable(done$).toBe(
+          '       -p------s', RDs       // resulting duration between pending & successful includes the callback
+        );
+      });
+    });
+  });
 });

src/app/core/cache/builders/remote-data-build.service.ts

@@ -1,5 +1,6 @@
 import { Injectable } from '@angular/core';
 import {
+  AsyncSubject,
   combineLatest as observableCombineLatest,
   Observable,
   of as observableOf,
@@ -24,6 +25,7 @@ import { hasSucceeded, isStale, RequestEntryState } from '../../data/request-ent
 import { getRequestFromRequestHref, getRequestFromRequestUUID } from '../../shared/request.operators';
 import { RequestEntry } from '../../data/request-entry.model';
 import { ResponseState } from '../../data/response-state.model';
+import { getFirstCompletedRemoteData } from '../../shared/operators';
 
 @Injectable()
 export class RemoteDataBuildService {
@@ -188,6 +190,49 @@ export class RemoteDataBuildService {
     return this.toRemoteDataObservable<T>(requestEntry$, payload$);
   }
 
+  /**
+   * Creates a {@link RemoteData} object for a rest request and its response
+   * and emits it only after the callback function is completed.
+   *
+   * @param requestUUID$    The UUID of the request we want to retrieve
+   * @param callback        A function that returns an Observable. It will only be called once the request has succeeded.
+   *                        Then, the response will only be emitted after this callback function has emitted.
+   * @param linksToFollow   List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved
+   */
+  buildFromRequestUUIDAndAwait<T>(requestUUID$: string | Observable<string>, callback: (rd?: RemoteData<T>) => Observable<unknown>, ...linksToFollow: FollowLinkConfig<any>[]): Observable<RemoteData<T>> {
+    const response$ = this.buildFromRequestUUID(requestUUID$, ...linksToFollow);
+
+    const callbackDone$ = new AsyncSubject<boolean>();
+    response$.pipe(
+      getFirstCompletedRemoteData(),
+      switchMap((rd: RemoteData<any>) => {
+        if (rd.hasSucceeded) {
+          // if the request succeeded, execute the callback
+          return callback(rd);
+        } else {
+          // otherwise, emit right away so the subscription doesn't stick around
+          return [true];
+        }
+      }),
+    ).subscribe(() => {
+      callbackDone$.next(true);
+      callbackDone$.complete();
+    });
+
+    return response$.pipe(
+      switchMap((rd: RemoteData<any>) => {
+        if (rd.hasSucceeded) {
+          // if the request succeeded, wait for the callback to finish
+          return callbackDone$.pipe(
+            map(() => rd),
+          );
+        } else {
+          return [rd];
+        }
+      })
+    );
+  }
+
   /**
    * Creates a {@link RemoteData} object for a rest request and its response
    *

src/app/core/data/bitstream-format-data.service.spec.ts

@@ -59,7 +59,8 @@ describe('BitstreamFormatDataService', () => {
   function initTestService(halService) {
     rd = createSuccessfulRemoteDataObject({});
     rdbService = jasmine.createSpyObj('rdbService', {
-      buildFromRequestUUID: observableOf(rd)
+      buildFromRequestUUID: observableOf(rd),
+      buildFromRequestUUIDAndAwait: observableOf(rd),
     });
 
     return new BitstreamFormatDataService(

src/app/core/data/data.service.spec.ts

@@ -920,24 +920,24 @@ describe('DataService', () => {
     let MOCK_SUCCEEDED_RD;
     let MOCK_FAILED_RD;
 
-    let invalidateByHrefSpy: jasmine.Spy;
-    let buildFromRequestUUIDSpy: jasmine.Spy;
+    let buildFromRequestUUIDAndAwaitSpy: jasmine.Spy;
     let getIDHrefObsSpy: jasmine.Spy;
     let deleteByHrefSpy: jasmine.Spy;
+    let invalidateByHrefSpy: jasmine.Spy;
 
     beforeEach(() => {
-      invalidateByHrefSpy = spyOn(service, 'invalidateByHref').and.returnValue(observableOf(true));
-      buildFromRequestUUIDSpy = spyOn(rdbService, 'buildFromRequestUUID').and.callThrough();
+      buildFromRequestUUIDAndAwaitSpy = spyOn(rdbService, 'buildFromRequestUUIDAndAwait').and.callThrough();
       getIDHrefObsSpy = spyOn(service, 'getIDHrefObs').and.callThrough();
       deleteByHrefSpy = spyOn(service, 'deleteByHref').and.callThrough();
+      invalidateByHrefSpy = spyOn(service, 'invalidateByHref').and.returnValue(observableOf(true));
 
       MOCK_SUCCEEDED_RD = createSuccessfulRemoteDataObject({});
       MOCK_FAILED_RD = createFailedRemoteDataObject('something went wrong');
     });
 
     it('should retrieve href by ID and call deleteByHref', () => {
       getIDHrefObsSpy.and.returnValue(observableOf('some-href'));
-      buildFromRequestUUIDSpy.and.returnValue(createSuccessfulRemoteDataObject$({}));
+      buildFromRequestUUIDAndAwaitSpy.and.returnValue(createSuccessfulRemoteDataObject$({}));
 
       service.delete('some-id', ['a', 'b', 'c']).subscribe(rd => {
         expect(getIDHrefObsSpy).toHaveBeenCalledWith('some-id');
@@ -946,65 +946,27 @@ describe('DataService', () => {
     });
 
     describe('deleteByHref', () => {
-      it('should call invalidateByHref if the DELETE request succeeds', (done) => {
-        buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
-
-        service.deleteByHref('some-href').subscribe(rd => {
-          expect(rd).toBe(MOCK_SUCCEEDED_RD);
-          expect(invalidateByHrefSpy).toHaveBeenCalled();
-          done();
-        });
+      beforeEach(() => {
+        buildFromRequestUUIDAndAwaitSpy.and.returnValue(createSuccessfulRemoteDataObject$({}));
       });
 
-      it('should call invalidateByHref even if not subscribing to returned Observable', fakeAsync(() => {
-        buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_SUCCEEDED_RD));
-
-        service.deleteByHref('some-href');
-        tick();
-
-        expect(invalidateByHrefSpy).toHaveBeenCalled();
-      }));
-
-      it('should not call invalidateByHref if the DELETE request fails', (done) => {
-        buildFromRequestUUIDSpy.and.returnValue(observableOf(MOCK_FAILED_RD));
+      it('should send a DELETE request', () => {
+        service.deleteByHref('https://somewhere.org/something/123', ['things', 'stuff']);
 
-        service.deleteByHref('some-href').subscribe(rd => {
-          expect(rd).toBe(MOCK_FAILED_RD);
-          expect(invalidateByHrefSpy).not.toHaveBeenCalled();
-          done();
-        });
+        expect(requestService.send).toHaveBeenCalledWith(jasmine.objectContaining({
+          method: 'DELETE',
+          href: 'https://somewhere.org/something/123?copyVirtualMetadata=things&copyVirtualMetadata=stuff'
+        }));
       });
 
-      it('should wait for invalidateByHref before emitting', () => {
-        testScheduler.run(({ cold, expectObservable }) => {
-          buildFromRequestUUIDSpy.and.returnValue(
-            cold('(r|)', { r: MOCK_SUCCEEDED_RD})      // RD emits right away
-          );
-          invalidateByHrefSpy.and.returnValue(
-            cold('----(t|)', BOOLEAN)                  // but we pretend that setting requests to stale takes longer
-          );
-
-          const done$ = service.deleteByHref('some-href');
-          expectObservable(done$).toBe(
-            '----(r|)', { r: MOCK_SUCCEEDED_RD}        // ...and expect the returned Observable to wait until that's done
-          );
-        });
-      });
+      it('should retrieve response via buildFromRequestUUIDAndAwait & invalidate within the callback', () => {
+        service.deleteByHref('https://somewhere.org/something/123');
 
-      it('should wait for the DELETE request to resolve before emitting', () => {
-        testScheduler.run(({ cold, expectObservable }) => {
-          buildFromRequestUUIDSpy.and.returnValue(
-            cold('----(r|)', { r: MOCK_SUCCEEDED_RD})   // the request takes a while
-          );
-          invalidateByHrefSpy.and.returnValue(
-            cold('(t|)', BOOLEAN)                       // but we pretend that setting to stale happens sooner
-          );                                            // e.g.: maybe already stale before this call?
-
-          const done$ = service.deleteByHref('some-href');
-          expectObservable(done$).toBe(
-            '----(r|)', { r: MOCK_SUCCEEDED_RD}         // ...and expect the returned Observable to wait for the request
-          );
-        });
+        expect(buildFromRequestUUIDAndAwaitSpy).toHaveBeenCalled();
+        expect(buildFromRequestUUIDAndAwaitSpy.calls.argsFor(0)[0]).toBe(requestService.generateRequestId());
+        const callback = buildFromRequestUUIDAndAwaitSpy.calls.argsFor(0)[1];
+        callback();
+        expect(invalidateByHrefSpy).toHaveBeenCalledWith('https://somewhere.org/something/123');
       });
     });
   });

src/app/core/data/data.service.ts

@@ -649,35 +649,7 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
     }
     this.requestService.send(request);
 
-    const response$ = this.rdbService.buildFromRequestUUID(requestId);
-
-    const invalidated$ = new AsyncSubject<boolean>();
-    response$.pipe(
-      getFirstCompletedRemoteData(),
-      switchMap((rd: RemoteData<NoContent>) => {
-        if (rd.hasSucceeded) {
-          return this.invalidateByHref(href);
-        } else {
-          return [true];
-        }
-      })
-    ).subscribe(() => {
-      invalidated$.next(true);
-      invalidated$.complete();
-    });
-
-    return response$.pipe(
-      switchMap((rd: RemoteData<NoContent>) => {
-        if (rd.hasSucceeded) {
-          return invalidated$.pipe(
-            filter((invalidated: boolean) => invalidated),
-            map(() => rd)
-          );
-        } else {
-          return [rd];
-        }
-      })
-    );
+    return this.rdbService.buildFromRequestUUIDAndAwait(requestId, () => this.invalidateByHref(href));
   }
 
   /**

src/app/core/eperson/group-data.service.spec.ts

@@ -95,6 +95,7 @@ describe('GroupDataService', () => {
     store = new Store<CoreState>(undefined, undefined, undefined);
     service = initTestService();
     spyOn(store, 'dispatch');
+    spyOn(rdbService, 'buildFromRequestUUIDAndAwait').and.callThrough();
   });
 
   describe('searchGroups', () => {
@@ -136,6 +137,11 @@ describe('GroupDataService', () => {
       expect(requestService.send).toHaveBeenCalledWith(expected);
     });
     it('should invalidate the previous requests of the parent group', () => {
+      expect(rdbService.buildFromRequestUUIDAndAwait).toHaveBeenCalled();
+      expect(rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[0]).toBe(requestService.generateRequestId());
+      const callback = rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[1];
+      callback();
+
       expect(objectCache.getByHref).toHaveBeenCalledWith(GroupMock._links.self.href);
       expect(requestService.setStaleByUUID).toHaveBeenCalledTimes(2);
       expect(requestService.setStaleByUUID).toHaveBeenCalledWith('request1');
@@ -156,6 +162,11 @@ describe('GroupDataService', () => {
       expect(requestService.send).toHaveBeenCalledWith(expected);
     });
     it('should invalidate the previous requests of the parent group\'', () => {
+      expect(rdbService.buildFromRequestUUIDAndAwait).toHaveBeenCalled();
+      expect(rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[0]).toBe(requestService.generateRequestId());
+      const callback = rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[1];
+      callback();
+
       expect(objectCache.getByHref).toHaveBeenCalledWith(GroupMock._links.self.href);
       expect(requestService.setStaleByUUID).toHaveBeenCalledTimes(2);
       expect(requestService.setStaleByUUID).toHaveBeenCalledWith('request1');
@@ -180,6 +191,11 @@ describe('GroupDataService', () => {
       expect(requestService.send).toHaveBeenCalledWith(expected);
     });
     it('should invalidate the previous requests of the EPerson and the group', () => {
+      expect(rdbService.buildFromRequestUUIDAndAwait).toHaveBeenCalled();
+      expect(rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[0]).toBe(requestService.generateRequestId());
+      const callback = rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[1];
+      callback();
+
       expect(objectCache.getByHref).toHaveBeenCalledWith(EPersonMock2._links.self.href);
       expect(objectCache.getByHref).toHaveBeenCalledWith(GroupMock._links.self.href);
       expect(requestService.setStaleByUUID).toHaveBeenCalledTimes(4);
@@ -201,6 +217,11 @@ describe('GroupDataService', () => {
       expect(requestService.send).toHaveBeenCalledWith(expected);
     });
     it('should invalidate the previous requests of the EPerson and the group', () => {
+      expect(rdbService.buildFromRequestUUIDAndAwait).toHaveBeenCalled();
+      expect(rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[0]).toBe(requestService.generateRequestId());
+      const callback = rdbService.buildFromRequestUUIDAndAwait.calls.argsFor(0)[1];
+      callback();
+
       expect(objectCache.getByHref).toHaveBeenCalledWith(EPersonMock._links.self.href);
       expect(objectCache.getByHref).toHaveBeenCalledWith(GroupMock._links.self.href);
       expect(requestService.setStaleByUUID).toHaveBeenCalledTimes(4);