Fix cache invalidation for versioning

Author: ybnd

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

@@ -5,23 +5,23 @@
  *
  * http://www.dspace.org/license/
  */
-import { FindListOptions } from '../find-list-options.model';
 import { getMockRequestService } from '../../../shared/mocks/request.service.mock';
 import { HALEndpointServiceStub } from '../../../shared/testing/hal-endpoint-service.stub';
 import { getMockRemoteDataBuildService } from '../../../shared/mocks/remote-data-build.service.mock';
 import { followLink } from '../../../shared/utils/follow-link-config.model';
 import { TestScheduler } from 'rxjs/testing';
 import { RemoteData } from '../remote-data';
 import { RequestEntryState } from '../request-entry-state.model';
-import { Observable, of as observableOf } from 'rxjs';
+import { of as observableOf } from 'rxjs';
 import { RequestService } from '../request.service';
 import { RemoteDataBuildService } from '../../cache/builders/remote-data-build.service';
 import { HALEndpointService } from '../../shared/hal-endpoint.service';
 import { ObjectCacheService } from '../../cache/object-cache.service';
 import { IdentifiableDataService } from './identifiable-data.service';
 import { EMBED_SEPARATOR } from './base-data.service';
 
-const endpoint = 'https://rest.api/core';
+const base = 'https://rest.api/core';
+const endpoint = 'test';
 
 class TestService extends IdentifiableDataService<any> {
   constructor(
@@ -30,11 +30,7 @@ class TestService extends IdentifiableDataService<any> {
     protected objectCache: ObjectCacheService,
     protected halService: HALEndpointService,
   ) {
-    super(undefined, requestService, rdbService, objectCache, halService);
-  }
-
-  public getBrowseEndpoint(options: FindListOptions = {}, linkPath: string = this.linkPath): Observable<string> {
-    return observableOf(endpoint);
+    super(endpoint, requestService, rdbService, objectCache, halService);
   }
 }
 
@@ -51,7 +47,7 @@ describe('IdentifiableDataService', () => {
 
   function initTestService(): TestService {
     requestService = getMockRequestService();
-    halService = new HALEndpointServiceStub('url') as any;
+    halService = new HALEndpointServiceStub(base) as any;
     rdbService = getMockRemoteDataBuildService();
     objectCache = {
 
@@ -143,4 +139,12 @@ describe('IdentifiableDataService', () => {
       expect(result).toEqual(expected);
     });
   });
+
+  describe('invalidateById', () => {
+    it('should invalidate the correct resource by href', () => {
+      spyOn(service, 'invalidateByHref').and.returnValue(observableOf(true));
+      service.invalidateById('123');
+      expect(service.invalidateByHref).toHaveBeenCalledWith(`${base}/${endpoint}/123`);
+    });
+  });
 });

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

@@ -8,7 +8,7 @@
 import { CacheableObject } from '../../cache/cacheable-object.model';
 import { FollowLinkConfig } from '../../../shared/utils/follow-link-config.model';
 import { Observable } from 'rxjs';
-import { map } from 'rxjs/operators';
+import { map, switchMap, take } from 'rxjs/operators';
 import { RemoteData } from '../remote-data';
 import { BaseDataService } from './base-data.service';
 import { RequestService } from '../request.service';
@@ -80,4 +80,19 @@ export class IdentifiableDataService<T extends CacheableObject> extends BaseData
     return this.getEndpoint().pipe(
       map((endpoint: string) => this.getIDHref(endpoint, resourceID, ...linksToFollow)));
   }
+
+  /**
+   * Invalidate a cached resource by its identifier
+   * @param resourceID  the identifier of the resource to invalidate
+   */
+  invalidateById(resourceID: string): Observable<boolean> {
+    const ok$ = this.getIDHrefObs(resourceID).pipe(
+      take(1),
+      switchMap((href) => this.invalidateByHref(href))
+    );
+
+    ok$.subscribe();
+
+    return ok$;
+  }
 }

src/app/core/data/version-history-data.service.spec.ts

@@ -65,6 +65,10 @@ describe('VersionHistoryDataService', () => {
       },
     },
   });
