Merge branch 'patch-support' into response-cache-refactoring

Conflicts:
	src/app/core/cache/builders/remote-data-build.service.ts
	src/app/core/shared/operators.ts

resources/i18n/en.json

@@ -92,7 +92,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",

src/app/+search-page/search-results/search-results.component.html

@@ -7,5 +7,12 @@
   [hideGear]="true">
 </ds-viewable-collection></div>
 <ds-loading *ngIf="!searchResults || searchResults?.isLoading" message="{{'loading.search-results' | translate}}"></ds-loading>
-<ds-error *ngIf="searchResults?.hasFailed" message="{{'error.search-results' | translate}}"></ds-error>
+<ds-error *ngIf="searchResults?.hasFailed && (!searchResults?.error || searchResults?.error?.statusCode != 400)" message="{{'error.search-results' | translate}}"></ds-error>
-<ds-error *ngIf="searchResults?.payload?.page.length == 0" message="{{'search.results.no-results' | translate}}"></ds-error>
+<div *ngIf="searchResults?.payload?.page.length == 0 || searchResults?.error?.statusCode == 400">
+    {{ 'search.results.no-results' | translate }}
+    <a [routerLink]="['/search']"
+      [queryParams]="{ query: surroundStringWithQuotes(searchConfig?.query) }"
+      queryParamsHandling="merge">
+      {{"search.results.no-results-link" | translate}}
+    </a>
+</div> 

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<SearchResultsComponent>;
-  let heading: DebugElement;
 
   beforeEach(async(() => {
     TestBed.configureTestingModule({
-      imports: [TranslateModule.forRoot()],
+      imports: [TranslateModule.forRoot(), NoopAnimationsModule],
-      declarations: [SearchResultsComponent],
+      declarations: [
+        SearchResultsComponent,
+        QueryParamsDirectiveStub],
       schemas: [NO_ERRORS_SCHEMA]
     }).compileComponents();
   }));
 
   beforeEach(() => {
     fixture = TestBed.createComponent(SearchResultsComponent);
-    comp = fixture.componentInstance; // SearchFormComponent test instance
+    comp = fixture.componentInstance; // SearchResultsComponent test instance
-    heading = fixture.debugElement.query(By.css('heading'));
   });
 
-  it('should display heading when results are not empty', fakeAsync(() => {
+  it('should display results when results are not empty', () => {
-    (comp as any).searchResults = 'test';
+    (comp as any).searchResults = { hasSucceeded: true, isLoading: false, payload: { page: { length: 2 } } };
-    (comp as any).searchConfig = {pagination: ''};
+    (comp as any).searchConfig = {};
     fixture.detectChanges();
-    tick();
+    expect(fixture.debugElement.query(By.css('ds-viewable-collection'))).not.toBeNull();
-    expect(heading).toBeDefined();
+  });
-  }));
 
-  it('should not display heading when results is empty', () => {
+  it('should not display link when results are not empty', () => {
-    expect(heading).toBeNull();
+    (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('');
   });
 });
 

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;
+  }
 }

src/app/app.module.ts

@@ -7,6 +7,7 @@ import { NgbModule } from '@ng-bootstrap/ng-bootstrap';
 import { EffectsModule } from '@ngrx/effects';
 import { RouterStateSerializer, StoreRouterConnectingModule } from '@ngrx/router-store';
 import { META_REDUCERS, MetaReducer, StoreModule } from '@ngrx/store';
+import { StoreDevtoolsModule } from '@ngrx/store-devtools';
 
 import { TranslateModule } from '@ngx-translate/core';
 
@@ -44,10 +45,7 @@ export function getMetaReducers(config: GlobalConfig): Array<MetaReducer<AppStat
   return config.debug ? [...metaReducers, ...debugMetaReducers] : metaReducers;
 }
 
