Merge pull request #1056 from atmire/stale-issue

Fix issue where stale RemoteData is emitted first

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

@@ -1,6 +1,6 @@
 import { HttpClient } from '@angular/common/http';
 import { Store } from '@ngrx/store';
-import { cold, getTestScheduler, hot } from 'jasmine-marbles';
+import { cold } from 'jasmine-marbles';
 import { Observable, of as observableOf } from 'rxjs';
 import { TestScheduler } from 'rxjs/testing';
 import { getMockRequestService } from '../../shared/mocks/request.service.mock';
@@ -13,16 +13,15 @@ import { HALEndpointService } from '../shared/hal-endpoint.service';
 import { ComColDataService } from './comcol-data.service';
 import { CommunityDataService } from './community-data.service';
 import { DSOChangeAnalyzer } from './dso-change-analyzer.service';
-import { FindListOptions, GetRequest } from './request.models';
-import { RequestEntry } from './request.reducer';
+import { FindListOptions } from './request.models';
 import { RequestService } from './request.service';
 import {
   createFailedRemoteDataObject$,
-  createNoContentRemoteDataObject$,
-  createSuccessfulRemoteDataObject$
+  createSuccessfulRemoteDataObject$,
+  createFailedRemoteDataObject,
+  createSuccessfulRemoteDataObject
 } from '../../shared/remote-data.utils';
 import { BitstreamDataService } from './bitstream-data.service';
-import { take } from 'rxjs/operators';
 
 const LINK_NAME = 'test';
 
@@ -50,8 +49,8 @@ class TestService extends ComColDataService<any> {
   }
 }
 
