Skip to content

Commit 5088567

Browse files
tdonohuetdonohue
authored
Merge pull request #3715 from DSpace/backport-3677-to-dspace-8_x
[Port dspace-8_x] Fix infinite loading on item pages and optimize menu resolver usage
2 parents f47bf46 + 970544c commit 5088567

File tree

9 files changed

+87
-45
lines changed

9 files changed

+87
-45
lines changed

‎cypress/e2e/item-edit.cy.ts‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,6 @@ beforeEach(() => {
99

1010
// This page is restricted, so we will be shown the login form. Fill it out & submit.
1111
cy.loginViaForm(Cypress.env('DSPACE_TEST_ADMIN_USER'), Cypress.env('DSPACE_TEST_ADMIN_PASSWORD'));
12-
13-
// We need to wait for the correction types allowed for the item to be loaded to be sure that each tab is fully loaded.
14-
// This because the edit item page causes often tests to fails due to timeout.
15-
cy.intercept('GET', 'server/api/config/correctiontypes/search/findByItem*').as('correctionTypes');
16-
cy.wait('@correctionTypes');
1712
});
1813

1914
describe('Edit Item > Edit Metadata tab', () => {

‎src/app/browse-by/browse-by-page-routes.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { Route } from '@angular/router';
22

3-
import { dsoEditMenuResolver } from '../shared/dso-page/dso-edit-menu.resolver';
43
import { browseByDSOBreadcrumbResolver } from './browse-by-dso-breadcrumb.resolver';
54
import { browseByGuard } from './browse-by-guard';
65
import { browseByI18nBreadcrumbResolver } from './browse-by-i18n-breadcrumb.resolver';
@@ -11,7 +10,6 @@ export const ROUTES: Route[] = [
1110
path: '',
1211
resolve: {
1312
breadcrumb: browseByDSOBreadcrumbResolver,
14-
menu: dsoEditMenuResolver,
1513
},
1614
children: [
1715
{

‎src/app/collection-page/collection-page-routes.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ export const ROUTES: Route[] = [
5454
resolve: {
5555
dso: collectionPageResolver,
5656
breadcrumb: collectionBreadcrumbResolver,
57-
menu: dsoEditMenuResolver,
5857
},
5958
runGuardsAndResolvers: 'always',
6059
children: [
@@ -83,6 +82,9 @@ export const ROUTES: Route[] = [
8382
{
8483
path: '',
8584
component: ThemedCollectionPageComponent,
85+
resolve: {
86+
menu: dsoEditMenuResolver,
87+
},
8688
children: [
8789
{
8890
path: '',

‎src/app/community-page/community-page-routes.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ export const ROUTES: Route[] = [
5151
resolve: {
5252
dso: communityPageResolver,
5353
breadcrumb: communityBreadcrumbResolver,
54-
menu: dsoEditMenuResolver,
5554
},
5655
runGuardsAndResolvers: 'always',
5756
children: [
@@ -70,6 +69,9 @@ export const ROUTES: Route[] = [
7069
{
7170
path: '',
7271
component: ThemedCommunityPageComponent,
72+
resolve: {
73+
menu: dsoEditMenuResolver,
74+
},
7375
children: [
7476
{
7577
path: '',

‎src/app/core/data/base/base-data.service.spec.ts‎

Lines changed: 67 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ describe('BaseDataService', () => {
6565
let selfLink;
6666
let linksToFollow;
6767
let testScheduler;
68+
let remoteDataTimestamp: number;
6869
let remoteDataMocks: { [responseType: string]: RemoteData<any> };
6970
let remoteDataPageMocks: { [responseType: string]: RemoteData<any> };
7071

@@ -85,7 +86,9 @@ describe('BaseDataService', () => {
8586
expect(actual).toEqual(expected);
8687
});
8788

88-
const timeStamp = new Date().getTime();
89+
// The response's lastUpdated equals the time of 60 seconds after the test started, ensuring they are not perceived
90+
// as cached values.
91+
remoteDataTimestamp = new Date().getTime() + 60 * 1000;
8992
const msToLive = 15 * 60 * 1000;
9093
const payload = {
9194
foo: 'bar',
@@ -112,22 +115,22 @@ describe('BaseDataService', () => {
112115
const statusCodeError = 404;
113116
const errorMessage = 'not found';
114117
remoteDataMocks = {
115-
RequestPending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
116-
ResponsePending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
117-
ResponsePendingStale: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined),
118-
Success: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Success, undefined, payload, statusCodeSuccess),
119-
SuccessStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.SuccessStale, undefined, payload, statusCodeSuccess),
120-
Error: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
121-
ErrorStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError),
118+
RequestPending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
119+
ResponsePending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
120+
ResponsePendingStale: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined),
121+
Success: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Success, undefined, payload, statusCodeSuccess),
122+
SuccessStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.SuccessStale, undefined, payload, statusCodeSuccess),
123+
Error: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
124+
ErrorStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError),
122125
};
123126
remoteDataPageMocks = {
124-
RequestPending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
125-
ResponsePending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
126-
ResponsePendingStale: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined),
127-
Success: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Success, undefined, createPaginatedList([payload]), statusCodeSuccess),
128-
SuccessStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.SuccessStale, undefined, createPaginatedList([payload]), statusCodeSuccess),
129-
Error: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
130-
ErrorStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError),
127+
RequestPending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
128+
ResponsePending: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
129+
ResponsePendingStale: new RemoteData(undefined, msToLive, remoteDataTimestamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined),
130+
Success: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Success, undefined, createPaginatedList([payload]), statusCodeSuccess),
131+
SuccessStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.SuccessStale, undefined, createPaginatedList([payload]), statusCodeSuccess),
132+
Error: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
133+
ErrorStale: new RemoteData(remoteDataTimestamp, msToLive, remoteDataTimestamp, RequestEntryState.ErrorStale, errorMessage, undefined, statusCodeError),
131134
};
132135

133136
return new TestService(
@@ -361,11 +364,15 @@ describe('BaseDataService', () => {
361364
spyOn(service as any, 'reRequestStaleRemoteData').and.callFake(() => (source) => source);
362365
});
363366

364-
365-
it(`should not emit a cached completed RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
367+
it('should not emit a cached completed RemoteData', () => {
368+
// Old cached value from 1 minute before the test started
369+
const oldCachedSucceededData: RemoteData<any> = Object.assign({}, remoteDataPageMocks.Success, {
370+
timeCompleted: remoteDataTimestamp - 2 * 60 * 1000,
371+
lastUpdated: remoteDataTimestamp - 2 * 60 * 1000,
372+
} as RemoteData<any>);
366373
testScheduler.run(({ cold, expectObservable }) => {
367374
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
368-
a: remoteDataMocks.Success,
375+
a: oldCachedSucceededData,
369376
b: remoteDataMocks.RequestPending,
370377
c: remoteDataMocks.ResponsePending,
371378
d: remoteDataMocks.Success,
@@ -383,6 +390,22 @@ describe('BaseDataService', () => {
383390
});
384391
});
385392

393+
it('should emit the first completed RemoteData since the request was made', () => {
394+
testScheduler.run(({ cold, expectObservable }) => {
395+
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b', {
396+
a: remoteDataMocks.Success,
397+
b: remoteDataMocks.SuccessStale,
398+
}));
399+
const expected = 'a-b';
400+
const values = {
401+
a: remoteDataMocks.Success,
402+
b: remoteDataMocks.SuccessStale,
403+
};
404+
405+
expectObservable(service.findByHref(selfLink, false, true, ...linksToFollow)).toBe(expected, values);
406+
});
407+
});
408+
386409
it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
387410
testScheduler.run(({ cold, expectObservable }) => {
388411
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', {
@@ -411,17 +434,12 @@ describe('BaseDataService', () => {
411434
it('should link all the followLinks of a cached object by calling addDependency', () => {
412435
spyOn(objectCache, 'addDependency').and.callThrough();
413436
testScheduler.run(({ cold, expectObservable, flush }) => {
414-
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d', {
437+
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a', {
415438
a: remoteDataMocks.Success,
416-
b: remoteDataMocks.RequestPending,
417-
c: remoteDataMocks.ResponsePending,
418-
d: remoteDataMocks.Success,
419439
}));
420-
const expected = '--b-c-d';
440+
const expected = 'a';
421441
const values = {
422-
b: remoteDataMocks.RequestPending,
423-
c: remoteDataMocks.ResponsePending,
424-
d: remoteDataMocks.Success,
442+
a: remoteDataMocks.Success,
425443
};
426444

427445
expectObservable(service.findByHref(selfLink, false, false, ...linksToFollow)).toBe(expected, values);
@@ -570,11 +588,15 @@ describe('BaseDataService', () => {
570588
spyOn(service as any, 'reRequestStaleRemoteData').and.callFake(() => (source) => source);
571589
});
572590

573-
574-
it(`should not emit a cached completed RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
591+
it('should not emit a cached completed RemoteData', () => {
575592
testScheduler.run(({ cold, expectObservable }) => {
593+
// Old cached value from 1 minute before the test started
594+
const oldCachedSucceededData: RemoteData<any> = Object.assign({}, remoteDataPageMocks.Success, {
595+
timeCompleted: remoteDataTimestamp - 2 * 60 * 1000,
596+
lastUpdated: remoteDataTimestamp - 2 * 60 * 1000,
597+
} as RemoteData<any>);
576598
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
577-
a: remoteDataPageMocks.Success,
599+
a: oldCachedSucceededData,
578600
b: remoteDataPageMocks.RequestPending,
579601
c: remoteDataPageMocks.ResponsePending,
580602
d: remoteDataPageMocks.Success,
@@ -592,6 +614,22 @@ describe('BaseDataService', () => {
592614
});
593615
});
594616

617+
it('should emit the first completed RemoteData since the request was made', () => {
618+
testScheduler.run(({ cold, expectObservable }) => {
619+
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b', {
620+
a: remoteDataPageMocks.Success,
621+
b: remoteDataPageMocks.SuccessStale,
622+
}));
623+
const expected = 'a-b';
624+
const values = {
625+
a: remoteDataPageMocks.Success,
626+
b: remoteDataPageMocks.SuccessStale,
627+
};
628+
629+
expectObservable(service.findListByHref(selfLink, findListOptions, false, true, ...linksToFollow)).toBe(expected, values);
630+
});
631+
});
632+
595633
it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
596634
testScheduler.run(({ cold, expectObservable }) => {
597635
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', {

‎src/app/core/data/base/base-data.service.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,14 +285,15 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
285285
map((href: string) => this.buildHrefFromFindOptions(href, {}, [], ...linksToFollow)),
286286
);
287287

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

290291
const response$: Observable<RemoteData<T>> = this.rdbService.buildSingle<T>(requestHref$, ...linksToFollow).pipe(
291292
// This skip ensures that if a stale object is present in the cache when you do a
292293
// call it isn't immediately returned, but we wait until the remote data for the new request
293294
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
294295
// cached completed object
295-
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
296+
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)),
296297
this.reRequestStaleRemoteData(reRequestOnStale, () =>
297298
this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
298299
);
@@ -338,14 +339,15 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
338339
map((href: string) => this.buildHrefFromFindOptions(href, options, [], ...linksToFollow)),
339340
);
340341

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

343345
const response$: Observable<RemoteData<PaginatedList<T>>> = this.rdbService.buildList<T>(requestHref$, ...linksToFollow).pipe(
344346
// This skip ensures that if a stale object is present in the cache when you do a
345347
// call it isn't immediately returned, but we wait until the remote data for the new request
346348
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
347349
// cached completed object
348-
skipWhile((rd: RemoteData<PaginatedList<T>>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
350+
skipWhile((rd: RemoteData<PaginatedList<T>>) => rd.isStale || (!useCachedVersionIfAvailable && rd.lastUpdated < startTime)),
349351
this.reRequestStaleRemoteData(reRequestOnStale, () =>
350352
this.findListByHref(href$, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
351353
);

‎src/app/item-page/edit-item-page/abstract-item-update/abstract-item-update.component.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,9 @@ import {
1616
Subscription,
1717
} from 'rxjs';
1818
import {
19-
first,
2019
map,
2120
switchMap,
21+
take,
2222
tap,
2323
} from 'rxjs/operators';
2424

@@ -102,7 +102,7 @@ export class AbstractItemUpdateComponent extends AbstractTrackableComponent impl
102102
super.ngOnInit();
103103

104104
this.discardTimeOut = environment.item.edit.undoTimeout;
105-
this.hasChanges().pipe(first()).subscribe((hasChanges) => {
105+
this.hasChanges().pipe(take(1)).subscribe((hasChanges) => {
106106
if (!hasChanges) {
107107
this.initializeOriginalFields();
108108
} else {
@@ -187,7 +187,7 @@ export class AbstractItemUpdateComponent extends AbstractTrackableComponent impl
187187
*/
188188
private checkLastModified() {
189189
const currentVersion = this.item.lastModified;
190-
this.objectUpdatesService.getLastModified(this.url).pipe(first()).subscribe(
190+
this.objectUpdatesService.getLastModified(this.url).pipe(take(1)).subscribe(
191191
(updateVersion: Date) => {
192192
if (updateVersion.getDate() !== currentVersion.getDate()) {
193193
this.notificationsService.warning(this.getNotificationTitle('outdated'), this.getNotificationContent('outdated'));

‎src/app/item-page/item-page-routes.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,23 @@ export const ROUTES: Route[] = [
2727
resolve: {
2828
dso: itemPageResolver,
2929
breadcrumb: itemBreadcrumbResolver,
30-
menu: dsoEditMenuResolver,
3130
},
3231
runGuardsAndResolvers: 'always',
3332
children: [
3433
{
3534
path: '',
3635
component: ThemedItemPageComponent,
3736
pathMatch: 'full',
37+
resolve: {
38+
menu: dsoEditMenuResolver,
39+
},
3840
},
3941
{
4042
path: 'full',
4143
component: ThemedFullItemPageComponent,
44+
resolve: {
45+
menu: dsoEditMenuResolver,
46+
},
4247
},
4348
{
4449
path: ITEM_EDIT_PATH,

‎src/app/shared/dso-page/dso-edit-menu-resolver.service.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ export class DSOEditMenuResolverService {
155155
this.dsoVersioningModalService.getVersioningTooltipMessage(dso, 'item.page.version.hasDraft', 'item.page.version.create'),
156156
this.authorizationService.isAuthorized(FeatureID.CanSynchronizeWithORCID, dso.self),
157157
this.authorizationService.isAuthorized(FeatureID.CanClaimItem, dso.self),
158-
this.correctionTypeDataService.findByItem(dso.uuid, false).pipe(
158+
this.correctionTypeDataService.findByItem(dso.uuid, true).pipe(
159159
getFirstCompletedRemoteData(),
160160
getRemoteDataPayload()),
161161
]).pipe(

0 commit comments

Comments
 (0)