+  const version1WithDraft = Object.assign(new Version(), {
+    ...version1,
+    versionhistory: createSuccessfulRemoteDataObject$(versionHistoryDraft),
+  });
   const versions = [version1, version2];
   versionHistory.versions = createSuccessfulRemoteDataObject$(createPaginatedList(versions));
   const item1 = Object.assign(new Item(), {
@@ -186,21 +190,18 @@ describe('VersionHistoryDataService', () => {
   });
 
   describe('hasDraftVersion$', () => {
-    beforeEach(waitForAsync(() => {
-      versionService.findByHref.and.returnValue(createSuccessfulRemoteDataObject$<Version>(version1));
-    }));
     it('should return false if draftVersion is false', fakeAsync(() => {
-      versionService.getHistoryFromVersion.and.returnValue(of(versionHistory));
+      versionService.findByHref.and.returnValue(createSuccessfulRemoteDataObject$<Version>(version1));
       service.hasDraftVersion$('href').subscribe((res) => {
         expect(res).toBeFalse();
       });
     }));
+
     it('should return true if draftVersion is true', fakeAsync(() => {
-      versionService.getHistoryFromVersion.and.returnValue(of(versionHistoryDraft));
+      versionService.findByHref.and.returnValue(createSuccessfulRemoteDataObject$<Version>(version1WithDraft));
       service.hasDraftVersion$('href').subscribe((res) => {
         expect(res).toBeTrue();
       });
     }));
   });
-
 });

src/app/core/data/version-history-data.service.ts

@@ -91,14 +91,24 @@ export class VersionHistoryDataService extends IdentifiableDataService<VersionHi
     requestHeaders = requestHeaders.append('Content-Type', 'text/uri-list');
     requestOptions.headers = requestHeaders;
 
