TUL/Browse links: carry only the parameters a browse page uses

src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.html

@@ -1,5 +1,5 @@
 <div class="d-flex flex-row">
-    <a *ngIf="linkType != linkTypes.None" [target]="(linkType == linkTypes.ExternalLink) ? '_blank' : '_self'" rel="noopener noreferrer" [routerLink]="[]" [queryParams]="queryParams$ | async" [queryParamsHandling]="'merge'" class="lead">
+    <a *ngIf="linkType != linkTypes.None" [target]="(linkType == linkTypes.ExternalLink) ? '_blank' : '_self'" rel="noopener noreferrer" [routerLink]="[]" [queryParams]="queryParams$ | async" class="lead">
         {{object.value}}
     </a>
     <span *ngIf="linkType == linkTypes.None" class="lead">

src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts

@@ -1,12 +1,16 @@
-import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
-import { ChangeDetectionStrategy, NO_ERRORS_SCHEMA } from '@angular/core';
+import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing';
+import { ChangeDetectionStrategy, Component, NO_ERRORS_SCHEMA } from '@angular/core';
 import { By } from '@angular/platform-browser';
+import { Params, Router } from '@angular/router';
+import { RouterTestingModule } from '@angular/router/testing';
+import { Store } from '@ngrx/store';
 import { TruncatePipe } from '../../utils/truncate.pipe';
 import { BrowseEntryListElementComponent } from './browse-entry-list-element.component';
 import { BrowseEntry } from '../../../core/shared/browse-entry.model';
 import { PaginationService } from '../../../core/pagination/pagination.service';
 import { RouteService } from '../../../core/services/route.service';
 import { of as observableOf } from 'rxjs';
+
 let browseEntryListElementComponent: BrowseEntryListElementComponent;
 let fixture: ComponentFixture<BrowseEntryListElementComponent>;
 
@@ -15,22 +19,21 @@ const mockValue: BrowseEntry = Object.assign(new BrowseEntry(), {
   value: 'De Langhe Kristof'
 });
 
-let paginationService;
-let routeService;
 const pageParam = 'bbm.page';
 
