From 0f4d71eb58d63c9c73af2e098126437da0f266c4 Mon Sep 17 00:00:00 2001 From: Alexandre Vryghem Date: Tue, 19 Nov 2024 16:35:45 +0100 Subject: [PATCH 1/3] 120109: Fixed BaseDataService not emitting when the request is too fast and the ResponsePending are not emitted --- .../core/data/base/base-data.service.spec.ts | 96 +++++++++++++------ src/app/core/data/base/base-data.service.ts | 6 +- 2 files changed, 71 insertions(+), 31 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 3f44ad5e5ac..fc3704bce56 100644 --- a/src/app/core/data/base/base-data.service.spec.ts +++ b/src/app/core/data/base/base-data.service.spec.ts @@ -65,6 +65,7 @@ describe('BaseDataService', () => { let selfLink; let linksToFollow; let testScheduler; + let remoteDataTimestamp: number; let remoteDataMocks: { [responseType: string]: RemoteData }; let remoteDataPageMocks: { [responseType: string]: RemoteData }; @@ -85,7 +86,9 @@ describe('BaseDataService', () => { expect(actual).toEqual(expected); }); - const timeStamp = new Date().getTime(); + // The response's lastUpdated equals the time of 60 seconds after the test started, ensuring they are not perceived + // as cached values. + remoteDataTimestamp = new Date().getTime() + 60 * 1000; const msToLive = 15 * 60 * 1000; const payload = { foo: 'bar', @@ -112,22 +115,22 @@ describe('BaseDataService', () => { 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), + RequestPending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.RequestPending, undefined, undefined, undefined), + ResponsePending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePending, undefined, undefined, undefined), + ResponsePendingStale: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined), + Success: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Success, undefined, payload, statusCodeSuccess), + SuccessStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.SuccessStale, undefined, payload, statusCodeSuccess), + Error: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError), + ErrorStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError), }; remoteDataPageMocks = { - 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, createPaginatedList([payload]), statusCodeSuccess), - SuccessStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.SuccessStale, undefined, createPaginatedList([payload]), statusCodeSuccess), - Error: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError), - ErrorStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError), + RequestPending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.RequestPending, undefined, undefined, undefined), + ResponsePending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePending, undefined, undefined, undefined), + ResponsePendingStale: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined), + Success: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Success, undefined, createPaginatedList([payload]), statusCodeSuccess), + SuccessStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.SuccessStale, undefined, createPaginatedList([payload]), statusCodeSuccess), + Error: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError), + ErrorStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError), }; return new TestService( @@ -361,11 +364,15 @@ describe('BaseDataService', () => { 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`, () => { + it('should not emit a cached completed RemoteData', () => { + // Old cached value from 1 minute before the test started + const oldCachedSucceededData: RemoteData = Object.assign({}, remoteDataPageMocks.Success, { + timeCompleted: remoteDataTimestamp - 2 * 60 * 1000, + lastUpdated: remoteDataTimestamp - 2 * 60 * 1000, + } as RemoteData); testScheduler.run(({ cold, expectObservable }) => { spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', { - a: remoteDataMocks.Success, + a: oldCachedSucceededData, b: remoteDataMocks.RequestPending, c: remoteDataMocks.ResponsePending, d: remoteDataMocks.Success, @@ -383,6 +390,22 @@ describe('BaseDataService', () => { }); }); + it('should emit the first completed RemoteData since the request was made', () => { + testScheduler.run(({ cold, expectObservable }) => { + spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b', { + a: remoteDataMocks.Success, + b: remoteDataMocks.SuccessStale, + })); + const expected = 'a-b'; + const values = { + a: remoteDataMocks.Success, + b: 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-f-g', { @@ -411,17 +434,12 @@ describe('BaseDataService', () => { it('should link all the followLinks of a cached object by calling addDependency', () => { spyOn(objectCache, 'addDependency').and.callThrough(); testScheduler.run(({ cold, expectObservable, flush }) => { - spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d', { + spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', { a: remoteDataMocks.Success, - b: remoteDataMocks.RequestPending, - c: remoteDataMocks.ResponsePending, - d: remoteDataMocks.Success, })); - const expected = '--b-c-d'; + const expected = 'a'; const values = { - b: remoteDataMocks.RequestPending, - c: remoteDataMocks.ResponsePending, - d: remoteDataMocks.Success, + a: remoteDataMocks.Success, }; expectObservable(service.findByHref(selfLink, false, false, ...linksToFollow)).toBe(expected, values); @@ -570,11 +588,15 @@ describe('BaseDataService', () => { 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`, () => { + it('should not emit a cached completed RemoteData', () => { testScheduler.run(({ cold, expectObservable }) => { + // Old cached value from 1 minute before the test started + const oldCachedSucceededData: RemoteData = Object.assign({}, remoteDataPageMocks.Success, { + timeCompleted: remoteDataTimestamp - 2 * 60 * 1000, + lastUpdated: remoteDataTimestamp - 2 * 60 * 1000, + } as RemoteData); spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', { - a: remoteDataPageMocks.Success, + a: oldCachedSucceededData, b: remoteDataPageMocks.RequestPending, c: remoteDataPageMocks.ResponsePending, d: remoteDataPageMocks.Success, @@ -592,6 +614,22 @@ describe('BaseDataService', () => { }); }); + it('should emit the first completed RemoteData since the request was made', () => { + testScheduler.run(({ cold, expectObservable }) => { + spyOn(rdbService, 'buildList').and.returnValue(cold('a-b', { + a: remoteDataPageMocks.Success, + b: remoteDataPageMocks.SuccessStale, + })); + const expected = 'a-b'; + const values = { + a: remoteDataPageMocks.Success, + b: remoteDataPageMocks.SuccessStale, + }; + + expectObservable(service.findListByHref(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-f-g', { diff --git a/src/app/core/data/base/base-data.service.ts b/src/app/core/data/base/base-data.service.ts index d09ee21ee03..979379e95b2 100644 --- a/src/app/core/data/base/base-data.service.ts +++ b/src/app/core/data/base/base-data.service.ts @@ -285,6 +285,7 @@ export class BaseDataService implements HALDataServic map((href: string) => this.buildHrefFromFindOptions(href, {}, [], ...linksToFollow)), ); + const startTime: number = new Date().getTime(); this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable); const response$: Observable> = this.rdbService.buildSingle(requestHref$, ...linksToFollow).pipe( @@ -292,7 +293,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) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)), + skipWhile((rd: RemoteData) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)), this.reRequestStaleRemoteData(reRequestOnStale, () => this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)), ); @@ -338,6 +339,7 @@ export class BaseDataService implements HALDataServic map((href: string) => this.buildHrefFromFindOptions(href, options, [], ...linksToFollow)), ); + const startTime: number = new Date().getTime(); this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable); const response$: Observable>> = this.rdbService.buildList(requestHref$, ...linksToFollow).pipe( @@ -345,7 +347,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>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)), + skipWhile((rd: RemoteData>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)), this.reRequestStaleRemoteData(reRequestOnStale, () => this.findListByHref(href$, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)), ); From decacec40437e891c672d1842598dfabd55f204a Mon Sep 17 00:00:00 2001 From: Alexandre Vryghem Date: Tue, 19 Nov 2024 18:14:12 +0100 Subject: [PATCH 2/3] 120109: Fixed "no elements in sequence" sometimes being thrown on the item bitstream & relationship tabs --- .../abstract-item-update/abstract-item-update.component.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/item-page/edit-item-page/abstract-item-update/abstract-item-update.component.ts b/src/app/item-page/edit-item-page/abstract-item-update/abstract-item-update.component.ts index 1e963a5cca2..254c44ad467 100644 --- a/src/app/item-page/edit-item-page/abstract-item-update/abstract-item-update.component.ts +++ b/src/app/item-page/edit-item-page/abstract-item-update/abstract-item-update.component.ts @@ -16,9 +16,9 @@ import { Subscription, } from 'rxjs'; import { - first, map, switchMap, + take, tap, } from 'rxjs/operators'; @@ -102,7 +102,7 @@ export class AbstractItemUpdateComponent extends AbstractTrackableComponent impl super.ngOnInit(); this.discardTimeOut = environment.item.edit.undoTimeout; - this.hasChanges().pipe(first()).subscribe((hasChanges) => { + this.hasChanges().pipe(take(1)).subscribe((hasChanges) => { if (!hasChanges) { this.initializeOriginalFields(); } else { @@ -187,7 +187,7 @@ export class AbstractItemUpdateComponent extends AbstractTrackableComponent impl */ private checkLastModified() { const currentVersion = this.item.lastModified; - this.objectUpdatesService.getLastModified(this.url).pipe(first()).subscribe( + this.objectUpdatesService.getLastModified(this.url).pipe(take(1)).subscribe( (updateVersion: Date) => { if (updateVersion.getDate() !== currentVersion.getDate()) { this.notificationsService.warning(this.getNotificationTitle('outdated'), this.getNotificationContent('outdated')); From 5c9f494f7692ae7d664cf89d970c330528878abe Mon Sep 17 00:00:00 2001 From: Alexandre Vryghem Date: Tue, 19 Nov 2024 18:45:16 +0100 Subject: [PATCH 3/3] 120109: Updated the route configuration to only resolve the dsoEditMenuResolver on pages who use the DsoEditMenuComponent --- cypress/e2e/item-edit.cy.ts | 5 ----- src/app/browse-by/browse-by-page-routes.ts | 2 -- src/app/collection-page/collection-page-routes.ts | 4 +++- src/app/community-page/community-page-routes.ts | 4 +++- src/app/item-page/item-page-routes.ts | 7 ++++++- src/app/shared/dso-page/dso-edit-menu-resolver.service.ts | 2 +- 6 files changed, 13 insertions(+), 11 deletions(-) diff --git a/cypress/e2e/item-edit.cy.ts b/cypress/e2e/item-edit.cy.ts index 45131baace2..604aab9d042 100644 --- a/cypress/e2e/item-edit.cy.ts +++ b/cypress/e2e/item-edit.cy.ts @@ -9,11 +9,6 @@ beforeEach(() => { // This page is restricted, so we will be shown the login form. Fill it out & submit. cy.loginViaForm(Cypress.env('DSPACE_TEST_ADMIN_USER'), Cypress.env('DSPACE_TEST_ADMIN_PASSWORD')); - - // We need to wait for the correction types allowed for the item to be loaded to be sure that each tab is fully loaded. - // This because the edit item page causes often tests to fails due to timeout. - cy.intercept('GET', 'server/api/config/correctiontypes/search/findByItem*').as('correctionTypes'); - cy.wait('@correctionTypes'); }); describe('Edit Item > Edit Metadata tab', () => { diff --git a/src/app/browse-by/browse-by-page-routes.ts b/src/app/browse-by/browse-by-page-routes.ts index 9c7e16ab39d..3843a50f6e0 100644 --- a/src/app/browse-by/browse-by-page-routes.ts +++ b/src/app/browse-by/browse-by-page-routes.ts @@ -1,6 +1,5 @@ import { Route } from '@angular/router'; -import { dsoEditMenuResolver } from '../shared/dso-page/dso-edit-menu.resolver'; import { browseByDSOBreadcrumbResolver } from './browse-by-dso-breadcrumb.resolver'; import { browseByGuard } from './browse-by-guard'; import { browseByI18nBreadcrumbResolver } from './browse-by-i18n-breadcrumb.resolver'; @@ -11,7 +10,6 @@ export const ROUTES: Route[] = [ path: '', resolve: { breadcrumb: browseByDSOBreadcrumbResolver, - menu: dsoEditMenuResolver, }, children: [ { diff --git a/src/app/collection-page/collection-page-routes.ts b/src/app/collection-page/collection-page-routes.ts index f2dadc3fbe0..e20e3ba8af1 100644 --- a/src/app/collection-page/collection-page-routes.ts +++ b/src/app/collection-page/collection-page-routes.ts @@ -54,7 +54,6 @@ export const ROUTES: Route[] = [ resolve: { dso: collectionPageResolver, breadcrumb: collectionBreadcrumbResolver, - menu: dsoEditMenuResolver, }, runGuardsAndResolvers: 'always', children: [ @@ -83,6 +82,9 @@ export const ROUTES: Route[] = [ { path: '', component: ThemedCollectionPageComponent, + resolve: { + menu: dsoEditMenuResolver, + }, children: [ { path: '', diff --git a/src/app/community-page/community-page-routes.ts b/src/app/community-page/community-page-routes.ts index d9505c53b13..2c8a7942a4a 100644 --- a/src/app/community-page/community-page-routes.ts +++ b/src/app/community-page/community-page-routes.ts @@ -51,7 +51,6 @@ export const ROUTES: Route[] = [ resolve: { dso: communityPageResolver, breadcrumb: communityBreadcrumbResolver, - menu: dsoEditMenuResolver, }, runGuardsAndResolvers: 'always', children: [ @@ -70,6 +69,9 @@ export const ROUTES: Route[] = [ { path: '', component: ThemedCommunityPageComponent, + resolve: { + menu: dsoEditMenuResolver, + }, children: [ { path: '', diff --git a/src/app/item-page/item-page-routes.ts b/src/app/item-page/item-page-routes.ts index 684ea564598..854d66fabe4 100644 --- a/src/app/item-page/item-page-routes.ts +++ b/src/app/item-page/item-page-routes.ts @@ -27,7 +27,6 @@ export const ROUTES: Route[] = [ resolve: { dso: itemPageResolver, breadcrumb: itemBreadcrumbResolver, - menu: dsoEditMenuResolver, }, runGuardsAndResolvers: 'always', children: [ @@ -35,10 +34,16 @@ export const ROUTES: Route[] = [ path: '', component: ThemedItemPageComponent, pathMatch: 'full', + resolve: { + menu: dsoEditMenuResolver, + }, }, { path: 'full', component: ThemedFullItemPageComponent, + resolve: { + menu: dsoEditMenuResolver, + }, }, { path: ITEM_EDIT_PATH, diff --git a/src/app/shared/dso-page/dso-edit-menu-resolver.service.ts b/src/app/shared/dso-page/dso-edit-menu-resolver.service.ts index e6bbeac619c..8c4fd15b7e5 100644 --- a/src/app/shared/dso-page/dso-edit-menu-resolver.service.ts +++ b/src/app/shared/dso-page/dso-edit-menu-resolver.service.ts @@ -155,7 +155,7 @@ export class DSOEditMenuResolverService { this.dsoVersioningModalService.getVersioningTooltipMessage(dso, 'item.page.version.hasDraft', 'item.page.version.create'), this.authorizationService.isAuthorized(FeatureID.CanSynchronizeWithORCID, dso.self), this.authorizationService.isAuthorized(FeatureID.CanClaimItem, dso.self), - this.correctionTypeDataService.findByItem(dso.uuid, false).pipe( + this.correctionTypeDataService.findByItem(dso.uuid, true).pipe( getFirstCompletedRemoteData(), getRemoteDataPayload()), ]).pipe(