From 153a53f1185ffe28790e6ec097dd66abbc9e035d Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Mon, 27 Mar 2023 16:31:26 +0200 Subject: [PATCH 1/5] 100302: Fix issues with live import --- ...bmission-import-external-collection.component.html | 1 - ...ssion-import-external-collection.component.spec.ts | 3 ++- ...submission-import-external-collection.component.ts | 1 + .../submission-import-external.component.ts | 11 +++++++++-- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.html b/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.html index 29c99732c3..6e88e53ad0 100644 --- a/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.html +++ b/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.html @@ -9,7 +9,6 @@ diff --git a/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.spec.ts b/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.spec.ts index 4f3c54b642..94ce4b91ed 100644 --- a/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.spec.ts +++ b/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.spec.ts @@ -64,7 +64,7 @@ describe('SubmissionImportExternalCollectionComponent test suite', () => { compAsAny = null; }); - it('should emit from selectedEvent on selectObject', () => { + it('should emit from selectedEvent on selectObject and set loading to true', () => { spyOn(comp.selectedEvent, 'emit').and.callThrough(); const entry = { @@ -79,6 +79,7 @@ describe('SubmissionImportExternalCollectionComponent test suite', () => { comp.selectObject(entry); expect(comp.selectedEvent.emit).toHaveBeenCalledWith(entry); + expect(comp.loading).toBeTrue(); }); it('should dismiss modal on closeCollectionModal', () => { diff --git a/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.ts b/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.ts index 5fb4e5d406..22430196d4 100644 --- a/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.ts +++ b/src/app/submission/import-external/import-external-collection/submission-import-external-collection.component.ts @@ -38,6 +38,7 @@ export class SubmissionImportExternalCollectionComponent { * This method emits the selected Collection from the 'selectedEvent' variable. */ public selectObject(object: CollectionListEntry): void { + this.loading = true; this.selectedEvent.emit(object); } diff --git a/src/app/submission/import-external/submission-import-external.component.ts b/src/app/submission/import-external/submission-import-external.component.ts index f0a9dca508..8c2c5eca9e 100644 --- a/src/app/submission/import-external/submission-import-external.component.ts +++ b/src/app/submission/import-external/submission-import-external.component.ts @@ -15,7 +15,9 @@ import { Context } from '../../core/shared/context.model'; import { PaginationComponentOptions } from '../../shared/pagination/pagination-component-options.model'; import { RouteService } from '../../core/services/route.service'; import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils'; -import { SubmissionImportExternalPreviewComponent } from './import-external-preview/submission-import-external-preview.component'; +import { + SubmissionImportExternalPreviewComponent +} from './import-external-preview/submission-import-external-preview.component'; import { fadeIn } from '../../shared/animations/fade'; import { PageInfo } from '../../core/shared/page-info.model'; import { hasValue, isNotEmpty } from '../../shared/empty.util'; @@ -186,12 +188,17 @@ export class SubmissionImportExternalComponent implements OnInit, OnDestroy { this.retrieveExternalSourcesSub = this.reload$.pipe( filter((sourceQueryObject: ExternalSourceData) => isNotEmpty(sourceQueryObject.sourceId) && isNotEmpty(sourceQueryObject.query)), switchMap((sourceQueryObject: ExternalSourceData) => { + const currentEntry = this.entriesRD$.getValue(); + let useCache = true; + if (hasValue(currentEntry) && currentEntry.isError) { + useCache = false; + } const query = sourceQueryObject.query; this.routeData = sourceQueryObject; return this.searchConfigService.paginatedSearchOptions.pipe( tap(() => this.isLoading$.next(true)), filter((searchOptions) => searchOptions.query === query), - mergeMap((searchOptions) => this.externalService.getExternalSourceEntries(this.routeData.sourceId, searchOptions).pipe( + mergeMap((searchOptions) => this.externalService.getExternalSourceEntries(this.routeData.sourceId, searchOptions, useCache).pipe( getFinishedRemoteData(), )) ); From 9f6616a5ce94202c10f33cdf6d33eb3b0dbb8697 Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Tue, 28 Mar 2023 14:38:44 +0200 Subject: [PATCH 2/5] 100302: Add support to check if a request has a cached value --- .../core/data/base/base-data.service.spec.ts | 56 +++++++++++++++++++ src/app/core/data/base/base-data.service.ts | 31 ++++++++++ .../data/external-source-data.service.spec.ts | 42 +++++++++++--- .../core/data/external-source-data.service.ts | 7 ++- src/app/shared/mocks/request.service.mock.ts | 3 +- .../submission-import-external.component.ts | 7 +-- 6 files changed, 129 insertions(+), 17 deletions(-) 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 17532f477a..098f075c10 100644 --- a/src/app/core/data/base/base-data.service.spec.ts +++ b/src/app/core/data/base/base-data.service.spec.ts @@ -641,6 +641,62 @@ describe('BaseDataService', () => { }); }); + describe('hasCachedResponse', () => { + it('should return false when the request will be dispatched', (done) => { + const result = service.hasCachedResponse('test-href'); + + result.subscribe((hasCachedResponse) => { + expect(hasCachedResponse).toBeFalse(); + done(); + }); + }); + + it('should return true when the request will not be dispatched', (done) => { + (requestService.shouldDispatchRequest as jasmine.Spy).and.returnValue(false); + const result = service.hasCachedResponse('test-href'); + + result.subscribe((hasCachedResponse) => { + expect(hasCachedResponse).toBeTrue(); + done(); + }); + }); + }); + + describe('hasCachedErrorResponse', () => { + it('should return false when no response is cached', (done) => { + spyOn(service,'hasCachedResponse').and.returnValue(observableOf(false)); + const result = service.hasCachedErrorResponse('test-href'); + + result.subscribe((hasCachedErrorResponse) => { + expect(hasCachedErrorResponse).toBeFalse(); + done(); + }); + }); + it('should return false when no error response is cached', (done) => { + spyOn(service,'hasCachedResponse').and.returnValue(observableOf(true)); + spyOn(rdbService,'buildSingle').and.returnValue(createSuccessfulRemoteDataObject$({})); + + const result = service.hasCachedErrorResponse('test-href'); + + result.subscribe((hasCachedErrorResponse) => { + expect(hasCachedErrorResponse).toBeFalse(); + done(); + }); + }); + + it('should return true when an error response is cached', (done) => { + spyOn(service,'hasCachedResponse').and.returnValue(observableOf(true)); + spyOn(rdbService,'buildSingle').and.returnValue(createFailedRemoteDataObject$()); + + const result = service.hasCachedErrorResponse('test-href'); + + result.subscribe((hasCachedErrorResponse) => { + expect(hasCachedErrorResponse).toBeTrue(); + done(); + }); + }); + }); + describe('addDependency', () => { let addDependencySpy; diff --git a/src/app/core/data/base/base-data.service.ts b/src/app/core/data/base/base-data.service.ts index 85603580a4..e373c8cfac 100644 --- a/src/app/core/data/base/base-data.service.ts +++ b/src/app/core/data/base/base-data.service.ts @@ -341,6 +341,37 @@ export class BaseDataService implements HALDataServic } } + hasCachedResponse(href$: string | Observable): Observable { + if (isNotEmpty(href$)) { + if (typeof href$ === 'string') { + href$ = observableOf(href$); + } + return href$.pipe( + isNotEmptyOperator(), + take(1), + map((href: string) => { + const requestId = this.requestService.generateRequestId(); + const request = new GetRequest(requestId, href); + return !this.requestService.shouldDispatchRequest(request, true); + }), + ); + } + } + + hasCachedErrorResponse(href$: string | Observable): Observable { + return this.hasCachedResponse(href$).pipe( + switchMap((hasCachedResponse) => { + if (hasCachedResponse) { + return this.rdbService.buildSingle(href$).pipe( + getFirstCompletedRemoteData(), + map((rd => rd.isError || rd.isErrorStale)) + ); + } + return observableOf(false); + }) + ); + } + /** * Return the links to traverse from the root of the api to the * endpoint this DataService represents diff --git a/src/app/core/data/external-source-data.service.spec.ts b/src/app/core/data/external-source-data.service.spec.ts index cdbdbaa006..723d7f9bed 100644 --- a/src/app/core/data/external-source-data.service.spec.ts +++ b/src/app/core/data/external-source-data.service.spec.ts @@ -5,6 +5,7 @@ import { ExternalSourceEntry } from '../shared/external-source-entry.model'; import { of as observableOf } from 'rxjs'; import { GetRequest } from './request.models'; import { testSearchDataImplementation } from './base/search-data.spec'; +import { take } from 'rxjs/operators'; describe('ExternalSourceService', () => { let service: ExternalSourceDataService; @@ -64,19 +65,42 @@ describe('ExternalSourceService', () => { }); describe('getExternalSourceEntries', () => { - let result; - beforeEach(() => { - result = service.getExternalSourceEntries('test'); + describe('when no error response is cached', () => { + let result; + beforeEach(() => { + spyOn(service, 'hasCachedErrorResponse').and.returnValue(observableOf(false)); + result = service.getExternalSourceEntries('test'); + }); + + it('should send a GetRequest', () => { + result.pipe(take(1)).subscribe(); + expect(requestService.send).toHaveBeenCalledWith(jasmine.any(GetRequest), true); + }); + + it('should return the entries', () => { + result.subscribe((resultRD) => { + expect(resultRD.payload.page).toBe(entries); + }); + }); }); - it('should send a GetRequest', () => { - expect(requestService.send).toHaveBeenCalledWith(jasmine.any(GetRequest), true); - }); + describe('when an error response is cached', () => { + let result; + beforeEach(() => { + spyOn(service, 'hasCachedErrorResponse').and.returnValue(observableOf(true)); + result = service.getExternalSourceEntries('test'); + }); - it('should return the entries', () => { - result.subscribe((resultRD) => { - expect(resultRD.payload.page).toBe(entries); + it('should send a GetRequest', () => { + result.pipe(take(1)).subscribe(); + expect(requestService.send).toHaveBeenCalledWith(jasmine.any(GetRequest), false); + }); + + it('should return the entries', () => { + result.subscribe((resultRD) => { + expect(resultRD.payload.page).toBe(entries); + }); }); }); }); diff --git a/src/app/core/data/external-source-data.service.ts b/src/app/core/data/external-source-data.service.ts index c0552aeaec..02c5e4a53c 100644 --- a/src/app/core/data/external-source-data.service.ts +++ b/src/app/core/data/external-source-data.service.ts @@ -74,7 +74,12 @@ export class ExternalSourceDataService extends IdentifiableDataService { + return this.findListByHref(href$, undefined, !hasCachedErrorResponse, reRequestOnStale, ...linksToFollow as any); + }) + ) as any; } /** diff --git a/src/app/shared/mocks/request.service.mock.ts b/src/app/shared/mocks/request.service.mock.ts index bce5b2d466..90db66dcd7 100644 --- a/src/app/shared/mocks/request.service.mock.ts +++ b/src/app/shared/mocks/request.service.mock.ts @@ -14,6 +14,7 @@ export function getMockRequestService(requestEntry$: Observable = removeByHrefSubstring: observableOf(true), setStaleByHrefSubstring: observableOf(true), setStaleByUUID: observableOf(true), - hasByHref$: observableOf(false) + hasByHref$: observableOf(false), + shouldDispatchRequest: true }); } diff --git a/src/app/submission/import-external/submission-import-external.component.ts b/src/app/submission/import-external/submission-import-external.component.ts index 8c2c5eca9e..25b1d5d1aa 100644 --- a/src/app/submission/import-external/submission-import-external.component.ts +++ b/src/app/submission/import-external/submission-import-external.component.ts @@ -188,17 +188,12 @@ export class SubmissionImportExternalComponent implements OnInit, OnDestroy { this.retrieveExternalSourcesSub = this.reload$.pipe( filter((sourceQueryObject: ExternalSourceData) => isNotEmpty(sourceQueryObject.sourceId) && isNotEmpty(sourceQueryObject.query)), switchMap((sourceQueryObject: ExternalSourceData) => { - const currentEntry = this.entriesRD$.getValue(); - let useCache = true; - if (hasValue(currentEntry) && currentEntry.isError) { - useCache = false; - } const query = sourceQueryObject.query; this.routeData = sourceQueryObject; return this.searchConfigService.paginatedSearchOptions.pipe( tap(() => this.isLoading$.next(true)), filter((searchOptions) => searchOptions.query === query), - mergeMap((searchOptions) => this.externalService.getExternalSourceEntries(this.routeData.sourceId, searchOptions, useCache).pipe( + mergeMap((searchOptions) => this.externalService.getExternalSourceEntries(this.routeData.sourceId, searchOptions).pipe( getFinishedRemoteData(), )) ); From 6c5ae4972ecdb877feecc5e563bb7893191827d6 Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Thu, 30 Mar 2023 16:53:10 +0200 Subject: [PATCH 3/5] 100302: Add an error to prevent piping or subscribing to undefined when no href is present --- src/app/core/data/base/base-data.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/core/data/base/base-data.service.ts b/src/app/core/data/base/base-data.service.ts index e373c8cfac..7363894502 100644 --- a/src/app/core/data/base/base-data.service.ts +++ b/src/app/core/data/base/base-data.service.ts @@ -356,6 +356,7 @@ export class BaseDataService implements HALDataServic }), ); } + throw new Error(`Can't check whether there is a cached response for an empty href$`); } hasCachedErrorResponse(href$: string | Observable): Observable { From 3590582832a405d3e316330f28af7d488e42c9d0 Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Mon, 3 Apr 2023 17:48:30 +0200 Subject: [PATCH 4/5] 100302: Change error check to hasFailed --- src/app/core/data/base/base-data.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/core/data/base/base-data.service.ts b/src/app/core/data/base/base-data.service.ts index 7363894502..544c01a69b 100644 --- a/src/app/core/data/base/base-data.service.ts +++ b/src/app/core/data/base/base-data.service.ts @@ -365,7 +365,7 @@ export class BaseDataService implements HALDataServic if (hasCachedResponse) { return this.rdbService.buildSingle(href$).pipe( getFirstCompletedRemoteData(), - map((rd => rd.isError || rd.isErrorStale)) + map((rd => rd.hasFailed)) ); } return observableOf(false); From 2a9021477805de7edf478456c78301d90e82f12b Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Fri, 28 Apr 2023 14:19:44 +0200 Subject: [PATCH 5/5] Add typedocs --- src/app/core/data/base/base-data.service.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/app/core/data/base/base-data.service.ts b/src/app/core/data/base/base-data.service.ts index 544c01a69b..edd6d9e2a4 100644 --- a/src/app/core/data/base/base-data.service.ts +++ b/src/app/core/data/base/base-data.service.ts @@ -341,6 +341,11 @@ export class BaseDataService implements HALDataServic } } + /** + * Checks for the provided href whether a response is already cached + * @param href$ The url for which to check whether there is a cached response. + * Can be a string or an Observable + */ hasCachedResponse(href$: string | Observable): Observable { if (isNotEmpty(href$)) { if (typeof href$ === 'string') { @@ -359,6 +364,11 @@ export class BaseDataService implements HALDataServic throw new Error(`Can't check whether there is a cached response for an empty href$`); } + /** + * Checks for the provided href whether an ERROR response is currently cached + * @param href$ The url for which to check whether there is a cached ERROR response. + * Can be a string or an Observable + */ hasCachedErrorResponse(href$: string | Observable): Observable { return this.hasCachedResponse(href$).pipe( switchMap((hasCachedResponse) => {