Fix infinite loading on item pages and optimize menu resolver usage

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', () => {

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: [
       {

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: '',

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: '',

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<any> };
   let remoteDataPageMocks: { [responseType: string]: RemoteData<any> };
 
@@ -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<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,
@@ -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<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,
@@ -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', {

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

@@ -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)),
     );
@@ -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)),
     );

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'));

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

@@ -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,

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(