-const DEV_MODULES: any[] = [];
+const IMPORTS = [
-
-@NgModule({
-  imports: [
   CommonModule,
   SharedModule,
   HttpClientModule,
@@ -58,9 +56,16 @@ const DEV_MODULES: any[] = [];
   EffectsModule.forRoot(appEffects),
   StoreModule.forRoot(appReducers),
   StoreRouterConnectingModule,
-    ...DEV_MODULES
+];
-  ],
+
-  providers: [
+IMPORTS.push(
+  StoreDevtoolsModule.instrument({
+    maxAge: 100,
+    logOnly: ENV_CONFIG.production,
+  })
+);
+
+const PROVIDERS = [
   {
     provide: GLOBAL_CONFIG,
     useFactory: (getConfig)
@@ -78,16 +83,34 @@ const DEV_MODULES: any[] = [];
     provide: RouterStateSerializer,
     useClass: DSpaceRouterStateSerializer
   }
-  ],
+];
-  declarations: [
+
+const DECLARATIONS = [
   AppComponent,
   HeaderComponent,
   FooterComponent,
   PageNotFoundComponent,
   NotificationComponent,
   NotificationsBoardComponent
+];
+
+const EXPORTS = [
+  AppComponent
+];
+
+@NgModule({
+  imports: [
+    ...IMPORTS
   ],
-  exports: [AppComponent]
+  providers: [
+    ...PROVIDERS
+  ],
+  declarations: [
+    ...DECLARATIONS
+  ],
+  exports: [
+    ...EXPORTS
+  ]
 })
 export class AppModule {
 

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, undefined);
+  });
+
+  describe('when toPaginatedList is called', () => {
+    let expected: RemoteData<PaginatedList<Item>>;
+
+    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);
+      });
+    });
+  });
+});

src/app/core/cache/builders/remote-data-build.service.ts

@@ -196,7 +196,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 +258,16 @@ export class RemoteDataBuildService {
       }))
   }
 
-  aggregatePaginatedList<T>(input: Observable<RemoteData<T[]>>, pageInfo: PageInfo): Observable<RemoteData<PaginatedList<T>>> {
+  private toPaginatedList<T>(input: Observable<RemoteData<T[] | PaginatedList<T>>>, pageInfo: PageInfo): Observable<RemoteData<PaginatedList<T>>> {
-    return input.pipe(map((rd) => Object.assign(rd, { payload: new PaginatedList(pageInfo, rd.payload) })));
+    return input.pipe(
+      map((rd: RemoteData<T[] | PaginatedList<T>>) => {
+        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) });
+        }
+      })
+    );
   }
 
 }

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;
@@ -21,6 +22,7 @@ class NullAction extends RemoveFromObjectCacheAction {
 describe('objectCacheReducer', () => {
   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: {
@@ -140,15 +142,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
+    const newState = objectCacheReducer(stateWithPatch, action);
-    objectCacheReducer(testState, action);
+    expect(newState[selfLink1].patches).toEqual([]);
+    expect((newState[selfLink1].data as any).name).toEqual(newName);
   });
 
 });

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<Observable<Action>> = bufferState.buffer
               .filter((entry: ServerSyncBufferEntry) => {

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 */
@@ -129,7 +128,7 @@ export abstract class BaseResponseParsingService {
   }
 
   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) {

src/app/core/metadata/metadata.service.spec.ts

@@ -31,6 +31,7 @@ import { MockItem } from '../../shared/mocks/mock-item';
 import { MockTranslateLoader } from '../../shared/mocks/mock-translate-loader';
 import { BrowseService } from '../browse/browse.service';
 import { HALEndpointService } from '../shared/hal-endpoint.service';
+import { EmptyError } from 'rxjs/internal-compatibility';
 
 /* tslint:disable:max-classes-per-file */
 @Component({
@@ -175,6 +176,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<RemoteData<Item>> => {
     return observableOf(new RemoteData<Item>(
       false,

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';
 
@@ -259,11 +259,19 @@ 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[]) => {
+      item.getFiles()
+        .pipe(
+          first((files) => isNotEmpty(files)),
+          catchError((error) => {
+            console.debug(error);
+            return []
+          }))
+        .subscribe((bitstreams: Bitstream[]) => {
           for (const bitstream of bitstreams) {
-          bitstream.format.pipe(first(),
+            bitstream.format.pipe(
+              first(),
               map((rd: RemoteData<BitstreamFormat>) => rd.payload),
-            filter((format: BitstreamFormat) => hasValue(format)),)
+              filter((format: BitstreamFormat) => hasValue(format)))
               .subscribe((format: BitstreamFormat) => {
                 if (format.mimetype === 'application/pdf') {
                   this.addMetaTag('citation_pdf_url', bitstream.content);

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();
     }
   }

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') queryParams: any;
+}

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 {}