diff --git a/package.json b/package.json index 8e2d86e9ed..6aead3664a 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "prebuild:prod": "yarn run prebuild", "build": "webpack --progress --mode development", "build:aot": "webpack --env.aot --env.server --mode development && webpack --env.aot --env.client --mode development", - "build:prod": "webpack --env.aot --env.server --env.production && webpack --env.aot --env.client --env.production", + "build:prod": "webpack --env.aot --env.server --mode production && webpack --env.aot --env.client --mode production", "postbuild:prod": "yarn run rollup", "rollup": "rollup -c rollup.config.js", "prestart": "yarn run build:prod", diff --git a/resources/i18n/en.json b/resources/i18n/en.json index e58c7cadff..814adba9a7 100644 --- a/resources/i18n/en.json +++ b/resources/i18n/en.json @@ -126,7 +126,8 @@ }, "results": { "head": "Search Results", - "no-results": "There were no results for this search" + "no-results": "Your search returned no results. Having trouble finding what you're looking for? Try putting", + "no-results-link": "quotes around it" }, "sidebar": { "close": "Back to results", diff --git a/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.html b/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.html index 084232edf4..bbe6d8d95b 100644 --- a/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.html +++ b/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.html @@ -1,7 +1,5 @@ -
- -
{{ label }}
- +
+
{{ label }}
diff --git a/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.spec.ts b/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.spec.ts index 47e7d6c34e..cce54edf64 100644 --- a/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.spec.ts +++ b/src/app/+item-page/field-components/metadata-field-wrapper/metadata-field-wrapper.component.spec.ts @@ -7,7 +7,8 @@ import { MetadataFieldWrapperComponent } from './metadata-field-wrapper.componen @Component({ selector: 'ds-component-with-content', template: '\n' + - '
\n' + + '
\n' + + ' \n' + '
\n' + '' }) @@ -30,25 +31,37 @@ describe('MetadataFieldWrapperComponent', () => { const wrapperSelector = '.simple-view-element'; const labelSelector = '.simple-view-element-header'; + const contentSelector = '.my-content'; it('should create', () => { expect(component).toBeDefined(); }); - it('should not show a label when there is no content', () => { + it('should not show the component when there is no content', () => { component.label = 'test label'; fixture.detectChanges(); - const debugLabel = fixture.debugElement.query(By.css(labelSelector)); - expect(debugLabel).toBeNull(); + const parentNative = fixture.nativeElement; + const nativeWrapper = parentNative.querySelector(wrapperSelector); + expect(nativeWrapper.classList.contains('d-none')).toBe(true); }); - it('should show a label when there is content', () => { + it('should not show the component when there is DOM content but no text', () => { const parentFixture = TestBed.createComponent(ContentComponent); parentFixture.detectChanges(); - const parentComponent = parentFixture.componentInstance; const parentNative = parentFixture.nativeElement; - const nativeLabel = parentNative.querySelector(labelSelector); - expect(nativeLabel.textContent).toContain('test label'); + const nativeWrapper = parentNative.querySelector(wrapperSelector); + expect(nativeWrapper.classList.contains('d-none')).toBe(true); + }); + + it('should show the component when there is text content', () => { + const parentFixture = TestBed.createComponent(ContentComponent); + parentFixture.detectChanges(); + const parentNative = parentFixture.nativeElement; + const nativeContent = parentNative.querySelector(contentSelector); + nativeContent.textContent = 'lorem ipsum'; + const nativeWrapper = parentNative.querySelector(wrapperSelector); + parentFixture.detectChanges(); + expect(nativeWrapper.classList.contains('d-none')).toBe(false); }); }); diff --git a/src/app/+search-page/search-results/search-results.component.html b/src/app/+search-page/search-results/search-results.component.html index ed6fc18d9c..4915b552c3 100644 --- a/src/app/+search-page/search-results/search-results.component.html +++ b/src/app/+search-page/search-results/search-results.component.html @@ -7,5 +7,12 @@ [hideGear]="true">
- - + +
+ {{ 'search.results.no-results' | translate }} + + {{"search.results.no-results-link" | translate}} + +
diff --git a/src/app/+search-page/search-results/search-results.component.spec.ts b/src/app/+search-page/search-results/search-results.component.spec.ts index 4f299c5c50..54463d916d 100644 --- a/src/app/+search-page/search-results/search-results.component.spec.ts +++ b/src/app/+search-page/search-results/search-results.component.spec.ts @@ -1,40 +1,92 @@ import { ComponentFixture, TestBed, async, tick, fakeAsync } from '@angular/core/testing'; import { By } from '@angular/platform-browser'; -import { DebugElement, NO_ERRORS_SCHEMA } from '@angular/core'; +import { NoopAnimationsModule } from '@angular/platform-browser/animations'; +import { NO_ERRORS_SCHEMA } from '@angular/core'; import { ResourceType } from '../../core/shared/resource-type'; import { Community } from '../../core/shared/community.model'; import { TranslateModule } from '@ngx-translate/core'; import { SearchResultsComponent } from './search-results.component'; +import { QueryParamsDirectiveStub } from '../../shared/testing/query-params-directive-stub'; describe('SearchResultsComponent', () => { let comp: SearchResultsComponent; let fixture: ComponentFixture; - let heading: DebugElement; beforeEach(async(() => { TestBed.configureTestingModule({ - imports: [TranslateModule.forRoot()], - declarations: [SearchResultsComponent], + imports: [TranslateModule.forRoot(), NoopAnimationsModule], + declarations: [ + SearchResultsComponent, + QueryParamsDirectiveStub], schemas: [NO_ERRORS_SCHEMA] }).compileComponents(); })); beforeEach(() => { fixture = TestBed.createComponent(SearchResultsComponent); - comp = fixture.componentInstance; // SearchFormComponent test instance - heading = fixture.debugElement.query(By.css('heading')); + comp = fixture.componentInstance; // SearchResultsComponent test instance }); - it('should display heading when results are not empty', fakeAsync(() => { - (comp as any).searchResults = 'test'; - (comp as any).searchConfig = {pagination: ''}; + it('should display results when results are not empty', () => { + (comp as any).searchResults = { hasSucceeded: true, isLoading: false, payload: { page: { length: 2 } } }; + (comp as any).searchConfig = {}; fixture.detectChanges(); - tick(); - expect(heading).toBeDefined(); - })); + expect(fixture.debugElement.query(By.css('ds-viewable-collection'))).not.toBeNull(); + }); - it('should not display heading when results is empty', () => { - expect(heading).toBeNull(); + it('should not display link when results are not empty', () => { + (comp as any).searchResults = { hasSucceeded: true, isLoading: false, payload: { page: { length: 2 } } }; + (comp as any).searchConfig = {}; + fixture.detectChanges(); + expect(fixture.debugElement.query(By.css('a'))).toBeNull(); + }); + + it('should display error message if error is != 400', () => { + (comp as any).searchResults = { hasFailed: true, error: { statusCode: 500 } }; + fixture.detectChanges(); + expect(fixture.debugElement.query(By.css('ds-error'))).not.toBeNull(); + }); + + it('should display link with new search where query is quoted if search return a error 400', () => { + (comp as any).searchResults = { hasFailed: true, error: { statusCode: 400 } }; + (comp as any).searchConfig = { query: 'foobar' }; + fixture.detectChanges(); + + const linkDes = fixture.debugElement.queryAll(By.directive(QueryParamsDirectiveStub)); + + // get attached link directive instances + // using each DebugElement's injector + const routerLinkQuery = linkDes.map((de) => de.injector.get(QueryParamsDirectiveStub)); + + expect(routerLinkQuery.length).toBe(1, 'should have 1 router link with query params'); + expect(routerLinkQuery[0].queryParams.query).toBe('"foobar"', 'query params should be "foobar"'); + }); + + it('should display link with new search where query is quoted if search result is empty', () => { + (comp as any).searchResults = { payload: { page: { length: 0 } } }; + (comp as any).searchConfig = { query: 'foobar' }; + fixture.detectChanges(); + + const linkDes = fixture.debugElement.queryAll(By.directive(QueryParamsDirectiveStub)); + + // get attached link directive instances + // using each DebugElement's injector + const routerLinkQuery = linkDes.map((de) => de.injector.get(QueryParamsDirectiveStub)); + + expect(routerLinkQuery.length).toBe(1, 'should have 1 router link with query params'); + expect(routerLinkQuery[0].queryParams.query).toBe('"foobar"', 'query params should be "foobar"'); + }); + + it('should add quotes around the given string', () => { + expect(comp.surroundStringWithQuotes('teststring')).toEqual('"teststring"'); + }); + + it('should not add quotes around the given string if they are already there', () => { + expect(comp.surroundStringWithQuotes('"teststring"')).toEqual('"teststring"'); + }); + + it('should not add quotes around a given empty string', () => { + expect(comp.surroundStringWithQuotes('')).toEqual(''); }); }); diff --git a/src/app/+search-page/search-results/search-results.component.ts b/src/app/+search-page/search-results/search-results.component.ts index 6399243f92..ae0abfcd27 100644 --- a/src/app/+search-page/search-results/search-results.component.ts +++ b/src/app/+search-page/search-results/search-results.component.ts @@ -6,6 +6,7 @@ import { SearchOptions } from '../search-options.model'; import { SearchResult } from '../search-result.model'; import { PaginatedList } from '../../core/data/paginated-list'; import { ViewMode } from '../../core/shared/view-mode.model'; +import { isNotEmpty } from '../../shared/empty.util'; @Component({ selector: 'ds-search-results', @@ -35,4 +36,16 @@ export class SearchResultsComponent { */ @Input() viewMode: ViewMode; + /** + * Method to change the given string by surrounding it by quotes if not already present. + */ + surroundStringWithQuotes(input: string): string { + let result = input; + + if (isNotEmpty(result) && !(result.startsWith('\"') && result.endsWith('\"'))) { + result = `"${result}"`; + } + + return result; + } } diff --git a/src/app/core/auth/auth-response-parsing.service.ts b/src/app/core/auth/auth-response-parsing.service.ts index 65d093de61..61559991ec 100644 --- a/src/app/core/auth/auth-response-parsing.service.ts +++ b/src/app/core/auth/auth-response-parsing.service.ts @@ -27,7 +27,7 @@ export class AuthResponseParsingService extends BaseResponseParsingService imple parse(request: RestRequest, data: DSpaceRESTV2Response): RestResponse { if (isNotEmpty(data.payload) && isNotEmpty(data.payload._links) && (data.statusCode === '200' || data.statusCode === 'OK')) { - const response = this.process(data.payload, request.href); + const response = this.process(data.payload, request.uuid); return new AuthStatusResponse(response, data.statusCode); } else { return new AuthStatusResponse(data.payload as AuthStatus, data.statusCode); diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 187db93f3c..d39c0a4590 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -148,7 +148,7 @@ describe('AuthService test', () => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, authReqService, router, cookieService, store, rdbService); + authService = new AuthService({}, window, undefined, authReqService, router, cookieService, store, rdbService); })); it('should return true when user is logged in', () => { @@ -207,7 +207,7 @@ describe('AuthService test', () => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, authReqService, router, cookieService, store, rdbService); + authService = new AuthService({}, window, undefined, authReqService, router, cookieService, store, rdbService); storage = (authService as any).storage; spyOn(storage, 'get'); spyOn(storage, 'remove'); diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 229c44bcfa..67f533d4ad 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -9,10 +9,10 @@ import { take, withLatestFrom } from 'rxjs/operators'; -import { Inject, Injectable } from '@angular/core'; +import { Inject, Injectable, Optional } from '@angular/core'; import { PRIMARY_OUTLET, Router, UrlSegmentGroup, UrlTree } from '@angular/router'; import { HttpHeaders } from '@angular/common/http'; -import { REQUEST } from '@nguniversal/express-engine/tokens'; +import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens'; import { RouterReducerState } from '@ngrx/router-store'; import { select, Store } from '@ngrx/store'; @@ -59,6 +59,7 @@ export class AuthService { constructor(@Inject(REQUEST) protected req: any, @Inject(NativeWindowService) protected _window: NativeWindowRef, protected authRequestService: AuthRequestService, + @Optional() @Inject(RESPONSE) private response: any, protected router: Router, protected storage: CookieService, protected store: Store, @@ -345,6 +346,10 @@ export class AuthService { if (this._window.nativeWindow.location) { // Hard redirect to login page, so that all state is definitely lost this._window.nativeWindow.location.href = redirectUrl; + } else if (this.response) { + if (!this.response._headerSent) { + this.response.redirect(302, redirectUrl); + } } else { this.router.navigateByUrl(redirectUrl); } @@ -357,14 +362,10 @@ export class AuthService { this.getRedirectUrl().pipe( first()) .subscribe((redirectUrl) => { + if (isNotEmpty(redirectUrl)) { this.clearRedirectUrl(); - - // override the route reuse strategy - this.router.routeReuseStrategy.shouldReuseRoute = () => { - return false; - }; - this.router.navigated = false; + this.router.onSameUrlNavigation = 'reload'; const url = decodeURIComponent(redirectUrl); this.router.navigateByUrl(url); /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index 9ab2d84c20..c83410f6e3 100644 --- a/src/app/core/auth/server-auth.service.ts +++ b/src/app/core/auth/server-auth.service.ts @@ -41,7 +41,6 @@ export class ServerAuthService extends AuthService { // TODO this should be cleaned up, AuthStatus could be parsed by the RemoteDataService as a whole... const person$ = this.rdbService.buildSingle(status.eperson.toString()); - // person$.subscribe(() => console.log('test')); return person$.pipe( map((eperson) => eperson.payload) ); diff --git a/src/app/core/browse/browse.service.ts b/src/app/core/browse/browse.service.ts index ca56c0d267..b807a77e99 100644 --- a/src/app/core/browse/browse.service.ts +++ b/src/app/core/browse/browse.service.ts @@ -28,8 +28,7 @@ import { configureRequest, filterSuccessfulResponses, getBrowseDefinitionLinks, - getRemoteDataPayload, - getRequestFromSelflink + getRemoteDataPayload, getRequestFromRequestHref } from '../shared/operators'; import { URLCombiner } from '../url-combiner/url-combiner'; import { Item } from '../shared/item.model'; @@ -68,7 +67,7 @@ export class BrowseService { ); const href$ = request$.pipe(map((request: RestRequest) => request.href)); - const requestEntry$ = href$.pipe(getRequestFromSelflink(this.requestService)); + const requestEntry$ = href$.pipe(getRequestFromRequestHref(this.requestService)); const payload$ = requestEntry$.pipe( filterSuccessfulResponses(), map((response: GenericSuccessResponse) => response.payload), @@ -111,7 +110,7 @@ export class BrowseService { const href$ = request$.pipe(map((request: RestRequest) => request.href)); - const requestEntry$ = href$.pipe(getRequestFromSelflink(this.requestService)); + const requestEntry$ = href$.pipe(getRequestFromRequestHref(this.requestService)); const payload$ = requestEntry$.pipe( filterSuccessfulResponses(), @@ -166,7 +165,7 @@ export class BrowseService { const href$ = request$.pipe(map((request: RestRequest) => request.href)); - const requestEntry$ = href$.pipe(getRequestFromSelflink(this.requestService)); + const requestEntry$ = href$.pipe(getRequestFromRequestHref(this.requestService)); const payload$ = requestEntry$.pipe( filterSuccessfulResponses(), diff --git a/src/app/core/cache/builders/remote-data-build.service.spec.ts b/src/app/core/cache/builders/remote-data-build.service.spec.ts new file mode 100644 index 0000000000..e4444ca803 --- /dev/null +++ b/src/app/core/cache/builders/remote-data-build.service.spec.ts @@ -0,0 +1,59 @@ +import { RemoteDataBuildService } from './remote-data-build.service'; +import { Item } from '../../shared/item.model'; +import { PaginatedList } from '../../data/paginated-list'; +import { PageInfo } from '../../shared/page-info.model'; +import { RemoteData } from '../../data/remote-data'; +import { of as observableOf } from 'rxjs'; + +const pageInfo = new PageInfo(); +const array = [ + Object.assign(new Item(), { + metadata: [ + { + key: 'dc.title', + language: 'en_US', + value: 'Item nr 1' + }] + }), + Object.assign(new Item(), { + metadata: [ + { + key: 'dc.title', + language: 'en_US', + value: 'Item nr 2' + }] + }) +]; +const paginatedList = new PaginatedList(pageInfo, array); +const arrayRD = new RemoteData(false, false, true, undefined, array); +const paginatedListRD = new RemoteData(false, false, true, undefined, paginatedList); + +describe('RemoteDataBuildService', () => { + let service: RemoteDataBuildService; + + beforeEach(() => { + service = new RemoteDataBuildService(undefined, undefined); + }); + + describe('when toPaginatedList is called', () => { + let expected: RemoteData>; + + beforeEach(() => { + expected = paginatedListRD; + }); + + it('should return the correct remoteData of a paginatedList when the input is a (remoteData of an) array', () => { + const result = (service as any).toPaginatedList(observableOf(arrayRD), pageInfo); + result.subscribe((resultRD) => { + expect(resultRD).toEqual(expected); + }); + }); + + it('should return the correct remoteData of a paginatedList when the input is a (remoteData of a) paginated list', () => { + const result = (service as any).toPaginatedList(observableOf(paginatedListRD), pageInfo); + result.subscribe((resultRD) => { + expect(resultRD).toEqual(expected); + }); + }); + }); +}); diff --git a/src/app/core/cache/builders/remote-data-build.service.ts b/src/app/core/cache/builders/remote-data-build.service.ts index 3877c19ff9..52ec4382ae 100644 --- a/src/app/core/cache/builders/remote-data-build.service.ts +++ b/src/app/core/cache/builders/remote-data-build.service.ts @@ -5,15 +5,7 @@ import { race as observableRace } from 'rxjs'; import { Injectable } from '@angular/core'; -import { - distinctUntilChanged, - first, - flatMap, - map, - startWith, - switchMap, - takeUntil, tap -} from 'rxjs/operators'; +import { distinctUntilChanged, first, flatMap, map, startWith, switchMap } from 'rxjs/operators'; import { hasValue, hasValueOperator, isEmpty, isNotEmpty } from '../../../shared/empty.util'; import { PaginatedList } from '../../data/paginated-list'; import { RemoteData } from '../../data/remote-data'; @@ -29,7 +21,7 @@ import { getMapsTo, getRelationMetadata, getRelationships } from './build-decora import { PageInfo } from '../../shared/page-info.model'; import { filterSuccessfulResponses, - getRequestFromSelflink, + getRequestFromRequestHref, getRequestFromRequestUUID, getResourceLinksFromResponse } from '../../shared/operators'; @@ -43,16 +35,16 @@ export class RemoteDataBuildService { if (typeof href$ === 'string') { href$ = observableOf(href$); } - const requestHref$ = href$.pipe( + const requestUUID$ = href$.pipe( switchMap((href: string) => - this.objectCache.getRequestHrefBySelfLink(href)), + this.objectCache.getRequestUUIDBySelfLink(href)), ); const requestEntry$ = observableRace( - href$.pipe(getRequestFromSelflink(this.requestService)), - requestHref$.pipe(getRequestFromSelflink(this.requestService)), + href$.pipe(getRequestFromRequestHref(this.requestService)), + requestUUID$.pipe(getRequestFromRequestUUID(this.requestService)), ).pipe( - first() + first() ); // always use self link if that is cached, only if it isn't, get it via the response. @@ -121,7 +113,7 @@ export class RemoteDataBuildService { href$ = observableOf(href$); } - const requestEntry$ = href$.pipe(getRequestFromSelflink(this.requestService)); + const requestEntry$ = href$.pipe(getRequestFromRequestHref(this.requestService)); const tDomainList$ = requestEntry$.pipe( getResourceLinksFromResponse(), flatMap((resourceUUIDs: string[]) => { @@ -196,7 +188,7 @@ export class RemoteDataBuildService { } if (hasValue(normalized[relationship].page)) { - links[relationship] = this.aggregatePaginatedList(result, normalized[relationship].pageInfo); + links[relationship] = this.toPaginatedList(result, normalized[relationship].pageInfo); } else { links[relationship] = result; } @@ -258,8 +250,16 @@ export class RemoteDataBuildService { })) } - aggregatePaginatedList(input: Observable>, pageInfo: PageInfo): Observable>> { - return input.pipe(map((rd) => Object.assign(rd, { payload: new PaginatedList(pageInfo, rd.payload) }))); + private toPaginatedList(input: Observable>>, pageInfo: PageInfo): Observable>> { + return input.pipe( + map((rd: RemoteData>) => { + if (Array.isArray(rd.payload)) { + return Object.assign(rd, { payload: new PaginatedList(pageInfo, rd.payload) }) + } else { + return Object.assign(rd, { payload: new PaginatedList(pageInfo, rd.payload.page) }); + } + }) + ); } } diff --git a/src/app/core/cache/object-cache.actions.ts b/src/app/core/cache/object-cache.actions.ts index 024a0e7061..8531677ffc 100644 --- a/src/app/core/cache/object-cache.actions.ts +++ b/src/app/core/cache/object-cache.actions.ts @@ -25,7 +25,7 @@ export class AddToObjectCacheAction implements Action { objectToCache: CacheableObject; timeAdded: number; msToLive: number; - requestHref: string; + requestUUID: string; }; /** @@ -42,8 +42,8 @@ export class AddToObjectCacheAction implements Action { * This isn't necessarily the same as the object's self * link, it could have been part of a list for example */ - constructor(objectToCache: CacheableObject, timeAdded: number, msToLive: number, requestHref: string) { - this.payload = { objectToCache, timeAdded, msToLive, requestHref }; + constructor(objectToCache: CacheableObject, timeAdded: number, msToLive: number, requestUUID: string) { + this.payload = { objectToCache, timeAdded, msToLive, requestUUID }; } } diff --git a/src/app/core/cache/object-cache.reducer.spec.ts b/src/app/core/cache/object-cache.reducer.spec.ts index 311f11c2ad..efa28d7249 100644 --- a/src/app/core/cache/object-cache.reducer.spec.ts +++ b/src/app/core/cache/object-cache.reducer.spec.ts @@ -8,6 +8,7 @@ import { RemoveFromObjectCacheAction, ResetObjectCacheTimestampsAction } from './object-cache.actions'; +import { Operation } from 'fast-json-patch'; class NullAction extends RemoveFromObjectCacheAction { type = null; @@ -19,8 +20,11 @@ class NullAction extends RemoveFromObjectCacheAction { } describe('objectCacheReducer', () => { + const requestUUID1 = '8646169a-a8fc-4b31-a368-384c07867eb1'; + const requestUUID2 = 'bd36820b-4bf7-4d58-bd80-b832058b7279'; const selfLink1 = 'https://localhost:8080/api/core/items/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; const selfLink2 = 'https://localhost:8080/api/core/items/28b04544-1766-4e82-9728-c4e93544ecd3'; + const newName = 'new different name'; const testState = { [selfLink1]: { data: { @@ -29,18 +33,18 @@ describe('objectCacheReducer', () => { }, timeAdded: new Date().getTime(), msToLive: 900000, - requestHref: selfLink1, + requestUUID: requestUUID1, patches: [], isDirty: false }, [selfLink2]: { data: { - self: selfLink2, + self: requestUUID2, foo: 'baz' }, timeAdded: new Date().getTime(), msToLive: 900000, - requestHref: selfLink2, + requestUUID: selfLink2, patches: [], isDirty: false } @@ -66,8 +70,8 @@ describe('objectCacheReducer', () => { const objectToCache = { self: selfLink1 }; const timeAdded = new Date().getTime(); const msToLive = 900000; - const requestHref = 'https://rest.api/endpoint/selfLink1'; - const action = new AddToObjectCacheAction(objectToCache, timeAdded, msToLive, requestHref); + const requestUUID = requestUUID1; + const action = new AddToObjectCacheAction(objectToCache, timeAdded, msToLive, requestUUID); const newState = objectCacheReducer(state, action); expect(newState[selfLink1].data).toEqual(objectToCache); @@ -79,8 +83,8 @@ describe('objectCacheReducer', () => { const objectToCache = { self: selfLink1, foo: 'baz', somethingElse: true }; const timeAdded = new Date().getTime(); const msToLive = 900000; - const requestHref = 'https://rest.api/endpoint/selfLink1'; - const action = new AddToObjectCacheAction(objectToCache, timeAdded, msToLive, requestHref); + const requestUUID = requestUUID1; + const action = new AddToObjectCacheAction(objectToCache, timeAdded, msToLive, requestUUID); const newState = objectCacheReducer(testState, action); /* tslint:disable:no-string-literal */ @@ -94,8 +98,8 @@ describe('objectCacheReducer', () => { const objectToCache = { self: selfLink1 }; const timeAdded = new Date().getTime(); const msToLive = 900000; - const requestHref = 'https://rest.api/endpoint/selfLink1'; - const action = new AddToObjectCacheAction(objectToCache, timeAdded, msToLive, requestHref); + const requestUUID = requestUUID1; + const action = new AddToObjectCacheAction(objectToCache, timeAdded, msToLive, requestUUID); deepFreeze(state); objectCacheReducer(state, action); @@ -140,15 +144,31 @@ describe('objectCacheReducer', () => { }); it('should perform the ADD_PATCH action without affecting the previous state', () => { - const action = new AddPatchObjectCacheAction(selfLink1, [{ op: 'replace', path: '/name', value: 'random string' }]); + const action = new AddPatchObjectCacheAction(selfLink1, [{ + op: 'replace', + path: '/name', + value: 'random string' + }]); // testState has already been frozen above objectCacheReducer(testState, action); }); - it('should perform the APPLY_PATCH action without affecting the previous state', () => { + it('should when the ADD_PATCH action dispatched', () => { + const patch = [{ op: 'add', path: '/name', value: newName } as Operation]; + const action = new AddPatchObjectCacheAction(selfLink1, patch); + const newState = objectCacheReducer(testState, action); + expect(newState[selfLink1].patches.map((p) => p.operations)).toContain(patch); + }); + + it('should when the APPLY_PATCH action dispatched', () => { + const patch = [{ op: 'add', path: '/name', value: newName } as Operation]; + const addPatchAction = new AddPatchObjectCacheAction(selfLink1, patch); + const stateWithPatch = objectCacheReducer(testState, addPatchAction); + const action = new ApplyPatchObjectCacheAction(selfLink1); - // testState has already been frozen above - objectCacheReducer(testState, action); + const newState = objectCacheReducer(stateWithPatch, action); + expect(newState[selfLink1].patches).toEqual([]); + expect((newState[selfLink1].data as any).name).toEqual(newName); }); }); diff --git a/src/app/core/cache/object-cache.reducer.ts b/src/app/core/cache/object-cache.reducer.ts index 4424bb2142..867f31e1bb 100644 --- a/src/app/core/cache/object-cache.reducer.ts +++ b/src/app/core/cache/object-cache.reducer.ts @@ -45,7 +45,7 @@ export class ObjectCacheEntry implements CacheEntry { data: CacheableObject; timeAdded: number; msToLive: number; - requestHref: string; + requestUUID: string; patches: Patch[] = []; isDirty: boolean; } @@ -119,7 +119,7 @@ function addToObjectCache(state: ObjectCacheState, action: AddToObjectCacheActio data: action.payload.objectToCache, timeAdded: action.payload.timeAdded, msToLive: action.payload.msToLive, - requestHref: action.payload.requestHref, + requestUUID: action.payload.requestUUID, isDirty: (hasValue(existing) ? isNotEmpty(existing.patches) : false), patches: (hasValue(existing) ? existing.patches : []) } diff --git a/src/app/core/cache/object-cache.service.spec.ts b/src/app/core/cache/object-cache.service.spec.ts index 5a1a3c5ac6..af353a38c1 100644 --- a/src/app/core/cache/object-cache.service.spec.ts +++ b/src/app/core/cache/object-cache.service.spec.ts @@ -22,6 +22,7 @@ describe('ObjectCacheService', () => { let store: Store; const selfLink = 'https://rest.api/endpoint/1698f1d3-be98-4c51-9fd8-6bfedcbd59b7'; + const requestUUID = '4d3a4ce8-a375-4b98-859b-39f0a014d736'; const timestamp = new Date().getTime(); const msToLive = 900000; let objectToCache = { @@ -58,8 +59,8 @@ describe('ObjectCacheService', () => { describe('add', () => { it('should dispatch an ADD action with the object to add, the time to live, and the current timestamp', () => { - service.add(objectToCache, msToLive, selfLink); - expect(store.dispatch).toHaveBeenCalledWith(new AddToObjectCacheAction(objectToCache, timestamp, msToLive, selfLink)); + service.add(objectToCache, msToLive, requestUUID); + expect(store.dispatch).toHaveBeenCalledWith(new AddToObjectCacheAction(objectToCache, timestamp, msToLive, requestUUID)); }); }); diff --git a/src/app/core/cache/object-cache.service.ts b/src/app/core/cache/object-cache.service.ts index 3ac644a045..44297d6f61 100644 --- a/src/app/core/cache/object-cache.service.ts +++ b/src/app/core/cache/object-cache.service.ts @@ -45,13 +45,11 @@ export class ObjectCacheService { * The object to add * @param msToLive * The number of milliseconds it should be cached for - * @param requestHref - * The selfLink of the request that resulted in this object - * This isn't necessarily the same as the object's self - * link, it could have been part of a list for example + * @param requestUUID + * The UUID of the request that resulted in this object */ - add(objectToCache: CacheableObject, msToLive: number, requestHref: string): void { - this.store.dispatch(new AddToObjectCacheAction(objectToCache, new Date().getTime(), msToLive, requestHref)); + add(objectToCache: CacheableObject, msToLive: number, requestUUID: string): void { + this.store.dispatch(new AddToObjectCacheAction(objectToCache, new Date().getTime(), msToLive, requestUUID)); } /** @@ -115,16 +113,16 @@ export class ObjectCacheService { ); } - getRequestHrefBySelfLink(selfLink: string): Observable { + getRequestUUIDBySelfLink(selfLink: string): Observable { return this.getEntry(selfLink).pipe( - map((entry: ObjectCacheEntry) => entry.requestHref), + map((entry: ObjectCacheEntry) => entry.requestUUID), distinctUntilChanged()); } - getRequestHrefByUUID(uuid: string): Observable { + getRequestUUIDByObjectUUID(uuid: string): Observable { return this.store.pipe( select(selfLinkFromUuidSelector(uuid)), - mergeMap((selfLink: string) => this.getRequestHrefBySelfLink(selfLink)) + mergeMap((selfLink: string) => this.getRequestUUIDBySelfLink(selfLink)) ); } diff --git a/src/app/core/cache/response.models.ts b/src/app/core/cache/response.models.ts index 90eb621ba4..66ed2ee7f1 100644 --- a/src/app/core/cache/response.models.ts +++ b/src/app/core/cache/response.models.ts @@ -141,7 +141,7 @@ export class ErrorResponse extends RestResponse { constructor(error: RequestError) { super(false, error.statusText); - // console.error(error); + console.error(error); this.errorMessage = error.message; } } diff --git a/src/app/core/cache/server-sync-buffer.actions.ts b/src/app/core/cache/server-sync-buffer.actions.ts index 638d837bea..fd7e04ef8a 100644 --- a/src/app/core/cache/server-sync-buffer.actions.ts +++ b/src/app/core/cache/server-sync-buffer.actions.ts @@ -15,7 +15,7 @@ export const ServerSyncBufferActionTypes = { /* tslint:disable:max-classes-per-file */ /** - * An ngrx action to add a new cached object to the server's sync buffer + * An ngrx action to add a new cached object to the server sync buffer */ export class AddToSSBAction implements Action { type = ServerSyncBufferActionTypes.ADD; diff --git a/src/app/core/cache/server-sync-buffer.effects.ts b/src/app/core/cache/server-sync-buffer.effects.ts index db2263c52a..5a0a5527d1 100644 --- a/src/app/core/cache/server-sync-buffer.effects.ts +++ b/src/app/core/cache/server-sync-buffer.effects.ts @@ -1,4 +1,4 @@ -import { delay, exhaustMap, first, map, switchMap } from 'rxjs/operators'; +import { delay, exhaustMap, first, map, switchMap, tap } from 'rxjs/operators'; import { Inject, Injectable } from '@angular/core'; import { Actions, Effect, ofType } from '@ngrx/effects'; import { @@ -38,7 +38,9 @@ export class ServerSyncBufferEffects { exhaustMap((action: AddToSSBAction) => { const autoSyncConfig = this.EnvConfig.cache.autoSync; const timeoutInSeconds = autoSyncConfig.timePerMethod[action.payload.method] || autoSyncConfig.defaultTime; - return observableOf(new CommitSSBAction(action.payload.method)).pipe(delay(timeoutInSeconds * 1000)) + return observableOf(new CommitSSBAction(action.payload.method)).pipe( + delay(timeoutInSeconds * 1000), + ) }) ); @@ -54,6 +56,7 @@ export class ServerSyncBufferEffects { switchMap((action: CommitSSBAction) => { return this.store.pipe( select(serverSyncBufferSelector()), + first(), /* necessary, otherwise delay will not have any effect after the first run */ switchMap((bufferState: ServerSyncBufferState) => { const actions: Array> = bufferState.buffer .filter((entry: ServerSyncBufferEntry) => { diff --git a/src/app/core/cache/server-sync-buffer.reducer.spec.ts b/src/app/core/cache/server-sync-buffer.reducer.spec.ts index 666144104b..8f1392c99d 100644 --- a/src/app/core/cache/server-sync-buffer.reducer.spec.ts +++ b/src/app/core/cache/server-sync-buffer.reducer.spec.ts @@ -1,13 +1,5 @@ import * as deepFreeze from 'deep-freeze'; - -import { objectCacheReducer } from './object-cache.reducer'; -import { - AddPatchObjectCacheAction, - AddToObjectCacheAction, ApplyPatchObjectCacheAction, - RemoveFromObjectCacheAction, - ResetObjectCacheTimestampsAction -} from './object-cache.actions'; -import { Operation } from '../../../../node_modules/fast-json-patch'; +import { RemoveFromObjectCacheAction } from './object-cache.actions'; import { serverSyncBufferReducer } from './server-sync-buffer.reducer'; import { RestRequestMethod } from '../data/rest-request-method'; import { AddToSSBAction, EmptySSBAction } from './server-sync-buffer.actions'; diff --git a/src/app/core/data/base-response-parsing.service.ts b/src/app/core/data/base-response-parsing.service.ts index 6075d605fd..eada156ce9 100644 --- a/src/app/core/data/base-response-parsing.service.ts +++ b/src/app/core/data/base-response-parsing.service.ts @@ -6,7 +6,6 @@ import { ObjectCacheService } from '../cache/object-cache.service'; import { GlobalConfig } from '../../../config/global-config.interface'; import { GenericConstructor } from '../shared/generic-constructor'; import { PaginatedList } from './paginated-list'; -import { NormalizedObject } from '../cache/models/normalized-object.model'; import { ResourceType } from '../shared/resource-type'; import { RESTURLCombiner } from '../url-combiner/rest-url-combiner'; @@ -15,7 +14,7 @@ function isObjectLevel(halObj: any) { } function isPaginatedResponse(halObj: any) { - return isNotEmpty(halObj.page) && hasValue(halObj._embedded); + return hasValue(halObj.page) && hasValue(halObj._embedded); } /* tslint:disable:max-classes-per-file */ @@ -26,14 +25,14 @@ export abstract class BaseResponseParsingService { protected abstract objectFactory: any; protected abstract toCache: boolean; - protected process(data: any, requestHref: string): any { + protected process(data: any, requestUUID: string): any { if (isNotEmpty(data)) { if (hasNoValue(data) || (typeof data !== 'object')) { return data; } else if (isPaginatedResponse(data)) { - return this.processPaginatedList(data, requestHref); + return this.processPaginatedList(data, requestUUID); } else if (Array.isArray(data)) { - return this.processArray(data, requestHref); + return this.processArray(data, requestUUID); } else if (isObjectLevel(data)) { data = this.fixBadEPersonRestResponse(data); const object = this.deserialize(data); @@ -42,7 +41,7 @@ export abstract class BaseResponseParsingService { .keys(data._embedded) .filter((property) => data._embedded.hasOwnProperty(property)) .forEach((property) => { - const parsedObj = this.process(data._embedded[property], requestHref); + const parsedObj = this.process(data._embedded[property], requestUUID); if (isNotEmpty(parsedObj)) { if (isPaginatedResponse(data._embedded[property])) { object[property] = parsedObj; @@ -56,7 +55,7 @@ export abstract class BaseResponseParsingService { }); } - this.cache(object, requestHref); + this.cache(object, requestUUID); return object; } const result = {}; @@ -64,7 +63,7 @@ export abstract class BaseResponseParsingService { .filter((property) => data.hasOwnProperty(property)) .filter((property) => hasValue(data[property])) .forEach((property) => { - const obj = this.process(data[property], requestHref); + const obj = this.process(data[property], requestUUID); result[property] = obj; }); return result; @@ -72,7 +71,7 @@ export abstract class BaseResponseParsingService { } } - protected processPaginatedList(data: any, requestHref: string): PaginatedList { + protected processPaginatedList(data: any, requestUUID: string): PaginatedList { const pageInfo: PageInfo = this.processPageInfo(data); let list = data._embedded; @@ -80,14 +79,14 @@ export abstract class BaseResponseParsingService { if (!Array.isArray(list)) { list = this.flattenSingleKeyObject(list); } - const page: ObjectDomain[] = this.processArray(list, requestHref); + const page: ObjectDomain[] = this.processArray(list, requestUUID); return new PaginatedList(pageInfo, page); } - protected processArray(data: any, requestHref: string): ObjectDomain[] { + protected processArray(data: any, requestUUID: string): ObjectDomain[] { let array: ObjectDomain[] = []; data.forEach((datum) => { - array = [...array, this.process(datum, requestHref)]; + array = [...array, this.process(datum, requestUUID)]; } ); return array; @@ -115,21 +114,21 @@ export abstract class BaseResponseParsingService { } } - protected cache(obj, requestHref) { + protected cache(obj, requestUUID) { if (this.toCache) { - this.addToObjectCache(obj, requestHref); + this.addToObjectCache(obj, requestUUID); } } - protected addToObjectCache(co: CacheableObject, requestHref: string): void { + protected addToObjectCache(co: CacheableObject, requestUUID: string): void { if (hasNoValue(co) || hasNoValue(co.self)) { throw new Error('The server returned an invalid object'); } - this.objectCache.add(co, this.EnvConfig.cache.msToLive.default, requestHref); + this.objectCache.add(co, this.EnvConfig.cache.msToLive.default, requestUUID); } processPageInfo(payload: any): PageInfo { - if (isNotEmpty(payload.page)) { + if (hasValue(payload.page)) { const pageObj = Object.assign({}, payload.page, { _links: payload._links }); const pageInfoObject = new DSpaceRESTv2Serializer(PageInfo).deserialize(pageObj); if (pageInfoObject.currentPage >= 0) { diff --git a/src/app/core/data/comcol-data.service.ts b/src/app/core/data/comcol-data.service.ts index 63c11dd8cb..d3eed88ffd 100644 --- a/src/app/core/data/comcol-data.service.ts +++ b/src/app/core/data/comcol-data.service.ts @@ -49,23 +49,6 @@ export abstract class ComColDataService this.responseCache.get(href)), - // map((entry: ResponseCacheEntry) => entry.response), - // mergeMap((response) => { - // if (response.isSuccessful) { - // const community$: Observable = this.objectCache.getByUUID(scopeID); - // return community$.pipe( - // map((community) => community._links[linkPath]), - // filter((href) => isNotEmpty(href)), - // distinctUntilChanged() - // ); - // } else if (!response.isSuccessful) { - // return observableThrowError(new Error(`The Community with scope ${scopeID} couldn't be retrieved`)) - // } - // }), - // distinctUntilChanged() - // ); const responses = scopeCommunityHrefObs.pipe( mergeMap((href: string) => this.requestService.getByHref(href)), getResponseFromEntry() diff --git a/src/app/core/data/config-response-parsing.service.ts b/src/app/core/data/config-response-parsing.service.ts index ddf884e02b..50303d0a09 100644 --- a/src/app/core/data/config-response-parsing.service.ts +++ b/src/app/core/data/config-response-parsing.service.ts @@ -28,7 +28,7 @@ export class ConfigResponseParsingService extends BaseResponseParsingService imp parse(request: RestRequest, data: DSpaceRESTV2Response): RestResponse { if (isNotEmpty(data.payload) && isNotEmpty(data.payload._links) && (data.statusCode === '201' || data.statusCode === '200' || data.statusCode === 'OK')) { - const configDefinition = this.process(data.payload, request.href); + const configDefinition = this.process(data.payload, request.uuid); return new ConfigSuccessResponse(configDefinition, data.statusCode, this.processPageInfo(data.payload)); } else { return new ErrorResponse( diff --git a/src/app/core/data/dso-response-parsing.service.ts b/src/app/core/data/dso-response-parsing.service.ts index 568114be1a..1066d11a50 100644 --- a/src/app/core/data/dso-response-parsing.service.ts +++ b/src/app/core/data/dso-response-parsing.service.ts @@ -28,7 +28,7 @@ export class DSOResponseParsingService extends BaseResponseParsingService implem } parse(request: RestRequest, data: DSpaceRESTV2Response): RestResponse { - const processRequestDTO = this.process(data.payload, request.href); + const processRequestDTO = this.process(data.payload, request.uuid); let objectList = processRequestDTO; if (hasNoValue(processRequestDTO)) { diff --git a/src/app/core/data/facet-value-map-response-parsing.service.ts b/src/app/core/data/facet-value-map-response-parsing.service.ts index 0fc5917847..2f580ee952 100644 --- a/src/app/core/data/facet-value-map-response-parsing.service.ts +++ b/src/app/core/data/facet-value-map-response-parsing.service.ts @@ -9,8 +9,6 @@ import { ResponseParsingService } from './parsing.service'; import { RestRequest } from './request.models'; import { DSpaceRESTV2Response } from '../dspace-rest-v2/dspace-rest-v2-response.model'; import { DSpaceRESTv2Serializer } from '../dspace-rest-v2/dspace-rest-v2.serializer'; -import { PageInfo } from '../shared/page-info.model'; -import { isNotEmpty } from '../../shared/empty.util'; import { FacetValue } from '../../+search-page/search-service/facet-value.model'; import { BaseResponseParsingService } from './base-response-parsing.service'; import { ObjectCacheService } from '../cache/object-cache.service'; diff --git a/src/app/core/data/facet-value-response-parsing.service.ts b/src/app/core/data/facet-value-response-parsing.service.ts index 585172c22e..54f36a0564 100644 --- a/src/app/core/data/facet-value-response-parsing.service.ts +++ b/src/app/core/data/facet-value-response-parsing.service.ts @@ -1,16 +1,9 @@ import { Inject, Injectable } from '@angular/core'; -import { - FacetValueMap, - FacetValueMapSuccessResponse, - FacetValueSuccessResponse, - RestResponse -} from '../cache/response.models'; +import { FacetValueSuccessResponse, RestResponse } from '../cache/response.models'; import { ResponseParsingService } from './parsing.service'; import { RestRequest } from './request.models'; import { DSpaceRESTV2Response } from '../dspace-rest-v2/dspace-rest-v2-response.model'; import { DSpaceRESTv2Serializer } from '../dspace-rest-v2/dspace-rest-v2.serializer'; -import { PageInfo } from '../shared/page-info.model'; -import { isNotEmpty } from '../../shared/empty.util'; import { FacetValue } from '../../+search-page/search-service/facet-value.model'; import { BaseResponseParsingService } from './base-response-parsing.service'; import { ObjectCacheService } from '../cache/object-cache.service'; diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index debddb748c..90d2edfc84 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -174,7 +174,7 @@ describe('RequestService', () => { // b: undefined // }); - scheduler.expectObservable(result).toBe('b', {b: undefined}); + scheduler.expectObservable(result).toBe('b', { b: undefined }); }); }); @@ -458,4 +458,105 @@ describe('RequestService', () => { }); }); }); + + describe('isReusable', () => { + describe('when the given UUID is has no value', () => { + let reusable; + beforeEach(() => { + const uuid = undefined; + reusable = serviceAsAny.isReusable(uuid); + }); + it('return an observable emitting false', () => { + reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + }) + }); + + describe('when the given UUID has a value, but no cached entry is found', () => { + let reusable; + beforeEach(() => { + spyOn(service, 'getByUUID').and.returnValue(observableOf(undefined)); + const uuid = 'a45bb291-1adb-40d9-b2fc-7ad9080607be'; + reusable = serviceAsAny.isReusable(uuid); + }); + it('return an observable emitting false', () => { + reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + }) + }); + + describe('when the given UUID has a value, a cached entry is found, but it has no response', () => { + let reusable; + beforeEach(() => { + spyOn(service, 'getByUUID').and.returnValue(observableOf({ response: undefined })); + const uuid = '53c9b814-ad8b-4567-9bc1-d9bb6cfba6c8'; + reusable = serviceAsAny.isReusable(uuid); + }); + it('return an observable emitting false', () => { + reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + }) + }); + + describe('when the given UUID has a value, a cached entry is found, but its response was not successful', () => { + let reusable; + beforeEach(() => { + spyOn(service, 'getByUUID').and.returnValue(observableOf({ response: { isSuccessful: false } })); + const uuid = '694c9b32-7b2e-4788-835b-ef3fc2252e6c'; + reusable = serviceAsAny.isReusable(uuid); + }); + it('return an observable emitting false', () => { + reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + }) + }); + + describe('when the given UUID has a value, a cached entry is found, its response was successful, but the response is outdated', () => { + let reusable; + const now = 100000; + const timeAdded = 99899; + const msToLive = 100; + + beforeEach(() => { + spyOn(Date.prototype, 'getTime').and.returnValue(now); + spyOn(service, 'getByUUID').and.returnValue(observableOf({ + response: { + isSuccessful: true, + timeAdded: timeAdded + }, + request: { + responseMsToLive: msToLive + } + })); + const uuid = 'f9b85788-881c-4994-86b6-bae8dad024d2'; + reusable = serviceAsAny.isReusable(uuid); + }); + + it('return an observable emitting false', () => { + reusable.subscribe((isReusable) => expect(isReusable).toBe(false)); + }) + }); + + describe('when the given UUID has a value, a cached entry is found, its response was successful, and the response is not outdated', () => { + let reusable; + const now = 100000; + const timeAdded = 99999; + const msToLive = 100; + + beforeEach(() => { + spyOn(Date.prototype, 'getTime').and.returnValue(now); + spyOn(service, 'getByUUID').and.returnValue(observableOf({ + response: { + isSuccessful: true, + timeAdded: timeAdded + }, + request: { + responseMsToLive: msToLive + } + })); + const uuid = 'f9b85788-881c-4994-86b6-bae8dad024d2'; + reusable = serviceAsAny.isReusable(uuid); + }); + + it('return an observable emitting true', () => { + reusable.subscribe((isReusable) => expect(isReusable).toBe(true)); + }) + }) + }) }); diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 4b7a7c9b49..285ed06545 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -76,7 +76,7 @@ export class RequestService { } getByUUID(uuid: string): Observable { - return observableRace( + return observableRace( this.store.pipe(select(this.entryFromUUIDSelector(uuid))), this.store.pipe( select(this.originalUUIDFromUUIDSelector(uuid)), @@ -94,6 +94,12 @@ export class RequestService { ); } + /** + * Configure a certain request + * Used to make sure a request is in the cache + * @param {RestRequest} request The request to send out + * @param {boolean} forceBypassCache When true, a new request is always dispatched + */ // TODO to review "overrideRequest" param when https://github.com/DSpace/dspace-angular/issues/217 will be fixed configure(request: RestRequest, forceBypassCache: boolean = false): void { const isGetRequest = request.method === RestRequestMethod.GET; @@ -113,6 +119,11 @@ export class RequestService { } } + /** + * Check if a request is in the cache or if it's still pending + * @param {GetRequest} request The request to check + * @returns {boolean} True if the request is cached or still pending + */ private isCachedOrPending(request: GetRequest) { let isCached = this.objectCache.hasBySelfLink(request.href); if (isCached) { @@ -142,6 +153,10 @@ export class RequestService { return isCached || isPending; } + /** + * Configure and execute the request + * @param {RestRequest} request to dispatch + */ private dispatchRequest(request: RestRequest) { this.store.dispatch(new RequestConfigureAction(request)); this.store.dispatch(new RequestExecuteAction(request.uuid)); @@ -164,6 +179,10 @@ export class RequestService { }); } + /** + * Dispatch commit action to send all changes (for a certain method) to the server (buffer) + * @param {RestRequestMethod} method RestRequestMethod for which the changes should be committed + */ commit(method?: RestRequestMethod) { this.store.dispatch(new CommitSSBAction(method)) } @@ -171,11 +190,11 @@ export class RequestService { /** * Check whether a Response should still be cached * - * @param entry - * the entry to check + * @param uuid + * the uuid of the entry to check * @return boolean - * false if the entry is null, undefined, or its time to - * live has been exceeded, true otherwise + * false if the uuid has no value, no entry could be found, the response was nog successful or its time to + * live has exceeded, true otherwise */ private isReusable(uuid: string): Observable { if (hasNoValue(uuid)) { @@ -194,7 +213,6 @@ export class RequestService { } }) ); - return observableOf(false); } } } diff --git a/src/app/core/integration/integration-response-parsing.service.ts b/src/app/core/integration/integration-response-parsing.service.ts index 6eff7ab792..ef278c93de 100644 --- a/src/app/core/integration/integration-response-parsing.service.ts +++ b/src/app/core/integration/integration-response-parsing.service.ts @@ -32,7 +32,7 @@ export class IntegrationResponseParsingService extends BaseResponseParsingServic parse(request: RestRequest, data: DSpaceRESTV2Response): RestResponse { if (isNotEmpty(data.payload) && isNotEmpty(data.payload._links)) { - const dataDefinition = this.process(data.payload, request.href); + const dataDefinition = this.process(data.payload, request.uuid); return new IntegrationSuccessResponse(dataDefinition, data.statusCode, this.processPageInfo(data.payload.page)); } else { return new ErrorResponse( diff --git a/src/app/core/metadata/metadata.service.spec.ts b/src/app/core/metadata/metadata.service.spec.ts index 2b47239729..21ace8a250 100644 --- a/src/app/core/metadata/metadata.service.spec.ts +++ b/src/app/core/metadata/metadata.service.spec.ts @@ -32,9 +32,9 @@ import { MockTranslateLoader } from '../../shared/mocks/mock-translate-loader'; import { BrowseService } from '../browse/browse.service'; import { HALEndpointService } from '../shared/hal-endpoint.service'; import { AuthService } from '../auth/auth.service'; -import { REQUEST } from '@nguniversal/express-engine/tokens'; import { NotificationsService } from '../../shared/notifications/notifications.service'; import { HttpClient } from '@angular/common/http'; +import { EmptyError } from 'rxjs/internal-compatibility'; /* tslint:disable:max-classes-per-file */ @Component({ @@ -184,6 +184,22 @@ describe('MetadataService', () => { expect(tagStore.get('description')[0].content).toEqual('This is a dummy item component for testing!'); })); + describe('when the item has no bitstreams', () => { + + beforeEach(() => { + spyOn(MockItem, 'getFiles').and.returnValue(observableOf([])); + }); + + it('processRemoteData should not produce an EmptyError', fakeAsync(() => { + spyOn(itemDataService, 'findById').and.returnValue(mockRemoteData(MockItem)); + spyOn(metadataService, 'processRemoteData').and.callThrough(); + router.navigate(['/items/0ec7ff22-f211-40ab-a69e-c819b0b1f357']); + tick(); + expect(metadataService.processRemoteData).not.toThrow(new EmptyError()); + })); + + }); + const mockRemoteData = (mockItem: Item): Observable> => { return observableOf(new RemoteData( false, diff --git a/src/app/core/metadata/metadata.service.ts b/src/app/core/metadata/metadata.service.ts index ede66f6952..8bec058d08 100644 --- a/src/app/core/metadata/metadata.service.ts +++ b/src/app/core/metadata/metadata.service.ts @@ -1,4 +1,4 @@ -import { distinctUntilKeyChanged, filter, first, map, take } from 'rxjs/operators'; +import { catchError, distinctUntilKeyChanged, filter, first, map, take } from 'rxjs/operators'; import { Inject, Injectable } from '@angular/core'; import { ActivatedRoute, NavigationEnd, Router } from '@angular/router'; @@ -52,8 +52,8 @@ export class MetadataService { route = this.getCurrentRoute(route); return { params: route.params, data: route.data }; }),).subscribe((routeInfo: any) => { - this.processRouteChange(routeInfo); - }); + this.processRouteChange(routeInfo); + }); } public processRemoteData(remoteData: Observable>): void { @@ -259,18 +259,30 @@ export class MetadataService { private setCitationPdfUrlTag(): void { if (this.currentObject.value instanceof Item) { const item = this.currentObject.value as Item; - item.getFiles().pipe(filter((files) => isNotEmpty(files)),first(),).subscribe((bitstreams: Bitstream[]) => { - for (const bitstream of bitstreams) { - bitstream.format.pipe(first(), - map((rd: RemoteData) => rd.payload), - filter((format: BitstreamFormat) => hasValue(format)),) - .subscribe((format: BitstreamFormat) => { - if (format.mimetype === 'application/pdf') { - this.addMetaTag('citation_pdf_url', bitstream.content); - } - }); - } - }); + item.getFiles() + .pipe( + first((files) => isNotEmpty(files)), + catchError((error) => { + console.debug(error.message); + return [] + })) + .subscribe((bitstreams: Bitstream[]) => { + for (const bitstream of bitstreams) { + bitstream.format.pipe( + first(), + catchError((error: Error) => { + console.debug(error.message); + return [] + }), + map((rd: RemoteData) => rd.payload), + filter((format: BitstreamFormat) => hasValue(format))) + .subscribe((format: BitstreamFormat) => { + if (format.mimetype === 'application/pdf') { + this.addMetaTag('citation_pdf_url', bitstream.content); + } + }); + } + }); } } diff --git a/src/app/core/shared/hal-endpoint.service.spec.ts b/src/app/core/shared/hal-endpoint.service.spec.ts index b65b3f905b..8b3011e7d7 100644 --- a/src/app/core/shared/hal-endpoint.service.spec.ts +++ b/src/app/core/shared/hal-endpoint.service.spec.ts @@ -15,6 +15,28 @@ describe('HALEndpointService', () => { const endpointMap = { test: 'https://rest.api/test', + foo: 'https://rest.api/foo', + bar: 'https://rest.api/bar', + endpoint: 'https://rest.api/endpoint', + link: 'https://rest.api/link', + another: 'https://rest.api/another', + }; + const start = 'http://start.com'; + const one = 'http://one.com'; + const two = 'http://two.com'; + const endpointMaps = { + [start]: { + one: one, + two: 'empty', + endpoint: 'https://rest.api/endpoint', + link: 'https://rest.api/link', + another: 'https://rest.api/another', + }, + [one]: { + one: 'empty', + two: two, + bar: 'https://rest.api/bar', + } }; const linkPath = 'test'; @@ -80,6 +102,50 @@ describe('HALEndpointService', () => { }); }); + describe('getEndpointAt', () => { + it('should throw an error when the list of hal endpoint names is empty', () => { + const endpointAtWithoutEndpointNames = () => { + (service as any).getEndpointAt('') + }; + expect(endpointAtWithoutEndpointNames).toThrow(); + }); + + it('should be at least called as many times as the length of halNames', () => { + spyOn(service as any, 'getEndpointMapAt').and.returnValue(observableOf(endpointMap)); + spyOn((service as any), 'getEndpointAt').and.callThrough(); + + (service as any).getEndpointAt('', 'endpoint').subscribe(); + + expect((service as any).getEndpointAt.calls.count()).toEqual(1); + + (service as any).getEndpointAt.calls.reset(); + + (service as any).getEndpointAt('', 'endpoint', 'another').subscribe(); + + expect((service as any).getEndpointAt.calls.count()).toBeGreaterThanOrEqual(2); + + (service as any).getEndpointAt.calls.reset(); + + (service as any).getEndpointAt('', 'endpoint', 'another', 'foo', 'bar', 'test').subscribe(); + + expect((service as any).getEndpointAt.calls.count()).toBeGreaterThanOrEqual(5); + }); + + it('should return the correct endpoint', () => { + spyOn(service as any, 'getEndpointMapAt').and.callFake((param) => { + return observableOf(endpointMaps[param]); + }); + + (service as any).getEndpointAt(start, 'one').subscribe((endpoint) => { + expect(endpoint).toEqual(one); + }); + + (service as any).getEndpointAt(start, 'one', 'two').subscribe((endpoint) => { + expect(endpoint).toEqual(two); + }); + }); + }); + describe('isEnabledOnRestApi', () => { beforeEach(() => { service = new HALEndpointService( diff --git a/src/app/core/shared/hal-endpoint.service.ts b/src/app/core/shared/hal-endpoint.service.ts index a0bf7aabd5..a93d54db64 100644 --- a/src/app/core/shared/hal-endpoint.service.ts +++ b/src/app/core/shared/hal-endpoint.service.ts @@ -47,6 +47,12 @@ export class HALEndpointService { return this.getEndpointAt(this.getRootHref(), ...linkPath.split('/')); } + /** + * Resolve the actual hal url based on a list of hal names + * @param {string} href The root url to start from + * @param {string} halNames List of hal names for which a url should be resolved + * @returns {Observable} Observable that emits the found hal url + */ private getEndpointAt(href: string, ...halNames: string[]): Observable { if (isEmpty(halNames)) { throw new Error('cant\'t fetch the URL without the HAL link names') diff --git a/src/app/core/shared/operators.spec.ts b/src/app/core/shared/operators.spec.ts index 5f29de9e93..6aeec230c4 100644 --- a/src/app/core/shared/operators.spec.ts +++ b/src/app/core/shared/operators.spec.ts @@ -7,22 +7,24 @@ import { RequestService } from '../data/request.service'; import { configureRequest, filterSuccessfulResponses, - getRemoteDataPayload, - getRequestFromSelflink, - getResourceLinksFromResponse, + getRemoteDataPayload, getRequestFromRequestHref, getRequestFromRequestUUID, + getResourceLinksFromResponse, getResponseFromEntry, } from './operators'; describe('Core Module - RxJS Operators', () => { let scheduler: TestScheduler; let requestService: RequestService; - const testSelfLink = 'https://rest.api/'; + const testRequestHref = 'https://rest.api/'; + const testRequestUUID = 'https://rest.api/'; const testRCEs = { a: { response: { isSuccessful: true, resourceSelfLinks: ['a', 'b', 'c', 'd'] } }, b: { response: { isSuccessful: false, resourceSelfLinks: ['e', 'f'] } }, c: { response: { isSuccessful: undefined, resourceSelfLinks: ['g', 'h', 'i'] } }, d: { response: { isSuccessful: true, resourceSelfLinks: ['j', 'k', 'l', 'm', 'n'] } }, - e: { response: { isSuccessful: 1, resourceSelfLinks: [] } } + e: { response: { isSuccessful: 1, resourceSelfLinks: [] } }, + f: { response: undefined }, + g: undefined }; const testResponses = { @@ -37,14 +39,14 @@ describe('Core Module - RxJS Operators', () => { scheduler = getTestScheduler(); }); - describe('getRequestFromSelflink', () => { + describe('getRequestFromRequestHref', () => { it('should return the RequestEntry corresponding to the self link in the source', () => { requestService = getMockRequestService(); - const source = hot('a', { a: testSelfLink }); - const result = source.pipe(getRequestFromSelflink(requestService)); - const expected = cold('a', { a: new RequestEntry()}); + const source = hot('a', { a: testRequestHref }); + const result = source.pipe(getRequestFromRequestHref(requestService)); + const expected = cold('a', { a: new RequestEntry() }); expect(result).toBeObservable(expected) }); @@ -52,18 +54,51 @@ describe('Core Module - RxJS Operators', () => { it('should use the requestService to fetch the request by its self link', () => { requestService = getMockRequestService(); - const source = hot('a', { a: testSelfLink }); - scheduler.schedule(() => source.pipe(getRequestFromSelflink(requestService)).subscribe()); + const source = hot('a', { a: testRequestHref }); + scheduler.schedule(() => source.pipe(getRequestFromRequestHref(requestService)).subscribe()); scheduler.flush(); - expect(requestService.getByHref).toHaveBeenCalledWith(testSelfLink) + expect(requestService.getByHref).toHaveBeenCalledWith(testRequestHref) }); it('shouldn\'t return anything if there is no request matching the self link', () => { requestService = getMockRequestService(cold('a', { a: undefined })); - const source = hot('a', { a: testSelfLink }); - const result = source.pipe(getRequestFromSelflink(requestService)); + const source = hot('a', { a: testRequestUUID }); + const result = source.pipe(getRequestFromRequestHref(requestService)); + const expected = cold('-'); + + expect(result).toBeObservable(expected) + }); + }); + + describe('getRequestFromRequestUUID', () => { + + it('should return the RequestEntry corresponding to the request uuid in the source', () => { + requestService = getMockRequestService(); + + const source = hot('a', { a: testRequestUUID }); + const result = source.pipe(getRequestFromRequestUUID(requestService)); + const expected = cold('a', { a: new RequestEntry() }); + + expect(result).toBeObservable(expected) + }); + + it('should use the requestService to fetch the request by its request uuid', () => { + requestService = getMockRequestService(); + + const source = hot('a', { a: testRequestUUID }); + scheduler.schedule(() => source.pipe(getRequestFromRequestUUID(requestService)).subscribe()); + scheduler.flush(); + + expect(requestService.getByUUID).toHaveBeenCalledWith(testRequestUUID) + }); + + it('shouldn\'t return anything if there is no request matching the request uuid', () => { + requestService = getMockRequestService(cold('a', { a: undefined })); + + const source = hot('a', { a: testRequestUUID }); + const result = source.pipe(getRequestFromRequestUUID(requestService)); const expected = cold('-'); expect(result).toBeObservable(expected) @@ -96,7 +131,7 @@ describe('Core Module - RxJS Operators', () => { describe('configureRequest', () => { it('should call requestService.configure with the source request', () => { requestService = getMockRequestService(); - const testRequest = new GetRequest('6b789e31-f026-4ff8-8993-4eb3b730c841', testSelfLink); + const testRequest = new GetRequest('6b789e31-f026-4ff8-8993-4eb3b730c841', testRequestHref); const source = hot('a', { a: testRequest }); scheduler.schedule(() => source.pipe(configureRequest(requestService)).subscribe()); scheduler.flush(); @@ -117,4 +152,20 @@ describe('Core Module - RxJS Operators', () => { expect(result).toBeObservable(expected) }); }); + + describe('getResponseFromEntry', () => { + it('should return the response for all not empty request entries, when they have a value', () => { + const source = hot('abcdefg', testRCEs); + const result = source.pipe(getResponseFromEntry()); + const expected = cold('abcde--', { + a: testRCEs.a.response, + b: testRCEs.b.response, + c: testRCEs.c.response, + d: testRCEs.d.response, + e: testRCEs.e.response + }); + + expect(result).toBeObservable(expected) + }); + }); }); diff --git a/src/app/core/shared/operators.ts b/src/app/core/shared/operators.ts index a6335ebb5d..4e6a95ff11 100644 --- a/src/app/core/shared/operators.ts +++ b/src/app/core/shared/operators.ts @@ -15,13 +15,20 @@ import { SearchResult } from '../../+search-page/search-result.model'; * This file contains custom RxJS operators that can be used in multiple places */ -export const getRequestFromSelflink = (requestService: RequestService) => +export const getRequestFromRequestHref = (requestService: RequestService) => (source: Observable): Observable => source.pipe( flatMap((href: string) => requestService.getByHref(href)), hasValueOperator() ); +export const getRequestFromRequestUUID = (requestService: RequestService) => + (source: Observable): Observable => + source.pipe( + flatMap((uuid: string) => requestService.getByUUID(uuid)), + hasValueOperator() + ); + export const filterSuccessfulResponses = () => (source: Observable): Observable => source.pipe( diff --git a/src/app/shared/form/form.service.spec.ts b/src/app/shared/form/form.service.spec.ts index 1c13039e3c..80dfa01d1b 100644 --- a/src/app/shared/form/form.service.spec.ts +++ b/src/app/shared/form/form.service.spec.ts @@ -2,7 +2,11 @@ import { Store, StoreModule } from '@ngrx/store'; import { async, inject, TestBed } from '@angular/core/testing'; import { AbstractControl, FormControl, FormGroup, Validators } from '@angular/forms'; -import { DynamicFormControlModel, DynamicInputModel } from '@ng-dynamic-forms/core'; +import { + DynamicFormControlModel, + DynamicFormGroupModel, + DynamicInputModel +} from '@ng-dynamic-forms/core'; import { FormService } from './form.service'; import { FormBuilderService } from './builder/form-builder.service'; @@ -37,30 +41,30 @@ describe('FormService test suite', () => { }), new DynamicInputModel({ id: 'date' }), new DynamicInputModel({ id: 'description' }), - // new DynamicFormGroupModel({ - // - // id: 'addressLocation', - // group: [ - // new DynamicInputModel({ - // - // id: 'zipCode', - // label: 'Zip Code', - // placeholder: 'ZIP' - // }), - // new DynamicInputModel({ - // - // id: 'state', - // label: 'State', - // placeholder: 'State' - // }), - // new DynamicInputModel({ - // - // id: 'city', - // label: 'City', - // placeholder: 'City' - // }) - // ] - // }), + new DynamicFormGroupModel({ + + id: 'addressLocation', + group: [ + new DynamicInputModel({ + + id: 'zipCode', + label: 'Zip Code', + placeholder: 'ZIP' + }), + new DynamicInputModel({ + + id: 'state', + label: 'State', + placeholder: 'State' + }), + new DynamicInputModel({ + + id: 'city', + label: 'City', + placeholder: 'City' + }) + ] + }), ]; let controls; diff --git a/src/app/shared/loading/loading.component.ts b/src/app/shared/loading/loading.component.ts index b827bcc683..fec05b4720 100644 --- a/src/app/shared/loading/loading.component.ts +++ b/src/app/shared/loading/loading.component.ts @@ -3,6 +3,7 @@ import { Component, Input, OnDestroy, OnInit } from '@angular/core'; import { TranslateService } from '@ngx-translate/core'; import { Subscription } from 'rxjs'; +import { hasValue } from '../empty.util'; @Component({ selector: 'ds-loading', @@ -28,7 +29,7 @@ export class LoadingComponent implements OnDestroy, OnInit { } ngOnDestroy() { - if (this.subscription !== undefined) { + if (hasValue(this.subscription)) { this.subscription.unsubscribe(); } } diff --git a/src/app/shared/mocks/mock-request.service.ts b/src/app/shared/mocks/mock-request.service.ts index d46100d56c..c947618ea1 100644 --- a/src/app/shared/mocks/mock-request.service.ts +++ b/src/app/shared/mocks/mock-request.service.ts @@ -2,10 +2,11 @@ import {of as observableOf, Observable } from 'rxjs'; import { RequestService } from '../../core/data/request.service'; import { RequestEntry } from '../../core/data/request.reducer'; -export function getMockRequestService(getByHref$: Observable = observableOf(new RequestEntry())): RequestService { +export function getMockRequestService(requestEntry$: Observable = observableOf(new RequestEntry())): RequestService { return jasmine.createSpyObj('requestService', { configure: false, generateRequestId: 'clients/b186e8ce-e99c-4183-bc9a-42b4821bdb78', - getByHref: getByHref$ + getByHref: requestEntry$, + getByUUID: requestEntry$ }); } diff --git a/src/app/shared/testing/query-params-directive-stub.ts b/src/app/shared/testing/query-params-directive-stub.ts new file mode 100644 index 0000000000..34216bb53c --- /dev/null +++ b/src/app/shared/testing/query-params-directive-stub.ts @@ -0,0 +1,10 @@ +import { Directive, Input } from '@angular/core'; + +/* tslint:disable:directive-class-suffix */ +@Directive({ + // tslint:disable-next-line:directive-selector + selector: '[queryParams]', +}) +export class QueryParamsDirectiveStub { + @Input() queryParams: any; +} diff --git a/src/app/shared/testing/test-module.ts b/src/app/shared/testing/test-module.ts new file mode 100644 index 0000000000..03d22640d3 --- /dev/null +++ b/src/app/shared/testing/test-module.ts @@ -0,0 +1,15 @@ +import { NgModule } from '@angular/core'; +import { QueryParamsDirectiveStub } from './query-params-directive-stub'; + +/** + * This module isn't used. It serves to prevent the AoT compiler + * complaining about components/pipes/directives that were + * created only for use in tests. + * See https://github.com/angular/angular/issues/13590 + */ +@NgModule({ + declarations: [ + QueryParamsDirectiveStub + ] +}) +export class TestModule {} diff --git a/src/server.ts b/src/server.ts index 13d0b2fd89..0526f196ba 100644 --- a/src/server.ts +++ b/src/server.ts @@ -17,6 +17,7 @@ import { ngExpressEngine } from '@nguniversal/express-engine'; import { ROUTES } from './routes'; import { ENV_CONFIG } from './config'; +import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens'; export function startServer(bootstrap: Type<{}> | NgModuleFactory<{}>) { const app = express(); @@ -31,9 +32,21 @@ export function startServer(bootstrap: Type<{}> | NgModuleFactory<{}>) { app.use(cookieParser()); app.use(bodyParser.json()); - app.engine('html', ngExpressEngine({ - bootstrap: bootstrap - })); + app.engine('html', (_, options, callback) => + ngExpressEngine({ + bootstrap: bootstrap, + providers: [ + { + provide: REQUEST, + useValue: options.req, + }, + { + provide: RESPONSE, + useValue: options.req.res, + }, + ], + })(_, options, callback) + ); app.set('view engine', 'html'); app.set('views', 'src'); @@ -53,9 +66,10 @@ export function startServer(bootstrap: Type<{}> | NgModuleFactory<{}>) { function ngApp(req, res) { function onHandleError(parentZoneDelegate, currentZone, targetZone, error) { - console.error('Error:', error); - console.warn('Error in SSR, serving for direct CSR'); - res.sendFile('index.csr.html', { root: './src' }); + if (!res._headerSent) { + console.warn('Error in SSR, serving for direct CSR'); + res.sendFile('index.csr.html', { root: './src' }); + } } if (ENV_CONFIG.universal.preboot) { diff --git a/webpack.config.js b/webpack.config.js index 8f02f3ce8a..6312cf3605 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -27,7 +27,7 @@ module.exports = function(env, options) { getAotPlugin('client', !!env.aot) ] }); - if (env.production) { + if (options.mode === 'production') { serverConfig = webpackMerge({}, serverConfig, prodPartial); clientConfig = webpackMerge({}, clientConfig, prodPartial); }