+// tslint:disable:no-shadowed-variable
 describe('ComColDataService', () => {
-  let scheduler: TestScheduler;
   let service: TestService;
   let requestService: RequestService;
   let cds: CommunityDataService;
@@ -59,6 +58,8 @@ describe('ComColDataService', () => {
   let halService: any = {};
   let bitstreamDataService: BitstreamDataService;
   let rdbService: RemoteDataBuildService;
+  let testScheduler: TestScheduler;
+  let topEndpoint: string;
 
   const store = {} as Store<CoreState>;
   const notificationsService = {} as NotificationsService;
@@ -69,17 +70,9 @@ describe('ComColDataService', () => {
   const options = Object.assign(new FindListOptions(), {
     scopeID: scopeID
   });
-  const getRequestEntry$ = (successful: boolean) => {
-    return observableOf({
-      response: { isSuccessful: successful } as any
-    } as RequestEntry);
-  };
-
   const communitiesEndpoint = 'https://rest.api/core/communities';
   const communityEndpoint = `${communitiesEndpoint}/${scopeID}`;
   const scopedEndpoint = `${communityEndpoint}/${LINK_NAME}`;
-  const serviceEndpoint = `https://rest.api/core/${LINK_NAME}`;
-  const authHeader = 'Bearer eyJhbGciOiJIUzI1NiJ9.eyJlaWQiOiJhNjA4NmIzNC0zOTE4LTQ1YjctOGRkZC05MzI5YTcwMmEyNmEiLCJzZyI6W10sImV4cCI6MTUzNDk0MDcyNX0.RV5GAtiX6cpwBN77P_v16iG9ipeyiO7faNYSNMzq_sQ';
 
   const mockHalService = {
     getEndpoint: (linkPath) => observableOf(communitiesEndpoint)
@@ -98,8 +91,8 @@ describe('ComColDataService', () => {
   }
 
   function initMockCommunityDataService(): CommunityDataService {
-    return jasmine.createSpyObj('responseCache', {
-      getEndpoint: hot('--a-', { a: communitiesEndpoint }),
+    return jasmine.createSpyObj('cds', {
+      getEndpoint: cold('--a-', { a: communitiesEndpoint }),
       getIDHref: communityEndpoint
     });
   }
@@ -134,7 +127,15 @@ describe('ComColDataService', () => {
     );
   }
 
+  const initTestScheduler = (): TestScheduler => {
+    return new TestScheduler((actual, expected) => {
+      expect(actual).toEqual(expected);
+    });
+  };
+
   beforeEach(() => {
+    topEndpoint = 'https://rest.api/core/communities/search/top';
+    testScheduler = initTestScheduler();
     cds = initMockCommunityDataService();
     requestService = getMockRequestService();
     objectCache = initMockObjectCacheService();
@@ -145,113 +146,94 @@ describe('ComColDataService', () => {
   });
 
   describe('getBrowseEndpoint', () => {
-    beforeEach(() => {
-      scheduler = getTestScheduler();
-    });
-
-    it('should send a new FindByIDRequest for the scope Community', () => {
-      cds = initMockCommunityDataService();
-      requestService = getMockRequestService(getRequestEntry$(true));
-      objectCache = initMockObjectCacheService();
-      service = initTestService();
-
-      const expected = new GetRequest(requestService.generateRequestId(), communityEndpoint);
-
-      scheduler.schedule(() => service.getBrowseEndpoint(options).subscribe());
-      scheduler.flush();
-
-      expect(requestService.send).toHaveBeenCalledWith(expected, true);
+    it(`should call createAndSendGetRequest with the scope Community's self link`, () => {
+      testScheduler.run(({ cold, flush, expectObservable }) => {
+        (cds.getEndpoint as jasmine.Spy).and.returnValue(cold('a', { a: communitiesEndpoint }));
+        (rdbService.buildSingle as jasmine.Spy).and.returnValue(cold('a', { a: createFailedRemoteDataObject() }));
+        spyOn(service as any, 'createAndSendGetRequest');
+        service.getBrowseEndpoint(options);
+        flush();
+        expectObservable((service as any).createAndSendGetRequest.calls.argsFor(0)[0]).toBe('(a|)', { a: communityEndpoint });
+        expect((service as any).createAndSendGetRequest.calls.argsFor(0)[1]).toBeTrue();
+      });
     });
 
     describe('if the scope Community can\'t be found', () => {
       it('should throw an error', () => {
-        const result = service.getBrowseEndpoint(options).pipe(take(1));
-        const expected = cold('--#-', undefined, new Error(`The Community with scope ${scopeID} couldn't be retrieved`));
-
-        expect(result).toBeObservable(expected);
-      });
-    });
-
-    describe('cache refresh', () => {
-      let communityWithoutParentHref;
-      let data;
-
-      beforeEach(() => {
-        spyOn(halService, 'getEndpoint').and.returnValue(observableOf('https://rest.api/core/communities/search/top'));
-      });
-
-      describe('cache refreshed top level community', () => {
-        beforeEach(() => {
-          (rdbService.buildSingle as jasmine.Spy).and.returnValue(createNoContentRemoteDataObject$());
-          data = {
-            dso: Object.assign(new Community(), {
-              metadata: [{
-                key: 'dc.title',
-                value: 'top level community'
-              }]
-            }),
-            _links: {
-              parentCommunity: {
-                href: 'topLevel/parentCommunity'
-              }
-            }
-          };
-          communityWithoutParentHref = {
-            dso: Object.assign(new Community(), {
-              metadata: [{
-                key: 'dc.title',
-                value: 'top level community'
-              }]
-            }),
-            _links: {}
-          };
-        });
-        it('top level community cache refreshed', () => {
-          scheduler.schedule(() => (service as any).refreshCache(data));
-          scheduler.flush();
-          expect(requestService.setStaleByHrefSubstring).toHaveBeenCalledWith('https://rest.api/core/communities/search/top');
-        });
-        it('top level community without parent link, cache not refreshed', () => {
-          scheduler.schedule(() => (service as any).refreshCache(communityWithoutParentHref));
-          scheduler.flush();
-          expect(requestService.setStaleByHrefSubstring).not.toHaveBeenCalled();
-        });
-      });
-
-      describe('cache refreshed child community', () => {
-        beforeEach(() => {
-          const parentCommunity = Object.assign(new Community(), {
-            uuid: 'a20da287-e174-466a-9926-f66as300d399',
-            id: 'a20da287-e174-466a-9926-f66as300d399',
-            metadata: [{
-              key: 'dc.title',
-              value: 'parent community'
-            }],
-            _links: {}
-          });
-          (rdbService.buildSingle as jasmine.Spy).and.returnValue(createSuccessfulRemoteDataObject$(parentCommunity));
-          data = {
-            dso: Object.assign(new Community(), {
-              metadata: [{
-                key: 'dc.title',
-                value: 'child community'
-              }]
-            }),
-            _links: {
-              parentCommunity: {
-                href: 'child/parentCommunity'
-              }
-            }
-          };
-        });
-        it('child level community cache refreshed', () => {
-          scheduler.schedule(() => (service as any).refreshCache(data));
-          scheduler.flush();
-          expect(requestService.setStaleByHrefSubstring).toHaveBeenCalledWith('a20da287-e174-466a-9926-f66as300d399');
+        testScheduler.run(({ cold, expectObservable }) => {
+          // spies re-defined here to use the "cold" function from rxjs's TestScheduler
+          // rather than the one imported from jasmine-marbles.
+          // Mixing the two seems to lead to unpredictable results
+          (cds.getEndpoint as jasmine.Spy).and.returnValue(cold('a', { a: communitiesEndpoint }));
+          (rdbService.buildSingle as jasmine.Spy).and.returnValue(cold('a', { a: createFailedRemoteDataObject() }));
+          const expectedError = new Error(`The Community with scope ${scopeID} couldn't be retrieved`);
+          expectObservable(service.getBrowseEndpoint(options)).toBe('#', undefined, expectedError);
         });
       });
     });
 
   });
 
+  describe('cache refresh', () => {
+    let communityWithoutParentHref;
+    let communityWithParentHref;
+
+    beforeEach(() => {
+      communityWithParentHref = {
+        _links: {
+          parentCommunity: {
+            href: 'topLevel/parentCommunity'
+          }
+        }
+      } as Community;
+      communityWithoutParentHref = {
+        _links: {}
+      } as Community;
+    });
+
+    describe('cache refreshed top level community', () => {
+      it(`should refresh the top level community cache when the dso has a parent link that can't be resolved`, () => {
+        testScheduler.run(({ flush, cold }) => {
+          spyOn(halService, 'getEndpoint').and.returnValue(cold('a', { a: topEndpoint }));
+          spyOn(service, 'findByHref').and.returnValue(cold('a', { a: createSuccessfulRemoteDataObject({}) }));
+          service.refreshCache(communityWithParentHref);
+          flush();
+          expect(requestService.setStaleByHrefSubstring).toHaveBeenCalledWith(topEndpoint);
+        });
+      });
+      it(`shouldn't do anything when the dso doesn't have a parent link`, () => {
+        testScheduler.run(({ flush, cold }) => {
+          spyOn(halService, 'getEndpoint').and.returnValue(cold('a', { a: topEndpoint }));
+          spyOn(service, 'findByHref').and.returnValue(cold('a', { a: createSuccessfulRemoteDataObject({}) }));
+          service.refreshCache(communityWithoutParentHref);
+          flush();
+          expect(requestService.setStaleByHrefSubstring).not.toHaveBeenCalled();
+        });
+      });
+    });
+
+    describe('cache refreshed child community', () => {
+      let parentCommunity: Community;
+      beforeEach(() => {
+        parentCommunity = Object.assign(new Community(), {
+          uuid: 'a20da287-e174-466a-9926-f66as300d399',
+          id: 'a20da287-e174-466a-9926-f66as300d399',
+          metadata: [{
+            key: 'dc.title',
+            value: 'parent community'
+          }],
+          _links: {}
+        });
+      });
+      it('should refresh a specific cached community when the parent link can be resolved', () => {
+        testScheduler.run(({ flush, cold }) => {
+          spyOn(halService, 'getEndpoint').and.returnValue(cold('a', { a: topEndpoint }));
+          spyOn(service, 'findByHref').and.returnValue(cold('a', { a: createSuccessfulRemoteDataObject(parentCommunity) }));
+          service.refreshCache(communityWithParentHref);
+          flush();
+          expect(requestService.setStaleByHrefSubstring).toHaveBeenCalledWith('a20da287-e174-466a-9926-f66as300d399');
+        });
+      });
+    });
+  });
 });

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

@@ -20,6 +20,9 @@ import { getMockRequestService } from '../../shared/mocks/request.service.mock';
 import { HALEndpointServiceStub } from '../../shared/testing/hal-endpoint-service.stub';
 import { RequestParam } from '../cache/models/request-param.model';
 import { getMockRemoteDataBuildService } from '../../shared/mocks/remote-data-build.service.mock';
+import { TestScheduler } from 'rxjs/testing';
+import { RemoteData } from './remote-data';
+import { RequestEntryState } from './request.reducer';
 
 const endpoint = 'https://rest.api/core';
 
@@ -63,6 +66,10 @@ describe('DataService', () => {
   let comparator;
   let objectCache;
   let store;
+  let selfLink;
+  let linksToFollow;
+  let testScheduler;
+  let remoteDataMocks;
 
   function initTestService(): TestService {
     requestService = getMockRequestService();
@@ -81,6 +88,34 @@ describe('DataService', () => {
       }
     } as any;
     store = {} as Store<CoreState>;
+    selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7';
+    linksToFollow = [
+      followLink('a'),
+      followLink('b')
+    ];
+
+    testScheduler = new TestScheduler((actual, expected) => {
+      // asserting the two objects are equal
+      // e.g. using chai.
+      expect(actual).toEqual(expected);
+    });
+
+    const timeStamp = new Date().getTime();
+    const msToLive = 15 * 60 * 1000;
+    const payload = { foo: 'bar' };
+    const statusCodeSuccess = 200;
+    const statusCodeError = 404;
+    const errorMessage = 'not found';
+    remoteDataMocks = {
+      RequestPending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
+      ResponsePending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
+      Success: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Success, undefined, payload, statusCodeSuccess),
+      SuccessStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.SuccessStale, undefined, payload, statusCodeSuccess),
+      Error: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
+      ErrorStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError),
+    };
+
+
     return new TestService(
       requestService,
       rdbService,
@@ -307,14 +342,12 @@ describe('DataService', () => {
 
   describe('update', () => {
     let operations;
-    let selfLink;
     let dso;
     let dso2;
     const name1 = 'random string';
     const name2 = 'another random string';
     beforeEach(() => {
       operations = [{ op: 'replace', path: '/0/value', value: name2 } as Operation];
-      selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7';
 
       dso = Object.assign(new DSpaceObject(), {
         _links: { self: { href: selfLink } },
@@ -340,5 +373,452 @@ describe('DataService', () => {
       expect(objectCache.addPatch).not.toHaveBeenCalled();
     });
   });
+
+  describe(`reRequestStaleRemoteData`, () => {
+    let callback: jasmine.Spy<jasmine.Func>;
+
+    beforeEach(() => {
+      callback = jasmine.createSpy();
+    });
+
+
+    describe(`when shouldReRequest is false`, () => {
+      it(`shouldn't do anything`, () => {
+        testScheduler.run(({ cold, expectObservable, flush }) => {
+          const expected = 'a-b-c-d-e-f';
+          const values = {
+            a: remoteDataMocks.RequestPending,
+            b: remoteDataMocks.ResponsePending,
+            c: remoteDataMocks.Success,
+            d: remoteDataMocks.SuccessStale,
+            e: remoteDataMocks.Error,
+            f: remoteDataMocks.ErrorStale,
+          };
+
+          expectObservable((service as any).reRequestStaleRemoteData(false, callback)(cold(expected, values))).toBe(expected, values);
+          // since the callback happens in a tap(), flush to ensure it has been executed
+          flush();
+          expect(callback).not.toHaveBeenCalled();
+        });
+      });
+    });
+
+    describe(`when shouldReRequest is true`, () => {
+      it(`should call the callback for stale RemoteData objects, but still pass the source observable unmodified`, () => {
+        testScheduler.run(({ cold, expectObservable, flush }) => {
+          const expected = 'a-b';
+          const values = {
+            a: remoteDataMocks.SuccessStale,
+            b: remoteDataMocks.ErrorStale,
+          };
+
+          expectObservable((service as any).reRequestStaleRemoteData(true, callback)(cold(expected, values))).toBe(expected, values);
+          // since the callback happens in a tap(), flush to ensure it has been executed
+          flush();
+          expect(callback).toHaveBeenCalledTimes(2);
+        });
+      });
+
+      it(`should only call the callback for stale RemoteData objects if something is subscribed to it`, (done) => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          const expected = 'a';
+          const values = {
+            a: remoteDataMocks.SuccessStale,
+          };
+
+          const result$ = (service as any).reRequestStaleRemoteData(true, callback)(cold(expected, values));
+          expectObservable(result$).toBe(expected, values);
+          expect(callback).not.toHaveBeenCalled();
+          result$.subscribe(() => {
+            expect(callback).toHaveBeenCalled();
+            done();
+          });
+        });
+      });
+
+      it(`shouldn't do anything for RemoteData objects that aren't stale`, () => {
+        testScheduler.run(({ cold, expectObservable, flush }) => {
+          const expected = 'a-b-c-d';
+          const values = {
+            a: remoteDataMocks.RequestPending,
+            b: remoteDataMocks.ResponsePending,
+            c: remoteDataMocks.Success,
+            d: remoteDataMocks.Error,
+          };
+
+          expectObservable((service as any).reRequestStaleRemoteData(true, callback)(cold(expected, values))).toBe(expected, values);
+          // since the callback happens in a tap(), flush to ensure it has been executed
+          flush();
+          expect(callback).not.toHaveBeenCalled();
+        });
+      });
+    });
+
+  });
+
+  describe(`findByHref`, () => {
+    beforeEach(() => {
+      spyOn(service as any, 'createAndSendGetRequest').and.callFake((href$) => { href$.subscribe().unsubscribe(); });
+    });
+
+    it(`should call buildHrefFromFindOptions with href and linksToFollow`, () => {
+      testScheduler.run(({ cold }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.Success }));
+
+        service.findByHref(selfLink, true, true, ...linksToFollow);
+        expect(service.buildHrefFromFindOptions).toHaveBeenCalledWith(selfLink, {}, [], ...linksToFollow);
+      });
+    });
+
+    it(`should call createAndSendGetRequest with the result from buildHrefFromFindOptions and useCachedVersionIfAvailable`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue('bingo!');
+        spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.Success }));
+
+        service.findByHref(selfLink, true, true, ...linksToFollow);
+        expect((service as any).createAndSendGetRequest).toHaveBeenCalledWith(jasmine.anything(), true);
+        expectObservable(rdbService.buildSingle.calls.argsFor(0)[0]).toBe('(a|)', { a: 'bingo!' });
+
+        service.findByHref(selfLink, false, true, ...linksToFollow);
+        expect((service as any).createAndSendGetRequest).toHaveBeenCalledWith(jasmine.anything(), false);
+        expectObservable(rdbService.buildSingle.calls.argsFor(1)[0]).toBe('(a|)', { a: 'bingo!' });
+      });
+    });
+
+    it(`should call rdbService.buildSingle with the result from buildHrefFromFindOptions and linksToFollow`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue('bingo!');
+        spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.Success }));
+
+        service.findByHref(selfLink, true, true, ...linksToFollow);
+        expect(rdbService.buildSingle).toHaveBeenCalledWith(jasmine.anything() as any, ...linksToFollow);
+        expectObservable(rdbService.buildSingle.calls.argsFor(0)[0]).toBe('(a|)', { a: 'bingo!' });
+      });
+    });
+
+    it(`should return a the output from reRequestStaleRemoteData`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: 'bingo!' }));
+        const expected = 'a';
+        const values = {
+          a: 'bingo!',
+        };
+
+        expectObservable(service.findByHref(selfLink, true, true, ...linksToFollow)).toBe(expected, values);
+      });
+    });
+
+    it(`should call reRequestStaleRemoteData with reRequestOnStale and the exact same findByHref call as a callback`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', { a: remoteDataMocks.SuccessStale }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.SuccessStale }));
+
+        service.findByHref(selfLink, true, true, ...linksToFollow);
+        expect((service as any).reRequestStaleRemoteData.calls.argsFor(0)[0]).toBeTrue();
+        spyOn(service, 'findByHref').and.returnValue(cold('a', { a: remoteDataMocks.SuccessStale }));
+        // prove that the spy we just added hasn't been called yet
+        expect(service.findByHref).not.toHaveBeenCalled();
+        // call the callback passed to reRequestStaleRemoteData
+        (service as any).reRequestStaleRemoteData.calls.argsFor(0)[1]();
+        // verify that findByHref _has_ been called now, with the same params as the original call
+        expect(service.findByHref).toHaveBeenCalledWith(jasmine.anything(), true, true, ...linksToFollow);
+        // ... except for selflink, which will have been turned in to an observable.
+        expectObservable((service.findByHref as jasmine.Spy).calls.argsFor(0)[0]).toBe('(a|)', { a: selfLink });
+      });
+    });
+
+    describe(`when useCachedVersionIfAvailable is true`, () => {
+      beforeEach(() => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(service as any, 'reRequestStaleRemoteData').and.callFake(() => (source) => source);
+      });
+
+      it(`should emit a cached completed RemoteData immediately, and keep emitting if it gets rerequested`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.Success,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = 'a-b-c-d-e';
+          const values = {
+            a: remoteDataMocks.Success,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findByHref(selfLink, true, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+      it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.SuccessStale,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = '--b-c-d-e';
+          const values = {
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findByHref(selfLink, true, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+    });
+
+    describe(`when useCachedVersionIfAvailable is false`, () => {
+      beforeEach(() => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(service as any, 'reRequestStaleRemoteData').and.callFake(() => (source) => source);
+      });
+
+
+      it(`should not emit a cached completed RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.Success,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = '--b-c-d-e';
+          const values = {
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findByHref(selfLink, false, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+      it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.SuccessStale,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = '--b-c-d-e';
+          const values = {
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findByHref(selfLink, false, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+    });
+
+  });
+
+  describe(`findAllByHref`, () => {
+    let findListOptions;
+    beforeEach(() => {
+      findListOptions = { currentPage: 5 };
+      spyOn(service as any, 'createAndSendGetRequest').and.callFake((href$) => { href$.subscribe().unsubscribe(); });
+    });
+
+    it(`should call buildHrefFromFindOptions with href and linksToFollow`, () => {
+      testScheduler.run(({ cold }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(rdbService, 'buildList').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.Success }));
+
+        service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow);
+        expect(service.buildHrefFromFindOptions).toHaveBeenCalledWith(selfLink, findListOptions, [], ...linksToFollow);
+      });
+    });
+
+    it(`should call createAndSendGetRequest with the result from buildHrefFromFindOptions and useCachedVersionIfAvailable`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue('bingo!');
+        spyOn(rdbService, 'buildList').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.Success }));
+
+        service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow);
+        expect((service as any).createAndSendGetRequest).toHaveBeenCalledWith(jasmine.anything(), true);
+        expectObservable(rdbService.buildList.calls.argsFor(0)[0]).toBe('(a|)', { a: 'bingo!' });
+
+        service.findAllByHref(selfLink, findListOptions, false, true, ...linksToFollow);
+        expect((service as any).createAndSendGetRequest).toHaveBeenCalledWith(jasmine.anything(), false);
+        expectObservable(rdbService.buildList.calls.argsFor(1)[0]).toBe('(a|)', { a: 'bingo!' });
+      });
+    });
+
+    it(`should call rdbService.buildList with the result from buildHrefFromFindOptions and linksToFollow`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue('bingo!');
+        spyOn(rdbService, 'buildList').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.Success }));
+
+        service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow);
+        expect(rdbService.buildList).toHaveBeenCalledWith(jasmine.anything() as any, ...linksToFollow);
+        expectObservable(rdbService.buildList.calls.argsFor(0)[0]).toBe('(a|)', { a: 'bingo!' });
+      });
+    });
+
+    it(`should call reRequestStaleRemoteData with reRequestOnStale and the exact same findAllByHref call as a callback`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue('bingo!');
+        spyOn(rdbService, 'buildList').and.returnValue(cold('a', { a: remoteDataMocks.SuccessStale }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: remoteDataMocks.SuccessStale }));
+
+        service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow);
+        expect((service as any).reRequestStaleRemoteData.calls.argsFor(0)[0]).toBeTrue();
+        spyOn(service, 'findAllByHref').and.returnValue(cold('a', { a: remoteDataMocks.SuccessStale }));
+        // prove that the spy we just added hasn't been called yet
+        expect(service.findAllByHref).not.toHaveBeenCalled();
+        // call the callback passed to reRequestStaleRemoteData
+        (service as any).reRequestStaleRemoteData.calls.argsFor(0)[1]();
+        // verify that findAllByHref _has_ been called now, with the same params as the original call
+        expect(service.findAllByHref).toHaveBeenCalledWith(jasmine.anything(), findListOptions, true, true, ...linksToFollow);
+        // ... except for selflink, which will have been turned in to an observable.
+        expectObservable((service.findAllByHref as jasmine.Spy).calls.argsFor(0)[0]).toBe('(a|)', { a: selfLink });
+      });
+    });
+
+    it(`should return a the output from reRequestStaleRemoteData`, () => {
+      testScheduler.run(({ cold, expectObservable }) => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(rdbService, 'buildList').and.returnValue(cold('a', { a: remoteDataMocks.Success }));
+        spyOn(service as any, 'reRequestStaleRemoteData').and.returnValue(() => cold('a', { a: 'bingo!' }));
+        const expected = 'a';
+        const values = {
+          a: 'bingo!',
+        };
+
+        expectObservable(service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow)).toBe(expected, values);
+      });
+    });
+
+    describe(`when useCachedVersionIfAvailable is true`, () => {
+      beforeEach(() => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(service as any, 'reRequestStaleRemoteData').and.callFake(() => (source) => source);
+      });
+
+      it(`should emit a cached completed RemoteData immediately, and keep emitting if it gets rerequested`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.Success,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = 'a-b-c-d-e';
+          const values = {
+            a: remoteDataMocks.Success,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+      it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.SuccessStale,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = '--b-c-d-e';
+          const values = {
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findAllByHref(selfLink, findListOptions, true, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+    });
+
+    describe(`when useCachedVersionIfAvailable is false`, () => {
+      beforeEach(() => {
+        spyOn(service, 'buildHrefFromFindOptions').and.returnValue(selfLink);
+        spyOn(service as any, 'reRequestStaleRemoteData').and.callFake(() => (source) => source);
+      });
+
+
+      it(`should not emit a cached completed RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.Success,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = '--b-c-d-e';
+          const values = {
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findAllByHref(selfLink, findListOptions, false, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+      it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
+        testScheduler.run(({ cold, expectObservable }) => {
+          spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
+            a: remoteDataMocks.SuccessStale,
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          }));
+          const expected = '--b-c-d-e';
+          const values = {
+            b: remoteDataMocks.RequestPending,
+            c: remoteDataMocks.ResponsePending,
+            d: remoteDataMocks.Success,
+            e: remoteDataMocks.SuccessStale,
+          };
+
+          expectObservable(service.findAllByHref(selfLink, findListOptions, false, true, ...linksToFollow)).toBe(expected, values);
+        });
+      });
+
+    });
+  });
 });
 /* tslint:enable:max-classes-per-file */

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