-function init() {
-  paginationService = jasmine.createSpyObj('paginationService', {
-    getPageParam: pageParam
-  });
-
-  routeService = jasmine.createSpyObj('routeService', {
-    getQueryParameterValue: observableOf('1')
-  });
+@Component({ template: '' })
+class DummyComponent {
 }
+
 describe('BrowseEntryListElementComponent', () => {
   beforeEach(waitForAsync(() => {
-    init();
+    const paginationService = jasmine.createSpyObj('paginationService', {
+      getPageParam: pageParam
+    });
+    const routeService = jasmine.createSpyObj('routeService', {
+      getQueryParameterValue: observableOf(undefined)
+    });
+
     TestBed.configureTestingModule({
       declarations: [BrowseEntryListElementComponent, TruncatePipe],
       providers: [
@@ -62,3 +65,85 @@ describe('BrowseEntryListElementComponent', () => {
     });
   });
 });
+
+describe('BrowseEntryListElementComponent link', () => {
+  // The real RouteService is used here on purpose: every parameter below reaches the component
+  // through the router, the way it does in the browser, so the assertions are on what the URL
+  // actually produces rather than on what a stub was told to answer.
+  const scopeUUID = 'a2f2d0a1-3f0e-4d3a-9c1b-5f7e8a9b0c1d';
+  let router: Router;
+
+  const hrefFor = (queryParams: Params): string => {
+    void router.navigate(['/browse/author'], { queryParams });
+    tick();
+
+    fixture = TestBed.createComponent(BrowseEntryListElementComponent);
+    fixture.componentInstance.object = mockValue;
+    fixture.detectChanges();
+
+    return fixture.debugElement.query(By.css('a.lead')).nativeElement.getAttribute('href');
+  };
+
+  beforeEach(waitForAsync(() => {
+    const paginationService = jasmine.createSpyObj('paginationService', {
+      getPageParam: pageParam
+    });
+
+    TestBed.configureTestingModule({
+      imports: [
+        RouterTestingModule.withRoutes([
+          { path: 'browse/author', component: DummyComponent }
+        ])
+      ],
+      declarations: [BrowseEntryListElementComponent, DummyComponent, TruncatePipe],
+      providers: [
+        { provide: 'objectElementProvider', useValue: { mockValue } },
+        {provide: PaginationService, useValue: paginationService},
+        {provide: Store, useValue: jasmine.createSpyObj('store', ['dispatch'])},
+      ],
+      schemas: [NO_ERRORS_SCHEMA]
+    }).compileComponents();
+  }));
+
+  beforeEach(() => {
+    router = TestBed.inject(Router);
+  });
+
+  it('should read its parameters through the real RouteService', () => {
+    expect(TestBed.inject(RouteService) instanceof RouteService).toBeTruthy();
+  });
+
+  it('should carry over the scope and the pagination settings', fakeAsync(() => {
+    const href = hrefFor({
+      scope: scopeUUID,
+      'bbm.rpp': '40',
+      'bbm.sf': 'title',
+      'bbm.sd': 'DESC'
+    });
+
+    expect(href).toContain(`scope=${scopeUUID}`);
+    expect(href).toContain('bbm.rpp=40');
+    expect(href).toContain('bbm.sf=title');
+    expect(href).toContain('bbm.sd=DESC');
+  }));
+
+  it('should replace the current page with the page to return to', fakeAsync(() => {
+    const href = hrefFor({ 'bbm.page': '3' });
+
+    expect(href).toContain('bbm.return=3');
+    expect(href).not.toContain('bbm.page=');
+  }));
+
+  it('should drop a parameter the browse page never asked for', fakeAsync(() => {
+    const href = hrefFor({ scope: scopeUUID, 'amp;value': 'Some Author' });
+
+    expect(href).toContain(`scope=${scopeUUID}`);
+    expect(href).not.toContain('amp');
+  }));
+
+  it('should not pass on a scope that is present but empty', fakeAsync(() => {
+    const href = hrefFor({ scope: '' });
+
+    expect(href).not.toContain('scope=');
+  }));
+});

src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.ts

@@ -8,7 +8,7 @@ import { PaginationService } from '../../../core/pagination/pagination.service';
 import { Params } from '@angular/router';
 import { BBM_PAGINATION_ID } from '../../../browse-by/browse-by-metadata-page/browse-by-metadata-page.component';
 import { RouteService } from 'src/app/core/services/route.service';
-import { Observable } from 'rxjs';
+import { combineLatest, Observable } from 'rxjs';
 import { map } from 'rxjs/operators';
 
 @Component({
@@ -37,16 +37,29 @@ export class BrowseEntryListElementComponent extends AbstractListableElementComp
 
   /**
    * Get the query params to access the item page of this browse entry.
+   *
+   * Carries over only the parameters a browse page actually uses. Anything else in the current URL
+   * is dropped, so a malformed parameter cannot be reflected back into the links we generate.
    */
   private getQueryParams(): Observable<Params> {
     const pageParamName = this.paginationService.getPageParam(BBM_PAGINATION_ID);
-    return this.routeService.getQueryParameterValue(pageParamName).pipe(
-      map((currentPage) => {
+    return combineLatest([
+      this.routeService.getQueryParameterValue(pageParamName),
+      this.routeService.getQueryParameterValue('scope'),
+      this.routeService.getQueryParameterValue(`${BBM_PAGINATION_ID}.rpp`),
+      this.routeService.getQueryParameterValue(`${BBM_PAGINATION_ID}.sf`),
+      this.routeService.getQueryParameterValue(`${BBM_PAGINATION_ID}.sd`),
+    ]).pipe(
+      map(([currentPage, scope, rpp, sortField, sortDirection]) => {
         return {
           value: this.object.value,
           authority: !!this.object.authority ? this.object.authority : undefined,
+          scope: scope || undefined,
           startsWith: undefined,
           [pageParamName]: null,
+          [`${BBM_PAGINATION_ID}.rpp`]: rpp || undefined,
+          [`${BBM_PAGINATION_ID}.sf`]: sortField || undefined,
+          [`${BBM_PAGINATION_ID}.sd`]: sortDirection || undefined,
           [BBM_PAGINATION_ID + '.return']: currentPage
         };
       })