Skip to content

Commit

Permalink
Merge pull request DSpace#3677 from alexandrevryghem/w2p-120109_fix-f…
Browse files Browse the repository at this point in the history 
…indByHref-and-findListByHref-skipping-their-response

Fix infinite loading on item pages and optimize menu resolver usage
  • Loading branch information
@tdonohue
tdonohue authored Dec 5, 2024
2 parents 37bd26e + 5c9f494 commit dcea3ba
Show file tree
Hide file tree
Showing 9 changed files with 87 additions and 45 deletions.
5 changes: 0 additions & 5 deletions cypress/e2e/item-edit.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
2 changes: 0 additions & 2 deletions src/app/browse-by/browse-by-page-routes.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -11,7 +10,6 @@ export const ROUTES: Route[] = [
path: '',
resolve: {
breadcrumb: browseByDSOBreadcrumbResolver,
menu: dsoEditMenuResolver,
},
children: [
{
Expand Down
4 changes: 3 additions & 1 deletion src/app/collection-page/collection-page-routes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export const ROUTES: Route[] = [
resolve: {
dso: collectionPageResolver,
breadcrumb: collectionBreadcrumbResolver,
menu: dsoEditMenuResolver,
},
runGuardsAndResolvers: 'always',
children: [
Expand Down Expand Up @@ -83,6 +82,9 @@ export const ROUTES: Route[] = [
{
path: '',
component: ThemedCollectionPageComponent,
resolve: {
menu: dsoEditMenuResolver,
},
children: [
{
path: '',
Expand Down
4 changes: 3 additions & 1 deletion src/app/community-page/community-page-routes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ export const ROUTES: Route[] = [
resolve: {
dso: communityPageResolver,
breadcrumb: communityBreadcrumbResolver,
menu: dsoEditMenuResolver,
},
runGuardsAndResolvers: 'always',
children: [
Expand All @@ -70,6 +69,9 @@ export const ROUTES: Route[] = [
{
path: '',
component: ThemedCommunityPageComponent,
resolve: {
menu: dsoEditMenuResolver,
},
children: [
{
path: '',
Expand Down
96 changes: 67 additions & 29 deletions src/app/core/data/base/base-data.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ describe('BaseDataService', () => {
let selfLink;
let linksToFollow;
let testScheduler;
let remoteDataTimestamp: number;
let remoteDataMocks: { [responseType: string]: RemoteData<any> };
let remoteDataPageMocks: { [responseType: string]: RemoteData<any> };

Expand All @@ -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',
Expand All @@ -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(
Expand Down Expand Up @@ -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<any> = Object.assign({}, remoteDataPageMocks.Success, {
timeCompleted: remoteDataTimestamp - 2 * 60 * 1000,
lastUpdated: remoteDataTimestamp - 2 * 60 * 1000,
} as RemoteData<any>);
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,
Expand All @@ -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', {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<any> = Object.assign({}, remoteDataPageMocks.Success, {
timeCompleted: remoteDataTimestamp - 2 * 60 * 1000,
lastUpdated: remoteDataTimestamp - 2 * 60 * 1000,
} as RemoteData<any>);
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,
Expand All @@ -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', {
Expand Down
6 changes: 4 additions & 2 deletions src/app/core/data/base/base-data.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,14 +285,15 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
map((href: string) => this.buildHrefFromFindOptions(href, {}, [], ...linksToFollow)),
);

const startTime: number = new Date().getTime();
this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable);

const response$: Observable<RemoteData<T>> = this.rdbService.buildSingle<T>(requestHref$, ...linksToFollow).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. If useCachedVersionIfAvailable is false it also ensures you don't get a
// cached completed object
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)),
this.reRequestStaleRemoteData(reRequestOnStale, () =>
this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
);
Expand Down Expand Up @@ -338,14 +339,15 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
map((href: string) => this.buildHrefFromFindOptions(href, options, [], ...linksToFollow)),
);

const startTime: number = new Date().getTime();
this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable);

const response$: Observable<RemoteData<PaginatedList<T>>> = this.rdbService.buildList<T>(requestHref$, ...linksToFollow).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. If useCachedVersionIfAvailable is false it also ensures you don't get a
// cached completed object
skipWhile((rd: RemoteData<PaginatedList<T>>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
skipWhile((rd: RemoteData<PaginatedList<T>>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)),
this.reRequestStaleRemoteData(reRequestOnStale, () =>
this.findListByHref(href$, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
);
Expand Down
6 changes: 3 additions & 3 deletions src/app/item-page/edit-item-page/abstract-item-update/abstract-item-update.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import {
Subscription,
} from 'rxjs';
import {
first,
map,
switchMap,
take,
tap,
} from 'rxjs/operators';

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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'));
Expand Down
7 changes: 6 additions & 1 deletion src/app/item-page/item-page-routes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,23 @@ export const ROUTES: Route[] = [
resolve: {
dso: itemPageResolver,
breadcrumb: itemBreadcrumbResolver,
menu: dsoEditMenuResolver,
},
runGuardsAndResolvers: 'always',
children: [
{
path: '',
component: ThemedItemPageComponent,
pathMatch: 'full',
resolve: {
menu: dsoEditMenuResolver,
},
},
{
path: 'full',
component: ThemedFullItemPageComponent,
resolve: {
menu: dsoEditMenuResolver,
},
},
{
path: ITEM_EDIT_PATH,
Expand Down
2 changes: 1 addition & 1 deletion src/app/shared/dso-page/dso-edit-menu-resolver.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit dcea3ba

Please sign in to comment.