@@ -12,6 +12,7 @@ import {
   takeWhile,
   switchMap,
   tap,
+  skipWhile,
 } from 'rxjs/operators';
 import { hasValue, isNotEmpty, isNotEmptyOperator } from '../../shared/empty.util';
 import { NotificationOptions } from '../../shared/notifications/models/notification-options.model';
@@ -45,29 +46,6 @@ import { UpdateDataService } from './update-data.service';
 import { GenericConstructor } from '../shared/generic-constructor';
 import { NoContent } from '../shared/NoContent.model';
 
-/**
- * An operator that will call the given function if the incoming RemoteData is stale and
- * shouldReRequest is true
- *
- * @param shouldReRequest  Whether or not to call the re-request function if the RemoteData is stale
- * @param requestFn        The function to call if the RemoteData is stale and shouldReRequest is
- *                         true
- */
-export const reRequestStaleRemoteData = <T>(shouldReRequest: boolean, requestFn: () => Observable<RemoteData<T>>) =>
-  (source: Observable<RemoteData<T>>): Observable<RemoteData<T>> => {
-    if (shouldReRequest === true) {
-      return source.pipe(
-        tap((remoteData: RemoteData<T>) => {
-          if (hasValue(remoteData) && remoteData.isStale) {
-            requestFn();
-          }
-        })
-      );
-    } else {
-      return source;
-    }
-  };
-
 export abstract class DataService<T extends CacheableObject> implements UpdateDataService<T> {
   protected abstract requestService: RequestService;
   protected abstract rdbService: RemoteDataBuildService;
@@ -332,6 +310,30 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
     return this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow);
   }
 
