diff --git a/src/app/core/data/base/base-data.service.spec.ts b/src/app/core/data/base/base-data.service.spec.ts index 098f075c101..75662a691fa 100644 --- a/src/app/core/data/base/base-data.service.spec.ts +++ b/src/app/core/data/base/base-data.service.spec.ts @@ -95,6 +95,7 @@ describe('BaseDataService', () => { remoteDataMocks = { RequestPending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.RequestPending, undefined, undefined, undefined), ResponsePending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePending, undefined, undefined, undefined), + ResponsePendingStale: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePendingStale, 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), @@ -303,19 +304,21 @@ describe('BaseDataService', () => { 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, + spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', { + a: remoteDataMocks.ResponsePendingStale, + b: remoteDataMocks.SuccessStale, + c: remoteDataMocks.ErrorStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, })); - const expected = '--b-c-d-e'; + const expected = '------d-e-f-g'; const values = { - b: remoteDataMocks.RequestPending, - c: remoteDataMocks.ResponsePending, - d: remoteDataMocks.Success, - e: remoteDataMocks.SuccessStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, }; expectObservable(service.findByHref(selfLink, true, true, ...linksToFollow)).toBe(expected, values); @@ -354,19 +357,21 @@ describe('BaseDataService', () => { 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, + spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', { + a: remoteDataMocks.ResponsePendingStale, + b: remoteDataMocks.SuccessStale, + c: remoteDataMocks.ErrorStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, })); - const expected = '--b-c-d-e'; + const expected = '------d-e-f-g'; const values = { - b: remoteDataMocks.RequestPending, - c: remoteDataMocks.ResponsePending, - d: remoteDataMocks.Success, - e: remoteDataMocks.SuccessStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, }; expectObservable(service.findByHref(selfLink, false, true, ...linksToFollow)).toBe(expected, values); @@ -487,19 +492,21 @@ describe('BaseDataService', () => { 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, + spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', { + a: remoteDataMocks.ResponsePendingStale, + b: remoteDataMocks.SuccessStale, + c: remoteDataMocks.ErrorStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, })); - const expected = '--b-c-d-e'; + const expected = '------d-e-f-g'; const values = { - b: remoteDataMocks.RequestPending, - c: remoteDataMocks.ResponsePending, - d: remoteDataMocks.Success, - e: remoteDataMocks.SuccessStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, }; expectObservable(service.findListByHref(selfLink, findListOptions, true, true, ...linksToFollow)).toBe(expected, values); @@ -538,21 +545,24 @@ describe('BaseDataService', () => { 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, + spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', { + a: remoteDataMocks.ResponsePendingStale, + b: remoteDataMocks.SuccessStale, + c: remoteDataMocks.ErrorStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, })); - const expected = '--b-c-d-e'; + const expected = '------d-e-f-g'; const values = { - b: remoteDataMocks.RequestPending, - c: remoteDataMocks.ResponsePending, - d: remoteDataMocks.Success, - e: remoteDataMocks.SuccessStale, + d: remoteDataMocks.RequestPending, + e: remoteDataMocks.ResponsePending, + f: remoteDataMocks.Success, + g: remoteDataMocks.SuccessStale, }; + expectObservable(service.findListByHref(selfLink, findListOptions, false, true, ...linksToFollow)).toBe(expected, values); }); }); diff --git a/src/app/core/data/base/base-data.service.ts b/src/app/core/data/base/base-data.service.ts index edd6d9e2a42..c7cd5b0a705 100644 --- a/src/app/core/data/base/base-data.service.ts +++ b/src/app/core/data/base/base-data.service.ts @@ -273,7 +273,7 @@ export class BaseDataService implements HALDataServic // 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) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted), + skipWhile((rd: RemoteData) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)), this.reRequestStaleRemoteData(reRequestOnStale, () => this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)), ); @@ -307,7 +307,7 @@ export class BaseDataService implements HALDataServic // 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>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted), + skipWhile((rd: RemoteData>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)), this.reRequestStaleRemoteData(reRequestOnStale, () => this.findListByHref(href$, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)), ); diff --git a/src/app/core/data/request-entry-state.model.spec.ts b/src/app/core/data/request-entry-state.model.spec.ts new file mode 100644 index 00000000000..7daa655566a --- /dev/null +++ b/src/app/core/data/request-entry-state.model.spec.ts @@ -0,0 +1,186 @@ +import { + isRequestPending, + isError, + isSuccess, + isErrorStale, + isSuccessStale, + isResponsePending, + isResponsePendingStale, + isLoading, + isStale, + hasFailed, + hasSucceeded, + hasCompleted, + RequestEntryState +} from './request-entry-state.model'; + +describe(`isRequestPending`, () => { + it(`should only return true if the given state is RequestPending`, () => { + expect(isRequestPending(RequestEntryState.RequestPending)).toBeTrue(); + + expect(isRequestPending(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isRequestPending(RequestEntryState.Error)).toBeFalse(); + expect(isRequestPending(RequestEntryState.Success)).toBeFalse(); + expect(isRequestPending(RequestEntryState.ResponsePendingStale)).toBeFalse(); + expect(isRequestPending(RequestEntryState.ErrorStale)).toBeFalse(); + expect(isRequestPending(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`isError`, () => { + it(`should only return true if the given state is Error`, () => { + expect(isError(RequestEntryState.Error)).toBeTrue(); + + expect(isError(RequestEntryState.RequestPending)).toBeFalse(); + expect(isError(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isError(RequestEntryState.Success)).toBeFalse(); + expect(isError(RequestEntryState.ResponsePendingStale)).toBeFalse(); + expect(isError(RequestEntryState.ErrorStale)).toBeFalse(); + expect(isError(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`isSuccess`, () => { + it(`should only return true if the given state is Success`, () => { + expect(isSuccess(RequestEntryState.Success)).toBeTrue(); + + expect(isSuccess(RequestEntryState.RequestPending)).toBeFalse(); + expect(isSuccess(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isSuccess(RequestEntryState.Error)).toBeFalse(); + expect(isSuccess(RequestEntryState.ResponsePendingStale)).toBeFalse(); + expect(isSuccess(RequestEntryState.ErrorStale)).toBeFalse(); + expect(isSuccess(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`isErrorStale`, () => { + it(`should only return true if the given state is ErrorStale`, () => { + expect(isErrorStale(RequestEntryState.ErrorStale)).toBeTrue(); + + expect(isErrorStale(RequestEntryState.RequestPending)).toBeFalse(); + expect(isErrorStale(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isErrorStale(RequestEntryState.Error)).toBeFalse(); + expect(isErrorStale(RequestEntryState.Success)).toBeFalse(); + expect(isErrorStale(RequestEntryState.ResponsePendingStale)).toBeFalse(); + expect(isErrorStale(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`isSuccessStale`, () => { + it(`should only return true if the given state is SuccessStale`, () => { + expect(isSuccessStale(RequestEntryState.SuccessStale)).toBeTrue(); + + expect(isSuccessStale(RequestEntryState.RequestPending)).toBeFalse(); + expect(isSuccessStale(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isSuccessStale(RequestEntryState.Error)).toBeFalse(); + expect(isSuccessStale(RequestEntryState.Success)).toBeFalse(); + expect(isSuccessStale(RequestEntryState.ResponsePendingStale)).toBeFalse(); + expect(isSuccessStale(RequestEntryState.ErrorStale)).toBeFalse(); + }); +}); + +describe(`isResponsePending`, () => { + it(`should only return true if the given state is ResponsePending`, () => { + expect(isResponsePending(RequestEntryState.ResponsePending)).toBeTrue(); + + expect(isResponsePending(RequestEntryState.RequestPending)).toBeFalse(); + expect(isResponsePending(RequestEntryState.Error)).toBeFalse(); + expect(isResponsePending(RequestEntryState.Success)).toBeFalse(); + expect(isResponsePending(RequestEntryState.ResponsePendingStale)).toBeFalse(); + expect(isResponsePending(RequestEntryState.ErrorStale)).toBeFalse(); + expect(isResponsePending(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`isResponsePendingStale`, () => { + it(`should only return true if the given state is requestPending`, () => { + expect(isResponsePendingStale(RequestEntryState.ResponsePendingStale)).toBeTrue(); + + expect(isResponsePendingStale(RequestEntryState.RequestPending)).toBeFalse(); + expect(isResponsePendingStale(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isResponsePendingStale(RequestEntryState.Error)).toBeFalse(); + expect(isResponsePendingStale(RequestEntryState.Success)).toBeFalse(); + expect(isResponsePendingStale(RequestEntryState.ErrorStale)).toBeFalse(); + expect(isResponsePendingStale(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`isLoading`, () => { + it(`should only return true if the given state is RequestPending, ResponsePending or ResponsePendingStale`, () => { + expect(isLoading(RequestEntryState.RequestPending)).toBeTrue(); + expect(isLoading(RequestEntryState.ResponsePending)).toBeTrue(); + expect(isLoading(RequestEntryState.ResponsePendingStale)).toBeTrue(); + + expect(isLoading(RequestEntryState.Error)).toBeFalse(); + expect(isLoading(RequestEntryState.Success)).toBeFalse(); + expect(isLoading(RequestEntryState.ErrorStale)).toBeFalse(); + expect(isLoading(RequestEntryState.SuccessStale)).toBeFalse(); + }); +}); + +describe(`hasFailed`, () => { + describe(`when the state is loading`, () => { + it(`should return undefined`, () => { + expect(hasFailed(RequestEntryState.RequestPending)).toBeUndefined(); + expect(hasFailed(RequestEntryState.ResponsePending)).toBeUndefined(); + expect(hasFailed(RequestEntryState.ResponsePendingStale)).toBeUndefined(); + }); + }); + + describe(`when the state has completed`, () => { + it(`should only return true if the given state is Error or ErrorStale`, () => { + expect(hasFailed(RequestEntryState.Error)).toBeTrue(); + expect(hasFailed(RequestEntryState.ErrorStale)).toBeTrue(); + + expect(hasFailed(RequestEntryState.Success)).toBeFalse(); + expect(hasFailed(RequestEntryState.SuccessStale)).toBeFalse(); + }); + }); +}); + +describe(`hasSucceeded`, () => { + describe(`when the state is loading`, () => { + it(`should return undefined`, () => { + expect(hasSucceeded(RequestEntryState.RequestPending)).toBeUndefined(); + expect(hasSucceeded(RequestEntryState.ResponsePending)).toBeUndefined(); + expect(hasSucceeded(RequestEntryState.ResponsePendingStale)).toBeUndefined(); + }); + }); + + describe(`when the state has completed`, () => { + it(`should only return true if the given state is Error or ErrorStale`, () => { + expect(hasSucceeded(RequestEntryState.Success)).toBeTrue(); + expect(hasSucceeded(RequestEntryState.SuccessStale)).toBeTrue(); + + expect(hasSucceeded(RequestEntryState.Error)).toBeFalse(); + expect(hasSucceeded(RequestEntryState.ErrorStale)).toBeFalse(); + }); + }); +}); + + +describe(`hasCompleted`, () => { + it(`should only return true if the given state is Error, Success, ErrorStale or SuccessStale`, () => { + expect(hasCompleted(RequestEntryState.Error)).toBeTrue(); + expect(hasCompleted(RequestEntryState.Success)).toBeTrue(); + expect(hasCompleted(RequestEntryState.ErrorStale)).toBeTrue(); + expect(hasCompleted(RequestEntryState.SuccessStale)).toBeTrue(); + + expect(hasCompleted(RequestEntryState.RequestPending)).toBeFalse(); + expect(hasCompleted(RequestEntryState.ResponsePending)).toBeFalse(); + expect(hasCompleted(RequestEntryState.ResponsePendingStale)).toBeFalse(); + }); +}); + +describe(`isStale`, () => { + it(`should only return true if the given state is ResponsePendingStale, SuccessStale or ErrorStale`, () => { + expect(isStale(RequestEntryState.ResponsePendingStale)).toBeTrue(); + expect(isStale(RequestEntryState.SuccessStale)).toBeTrue(); + expect(isStale(RequestEntryState.ErrorStale)).toBeTrue(); + + expect(isStale(RequestEntryState.RequestPending)).toBeFalse(); + expect(isStale(RequestEntryState.ResponsePending)).toBeFalse(); + expect(isStale(RequestEntryState.Error)).toBeFalse(); + expect(isStale(RequestEntryState.Success)).toBeFalse(); + }); +}); diff --git a/src/app/core/data/request-entry-state.model.ts b/src/app/core/data/request-entry-state.model.ts index a813b6e7436..3aeace39d29 100644 --- a/src/app/core/data/request-entry-state.model.ts +++ b/src/app/core/data/request-entry-state.model.ts @@ -3,8 +3,9 @@ export enum RequestEntryState { ResponsePending = 'ResponsePending', Error = 'Error', Success = 'Success', + ResponsePendingStale = 'ResponsePendingStale', ErrorStale = 'ErrorStale', - SuccessStale = 'SuccessStale' + SuccessStale = 'SuccessStale', } /** @@ -42,12 +43,21 @@ export const isSuccessStale = (state: RequestEntryState) => */ export const isResponsePending = (state: RequestEntryState) => state === RequestEntryState.ResponsePending; + /** - * Returns true if the given state is RequestPending or ResponsePending, - * false otherwise + * Returns true if the given state is ResponsePendingStale, false otherwise + */ +export const isResponsePendingStale = (state: RequestEntryState) => + state === RequestEntryState.ResponsePendingStale; + +/** + * Returns true if the given state is RequestPending, RequestPendingStale, ResponsePending, or + * ResponsePendingStale, false otherwise */ export const isLoading = (state: RequestEntryState) => - isRequestPending(state) || isResponsePending(state); + isRequestPending(state) || + isResponsePending(state) || + isResponsePendingStale(state); /** * If isLoading is true for the given state, this method returns undefined, we can't know yet. @@ -82,7 +92,10 @@ export const hasCompleted = (state: RequestEntryState) => !isLoading(state); /** - * Returns true if the given state is SuccessStale or ErrorStale, false otherwise + * Returns true if the given state is isRequestPendingStale, isResponsePendingStale, SuccessStale or + * ErrorStale, false otherwise */ export const isStale = (state: RequestEntryState) => - isSuccessStale(state) || isErrorStale(state); + isResponsePendingStale(state) || + isSuccessStale(state) || + isErrorStale(state); diff --git a/src/app/core/data/request.reducer.spec.ts b/src/app/core/data/request.reducer.spec.ts index 05f074a96a7..86b9c4cd5dc 100644 --- a/src/app/core/data/request.reducer.spec.ts +++ b/src/app/core/data/request.reducer.spec.ts @@ -48,9 +48,16 @@ describe('requestReducer', () => { lastUpdated: 0 } }; + const testResponsePendingState = { + [id1]: { + state: RequestEntryState.ResponsePending, + lastUpdated: 0 + } + }; deepFreeze(testInitState); deepFreeze(testSuccessState); deepFreeze(testErrorState); + deepFreeze(testResponsePendingState); it('should return the current state when no valid actions have been made', () => { const action = new NullAction(); @@ -91,29 +98,94 @@ describe('requestReducer', () => { expect(newState[id1].response).toEqual(undefined); }); - it('should set state to Success for the given RestRequest in the state, in response to a SUCCESS action', () => { - const state = testInitState; + describe(`in response to a SUCCESS action`, () => { + let startState; + describe(`when the entry isn't stale`, () => { + beforeEach(() => { + startState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.ResponsePending + }) + }); + deepFreeze(startState); + }); + it('should set state to Success for the given RestRequest in the state', () => { + const action = new RequestSuccessAction(id1, 200); + const newState = requestReducer(startState, action); - const action = new RequestSuccessAction(id1, 200); - const newState = requestReducer(state, action); + expect(newState[id1].request.uuid).toEqual(id1); + expect(newState[id1].request.href).toEqual(link1); + expect(newState[id1].state).toEqual(RequestEntryState.Success); + expect(newState[id1].response.statusCode).toEqual(200); + }); + }); + + describe(`when the entry is stale`, () => { + beforeEach(() => { + startState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.ResponsePendingStale + }) + }); + deepFreeze(startState); + }); + it('should set state to SuccessStale for the given RestRequest in the state', () => { + const action = new RequestSuccessAction(id1, 200); + const newState = requestReducer(startState, action); + + expect(newState[id1].request.uuid).toEqual(id1); + expect(newState[id1].request.href).toEqual(link1); + expect(newState[id1].state).toEqual(RequestEntryState.SuccessStale); + expect(newState[id1].response.statusCode).toEqual(200); + }); + }); - expect(newState[id1].request.uuid).toEqual(id1); - expect(newState[id1].request.href).toEqual(link1); - expect(newState[id1].state).toEqual(RequestEntryState.Success); - expect(newState[id1].response.statusCode).toEqual(200); }); - it('should set state to Error for the given RestRequest in the state, in response to an ERROR action', () => { - const state = testInitState; + describe(`in response to an ERROR action`, () => { + let startState; + describe(`when the entry isn't stale`, () => { + beforeEach(() => { + startState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.ResponsePending + }) + }); + deepFreeze(startState); + }); + it('should set state to Error for the given RestRequest in the state', () => { + const action = new RequestErrorAction(id1, 404, 'Not Found'); + const newState = requestReducer(startState, action); - const action = new RequestErrorAction(id1, 404, 'Not Found'); - const newState = requestReducer(state, action); + expect(newState[id1].request.uuid).toEqual(id1); + expect(newState[id1].request.href).toEqual(link1); + expect(newState[id1].state).toEqual(RequestEntryState.Error); + expect(newState[id1].response.statusCode).toEqual(404); + expect(newState[id1].response.errorMessage).toEqual('Not Found'); + }); + }); - expect(newState[id1].request.uuid).toEqual(id1); - expect(newState[id1].request.href).toEqual(link1); - expect(newState[id1].state).toEqual(RequestEntryState.Error); - expect(newState[id1].response.statusCode).toEqual(404); - expect(newState[id1].response.errorMessage).toEqual('Not Found'); + describe(`when the entry is stale`, () => { + beforeEach(() => { + startState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.ResponsePendingStale + }) + }); + deepFreeze(startState); + }); + it('should set state to ErrorStale for the given RestRequest in the state', () => { + const action = new RequestErrorAction(id1, 404, 'Not Found'); + const newState = requestReducer(startState, action); + + expect(newState[id1].request.uuid).toEqual(id1); + expect(newState[id1].request.href).toEqual(link1); + expect(newState[id1].state).toEqual(RequestEntryState.ErrorStale); + expect(newState[id1].response.statusCode).toEqual(404); + expect(newState[id1].response.errorMessage).toEqual('Not Found'); + }); + + }); }); it('should update the response\'s timeCompleted for the given RestRequest in the state, in response to a RESET_TIMESTAMPS action', () => { @@ -145,28 +217,112 @@ describe('requestReducer', () => { expect(newState[id1]).toBeNull(); }); - describe(`for an entry with state: Success`, () => { - it(`should set the state to SuccessStale, in response to a STALE action`, () => { - const state = testSuccessState; + describe(`in response to a STALE action`, () => { + describe(`when the entry has been removed`, () => { + it(`shouldn't do anything`, () => { + const startState = { + [id1]: null + }; + deepFreeze(startState); - const action = new RequestStaleAction(id1); - const newState = requestReducer(state, action); + const action = new RequestStaleAction(id1); + const newState = requestReducer(startState, action); - expect(newState[id1].state).toEqual(RequestEntryState.SuccessStale); - expect(newState[id1].lastUpdated).toBe(action.lastUpdated); + expect(newState[id1]).toBeNull(); + }); }); - }); - describe(`for an entry with state: Error`, () => { - it(`should set the state to ErrorStale, in response to a STALE action`, () => { - const state = testErrorState; + describe(`for stale entries`, () => { + it(`shouldn't do anything`, () => { + const rpsStartState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.ResponsePendingStale + }) + }); + deepFreeze(rpsStartState); + + const action = new RequestStaleAction(id1); + let newState = requestReducer(rpsStartState, action); + + expect(newState[id1].state).toEqual(rpsStartState[id1].state); + expect(newState[id1].lastUpdated).toBe(rpsStartState[id1].lastUpdated); - const action = new RequestStaleAction(id1); - const newState = requestReducer(state, action); + const ssStartState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.SuccessStale + }) + }); - expect(newState[id1].state).toEqual(RequestEntryState.ErrorStale); - expect(newState[id1].lastUpdated).toBe(action.lastUpdated); + newState = requestReducer(ssStartState, action); + + expect(newState[id1].state).toEqual(ssStartState[id1].state); + expect(newState[id1].lastUpdated).toBe(ssStartState[id1].lastUpdated); + + const esStartState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.ErrorStale + }) + }); + + newState = requestReducer(esStartState, action); + + expect(newState[id1].state).toEqual(esStartState[id1].state); + expect(newState[id1].lastUpdated).toBe(esStartState[id1].lastUpdated); + + }); }); - }); + describe(`for and entry with state: RequestPending`, () => { + it(`shouldn't do anything`, () => { + const startState = Object.assign({}, testInitState, { + [id1]: Object.assign({}, testInitState[id1], { + state: RequestEntryState.RequestPending + }) + }); + + const action = new RequestStaleAction(id1); + const newState = requestReducer(startState, action); + + expect(newState[id1].state).toEqual(startState[id1].state); + expect(newState[id1].lastUpdated).toBe(startState[id1].lastUpdated); + + }); + }); + + describe(`for an entry with state: ResponsePending`, () => { + it(`should set the state to ResponsePendingStale`, () => { + const state = testResponsePendingState; + + const action = new RequestStaleAction(id1); + const newState = requestReducer(state, action); + + expect(newState[id1].state).toEqual(RequestEntryState.ResponsePendingStale); + expect(newState[id1].lastUpdated).toBe(action.lastUpdated); + }); + }); + + describe(`for an entry with state: Success`, () => { + it(`should set the state to SuccessStale`, () => { + const state = testSuccessState; + + const action = new RequestStaleAction(id1); + const newState = requestReducer(state, action); + + expect(newState[id1].state).toEqual(RequestEntryState.SuccessStale); + expect(newState[id1].lastUpdated).toBe(action.lastUpdated); + }); + }); + + describe(`for an entry with state: Error`, () => { + it(`should set the state to ErrorStale`, () => { + const state = testErrorState; + + const action = new RequestStaleAction(id1); + const newState = requestReducer(state, action); + + expect(newState[id1].state).toEqual(RequestEntryState.ErrorStale); + expect(newState[id1].lastUpdated).toBe(action.lastUpdated); + }); + }); + }); }); diff --git a/src/app/core/data/request.reducer.ts b/src/app/core/data/request.reducer.ts index 9bf17faf8d7..9cf4fee0e2c 100644 --- a/src/app/core/data/request.reducer.ts +++ b/src/app/core/data/request.reducer.ts @@ -11,7 +11,13 @@ import { ResetResponseTimestampsAction } from './request.actions'; import { isNull } from '../../shared/empty.util'; -import { hasSucceeded, isStale, RequestEntryState } from './request-entry-state.model'; +import { + hasSucceeded, + isStale, + RequestEntryState, + isRequestPending, + isResponsePending +} from './request-entry-state.model'; import { RequestState } from './request-state.model'; // Object.create(null) ensures the object has no default js properties (e.g. `__proto__`) @@ -91,14 +97,17 @@ function executeRequest(storeState: RequestState, action: RequestExecuteAction): * the new storeState, with the response added to the request */ function completeSuccessRequest(storeState: RequestState, action: RequestSuccessAction): RequestState { - if (isNull(storeState[action.payload.uuid])) { + const prevEntry = storeState[action.payload.uuid]; + if (isNull(prevEntry)) { // after a request has been removed it's possible pending changes still come in. // Don't store them return storeState; } else { return Object.assign({}, storeState, { - [action.payload.uuid]: Object.assign({}, storeState[action.payload.uuid], { - state: RequestEntryState.Success, + [action.payload.uuid]: Object.assign({}, prevEntry, { + // If a response comes in for a request that's already stale, still store it otherwise + // components that are waiting for it might freeze + state: isStale(prevEntry.state) ? RequestEntryState.SuccessStale : RequestEntryState.Success, response: { timeCompleted: action.payload.timeCompleted, lastUpdated: action.payload.timeCompleted, @@ -124,14 +133,17 @@ function completeSuccessRequest(storeState: RequestState, action: RequestSuccess * the new storeState, with the response added to the request */ function completeFailedRequest(storeState: RequestState, action: RequestErrorAction): RequestState { - if (isNull(storeState[action.payload.uuid])) { + const prevEntry = storeState[action.payload.uuid]; + if (isNull(prevEntry)) { // after a request has been removed it's possible pending changes still come in. // Don't store them return storeState; } else { return Object.assign({}, storeState, { - [action.payload.uuid]: Object.assign({}, storeState[action.payload.uuid], { - state: RequestEntryState.Error, + [action.payload.uuid]: Object.assign({}, prevEntry, { + // If a response comes in for a request that's already stale, still store it otherwise + // components that are waiting for it might freeze + state: isStale(prevEntry.state) ? RequestEntryState.ErrorStale : RequestEntryState.Error, response: { timeCompleted: action.payload.timeCompleted, lastUpdated: action.payload.timeCompleted, @@ -155,22 +167,27 @@ function completeFailedRequest(storeState: RequestState, action: RequestErrorAct * the new storeState, set to stale */ function expireRequest(storeState: RequestState, action: RequestStaleAction): RequestState { - if (isNull(storeState[action.payload.uuid])) { - // after a request has been removed it's possible pending changes still come in. - // Don't store them + const prevEntry = storeState[action.payload.uuid]; + if (isNull(prevEntry) || isStale(prevEntry.state) || isRequestPending(prevEntry.state)) { + // No need to do anything if the entry doesn't exist, is already stale, or if the request is + // still pending, because that means it still needs to be sent to the server. Any response + // is guaranteed to have been generated after the request was set to stale. return storeState; } else { - const prevEntry = storeState[action.payload.uuid]; - if (isStale(prevEntry.state)) { - return storeState; + let nextRequestEntryState: RequestEntryState; + if (isResponsePending(prevEntry.state)) { + nextRequestEntryState = RequestEntryState.ResponsePendingStale; + } else if (hasSucceeded(prevEntry.state)) { + nextRequestEntryState = RequestEntryState.SuccessStale; } else { - return Object.assign({}, storeState, { - [action.payload.uuid]: Object.assign({}, prevEntry, { - state: hasSucceeded(prevEntry.state) ? RequestEntryState.SuccessStale : RequestEntryState.ErrorStale, - lastUpdated: action.lastUpdated - }) - }); + nextRequestEntryState = RequestEntryState.ErrorStale; } + return Object.assign({}, storeState, { + [action.payload.uuid]: Object.assign({}, prevEntry, { + state: nextRequestEntryState, + lastUpdated: action.lastUpdated + }) + }); } } diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index ec633370ce2..9f43c3f5992 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -164,7 +164,7 @@ export class RequestService { this.getByHref(request.href).pipe( take(1)) .subscribe((re: RequestEntry) => { - isPending = (hasValue(re) && isLoading(re.state)); + isPending = (hasValue(re) && isLoading(re.state) && !isStale(re.state)); }); return isPending; } diff --git a/src/app/core/shared/hal-endpoint.service.spec.ts b/src/app/core/shared/hal-endpoint.service.spec.ts index 56e890b3189..b81d0806dfd 100644 --- a/src/app/core/shared/hal-endpoint.service.spec.ts +++ b/src/app/core/shared/hal-endpoint.service.spec.ts @@ -1,4 +1,3 @@ -import { cold, hot } from 'jasmine-marbles'; import { getMockRequestService } from '../../shared/mocks/request.service.mock'; import { RequestService } from '../data/request.service'; import { HALEndpointService } from './hal-endpoint.service'; @@ -7,12 +6,17 @@ import { combineLatest as observableCombineLatest, of as observableOf } from 'rx import { environment } from '../../../environments/environment'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils'; +import { TestScheduler } from 'rxjs/testing'; +import { RemoteData } from '../data/remote-data'; +import { RequestEntryState } from '../data/request-entry-state.model'; describe('HALEndpointService', () => { let service: HALEndpointService; let requestService: RequestService; let rdbService: RemoteDataBuildService; let envConfig; + let testScheduler; + let remoteDataMocks; const endpointMap = { test: { href: 'https://rest.api/test' @@ -68,7 +72,30 @@ describe('HALEndpointService', () => { }; const linkPath = 'test'; + const timeStamp = new Date().getTime(); + const msToLive = 15 * 60 * 1000; + const payload = { + _links: endpointMaps[one] + }; + 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), + ResponsePendingStale: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePendingStale, 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), + }; + beforeEach(() => { + testScheduler = new TestScheduler((actual, expected) => { + // asserting the two objects are equal + // e.g. using chai. + expect(actual).toEqual(expected); + }); requestService = getMockRequestService(); rdbService = jasmine.createSpyObj('rdbService', { buildFromHref: createSuccessfulRemoteDataObject$({ @@ -111,20 +138,28 @@ describe('HALEndpointService', () => { }); it(`should return the endpoint URL for the service's linkPath`, () => { - spyOn(service as any, 'getEndpointAt').and - .returnValue(hot('a-', { a: 'https://rest.api/test' })); - const result = service.getEndpoint(linkPath); - - const expected = cold('(b|)', { b: endpointMap.test.href }); - expect(result).toBeObservable(expected); + testScheduler.run(({ cold, expectObservable }) => { + spyOn(service as any, 'getEndpointAt').and + .returnValue(cold('a-', { a: 'https://rest.api/test' })); + const result = service.getEndpoint(linkPath); + + const expected = '(b|)'; + const values = { + b: endpointMap.test.href + }; + expectObservable(result).toBe(expected, values); + }); }); it('should return undefined for a linkPath that isn\'t in the endpoint map', () => { - spyOn(service as any, 'getEndpointAt').and - .returnValue(hot('a-', { a: undefined })); - const result = service.getEndpoint('unknown'); - const expected = cold('(b|)', { b: undefined }); - expect(result).toBeObservable(expected); + testScheduler.run(({ cold, expectObservable }) => { + spyOn(service as any, 'getEndpointAt').and + .returnValue(cold('a-', { a: undefined })); + const result = service.getEndpoint('unknown'); + const expected = '(b|)'; + const values = { b: undefined }; + expectObservable(result).toBe(expected, values); + }); }); }); @@ -183,29 +218,118 @@ describe('HALEndpointService', () => { }); it('should return undefined as long as getRootEndpointMap hasn\'t fired', () => { - spyOn(service as any, 'getRootEndpointMap').and - .returnValue(hot('----')); - - const result = service.isEnabledOnRestApi(linkPath); - const expected = cold('b---', { b: undefined }); - expect(result).toBeObservable(expected); + testScheduler.run(({ cold, expectObservable }) => { + spyOn(service as any, 'getRootEndpointMap').and + .returnValue(cold('----')); + + const result = service.isEnabledOnRestApi(linkPath); + const expected = 'b---'; + const values = { b: undefined }; + expectObservable(result).toBe(expected, values); + }); }); it('should return true if the service\'s linkPath is in the endpoint map', () => { - spyOn(service as any, 'getRootEndpointMap').and - .returnValue(hot('--a-', { a: endpointMap })); - const result = service.isEnabledOnRestApi(linkPath); - const expected = cold('b-c-', { b: undefined, c: true }); - expect(result).toBeObservable(expected); + testScheduler.run(({ cold, expectObservable }) => { + spyOn(service as any, 'getRootEndpointMap').and + .returnValue(cold('--a-', { a: endpointMap })); + const result = service.isEnabledOnRestApi(linkPath); + const expected = 'b-c-'; + const values = { b: undefined, c: true }; + expectObservable(result).toBe(expected, values); + }); }); it('should return false if the service\'s linkPath isn\'t in the endpoint map', () => { - spyOn(service as any, 'getRootEndpointMap').and - .returnValue(hot('--a-', { a: endpointMap })); + testScheduler.run(({ cold, expectObservable }) => { + spyOn(service as any, 'getRootEndpointMap').and + .returnValue(cold('--a-', { a: endpointMap })); + + const result = service.isEnabledOnRestApi('unknown'); + const expected = 'b-c-'; + const values = { b: undefined, c: false }; + expectObservable(result).toBe(expected, values); + }); + }); + + }); + + describe(`getEndpointMapAt`, () => { + const href = 'https://rest.api/some/sub/path'; + + it(`should call requestService.send with a new EndpointMapRequest for the given href. useCachedVersionIfAvailable should be true`, () => { + testScheduler.run(() => { + (service as any).getEndpointMapAt(href); + }); + const expected = new EndpointMapRequest(requestService.generateRequestId(), href); + expect(requestService.send).toHaveBeenCalledWith(expected, true); + }); + + it(`should call rdbService.buildFromHref with the given href`, () => { + testScheduler.run(() => { + (service as any).getEndpointMapAt(href); + }); + expect(rdbService.buildFromHref).toHaveBeenCalledWith(href); + }); + + describe(`when the RemoteData returned from rdbService is stale`, () => { + it(`should re-request it`, () => { + spyOn(service as any, 'getEndpointMapAt').and.callThrough(); + testScheduler.run(({ cold }) => { + (rdbService.buildFromHref as jasmine.Spy).and.returnValue(cold('a', { a: remoteDataMocks.ResponsePendingStale })); + // we need to subscribe to the result, to ensure the "tap" that does the re-request can fire + (service as any).getEndpointMapAt(href).subscribe(); + }); + expect((service as any).getEndpointMapAt).toHaveBeenCalledTimes(2); + }); + }); + + describe(`when the RemoteData returned from rdbService isn't stale`, () => { + it(`should not re-request it`, () => { + spyOn(service as any, 'getEndpointMapAt').and.callThrough(); + testScheduler.run(({ cold }) => { + (rdbService.buildFromHref as jasmine.Spy).and.returnValue(cold('a', { a: remoteDataMocks.ResponsePending })); + // we need to subscribe to the result, to ensure the "tap" that does the re-request can fire + (service as any).getEndpointMapAt(href).subscribe(); + }); + expect((service as any).getEndpointMapAt).toHaveBeenCalledTimes(1); + }); + }); - const result = service.isEnabledOnRestApi('unknown'); - const expected = cold('b-c-', { b: undefined, c: false }); - expect(result).toBeObservable(expected); + it(`should emit exactly once, returning the endpoint map in the response, when the RemoteData completes`, () => { + testScheduler.run(({ cold, expectObservable }) => { + (rdbService.buildFromHref as jasmine.Spy).and.returnValue(cold('a-b-c-d-e-f-g-h-i-j-k-l', { + a: remoteDataMocks.RequestPending, + b: remoteDataMocks.ResponsePending, + c: remoteDataMocks.ResponsePendingStale, + d: remoteDataMocks.SuccessStale, + e: remoteDataMocks.RequestPending, + f: remoteDataMocks.ResponsePending, + g: remoteDataMocks.Success, + h: remoteDataMocks.SuccessStale, + i: remoteDataMocks.RequestPending, + k: remoteDataMocks.ResponsePending, + l: remoteDataMocks.Error, + })); + const expected = '------------(g|)'; + const values = { + g: endpointMaps[one] + }; + expectObservable((service as any).getEndpointMapAt(one)).toBe(expected, values); + }); + }); + + it(`should emit undefined when the response doesn't have a payload`, () => { + testScheduler.run(({ cold, expectObservable }) => { + (rdbService.buildFromHref as jasmine.Spy).and.returnValue(cold('a', { + a: remoteDataMocks.Error, + })); + const expected = '(a|)'; + const values = { + g: undefined + }; + expectObservable((service as any).getEndpointMapAt(href)).toBe(expected, values); + }); }); }); diff --git a/src/app/core/shared/hal-endpoint.service.ts b/src/app/core/shared/hal-endpoint.service.ts index 5cdd7ccfad7..07754616c73 100644 --- a/src/app/core/shared/hal-endpoint.service.ts +++ b/src/app/core/shared/hal-endpoint.service.ts @@ -1,11 +1,19 @@ import { Observable } from 'rxjs'; -import { distinctUntilChanged, map, startWith, switchMap, take, skipWhile } from 'rxjs/operators'; +import { + distinctUntilChanged, + map, + startWith, + switchMap, + take, + tap, filter +} from 'rxjs/operators'; import { RequestService } from '../data/request.service'; import { EndpointMapRequest } from '../data/request.models'; import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; import { RESTURLCombiner } from '../url-combiner/rest-url-combiner'; import { Injectable } from '@angular/core'; import { EndpointMap } from '../cache/response.models'; +import { getFirstCompletedRemoteData } from './operators'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { RemoteData } from '../data/remote-data'; import { CacheableObject } from '../cache/cacheable-object.model'; @@ -33,11 +41,16 @@ export class HALEndpointService { this.requestService.send(request, true); return this.rdbService.buildFromHref(href).pipe( - // 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. - skipWhile((rd: RemoteData) => rd.isLoading || rd.isStale), - take(1), + // Re-request stale responses + tap((rd: RemoteData) => { + if (hasValue(rd) && rd.isStale) { + this.getEndpointMapAt(href); + } + }), + // Filter out all stale responses. We're only interested in a single, non-stale, + // completed RemoteData + filter((rd: RemoteData) => !rd.isStale), + getFirstCompletedRemoteData(), map((response: RemoteData) => { if (hasValue(response.payload)) { return response.payload._links;