-    return this.halService.getEndpoint(this.versionsEndpoint).pipe(
+    const response$ = this.halService.getEndpoint(this.versionsEndpoint).pipe(
       take(1),
       map((endpointUrl: string) => (summary?.length > 0) ? `${endpointUrl}?summary=${summary}` : `${endpointUrl}`),
       map((endpointURL: string) => new PostRequest(this.requestService.generateRequestId(), endpointURL, itemHref, requestOptions)),
       sendRequest(this.requestService),
       switchMap((restRequest: RestRequest) => this.rdbService.buildFromRequestUUID(restRequest.uuid)),
       getFirstCompletedRemoteData()
     ) as Observable<RemoteData<Version>>;
+
+    response$.subscribe((versionRD: RemoteData<Version>) => {
+      // invalidate version history
+      // note: we should do this regardless of whether the request succeeds,
+      //       because it may have failed due to cached data that is out of date
+      this.requestService.setStaleByHrefSubstring(versionRD.payload._links.self.href);
+      this.requestService.setStaleByHrefSubstring(versionRD.payload._links.versionhistory.href);
+    });
+
+    return response$;
   }
 
   /**
@@ -158,14 +168,20 @@ export class VersionHistoryDataService extends IdentifiableDataService<VersionHi
    * @returns `true` if a workspace item exists, `false` otherwise, or `null` if a version history does not exist
    */
   hasDraftVersion$(versionHref: string): Observable<boolean> {
-    return this.versionDataService.findByHref(versionHref, true, true, followLink('versionhistory')).pipe(
+    return this.versionDataService.findByHref(versionHref, false, true, followLink('versionhistory')).pipe(
       getFirstCompletedRemoteData(),
       switchMap((res) => {
         if (res.hasSucceeded && !res.hasNoContent) {
-          return of(res).pipe(
-            getFirstSucceededRemoteDataPayload(),
-            switchMap((version) => this.versionDataService.getHistoryFromVersion(version)),
-            map((versionHistory) => versionHistory ? versionHistory.draftVersion : false),
+          return res.payload.versionhistory.pipe(
+            getFirstCompletedRemoteData(),
+            map((versionHistoryRD) => {
+              if (res.hasSucceeded) {
+                const versionHistory = versionHistoryRD.payload;
+                return versionHistory ? versionHistory.draftVersion : false;
+              } else {
+                return false;
+              }
+            }),
           );
         } else {
           return of(false);

src/app/core/submission/workflowitem-data.service.ts

@@ -13,7 +13,6 @@ import { RemoteData } from '../data/remote-data';
 import { NoContent } from '../shared/NoContent.model';
 import { getFirstCompletedRemoteData } from '../shared/operators';
 import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model';
-import { WorkspaceItem } from './models/workspaceitem.model';
 import { RequestParam } from '../cache/models/request-param.model';
 import { FindListOptions } from '../data/find-list-options.model';
 import { IdentifiableDataService } from '../data/base/identifiable-data.service';
@@ -28,7 +27,6 @@ import { dataService } from '../data/base/data-service.decorator';
 @Injectable()
 @dataService(WorkflowItem.type)
 export class WorkflowItemDataService extends IdentifiableDataService<WorkflowItem> implements SearchData<WorkflowItem>, DeleteData<WorkflowItem> {
-  protected linkPath = 'workflowitems';
   protected searchByItemLinkPath = 'item';
   protected responseMsToLive = 10 * 1000;
 
@@ -42,7 +40,7 @@ export class WorkflowItemDataService extends IdentifiableDataService<WorkflowIte
     protected halService: HALEndpointService,
     protected notificationsService: NotificationsService,
   ) {
-    super('workspaceitems', requestService, rdbService, objectCache, halService);
+    super('workflowitems', requestService, rdbService, objectCache, halService);
 
     this.searchData = new SearchDataImpl(this.linkPath, requestService, rdbService, objectCache, halService, this.responseMsToLive);
     this.deleteData = new DeleteDataImpl(this.linkPath, requestService, rdbService, objectCache, halService, notificationsService, this.responseMsToLive, this.constructIdEndpoint);
@@ -105,7 +103,7 @@ export class WorkflowItemDataService extends IdentifiableDataService<WorkflowIte
    * @param options        The {@link FindListOptions} object
    * @param linksToFollow  List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved
    */
-  public findByItem(uuid: string, useCachedVersionIfAvailable = false, reRequestOnStale = true, options: FindListOptions = {}, ...linksToFollow: FollowLinkConfig<WorkspaceItem>[]): Observable<RemoteData<WorkspaceItem>> {
+  public findByItem(uuid: string, useCachedVersionIfAvailable = false, reRequestOnStale = true, options: FindListOptions = {}, ...linksToFollow: FollowLinkConfig<WorkflowItem>[]): Observable<RemoteData<WorkflowItem>> {
     const findListOptions = new FindListOptions();
     findListOptions.searchParams = [new RequestParam('uuid', encodeURIComponent(uuid))];
     const href$ = this.searchData.getSearchByHref(this.searchByItemLinkPath, findListOptions, ...linksToFollow);
@@ -126,7 +124,7 @@ export class WorkflowItemDataService extends IdentifiableDataService<WorkflowIte
    * @return {Observable<RemoteData<PaginatedList<T>>}
    *    Return an observable that emits response from the server
    */
-  public searchBy(searchMethod: string, options?: FindListOptions, useCachedVersionIfAvailable?: boolean, reRequestOnStale?: boolean, ...linksToFollow: FollowLinkConfig<WorkspaceItem>[]): Observable<RemoteData<PaginatedList<WorkspaceItem>>> {
+  public searchBy(searchMethod: string, options?: FindListOptions, useCachedVersionIfAvailable?: boolean, reRequestOnStale?: boolean, ...linksToFollow: FollowLinkConfig<WorkflowItem>[]): Observable<RemoteData<PaginatedList<WorkflowItem>>> {
     return this.searchData.searchBy(searchMethod, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow);
   }
 

src/app/shared/mydspace-actions/claimed-task/approve/claimed-task-actions-approve.component.spec.ts

@@ -2,7 +2,7 @@ import { ChangeDetectionStrategy, Injector, NO_ERRORS_SCHEMA } from '@angular/co
 import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
 import { By } from '@angular/platform-browser';
 import { TranslateLoader, TranslateModule } from '@ngx-translate/core';
-import { of, of as observableOf } from 'rxjs';
+import { of as observableOf } from 'rxjs';
 
 import { ClaimedTaskActionsApproveComponent } from './claimed-task-actions-approve.component';
 import { TranslateLoaderMock } from '../../../mocks/translate-loader.mock';
@@ -18,6 +18,7 @@ import { Router } from '@angular/router';
 import { RouterStub } from '../../../testing/router.stub';
 import { SearchService } from '../../../../core/shared/search/search.service';
 import { RequestService } from '../../../../core/data/request.service';
+import { WorkflowItemDataService } from '../../../../core/submission/workflowitem-data.service';
 
 let component: ClaimedTaskActionsApproveComponent;
 let fixture: ComponentFixture<ClaimedTaskActionsApproveComponent>;
@@ -27,6 +28,7 @@ const searchService = getMockSearchService();
 const requestService = getMockRequestService();
 
 let mockPoolTaskDataService: PoolTaskDataService;
+let mockWorkflowItemDataService: WorkflowItemDataService;
 
 describe('ClaimedTaskActionsApproveComponent', () => {
   const object = Object.assign(new ClaimedTask(), { id: 'claimed-task-1' });
@@ -36,6 +38,10 @@ describe('ClaimedTaskActionsApproveComponent', () => {
 
   beforeEach(waitForAsync(() => {
     mockPoolTaskDataService = new PoolTaskDataService(null, null, null, null);
+    mockWorkflowItemDataService = jasmine.createSpyObj('WorkflowItemDataService', {
+      'invalidateByHref': observableOf(false),
+    });
+
     TestBed.configureTestingModule({
       imports: [
         TranslateModule.forRoot({
@@ -53,6 +59,7 @@ describe('ClaimedTaskActionsApproveComponent', () => {
         { provide: SearchService, useValue: searchService },
         { provide: RequestService, useValue: requestService },
         { provide: PoolTaskDataService, useValue: mockPoolTaskDataService },
+        { provide: WorkflowItemDataService, useValue: mockWorkflowItemDataService },
       ],
       declarations: [ClaimedTaskActionsApproveComponent],
       schemas: [NO_ERRORS_SCHEMA]
@@ -89,7 +96,7 @@ describe('ClaimedTaskActionsApproveComponent', () => {
 
     beforeEach(() => {
       spyOn(component.processCompleted, 'emit');
-      spyOn(component, 'startActionExecution').and.returnValue(of(null));
+      spyOn(component, 'startActionExecution').and.returnValue(observableOf(null));
 
       expectedBody = {
         [component.option]: 'true'

src/app/shared/mydspace-actions/claimed-task/approve/claimed-task-actions-approve.component.ts

@@ -10,6 +10,7 @@ import { TranslateService } from '@ngx-translate/core';
 import { SearchService } from '../../../../core/shared/search/search.service';
 import { RequestService } from '../../../../core/data/request.service';
 import { ClaimedApprovedTaskSearchResult } from '../../../object-collection/shared/claimed-approved-task-search-result.model';
+import { WorkflowItemDataService } from '../../../../core/submission/workflowitem-data.service';
 
 export const WORKFLOW_TASK_OPTION_APPROVE = 'submit_approve';
 
@@ -28,12 +29,15 @@ export class ClaimedTaskActionsApproveComponent extends ClaimedTaskActionsAbstra
    */
   option = WORKFLOW_TASK_OPTION_APPROVE;
 
-  constructor(protected injector: Injector,
-              protected router: Router,
-              protected notificationsService: NotificationsService,
-              protected translate: TranslateService,
-              protected searchService: SearchService,
-              protected requestService: RequestService) {
+  constructor(
+    protected injector: Injector,
+    protected router: Router,
+    protected notificationsService: NotificationsService,
+    protected translate: TranslateService,
+    protected searchService: SearchService,
+    protected requestService: RequestService,
+    protected workflowItemDataService: WorkflowItemDataService,
+  ) {
     super(injector, router, notificationsService, translate, searchService, requestService);
   }
 
@@ -48,4 +52,13 @@ export class ClaimedTaskActionsApproveComponent extends ClaimedTaskActionsAbstra
     return reloadedObject;
   }
 
+  public handleReloadableActionResponse(result: boolean, dso: DSpaceObject): void {
+    super.handleReloadableActionResponse(result, dso);
+
+    // Item page version table includes labels for workflow Items, determined
+    // based on the result of /workflowitems/search/item?uuid=...
+    // In order for this label to be in sync with the workflow state, we should
+    // invalidate WFIs as they are approved.
+    this.workflowItemDataService.invalidateByHref(this.object?._links.workflowitem?.href);
+  }
 }

src/app/submission/objects/submission-objects.effects.spec.ts

@@ -66,6 +66,8 @@ describe('SubmissionObjectEffects test suite', () => {
   let submissionServiceStub;
   let submissionJsonPatchOperationsServiceStub;
   let submissionObjectDataServiceStub;
+  let workspaceItemDataService;
+
   const collectionId: string = mockSubmissionCollectionId;
   const submissionId: string = mockSubmissionId;
   const submissionDefinitionResponse: any = mockSubmissionDefinitionResponse;
@@ -82,6 +84,10 @@ describe('SubmissionObjectEffects test suite', () => {
 
     submissionServiceStub.hasUnsavedModification.and.returnValue(observableOf(true));
 
+    workspaceItemDataService = jasmine.createSpyObj('WorkspaceItemDataService', {
+      invalidateById: observableOf(true),
+    });
+
     TestBed.configureTestingModule({
       imports: [
         StoreModule.forRoot({}, storeModuleConfig),
@@ -106,6 +112,7 @@ describe('SubmissionObjectEffects test suite', () => {
         { provide: WorkflowItemDataService, useValue: {} },
         { provide: HALEndpointService, useValue: {} },
         { provide: SubmissionObjectDataService, useValue: submissionObjectDataServiceStub },
+        { provide: WorkspaceitemDataService, useValue: workspaceItemDataService },
       ],
     });
 

src/app/submission/objects/submission-objects.effects.ts

@@ -55,6 +55,7 @@ import parseSectionErrorPaths, { SectionErrorPath } from '../utils/parseSectionE
 import { FormState } from '../../shared/form/form.reducer';
 import { SubmissionSectionObject } from './submission-section-object.model';
 import { SubmissionSectionError } from './submission-section-error.model';
+import { WorkspaceitemDataService } from '../../core/submission/workspaceitem-data.service';
 
 @Injectable()
 export class SubmissionObjectEffects {
@@ -258,6 +259,7 @@ export class SubmissionObjectEffects {
   depositSubmissionSuccess$ = createEffect(() => this.actions$.pipe(
     ofType(SubmissionObjectActionTypes.DEPOSIT_SUBMISSION_SUCCESS),
     tap(() => this.notificationsService.success(null, this.translate.get('submission.sections.general.deposit_success_notice'))),
+    tap((action: DepositSubmissionSuccessAction) => this.workspaceItemDataService.invalidateById(action.payload.submissionId)),
     tap(() => this.submissionService.redirectToMyDSpace())), { dispatch: false });
 
   /**
@@ -326,14 +328,17 @@ export class SubmissionObjectEffects {
     ofType(SubmissionObjectActionTypes.DISCARD_SUBMISSION_ERROR),
     tap(() => this.notificationsService.error(null, this.translate.get('submission.sections.general.discard_error_notice')))), { dispatch: false });
 
-  constructor(private actions$: Actions,
+  constructor(
+    private actions$: Actions,
     private notificationsService: NotificationsService,
     private operationsService: SubmissionJsonPatchOperationsService,
     private sectionService: SectionsService,
     private store$: Store<any>,
     private submissionService: SubmissionService,
     private submissionObjectService: SubmissionObjectDataService,
-    private translate: TranslateService) {
+    private translate: TranslateService,
+    private workspaceItemDataService: WorkspaceitemDataService,
+  ) {
   }
 
   /**