From 32d0c8118bf50a3a653eb679f341b99d47f230c0 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Thu, 20 Feb 2020 18:23:08 +0100 Subject: [PATCH 01/13] 68930: add embed query param for followLinks in buildHref + tests removed fdescribe --- .java-version | 1 + .../core/cache/builders/link.service.spec.ts | 10 +- src/app/core/data/data.service.spec.ts | 97 ++++++++++++++++--- src/app/core/data/data.service.ts | 59 +++++++++-- .../shared/utils/follow-link-config.model.ts | 9 ++ 5 files changed, 149 insertions(+), 27 deletions(-) create mode 100644 .java-version diff --git a/.java-version b/.java-version new file mode 100644 index 0000000000..6259340971 --- /dev/null +++ b/.java-version @@ -0,0 +1 @@ +1.8 diff --git a/src/app/core/cache/builders/link.service.spec.ts b/src/app/core/cache/builders/link.service.spec.ts index dbd65eefb5..e9b8447c22 100644 --- a/src/app/core/cache/builders/link.service.spec.ts +++ b/src/app/core/cache/builders/link.service.spec.ts @@ -90,7 +90,7 @@ describe('LinkService', () => { propertyName: 'predecessor' }); spyOnFunction(decorators, 'getDataServiceFor').and.returnValue(TestDataService); - service.resolveLink(testModel, followLink('predecessor', {}, followLink('successor'))) + service.resolveLink(testModel, followLink('predecessor', {}, true, followLink('successor'))) }); it('should call dataservice.findByHref with the correct href and nested links', () => { expect(testDataService.findByHref).toHaveBeenCalledWith(testModel._links.predecessor.href, followLink('successor')); @@ -105,7 +105,7 @@ describe('LinkService', () => { isList: true }); spyOnFunction(decorators, 'getDataServiceFor').and.returnValue(TestDataService); - service.resolveLink(testModel, followLink('predecessor', { some: 'options '} as any, followLink('successor'))) + service.resolveLink(testModel, followLink('predecessor', { some: 'options '} as any, true, followLink('successor'))) }); it('should call dataservice.findAllByHref with the correct href, findListOptions, and nested links', () => { expect(testDataService.findAllByHref).toHaveBeenCalledWith(testModel._links.predecessor.href, { some: 'options '} as any, followLink('successor')); @@ -119,7 +119,7 @@ describe('LinkService', () => { propertyName: 'predecessor' }); spyOnFunction(decorators, 'getDataServiceFor').and.returnValue(TestDataService); - result = service.resolveLink(testModel, followLink('predecessor', {}, followLink('successor'))) + result = service.resolveLink(testModel, followLink('predecessor', {}, true, followLink('successor'))) }); it('should call getLinkDefinition with the correct model and link', () => { @@ -144,7 +144,7 @@ describe('LinkService', () => { }); it('should throw an error', () => { expect(() => { - service.resolveLink(testModel, followLink('predecessor', {}, followLink('successor'))) + service.resolveLink(testModel, followLink('predecessor', {}, true, followLink('successor'))) }).toThrow(); }); }); @@ -160,7 +160,7 @@ describe('LinkService', () => { }); it('should throw an error', () => { expect(() => { - service.resolveLink(testModel, followLink('predecessor', {}, followLink('successor'))) + service.resolveLink(testModel, followLink('predecessor', {}, true, followLink('successor'))) }).toThrow(); }); }); diff --git a/src/app/core/data/data.service.spec.ts b/src/app/core/data/data.service.spec.ts index 347dfa83a4..177d7b5683 100644 --- a/src/app/core/data/data.service.spec.ts +++ b/src/app/core/data/data.service.spec.ts @@ -1,21 +1,23 @@ -import { DataService } from './data.service'; -import { RequestService } from './request.service'; -import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; -import { CoreState } from '../core.reducers'; +import { HttpClient } from '@angular/common/http'; import { Store } from '@ngrx/store'; -import { HALEndpointService } from '../shared/hal-endpoint.service'; +import { compare, Operation } from 'fast-json-patch'; import { Observable, of as observableOf } from 'rxjs'; -import { FindListOptions } from './request.models'; +import * as uuidv4 from 'uuid/v4'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { createSuccessfulRemoteDataObject$ } from '../../shared/testing/utils'; +import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model'; +import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { SortDirection, SortOptions } from '../cache/models/sort-options.model'; import { ObjectCacheService } from '../cache/object-cache.service'; -import { compare, Operation } from 'fast-json-patch'; +import { CoreState } from '../core.reducers'; +import { Collection } from '../shared/collection.model'; import { DSpaceObject } from '../shared/dspace-object.model'; -import { ChangeAnalyzer } from './change-analyzer'; -import { HttpClient } from '@angular/common/http'; -import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { HALEndpointService } from '../shared/hal-endpoint.service'; import { Item } from '../shared/item.model'; -import * as uuidv4 from 'uuid/v4'; -import { createSuccessfulRemoteDataObject$ } from '../../shared/testing/utils'; +import { ChangeAnalyzer } from './change-analyzer'; +import { DataService } from './data.service'; +import { FindListOptions } from './request.models'; +import { RequestService } from './request.service'; const endpoint = 'https://rest.api/core'; @@ -40,6 +42,7 @@ class TestService extends DataService { return observableOf(endpoint); } } + class DummyChangeAnalyzer implements ChangeAnalyzer { diff(object1: Item, object2: Item): Operation[] { return compare((object1 as any).metadata, (object2 as any).metadata); @@ -50,7 +53,7 @@ class DummyChangeAnalyzer implements ChangeAnalyzer { describe('DataService', () => { let service: TestService; let options: FindListOptions; - const requestService = {generateRequestId: () => uuidv4()} as RequestService; + const requestService = { generateRequestId: () => uuidv4() } as RequestService; const halService = {} as HALEndpointService; const rdbService = {} as RemoteDataBuildService; const notificationsService = {} as NotificationsService; @@ -144,7 +147,73 @@ describe('DataService', () => { (service as any).getFindAllHref(options).subscribe((value) => { expect(value).toBe(expected); }); - }) + }); + + it('should include single linksToFollow as embed', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + }); + const expected = `${endpoint}?embed=bundles`; + + (service as any).getFindAllHref({}, null, mockFollowLinkConfig).subscribe((value) => { + expect(value).toBe(expected); + }); + }); + + it('should include multiple linksToFollow as embed', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + }); + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'templateItemOf' as any, + }); + const expected = `${endpoint}?embed=bundles&embed=owningCollection&embed=templateItemOf`; + + (service as any).getFindAllHref({}, null, mockFollowLinkConfig, mockFollowLinkConfig2, mockFollowLinkConfig3).subscribe((value) => { + expect(value).toBe(expected); + }); + }); + + it('should not include linksToFollow with forwardToRest = false', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + forwardToRest: false, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + forwardToRest: false, + }); + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'templateItemOf' as any, + }); + const expected = `${endpoint}?embed=templateItemOf`; + + (service as any).getFindAllHref({}, null, mockFollowLinkConfig, mockFollowLinkConfig2, mockFollowLinkConfig3).subscribe((value) => { + expect(value).toBe(expected); + }); + }); + + it('should include nested linksToFollow 3lvl', () => { + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'relationships' as any, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'itemtemplate' as any, + linksToFollow: mockFollowLinkConfig3, + }); + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + linksToFollow: mockFollowLinkConfig2, + }); + const expected = `${endpoint}?embed=owningCollection/itemtemplate/relationships`; + + (service as any).getFindAllHref({}, null, mockFollowLinkConfig).subscribe((value) => { + expect(value).toBe(expected); + }); + }); }); describe('patch', () => { let operations; diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 7dcfb6bd6e..5bd5db0af7 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -83,14 +83,15 @@ export abstract class DataService { * @param linkPath The link path for the object * @return {Observable} * Return an observable that emits created HREF + * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ - protected getFindAllHref(options: FindListOptions = {}, linkPath?: string): Observable { + protected getFindAllHref(options: FindListOptions = {}, linkPath?: string, ...linksToFollow: Array>): Observable { let result$: Observable; const args = []; result$ = this.getBrowseEndpoint(options, linkPath).pipe(distinctUntilChanged()); - return result$.pipe(map((result: string) => this.buildHrefFromFindOptions(result, options, args))); + return result$.pipe(map((result: string) => this.buildHrefFromFindOptions(result, options, args, ...linksToFollow))); } /** @@ -100,8 +101,9 @@ export abstract class DataService { * @param options The [[FindListOptions]] object * @return {Observable} * Return an observable that emits created HREF + * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ - protected getSearchByHref(searchMethod: string, options: FindListOptions = {}): Observable { + protected getSearchByHref(searchMethod: string, options: FindListOptions = {}, ...linksToFollow: Array>): Observable { let result$: Observable; const args = []; @@ -113,7 +115,7 @@ export abstract class DataService { }) } - return result$.pipe(map((result: string) => this.buildHrefFromFindOptions(result, options, args))); + return result$.pipe(map((result: string) => this.buildHrefFromFindOptions(result, options, args, ...linksToFollow))); } /** @@ -124,8 +126,9 @@ export abstract class DataService { * @param extraArgs Array with additional params to combine with query string * @return {Observable} * Return an observable that emits created HREF + * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ - protected buildHrefFromFindOptions(href: string, options: FindListOptions, extraArgs: string[] = []): string { + protected buildHrefFromFindOptions(href: string, options: FindListOptions, extraArgs: string[] = [], ...linksToFollow: Array>): string { let args = [...extraArgs]; @@ -142,6 +145,7 @@ export abstract class DataService { if (hasValue(options.startsWith)) { args = [...args, `startsWith=${options.startsWith}`]; } + args = this.addEmbedParams(args, linksToFollow); if (isNotEmpty(args)) { return new URLCombiner(href, `?${args.join('&')}`).toString(); } else { @@ -149,6 +153,45 @@ export abstract class DataService { } } + /** + * Adds the embed options to the link for the request + * @param args params for the query string + * @param linksToFollow links we want to embed in query string if forwardToRest is true + */ + protected addEmbedParams(args: any, linksToFollow: Array>) { + if (linksToFollow !== undefined) { + linksToFollow.forEach((linkToFollow: FollowLinkConfig) => { + console.log('linksToFollow', linksToFollow) + if (linkToFollow.forwardToRest) { + const embedString = 'embed=' + String(linkToFollow.name); + const embedWithNestedString = this.addNestedEmbeds(embedString, linkToFollow.linksToFollow); + args = [...args, embedWithNestedString]; + } + }); + } + return args; + } + + /** + * Add the nested followLinks to the embed param, recursively, separated by a / + * @param embedString + * @param linksToFollow + */ + protected addNestedEmbeds(embedString: string, linksToFollow: Array>): string { + let nestEmbed = embedString; + if (linksToFollow !== undefined) { + linksToFollow.forEach((linkToFollow: FollowLinkConfig) => { + if (linkToFollow.forwardToRest) { + nestEmbed = nestEmbed + '/' + String(linkToFollow.name); + if (linkToFollow.linksToFollow !== undefined) { + nestEmbed = this.addNestedEmbeds(nestEmbed, linkToFollow.linksToFollow); + } + } + }) + } + return nestEmbed; + } + /** * Returns {@link RemoteData} of all object with a list of {@link FollowLinkConfig}, to indicate which embedded * info should be added to the objects @@ -223,7 +266,7 @@ export abstract class DataService { * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ findByHref(href: string, ...linksToFollow: Array>): Observable> { - const requestHref = this.buildHrefFromFindOptions(href, {}, []); + const requestHref = this.buildHrefFromFindOptions(href, {}, [], ...linksToFollow); const request = new GetRequest(this.requestService.generateRequestId(), requestHref); if (hasValue(this.responseMsToLive)) { request.responseMsToLive = this.responseMsToLive; @@ -240,7 +283,7 @@ export abstract class DataService { * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ findAllByHref(href: string, findListOptions: FindListOptions = {}, ...linksToFollow: Array>): Observable>> { - const requestHref = this.buildHrefFromFindOptions(href, findListOptions, []); + const requestHref = this.buildHrefFromFindOptions(href, findListOptions, [], ...linksToFollow); const request = new GetRequest(this.requestService.generateRequestId(), requestHref); if (hasValue(this.responseMsToLive)) { request.responseMsToLive = this.responseMsToLive; @@ -271,7 +314,7 @@ export abstract class DataService { */ protected searchBy(searchMethod: string, options: FindListOptions = {}, ...linksToFollow: Array>): Observable>> { - const hrefObs = this.getSearchByHref(searchMethod, options); + const hrefObs = this.getSearchByHref(searchMethod, options, ...linksToFollow); return hrefObs.pipe( find((href: string) => hasValue(href)), diff --git a/src/app/shared/utils/follow-link-config.model.ts b/src/app/shared/utils/follow-link-config.model.ts index 21df288690..ea295bf78f 100644 --- a/src/app/shared/utils/follow-link-config.model.ts +++ b/src/app/shared/utils/follow-link-config.model.ts @@ -23,6 +23,11 @@ export class FollowLinkConfig { * use on the retrieved object. */ linksToFollow?: Array>; + + /** + * Forward to rest which links we're following, so these can already be embedded + */ + forwardToRest? = true; } /** @@ -36,15 +41,19 @@ export class FollowLinkConfig { * in a certain way * @param linksToFollow: a list of {@link FollowLinkConfig}s to * use on the retrieved object. + * @param forwardToRest: boolean to check whether to forward info on followLinks to rest, + * so these can be embedded, default true */ export const followLink = ( linkName: keyof R['_links'], findListOptions?: FindListOptions, + forwardToRest = true, ...linksToFollow: Array> ): FollowLinkConfig => { return { name: linkName, findListOptions, + forwardToRest, linksToFollow } }; From dffcd745afe8abbafb967fffa813dedc9b784675 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 21 Feb 2020 15:53:05 +0100 Subject: [PATCH 02/13] 68930: rename forwardToRest, embed query param added to getIDHref, tests wip --- src/app/core/data/data.service.spec.ts | 75 ++++++++++++++++++- src/app/core/data/data.service.ts | 37 ++++----- .../core/data/dso-redirect-data.service.ts | 8 +- .../core/data/dspace-object-data.service.ts | 6 +- .../shared/utils/follow-link-config.model.ts | 8 +- 5 files changed, 104 insertions(+), 30 deletions(-) diff --git a/src/app/core/data/data.service.spec.ts b/src/app/core/data/data.service.spec.ts index 177d7b5683..fcb7929fad 100644 --- a/src/app/core/data/data.service.spec.ts +++ b/src/app/core/data/data.service.spec.ts @@ -177,14 +177,14 @@ describe('DataService', () => { }); }); - it('should not include linksToFollow with forwardToRest = false', () => { + it('should not include linksToFollow with shootEmbed = false', () => { const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'bundles' as any, - forwardToRest: false, + shootEmbed: false, }); const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'owningCollection' as any, - forwardToRest: false, + shootEmbed: false, }); const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'templateItemOf' as any, @@ -215,6 +215,75 @@ describe('DataService', () => { }); }); }); + + describe('getIDHref', () => { + const endpointMock = 'https://dspace7-internal.atmire.com/server/api/core/items'; + const resourceIdMock = '003c99b4-d4fe-44b0-a945-e12182a7ca89'; + + it('should return endpoint', () => { + const result = (service as any).getIDHref(endpointMock, resourceIdMock); + expect(result).toEqual(endpointMock + '/' + resourceIdMock); + }); + + it('should include single linksToFollow as embed', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + }); + const expected = `${endpointMock}/${resourceIdMock}?embed=bundles`; + const result = (service as any).getIDHref(endpointMock, resourceIdMock, mockFollowLinkConfig); + expect(result).toEqual(expected); + }); + + it('should include multiple linksToFollow as embed', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + }); + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'templateItemOf' as any, + }); + const expected = `${endpointMock}/${resourceIdMock}?embed=bundles&embed=owningCollection&embed=templateItemOf`; + const result = (service as any).getIDHref(endpointMock, resourceIdMock, mockFollowLinkConfig, mockFollowLinkConfig2, mockFollowLinkConfig3); + expect(result).toEqual(expected); + }); + + it('should not include linksToFollow with shootEmbed = false', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + shootEmbed: false, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + shootEmbed: false, + }); + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'templateItemOf' as any, + }); + const expected = `${endpointMock}/${resourceIdMock}?embed=templateItemOf`; + const result = (service as any).getIDHref(endpointMock, resourceIdMock, mockFollowLinkConfig, mockFollowLinkConfig2, mockFollowLinkConfig3); + expect(result).toEqual(expected); + }); + + it('should include nested linksToFollow 3lvl', () => { + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'relationships' as any, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'itemtemplate' as any, + linksToFollow: mockFollowLinkConfig3, + }); + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + linksToFollow: mockFollowLinkConfig2, + }); + const expected = `${endpointMock}/${resourceIdMock}?embed=owningCollection/itemtemplate/relationships`; + const result = (service as any).getIDHref(endpointMock, resourceIdMock, mockFollowLinkConfig); + expect(result).toEqual(expected); + }); + }); + describe('patch', () => { let operations; let selfLink; diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 5bd5db0af7..ee48ca1a83 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -129,7 +129,6 @@ export abstract class DataService { * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ protected buildHrefFromFindOptions(href: string, options: FindListOptions, extraArgs: string[] = [], ...linksToFollow: Array>): string { - let args = [...extraArgs]; if (hasValue(options.currentPage) && typeof options.currentPage === 'number') { @@ -145,7 +144,7 @@ export abstract class DataService { if (hasValue(options.startsWith)) { args = [...args, `startsWith=${options.startsWith}`]; } - args = this.addEmbedParams(args, linksToFollow); + args = this.addEmbedParams(args, ...linksToFollow); if (isNotEmpty(args)) { return new URLCombiner(href, `?${args.join('&')}`).toString(); } else { @@ -156,15 +155,15 @@ export abstract class DataService { /** * Adds the embed options to the link for the request * @param args params for the query string - * @param linksToFollow links we want to embed in query string if forwardToRest is true + * @param linksToFollow links we want to embed in query string if shootEmbed is true */ - protected addEmbedParams(args: any, linksToFollow: Array>) { + protected addEmbedParams(args: any, ...linksToFollow: Array>) { if (linksToFollow !== undefined) { - linksToFollow.forEach((linkToFollow: FollowLinkConfig) => { + [...linksToFollow].forEach((linkToFollow: FollowLinkConfig) => { console.log('linksToFollow', linksToFollow) - if (linkToFollow.forwardToRest) { + if (linkToFollow.shootEmbed) { const embedString = 'embed=' + String(linkToFollow.name); - const embedWithNestedString = this.addNestedEmbeds(embedString, linkToFollow.linksToFollow); + const embedWithNestedString = this.addNestedEmbeds(embedString, ...linkToFollow.linksToFollow); args = [...args, embedWithNestedString]; } }); @@ -174,17 +173,18 @@ export abstract class DataService { /** * Add the nested followLinks to the embed param, recursively, separated by a / - * @param embedString - * @param linksToFollow + * @param embedString embedString so far (recursive) + * @param linksToFollow links we want to embed in query string if shootEmbed is true */ - protected addNestedEmbeds(embedString: string, linksToFollow: Array>): string { + protected addNestedEmbeds(embedString: string, ...linksToFollow: Array>): string { let nestEmbed = embedString; if (linksToFollow !== undefined) { - linksToFollow.forEach((linkToFollow: FollowLinkConfig) => { - if (linkToFollow.forwardToRest) { + console.log('linksToFollow addNestedEmbed', linksToFollow); + [...linksToFollow].forEach((linkToFollow: FollowLinkConfig) => { + if (linkToFollow.shootEmbed) { nestEmbed = nestEmbed + '/' + String(linkToFollow.name); if (linkToFollow.linksToFollow !== undefined) { - nestEmbed = this.addNestedEmbeds(nestEmbed, linkToFollow.linksToFollow); + nestEmbed = this.addNestedEmbeds(nestEmbed, ...linkToFollow.linksToFollow); } } }) @@ -227,12 +227,13 @@ export abstract class DataService { } /** - * Create the HREF for a specific object based on its identifier + * Create the HREF for a specific object based on its identifier; with possible embed query params based on linksToFollow * @param endpoint The base endpoint for the type of object * @param resourceID The identifier for the object + * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ - getIDHref(endpoint, resourceID): string { - return `${endpoint}/${resourceID}`; + getIDHref(endpoint, resourceID, ...linksToFollow: Array>): string { + return this.buildHrefFromFindOptions(endpoint + '/' + resourceID, {}, [], ...linksToFollow); } /** @@ -242,9 +243,9 @@ export abstract class DataService { * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ findById(id: string, ...linksToFollow: Array>): Observable> { - + console.log('findById'); const hrefObs = this.halService.getEndpoint(this.linkPath).pipe( - map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id)))); + map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id), ...linksToFollow))); 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 232fde65d0..87259a4279 100644 --- a/src/app/core/data/dso-redirect-data.service.ts +++ b/src/app/core/data/dso-redirect-data.service.ts @@ -6,6 +6,7 @@ import { Observable } from 'rxjs'; import { take, tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { ObjectCacheService } from '../cache/object-cache.service'; import { CoreState } from '../core.reducers'; @@ -45,10 +46,11 @@ export class DsoRedirectDataService extends DataService { } } - getIDHref(endpoint, resourceID): string { + getIDHref(endpoint, resourceID, ...linksToFollow: Array>): string { // Supporting both identifier (pid) and uuid (dso) endpoints - return endpoint.replace(/\{\?id\}/, `?id=${resourceID}`) - .replace(/\{\?uuid\}/, `?uuid=${resourceID}`); + return this.buildHrefFromFindOptions( endpoint.replace(/\{\?id\}/, `?id=${resourceID}`) + .replace(/\{\?uuid\}/, `?uuid=${resourceID}`), + {}, [], ...linksToFollow); } findByIdAndIDType(id: string, identifierType = IdentifierType.UUID): Observable> { diff --git a/src/app/core/data/dspace-object-data.service.ts b/src/app/core/data/dspace-object-data.service.ts index 38e9f8d888..61cc98281e 100644 --- a/src/app/core/data/dspace-object-data.service.ts +++ b/src/app/core/data/dspace-object-data.service.ts @@ -3,6 +3,7 @@ import { Injectable } from '@angular/core'; import { Store } from '@ngrx/store'; import { Observable } from 'rxjs'; import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model'; import { dataService } from '../cache/builders/build-decorators'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { ObjectCacheService } from '../cache/object-cache.service'; @@ -31,8 +32,9 @@ class DataServiceImpl extends DataService { super(); } - getIDHref(endpoint, resourceID): string { - return endpoint.replace(/\{\?uuid\}/, `?uuid=${resourceID}`); + getIDHref(endpoint, resourceID, ...linksToFollow: Array>): string { + return this.buildHrefFromFindOptions( endpoint.replace(/\{\?uuid\}/, `?uuid=${resourceID}`), + {}, [], ...linksToFollow); } } diff --git a/src/app/shared/utils/follow-link-config.model.ts b/src/app/shared/utils/follow-link-config.model.ts index ea295bf78f..49866dc641 100644 --- a/src/app/shared/utils/follow-link-config.model.ts +++ b/src/app/shared/utils/follow-link-config.model.ts @@ -27,7 +27,7 @@ export class FollowLinkConfig { /** * Forward to rest which links we're following, so these can already be embedded */ - forwardToRest? = true; + shootEmbed? = true; } /** @@ -41,19 +41,19 @@ export class FollowLinkConfig { * in a certain way * @param linksToFollow: a list of {@link FollowLinkConfig}s to * use on the retrieved object. - * @param forwardToRest: boolean to check whether to forward info on followLinks to rest, + * @param shootEmbed: boolean to check whether to forward info on followLinks to rest, * so these can be embedded, default true */ export const followLink = ( linkName: keyof R['_links'], findListOptions?: FindListOptions, - forwardToRest = true, + shootEmbed = true, ...linksToFollow: Array> ): FollowLinkConfig => { return { name: linkName, findListOptions, - forwardToRest, + shootEmbed: shootEmbed, linksToFollow } }; From 1f71274db66eab16d0989567dcd8b21a22a5bd5a Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 21 Feb 2020 15:55:41 +0100 Subject: [PATCH 03/13] 68930: rename forwardToRest typo fix --- src/app/core/data/data.service.spec.ts | 12 ++++++------ src/app/core/data/data.service.ts | 8 ++++---- src/app/shared/utils/follow-link-config.model.ts | 8 ++++---- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/app/core/data/data.service.spec.ts b/src/app/core/data/data.service.spec.ts index fcb7929fad..c5748b05fb 100644 --- a/src/app/core/data/data.service.spec.ts +++ b/src/app/core/data/data.service.spec.ts @@ -177,14 +177,14 @@ describe('DataService', () => { }); }); - it('should not include linksToFollow with shootEmbed = false', () => { + it('should not include linksToFollow with shouldEmbed = false', () => { const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'bundles' as any, - shootEmbed: false, + shouldEmbed: false, }); const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'owningCollection' as any, - shootEmbed: false, + shouldEmbed: false, }); const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'templateItemOf' as any, @@ -249,14 +249,14 @@ describe('DataService', () => { expect(result).toEqual(expected); }); - it('should not include linksToFollow with shootEmbed = false', () => { + it('should not include linksToFollow with shouldEmbed = false', () => { const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'bundles' as any, - shootEmbed: false, + shouldEmbed: false, }); const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'owningCollection' as any, - shootEmbed: false, + shouldEmbed: false, }); const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { name: 'templateItemOf' as any, diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index ee48ca1a83..71f4295b68 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -155,13 +155,13 @@ export abstract class DataService { /** * Adds the embed options to the link for the request * @param args params for the query string - * @param linksToFollow links we want to embed in query string if shootEmbed is true + * @param linksToFollow links we want to embed in query string if shouldEmbed is true */ protected addEmbedParams(args: any, ...linksToFollow: Array>) { if (linksToFollow !== undefined) { [...linksToFollow].forEach((linkToFollow: FollowLinkConfig) => { console.log('linksToFollow', linksToFollow) - if (linkToFollow.shootEmbed) { + if (linkToFollow.shouldEmbed) { const embedString = 'embed=' + String(linkToFollow.name); const embedWithNestedString = this.addNestedEmbeds(embedString, ...linkToFollow.linksToFollow); args = [...args, embedWithNestedString]; @@ -174,14 +174,14 @@ export abstract class DataService { /** * Add the nested followLinks to the embed param, recursively, separated by a / * @param embedString embedString so far (recursive) - * @param linksToFollow links we want to embed in query string if shootEmbed is true + * @param linksToFollow links we want to embed in query string if shouldEmbed is true */ protected addNestedEmbeds(embedString: string, ...linksToFollow: Array>): string { let nestEmbed = embedString; if (linksToFollow !== undefined) { console.log('linksToFollow addNestedEmbed', linksToFollow); [...linksToFollow].forEach((linkToFollow: FollowLinkConfig) => { - if (linkToFollow.shootEmbed) { + if (linkToFollow.shouldEmbed) { nestEmbed = nestEmbed + '/' + String(linkToFollow.name); if (linkToFollow.linksToFollow !== undefined) { nestEmbed = this.addNestedEmbeds(nestEmbed, ...linkToFollow.linksToFollow); diff --git a/src/app/shared/utils/follow-link-config.model.ts b/src/app/shared/utils/follow-link-config.model.ts index 49866dc641..87942d8467 100644 --- a/src/app/shared/utils/follow-link-config.model.ts +++ b/src/app/shared/utils/follow-link-config.model.ts @@ -27,7 +27,7 @@ export class FollowLinkConfig { /** * Forward to rest which links we're following, so these can already be embedded */ - shootEmbed? = true; + shouldEmbed? = true; } /** @@ -41,19 +41,19 @@ export class FollowLinkConfig { * in a certain way * @param linksToFollow: a list of {@link FollowLinkConfig}s to * use on the retrieved object. - * @param shootEmbed: boolean to check whether to forward info on followLinks to rest, + * @param shouldEmbed: boolean to check whether to forward info on followLinks to rest, * so these can be embedded, default true */ export const followLink = ( linkName: keyof R['_links'], findListOptions?: FindListOptions, - shootEmbed = true, + shouldEmbed = true, ...linksToFollow: Array> ): FollowLinkConfig => { return { name: linkName, findListOptions, - shootEmbed: shootEmbed, + shouldEmbed: shouldEmbed, linksToFollow } }; From 781a558458b784a0cb053c6df3414ea3faae2347 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 21 Feb 2020 16:44:32 +0100 Subject: [PATCH 04/13] 68930: getIDHref tests & removed undefined check, not needed because of spread array --- src/app/core/data/data.service.ts | 36 ++++---- .../data/dso-redirect-data.service.spec.ts | 83 +++++++++++++++++-- 2 files changed, 91 insertions(+), 28 deletions(-) diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 71f4295b68..d2abbf1e40 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -157,17 +157,14 @@ export abstract class DataService { * @param args params for the query string * @param linksToFollow links we want to embed in query string if shouldEmbed is true */ - protected addEmbedParams(args: any, ...linksToFollow: Array>) { - if (linksToFollow !== undefined) { - [...linksToFollow].forEach((linkToFollow: FollowLinkConfig) => { - console.log('linksToFollow', linksToFollow) - if (linkToFollow.shouldEmbed) { - const embedString = 'embed=' + String(linkToFollow.name); - const embedWithNestedString = this.addNestedEmbeds(embedString, ...linkToFollow.linksToFollow); - args = [...args, embedWithNestedString]; - } - }); - } + protected addEmbedParams(args: string[], ...linksToFollow: Array>) { + linksToFollow.forEach((linkToFollow: FollowLinkConfig) => { + if (linkToFollow !== undefined && linkToFollow.shouldEmbed) { + const embedString = 'embed=' + String(linkToFollow.name); + const embedWithNestedString = this.addNestedEmbeds(embedString, ...linkToFollow.linksToFollow); + args = [...args, embedWithNestedString]; + } + }); return args; } @@ -178,17 +175,14 @@ export abstract class DataService { */ protected addNestedEmbeds(embedString: string, ...linksToFollow: Array>): string { let nestEmbed = embedString; - if (linksToFollow !== undefined) { - console.log('linksToFollow addNestedEmbed', linksToFollow); - [...linksToFollow].forEach((linkToFollow: FollowLinkConfig) => { - if (linkToFollow.shouldEmbed) { - nestEmbed = nestEmbed + '/' + String(linkToFollow.name); - if (linkToFollow.linksToFollow !== undefined) { - nestEmbed = this.addNestedEmbeds(nestEmbed, ...linkToFollow.linksToFollow); - } + linksToFollow.forEach((linkToFollow: FollowLinkConfig) => { + if (linkToFollow !== undefined && linkToFollow.shouldEmbed) { + nestEmbed = nestEmbed + '/' + String(linkToFollow.name); + if (linkToFollow.linksToFollow !== undefined) { + nestEmbed = this.addNestedEmbeds(nestEmbed, ...linkToFollow.linksToFollow); } - }) - } + } + }); return nestEmbed; } 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 25a148d92b..4ef5bcb8b4 100644 --- a/src/app/core/data/dso-redirect-data.service.spec.ts +++ b/src/app/core/data/dso-redirect-data.service.spec.ts @@ -1,15 +1,18 @@ +import { HttpClient } from '@angular/common/http'; +import { Store } from '@ngrx/store'; import { cold, getTestScheduler } from 'jasmine-marbles'; import { TestScheduler } from 'rxjs/testing'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { CoreState } from '../core.reducers'; +import { Collection } from '../shared/collection.model'; import { HALEndpointService } from '../shared/hal-endpoint.service'; +import { Item } from '../shared/item.model'; +import { DsoRedirectDataService } from './dso-redirect-data.service'; 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 { DsoRedirectDataService } from './dso-redirect-data.service'; -import { Store } from '@ngrx/store'; -import { CoreState } from '../core.reducers'; describe('DsoRedirectDataService', () => { let scheduler: TestScheduler; @@ -148,5 +151,71 @@ describe('DsoRedirectDataService', () => { scheduler.flush(); expect(router.navigate).toHaveBeenCalledWith(['communities/' + remoteData.payload.uuid]); }); - }) + }); + + describe('getIDHref', () => { + it('should return endpoint', () => { + const result = (service as any).getIDHref(pidLink, dsoUUID); + expect(result).toEqual(requestUUIDURL); + }); + + it('should include single linksToFollow as embed', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + }); + const expected = `${requestUUIDURL}&embed=bundles`; + const result = (service as any).getIDHref(pidLink, dsoUUID, mockFollowLinkConfig); + expect(result).toEqual(expected); + }); + + it('should include multiple linksToFollow as embed', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + }); + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'templateItemOf' as any, + }); + const expected = `${requestUUIDURL}&embed=bundles&embed=owningCollection&embed=templateItemOf`; + const result = (service as any).getIDHref(pidLink, dsoUUID, mockFollowLinkConfig, mockFollowLinkConfig2, mockFollowLinkConfig3); + expect(result).toEqual(expected); + }); + + it('should not include linksToFollow with shouldEmbed = false', () => { + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'bundles' as any, + shouldEmbed: false, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + shouldEmbed: false, + }); + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'templateItemOf' as any, + }); + const expected = `${requestUUIDURL}&embed=templateItemOf`; + const result = (service as any).getIDHref(pidLink, dsoUUID, mockFollowLinkConfig, mockFollowLinkConfig2, mockFollowLinkConfig3); + expect(result).toEqual(expected); + }); + + it('should include nested linksToFollow 3lvl', () => { + const mockFollowLinkConfig3: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'relationships' as any, + }); + const mockFollowLinkConfig2: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'itemtemplate' as any, + linksToFollow: mockFollowLinkConfig3, + }); + const mockFollowLinkConfig: FollowLinkConfig = Object.assign(new FollowLinkConfig(), { + name: 'owningCollection' as any, + linksToFollow: mockFollowLinkConfig2, + }); + const expected = `${requestUUIDURL}&embed=owningCollection/itemtemplate/relationships`; + const result = (service as any).getIDHref(pidLink, dsoUUID, mockFollowLinkConfig); + expect(result).toEqual(expected); + }); + }); + }); From 525c333e097acb2ff637cb327b87f9fda5e433ec Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 21 Feb 2020 17:10:03 +0100 Subject: [PATCH 05/13] log removed --- src/app/core/data/data.service.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index d2abbf1e40..3e67675290 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -237,7 +237,6 @@ export abstract class DataService { * @param linksToFollow List of {@link FollowLinkConfig} that indicate which {@link HALLink}s should be automatically resolved */ findById(id: string, ...linksToFollow: Array>): Observable> { - console.log('findById'); const hrefObs = this.halService.getEndpoint(this.linkPath).pipe( map((endpoint: string) => this.getIDHref(endpoint, encodeURIComponent(id), ...linksToFollow))); From 677b3f63dedd6244bcbd4f2a65bd9a42b4c5994b Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Wed, 4 Mar 2020 11:01:20 +0100 Subject: [PATCH 06/13] Fixes after rebasing from upstream/master --- src/app/core/breadcrumbs/collection-breadcrumb.resolver.ts | 2 +- src/app/core/breadcrumbs/item-breadcrumb.resolver.ts | 4 ++-- .../claimed-task-search-result-detail-element.component.ts | 6 ++---- .../pool-search-result-detail-element.component.ts | 6 ++---- .../claimed-search-result-list-element.component.ts | 7 ++----- .../pool-search-result-list-element.component.ts | 7 ++----- 6 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/app/core/breadcrumbs/collection-breadcrumb.resolver.ts b/src/app/core/breadcrumbs/collection-breadcrumb.resolver.ts index d9df7cd767..7384a031db 100644 --- a/src/app/core/breadcrumbs/collection-breadcrumb.resolver.ts +++ b/src/app/core/breadcrumbs/collection-breadcrumb.resolver.ts @@ -21,7 +21,7 @@ export class CollectionBreadcrumbResolver extends DSOBreadcrumbResolver> { return [ - followLink('parentCommunity', undefined, + followLink('parentCommunity', undefined, true, followLink('parentCommunity') ) ]; diff --git a/src/app/core/breadcrumbs/item-breadcrumb.resolver.ts b/src/app/core/breadcrumbs/item-breadcrumb.resolver.ts index 8390c0e001..cd0c23cf82 100644 --- a/src/app/core/breadcrumbs/item-breadcrumb.resolver.ts +++ b/src/app/core/breadcrumbs/item-breadcrumb.resolver.ts @@ -21,8 +21,8 @@ export class ItemBreadcrumbResolver extends DSOBreadcrumbResolver { */ get followLinks(): Array> { return [ - followLink('owningCollection', undefined, - followLink('parentCommunity', undefined, + followLink('owningCollection', undefined, true, + followLink('parentCommunity', undefined, true, followLink('parentCommunity')) ), followLink('bundles'), diff --git a/src/app/shared/object-detail/my-dspace-result-detail-element/claimed-task-search-result/claimed-task-search-result-detail-element.component.ts b/src/app/shared/object-detail/my-dspace-result-detail-element/claimed-task-search-result/claimed-task-search-result-detail-element.component.ts index f6abb444d5..4647a4d4a7 100644 --- a/src/app/shared/object-detail/my-dspace-result-detail-element/claimed-task-search-result/claimed-task-search-result-detail-element.component.ts +++ b/src/app/shared/object-detail/my-dspace-result-detail-element/claimed-task-search-result/claimed-task-search-result-detail-element.component.ts @@ -49,10 +49,8 @@ export class ClaimedTaskSearchResultDetailElementComponent extends SearchResultD */ ngOnInit() { super.ngOnInit(); - this.linkService.resolveLink(this.dso, followLink( - 'workflowitem', - null, - followLink('item', null, followLink('bundles')), + this.linkService.resolveLink(this.dso, followLink('workflowitem', null, true, + followLink('item', null, true, followLink('bundles')), followLink('submitter') )); this.workflowitemRD$ = this.dso.workflowitem as Observable>; diff --git a/src/app/shared/object-detail/my-dspace-result-detail-element/pool-search-result/pool-search-result-detail-element.component.ts b/src/app/shared/object-detail/my-dspace-result-detail-element/pool-search-result/pool-search-result-detail-element.component.ts index afa4f57d78..423931225e 100644 --- a/src/app/shared/object-detail/my-dspace-result-detail-element/pool-search-result/pool-search-result-detail-element.component.ts +++ b/src/app/shared/object-detail/my-dspace-result-detail-element/pool-search-result/pool-search-result-detail-element.component.ts @@ -48,10 +48,8 @@ export class PoolSearchResultDetailElementComponent extends SearchResultDetailEl */ ngOnInit() { super.ngOnInit(); - this.linkService.resolveLink(this.dso, followLink( - 'workflowitem', - null, - followLink('item', null, followLink('bundles')), + this.linkService.resolveLink(this.dso, followLink('workflowitem', null, true, + followLink('item', null, true, followLink('bundles')), followLink('submitter') )); this.workflowitemRD$ = this.dso.workflowitem as Observable>; diff --git a/src/app/shared/object-list/my-dspace-result-list-element/claimed-search-result/claimed-search-result-list-element.component.ts b/src/app/shared/object-list/my-dspace-result-list-element/claimed-search-result/claimed-search-result-list-element.component.ts index 1648a16d59..cb46e25282 100644 --- a/src/app/shared/object-list/my-dspace-result-list-element/claimed-search-result/claimed-search-result-list-element.component.ts +++ b/src/app/shared/object-list/my-dspace-result-list-element/claimed-search-result/claimed-search-result-list-element.component.ts @@ -55,11 +55,8 @@ export class ClaimedSearchResultListElementComponent extends SearchResultListEle */ ngOnInit() { super.ngOnInit(); - this.linkService.resolveLink(this.dso, followLink( - 'workflowitem', - null, - followLink('item'), - followLink('submitter') + this.linkService.resolveLink(this.dso, followLink('workflowitem', null, true, + followLink('item'), followLink('submitter') )); this.workflowitemRD$ = this.dso.workflowitem as Observable>; } diff --git a/src/app/shared/object-list/my-dspace-result-list-element/pool-search-result/pool-search-result-list-element.component.ts b/src/app/shared/object-list/my-dspace-result-list-element/pool-search-result/pool-search-result-list-element.component.ts index f3368cf64c..8ab00f4b9b 100644 --- a/src/app/shared/object-list/my-dspace-result-list-element/pool-search-result/pool-search-result-list-element.component.ts +++ b/src/app/shared/object-list/my-dspace-result-list-element/pool-search-result/pool-search-result-list-element.component.ts @@ -58,11 +58,8 @@ export class PoolSearchResultListElementComponent extends SearchResultListElemen */ ngOnInit() { super.ngOnInit(); - this.linkService.resolveLink(this.dso, followLink( - 'workflowitem', - null, - followLink('item'), - followLink('submitter') + this.linkService.resolveLink(this.dso, followLink('workflowitem', null, true, + followLink('item'), followLink('submitter') )); this.workflowitemRD$ = this.dso.workflowitem as Observable>; } From 373acc6c82a62b30b8ce8910d4b535e4e987f2e0 Mon Sep 17 00:00:00 2001 From: Ben Bosman Date: Thu, 5 Mar 2020 11:01:19 +0100 Subject: [PATCH 07/13] Refusing to cache an object which can't be parsed --- src/app/core/data/base-response-parsing.service.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/core/data/base-response-parsing.service.ts b/src/app/core/data/base-response-parsing.service.ts index 3615ab4023..3d00c5d0bd 100644 --- a/src/app/core/data/base-response-parsing.service.ts +++ b/src/app/core/data/base-response-parsing.service.ts @@ -117,10 +117,12 @@ export abstract class BaseResponseParsingService { const serializer = new this.serializerConstructor(objConstructor); return serializer.deserialize(obj); } else { + console.warn('cannot deserialize type ' + type); return null; } } else { + console.warn('cannot deserialize type ' + type); return null; } } @@ -142,7 +144,8 @@ export abstract class BaseResponseParsingService { } else { dataJSON = JSON.stringify(data); } - throw new Error(`Can't cache incomplete ${type}: ${JSON.stringify(co)}, parsed from (partial) response: ${dataJSON}`); + console.warn(`Can't cache incomplete ${type}: ${JSON.stringify(co)}, parsed from (partial) response: ${dataJSON}`); + return; } this.objectCache.add(co, hasValue(request.responseMsToLive) ? request.responseMsToLive : this.EnvConfig.cache.msToLive.default, request.uuid); } From 60d2f386df6c0d1df1e974797962ade236f3ac66 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Fri, 6 Mar 2020 13:16:15 +0100 Subject: [PATCH 08/13] 69314: BaseResponseParsingService test cases for cache and process --- .../base-response-parsing.service.spec.ts | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 src/app/core/data/base-response-parsing.service.spec.ts diff --git a/src/app/core/data/base-response-parsing.service.spec.ts b/src/app/core/data/base-response-parsing.service.spec.ts new file mode 100644 index 0000000000..a1d602dc65 --- /dev/null +++ b/src/app/core/data/base-response-parsing.service.spec.ts @@ -0,0 +1,104 @@ +import { BaseResponseParsingService } from './base-response-parsing.service'; +import { GlobalConfig } from '../../../config/global-config.interface'; +import { ObjectCacheService } from '../cache/object-cache.service'; +import { CacheableObject } from '../cache/object-cache.reducer'; +import { GetRequest, RestRequest } from './request.models'; +import { DSpaceObject } from '../shared/dspace-object.model'; + +/* tslint:disable:max-classes-per-file */ +class TestService extends BaseResponseParsingService { + toCache = true; + + constructor(protected EnvConfig: GlobalConfig, + protected objectCache: ObjectCacheService) { + super(); + } + + // Overwrite methods to make them public for testing + public process(data: any, request: RestRequest): any { + super.process(data, request); + } + + public cache(obj, request: RestRequest, data: any) { + super.cache(obj, request, data); + } +} + +describe('BaseResponseParsingService', () => { + let service: TestService; + let config: GlobalConfig; + let objectCache: ObjectCacheService; + + const requestUUID = 'request-uuid'; + const requestHref = 'request-href'; + const request = new GetRequest(requestUUID, requestHref); + + beforeEach(() => { + config = Object.assign({}); + objectCache = jasmine.createSpyObj('objectCache', { + add: {} + }); + service = new TestService(config, objectCache); + }); + + describe('cache', () => { + let obj: CacheableObject; + + describe('when the object is undefined', () => { + it('should not throw an error', () => { + expect(() => { service.cache(obj, request, {}) }).not.toThrow(); + }); + + it('should not call objectCache add', () => { + service.cache(obj, request, {}); + expect(objectCache.add).not.toHaveBeenCalled(); + }); + }); + + describe('when the object has a self link', () => { + beforeEach(() => { + obj = Object.assign(new DSpaceObject(), { + _links: { + self: { href: 'obj-selflink' } + } + }); + }); + + it('should call objectCache add', () => { + service.cache(obj, request, {}); + expect(objectCache.add).toHaveBeenCalledWith(obj, request.responseMsToLive, request.uuid); + }); + }); + }); + + describe('process', () => { + let data: any; + let result: any; + + describe('when data is valid, but not a real type', () => { + beforeEach(() => { + data = { + type: 'NotARealType', + _links: { + self: { href: 'data-selflink' } + } + }; + }); + + it('should not throw an error', () => { + expect(() => { result = service.process(data, request) }).not.toThrow(); + }); + + it('should return undefined', () => { + result = service.process(data, request); + expect(result).toBeUndefined(); + }); + + it('should not call objectCache add', () => { + result = service.process(data, request); + expect(objectCache.add).not.toHaveBeenCalled(); + }); + }); + }); +}); +/* tslint:enable:max-classes-per-file */ From 7803c2a97b9a2af1a1a7267eebefbac02a1c01c0 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Mon, 9 Mar 2020 10:30:02 +0100 Subject: [PATCH 09/13] local java environment file removed, accidentally commited --- .gitignore | 2 ++ .java-version | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) delete mode 100644 .java-version diff --git a/.gitignore b/.gitignore index 01fc9231bc..cbf74702cb 100644 --- a/.gitignore +++ b/.gitignore @@ -34,3 +34,5 @@ yarn-error.log *.css package-lock.json + +.java-version diff --git a/.java-version b/.java-version deleted file mode 100644 index 6259340971..0000000000 --- a/.java-version +++ /dev/null @@ -1 +0,0 @@ -1.8 From a970aeaab86a9d10b649045a11571c44d08eab61 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 11 Mar 2020 14:53:55 +0100 Subject: [PATCH 10/13] disregard embed url params when indexing and checking indexed request urls --- src/app/core/data/request.service.spec.ts | 2 ++ src/app/core/data/request.service.ts | 35 ++++++++++---------- src/app/core/index/index.effects.ts | 3 +- src/app/core/index/index.selectors.spec.ts | 32 +++++++++++++++++++ src/app/core/index/index.selectors.ts | 37 ++++++++++++++++++++-- src/app/core/url-combiner/url-combiner.ts | 4 +-- 6 files changed, 92 insertions(+), 21 deletions(-) create mode 100644 src/app/core/index/index.selectors.spec.ts diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 017721fdf9..23c27093e0 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -1,3 +1,4 @@ +import { NgZone } from '@angular/core'; import * as ngrx from '@ngrx/store'; import { ActionsSubject, Store } from '@ngrx/store'; import { cold, getTestScheduler, hot } from 'jasmine-marbles'; @@ -62,6 +63,7 @@ describe('RequestService', () => { objectCache, uuidService, store, + new NgZone({}), undefined ); serviceAsAny = service as any; diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 1101c851ac..c63490d8e7 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -1,9 +1,9 @@ -import { Injectable } from '@angular/core'; +import { Injectable, NgZone } from '@angular/core'; import { HttpHeaders } from '@angular/common/http'; import { createSelector, MemoizedSelector, select, Store } from '@ngrx/store'; import { Observable, race as observableRace } from 'rxjs'; -import { filter, map, mergeMap, switchMap, take } from 'rxjs/operators'; +import { filter, map, mergeMap, take } from 'rxjs/operators'; import { cloneDeep, remove } from 'lodash'; import { hasValue, isEmpty, isNotEmpty } from '../../shared/empty.util'; import { CacheableObject } from '../cache/object-cache.reducer'; @@ -80,6 +80,7 @@ export class RequestService { constructor(private objectCache: ObjectCacheService, private uuidService: UUIDService, private store: Store, + private zone: NgZone, private indexStore: Store) { } @@ -147,21 +148,23 @@ export class RequestService { * @param {RestRequest} request The request to send out */ configure(request: RestRequest): void { - const isGetRequest = request.method === RestRequestMethod.GET; - if (!isGetRequest || request.forceBypassCache || !this.isCachedOrPending(request)) { - this.dispatchRequest(request); - if (isGetRequest) { - this.trackRequestsOnTheirWayToTheStore(request); - } - } else { - this.getByHref(request.href).pipe( - filter((entry) => hasValue(entry)), - take(1) - ).subscribe((entry) => { - return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) + this.zone.runOutsideAngular(() => { + const isGetRequest = request.method === RestRequestMethod.GET; + if (!isGetRequest || request.forceBypassCache || !this.isCachedOrPending(request)) { + this.dispatchRequest(request); + if (isGetRequest) { + this.trackRequestsOnTheirWayToTheStore(request); } - ) - } + } else { + this.getByHref(request.href).pipe( + filter((entry) => hasValue(entry)), + take(1) + ).subscribe((entry) => { + return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) + } + ) + } + }); } /** diff --git a/src/app/core/index/index.effects.ts b/src/app/core/index/index.effects.ts index c9f6eace8f..f885db1436 100644 --- a/src/app/core/index/index.effects.ts +++ b/src/app/core/index/index.effects.ts @@ -12,6 +12,7 @@ import { AddToIndexAction, RemoveFromIndexByValueAction } from './index.actions' import { hasValue } from '../../shared/empty.util'; import { IndexName } from './index.reducer'; import { RestRequestMethod } from '../data/rest-request-method'; +import { getUrlWithoutEmbedParams } from './index.selectors'; @Injectable() export class UUIDIndexEffects { @@ -47,7 +48,7 @@ export class UUIDIndexEffects { map((action: RequestConfigureAction) => { return new AddToIndexAction( IndexName.REQUEST, - action.payload.href, + getUrlWithoutEmbedParams(action.payload.href), action.payload.uuid ); }) diff --git a/src/app/core/index/index.selectors.spec.ts b/src/app/core/index/index.selectors.spec.ts new file mode 100644 index 0000000000..02cce4b7d6 --- /dev/null +++ b/src/app/core/index/index.selectors.spec.ts @@ -0,0 +1,32 @@ +import { getUrlWithoutEmbedParams } from './index.selectors'; + +describe(`index selectors`, () => { + + describe(`getUrlWithoutEmbedParams`, () => { + + it(`should return a url without its embed params`, () => { + const source = 'https://rest.api/resource?a=1&embed=2&b=3&embed=4/5&c=6&embed=7/8/9'; + const result = getUrlWithoutEmbedParams(source); + expect(result).toBe('https://rest.api/resource?a=1&b=3&c=6'); + }); + + it(`should return a url without embed params unmodified`, () => { + const source = 'https://rest.api/resource?a=1&b=3&c=6'; + const result = getUrlWithoutEmbedParams(source); + expect(result).toBe(source); + }); + + it(`should return a string that isn't a url unmodified`, () => { + const source = 'a=1&embed=2&b=3&embed=4/5&c=6&embed=7/8/9'; + const result = getUrlWithoutEmbedParams(source); + expect(result).toBe(source); + }); + + it(`should return undefined or null unmodified`, () => { + expect(getUrlWithoutEmbedParams(undefined)).toBe(undefined); + expect(getUrlWithoutEmbedParams(null)).toBe(null); + }); + + }); + +}); diff --git a/src/app/core/index/index.selectors.ts b/src/app/core/index/index.selectors.ts index de4adab09b..b23496c501 100644 --- a/src/app/core/index/index.selectors.ts +++ b/src/app/core/index/index.selectors.ts @@ -1,8 +1,41 @@ import { createSelector, MemoizedSelector } from '@ngrx/store'; -import { hasValue } from '../../shared/empty.util'; +import { hasValue, isNotEmpty } from '../../shared/empty.util'; import { CoreState } from '../core.reducers'; import { coreSelector } from '../core.selectors'; +import { URLCombiner } from '../url-combiner/url-combiner'; import { IndexName, IndexState, MetaIndexState } from './index.reducer'; +import * as parse from 'url-parse'; + +/** + * Return the given url without `embed` params. + * + * E.g. https://rest.api/resource?size=5&embed=subresource&rpp=3 + * becomes https://rest.api/resource?size=5&rpp=3 + * + * When you index a request url you don't want to include + * embed params because embedded data isn't relevant when + * you want to know + * + * @param url The url to use + */ +export const getUrlWithoutEmbedParams = (url: string): string => { + if (isNotEmpty(url)) { + const parsed = parse(url); + if (isNotEmpty(parsed.query)) { + const parts = parsed.query.split(/[?|&]/) + .filter((part: string) => isNotEmpty(part)) + .filter((part: string) => !part.startsWith('embed=')); + let args = ''; + if (isNotEmpty(parts)) { + args = `?${parts.join('&')}`; + } + url = new URLCombiner(parsed.origin, parsed.pathname, args).toString(); + return url; + } + } + + return url; +}; /** * Return the MetaIndexState based on the CoreSate @@ -74,7 +107,7 @@ export const selfLinkFromUuidSelector = export const uuidFromHrefSelector = (href: string): MemoizedSelector => createSelector( requestIndexSelector, - (state: IndexState) => hasValue(state) ? state[href] : undefined + (state: IndexState) => hasValue(state) ? state[getUrlWithoutEmbedParams(href)] : undefined ); /** diff --git a/src/app/core/url-combiner/url-combiner.ts b/src/app/core/url-combiner/url-combiner.ts index ae622ab976..e7468c6107 100644 --- a/src/app/core/url-combiner/url-combiner.ts +++ b/src/app/core/url-combiner/url-combiner.ts @@ -41,8 +41,8 @@ export class URLCombiner { // remove consecutive slashes url = url.replace(/([^:\s])\/+/g, '$1/'); - // remove trailing slash before parameters or hash - url = url.replace(/\/(\?|&|#[^!])/g, '$1'); + // remove trailing slash + url = url.replace(/\/($|\?|&|#[^!])/g, '$1'); // replace ? in parameters with & url = url.replace(/(\?.+)\?/g, '$1&'); From 383dff736cc3ea044ee2239d9d965fbdffe3e8ae Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 11 Mar 2020 16:22:09 +0100 Subject: [PATCH 11/13] add comment to this.zone.runOutsideAngular --- src/app/core/data/request.service.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index c63490d8e7..a679577d61 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -148,6 +148,13 @@ export class RequestService { * @param {RestRequest} request The request to send out */ configure(request: RestRequest): void { + /** + * Since this method doesn't return anything, is used very often and has + * problems with actions being dispatched to the store but not reduced before + * that info is needed again, we may as well run it in a separate zone. That way + * it won't block the UI, and actions have a better chance of being already + * processed when the next isCachedOrPending call comes + */ this.zone.runOutsideAngular(() => { const isGetRequest = request.method === RestRequestMethod.GET; if (!isGetRequest || request.forceBypassCache || !this.isCachedOrPending(request)) { From 795d638aa66d29194227d69eda70743012e5527a Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 11 Mar 2020 16:51:32 +0100 Subject: [PATCH 12/13] remove zone.runOutsideAngular from requestService.configure due to AoT build issues --- src/app/core/data/request.service.spec.ts | 2 -- src/app/core/data/request.service.ts | 38 +++++++++-------------- 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 23c27093e0..017721fdf9 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -1,4 +1,3 @@ -import { NgZone } from '@angular/core'; import * as ngrx from '@ngrx/store'; import { ActionsSubject, Store } from '@ngrx/store'; import { cold, getTestScheduler, hot } from 'jasmine-marbles'; @@ -63,7 +62,6 @@ describe('RequestService', () => { objectCache, uuidService, store, - new NgZone({}), undefined ); serviceAsAny = service as any; diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index a679577d61..c2e7327d22 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -80,7 +80,6 @@ export class RequestService { constructor(private objectCache: ObjectCacheService, private uuidService: UUIDService, private store: Store, - private zone: NgZone, private indexStore: Store) { } @@ -148,30 +147,21 @@ export class RequestService { * @param {RestRequest} request The request to send out */ configure(request: RestRequest): void { - /** - * Since this method doesn't return anything, is used very often and has - * problems with actions being dispatched to the store but not reduced before - * that info is needed again, we may as well run it in a separate zone. That way - * it won't block the UI, and actions have a better chance of being already - * processed when the next isCachedOrPending call comes - */ - this.zone.runOutsideAngular(() => { - const isGetRequest = request.method === RestRequestMethod.GET; - if (!isGetRequest || request.forceBypassCache || !this.isCachedOrPending(request)) { - this.dispatchRequest(request); - if (isGetRequest) { - this.trackRequestsOnTheirWayToTheStore(request); - } - } else { - this.getByHref(request.href).pipe( - filter((entry) => hasValue(entry)), - take(1) - ).subscribe((entry) => { - return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) - } - ) + const isGetRequest = request.method === RestRequestMethod.GET; + if (!isGetRequest || request.forceBypassCache || !this.isCachedOrPending(request)) { + this.dispatchRequest(request); + if (isGetRequest) { + this.trackRequestsOnTheirWayToTheStore(request); } - }); + } else { + this.getByHref(request.href).pipe( + filter((entry) => hasValue(entry)), + take(1) + ).subscribe((entry) => { + return this.store.dispatch(new AddToIndexAction(IndexName.UUID_MAPPING, request.uuid, entry.request.uuid)) + } + ) + } } /** From c6c34b667a6f92910dd6cd24d6df5d6c74b73e46 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Wed, 11 Mar 2020 16:53:43 +0100 Subject: [PATCH 13/13] remove unused import --- src/app/core/data/request.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index c2e7327d22..810b0721ae 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -1,4 +1,4 @@ -import { Injectable, NgZone } from '@angular/core'; +import { Injectable } from '@angular/core'; import { HttpHeaders } from '@angular/common/http'; import { createSelector, MemoizedSelector, select, Store } from '@ngrx/store';