+  /**
+   * An operator that will call the given function if the incoming RemoteData is stale and
+   * shouldReRequest is true
+   *
+   * @param shouldReRequest  Whether or not to call the re-request function if the RemoteData is stale
+   * @param requestFn        The function to call if the RemoteData is stale and shouldReRequest is
+   *                         true
+   */
+  protected reRequestStaleRemoteData<O>(shouldReRequest: boolean, requestFn: () => Observable<RemoteData<O>>) {
+    return (source: Observable<RemoteData<O>>): Observable<RemoteData<O>> => {
+      if (shouldReRequest === true) {
+        return source.pipe(
+          tap((remoteData: RemoteData<O>) => {
+            if (hasValue(remoteData) && remoteData.isStale) {
+              requestFn();
+            }
+          })
+        );
+      } else {
+        return source;
+      }
+    };
+  }
+
   /**
    * Returns an observable of {@link RemoteData} of an object, based on an href, with a list of
    * {@link FollowLinkConfig}, to automatically resolve {@link HALLink}s of the object
@@ -358,7 +360,12 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
     this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable);
 
     return this.rdbService.buildSingle<T>(requestHref$, ...linksToFollow).pipe(
-      reRequestStaleRemoteData(reRequestOnStale, () =>
+      // This skip ensures that if a stale object is present in the cache when you do a
+      // call it isn't immediately returned, but we wait until the remote data for the new request
+      // is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
+      // cached completed object
+      skipWhile((rd: RemoteData<T>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted),
+      this.reRequestStaleRemoteData(reRequestOnStale, () =>
         this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow))
     );
   }
@@ -390,7 +397,12 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa
     this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable);
 
     return this.rdbService.buildList<T>(requestHref$, ...linksToFollow).pipe(
-      reRequestStaleRemoteData(reRequestOnStale, () =>
+      // This skip ensures that if a stale object is present in the cache when you do a
+      // call it isn't immediately returned, but we wait until the remote data for the new request
+      // is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
+      // cached completed object
+      skipWhile((rd: RemoteData<PaginatedList<T>>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted),
+      this.reRequestStaleRemoteData(reRequestOnStale, () =>
         this.findAllByHref(href$, findListOptions, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow))
     );
   }

src/app/core/tasks/tasks.service.ts

@@ -110,7 +110,6 @@ export abstract class TasksService<T extends CacheableObject> extends DataServic
       find((href: string) => hasValue(href)),
       mergeMap((href) => this.findByHref(href, false, true).pipe(
         getAllCompletedRemoteData(),
-        filter((rd: RemoteData<T>) => !rd.isSuccessStale),
         tap(() => this.requestService.setStaleByHrefSubstring(href)))
       )
     );