From 6bf086b70748c04268a91c316198be80157a39ca Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 25 Sep 2019 17:37:12 -0700 Subject: [PATCH 01/22] Updated work on routing by id. Fixed unit tests. Updated to use pid REST endpoint. Minor change in data.service and unit test update. Updated the objectnotfound page with new text and go home button. --- .../lookup-by-id-routing.module.ts | 19 +++ src/app/+lookup-by-id/lookup-by-id.module.ts | 23 ++++ src/app/+lookup-by-id/lookup-guard.ts | 52 ++++++++ .../objectnotfound.component.html | 8 ++ .../objectnotfound.component.scss | 0 .../objectnotfound.component.ts | 45 +++++++ src/app/app-routing.module.ts | 2 + src/app/app.module.ts | 2 +- src/app/core/cache/object-cache.reducer.ts | 1 + src/app/core/cache/object-cache.service.ts | 16 +-- src/app/core/core.effects.ts | 4 +- src/app/core/data/comcol-data.service.spec.ts | 4 +- src/app/core/data/comcol-data.service.ts | 2 +- src/app/core/data/data.service.ts | 18 ++- .../data/dso-data-redirect.service.spec.ts | 112 ++++++++++++++++++ .../core/data/dso-data-redirect.service.ts | 78 ++++++++++++ .../data/dspace-object-data.service.spec.ts | 3 +- src/app/core/data/request.models.ts | 6 +- src/app/core/data/request.service.ts | 18 ++- src/app/core/index/index.actions.ts | 14 +-- src/app/core/index/index.effects.ts | 38 ++++-- src/app/core/index/index.reducer.spec.ts | 40 +++++-- src/app/core/index/index.reducer.ts | 41 ++++--- src/app/core/index/index.selectors.ts | 34 ++++-- 24 files changed, 498 insertions(+), 82 deletions(-) create mode 100644 src/app/+lookup-by-id/lookup-by-id-routing.module.ts create mode 100644 src/app/+lookup-by-id/lookup-by-id.module.ts create mode 100644 src/app/+lookup-by-id/lookup-guard.ts create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.scss create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts create mode 100644 src/app/core/data/dso-data-redirect.service.spec.ts create mode 100644 src/app/core/data/dso-data-redirect.service.ts diff --git a/src/app/+lookup-by-id/lookup-by-id-routing.module.ts b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts new file mode 100644 index 0000000000..012345e791 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts @@ -0,0 +1,19 @@ +import { LookupGuard } from './lookup-guard'; +import { NgModule } from '@angular/core'; +import { RouterModule } from '@angular/router'; +import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; + +@NgModule({ + imports: [ + RouterModule.forChild([ + { path: ':idType/:id', canActivate: [LookupGuard], component: ObjectNotFoundComponent } + ]) + ], + providers: [ + LookupGuard + ] +}) + +export class LookupRoutingModule { + +} diff --git a/src/app/+lookup-by-id/lookup-by-id.module.ts b/src/app/+lookup-by-id/lookup-by-id.module.ts new file mode 100644 index 0000000000..4620f57824 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-by-id.module.ts @@ -0,0 +1,23 @@ +import { NgModule } from '@angular/core'; +import { CommonModule } from '@angular/common'; +import { SharedModule } from '../shared/shared.module'; +import { LookupRoutingModule } from './lookup-by-id-routing.module'; +import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; +import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; + +@NgModule({ + imports: [ + LookupRoutingModule, + CommonModule, + SharedModule, + ], + declarations: [ + ObjectNotFoundComponent + ], + providers: [ + DsoDataRedirectService + ] +}) +export class LookupIdModule { + +} diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts new file mode 100644 index 0000000000..61e3688ee2 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -0,0 +1,52 @@ +import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from '@angular/router'; +import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; +import { Injectable } from '@angular/core'; +import { IdentifierType } from '../core/index/index.reducer'; +import { Observable } from 'rxjs'; +import { map } from 'rxjs/operators'; +import { RemoteData } from '../core/data/remote-data'; +import { FindByIDRequest } from '../core/data/request.models'; + +interface LookupParams { + type: IdentifierType; + id: string; +} + +@Injectable() +export class LookupGuard implements CanActivate { + constructor(private dsoService: DsoDataRedirectService, private router: Router) { + } + + canActivate(route: ActivatedRouteSnapshot, state:RouterStateSnapshot): Observable { + const params = this.getLookupParams(route); + return this.dsoService.findById(params.id, params.type).pipe( + map((response: RemoteData) => response.hasFailed) + ); + } + + private getLookupParams(route: ActivatedRouteSnapshot): LookupParams { + let type; + let id; + const idType = route.params.idType; + + // If the idType is not recognized, assume a legacy handle request (handle/prefix/id) + if (idType !== IdentifierType.HANDLE && idType !== IdentifierType.UUID) { + type = IdentifierType.HANDLE; + const prefix = route.params.idType; + const handleId = route.params.id; + id = `${prefix}%2F${handleId}`; + + } else if (route.params.idType === IdentifierType.HANDLE) { + type = IdentifierType.HANDLE; + id = route.params.id; + + } else { + type = IdentifierType.UUID; + id = route.params.id; + } + return { + type: type, + id: id + }; + } +} diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html new file mode 100644 index 0000000000..662d3cde52 --- /dev/null +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html @@ -0,0 +1,8 @@ +
+

{{"error.item" | translate}}

+

{{missingItem}}

+
+

+ {{"404.link.home-page" | translate}} +

+
diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.scss b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.scss new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts new file mode 100644 index 0000000000..0116575154 --- /dev/null +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts @@ -0,0 +1,45 @@ + +import { Component, ChangeDetectionStrategy, OnInit } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; + +/** + * This component representing the `PageNotFound` DSpace page. + */ +@Component({ + selector: 'ds-objnotfound', + styleUrls: ['./objectnotfound.component.scss'], + templateUrl: './objectnotfound.component.html', + changeDetection: ChangeDetectionStrategy.Default +}) +export class ObjectNotFoundComponent implements OnInit { + + idType: string; + + id: string; + + missingItem: string; + + /** + * Initialize instance variables + * + * @param {AuthService} authservice + * @param {ServerResponseService} responseService + */ + constructor(private route: ActivatedRoute) { + route.params.subscribe((params) => { + this.idType = params.idType; + this.id = params.id; + }) + } + + ngOnInit(): void { + if (this.idType.startsWith('handle')) { + this.missingItem = 'handle: ' + this.id; + } else if (this.idType.startsWith('uuid')) { + this.missingItem = 'uuid: ' + this.id; + } else { + this.missingItem = 'handle: ' + this.idType + '/' + this.id; + } + } + +} diff --git a/src/app/app-routing.module.ts b/src/app/app-routing.module.ts index e1ddc2b889..5085633a5b 100644 --- a/src/app/app-routing.module.ts +++ b/src/app/app-routing.module.ts @@ -27,6 +27,8 @@ export function getAdminModulePath() { RouterModule.forRoot([ { path: '', redirectTo: '/home', pathMatch: 'full' }, { path: 'home', loadChildren: './+home-page/home-page.module#HomePageModule' }, + { path: 'id', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, + { path: 'handle', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, { path: COMMUNITY_MODULE_PATH, loadChildren: './+community-page/community-page.module#CommunityPageModule' }, { path: COLLECTION_MODULE_PATH, loadChildren: './+collection-page/collection-page.module#CollectionPageModule' }, { path: ITEM_MODULE_PATH, loadChildren: './+item-page/item-page.module#ItemPageModule' }, diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 916788df8c..3d8bf0ed43 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -128,7 +128,7 @@ const EXPORTS = [ ...PROVIDERS ], declarations: [ - ...DECLARATIONS, + ...DECLARATIONS ], exports: [ ...EXPORTS diff --git a/src/app/core/cache/object-cache.reducer.ts b/src/app/core/cache/object-cache.reducer.ts index f41151fd90..afc040bf59 100644 --- a/src/app/core/cache/object-cache.reducer.ts +++ b/src/app/core/cache/object-cache.reducer.ts @@ -44,6 +44,7 @@ export abstract class TypedObject { */ export class CacheableObject extends TypedObject { uuid?: string; + handle?: string; self: string; // isNew: boolean; // dirtyType: DirtyType; diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 11f3a6ce3e..95e96db0c8 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -4,7 +4,7 @@ import { applyPatch, Operation } from 'fast-json-patch'; import { combineLatest as observableCombineLatest, Observable } from 'rxjs'; import { distinctUntilChanged, filter, map, mergeMap, take, } from 'rxjs/operators'; -import { hasNoValue, hasValue, isNotEmpty } from '../../shared/empty.util'; +import { hasNoValue, isNotEmpty } from '../../shared/empty.util'; import { CoreState } from '../core.reducers'; import { coreSelector } from '../core.selectors'; import { RestRequestMethod } from '../data/rest-request-method'; @@ -21,6 +21,7 @@ import { import { CacheableObject, ObjectCacheEntry, ObjectCacheState } from './object-cache.reducer'; import { AddToSSBAction } from './server-sync-buffer.actions'; import { getMapsToType } from './builders/build-decorators'; +import { IdentifierType } from '../index/index.reducer'; /** * The base selector function to select the object cache in the store @@ -75,14 +76,15 @@ export class ObjectCacheService { /** * Get an observable of the object with the specified UUID * - * @param uuid + * @param id * The UUID of the object to get * @return Observable> * An observable of the requested object in normalized form */ - getObjectByUUID(uuid: string): Observable> { + getObjectByID(id: string, identifierType: IdentifierType = IdentifierType.UUID): + Observable> { return this.store.pipe( - select(selfLinkFromUuidSelector(uuid)), + select(selfLinkFromUuidSelector(id, identifierType)), mergeMap((selfLink: string) => this.getObjectBySelfLink(selfLink) ) ) @@ -188,17 +190,17 @@ export class ObjectCacheService { /** * Check whether the object with the specified UUID is cached * - * @param uuid + * @param id * The UUID of the object to check * @return boolean * true if the object with the specified UUID is cached, * false otherwise */ - hasByUUID(uuid: string): boolean { + hasById(id: string, identifierType: IdentifierType = IdentifierType.UUID): boolean { let result: boolean; this.store.pipe( - select(selfLinkFromUuidSelector(uuid)), + select(selfLinkFromUuidSelector(id, identifierType)), take(1) ).subscribe((selfLink: string) => result = this.hasBySelfLink(selfLink)); diff --git a/src/app/core/core.effects.ts b/src/app/core/core.effects.ts index 0eabfc5dc8..ae6f7f3cfa 100644 --- a/src/app/core/core.effects.ts +++ b/src/app/core/core.effects.ts @@ -1,5 +1,5 @@ import { ObjectCacheEffects } from './cache/object-cache.effects'; -import { UUIDIndexEffects } from './index/index.effects'; +import { IdentifierIndexEffects } from './index/index.effects'; import { RequestEffects } from './data/request.effects'; import { AuthEffects } from './auth/auth.effects'; import { JsonPatchOperationsEffects } from './json-patch/json-patch-operations.effects'; @@ -10,7 +10,7 @@ import { RouteEffects } from './services/route.effects'; export const coreEffects = [ RequestEffects, ObjectCacheEffects, - UUIDIndexEffects, + IdentifierIndexEffects, AuthEffects, JsonPatchOperationsEffects, ServerSyncBufferEffects, diff --git a/src/app/core/data/comcol-data.service.spec.ts b/src/app/core/data/comcol-data.service.spec.ts index b5232b0bff..de17d7a39f 100644 --- a/src/app/core/data/comcol-data.service.spec.ts +++ b/src/app/core/data/comcol-data.service.spec.ts @@ -95,7 +95,7 @@ describe('ComColDataService', () => { function initMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCache', { - getObjectByUUID: cold('d-', { + getObjectByID: cold('d-', { d: { _links: { [LINK_NAME]: scopedEndpoint @@ -160,7 +160,7 @@ describe('ComColDataService', () => { it('should fetch the scope Community from the cache', () => { scheduler.schedule(() => service.getBrowseEndpoint(options).subscribe()); scheduler.flush(); - expect(objectCache.getObjectByUUID).toHaveBeenCalledWith(scopeID); + expect(objectCache.getObjectByID).toHaveBeenCalledWith(scopeID); }); it('should return the endpoint to fetch resources within the given scope', () => { diff --git a/src/app/core/data/comcol-data.service.ts b/src/app/core/data/comcol-data.service.ts index 68eb3e4880..3059d568df 100644 --- a/src/app/core/data/comcol-data.service.ts +++ b/src/app/core/data/comcol-data.service.ts @@ -49,7 +49,7 @@ export abstract class ComColDataService extends DataS ); const successResponses = responses.pipe( filter((response) => response.isSuccessful), - mergeMap(() => this.objectCache.getObjectByUUID(options.scopeID)), + mergeMap(() => this.objectCache.getObjectByID(options.scopeID)), map((nc: NormalizedCommunity) => nc._links[linkPath]), filter((href) => isNotEmpty(href)) ); diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index ad0db51980..0f7ca74d15 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -37,6 +37,7 @@ import { NormalizedObjectBuildService } from '../cache/builders/normalized-objec import { ChangeAnalyzer } from './change-analyzer'; import { RestRequestMethod } from './rest-request-method'; import { getMapsToType } from '../cache/builders/build-decorators'; +import { IdentifierType } from '../index/index.reducer'; export abstract class DataService { protected abstract requestService: RequestService; @@ -146,14 +147,21 @@ export abstract class DataService { return `${endpoint}/${resourceID}`; } - findById(id: string): Observable> { - const hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => this.getIDHref(endpoint, id))); - + findById(id: string, identifierType: IdentifierType = IdentifierType.UUID): Observable> { + let hrefObs; + if (identifierType === IdentifierType.UUID) { + hrefObs = this.halService.getEndpoint(this.linkPath).pipe( + map((endpoint: string) => this.getIDHref(endpoint, id))); + } else if (identifierType === IdentifierType.HANDLE) { + hrefObs = this.halService.getEndpoint(this.linkPath).pipe( + map((endpoint: string) => { + return this.getIDHref(endpoint, encodeURIComponent(id)); + })); + } hrefObs.pipe( find((href: string) => hasValue(href))) .subscribe((href: string) => { - const request = new FindByIDRequest(this.requestService.generateRequestId(), href, id); + const request = new FindByIDRequest(this.requestService.generateRequestId(), href, id, identifierType); this.requestService.configure(request, this.forceBypassCache); }); diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts new file mode 100644 index 0000000000..ece3c242fc --- /dev/null +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -0,0 +1,112 @@ +import { cold, getTestScheduler } from 'jasmine-marbles'; +import { TestScheduler } from 'rxjs/testing'; +import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import { DSpaceObject } from '../shared/dspace-object.model'; +import { HALEndpointService } from '../shared/hal-endpoint.service'; +import { FindByIDRequest } from './request.models'; +import { RequestService } from './request.service'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { HttpClient } from '@angular/common/http'; +import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; +import { IdentifierType } from '../index/index.reducer'; +import { DsoDataRedirectService } from './dso-data-redirect.service'; +import { Router } from '@angular/router'; +import { Store } from '@ngrx/store'; +import { CoreState } from '../core.reducers'; + +describe('DsoDataRedirectService', () => { + let scheduler: TestScheduler; + let service: DsoDataRedirectService; + let halService: HALEndpointService; + let requestService: RequestService; + let rdbService: RemoteDataBuildService; + let objectCache: ObjectCacheService; + let router: Router; + let remoteData; + const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; + const dsoHandle = '1234567789/22'; + const encodedHandle = encodeURIComponent(dsoHandle); + const pidLink = 'https://rest.api/rest/api/pid/find{?id}'; + const requestHandleURL = `https://rest.api/rest/api/pid/find?id=${encodedHandle}`; + const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; + const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; + const testObject = { + uuid: dsoUUID + } as DSpaceObject; + + beforeEach(() => { + scheduler = getTestScheduler(); + + halService = jasmine.createSpyObj('halService', { + getEndpoint: cold('a', { a: pidLink }) + }); + requestService = jasmine.createSpyObj('requestService', { + generateRequestId: requestUUID, + configure: true + }); + rdbService = jasmine.createSpyObj('rdbService', { + buildSingle: cold('a', { + a: { + payload: testObject + } + }) + }); + router = jasmine.createSpyObj('router', { + navigate: () => true + }); + remoteData = { + isSuccessful: true, + error: undefined, + hasSucceeded: true, + payload: { + type: 'item', + id: '123456789' + } + }; + objectCache = {} as ObjectCacheService; + const store = {} as Store; + const notificationsService = {} as NotificationsService; + const http = {} as HttpClient; + const comparator = {} as any; + const dataBuildService = {} as NormalizedObjectBuildService; + + service = new DsoDataRedirectService( + requestService, + rdbService, + dataBuildService, + store, + objectCache, + halService, + notificationsService, + http, + comparator, + router + ); + }); + + describe('findById', () => { + it('should call HALEndpointService with the path to the dso endpoint', () => { + scheduler.schedule(() => service.findById(dsoUUID)); + scheduler.flush(); + + expect(halService.getEndpoint).toHaveBeenCalledWith('pid'); + }); + + it('should configure the proper FindByIDRequest for uuid', () => { + scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); + scheduler.flush(); + + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestUUIDURL, dsoUUID, IdentifierType.UUID), false); + }); + + it('should configure the proper FindByIDRequest for handle', () => { + scheduler.schedule(() => service.findById(dsoHandle, IdentifierType.HANDLE)); + scheduler.flush(); + + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); + }); + + // TODO: test for router.navigate + }); +}); diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-data-redirect.service.ts new file mode 100644 index 0000000000..b568a23c8a --- /dev/null +++ b/src/app/core/data/dso-data-redirect.service.ts @@ -0,0 +1,78 @@ +import { DataService } from './data.service'; +import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; +import { HALEndpointService } from '../shared/hal-endpoint.service'; +import { HttpClient } from '@angular/common/http'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import { RequestService } from './request.service'; +import { Store } from '@ngrx/store'; +import { CoreState } from '../core.reducers'; +import { FindAllOptions, FindByIDRequest } from './request.models'; +import { Observable, of } from 'rxjs'; +import { IdentifierType } from '../index/index.reducer'; +import { RemoteData } from './remote-data'; +import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; +import { Injectable } from '@angular/core'; +import { map, tap } from 'rxjs/operators'; +import { hasValue } from '../../shared/empty.util'; +import { getFinishedRemoteData, getSucceededRemoteData } from '../shared/operators'; +import { Router } from '@angular/router'; + +@Injectable() +export class DsoDataRedirectService extends DataService { + + protected linkPath = 'pid'; + protected forceBypassCache = false; + + constructor( + protected requestService: RequestService, + protected rdbService: RemoteDataBuildService, + protected dataBuildService: NormalizedObjectBuildService, + protected store: Store, + protected objectCache: ObjectCacheService, + protected halService: HALEndpointService, + protected notificationsService: NotificationsService, + protected http: HttpClient, + protected comparator: DSOChangeAnalyzer, + private router: Router) { + super(); + } + + getBrowseEndpoint(options: FindAllOptions = {}, linkPath: string = this.linkPath): Observable { + return this.halService.getEndpoint(linkPath); + } + + getIDHref(endpoint, resourceID): string { + return endpoint.replace(/\{\?id\}/,`?id=${resourceID}`); + } + + findById(id: string, identifierType = IdentifierType.UUID): Observable> { + return super.findById(id, identifierType).pipe( + getFinishedRemoteData(), + tap((response) => { + if (response.hasSucceeded) { + const uuid = response.payload.uuid; + // Is there an existing method somewhere that converts dso type to endpoint? + // This will not work for all endpoints! + const dsoType = this.getEndpointFromDSOType(response.payload.type); + if (hasValue(uuid) && hasValue(dsoType)) { + this.router.navigate([dsoType + '/' + uuid]); + } + } + }) + ); + } + + getEndpointFromDSOType(dsoType: string): string { + if (dsoType.startsWith('item')) { + return 'items' + } else if (dsoType.startsWith('community')) { + return 'communities'; + } else if (dsoType.startsWith('collection')) { + return 'collections' + } else { + return ''; + } + } +} diff --git a/src/app/core/data/dspace-object-data.service.spec.ts b/src/app/core/data/dspace-object-data.service.spec.ts index a0bba214ae..b411028420 100644 --- a/src/app/core/data/dspace-object-data.service.spec.ts +++ b/src/app/core/data/dspace-object-data.service.spec.ts @@ -10,6 +10,7 @@ import { ObjectCacheService } from '../cache/object-cache.service'; import { NotificationsService } from '../../shared/notifications/notifications.service'; import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; +import { IdentifierType } from '../index/index.reducer'; describe('DSpaceObjectDataService', () => { let scheduler: TestScheduler; @@ -72,7 +73,7 @@ describe('DSpaceObjectDataService', () => { scheduler.schedule(() => service.findById(testObject.uuid)); scheduler.flush(); - expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestURL, testObject.uuid), false); + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestURL, testObject.uuid, IdentifierType.UUID), false); }); it('should return a RemoteData for the object with the given ID', () => { diff --git a/src/app/core/data/request.models.ts b/src/app/core/data/request.models.ts index a2b3423960..d3117d94b2 100644 --- a/src/app/core/data/request.models.ts +++ b/src/app/core/data/request.models.ts @@ -18,6 +18,7 @@ import { MetadataschemaParsingService } from './metadataschema-parsing.service'; import { MetadatafieldParsingService } from './metadatafield-parsing.service'; import { URLCombiner } from '../url-combiner/url-combiner'; import { TaskResponseParsingService } from '../tasks/task-response-parsing.service'; +import { IdentifierType } from '../index/index.reducer'; /* tslint:disable:max-classes-per-file */ @@ -48,7 +49,7 @@ export class GetRequest extends RestRequest { public uuid: string, public href: string, public body?: any, - public options?: HttpOptions, + public options?: HttpOptions ) { super(uuid, href, RestRequestMethod.GET, body, options) } @@ -124,7 +125,8 @@ export class FindByIDRequest extends GetRequest { constructor( uuid: string, href: string, - public resourceID: string + public resourceID: string, + public identifierType?: IdentifierType ) { super(uuid, href); } diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 0980d48537..ac65042238 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -11,7 +11,7 @@ import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; import { ObjectCacheService } from '../cache/object-cache.service'; import { CoreState } from '../core.reducers'; -import { IndexName, IndexState, MetaIndexState } from '../index/index.reducer'; +import { IdentifierType, IndexState, MetaIndexState, REQUEST, UUID_MAPPING } from '../index/index.reducer'; import { originalRequestUUIDFromRequestUUIDSelector, requestIndexSelector, @@ -19,7 +19,7 @@ import { } from '../index/index.selectors'; import { UUIDService } from '../shared/uuid.service'; import { RequestConfigureAction, RequestExecuteAction, RequestRemoveAction } from './request.actions'; -import { GetRequest, RestRequest } from './request.models'; +import { FindByIDRequest, GetRequest, RestRequest } from './request.models'; import { RequestEntry, RequestState } from './request.reducer'; import { CommitSSBAction } from '../cache/server-sync-buffer.actions'; import { RestRequestMethod } from './rest-request-method'; @@ -162,7 +162,7 @@ export class RequestService { filter((entry) => hasValue(entry)), take(1) ).subscribe((entry) => { - return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) + return this.store.dispatch(new AddToIndexAction(UUID_MAPPING, request.uuid, entry.request.uuid)) } ) } @@ -206,7 +206,7 @@ export class RequestService { } }); this.requestsOnTheirWayToTheStore = this.requestsOnTheirWayToTheStore.filter((reqHref: string) => reqHref.indexOf(href) < 0); - this.indexStore.dispatch(new RemoveFromIndexBySubstringAction(IndexName.REQUEST, href)); + this.indexStore.dispatch(new RemoveFromIndexBySubstringAction(REQUEST, href)); } /** @@ -225,8 +225,14 @@ export class RequestService { private isCachedOrPending(request: GetRequest): boolean { const inReqCache = this.hasByHref(request.href); const inObjCache = this.objectCache.hasBySelfLink(request.href); - const isCached = inReqCache || inObjCache; - + let inObjIdCache = false; + if (request instanceof FindByIDRequest) { + const req = request as FindByIDRequest; + if (hasValue(req.identifierType && hasValue(req.resourceID))) { + inObjIdCache = this.objectCache.hasById(req.resourceID, req.identifierType) + } + } + const isCached = inReqCache || inObjCache || inObjIdCache; const isPending = this.isPending(request); return isCached || isPending; } diff --git a/src/app/core/index/index.actions.ts b/src/app/core/index/index.actions.ts index 42804dbe26..24f031b33c 100644 --- a/src/app/core/index/index.actions.ts +++ b/src/app/core/index/index.actions.ts @@ -1,7 +1,7 @@ import { Action } from '@ngrx/store'; import { type } from '../../shared/ngrx/type'; -import { IndexName } from './index.reducer'; +import { } from './index.reducer'; /** * The list of HrefIndexAction type definitions @@ -19,7 +19,7 @@ export const IndexActionTypes = { export class AddToIndexAction implements Action { type = IndexActionTypes.ADD; payload: { - name: IndexName; + name: string; value: string; key: string; }; @@ -34,7 +34,7 @@ export class AddToIndexAction implements Action { * @param value * the self link of the resource the key belongs to */ - constructor(name: IndexName, key: string, value: string) { + constructor(name: string, key: string, value: string) { this.payload = { name, key, value }; } } @@ -45,7 +45,7 @@ export class AddToIndexAction implements Action { export class RemoveFromIndexByValueAction implements Action { type = IndexActionTypes.REMOVE_BY_VALUE; payload: { - name: IndexName, + name: string, value: string }; @@ -57,7 +57,7 @@ export class RemoveFromIndexByValueAction implements Action { * @param value * the value to remove the UUID for */ - constructor(name: IndexName, value: string) { + constructor(name: string, value: string) { this.payload = { name, value }; } @@ -69,7 +69,7 @@ export class RemoveFromIndexByValueAction implements Action { export class RemoveFromIndexBySubstringAction implements Action { type = IndexActionTypes.REMOVE_BY_SUBSTRING; payload: { - name: IndexName, + name: string, value: string }; @@ -81,7 +81,7 @@ export class RemoveFromIndexBySubstringAction implements Action { * @param value * the value to remove the UUID for */ - constructor(name: IndexName, value: string) { + constructor(name: string, value: string) { this.payload = { name, value }; } diff --git a/src/app/core/index/index.effects.ts b/src/app/core/index/index.effects.ts index 61cf313ab1..0cdf6bea6c 100644 --- a/src/app/core/index/index.effects.ts +++ b/src/app/core/index/index.effects.ts @@ -10,43 +10,67 @@ import { import { RequestActionTypes, RequestConfigureAction } from '../data/request.actions'; import { AddToIndexAction, RemoveFromIndexByValueAction } from './index.actions'; import { hasValue } from '../../shared/empty.util'; -import { IndexName } from './index.reducer'; +import { getIdentiferByIndexName, IdentifierType, REQUEST } from './index.reducer'; import { RestRequestMethod } from '../data/rest-request-method'; @Injectable() -export class UUIDIndexEffects { +export class IdentifierIndexEffects { - @Effect() addObject$ = this.actions$ + @Effect() addObjectByUUID$ = this.actions$ .pipe( ofType(ObjectCacheActionTypes.ADD), filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.uuid)), map((action: AddToObjectCacheAction) => { return new AddToIndexAction( - IndexName.OBJECT, + getIdentiferByIndexName(IdentifierType.UUID), action.payload.objectToCache.uuid, action.payload.objectToCache.self ); }) ); - @Effect() removeObject$ = this.actions$ + @Effect() addObjectByHandle$ = this.actions$ + .pipe( + ofType(ObjectCacheActionTypes.ADD), + filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.handle)), + map((action: AddToObjectCacheAction) => { + return new AddToIndexAction( + getIdentiferByIndexName(IdentifierType.HANDLE), + action.payload.objectToCache.handle, + action.payload.objectToCache.self + ); + }) + ); + + @Effect() removeObjectByUUID$ = this.actions$ .pipe( ofType(ObjectCacheActionTypes.REMOVE), map((action: RemoveFromObjectCacheAction) => { return new RemoveFromIndexByValueAction( - IndexName.OBJECT, + getIdentiferByIndexName(IdentifierType.UUID), action.payload ); }) ); + @Effect() removeObjectByHandle$ = this.actions$ + .pipe( + ofType(ObjectCacheActionTypes.REMOVE), + map((action: RemoveFromObjectCacheAction) => { + return new RemoveFromIndexByValueAction( + getIdentiferByIndexName(IdentifierType.HANDLE), + action.payload + ); + }) + ); + @Effect() addRequest$ = this.actions$ .pipe( ofType(RequestActionTypes.CONFIGURE), filter((action: RequestConfigureAction) => action.payload.method === RestRequestMethod.GET), map((action: RequestConfigureAction) => { return new AddToIndexAction( - IndexName.REQUEST, + REQUEST, action.payload.href, action.payload.uuid ); diff --git a/src/app/core/index/index.reducer.spec.ts b/src/app/core/index/index.reducer.spec.ts index ef46c760c6..35460a9ef5 100644 --- a/src/app/core/index/index.reducer.spec.ts +++ b/src/app/core/index/index.reducer.spec.ts @@ -1,6 +1,6 @@ import * as deepFreeze from 'deep-freeze'; -import { IndexName, indexReducer, MetaIndexState } from './index.reducer'; +import { getIdentiferByIndexName, IdentifierType, indexReducer, MetaIndexState, REQUEST, } from './index.reducer'; import { AddToIndexAction, RemoveFromIndexBySubstringAction, RemoveFromIndexByValueAction } from './index.actions'; class NullAction extends AddToIndexAction { @@ -15,14 +15,19 @@ class NullAction extends AddToIndexAction { describe('requestReducer', () => { const key1 = '567a639f-f5ff-4126-807c-b7d0910808c8'; const key2 = '1911e8a4-6939-490c-b58b-a5d70f8d91fb'; + const key3 = '123456789/22'; const val1 = 'https://dspace7.4science.it/dspace-spring-rest/api/core/items/567a639f-f5ff-4126-807c-b7d0910808c8'; const val2 = 'https://dspace7.4science.it/dspace-spring-rest/api/core/items/1911e8a4-6939-490c-b58b-a5d70f8d91fb'; + const uuidIndex = getIdentiferByIndexName(IdentifierType.UUID); + const handleIndex = getIdentiferByIndexName(IdentifierType.HANDLE); const testState: MetaIndexState = { - [IndexName.OBJECT]: { + 'object/uuid-to-self-link/uuid': { [key1]: val1 - },[IndexName.REQUEST]: { + },'object/uuid-to-self-link/handle': { + [key3]: val1 + },'get-request/href-to-uuid': { [key1]: val1 - },[IndexName.UUID_MAPPING]: { + },'get-request/configured-to-cache-uuid': { [key1]: val1 } }; @@ -45,27 +50,38 @@ describe('requestReducer', () => { it('should add the \'key\' with the corresponding \'value\' to the correct substate, in response to an ADD action', () => { const state = testState; - const action = new AddToIndexAction(IndexName.REQUEST, key2, val2); + const action = new AddToIndexAction(REQUEST, key2, val2); const newState = indexReducer(state, action); - expect(newState[IndexName.REQUEST][key2]).toEqual(val2); + expect(newState[REQUEST][key2]).toEqual(val2); }); it('should remove the given \'value\' from its corresponding \'key\' in the correct substate, in response to a REMOVE_BY_VALUE action', () => { const state = testState; - const action = new RemoveFromIndexByValueAction(IndexName.OBJECT, val1); - const newState = indexReducer(state, action); + let action = new RemoveFromIndexByValueAction(uuidIndex, val1); + let newState = indexReducer(state, action); + + expect(newState[uuidIndex][key1]).toBeUndefined(); + + action = new RemoveFromIndexByValueAction(handleIndex, val1); + newState = indexReducer(state, action); + + expect(newState[handleIndex][key3]).toBeUndefined(); - expect(newState[IndexName.OBJECT][key1]).toBeUndefined(); }); it('should remove the given \'value\' from its corresponding \'key\' in the correct substate, in response to a REMOVE_BY_SUBSTRING action', () => { const state = testState; - const action = new RemoveFromIndexBySubstringAction(IndexName.OBJECT, key1); - const newState = indexReducer(state, action); + let action = new RemoveFromIndexBySubstringAction(uuidIndex, key1); + let newState = indexReducer(state, action); - expect(newState[IndexName.OBJECT][key1]).toBeUndefined(); + expect(newState[uuidIndex][key1]).toBeUndefined(); + + action = new RemoveFromIndexBySubstringAction(handleIndex, key3); + newState = indexReducer(state, action); + + expect(newState[uuidIndex][key3]).toBeUndefined(); }); }); diff --git a/src/app/core/index/index.reducer.ts b/src/app/core/index/index.reducer.ts index b4cd8aa84b..631d579911 100644 --- a/src/app/core/index/index.reducer.ts +++ b/src/app/core/index/index.reducer.ts @@ -6,24 +6,26 @@ import { RemoveFromIndexByValueAction } from './index.actions'; -/** - * An enum containing all index names - */ -export enum IndexName { - // Contains all objects in the object cache indexed by UUID - OBJECT = 'object/uuid-to-self-link', - - // contains all requests in the request cache indexed by UUID - REQUEST = 'get-request/href-to-uuid', - - /** - * Contains the UUIDs of requests that were sent to the server and - * have their responses cached, indexed by the UUIDs of requests that - * weren't sent because the response they requested was already cached - */ - UUID_MAPPING = 'get-request/configured-to-cache-uuid' +export enum IdentifierType { + UUID ='uuid', + HANDLE = 'handle' } +/** + * Contains the UUIDs of requests that were sent to the server and + * have their responses cached, indexed by the UUIDs of requests that + * weren't sent because the response they requested was already cached + */ +export const UUID_MAPPING = 'get-request/configured-to-cache-uuid'; + +// contains all requests in the request cache indexed by UUID +export const REQUEST = 'get-request/href-to-uuid'; + +// returns the index for the provided id type (uuid, handle) +export const getIdentiferByIndexName = (idType: IdentifierType): string => { + return `object/uuid-to-self-link/${idType}`; +}; + /** * The state of a single index */ @@ -34,8 +36,11 @@ export interface IndexState { /** * The state that contains all indices */ -export type MetaIndexState = { - [name in IndexName]: IndexState +export interface MetaIndexState { + 'get-request/configured-to-cache-uuid': IndexState, + 'get-request/href-to-uuid': IndexState, + 'object/uuid-to-self-link/uuid': IndexState, + 'object/uuid-to-self-link/handle': IndexState } // Object.create(null) ensures the object has no default js properties (e.g. `__proto__`) diff --git a/src/app/core/index/index.selectors.ts b/src/app/core/index/index.selectors.ts index 3c7b331a92..80a7c0d46a 100644 --- a/src/app/core/index/index.selectors.ts +++ b/src/app/core/index/index.selectors.ts @@ -3,7 +3,14 @@ import { AppState } from '../../app.reducer'; import { hasValue } from '../../shared/empty.util'; import { CoreState } from '../core.reducers'; import { coreSelector } from '../core.selectors'; -import { IndexName, IndexState, MetaIndexState } from './index.reducer'; +import { + getIdentiferByIndexName, + IdentifierType, + IndexState, + MetaIndexState, + REQUEST, + UUID_MAPPING +} from './index.reducer'; /** * Return the MetaIndexState based on the CoreSate @@ -20,13 +27,17 @@ export const metaIndexSelector: MemoizedSelector = cre * Return the object index based on the MetaIndexState * It contains all objects in the object cache indexed by UUID * + * @param identifierType the type of index, used to select index from state + * * @returns * a MemoizedSelector to select the object index */ -export const objectIndexSelector: MemoizedSelector = createSelector( - metaIndexSelector, - (state: MetaIndexState) => state[IndexName.OBJECT] -); +export const objectIndexSelector = (identifierType: IdentifierType = IdentifierType.UUID): MemoizedSelector => { + return createSelector( + metaIndexSelector, + (state: MetaIndexState) => state[getIdentiferByIndexName(identifierType)] + ); +} /** * Return the request index based on the MetaIndexState @@ -36,7 +47,7 @@ export const objectIndexSelector: MemoizedSelector = creat */ export const requestIndexSelector: MemoizedSelector = createSelector( metaIndexSelector, - (state: MetaIndexState) => state[IndexName.REQUEST] + (state: MetaIndexState) => state[REQUEST] ); /** @@ -47,21 +58,22 @@ export const requestIndexSelector: MemoizedSelector = crea */ export const requestUUIDIndexSelector: MemoizedSelector = createSelector( metaIndexSelector, - (state: MetaIndexState) => state[IndexName.UUID_MAPPING] + (state: MetaIndexState) => state[UUID_MAPPING] ); /** * Return the self link of an object in the object-cache based on its UUID * - * @param uuid + * @param id * the UUID for which you want to find the matching self link + * @param identifierType the type of index, used to select index from state * @returns * a MemoizedSelector to select the self link */ export const selfLinkFromUuidSelector = - (uuid: string): MemoizedSelector => createSelector( - objectIndexSelector, - (state: IndexState) => hasValue(state) ? state[uuid] : undefined + (id: string, identifierType: IdentifierType = IdentifierType.UUID): MemoizedSelector => createSelector( + objectIndexSelector(identifierType), + (state: IndexState) => hasValue(state) ? state[id] : undefined ); /** From e41b3083aab476c71ba963d74fac9cb1fb3ca32a Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Mon, 30 Sep 2019 16:57:06 -0700 Subject: [PATCH 02/22] Added new unit tests. --- .../objectnotfound.component.spec.ts | 79 +++++++++++++++++++ .../data/dso-data-redirect.service.spec.ts | 35 ++++---- .../core/data/dso-data-redirect.service.ts | 6 +- 3 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts new file mode 100644 index 0000000000..7905655a06 --- /dev/null +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts @@ -0,0 +1,79 @@ +import { NO_ERRORS_SCHEMA } from '@angular/core'; +import { async, ComponentFixture, TestBed } from '@angular/core/testing'; +import { TranslateModule } from '@ngx-translate/core'; + +import { ObjectNotFoundComponent } from './objectnotfound.component'; +import { ActivatedRouteStub } from '../../shared/testing/active-router-stub'; +import { of as observableOf } from 'rxjs'; +import { ActivatedRoute } from '@angular/router'; + +describe('ObjectNotFoundComponent', () => { + let comp: ObjectNotFoundComponent; + let fixture: ComponentFixture; + const testUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; + const uuidType = 'uuid'; + const handlePrefix = '123456789'; + const handleId = '22'; + const activatedRouteStub = Object.assign(new ActivatedRouteStub(), { + params: observableOf({id: testUUID, idType: uuidType}) + }); + const activatedRouteStubHandle = Object.assign(new ActivatedRouteStub(), { + params: observableOf({id: handleId, idType: handlePrefix}) + }); + describe('uuid request', () => { + beforeEach(async(() => { + TestBed.configureTestingModule({ + imports: [ + TranslateModule.forRoot() + ], providers: [ + {provide: ActivatedRoute, useValue: activatedRouteStub} + ], + declarations: [ObjectNotFoundComponent], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); + + beforeEach(() => { + fixture = TestBed.createComponent(ObjectNotFoundComponent); + comp = fixture.componentInstance; + fixture.detectChanges(); + }); + + it('should create instance', () => { + expect(comp).toBeDefined() + }); + + it('should have id and idType', () => { + expect(comp.id).toEqual(testUUID); + expect(comp.idType).toEqual(uuidType); + expect(comp.missingItem).toEqual('uuid: ' + testUUID); + }); + }); + + describe( 'legacy handle request', () => { + beforeEach(async(() => { + TestBed.configureTestingModule({ + imports: [ + TranslateModule.forRoot() + ], providers: [ + {provide: ActivatedRoute, useValue: activatedRouteStubHandle} + ], + declarations: [ObjectNotFoundComponent], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); + + beforeEach(() => { + fixture = TestBed.createComponent(ObjectNotFoundComponent); + comp = fixture.componentInstance; + fixture.detectChanges(); + }); + + it('should have handle prefix and id', () => { + expect(comp.id).toEqual(handleId); + expect(comp.idType).toEqual(handlePrefix); + expect(comp.missingItem).toEqual('handle: ' + handlePrefix + '/' + handleId); + }); + }); + +}); diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts index ece3c242fc..040cecc435 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -1,7 +1,6 @@ import { cold, getTestScheduler } from 'jasmine-marbles'; import { TestScheduler } from 'rxjs/testing'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; -import { DSpaceObject } from '../shared/dspace-object.model'; import { HALEndpointService } from '../shared/hal-endpoint.service'; import { FindByIDRequest } from './request.models'; import { RequestService } from './request.service'; @@ -11,7 +10,6 @@ import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; import { IdentifierType } from '../index/index.reducer'; import { DsoDataRedirectService } from './dso-data-redirect.service'; -import { Router } from '@angular/router'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; @@ -22,7 +20,7 @@ describe('DsoDataRedirectService', () => { let requestService: RequestService; let rdbService: RemoteDataBuildService; let objectCache: ObjectCacheService; - let router: Router; + let router; let remoteData; const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; const dsoHandle = '1234567789/22'; @@ -31,15 +29,12 @@ describe('DsoDataRedirectService', () => { const requestHandleURL = `https://rest.api/rest/api/pid/find?id=${encodedHandle}`; const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; - const testObject = { - uuid: dsoUUID - } as DSpaceObject; beforeEach(() => { scheduler = getTestScheduler(); halService = jasmine.createSpyObj('halService', { - getEndpoint: cold('a', { a: pidLink }) + getEndpoint: cold('a', {a: pidLink}) }); requestService = jasmine.createSpyObj('requestService', { generateRequestId: requestUUID, @@ -47,21 +42,20 @@ describe('DsoDataRedirectService', () => { }); rdbService = jasmine.createSpyObj('rdbService', { buildSingle: cold('a', { - a: { - payload: testObject - } + a: remoteData }) }); - router = jasmine.createSpyObj('router', { - navigate: () => true - }); + router = { + navigate: jasmine.createSpy('navigate') + }; remoteData = { isSuccessful: true, error: undefined, hasSucceeded: true, + isLoading: false, payload: { type: 'item', - id: '123456789' + uuid: '123456789' } }; objectCache = {} as ObjectCacheService; @@ -86,7 +80,7 @@ describe('DsoDataRedirectService', () => { }); describe('findById', () => { - it('should call HALEndpointService with the path to the dso endpoint', () => { + it('should call HALEndpointService with the path to the pid endpoint', () => { scheduler.schedule(() => service.findById(dsoUUID)); scheduler.flush(); @@ -107,6 +101,13 @@ describe('DsoDataRedirectService', () => { expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); }); - // TODO: test for router.navigate - }); + it('should navigate to dso route', () => { + const redir = service.findById(dsoHandle, IdentifierType.HANDLE); + // The framework would normally subscribe but do it here so we can test navigation. + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(router.navigate).toHaveBeenCalledWith([remoteData.payload.type + 's/' + remoteData.payload.uuid]); + }); + }) }); diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-data-redirect.service.ts index b568a23c8a..3d779de5cd 100644 --- a/src/app/core/data/dso-data-redirect.service.ts +++ b/src/app/core/data/dso-data-redirect.service.ts @@ -9,14 +9,14 @@ import { RequestService } from './request.service'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; import { FindAllOptions, FindByIDRequest } from './request.models'; -import { Observable, of } from 'rxjs'; +import { Observable } from 'rxjs'; import { IdentifierType } from '../index/index.reducer'; import { RemoteData } from './remote-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; import { Injectable } from '@angular/core'; -import { map, tap } from 'rxjs/operators'; +import { tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; -import { getFinishedRemoteData, getSucceededRemoteData } from '../shared/operators'; +import { getFinishedRemoteData } from '../shared/operators'; import { Router } from '@angular/router'; @Injectable() From 8fe37eeaf796512605a57cba03f5b1387cbc4b44 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 01:06:12 -0700 Subject: [PATCH 03/22] Minor change to comment. --- src/app/core/data/dso-data-redirect.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-data-redirect.service.ts index 3d779de5cd..1cb3b20f2a 100644 --- a/src/app/core/data/dso-data-redirect.service.ts +++ b/src/app/core/data/dso-data-redirect.service.ts @@ -53,8 +53,7 @@ export class DsoDataRedirectService extends DataService { tap((response) => { if (response.hasSucceeded) { const uuid = response.payload.uuid; - // Is there an existing method somewhere that converts dso type to endpoint? - // This will not work for all endpoints! + // Is there an existing method somewhere that converts dso type route? const dsoType = this.getEndpointFromDSOType(response.payload.type); if (hasValue(uuid) && hasValue(dsoType)) { this.router.navigate([dsoType + '/' + uuid]); @@ -65,6 +64,7 @@ export class DsoDataRedirectService extends DataService { } getEndpointFromDSOType(dsoType: string): string { + // Are there other routes to consider? if (dsoType.startsWith('item')) { return 'items' } else if (dsoType.startsWith('community')) { From 24be6c1544f7c4b47ddab8f550347a0af6cbe5c4 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 03:03:08 -0700 Subject: [PATCH 04/22] Added unit tests. --- src/app/+lookup-by-id/lookup-guard.spec.ts | 50 ++++++++++++ src/app/+lookup-by-id/lookup-guard.ts | 2 +- .../data/dso-data-redirect.service.spec.ts | 81 +++++++++++++------ 3 files changed, 106 insertions(+), 27 deletions(-) create mode 100644 src/app/+lookup-by-id/lookup-guard.spec.ts diff --git a/src/app/+lookup-by-id/lookup-guard.spec.ts b/src/app/+lookup-by-id/lookup-guard.spec.ts new file mode 100644 index 0000000000..ea077d0811 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-guard.spec.ts @@ -0,0 +1,50 @@ +import {LookupGuard} from "./lookup-guard"; +import {of as observableOf} from "rxjs"; +import {IdentifierType} from "../core/index/index.reducer"; + +describe('LookupGuard', () => { + let dsoService: any; + let guard: any; + + beforeEach(() => { + dsoService = { + findById: jasmine.createSpy('findById').and.returnValue(observableOf({ hasFailed: false, + hasSucceeded: true })) + }; + guard = new LookupGuard(dsoService); + }); + + it('should call findById with handle params', () => { + const scopedRoute = { + params: { + id: '1234', + idType: '123456789' + } + }; + guard.canActivate(scopedRoute as any, undefined); + expect(dsoService.findById).toHaveBeenCalledWith('123456789%2F1234', IdentifierType.HANDLE) + }); + + it('should call findById with handle params', () => { + const scopedRoute = { + params: { + id: '123456789%2F1234', + idType: 'handle' + } + }; + guard.canActivate(scopedRoute as any, undefined); + expect(dsoService.findById).toHaveBeenCalledWith('123456789%2F1234', IdentifierType.HANDLE) + }); + + it('should call findById with UUID params', () => { + const scopedRoute = { + params: { + id: '34cfed7c-f597-49ef-9cbe-ea351f0023c2', + idType: 'uuid' + } + }; + guard.canActivate(scopedRoute as any, undefined); + expect(dsoService.findById).toHaveBeenCalledWith('34cfed7c-f597-49ef-9cbe-ea351f0023c2', IdentifierType.UUID) + }); + +}); diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts index 61e3688ee2..ceb11b7cf5 100644 --- a/src/app/+lookup-by-id/lookup-guard.ts +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -14,7 +14,7 @@ interface LookupParams { @Injectable() export class LookupGuard implements CanActivate { - constructor(private dsoService: DsoDataRedirectService, private router: Router) { + constructor(private dsoService: DsoDataRedirectService) { } canActivate(route: ActivatedRouteSnapshot, state:RouterStateSnapshot): Observable { diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts index 040cecc435..a9a83e72d6 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -19,7 +19,6 @@ describe('DsoDataRedirectService', () => { let halService: HALEndpointService; let requestService: RequestService; let rdbService: RemoteDataBuildService; - let objectCache: ObjectCacheService; let router; let remoteData; const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; @@ -29,7 +28,13 @@ describe('DsoDataRedirectService', () => { const requestHandleURL = `https://rest.api/rest/api/pid/find?id=${encodedHandle}`; const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; - + const store = {} as Store; + const notificationsService = {} as NotificationsService; + const http = {} as HttpClient; + const comparator = {} as any; + const dataBuildService = {} as NormalizedObjectBuildService; + const objectCache = {} as ObjectCacheService; + let setup; beforeEach(() => { scheduler = getTestScheduler(); @@ -40,14 +45,10 @@ describe('DsoDataRedirectService', () => { generateRequestId: requestUUID, configure: true }); - rdbService = jasmine.createSpyObj('rdbService', { - buildSingle: cold('a', { - a: remoteData - }) - }); router = { navigate: jasmine.createSpy('navigate') }; + remoteData = { isSuccessful: true, error: undefined, @@ -58,29 +59,31 @@ describe('DsoDataRedirectService', () => { uuid: '123456789' } }; - objectCache = {} as ObjectCacheService; - const store = {} as Store; - const notificationsService = {} as NotificationsService; - const http = {} as HttpClient; - const comparator = {} as any; - const dataBuildService = {} as NormalizedObjectBuildService; - service = new DsoDataRedirectService( - requestService, - rdbService, - dataBuildService, - store, - objectCache, - halService, - notificationsService, - http, - comparator, - router - ); + setup = () => { + rdbService = jasmine.createSpyObj('rdbService', { + buildSingle: cold('a', { + a: remoteData + }) + }); + service = new DsoDataRedirectService( + requestService, + rdbService, + dataBuildService, + store, + objectCache, + halService, + notificationsService, + http, + comparator, + router + ); + } }); describe('findById', () => { it('should call HALEndpointService with the path to the pid endpoint', () => { + setup(); scheduler.schedule(() => service.findById(dsoUUID)); scheduler.flush(); @@ -88,6 +91,7 @@ describe('DsoDataRedirectService', () => { }); it('should configure the proper FindByIDRequest for uuid', () => { + setup(); scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); scheduler.flush(); @@ -95,13 +99,16 @@ describe('DsoDataRedirectService', () => { }); it('should configure the proper FindByIDRequest for handle', () => { + setup(); scheduler.schedule(() => service.findById(dsoHandle, IdentifierType.HANDLE)); scheduler.flush(); expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); }); - it('should navigate to dso route', () => { + it('should navigate to item route', () => { + remoteData.payload.type = 'item'; + setup(); const redir = service.findById(dsoHandle, IdentifierType.HANDLE); // The framework would normally subscribe but do it here so we can test navigation. redir.subscribe(); @@ -109,5 +116,27 @@ describe('DsoDataRedirectService', () => { scheduler.flush(); expect(router.navigate).toHaveBeenCalledWith([remoteData.payload.type + 's/' + remoteData.payload.uuid]); }); + + it('should navigate to collections route', () => { + remoteData.payload.type = 'collection'; + setup(); + const redir = service.findById(dsoHandle, IdentifierType.HANDLE); + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(router.navigate).toHaveBeenCalledWith([remoteData.payload.type + 's/' + remoteData.payload.uuid]); + }); + + it('should navigate to communities route', () => { + remoteData.payload.type = 'community'; + setup(); + const redir = service.findById(dsoHandle, IdentifierType.HANDLE); + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(router.navigate).toHaveBeenCalledWith(['communities/' + remoteData.payload.uuid]); + }); }) }); + + From dd817c91592a8513d86142e4c53988cc71b9dc0b Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 03:38:56 -0700 Subject: [PATCH 05/22] Fixed lint errors. --- src/app/+lookup-by-id/lookup-guard.spec.ts | 6 +++--- src/app/core/data/dso-data-redirect.service.spec.ts | 2 -- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/app/+lookup-by-id/lookup-guard.spec.ts b/src/app/+lookup-by-id/lookup-guard.spec.ts index ea077d0811..7b00383783 100644 --- a/src/app/+lookup-by-id/lookup-guard.spec.ts +++ b/src/app/+lookup-by-id/lookup-guard.spec.ts @@ -1,6 +1,6 @@ -import {LookupGuard} from "./lookup-guard"; -import {of as observableOf} from "rxjs"; -import {IdentifierType} from "../core/index/index.reducer"; +import { LookupGuard } from './lookup-guard'; +import { of as observableOf } from 'rxjs'; +import { IdentifierType } from '../core/index/index.reducer'; describe('LookupGuard', () => { let dsoService: any; diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts index a9a83e72d6..f2c35727fa 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -138,5 +138,3 @@ describe('DsoDataRedirectService', () => { }); }) }); - - From b89e57a1a611de6da922c325965c09ba11a4349e Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 12:25:41 -0700 Subject: [PATCH 06/22] Added unit test for isCachedOrPending lookup by id (FindByIdRequest). --- src/app/core/data/request.service.spec.ts | 14 +++++++++++++- src/app/shared/mocks/mock-object-cache.service.ts | 4 ++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index e2bc04040f..5ceca8b4bd 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -11,6 +11,7 @@ import { UUIDService } from '../shared/uuid.service'; import { RequestConfigureAction, RequestExecuteAction } from './request.actions'; import { DeleteRequest, + FindByIDRequest, GetRequest, HeadRequest, OptionsRequest, @@ -21,6 +22,7 @@ import { } from './request.models'; import { RequestService } from './request.service'; import { TestScheduler } from 'rxjs/testing'; +import { IdentifierType } from '../index/index.reducer'; describe('RequestService', () => { let scheduler: TestScheduler; @@ -38,6 +40,7 @@ describe('RequestService', () => { const testDeleteRequest = new DeleteRequest(testUUID, testHref); const testOptionsRequest = new OptionsRequest(testUUID, testHref); const testHeadRequest = new HeadRequest(testUUID, testHref); + const testFindByIdRequest = new FindByIDRequest(testUUID, testHref, testUUID, IdentifierType.UUID); const testPatchRequest = new PatchRequest(testUUID, testHref); let selectSpy; @@ -298,13 +301,22 @@ describe('RequestService', () => { describe('in the ObjectCache', () => { beforeEach(() => { (objectCache.hasBySelfLink as any).and.returnValue(true); + (objectCache.hasById as any).and.returnValue(true); spyOn(serviceAsAny, 'hasByHref').and.returnValue(false); }); - it('should return true', () => { + it('should return true for GetRequest', () => { const result = serviceAsAny.isCachedOrPending(testGetRequest); const expected = true; + expect(result).toEqual(expected); + }); + it('should return true for instance of FindByIdRequest', () => { + (objectCache.hasBySelfLink as any).and.returnValue(false); + const result = serviceAsAny.isCachedOrPending(testFindByIdRequest); + expect(objectCache.hasById).toHaveBeenCalledWith(testUUID, IdentifierType.UUID); + const expected = true; + expect(result).toEqual(expected); }); }); diff --git a/src/app/shared/mocks/mock-object-cache.service.ts b/src/app/shared/mocks/mock-object-cache.service.ts index 9e35a519ff..d096cfdf5f 100644 --- a/src/app/shared/mocks/mock-object-cache.service.ts +++ b/src/app/shared/mocks/mock-object-cache.service.ts @@ -4,12 +4,12 @@ export function getMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCacheService', [ 'add', 'remove', - 'getByUUID', + 'getByID', 'getBySelfLink', 'getRequestHrefBySelfLink', 'getRequestHrefByUUID', 'getList', - 'hasByUUID', + 'hasById', 'hasBySelfLink' ]); From efc91a4591344761620f34c7befa4c1191f331be Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 25 Sep 2019 17:37:12 -0700 Subject: [PATCH 07/22] Updated work on routing by id. Fixed unit tests. Updated to use pid REST endpoint. Minor change in data.service and unit test update. Updated the objectnotfound page with new text and go home button. --- .../lookup-by-id-routing.module.ts | 19 +++ src/app/+lookup-by-id/lookup-by-id.module.ts | 23 ++++ src/app/+lookup-by-id/lookup-guard.ts | 52 ++++++++ .../objectnotfound.component.html | 8 ++ .../objectnotfound.component.scss | 0 .../objectnotfound.component.ts | 45 +++++++ src/app/app-routing.module.ts | 2 + src/app/app.module.ts | 2 +- src/app/core/cache/object-cache.reducer.ts | 1 + src/app/core/cache/object-cache.service.ts | 16 +-- src/app/core/core.effects.ts | 4 +- src/app/core/data/comcol-data.service.spec.ts | 4 +- src/app/core/data/comcol-data.service.ts | 2 +- src/app/core/data/data.service.ts | 18 ++- .../data/dso-data-redirect.service.spec.ts | 112 ++++++++++++++++++ .../core/data/dso-data-redirect.service.ts | 78 ++++++++++++ .../data/dspace-object-data.service.spec.ts | 3 +- src/app/core/data/request.models.ts | 6 +- src/app/core/data/request.service.ts | 18 ++- src/app/core/index/index.actions.ts | 14 +-- src/app/core/index/index.effects.ts | 38 ++++-- src/app/core/index/index.reducer.spec.ts | 40 +++++-- src/app/core/index/index.reducer.ts | 41 ++++--- src/app/core/index/index.selectors.ts | 34 ++++-- 24 files changed, 498 insertions(+), 82 deletions(-) create mode 100644 src/app/+lookup-by-id/lookup-by-id-routing.module.ts create mode 100644 src/app/+lookup-by-id/lookup-by-id.module.ts create mode 100644 src/app/+lookup-by-id/lookup-guard.ts create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.scss create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts create mode 100644 src/app/core/data/dso-data-redirect.service.spec.ts create mode 100644 src/app/core/data/dso-data-redirect.service.ts diff --git a/src/app/+lookup-by-id/lookup-by-id-routing.module.ts b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts new file mode 100644 index 0000000000..012345e791 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts @@ -0,0 +1,19 @@ +import { LookupGuard } from './lookup-guard'; +import { NgModule } from '@angular/core'; +import { RouterModule } from '@angular/router'; +import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; + +@NgModule({ + imports: [ + RouterModule.forChild([ + { path: ':idType/:id', canActivate: [LookupGuard], component: ObjectNotFoundComponent } + ]) + ], + providers: [ + LookupGuard + ] +}) + +export class LookupRoutingModule { + +} diff --git a/src/app/+lookup-by-id/lookup-by-id.module.ts b/src/app/+lookup-by-id/lookup-by-id.module.ts new file mode 100644 index 0000000000..4620f57824 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-by-id.module.ts @@ -0,0 +1,23 @@ +import { NgModule } from '@angular/core'; +import { CommonModule } from '@angular/common'; +import { SharedModule } from '../shared/shared.module'; +import { LookupRoutingModule } from './lookup-by-id-routing.module'; +import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; +import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; + +@NgModule({ + imports: [ + LookupRoutingModule, + CommonModule, + SharedModule, + ], + declarations: [ + ObjectNotFoundComponent + ], + providers: [ + DsoDataRedirectService + ] +}) +export class LookupIdModule { + +} diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts new file mode 100644 index 0000000000..61e3688ee2 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -0,0 +1,52 @@ +import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from '@angular/router'; +import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; +import { Injectable } from '@angular/core'; +import { IdentifierType } from '../core/index/index.reducer'; +import { Observable } from 'rxjs'; +import { map } from 'rxjs/operators'; +import { RemoteData } from '../core/data/remote-data'; +import { FindByIDRequest } from '../core/data/request.models'; + +interface LookupParams { + type: IdentifierType; + id: string; +} + +@Injectable() +export class LookupGuard implements CanActivate { + constructor(private dsoService: DsoDataRedirectService, private router: Router) { + } + + canActivate(route: ActivatedRouteSnapshot, state:RouterStateSnapshot): Observable { + const params = this.getLookupParams(route); + return this.dsoService.findById(params.id, params.type).pipe( + map((response: RemoteData) => response.hasFailed) + ); + } + + private getLookupParams(route: ActivatedRouteSnapshot): LookupParams { + let type; + let id; + const idType = route.params.idType; + + // If the idType is not recognized, assume a legacy handle request (handle/prefix/id) + if (idType !== IdentifierType.HANDLE && idType !== IdentifierType.UUID) { + type = IdentifierType.HANDLE; + const prefix = route.params.idType; + const handleId = route.params.id; + id = `${prefix}%2F${handleId}`; + + } else if (route.params.idType === IdentifierType.HANDLE) { + type = IdentifierType.HANDLE; + id = route.params.id; + + } else { + type = IdentifierType.UUID; + id = route.params.id; + } + return { + type: type, + id: id + }; + } +} diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html new file mode 100644 index 0000000000..662d3cde52 --- /dev/null +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html @@ -0,0 +1,8 @@ +
+

{{"error.item" | translate}}

+

{{missingItem}}

+
+

+ {{"404.link.home-page" | translate}} +

+
diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.scss b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.scss new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts new file mode 100644 index 0000000000..0116575154 --- /dev/null +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts @@ -0,0 +1,45 @@ + +import { Component, ChangeDetectionStrategy, OnInit } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; + +/** + * This component representing the `PageNotFound` DSpace page. + */ +@Component({ + selector: 'ds-objnotfound', + styleUrls: ['./objectnotfound.component.scss'], + templateUrl: './objectnotfound.component.html', + changeDetection: ChangeDetectionStrategy.Default +}) +export class ObjectNotFoundComponent implements OnInit { + + idType: string; + + id: string; + + missingItem: string; + + /** + * Initialize instance variables + * + * @param {AuthService} authservice + * @param {ServerResponseService} responseService + */ + constructor(private route: ActivatedRoute) { + route.params.subscribe((params) => { + this.idType = params.idType; + this.id = params.id; + }) + } + + ngOnInit(): void { + if (this.idType.startsWith('handle')) { + this.missingItem = 'handle: ' + this.id; + } else if (this.idType.startsWith('uuid')) { + this.missingItem = 'uuid: ' + this.id; + } else { + this.missingItem = 'handle: ' + this.idType + '/' + this.id; + } + } + +} diff --git a/src/app/app-routing.module.ts b/src/app/app-routing.module.ts index e1ddc2b889..5085633a5b 100644 --- a/src/app/app-routing.module.ts +++ b/src/app/app-routing.module.ts @@ -27,6 +27,8 @@ export function getAdminModulePath() { RouterModule.forRoot([ { path: '', redirectTo: '/home', pathMatch: 'full' }, { path: 'home', loadChildren: './+home-page/home-page.module#HomePageModule' }, + { path: 'id', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, + { path: 'handle', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, { path: COMMUNITY_MODULE_PATH, loadChildren: './+community-page/community-page.module#CommunityPageModule' }, { path: COLLECTION_MODULE_PATH, loadChildren: './+collection-page/collection-page.module#CollectionPageModule' }, { path: ITEM_MODULE_PATH, loadChildren: './+item-page/item-page.module#ItemPageModule' }, diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 916788df8c..3d8bf0ed43 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -128,7 +128,7 @@ const EXPORTS = [ ...PROVIDERS ], declarations: [ - ...DECLARATIONS, + ...DECLARATIONS ], exports: [ ...EXPORTS diff --git a/src/app/core/cache/object-cache.reducer.ts b/src/app/core/cache/object-cache.reducer.ts index f41151fd90..afc040bf59 100644 --- a/src/app/core/cache/object-cache.reducer.ts +++ b/src/app/core/cache/object-cache.reducer.ts @@ -44,6 +44,7 @@ export abstract class TypedObject { */ export class CacheableObject extends TypedObject { uuid?: string; + handle?: string; self: string; // isNew: boolean; // dirtyType: DirtyType; diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 11f3a6ce3e..95e96db0c8 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -4,7 +4,7 @@ import { applyPatch, Operation } from 'fast-json-patch'; import { combineLatest as observableCombineLatest, Observable } from 'rxjs'; import { distinctUntilChanged, filter, map, mergeMap, take, } from 'rxjs/operators'; -import { hasNoValue, hasValue, isNotEmpty } from '../../shared/empty.util'; +import { hasNoValue, isNotEmpty } from '../../shared/empty.util'; import { CoreState } from '../core.reducers'; import { coreSelector } from '../core.selectors'; import { RestRequestMethod } from '../data/rest-request-method'; @@ -21,6 +21,7 @@ import { import { CacheableObject, ObjectCacheEntry, ObjectCacheState } from './object-cache.reducer'; import { AddToSSBAction } from './server-sync-buffer.actions'; import { getMapsToType } from './builders/build-decorators'; +import { IdentifierType } from '../index/index.reducer'; /** * The base selector function to select the object cache in the store @@ -75,14 +76,15 @@ export class ObjectCacheService { /** * Get an observable of the object with the specified UUID * - * @param uuid + * @param id * The UUID of the object to get * @return Observable> * An observable of the requested object in normalized form */ - getObjectByUUID(uuid: string): Observable> { + getObjectByID(id: string, identifierType: IdentifierType = IdentifierType.UUID): + Observable> { return this.store.pipe( - select(selfLinkFromUuidSelector(uuid)), + select(selfLinkFromUuidSelector(id, identifierType)), mergeMap((selfLink: string) => this.getObjectBySelfLink(selfLink) ) ) @@ -188,17 +190,17 @@ export class ObjectCacheService { /** * Check whether the object with the specified UUID is cached * - * @param uuid + * @param id * The UUID of the object to check * @return boolean * true if the object with the specified UUID is cached, * false otherwise */ - hasByUUID(uuid: string): boolean { + hasById(id: string, identifierType: IdentifierType = IdentifierType.UUID): boolean { let result: boolean; this.store.pipe( - select(selfLinkFromUuidSelector(uuid)), + select(selfLinkFromUuidSelector(id, identifierType)), take(1) ).subscribe((selfLink: string) => result = this.hasBySelfLink(selfLink)); diff --git a/src/app/core/core.effects.ts b/src/app/core/core.effects.ts index 0eabfc5dc8..ae6f7f3cfa 100644 --- a/src/app/core/core.effects.ts +++ b/src/app/core/core.effects.ts @@ -1,5 +1,5 @@ import { ObjectCacheEffects } from './cache/object-cache.effects'; -import { UUIDIndexEffects } from './index/index.effects'; +import { IdentifierIndexEffects } from './index/index.effects'; import { RequestEffects } from './data/request.effects'; import { AuthEffects } from './auth/auth.effects'; import { JsonPatchOperationsEffects } from './json-patch/json-patch-operations.effects'; @@ -10,7 +10,7 @@ import { RouteEffects } from './services/route.effects'; export const coreEffects = [ RequestEffects, ObjectCacheEffects, - UUIDIndexEffects, + IdentifierIndexEffects, AuthEffects, JsonPatchOperationsEffects, ServerSyncBufferEffects, diff --git a/src/app/core/data/comcol-data.service.spec.ts b/src/app/core/data/comcol-data.service.spec.ts index b5232b0bff..de17d7a39f 100644 --- a/src/app/core/data/comcol-data.service.spec.ts +++ b/src/app/core/data/comcol-data.service.spec.ts @@ -95,7 +95,7 @@ describe('ComColDataService', () => { function initMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCache', { - getObjectByUUID: cold('d-', { + getObjectByID: cold('d-', { d: { _links: { [LINK_NAME]: scopedEndpoint @@ -160,7 +160,7 @@ describe('ComColDataService', () => { it('should fetch the scope Community from the cache', () => { scheduler.schedule(() => service.getBrowseEndpoint(options).subscribe()); scheduler.flush(); - expect(objectCache.getObjectByUUID).toHaveBeenCalledWith(scopeID); + expect(objectCache.getObjectByID).toHaveBeenCalledWith(scopeID); }); it('should return the endpoint to fetch resources within the given scope', () => { diff --git a/src/app/core/data/comcol-data.service.ts b/src/app/core/data/comcol-data.service.ts index 68eb3e4880..3059d568df 100644 --- a/src/app/core/data/comcol-data.service.ts +++ b/src/app/core/data/comcol-data.service.ts @@ -49,7 +49,7 @@ export abstract class ComColDataService extends DataS ); const successResponses = responses.pipe( filter((response) => response.isSuccessful), - mergeMap(() => this.objectCache.getObjectByUUID(options.scopeID)), + mergeMap(() => this.objectCache.getObjectByID(options.scopeID)), map((nc: NormalizedCommunity) => nc._links[linkPath]), filter((href) => isNotEmpty(href)) ); diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index ad0db51980..0f7ca74d15 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -37,6 +37,7 @@ import { NormalizedObjectBuildService } from '../cache/builders/normalized-objec import { ChangeAnalyzer } from './change-analyzer'; import { RestRequestMethod } from './rest-request-method'; import { getMapsToType } from '../cache/builders/build-decorators'; +import { IdentifierType } from '../index/index.reducer'; export abstract class DataService { protected abstract requestService: RequestService; @@ -146,14 +147,21 @@ export abstract class DataService { return `${endpoint}/${resourceID}`; } - findById(id: string): Observable> { - const hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => this.getIDHref(endpoint, id))); - + findById(id: string, identifierType: IdentifierType = IdentifierType.UUID): Observable> { + let hrefObs; + if (identifierType === IdentifierType.UUID) { + hrefObs = this.halService.getEndpoint(this.linkPath).pipe( + map((endpoint: string) => this.getIDHref(endpoint, id))); + } else if (identifierType === IdentifierType.HANDLE) { + hrefObs = this.halService.getEndpoint(this.linkPath).pipe( + map((endpoint: string) => { + return this.getIDHref(endpoint, encodeURIComponent(id)); + })); + } hrefObs.pipe( find((href: string) => hasValue(href))) .subscribe((href: string) => { - const request = new FindByIDRequest(this.requestService.generateRequestId(), href, id); + const request = new FindByIDRequest(this.requestService.generateRequestId(), href, id, identifierType); this.requestService.configure(request, this.forceBypassCache); }); diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts new file mode 100644 index 0000000000..ece3c242fc --- /dev/null +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -0,0 +1,112 @@ +import { cold, getTestScheduler } from 'jasmine-marbles'; +import { TestScheduler } from 'rxjs/testing'; +import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import { DSpaceObject } from '../shared/dspace-object.model'; +import { HALEndpointService } from '../shared/hal-endpoint.service'; +import { FindByIDRequest } from './request.models'; +import { RequestService } from './request.service'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { HttpClient } from '@angular/common/http'; +import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; +import { IdentifierType } from '../index/index.reducer'; +import { DsoDataRedirectService } from './dso-data-redirect.service'; +import { Router } from '@angular/router'; +import { Store } from '@ngrx/store'; +import { CoreState } from '../core.reducers'; + +describe('DsoDataRedirectService', () => { + let scheduler: TestScheduler; + let service: DsoDataRedirectService; + let halService: HALEndpointService; + let requestService: RequestService; + let rdbService: RemoteDataBuildService; + let objectCache: ObjectCacheService; + let router: Router; + let remoteData; + const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; + const dsoHandle = '1234567789/22'; + const encodedHandle = encodeURIComponent(dsoHandle); + const pidLink = 'https://rest.api/rest/api/pid/find{?id}'; + const requestHandleURL = `https://rest.api/rest/api/pid/find?id=${encodedHandle}`; + const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; + const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; + const testObject = { + uuid: dsoUUID + } as DSpaceObject; + + beforeEach(() => { + scheduler = getTestScheduler(); + + halService = jasmine.createSpyObj('halService', { + getEndpoint: cold('a', { a: pidLink }) + }); + requestService = jasmine.createSpyObj('requestService', { + generateRequestId: requestUUID, + configure: true + }); + rdbService = jasmine.createSpyObj('rdbService', { + buildSingle: cold('a', { + a: { + payload: testObject + } + }) + }); + router = jasmine.createSpyObj('router', { + navigate: () => true + }); + remoteData = { + isSuccessful: true, + error: undefined, + hasSucceeded: true, + payload: { + type: 'item', + id: '123456789' + } + }; + objectCache = {} as ObjectCacheService; + const store = {} as Store; + const notificationsService = {} as NotificationsService; + const http = {} as HttpClient; + const comparator = {} as any; + const dataBuildService = {} as NormalizedObjectBuildService; + + service = new DsoDataRedirectService( + requestService, + rdbService, + dataBuildService, + store, + objectCache, + halService, + notificationsService, + http, + comparator, + router + ); + }); + + describe('findById', () => { + it('should call HALEndpointService with the path to the dso endpoint', () => { + scheduler.schedule(() => service.findById(dsoUUID)); + scheduler.flush(); + + expect(halService.getEndpoint).toHaveBeenCalledWith('pid'); + }); + + it('should configure the proper FindByIDRequest for uuid', () => { + scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); + scheduler.flush(); + + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestUUIDURL, dsoUUID, IdentifierType.UUID), false); + }); + + it('should configure the proper FindByIDRequest for handle', () => { + scheduler.schedule(() => service.findById(dsoHandle, IdentifierType.HANDLE)); + scheduler.flush(); + + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); + }); + + // TODO: test for router.navigate + }); +}); diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-data-redirect.service.ts new file mode 100644 index 0000000000..b568a23c8a --- /dev/null +++ b/src/app/core/data/dso-data-redirect.service.ts @@ -0,0 +1,78 @@ +import { DataService } from './data.service'; +import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; +import { HALEndpointService } from '../shared/hal-endpoint.service'; +import { HttpClient } from '@angular/common/http'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import { RequestService } from './request.service'; +import { Store } from '@ngrx/store'; +import { CoreState } from '../core.reducers'; +import { FindAllOptions, FindByIDRequest } from './request.models'; +import { Observable, of } from 'rxjs'; +import { IdentifierType } from '../index/index.reducer'; +import { RemoteData } from './remote-data'; +import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; +import { Injectable } from '@angular/core'; +import { map, tap } from 'rxjs/operators'; +import { hasValue } from '../../shared/empty.util'; +import { getFinishedRemoteData, getSucceededRemoteData } from '../shared/operators'; +import { Router } from '@angular/router'; + +@Injectable() +export class DsoDataRedirectService extends DataService { + + protected linkPath = 'pid'; + protected forceBypassCache = false; + + constructor( + protected requestService: RequestService, + protected rdbService: RemoteDataBuildService, + protected dataBuildService: NormalizedObjectBuildService, + protected store: Store, + protected objectCache: ObjectCacheService, + protected halService: HALEndpointService, + protected notificationsService: NotificationsService, + protected http: HttpClient, + protected comparator: DSOChangeAnalyzer, + private router: Router) { + super(); + } + + getBrowseEndpoint(options: FindAllOptions = {}, linkPath: string = this.linkPath): Observable { + return this.halService.getEndpoint(linkPath); + } + + getIDHref(endpoint, resourceID): string { + return endpoint.replace(/\{\?id\}/,`?id=${resourceID}`); + } + + findById(id: string, identifierType = IdentifierType.UUID): Observable> { + return super.findById(id, identifierType).pipe( + getFinishedRemoteData(), + tap((response) => { + if (response.hasSucceeded) { + const uuid = response.payload.uuid; + // Is there an existing method somewhere that converts dso type to endpoint? + // This will not work for all endpoints! + const dsoType = this.getEndpointFromDSOType(response.payload.type); + if (hasValue(uuid) && hasValue(dsoType)) { + this.router.navigate([dsoType + '/' + uuid]); + } + } + }) + ); + } + + getEndpointFromDSOType(dsoType: string): string { + if (dsoType.startsWith('item')) { + return 'items' + } else if (dsoType.startsWith('community')) { + return 'communities'; + } else if (dsoType.startsWith('collection')) { + return 'collections' + } else { + return ''; + } + } +} diff --git a/src/app/core/data/dspace-object-data.service.spec.ts b/src/app/core/data/dspace-object-data.service.spec.ts index a0bba214ae..b411028420 100644 --- a/src/app/core/data/dspace-object-data.service.spec.ts +++ b/src/app/core/data/dspace-object-data.service.spec.ts @@ -10,6 +10,7 @@ import { ObjectCacheService } from '../cache/object-cache.service'; import { NotificationsService } from '../../shared/notifications/notifications.service'; import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; +import { IdentifierType } from '../index/index.reducer'; describe('DSpaceObjectDataService', () => { let scheduler: TestScheduler; @@ -72,7 +73,7 @@ describe('DSpaceObjectDataService', () => { scheduler.schedule(() => service.findById(testObject.uuid)); scheduler.flush(); - expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestURL, testObject.uuid), false); + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestURL, testObject.uuid, IdentifierType.UUID), false); }); it('should return a RemoteData for the object with the given ID', () => { diff --git a/src/app/core/data/request.models.ts b/src/app/core/data/request.models.ts index 43ab9e58e7..45e8fdea16 100644 --- a/src/app/core/data/request.models.ts +++ b/src/app/core/data/request.models.ts @@ -19,6 +19,7 @@ import { MetadatafieldParsingService } from './metadatafield-parsing.service'; import { URLCombiner } from '../url-combiner/url-combiner'; import { TaskResponseParsingService } from '../tasks/task-response-parsing.service'; import { MappedCollectionsReponseParsingService } from './mapped-collections-reponse-parsing.service'; +import { IdentifierType } from '../index/index.reducer'; /* tslint:disable:max-classes-per-file */ @@ -49,7 +50,7 @@ export class GetRequest extends RestRequest { public uuid: string, public href: string, public body?: any, - public options?: HttpOptions, + public options?: HttpOptions ) { super(uuid, href, RestRequestMethod.GET, body, options) } @@ -125,7 +126,8 @@ export class FindByIDRequest extends GetRequest { constructor( uuid: string, href: string, - public resourceID: string + public resourceID: string, + public identifierType?: IdentifierType ) { super(uuid, href); } diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 0980d48537..ac65042238 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -11,7 +11,7 @@ import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; import { ObjectCacheService } from '../cache/object-cache.service'; import { CoreState } from '../core.reducers'; -import { IndexName, IndexState, MetaIndexState } from '../index/index.reducer'; +import { IdentifierType, IndexState, MetaIndexState, REQUEST, UUID_MAPPING } from '../index/index.reducer'; import { originalRequestUUIDFromRequestUUIDSelector, requestIndexSelector, @@ -19,7 +19,7 @@ import { } from '../index/index.selectors'; import { UUIDService } from '../shared/uuid.service'; import { RequestConfigureAction, RequestExecuteAction, RequestRemoveAction } from './request.actions'; -import { GetRequest, RestRequest } from './request.models'; +import { FindByIDRequest, GetRequest, RestRequest } from './request.models'; import { RequestEntry, RequestState } from './request.reducer'; import { CommitSSBAction } from '../cache/server-sync-buffer.actions'; import { RestRequestMethod } from './rest-request-method'; @@ -162,7 +162,7 @@ export class RequestService { filter((entry) => hasValue(entry)), take(1) ).subscribe((entry) => { - return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) + return this.store.dispatch(new AddToIndexAction(UUID_MAPPING, request.uuid, entry.request.uuid)) } ) } @@ -206,7 +206,7 @@ export class RequestService { } }); this.requestsOnTheirWayToTheStore = this.requestsOnTheirWayToTheStore.filter((reqHref: string) => reqHref.indexOf(href) < 0); - this.indexStore.dispatch(new RemoveFromIndexBySubstringAction(IndexName.REQUEST, href)); + this.indexStore.dispatch(new RemoveFromIndexBySubstringAction(REQUEST, href)); } /** @@ -225,8 +225,14 @@ export class RequestService { private isCachedOrPending(request: GetRequest): boolean { const inReqCache = this.hasByHref(request.href); const inObjCache = this.objectCache.hasBySelfLink(request.href); - const isCached = inReqCache || inObjCache; - + let inObjIdCache = false; + if (request instanceof FindByIDRequest) { + const req = request as FindByIDRequest; + if (hasValue(req.identifierType && hasValue(req.resourceID))) { + inObjIdCache = this.objectCache.hasById(req.resourceID, req.identifierType) + } + } + const isCached = inReqCache || inObjCache || inObjIdCache; const isPending = this.isPending(request); return isCached || isPending; } diff --git a/src/app/core/index/index.actions.ts b/src/app/core/index/index.actions.ts index 42804dbe26..24f031b33c 100644 --- a/src/app/core/index/index.actions.ts +++ b/src/app/core/index/index.actions.ts @@ -1,7 +1,7 @@ import { Action } from '@ngrx/store'; import { type } from '../../shared/ngrx/type'; -import { IndexName } from './index.reducer'; +import { } from './index.reducer'; /** * The list of HrefIndexAction type definitions @@ -19,7 +19,7 @@ export const IndexActionTypes = { export class AddToIndexAction implements Action { type = IndexActionTypes.ADD; payload: { - name: IndexName; + name: string; value: string; key: string; }; @@ -34,7 +34,7 @@ export class AddToIndexAction implements Action { * @param value * the self link of the resource the key belongs to */ - constructor(name: IndexName, key: string, value: string) { + constructor(name: string, key: string, value: string) { this.payload = { name, key, value }; } } @@ -45,7 +45,7 @@ export class AddToIndexAction implements Action { export class RemoveFromIndexByValueAction implements Action { type = IndexActionTypes.REMOVE_BY_VALUE; payload: { - name: IndexName, + name: string, value: string }; @@ -57,7 +57,7 @@ export class RemoveFromIndexByValueAction implements Action { * @param value * the value to remove the UUID for */ - constructor(name: IndexName, value: string) { + constructor(name: string, value: string) { this.payload = { name, value }; } @@ -69,7 +69,7 @@ export class RemoveFromIndexByValueAction implements Action { export class RemoveFromIndexBySubstringAction implements Action { type = IndexActionTypes.REMOVE_BY_SUBSTRING; payload: { - name: IndexName, + name: string, value: string }; @@ -81,7 +81,7 @@ export class RemoveFromIndexBySubstringAction implements Action { * @param value * the value to remove the UUID for */ - constructor(name: IndexName, value: string) { + constructor(name: string, value: string) { this.payload = { name, value }; } diff --git a/src/app/core/index/index.effects.ts b/src/app/core/index/index.effects.ts index 61cf313ab1..0cdf6bea6c 100644 --- a/src/app/core/index/index.effects.ts +++ b/src/app/core/index/index.effects.ts @@ -10,43 +10,67 @@ import { import { RequestActionTypes, RequestConfigureAction } from '../data/request.actions'; import { AddToIndexAction, RemoveFromIndexByValueAction } from './index.actions'; import { hasValue } from '../../shared/empty.util'; -import { IndexName } from './index.reducer'; +import { getIdentiferByIndexName, IdentifierType, REQUEST } from './index.reducer'; import { RestRequestMethod } from '../data/rest-request-method'; @Injectable() -export class UUIDIndexEffects { +export class IdentifierIndexEffects { - @Effect() addObject$ = this.actions$ + @Effect() addObjectByUUID$ = this.actions$ .pipe( ofType(ObjectCacheActionTypes.ADD), filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.uuid)), map((action: AddToObjectCacheAction) => { return new AddToIndexAction( - IndexName.OBJECT, + getIdentiferByIndexName(IdentifierType.UUID), action.payload.objectToCache.uuid, action.payload.objectToCache.self ); }) ); - @Effect() removeObject$ = this.actions$ + @Effect() addObjectByHandle$ = this.actions$ + .pipe( + ofType(ObjectCacheActionTypes.ADD), + filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.handle)), + map((action: AddToObjectCacheAction) => { + return new AddToIndexAction( + getIdentiferByIndexName(IdentifierType.HANDLE), + action.payload.objectToCache.handle, + action.payload.objectToCache.self + ); + }) + ); + + @Effect() removeObjectByUUID$ = this.actions$ .pipe( ofType(ObjectCacheActionTypes.REMOVE), map((action: RemoveFromObjectCacheAction) => { return new RemoveFromIndexByValueAction( - IndexName.OBJECT, + getIdentiferByIndexName(IdentifierType.UUID), action.payload ); }) ); + @Effect() removeObjectByHandle$ = this.actions$ + .pipe( + ofType(ObjectCacheActionTypes.REMOVE), + map((action: RemoveFromObjectCacheAction) => { + return new RemoveFromIndexByValueAction( + getIdentiferByIndexName(IdentifierType.HANDLE), + action.payload + ); + }) + ); + @Effect() addRequest$ = this.actions$ .pipe( ofType(RequestActionTypes.CONFIGURE), filter((action: RequestConfigureAction) => action.payload.method === RestRequestMethod.GET), map((action: RequestConfigureAction) => { return new AddToIndexAction( - IndexName.REQUEST, + REQUEST, action.payload.href, action.payload.uuid ); diff --git a/src/app/core/index/index.reducer.spec.ts b/src/app/core/index/index.reducer.spec.ts index ef46c760c6..35460a9ef5 100644 --- a/src/app/core/index/index.reducer.spec.ts +++ b/src/app/core/index/index.reducer.spec.ts @@ -1,6 +1,6 @@ import * as deepFreeze from 'deep-freeze'; -import { IndexName, indexReducer, MetaIndexState } from './index.reducer'; +import { getIdentiferByIndexName, IdentifierType, indexReducer, MetaIndexState, REQUEST, } from './index.reducer'; import { AddToIndexAction, RemoveFromIndexBySubstringAction, RemoveFromIndexByValueAction } from './index.actions'; class NullAction extends AddToIndexAction { @@ -15,14 +15,19 @@ class NullAction extends AddToIndexAction { describe('requestReducer', () => { const key1 = '567a639f-f5ff-4126-807c-b7d0910808c8'; const key2 = '1911e8a4-6939-490c-b58b-a5d70f8d91fb'; + const key3 = '123456789/22'; const val1 = 'https://dspace7.4science.it/dspace-spring-rest/api/core/items/567a639f-f5ff-4126-807c-b7d0910808c8'; const val2 = 'https://dspace7.4science.it/dspace-spring-rest/api/core/items/1911e8a4-6939-490c-b58b-a5d70f8d91fb'; + const uuidIndex = getIdentiferByIndexName(IdentifierType.UUID); + const handleIndex = getIdentiferByIndexName(IdentifierType.HANDLE); const testState: MetaIndexState = { - [IndexName.OBJECT]: { + 'object/uuid-to-self-link/uuid': { [key1]: val1 - },[IndexName.REQUEST]: { + },'object/uuid-to-self-link/handle': { + [key3]: val1 + },'get-request/href-to-uuid': { [key1]: val1 - },[IndexName.UUID_MAPPING]: { + },'get-request/configured-to-cache-uuid': { [key1]: val1 } }; @@ -45,27 +50,38 @@ describe('requestReducer', () => { it('should add the \'key\' with the corresponding \'value\' to the correct substate, in response to an ADD action', () => { const state = testState; - const action = new AddToIndexAction(IndexName.REQUEST, key2, val2); + const action = new AddToIndexAction(REQUEST, key2, val2); const newState = indexReducer(state, action); - expect(newState[IndexName.REQUEST][key2]).toEqual(val2); + expect(newState[REQUEST][key2]).toEqual(val2); }); it('should remove the given \'value\' from its corresponding \'key\' in the correct substate, in response to a REMOVE_BY_VALUE action', () => { const state = testState; - const action = new RemoveFromIndexByValueAction(IndexName.OBJECT, val1); - const newState = indexReducer(state, action); + let action = new RemoveFromIndexByValueAction(uuidIndex, val1); + let newState = indexReducer(state, action); + + expect(newState[uuidIndex][key1]).toBeUndefined(); + + action = new RemoveFromIndexByValueAction(handleIndex, val1); + newState = indexReducer(state, action); + + expect(newState[handleIndex][key3]).toBeUndefined(); - expect(newState[IndexName.OBJECT][key1]).toBeUndefined(); }); it('should remove the given \'value\' from its corresponding \'key\' in the correct substate, in response to a REMOVE_BY_SUBSTRING action', () => { const state = testState; - const action = new RemoveFromIndexBySubstringAction(IndexName.OBJECT, key1); - const newState = indexReducer(state, action); + let action = new RemoveFromIndexBySubstringAction(uuidIndex, key1); + let newState = indexReducer(state, action); - expect(newState[IndexName.OBJECT][key1]).toBeUndefined(); + expect(newState[uuidIndex][key1]).toBeUndefined(); + + action = new RemoveFromIndexBySubstringAction(handleIndex, key3); + newState = indexReducer(state, action); + + expect(newState[uuidIndex][key3]).toBeUndefined(); }); }); diff --git a/src/app/core/index/index.reducer.ts b/src/app/core/index/index.reducer.ts index b4cd8aa84b..631d579911 100644 --- a/src/app/core/index/index.reducer.ts +++ b/src/app/core/index/index.reducer.ts @@ -6,24 +6,26 @@ import { RemoveFromIndexByValueAction } from './index.actions'; -/** - * An enum containing all index names - */ -export enum IndexName { - // Contains all objects in the object cache indexed by UUID - OBJECT = 'object/uuid-to-self-link', - - // contains all requests in the request cache indexed by UUID - REQUEST = 'get-request/href-to-uuid', - - /** - * Contains the UUIDs of requests that were sent to the server and - * have their responses cached, indexed by the UUIDs of requests that - * weren't sent because the response they requested was already cached - */ - UUID_MAPPING = 'get-request/configured-to-cache-uuid' +export enum IdentifierType { + UUID ='uuid', + HANDLE = 'handle' } +/** + * Contains the UUIDs of requests that were sent to the server and + * have their responses cached, indexed by the UUIDs of requests that + * weren't sent because the response they requested was already cached + */ +export const UUID_MAPPING = 'get-request/configured-to-cache-uuid'; + +// contains all requests in the request cache indexed by UUID +export const REQUEST = 'get-request/href-to-uuid'; + +// returns the index for the provided id type (uuid, handle) +export const getIdentiferByIndexName = (idType: IdentifierType): string => { + return `object/uuid-to-self-link/${idType}`; +}; + /** * The state of a single index */ @@ -34,8 +36,11 @@ export interface IndexState { /** * The state that contains all indices */ -export type MetaIndexState = { - [name in IndexName]: IndexState +export interface MetaIndexState { + 'get-request/configured-to-cache-uuid': IndexState, + 'get-request/href-to-uuid': IndexState, + 'object/uuid-to-self-link/uuid': IndexState, + 'object/uuid-to-self-link/handle': IndexState } // Object.create(null) ensures the object has no default js properties (e.g. `__proto__`) diff --git a/src/app/core/index/index.selectors.ts b/src/app/core/index/index.selectors.ts index 3c7b331a92..80a7c0d46a 100644 --- a/src/app/core/index/index.selectors.ts +++ b/src/app/core/index/index.selectors.ts @@ -3,7 +3,14 @@ import { AppState } from '../../app.reducer'; import { hasValue } from '../../shared/empty.util'; import { CoreState } from '../core.reducers'; import { coreSelector } from '../core.selectors'; -import { IndexName, IndexState, MetaIndexState } from './index.reducer'; +import { + getIdentiferByIndexName, + IdentifierType, + IndexState, + MetaIndexState, + REQUEST, + UUID_MAPPING +} from './index.reducer'; /** * Return the MetaIndexState based on the CoreSate @@ -20,13 +27,17 @@ export const metaIndexSelector: MemoizedSelector = cre * Return the object index based on the MetaIndexState * It contains all objects in the object cache indexed by UUID * + * @param identifierType the type of index, used to select index from state + * * @returns * a MemoizedSelector to select the object index */ -export const objectIndexSelector: MemoizedSelector = createSelector( - metaIndexSelector, - (state: MetaIndexState) => state[IndexName.OBJECT] -); +export const objectIndexSelector = (identifierType: IdentifierType = IdentifierType.UUID): MemoizedSelector => { + return createSelector( + metaIndexSelector, + (state: MetaIndexState) => state[getIdentiferByIndexName(identifierType)] + ); +} /** * Return the request index based on the MetaIndexState @@ -36,7 +47,7 @@ export const objectIndexSelector: MemoizedSelector = creat */ export const requestIndexSelector: MemoizedSelector = createSelector( metaIndexSelector, - (state: MetaIndexState) => state[IndexName.REQUEST] + (state: MetaIndexState) => state[REQUEST] ); /** @@ -47,21 +58,22 @@ export const requestIndexSelector: MemoizedSelector = crea */ export const requestUUIDIndexSelector: MemoizedSelector = createSelector( metaIndexSelector, - (state: MetaIndexState) => state[IndexName.UUID_MAPPING] + (state: MetaIndexState) => state[UUID_MAPPING] ); /** * Return the self link of an object in the object-cache based on its UUID * - * @param uuid + * @param id * the UUID for which you want to find the matching self link + * @param identifierType the type of index, used to select index from state * @returns * a MemoizedSelector to select the self link */ export const selfLinkFromUuidSelector = - (uuid: string): MemoizedSelector => createSelector( - objectIndexSelector, - (state: IndexState) => hasValue(state) ? state[uuid] : undefined + (id: string, identifierType: IdentifierType = IdentifierType.UUID): MemoizedSelector => createSelector( + objectIndexSelector(identifierType), + (state: IndexState) => hasValue(state) ? state[id] : undefined ); /** From 5ec9b7c29f4c90f1604613e123d140e14b4150a9 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Mon, 30 Sep 2019 16:57:06 -0700 Subject: [PATCH 08/22] Added new unit tests. --- .../objectnotfound.component.spec.ts | 79 +++++++++++++++++++ .../data/dso-data-redirect.service.spec.ts | 35 ++++---- .../core/data/dso-data-redirect.service.ts | 6 +- 3 files changed, 100 insertions(+), 20 deletions(-) create mode 100644 src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts new file mode 100644 index 0000000000..7905655a06 --- /dev/null +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.spec.ts @@ -0,0 +1,79 @@ +import { NO_ERRORS_SCHEMA } from '@angular/core'; +import { async, ComponentFixture, TestBed } from '@angular/core/testing'; +import { TranslateModule } from '@ngx-translate/core'; + +import { ObjectNotFoundComponent } from './objectnotfound.component'; +import { ActivatedRouteStub } from '../../shared/testing/active-router-stub'; +import { of as observableOf } from 'rxjs'; +import { ActivatedRoute } from '@angular/router'; + +describe('ObjectNotFoundComponent', () => { + let comp: ObjectNotFoundComponent; + let fixture: ComponentFixture; + const testUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; + const uuidType = 'uuid'; + const handlePrefix = '123456789'; + const handleId = '22'; + const activatedRouteStub = Object.assign(new ActivatedRouteStub(), { + params: observableOf({id: testUUID, idType: uuidType}) + }); + const activatedRouteStubHandle = Object.assign(new ActivatedRouteStub(), { + params: observableOf({id: handleId, idType: handlePrefix}) + }); + describe('uuid request', () => { + beforeEach(async(() => { + TestBed.configureTestingModule({ + imports: [ + TranslateModule.forRoot() + ], providers: [ + {provide: ActivatedRoute, useValue: activatedRouteStub} + ], + declarations: [ObjectNotFoundComponent], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); + + beforeEach(() => { + fixture = TestBed.createComponent(ObjectNotFoundComponent); + comp = fixture.componentInstance; + fixture.detectChanges(); + }); + + it('should create instance', () => { + expect(comp).toBeDefined() + }); + + it('should have id and idType', () => { + expect(comp.id).toEqual(testUUID); + expect(comp.idType).toEqual(uuidType); + expect(comp.missingItem).toEqual('uuid: ' + testUUID); + }); + }); + + describe( 'legacy handle request', () => { + beforeEach(async(() => { + TestBed.configureTestingModule({ + imports: [ + TranslateModule.forRoot() + ], providers: [ + {provide: ActivatedRoute, useValue: activatedRouteStubHandle} + ], + declarations: [ObjectNotFoundComponent], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); + + beforeEach(() => { + fixture = TestBed.createComponent(ObjectNotFoundComponent); + comp = fixture.componentInstance; + fixture.detectChanges(); + }); + + it('should have handle prefix and id', () => { + expect(comp.id).toEqual(handleId); + expect(comp.idType).toEqual(handlePrefix); + expect(comp.missingItem).toEqual('handle: ' + handlePrefix + '/' + handleId); + }); + }); + +}); diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts index ece3c242fc..040cecc435 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -1,7 +1,6 @@ import { cold, getTestScheduler } from 'jasmine-marbles'; import { TestScheduler } from 'rxjs/testing'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; -import { DSpaceObject } from '../shared/dspace-object.model'; import { HALEndpointService } from '../shared/hal-endpoint.service'; import { FindByIDRequest } from './request.models'; import { RequestService } from './request.service'; @@ -11,7 +10,6 @@ import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; import { IdentifierType } from '../index/index.reducer'; import { DsoDataRedirectService } from './dso-data-redirect.service'; -import { Router } from '@angular/router'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; @@ -22,7 +20,7 @@ describe('DsoDataRedirectService', () => { let requestService: RequestService; let rdbService: RemoteDataBuildService; let objectCache: ObjectCacheService; - let router: Router; + let router; let remoteData; const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; const dsoHandle = '1234567789/22'; @@ -31,15 +29,12 @@ describe('DsoDataRedirectService', () => { const requestHandleURL = `https://rest.api/rest/api/pid/find?id=${encodedHandle}`; const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; - const testObject = { - uuid: dsoUUID - } as DSpaceObject; beforeEach(() => { scheduler = getTestScheduler(); halService = jasmine.createSpyObj('halService', { - getEndpoint: cold('a', { a: pidLink }) + getEndpoint: cold('a', {a: pidLink}) }); requestService = jasmine.createSpyObj('requestService', { generateRequestId: requestUUID, @@ -47,21 +42,20 @@ describe('DsoDataRedirectService', () => { }); rdbService = jasmine.createSpyObj('rdbService', { buildSingle: cold('a', { - a: { - payload: testObject - } + a: remoteData }) }); - router = jasmine.createSpyObj('router', { - navigate: () => true - }); + router = { + navigate: jasmine.createSpy('navigate') + }; remoteData = { isSuccessful: true, error: undefined, hasSucceeded: true, + isLoading: false, payload: { type: 'item', - id: '123456789' + uuid: '123456789' } }; objectCache = {} as ObjectCacheService; @@ -86,7 +80,7 @@ describe('DsoDataRedirectService', () => { }); describe('findById', () => { - it('should call HALEndpointService with the path to the dso endpoint', () => { + it('should call HALEndpointService with the path to the pid endpoint', () => { scheduler.schedule(() => service.findById(dsoUUID)); scheduler.flush(); @@ -107,6 +101,13 @@ describe('DsoDataRedirectService', () => { expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); }); - // TODO: test for router.navigate - }); + it('should navigate to dso route', () => { + const redir = service.findById(dsoHandle, IdentifierType.HANDLE); + // The framework would normally subscribe but do it here so we can test navigation. + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(router.navigate).toHaveBeenCalledWith([remoteData.payload.type + 's/' + remoteData.payload.uuid]); + }); + }) }); diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-data-redirect.service.ts index b568a23c8a..3d779de5cd 100644 --- a/src/app/core/data/dso-data-redirect.service.ts +++ b/src/app/core/data/dso-data-redirect.service.ts @@ -9,14 +9,14 @@ import { RequestService } from './request.service'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; import { FindAllOptions, FindByIDRequest } from './request.models'; -import { Observable, of } from 'rxjs'; +import { Observable } from 'rxjs'; import { IdentifierType } from '../index/index.reducer'; import { RemoteData } from './remote-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; import { Injectable } from '@angular/core'; -import { map, tap } from 'rxjs/operators'; +import { tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; -import { getFinishedRemoteData, getSucceededRemoteData } from '../shared/operators'; +import { getFinishedRemoteData } from '../shared/operators'; import { Router } from '@angular/router'; @Injectable() From a1d21cd6af79d5bcf82d0ad11df632dbf48abcb5 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 01:06:12 -0700 Subject: [PATCH 09/22] Minor change to comment. --- src/app/core/data/dso-data-redirect.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-data-redirect.service.ts index 3d779de5cd..1cb3b20f2a 100644 --- a/src/app/core/data/dso-data-redirect.service.ts +++ b/src/app/core/data/dso-data-redirect.service.ts @@ -53,8 +53,7 @@ export class DsoDataRedirectService extends DataService { tap((response) => { if (response.hasSucceeded) { const uuid = response.payload.uuid; - // Is there an existing method somewhere that converts dso type to endpoint? - // This will not work for all endpoints! + // Is there an existing method somewhere that converts dso type route? const dsoType = this.getEndpointFromDSOType(response.payload.type); if (hasValue(uuid) && hasValue(dsoType)) { this.router.navigate([dsoType + '/' + uuid]); @@ -65,6 +64,7 @@ export class DsoDataRedirectService extends DataService { } getEndpointFromDSOType(dsoType: string): string { + // Are there other routes to consider? if (dsoType.startsWith('item')) { return 'items' } else if (dsoType.startsWith('community')) { From 2aaab8342773e63b5db6314e603b36a0d159a170 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 03:03:08 -0700 Subject: [PATCH 10/22] Added unit tests. --- src/app/+lookup-by-id/lookup-guard.spec.ts | 50 ++++++++++++ src/app/+lookup-by-id/lookup-guard.ts | 2 +- .../data/dso-data-redirect.service.spec.ts | 81 +++++++++++++------ 3 files changed, 106 insertions(+), 27 deletions(-) create mode 100644 src/app/+lookup-by-id/lookup-guard.spec.ts diff --git a/src/app/+lookup-by-id/lookup-guard.spec.ts b/src/app/+lookup-by-id/lookup-guard.spec.ts new file mode 100644 index 0000000000..ea077d0811 --- /dev/null +++ b/src/app/+lookup-by-id/lookup-guard.spec.ts @@ -0,0 +1,50 @@ +import {LookupGuard} from "./lookup-guard"; +import {of as observableOf} from "rxjs"; +import {IdentifierType} from "../core/index/index.reducer"; + +describe('LookupGuard', () => { + let dsoService: any; + let guard: any; + + beforeEach(() => { + dsoService = { + findById: jasmine.createSpy('findById').and.returnValue(observableOf({ hasFailed: false, + hasSucceeded: true })) + }; + guard = new LookupGuard(dsoService); + }); + + it('should call findById with handle params', () => { + const scopedRoute = { + params: { + id: '1234', + idType: '123456789' + } + }; + guard.canActivate(scopedRoute as any, undefined); + expect(dsoService.findById).toHaveBeenCalledWith('123456789%2F1234', IdentifierType.HANDLE) + }); + + it('should call findById with handle params', () => { + const scopedRoute = { + params: { + id: '123456789%2F1234', + idType: 'handle' + } + }; + guard.canActivate(scopedRoute as any, undefined); + expect(dsoService.findById).toHaveBeenCalledWith('123456789%2F1234', IdentifierType.HANDLE) + }); + + it('should call findById with UUID params', () => { + const scopedRoute = { + params: { + id: '34cfed7c-f597-49ef-9cbe-ea351f0023c2', + idType: 'uuid' + } + }; + guard.canActivate(scopedRoute as any, undefined); + expect(dsoService.findById).toHaveBeenCalledWith('34cfed7c-f597-49ef-9cbe-ea351f0023c2', IdentifierType.UUID) + }); + +}); diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts index 61e3688ee2..ceb11b7cf5 100644 --- a/src/app/+lookup-by-id/lookup-guard.ts +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -14,7 +14,7 @@ interface LookupParams { @Injectable() export class LookupGuard implements CanActivate { - constructor(private dsoService: DsoDataRedirectService, private router: Router) { + constructor(private dsoService: DsoDataRedirectService) { } canActivate(route: ActivatedRouteSnapshot, state:RouterStateSnapshot): Observable { diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts index 040cecc435..a9a83e72d6 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -19,7 +19,6 @@ describe('DsoDataRedirectService', () => { let halService: HALEndpointService; let requestService: RequestService; let rdbService: RemoteDataBuildService; - let objectCache: ObjectCacheService; let router; let remoteData; const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; @@ -29,7 +28,13 @@ describe('DsoDataRedirectService', () => { const requestHandleURL = `https://rest.api/rest/api/pid/find?id=${encodedHandle}`; const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; - + const store = {} as Store; + const notificationsService = {} as NotificationsService; + const http = {} as HttpClient; + const comparator = {} as any; + const dataBuildService = {} as NormalizedObjectBuildService; + const objectCache = {} as ObjectCacheService; + let setup; beforeEach(() => { scheduler = getTestScheduler(); @@ -40,14 +45,10 @@ describe('DsoDataRedirectService', () => { generateRequestId: requestUUID, configure: true }); - rdbService = jasmine.createSpyObj('rdbService', { - buildSingle: cold('a', { - a: remoteData - }) - }); router = { navigate: jasmine.createSpy('navigate') }; + remoteData = { isSuccessful: true, error: undefined, @@ -58,29 +59,31 @@ describe('DsoDataRedirectService', () => { uuid: '123456789' } }; - objectCache = {} as ObjectCacheService; - const store = {} as Store; - const notificationsService = {} as NotificationsService; - const http = {} as HttpClient; - const comparator = {} as any; - const dataBuildService = {} as NormalizedObjectBuildService; - service = new DsoDataRedirectService( - requestService, - rdbService, - dataBuildService, - store, - objectCache, - halService, - notificationsService, - http, - comparator, - router - ); + setup = () => { + rdbService = jasmine.createSpyObj('rdbService', { + buildSingle: cold('a', { + a: remoteData + }) + }); + service = new DsoDataRedirectService( + requestService, + rdbService, + dataBuildService, + store, + objectCache, + halService, + notificationsService, + http, + comparator, + router + ); + } }); describe('findById', () => { it('should call HALEndpointService with the path to the pid endpoint', () => { + setup(); scheduler.schedule(() => service.findById(dsoUUID)); scheduler.flush(); @@ -88,6 +91,7 @@ describe('DsoDataRedirectService', () => { }); it('should configure the proper FindByIDRequest for uuid', () => { + setup(); scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); scheduler.flush(); @@ -95,13 +99,16 @@ describe('DsoDataRedirectService', () => { }); it('should configure the proper FindByIDRequest for handle', () => { + setup(); scheduler.schedule(() => service.findById(dsoHandle, IdentifierType.HANDLE)); scheduler.flush(); expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); }); - it('should navigate to dso route', () => { + it('should navigate to item route', () => { + remoteData.payload.type = 'item'; + setup(); const redir = service.findById(dsoHandle, IdentifierType.HANDLE); // The framework would normally subscribe but do it here so we can test navigation. redir.subscribe(); @@ -109,5 +116,27 @@ describe('DsoDataRedirectService', () => { scheduler.flush(); expect(router.navigate).toHaveBeenCalledWith([remoteData.payload.type + 's/' + remoteData.payload.uuid]); }); + + it('should navigate to collections route', () => { + remoteData.payload.type = 'collection'; + setup(); + const redir = service.findById(dsoHandle, IdentifierType.HANDLE); + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(router.navigate).toHaveBeenCalledWith([remoteData.payload.type + 's/' + remoteData.payload.uuid]); + }); + + it('should navigate to communities route', () => { + remoteData.payload.type = 'community'; + setup(); + const redir = service.findById(dsoHandle, IdentifierType.HANDLE); + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(router.navigate).toHaveBeenCalledWith(['communities/' + remoteData.payload.uuid]); + }); }) }); + + From 5e40d7a4c167d8bd30d84d36f6c8e92567f23024 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 03:38:56 -0700 Subject: [PATCH 11/22] Fixed lint errors. --- src/app/+lookup-by-id/lookup-guard.spec.ts | 6 +++--- src/app/core/data/dso-data-redirect.service.spec.ts | 2 -- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/app/+lookup-by-id/lookup-guard.spec.ts b/src/app/+lookup-by-id/lookup-guard.spec.ts index ea077d0811..7b00383783 100644 --- a/src/app/+lookup-by-id/lookup-guard.spec.ts +++ b/src/app/+lookup-by-id/lookup-guard.spec.ts @@ -1,6 +1,6 @@ -import {LookupGuard} from "./lookup-guard"; -import {of as observableOf} from "rxjs"; -import {IdentifierType} from "../core/index/index.reducer"; +import { LookupGuard } from './lookup-guard'; +import { of as observableOf } from 'rxjs'; +import { IdentifierType } from '../core/index/index.reducer'; describe('LookupGuard', () => { let dsoService: any; diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-data-redirect.service.spec.ts index a9a83e72d6..f2c35727fa 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-data-redirect.service.spec.ts @@ -138,5 +138,3 @@ describe('DsoDataRedirectService', () => { }); }) }); - - From 2d49f3e765e73c1d838813d19406698a87d50436 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 1 Oct 2019 12:25:41 -0700 Subject: [PATCH 12/22] Added unit test for isCachedOrPending lookup by id (FindByIdRequest). --- src/app/core/data/request.service.spec.ts | 14 +++++++++++++- src/app/shared/mocks/mock-object-cache.service.ts | 4 ++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index e2bc04040f..5ceca8b4bd 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -11,6 +11,7 @@ import { UUIDService } from '../shared/uuid.service'; import { RequestConfigureAction, RequestExecuteAction } from './request.actions'; import { DeleteRequest, + FindByIDRequest, GetRequest, HeadRequest, OptionsRequest, @@ -21,6 +22,7 @@ import { } from './request.models'; import { RequestService } from './request.service'; import { TestScheduler } from 'rxjs/testing'; +import { IdentifierType } from '../index/index.reducer'; describe('RequestService', () => { let scheduler: TestScheduler; @@ -38,6 +40,7 @@ describe('RequestService', () => { const testDeleteRequest = new DeleteRequest(testUUID, testHref); const testOptionsRequest = new OptionsRequest(testUUID, testHref); const testHeadRequest = new HeadRequest(testUUID, testHref); + const testFindByIdRequest = new FindByIDRequest(testUUID, testHref, testUUID, IdentifierType.UUID); const testPatchRequest = new PatchRequest(testUUID, testHref); let selectSpy; @@ -298,13 +301,22 @@ describe('RequestService', () => { describe('in the ObjectCache', () => { beforeEach(() => { (objectCache.hasBySelfLink as any).and.returnValue(true); + (objectCache.hasById as any).and.returnValue(true); spyOn(serviceAsAny, 'hasByHref').and.returnValue(false); }); - it('should return true', () => { + it('should return true for GetRequest', () => { const result = serviceAsAny.isCachedOrPending(testGetRequest); const expected = true; + expect(result).toEqual(expected); + }); + it('should return true for instance of FindByIdRequest', () => { + (objectCache.hasBySelfLink as any).and.returnValue(false); + const result = serviceAsAny.isCachedOrPending(testFindByIdRequest); + expect(objectCache.hasById).toHaveBeenCalledWith(testUUID, IdentifierType.UUID); + const expected = true; + expect(result).toEqual(expected); }); }); diff --git a/src/app/shared/mocks/mock-object-cache.service.ts b/src/app/shared/mocks/mock-object-cache.service.ts index 9e35a519ff..d096cfdf5f 100644 --- a/src/app/shared/mocks/mock-object-cache.service.ts +++ b/src/app/shared/mocks/mock-object-cache.service.ts @@ -4,12 +4,12 @@ export function getMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCacheService', [ 'add', 'remove', - 'getByUUID', + 'getByID', 'getBySelfLink', 'getRequestHrefBySelfLink', 'getRequestHrefByUUID', 'getList', - 'hasByUUID', + 'hasById', 'hasBySelfLink' ]); From 85d179e27fcc4db74061d2b07af64171e4d975c9 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Tue, 15 Oct 2019 11:18:06 -0700 Subject: [PATCH 13/22] Bugfix for request by handle (removed unnecessary encoding) --- src/app/+lookup-by-id/lookup-guard.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts index ceb11b7cf5..7f38b6db7a 100644 --- a/src/app/+lookup-by-id/lookup-guard.ts +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -34,7 +34,7 @@ export class LookupGuard implements CanActivate { type = IdentifierType.HANDLE; const prefix = route.params.idType; const handleId = route.params.id; - id = `${prefix}%2F${handleId}`; + id = `${prefix}/${handleId}`; } else if (route.params.idType === IdentifierType.HANDLE) { type = IdentifierType.HANDLE; From b695da84878429076c68f497f047fcf7f10f91a5 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 16 Oct 2019 23:39:47 -0700 Subject: [PATCH 14/22] Encoding removed. --- src/app/+lookup-by-id/lookup-guard.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/+lookup-by-id/lookup-guard.spec.ts b/src/app/+lookup-by-id/lookup-guard.spec.ts index 7b00383783..824e31c62b 100644 --- a/src/app/+lookup-by-id/lookup-guard.spec.ts +++ b/src/app/+lookup-by-id/lookup-guard.spec.ts @@ -22,7 +22,7 @@ describe('LookupGuard', () => { } }; guard.canActivate(scopedRoute as any, undefined); - expect(dsoService.findById).toHaveBeenCalledWith('123456789%2F1234', IdentifierType.HANDLE) + expect(dsoService.findById).toHaveBeenCalledWith('123456789/1234', IdentifierType.HANDLE) }); it('should call findById with handle params', () => { From 6b26506d881910b2e18ad4d4e893de340341d3dc Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Thu, 17 Oct 2019 14:55:45 -0700 Subject: [PATCH 15/22] Added matcher function to router that supports handle paths (eliminates the need to encode the forward slash). Fixed comment. --- .../lookup-by-id-routing.module.ts | 28 +++++++++++++++++-- src/app/+lookup-by-id/lookup-by-id.module.ts | 4 +-- src/app/+lookup-by-id/lookup-guard.ts | 4 +-- ...c.ts => dso-redirect-data.service.spec.ts} | 8 +++--- ...ervice.ts => dso-redirect-data.service.ts} | 2 +- 5 files changed, 35 insertions(+), 11 deletions(-) rename src/app/core/data/{dso-data-redirect.service.spec.ts => dso-redirect-data.service.spec.ts} (96%) rename src/app/core/data/{dso-data-redirect.service.ts => dso-redirect-data.service.ts} (97%) diff --git a/src/app/+lookup-by-id/lookup-by-id-routing.module.ts b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts index 012345e791..758287bcbc 100644 --- a/src/app/+lookup-by-id/lookup-by-id-routing.module.ts +++ b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts @@ -1,12 +1,36 @@ import { LookupGuard } from './lookup-guard'; import { NgModule } from '@angular/core'; -import { RouterModule } from '@angular/router'; +import { RouterModule, UrlSegment } from '@angular/router'; import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; +import { hasValue } from '../shared/empty.util'; @NgModule({ imports: [ RouterModule.forChild([ - { path: ':idType/:id', canActivate: [LookupGuard], component: ObjectNotFoundComponent } + { + matcher: (url) => { + // The expected path is :idType/:id + const idType = url[0].path; + let id; + // Allow for handles that are delimited with a forward slash. + if (url.length === 3) { + id = url[1].path + '/' + url[2].path; + } else { + id = url[1].path; + } + if (hasValue(idType) && hasValue(id)) { + return { + consumed: url, + posParams: { + idType: new UrlSegment(idType, {}), + id: new UrlSegment(id, {}) + } + }; + } + return null; + }, + canActivate: [LookupGuard], + component: ObjectNotFoundComponent } ]) ], providers: [ diff --git a/src/app/+lookup-by-id/lookup-by-id.module.ts b/src/app/+lookup-by-id/lookup-by-id.module.ts index 4620f57824..1b070c1279 100644 --- a/src/app/+lookup-by-id/lookup-by-id.module.ts +++ b/src/app/+lookup-by-id/lookup-by-id.module.ts @@ -3,7 +3,7 @@ import { CommonModule } from '@angular/common'; import { SharedModule } from '../shared/shared.module'; import { LookupRoutingModule } from './lookup-by-id-routing.module'; import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; -import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; +import { DsoRedirectDataService } from '../core/data/dso-redirect-data.service'; @NgModule({ imports: [ @@ -15,7 +15,7 @@ import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; ObjectNotFoundComponent ], providers: [ - DsoDataRedirectService + DsoRedirectDataService ] }) export class LookupIdModule { diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts index 7f38b6db7a..5cc101f2b0 100644 --- a/src/app/+lookup-by-id/lookup-guard.ts +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -1,5 +1,5 @@ import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from '@angular/router'; -import { DsoDataRedirectService } from '../core/data/dso-data-redirect.service'; +import { DsoRedirectDataService } from '../core/data/dso-redirect-data.service'; import { Injectable } from '@angular/core'; import { IdentifierType } from '../core/index/index.reducer'; import { Observable } from 'rxjs'; @@ -14,7 +14,7 @@ interface LookupParams { @Injectable() export class LookupGuard implements CanActivate { - constructor(private dsoService: DsoDataRedirectService) { + constructor(private dsoService: DsoRedirectDataService) { } canActivate(route: ActivatedRouteSnapshot, state:RouterStateSnapshot): Observable { diff --git a/src/app/core/data/dso-data-redirect.service.spec.ts b/src/app/core/data/dso-redirect-data.service.spec.ts similarity index 96% rename from src/app/core/data/dso-data-redirect.service.spec.ts rename to src/app/core/data/dso-redirect-data.service.spec.ts index f2c35727fa..3bc37b872d 100644 --- a/src/app/core/data/dso-data-redirect.service.spec.ts +++ b/src/app/core/data/dso-redirect-data.service.spec.ts @@ -9,13 +9,13 @@ import { NotificationsService } from '../../shared/notifications/notifications.s import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; import { IdentifierType } from '../index/index.reducer'; -import { DsoDataRedirectService } from './dso-data-redirect.service'; +import { DsoRedirectDataService } from './dso-redirect-data.service'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; -describe('DsoDataRedirectService', () => { +describe('DsoRedirectDataService', () => { let scheduler: TestScheduler; - let service: DsoDataRedirectService; + let service: DsoRedirectDataService; let halService: HALEndpointService; let requestService: RequestService; let rdbService: RemoteDataBuildService; @@ -66,7 +66,7 @@ describe('DsoDataRedirectService', () => { a: remoteData }) }); - service = new DsoDataRedirectService( + service = new DsoRedirectDataService( requestService, rdbService, dataBuildService, diff --git a/src/app/core/data/dso-data-redirect.service.ts b/src/app/core/data/dso-redirect-data.service.ts similarity index 97% rename from src/app/core/data/dso-data-redirect.service.ts rename to src/app/core/data/dso-redirect-data.service.ts index 1cb3b20f2a..b8d4cf545d 100644 --- a/src/app/core/data/dso-data-redirect.service.ts +++ b/src/app/core/data/dso-redirect-data.service.ts @@ -20,7 +20,7 @@ import { getFinishedRemoteData } from '../shared/operators'; import { Router } from '@angular/router'; @Injectable() -export class DsoDataRedirectService extends DataService { +export class DsoRedirectDataService extends DataService { protected linkPath = 'pid'; protected forceBypassCache = false; From dfd1881f8922f71ad0bf01a48010f8943a20f0a8 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Fri, 18 Oct 2019 10:59:24 -0700 Subject: [PATCH 16/22] Added support for using the dso (uuid) endpoint. --- src/app/core/data/data.service.ts | 4 +-- .../core/data/dso-redirect-data.service.ts | 33 ++++++++++++------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 0f7ca74d15..7bfd1b6577 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -154,9 +154,7 @@ export abstract class DataService { map((endpoint: string) => this.getIDHref(endpoint, id))); } else if (identifierType === IdentifierType.HANDLE) { hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => { - return this.getIDHref(endpoint, encodeURIComponent(id)); - })); + map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id)))); } hrefObs.pipe( find((href: string) => hasValue(href))) diff --git a/src/app/core/data/dso-redirect-data.service.ts b/src/app/core/data/dso-redirect-data.service.ts index b8d4cf545d..b7acc371bf 100644 --- a/src/app/core/data/dso-redirect-data.service.ts +++ b/src/app/core/data/dso-redirect-data.service.ts @@ -14,7 +14,7 @@ import { IdentifierType } from '../index/index.reducer'; import { RemoteData } from './remote-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; import { Injectable } from '@angular/core'; -import { tap } from 'rxjs/operators'; +import { filter, tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; import { getFinishedRemoteData } from '../shared/operators'; import { Router } from '@angular/router'; @@ -22,8 +22,10 @@ import { Router } from '@angular/router'; @Injectable() export class DsoRedirectDataService extends DataService { + // Set the default link path to the identifier lookup endpoint. protected linkPath = 'pid'; protected forceBypassCache = false; + private uuidEndpoint = 'dso'; constructor( protected requestService: RequestService, @@ -43,28 +45,37 @@ export class DsoRedirectDataService extends DataService { return this.halService.getEndpoint(linkPath); } + setLinkPath(identifierType: IdentifierType) { + // The default 'pid' endpoint for identifiers does not support uuid lookups. + // For uuid lookups we need to change the linkPath. + if (identifierType === IdentifierType.UUID) { + this.linkPath = this.uuidEndpoint; + } + } + getIDHref(endpoint, resourceID): string { - return endpoint.replace(/\{\?id\}/,`?id=${resourceID}`); + // Supporting both identifier (pid) and uuid (dso) endpoints + return endpoint.replace(/\{\?id\}/, `?id=${resourceID}`) + .replace(/\{\?uuid\}/, `?uuid=${resourceID}`); } findById(id: string, identifierType = IdentifierType.UUID): Observable> { + this.setLinkPath(identifierType); return super.findById(id, identifierType).pipe( getFinishedRemoteData(), + filter((response) => response.hasSucceeded), tap((response) => { - if (response.hasSucceeded) { - const uuid = response.payload.uuid; - // Is there an existing method somewhere that converts dso type route? - const dsoType = this.getEndpointFromDSOType(response.payload.type); - if (hasValue(uuid) && hasValue(dsoType)) { - this.router.navigate([dsoType + '/' + uuid]); - } + const uuid = response.payload.uuid; + const newRoute = this.getEndpointFromDSOType(response.payload.type); + if (hasValue(uuid) && hasValue(newRoute)) { + this.router.navigate([newRoute + '/' + uuid]); } }) ); } - + // Is there an existing method somewhere else that converts dso type to route? getEndpointFromDSOType(dsoType: string): string { - // Are there other routes to consider? + // Are there other types to consider? if (dsoType.startsWith('item')) { return 'items' } else if (dsoType.startsWith('community')) { From 03c36ab23347c27956de1c6044834b270c668ea5 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Fri, 18 Oct 2019 11:18:21 -0700 Subject: [PATCH 17/22] Updated unit test. --- .../data/dso-redirect-data.service.spec.ts | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/app/core/data/dso-redirect-data.service.spec.ts b/src/app/core/data/dso-redirect-data.service.spec.ts index 3bc37b872d..84ca3918b8 100644 --- a/src/app/core/data/dso-redirect-data.service.spec.ts +++ b/src/app/core/data/dso-redirect-data.service.spec.ts @@ -84,12 +84,28 @@ describe('DsoRedirectDataService', () => { describe('findById', () => { it('should call HALEndpointService with the path to the pid endpoint', () => { setup(); - scheduler.schedule(() => service.findById(dsoUUID)); + scheduler.schedule(() => service.findById(dsoHandle, IdentifierType.HANDLE)); scheduler.flush(); expect(halService.getEndpoint).toHaveBeenCalledWith('pid'); }); + it('should call HALEndpointService with the path to the dso endpoint', () => { + setup(); + scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); + scheduler.flush(); + + expect(halService.getEndpoint).toHaveBeenCalledWith('dso'); + }); + + it('should call HALEndpointService with the path to the dso endpoint when identifier type not specified', () => { + setup(); + scheduler.schedule(() => service.findById(dsoUUID)); + scheduler.flush(); + + expect(halService.getEndpoint).toHaveBeenCalledWith('dso'); + }); + it('should configure the proper FindByIDRequest for uuid', () => { setup(); scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); From d3a84c7e7f5f31af42f0bdbb6bbfb96bfbfe33df Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 30 Oct 2019 09:23:50 -0700 Subject: [PATCH 18/22] Updated matcher in lookup-by-id routing module. Converted id to const. --- .../+lookup-by-id/lookup-by-id-routing.module.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/app/+lookup-by-id/lookup-by-id-routing.module.ts b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts index 758287bcbc..5ad3c2fd82 100644 --- a/src/app/+lookup-by-id/lookup-by-id-routing.module.ts +++ b/src/app/+lookup-by-id/lookup-by-id-routing.module.ts @@ -2,7 +2,7 @@ import { LookupGuard } from './lookup-guard'; import { NgModule } from '@angular/core'; import { RouterModule, UrlSegment } from '@angular/router'; import { ObjectNotFoundComponent } from './objectnotfound/objectnotfound.component'; -import { hasValue } from '../shared/empty.util'; +import { hasValue, isNotEmpty } from '../shared/empty.util'; @NgModule({ imports: [ @@ -11,14 +11,12 @@ import { hasValue } from '../shared/empty.util'; matcher: (url) => { // The expected path is :idType/:id const idType = url[0].path; - let id; // Allow for handles that are delimited with a forward slash. - if (url.length === 3) { - id = url[1].path + '/' + url[2].path; - } else { - id = url[1].path; - } - if (hasValue(idType) && hasValue(id)) { + const id = url + .slice(1) + .map((us: UrlSegment) => us.path) + .join('/'); + if (isNotEmpty(idType) && isNotEmpty(id)) { return { consumed: url, posParams: { From 9a0a1645fb14a88aa3df60dc5710cc1bbb22a6d4 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 30 Oct 2019 11:53:46 -0700 Subject: [PATCH 19/22] Removed the success filter from DSO data response so that all responses are returned, including 404. --- src/app/core/data/dso-redirect-data.service.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/app/core/data/dso-redirect-data.service.ts b/src/app/core/data/dso-redirect-data.service.ts index b7acc371bf..2df8b70cd8 100644 --- a/src/app/core/data/dso-redirect-data.service.ts +++ b/src/app/core/data/dso-redirect-data.service.ts @@ -14,7 +14,7 @@ import { IdentifierType } from '../index/index.reducer'; import { RemoteData } from './remote-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; import { Injectable } from '@angular/core'; -import { filter, tap } from 'rxjs/operators'; +import { filter, take, tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; import { getFinishedRemoteData } from '../shared/operators'; import { Router } from '@angular/router'; @@ -63,12 +63,14 @@ export class DsoRedirectDataService extends DataService { this.setLinkPath(identifierType); return super.findById(id, identifierType).pipe( getFinishedRemoteData(), - filter((response) => response.hasSucceeded), + take(1), tap((response) => { - const uuid = response.payload.uuid; - const newRoute = this.getEndpointFromDSOType(response.payload.type); - if (hasValue(uuid) && hasValue(newRoute)) { - this.router.navigate([newRoute + '/' + uuid]); + if (response.hasSucceeded) { + const uuid = response.payload.uuid; + const newRoute = this.getEndpointFromDSOType(response.payload.type); + if (hasValue(uuid) && hasValue(newRoute)) { + this.router.navigate([newRoute + '/' + uuid]); + } } }) ); From d309b2081c79dd5cbddd6f9eaad94c0328c8ded1 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 30 Oct 2019 13:59:10 -0700 Subject: [PATCH 20/22] Removed the handle index (not needed because the uuid index will always be used internally) and all associated code. --- src/app/+lookup-by-id/lookup-guard.spec.ts | 2 +- src/app/+lookup-by-id/lookup-guard.ts | 2 +- src/app/core/cache/object-cache.service.ts | 9 ++--- src/app/core/core.effects.ts | 4 +- src/app/core/data/data.service.ts | 17 +++----- .../data/dso-redirect-data.service.spec.ts | 7 ++-- .../core/data/dso-redirect-data.service.ts | 5 +-- .../data/dspace-object-data.service.spec.ts | 3 +- src/app/core/data/request.models.ts | 10 +++-- src/app/core/data/request.service.spec.ts | 11 ----- src/app/core/data/request.service.ts | 17 +++----- src/app/core/index/index.actions.ts | 14 +++---- src/app/core/index/index.effects.ts | 38 ++++-------------- src/app/core/index/index.reducer.spec.ts | 40 ++++++------------- src/app/core/index/index.reducer.ts | 35 +++++++--------- src/app/core/index/index.selectors.ts | 31 +++++--------- 16 files changed, 83 insertions(+), 162 deletions(-) diff --git a/src/app/+lookup-by-id/lookup-guard.spec.ts b/src/app/+lookup-by-id/lookup-guard.spec.ts index 824e31c62b..dce039eff3 100644 --- a/src/app/+lookup-by-id/lookup-guard.spec.ts +++ b/src/app/+lookup-by-id/lookup-guard.spec.ts @@ -1,6 +1,6 @@ import { LookupGuard } from './lookup-guard'; import { of as observableOf } from 'rxjs'; -import { IdentifierType } from '../core/index/index.reducer'; +import { IdentifierType } from '../core/data/request.models'; describe('LookupGuard', () => { let dsoService: any; diff --git a/src/app/+lookup-by-id/lookup-guard.ts b/src/app/+lookup-by-id/lookup-guard.ts index 4857fb87fb..c89e329241 100644 --- a/src/app/+lookup-by-id/lookup-guard.ts +++ b/src/app/+lookup-by-id/lookup-guard.ts @@ -1,6 +1,6 @@ import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from '@angular/router'; import { Injectable } from '@angular/core'; -import { IdentifierType } from '../core/index/index.reducer'; +import { IdentifierType } from '../core/data/request.models'; import { Observable } from 'rxjs'; import { map } from 'rxjs/operators'; import { RemoteData } from '../core/data/remote-data'; diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 95e96db0c8..7dc659d89c 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -21,7 +21,6 @@ import { import { CacheableObject, ObjectCacheEntry, ObjectCacheState } from './object-cache.reducer'; import { AddToSSBAction } from './server-sync-buffer.actions'; import { getMapsToType } from './builders/build-decorators'; -import { IdentifierType } from '../index/index.reducer'; /** * The base selector function to select the object cache in the store @@ -81,10 +80,10 @@ export class ObjectCacheService { * @return Observable> * An observable of the requested object in normalized form */ - getObjectByID(id: string, identifierType: IdentifierType = IdentifierType.UUID): + getObjectByID(id: string): Observable> { return this.store.pipe( - select(selfLinkFromUuidSelector(id, identifierType)), + select(selfLinkFromUuidSelector(id)), mergeMap((selfLink: string) => this.getObjectBySelfLink(selfLink) ) ) @@ -196,11 +195,11 @@ export class ObjectCacheService { * true if the object with the specified UUID is cached, * false otherwise */ - hasById(id: string, identifierType: IdentifierType = IdentifierType.UUID): boolean { + hasById(id: string): boolean { let result: boolean; this.store.pipe( - select(selfLinkFromUuidSelector(id, identifierType)), + select(selfLinkFromUuidSelector(id)), take(1) ).subscribe((selfLink: string) => result = this.hasBySelfLink(selfLink)); diff --git a/src/app/core/core.effects.ts b/src/app/core/core.effects.ts index ae6f7f3cfa..0eabfc5dc8 100644 --- a/src/app/core/core.effects.ts +++ b/src/app/core/core.effects.ts @@ -1,5 +1,5 @@ import { ObjectCacheEffects } from './cache/object-cache.effects'; -import { IdentifierIndexEffects } from './index/index.effects'; +import { UUIDIndexEffects } from './index/index.effects'; import { RequestEffects } from './data/request.effects'; import { AuthEffects } from './auth/auth.effects'; import { JsonPatchOperationsEffects } from './json-patch/json-patch-operations.effects'; @@ -10,7 +10,7 @@ import { RouteEffects } from './services/route.effects'; export const coreEffects = [ RequestEffects, ObjectCacheEffects, - IdentifierIndexEffects, + UUIDIndexEffects, AuthEffects, JsonPatchOperationsEffects, ServerSyncBufferEffects, diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 7bfd1b6577..77392a8528 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -37,7 +37,6 @@ import { NormalizedObjectBuildService } from '../cache/builders/normalized-objec import { ChangeAnalyzer } from './change-analyzer'; import { RestRequestMethod } from './rest-request-method'; import { getMapsToType } from '../cache/builders/build-decorators'; -import { IdentifierType } from '../index/index.reducer'; export abstract class DataService { protected abstract requestService: RequestService; @@ -147,19 +146,15 @@ export abstract class DataService { return `${endpoint}/${resourceID}`; } - findById(id: string, identifierType: IdentifierType = IdentifierType.UUID): Observable> { - let hrefObs; - if (identifierType === IdentifierType.UUID) { - hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => this.getIDHref(endpoint, id))); - } else if (identifierType === IdentifierType.HANDLE) { - hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id)))); - } + findById(id: string): Observable> { + + const hrefObs = this.halService.getEndpoint(this.linkPath).pipe( + map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id)))) + hrefObs.pipe( find((href: string) => hasValue(href))) .subscribe((href: string) => { - const request = new FindByIDRequest(this.requestService.generateRequestId(), href, id, identifierType); + const request = new FindByIDRequest(this.requestService.generateRequestId(), href, id); this.requestService.configure(request, this.forceBypassCache); }); diff --git a/src/app/core/data/dso-redirect-data.service.spec.ts b/src/app/core/data/dso-redirect-data.service.spec.ts index 84ca3918b8..36f3a6ebdb 100644 --- a/src/app/core/data/dso-redirect-data.service.spec.ts +++ b/src/app/core/data/dso-redirect-data.service.spec.ts @@ -2,13 +2,12 @@ import { cold, getTestScheduler } from 'jasmine-marbles'; import { TestScheduler } from 'rxjs/testing'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { HALEndpointService } from '../shared/hal-endpoint.service'; -import { FindByIDRequest } from './request.models'; +import { FindByIDRequest, IdentifierType } from './request.models'; import { RequestService } from './request.service'; import { ObjectCacheService } from '../cache/object-cache.service'; import { NotificationsService } from '../../shared/notifications/notifications.service'; import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; -import { IdentifierType } from '../index/index.reducer'; import { DsoRedirectDataService } from './dso-redirect-data.service'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; @@ -111,7 +110,7 @@ describe('DsoRedirectDataService', () => { scheduler.schedule(() => service.findById(dsoUUID, IdentifierType.UUID)); scheduler.flush(); - expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestUUIDURL, dsoUUID, IdentifierType.UUID), false); + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestUUIDURL, dsoUUID), false); }); it('should configure the proper FindByIDRequest for handle', () => { @@ -119,7 +118,7 @@ describe('DsoRedirectDataService', () => { scheduler.schedule(() => service.findById(dsoHandle, IdentifierType.HANDLE)); scheduler.flush(); - expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle, IdentifierType.HANDLE), false); + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestHandleURL, dsoHandle), false); }); it('should navigate to item route', () => { diff --git a/src/app/core/data/dso-redirect-data.service.ts b/src/app/core/data/dso-redirect-data.service.ts index 2df8b70cd8..7e71f82bbf 100644 --- a/src/app/core/data/dso-redirect-data.service.ts +++ b/src/app/core/data/dso-redirect-data.service.ts @@ -8,9 +8,8 @@ import { RemoteDataBuildService } from '../cache/builders/remote-data-build.serv import { RequestService } from './request.service'; import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; -import { FindAllOptions, FindByIDRequest } from './request.models'; +import { FindAllOptions, FindByIDRequest, IdentifierType } from './request.models'; import { Observable } from 'rxjs'; -import { IdentifierType } from '../index/index.reducer'; import { RemoteData } from './remote-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; import { Injectable } from '@angular/core'; @@ -61,7 +60,7 @@ export class DsoRedirectDataService extends DataService { findById(id: string, identifierType = IdentifierType.UUID): Observable> { this.setLinkPath(identifierType); - return super.findById(id, identifierType).pipe( + return super.findById(id).pipe( getFinishedRemoteData(), take(1), tap((response) => { diff --git a/src/app/core/data/dspace-object-data.service.spec.ts b/src/app/core/data/dspace-object-data.service.spec.ts index b411028420..a0bba214ae 100644 --- a/src/app/core/data/dspace-object-data.service.spec.ts +++ b/src/app/core/data/dspace-object-data.service.spec.ts @@ -10,7 +10,6 @@ import { ObjectCacheService } from '../cache/object-cache.service'; import { NotificationsService } from '../../shared/notifications/notifications.service'; import { HttpClient } from '@angular/common/http'; import { NormalizedObjectBuildService } from '../cache/builders/normalized-object-build.service'; -import { IdentifierType } from '../index/index.reducer'; describe('DSpaceObjectDataService', () => { let scheduler: TestScheduler; @@ -73,7 +72,7 @@ describe('DSpaceObjectDataService', () => { scheduler.schedule(() => service.findById(testObject.uuid)); scheduler.flush(); - expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestURL, testObject.uuid, IdentifierType.UUID), false); + expect(requestService.configure).toHaveBeenCalledWith(new FindByIDRequest(requestUUID, requestURL, testObject.uuid), false); }); it('should return a RemoteData for the object with the given ID', () => { diff --git a/src/app/core/data/request.models.ts b/src/app/core/data/request.models.ts index 45e8fdea16..b9f24dd96c 100644 --- a/src/app/core/data/request.models.ts +++ b/src/app/core/data/request.models.ts @@ -19,10 +19,15 @@ import { MetadatafieldParsingService } from './metadatafield-parsing.service'; import { URLCombiner } from '../url-combiner/url-combiner'; import { TaskResponseParsingService } from '../tasks/task-response-parsing.service'; import { MappedCollectionsReponseParsingService } from './mapped-collections-reponse-parsing.service'; -import { IdentifierType } from '../index/index.reducer'; /* tslint:disable:max-classes-per-file */ +// uuid and handle requests have separate endpoints +export enum IdentifierType { + UUID ='uuid', + HANDLE = 'handle' +} + export abstract class RestRequest { public responseMsToLive = 0; constructor( @@ -126,8 +131,7 @@ export class FindByIDRequest extends GetRequest { constructor( uuid: string, href: string, - public resourceID: string, - public identifierType?: IdentifierType + public resourceID: string ) { super(uuid, href); } diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 5ceca8b4bd..2a3f26f1bb 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -11,7 +11,6 @@ import { UUIDService } from '../shared/uuid.service'; import { RequestConfigureAction, RequestExecuteAction } from './request.actions'; import { DeleteRequest, - FindByIDRequest, GetRequest, HeadRequest, OptionsRequest, @@ -22,7 +21,6 @@ import { } from './request.models'; import { RequestService } from './request.service'; import { TestScheduler } from 'rxjs/testing'; -import { IdentifierType } from '../index/index.reducer'; describe('RequestService', () => { let scheduler: TestScheduler; @@ -40,7 +38,6 @@ describe('RequestService', () => { const testDeleteRequest = new DeleteRequest(testUUID, testHref); const testOptionsRequest = new OptionsRequest(testUUID, testHref); const testHeadRequest = new HeadRequest(testUUID, testHref); - const testFindByIdRequest = new FindByIDRequest(testUUID, testHref, testUUID, IdentifierType.UUID); const testPatchRequest = new PatchRequest(testUUID, testHref); let selectSpy; @@ -309,14 +306,6 @@ describe('RequestService', () => { const result = serviceAsAny.isCachedOrPending(testGetRequest); const expected = true; - expect(result).toEqual(expected); - }); - it('should return true for instance of FindByIdRequest', () => { - (objectCache.hasBySelfLink as any).and.returnValue(false); - const result = serviceAsAny.isCachedOrPending(testFindByIdRequest); - expect(objectCache.hasById).toHaveBeenCalledWith(testUUID, IdentifierType.UUID); - const expected = true; - expect(result).toEqual(expected); }); }); diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index ac65042238..789f8c165e 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -11,7 +11,7 @@ import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; import { ObjectCacheService } from '../cache/object-cache.service'; import { CoreState } from '../core.reducers'; -import { IdentifierType, IndexState, MetaIndexState, REQUEST, UUID_MAPPING } from '../index/index.reducer'; +import { IndexName, IndexState, MetaIndexState } from '../index/index.reducer'; import { originalRequestUUIDFromRequestUUIDSelector, requestIndexSelector, @@ -19,7 +19,7 @@ import { } from '../index/index.selectors'; import { UUIDService } from '../shared/uuid.service'; import { RequestConfigureAction, RequestExecuteAction, RequestRemoveAction } from './request.actions'; -import { FindByIDRequest, GetRequest, RestRequest } from './request.models'; +import { GetRequest, RestRequest } from './request.models'; import { RequestEntry, RequestState } from './request.reducer'; import { CommitSSBAction } from '../cache/server-sync-buffer.actions'; import { RestRequestMethod } from './rest-request-method'; @@ -162,7 +162,7 @@ export class RequestService { filter((entry) => hasValue(entry)), take(1) ).subscribe((entry) => { - return this.store.dispatch(new AddToIndexAction(UUID_MAPPING, request.uuid, entry.request.uuid)) + return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) } ) } @@ -206,7 +206,7 @@ export class RequestService { } }); this.requestsOnTheirWayToTheStore = this.requestsOnTheirWayToTheStore.filter((reqHref: string) => reqHref.indexOf(href) < 0); - this.indexStore.dispatch(new RemoveFromIndexBySubstringAction(REQUEST, href)); + this.indexStore.dispatch(new RemoveFromIndexBySubstringAction(IndexName.REQUEST, href)); } /** @@ -225,14 +225,7 @@ export class RequestService { private isCachedOrPending(request: GetRequest): boolean { const inReqCache = this.hasByHref(request.href); const inObjCache = this.objectCache.hasBySelfLink(request.href); - let inObjIdCache = false; - if (request instanceof FindByIDRequest) { - const req = request as FindByIDRequest; - if (hasValue(req.identifierType && hasValue(req.resourceID))) { - inObjIdCache = this.objectCache.hasById(req.resourceID, req.identifierType) - } - } - const isCached = inReqCache || inObjCache || inObjIdCache; + const isCached = inReqCache || inObjCache; const isPending = this.isPending(request); return isCached || isPending; } diff --git a/src/app/core/index/index.actions.ts b/src/app/core/index/index.actions.ts index 24f031b33c..42804dbe26 100644 --- a/src/app/core/index/index.actions.ts +++ b/src/app/core/index/index.actions.ts @@ -1,7 +1,7 @@ import { Action } from '@ngrx/store'; import { type } from '../../shared/ngrx/type'; -import { } from './index.reducer'; +import { IndexName } from './index.reducer'; /** * The list of HrefIndexAction type definitions @@ -19,7 +19,7 @@ export const IndexActionTypes = { export class AddToIndexAction implements Action { type = IndexActionTypes.ADD; payload: { - name: string; + name: IndexName; value: string; key: string; }; @@ -34,7 +34,7 @@ export class AddToIndexAction implements Action { * @param value * the self link of the resource the key belongs to */ - constructor(name: string, key: string, value: string) { + constructor(name: IndexName, key: string, value: string) { this.payload = { name, key, value }; } } @@ -45,7 +45,7 @@ export class AddToIndexAction implements Action { export class RemoveFromIndexByValueAction implements Action { type = IndexActionTypes.REMOVE_BY_VALUE; payload: { - name: string, + name: IndexName, value: string }; @@ -57,7 +57,7 @@ export class RemoveFromIndexByValueAction implements Action { * @param value * the value to remove the UUID for */ - constructor(name: string, value: string) { + constructor(name: IndexName, value: string) { this.payload = { name, value }; } @@ -69,7 +69,7 @@ export class RemoveFromIndexByValueAction implements Action { export class RemoveFromIndexBySubstringAction implements Action { type = IndexActionTypes.REMOVE_BY_SUBSTRING; payload: { - name: string, + name: IndexName, value: string }; @@ -81,7 +81,7 @@ export class RemoveFromIndexBySubstringAction implements Action { * @param value * the value to remove the UUID for */ - constructor(name: string, value: string) { + constructor(name: IndexName, value: string) { this.payload = { name, value }; } diff --git a/src/app/core/index/index.effects.ts b/src/app/core/index/index.effects.ts index 0cdf6bea6c..61cf313ab1 100644 --- a/src/app/core/index/index.effects.ts +++ b/src/app/core/index/index.effects.ts @@ -10,67 +10,43 @@ import { import { RequestActionTypes, RequestConfigureAction } from '../data/request.actions'; import { AddToIndexAction, RemoveFromIndexByValueAction } from './index.actions'; import { hasValue } from '../../shared/empty.util'; -import { getIdentiferByIndexName, IdentifierType, REQUEST } from './index.reducer'; +import { IndexName } from './index.reducer'; import { RestRequestMethod } from '../data/rest-request-method'; @Injectable() -export class IdentifierIndexEffects { +export class UUIDIndexEffects { - @Effect() addObjectByUUID$ = this.actions$ + @Effect() addObject$ = this.actions$ .pipe( ofType(ObjectCacheActionTypes.ADD), filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.uuid)), map((action: AddToObjectCacheAction) => { return new AddToIndexAction( - getIdentiferByIndexName(IdentifierType.UUID), + IndexName.OBJECT, action.payload.objectToCache.uuid, action.payload.objectToCache.self ); }) ); - @Effect() addObjectByHandle$ = this.actions$ - .pipe( - ofType(ObjectCacheActionTypes.ADD), - filter((action: AddToObjectCacheAction) => hasValue(action.payload.objectToCache.handle)), - map((action: AddToObjectCacheAction) => { - return new AddToIndexAction( - getIdentiferByIndexName(IdentifierType.HANDLE), - action.payload.objectToCache.handle, - action.payload.objectToCache.self - ); - }) - ); - - @Effect() removeObjectByUUID$ = this.actions$ + @Effect() removeObject$ = this.actions$ .pipe( ofType(ObjectCacheActionTypes.REMOVE), map((action: RemoveFromObjectCacheAction) => { return new RemoveFromIndexByValueAction( - getIdentiferByIndexName(IdentifierType.UUID), + IndexName.OBJECT, action.payload ); }) ); - @Effect() removeObjectByHandle$ = this.actions$ - .pipe( - ofType(ObjectCacheActionTypes.REMOVE), - map((action: RemoveFromObjectCacheAction) => { - return new RemoveFromIndexByValueAction( - getIdentiferByIndexName(IdentifierType.HANDLE), - action.payload - ); - }) - ); - @Effect() addRequest$ = this.actions$ .pipe( ofType(RequestActionTypes.CONFIGURE), filter((action: RequestConfigureAction) => action.payload.method === RestRequestMethod.GET), map((action: RequestConfigureAction) => { return new AddToIndexAction( - REQUEST, + IndexName.REQUEST, action.payload.href, action.payload.uuid ); diff --git a/src/app/core/index/index.reducer.spec.ts b/src/app/core/index/index.reducer.spec.ts index 35460a9ef5..ef46c760c6 100644 --- a/src/app/core/index/index.reducer.spec.ts +++ b/src/app/core/index/index.reducer.spec.ts @@ -1,6 +1,6 @@ import * as deepFreeze from 'deep-freeze'; -import { getIdentiferByIndexName, IdentifierType, indexReducer, MetaIndexState, REQUEST, } from './index.reducer'; +import { IndexName, indexReducer, MetaIndexState } from './index.reducer'; import { AddToIndexAction, RemoveFromIndexBySubstringAction, RemoveFromIndexByValueAction } from './index.actions'; class NullAction extends AddToIndexAction { @@ -15,19 +15,14 @@ class NullAction extends AddToIndexAction { describe('requestReducer', () => { const key1 = '567a639f-f5ff-4126-807c-b7d0910808c8'; const key2 = '1911e8a4-6939-490c-b58b-a5d70f8d91fb'; - const key3 = '123456789/22'; const val1 = 'https://dspace7.4science.it/dspace-spring-rest/api/core/items/567a639f-f5ff-4126-807c-b7d0910808c8'; const val2 = 'https://dspace7.4science.it/dspace-spring-rest/api/core/items/1911e8a4-6939-490c-b58b-a5d70f8d91fb'; - const uuidIndex = getIdentiferByIndexName(IdentifierType.UUID); - const handleIndex = getIdentiferByIndexName(IdentifierType.HANDLE); const testState: MetaIndexState = { - 'object/uuid-to-self-link/uuid': { + [IndexName.OBJECT]: { [key1]: val1 - },'object/uuid-to-self-link/handle': { - [key3]: val1 - },'get-request/href-to-uuid': { + },[IndexName.REQUEST]: { [key1]: val1 - },'get-request/configured-to-cache-uuid': { + },[IndexName.UUID_MAPPING]: { [key1]: val1 } }; @@ -50,38 +45,27 @@ describe('requestReducer', () => { it('should add the \'key\' with the corresponding \'value\' to the correct substate, in response to an ADD action', () => { const state = testState; - const action = new AddToIndexAction(REQUEST, key2, val2); + const action = new AddToIndexAction(IndexName.REQUEST, key2, val2); const newState = indexReducer(state, action); - expect(newState[REQUEST][key2]).toEqual(val2); + expect(newState[IndexName.REQUEST][key2]).toEqual(val2); }); it('should remove the given \'value\' from its corresponding \'key\' in the correct substate, in response to a REMOVE_BY_VALUE action', () => { const state = testState; - let action = new RemoveFromIndexByValueAction(uuidIndex, val1); - let newState = indexReducer(state, action); - - expect(newState[uuidIndex][key1]).toBeUndefined(); - - action = new RemoveFromIndexByValueAction(handleIndex, val1); - newState = indexReducer(state, action); - - expect(newState[handleIndex][key3]).toBeUndefined(); + const action = new RemoveFromIndexByValueAction(IndexName.OBJECT, val1); + const newState = indexReducer(state, action); + expect(newState[IndexName.OBJECT][key1]).toBeUndefined(); }); it('should remove the given \'value\' from its corresponding \'key\' in the correct substate, in response to a REMOVE_BY_SUBSTRING action', () => { const state = testState; - let action = new RemoveFromIndexBySubstringAction(uuidIndex, key1); - let newState = indexReducer(state, action); + const action = new RemoveFromIndexBySubstringAction(IndexName.OBJECT, key1); + const newState = indexReducer(state, action); - expect(newState[uuidIndex][key1]).toBeUndefined(); - - action = new RemoveFromIndexBySubstringAction(handleIndex, key3); - newState = indexReducer(state, action); - - expect(newState[uuidIndex][key3]).toBeUndefined(); + expect(newState[IndexName.OBJECT][key1]).toBeUndefined(); }); }); diff --git a/src/app/core/index/index.reducer.ts b/src/app/core/index/index.reducer.ts index 631d579911..b4cd8aa84b 100644 --- a/src/app/core/index/index.reducer.ts +++ b/src/app/core/index/index.reducer.ts @@ -6,25 +6,23 @@ import { RemoveFromIndexByValueAction } from './index.actions'; -export enum IdentifierType { - UUID ='uuid', - HANDLE = 'handle' -} - /** - * Contains the UUIDs of requests that were sent to the server and - * have their responses cached, indexed by the UUIDs of requests that - * weren't sent because the response they requested was already cached + * An enum containing all index names */ -export const UUID_MAPPING = 'get-request/configured-to-cache-uuid'; +export enum IndexName { + // Contains all objects in the object cache indexed by UUID + OBJECT = 'object/uuid-to-self-link', -// contains all requests in the request cache indexed by UUID -export const REQUEST = 'get-request/href-to-uuid'; + // contains all requests in the request cache indexed by UUID + REQUEST = 'get-request/href-to-uuid', -// returns the index for the provided id type (uuid, handle) -export const getIdentiferByIndexName = (idType: IdentifierType): string => { - return `object/uuid-to-self-link/${idType}`; -}; + /** + * Contains the UUIDs of requests that were sent to the server and + * have their responses cached, indexed by the UUIDs of requests that + * weren't sent because the response they requested was already cached + */ + UUID_MAPPING = 'get-request/configured-to-cache-uuid' +} /** * The state of a single index @@ -36,11 +34,8 @@ export interface IndexState { /** * The state that contains all indices */ -export interface MetaIndexState { - 'get-request/configured-to-cache-uuid': IndexState, - 'get-request/href-to-uuid': IndexState, - 'object/uuid-to-self-link/uuid': IndexState, - 'object/uuid-to-self-link/handle': IndexState +export type MetaIndexState = { + [name in IndexName]: IndexState } // Object.create(null) ensures the object has no default js properties (e.g. `__proto__`) diff --git a/src/app/core/index/index.selectors.ts b/src/app/core/index/index.selectors.ts index 80a7c0d46a..a6d7b7e7b0 100644 --- a/src/app/core/index/index.selectors.ts +++ b/src/app/core/index/index.selectors.ts @@ -3,14 +3,7 @@ import { AppState } from '../../app.reducer'; import { hasValue } from '../../shared/empty.util'; import { CoreState } from '../core.reducers'; import { coreSelector } from '../core.selectors'; -import { - getIdentiferByIndexName, - IdentifierType, - IndexState, - MetaIndexState, - REQUEST, - UUID_MAPPING -} from './index.reducer'; +import { IndexName, IndexState, MetaIndexState } from './index.reducer'; /** * Return the MetaIndexState based on the CoreSate @@ -27,17 +20,13 @@ export const metaIndexSelector: MemoizedSelector = cre * Return the object index based on the MetaIndexState * It contains all objects in the object cache indexed by UUID * - * @param identifierType the type of index, used to select index from state - * * @returns * a MemoizedSelector to select the object index */ -export const objectIndexSelector = (identifierType: IdentifierType = IdentifierType.UUID): MemoizedSelector => { - return createSelector( - metaIndexSelector, - (state: MetaIndexState) => state[getIdentiferByIndexName(identifierType)] - ); -} +export const objectIndexSelector: MemoizedSelector = createSelector( + metaIndexSelector, + (state: MetaIndexState) => state[IndexName.OBJECT] +); /** * Return the request index based on the MetaIndexState @@ -47,7 +36,7 @@ export const objectIndexSelector = (identifierType: IdentifierType = IdentifierT */ export const requestIndexSelector: MemoizedSelector = createSelector( metaIndexSelector, - (state: MetaIndexState) => state[REQUEST] + (state: MetaIndexState) => state[IndexName.REQUEST] ); /** @@ -58,7 +47,7 @@ export const requestIndexSelector: MemoizedSelector = crea */ export const requestUUIDIndexSelector: MemoizedSelector = createSelector( metaIndexSelector, - (state: MetaIndexState) => state[UUID_MAPPING] + (state: MetaIndexState) => state[IndexName.UUID_MAPPING] ); /** @@ -71,9 +60,9 @@ export const requestUUIDIndexSelector: MemoizedSelector = * a MemoizedSelector to select the self link */ export const selfLinkFromUuidSelector = - (id: string, identifierType: IdentifierType = IdentifierType.UUID): MemoizedSelector => createSelector( - objectIndexSelector(identifierType), - (state: IndexState) => hasValue(state) ? state[id] : undefined + (uuid: string): MemoizedSelector => createSelector( + objectIndexSelector, + (state: IndexState) => hasValue(state) ? state[uuid] : undefined ); /** From 0dd765d84a037e16b89e390b056523b3170edaf8 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Wed, 30 Oct 2019 14:23:50 -0700 Subject: [PATCH 21/22] Reverted object cache method names back to the original UUID convention. --- src/app/core/cache/object-cache.service.ts | 12 ++++++------ src/app/core/data/comcol-data.service.spec.ts | 4 ++-- src/app/core/data/comcol-data.service.ts | 2 +- src/app/core/data/data.service.ts | 2 +- src/app/core/data/request.service.spec.ts | 2 +- src/app/shared/mocks/mock-object-cache.service.ts | 4 ++-- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 7dc659d89c..e12629fa27 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -75,15 +75,15 @@ export class ObjectCacheService { /** * Get an observable of the object with the specified UUID * - * @param id + * @param uuid * The UUID of the object to get * @return Observable> * An observable of the requested object in normalized form */ - getObjectByID(id: string): + getObjectByUUID(uuid: string): Observable> { return this.store.pipe( - select(selfLinkFromUuidSelector(id)), + select(selfLinkFromUuidSelector(uuid)), mergeMap((selfLink: string) => this.getObjectBySelfLink(selfLink) ) ) @@ -189,17 +189,17 @@ export class ObjectCacheService { /** * Check whether the object with the specified UUID is cached * - * @param id + * @param uuid * The UUID of the object to check * @return boolean * true if the object with the specified UUID is cached, * false otherwise */ - hasById(id: string): boolean { + hasByUUID(uuid: string): boolean { let result: boolean; this.store.pipe( - select(selfLinkFromUuidSelector(id)), + select(selfLinkFromUuidSelector(uuid)), take(1) ).subscribe((selfLink: string) => result = this.hasBySelfLink(selfLink)); diff --git a/src/app/core/data/comcol-data.service.spec.ts b/src/app/core/data/comcol-data.service.spec.ts index de17d7a39f..b5232b0bff 100644 --- a/src/app/core/data/comcol-data.service.spec.ts +++ b/src/app/core/data/comcol-data.service.spec.ts @@ -95,7 +95,7 @@ describe('ComColDataService', () => { function initMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCache', { - getObjectByID: cold('d-', { + getObjectByUUID: cold('d-', { d: { _links: { [LINK_NAME]: scopedEndpoint @@ -160,7 +160,7 @@ describe('ComColDataService', () => { it('should fetch the scope Community from the cache', () => { scheduler.schedule(() => service.getBrowseEndpoint(options).subscribe()); scheduler.flush(); - expect(objectCache.getObjectByID).toHaveBeenCalledWith(scopeID); + expect(objectCache.getObjectByUUID).toHaveBeenCalledWith(scopeID); }); it('should return the endpoint to fetch resources within the given scope', () => { diff --git a/src/app/core/data/comcol-data.service.ts b/src/app/core/data/comcol-data.service.ts index 3059d568df..68eb3e4880 100644 --- a/src/app/core/data/comcol-data.service.ts +++ b/src/app/core/data/comcol-data.service.ts @@ -49,7 +49,7 @@ export abstract class ComColDataService extends DataS ); const successResponses = responses.pipe( filter((response) => response.isSuccessful), - mergeMap(() => this.objectCache.getObjectByID(options.scopeID)), + mergeMap(() => this.objectCache.getObjectByUUID(options.scopeID)), map((nc: NormalizedCommunity) => nc._links[linkPath]), filter((href) => isNotEmpty(href)) ); diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 77392a8528..6b3994f416 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -149,7 +149,7 @@ export abstract class DataService { findById(id: string): Observable> { const hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id)))) + map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id)))); hrefObs.pipe( find((href: string) => hasValue(href))) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 2a3f26f1bb..5807666d66 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -298,7 +298,7 @@ describe('RequestService', () => { describe('in the ObjectCache', () => { beforeEach(() => { (objectCache.hasBySelfLink as any).and.returnValue(true); - (objectCache.hasById as any).and.returnValue(true); + (objectCache.hasByUUID as any).and.returnValue(true); spyOn(serviceAsAny, 'hasByHref').and.returnValue(false); }); diff --git a/src/app/shared/mocks/mock-object-cache.service.ts b/src/app/shared/mocks/mock-object-cache.service.ts index d096cfdf5f..9e35a519ff 100644 --- a/src/app/shared/mocks/mock-object-cache.service.ts +++ b/src/app/shared/mocks/mock-object-cache.service.ts @@ -4,12 +4,12 @@ export function getMockObjectCacheService(): ObjectCacheService { return jasmine.createSpyObj('objectCacheService', [ 'add', 'remove', - 'getByID', + 'getByUUID', 'getBySelfLink', 'getRequestHrefBySelfLink', 'getRequestHrefByUUID', 'getList', - 'hasById', + 'hasByUUID', 'hasBySelfLink' ]); From dc43e2330149faffb52356f258d7d4fac95ce828 Mon Sep 17 00:00:00 2001 From: Michael W Spalti Date: Fri, 1 Nov 2019 13:40:19 -0700 Subject: [PATCH 22/22] Added not found for identifier message to i18n. --- resources/i18n/en.json5 | 1 + .../objectnotfound/objectnotfound.component.html | 2 +- .../objectnotfound/objectnotfound.component.ts | 6 ++---- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/resources/i18n/en.json5 b/resources/i18n/en.json5 index 0ad08652b5..583b88b662 100644 --- a/resources/i18n/en.json5 +++ b/resources/i18n/en.json5 @@ -204,6 +204,7 @@ "error.collection": "Error fetching collection", "error.collections": "Error fetching collections", "error.community": "Error fetching community", + "error.identifier": "No item found for the identifier", "error.default": "Error", "error.item": "Error fetching item", "error.items": "Error fetching items", diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html index 662d3cde52..e1cf58b5b2 100644 --- a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.html @@ -1,5 +1,5 @@
-

{{"error.item" | translate}}

+

{{"error.identifier" | translate}}

{{missingItem}}


diff --git a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts index 0116575154..813b56920a 100644 --- a/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts +++ b/src/app/+lookup-by-id/objectnotfound/objectnotfound.component.ts @@ -33,10 +33,8 @@ export class ObjectNotFoundComponent implements OnInit { } ngOnInit(): void { - if (this.idType.startsWith('handle')) { - this.missingItem = 'handle: ' + this.id; - } else if (this.idType.startsWith('uuid')) { - this.missingItem = 'uuid: ' + this.id; + if (this.idType.startsWith('handle') || this.idType.startsWith('uuid')) { + this.missingItem = this.idType + ': ' + this.id; } else { this.missingItem = 'handle: ' + this.idType + '/' + this.id; }