111731: Made the code completely dependent on the SearchService#appliedFilters$ instead of relying on the route query parameters

Author: alexandrevryghem

src/app/core/shared/search/search-filter.service.spec.ts

@@ -153,17 +153,6 @@ describe('SearchFilterService', () => {
     });
   });
 
-  describe('when the getSelectedValuesForFilter method is called', () => {
-    beforeEach(() => {
-      spyOn(routeServiceStub, 'getQueryParameterValues');
-      service.getSelectedValuesForFilter(mockFilterConfig);
-    });
-
-    it('should call getQueryParameterValues on the route service with the same parameters', () => {
-      expect(routeServiceStub.getQueryParameterValues).toHaveBeenCalledWith(mockFilterConfig.paramName);
-    });
-  });
-
   describe('when the getCurrentScope method is called', () => {
     beforeEach(() => {
       spyOn(routeServiceStub, 'getQueryParameterValue');

src/app/core/shared/search/search-filter.service.ts

@@ -136,27 +136,6 @@ export class SearchFilterService {
     return this.routeService.getQueryParameterValue('view');
   }
 
-  /**
-   * Requests the active filter values set for a given filter
-   * @param {SearchFilterConfig} filterConfig The configuration for which the filters are active
-   * @returns {Observable<string[]>} Emits the active filters for the given filter configuration
-   */
-  getSelectedValuesForFilter(filterConfig: SearchFilterConfig): Observable<string[]> {
-    const values$ = this.routeService.getQueryParameterValues(filterConfig.paramName);
-    const prefixValues$ = this.routeService.getQueryParamsWithPrefix(filterConfig.paramName + '.').pipe(
-      map((params: Params) => [].concat(...Object.values(params))),
-    );
-    return observableCombineLatest(values$, prefixValues$).pipe(
-      map(([values, prefixValues]) => {
-          if (isNotEmpty(values)) {
-            return values;
-          }
-          return prefixValues;
-        }
-      )
-    );
-  }
-
   /**
    * Updates the found facet value suggestions for a given query
    * Transforms the found values into display values

src/app/core/shared/search/search.service.ts

@@ -1,7 +1,7 @@
 /* eslint-disable max-classes-per-file */
 import { combineLatest as observableCombineLatest, Observable, BehaviorSubject } from 'rxjs';
-import { Injectable, OnDestroy } from '@angular/core';
-import { map, switchMap, take, tap } from 'rxjs/operators';
+import { Injectable } from '@angular/core';
+import { map, switchMap, take, tap, distinctUntilChanged } from 'rxjs/operators';
 import { FollowLinkConfig } from '../../../shared/utils/follow-link-config.model';
 import { ResponseParsingService } from '../../data/parsing.service';
 import { RemoteData } from '../../data/remote-data';
@@ -61,7 +61,7 @@ class SearchDataService extends BaseDataService<any> {
  * Service that performs all general actions that have to do with the search page
  */
 @Injectable()
-export class SearchService implements OnDestroy {
+export class SearchService {
 
   /**
    * Endpoint link path for retrieving general search results
@@ -78,11 +78,6 @@ export class SearchService implements OnDestroy {
    */
   private request: GenericConstructor<RestRequest> = GetRequest;
 
-  /**
-   * Subscription to unsubscribe from
-   */
-  private sub;
-
   /**
    * Instance of SearchDataService to forward data service methods to
    */
@@ -103,6 +98,18 @@ export class SearchService implements OnDestroy {
     this.searchDataService = new SearchDataService();
   }
 
+  /**
+   * Get the currently {@link AppliedFilter}s for the given filter.
+   *
+   * @param filterName The name of the filter
+   */
+  getSelectedValuesForFilter(filterName: string): Observable<AppliedFilter[]> {
+    return this.appliedFilters$.pipe(
+      map((appliedFilters: AppliedFilter[]) => appliedFilters.filter((appliedFilter: AppliedFilter) => appliedFilter.filter === filterName)),
+      distinctUntilChanged((previous: AppliedFilter[], next: AppliedFilter[]) => JSON.stringify(previous) === JSON.stringify(next)),
+    );
+  }
+
   /**
    * Method to set service options
    * @param {GenericConstructor<ResponseParsingService>} parser The ResponseParsingService constructor name
@@ -176,26 +183,11 @@ export class SearchService implements OnDestroy {
     return this.directlyAttachIndexableObjects(sqr$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow);
   }
 
-  /**
-   * Method to retrieve request entries for search results from the server
-   * @param {PaginatedSearchOptions} searchOptions The configuration necessary to perform this search
-   * @returns {Observable<RemoteData<SearchObjects<T>>>} Emits a paginated list with all search results found
-   */
-  searchEntries<T extends DSpaceObject>(searchOptions?: PaginatedSearchOptions): Observable<RemoteData<SearchObjects<T>>> {
-    const href$ = this.getEndpoint(searchOptions);
-
-    const sqr$ = href$.pipe(
-      switchMap((href: string) => this.rdb.buildFromHref<SearchObjects<T>>(href))
-    );
-
-    return this.directlyAttachIndexableObjects(sqr$);
-  }
-
   /**
    * Method to directly attach the indexableObjects to search results, instead of using RemoteData.
    * For compatibility with the way the search was written originally
    *
-   * @param sqr$:                       a SearchObjects RemotaData Observable without its
+   * @param sqr$                        A {@link SearchObjects} {@link RemoteData} Observable without its
    *                                    indexableObjects attached
    * @param useCachedVersionIfAvailable If this is true, the request will only be sent if there's
    *                                    no valid cached version. Defaults to true
@@ -384,12 +376,4 @@ export class SearchService implements OnDestroy {
     return '/search';
   }
 
-  /**
-   * Unsubscribe from the subscription
-   */
-  ngOnDestroy(): void {
-    if (this.sub !== undefined) {
-      this.sub.unsubscribe();
-    }
-  }
 }

src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts

@@ -3,7 +3,7 @@ import { Component, Inject, OnDestroy, OnInit } from '@angular/core';
 import { Router, Params } from '@angular/router';
 
 import { BehaviorSubject, combineLatest as observableCombineLatest, Observable, of as observableOf, Subscription } from 'rxjs';
-import { debounceTime, distinctUntilChanged, filter, map, mergeMap, switchMap, take, tap } from 'rxjs/operators';
+import { distinctUntilChanged, map, switchMap, take, tap } from 'rxjs/operators';
 
 import { RemoteDataBuildService } from '../../../../../core/cache/builders/remote-data-build.service';
 import { hasNoValue, hasValue } from '../../../../empty.util';
@@ -17,7 +17,6 @@ import { InputSuggestion } from '../../../../input-suggestions/input-suggestions
 import { SearchOptions } from '../../../models/search-options.model';
 import { SEARCH_CONFIG_SERVICE } from '../../../../../my-dspace-page/my-dspace-page.component';
 import { currentPath } from '../../../../utils/route.utils';
-import { stripOperatorFromFilterValue } from '../../../search.utils';
 import { FacetValues } from '../../../models/facet-values.model';
 import { AppliedFilter } from '../../../models/applied-filter.model';
 
@@ -103,14 +102,9 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy {
     this.searchOptions$ = this.searchConfigService.searchOptions;
     this.subs.push(
       this.searchOptions$.subscribe(() => this.updateFilterValueList()),
-      this.refreshFilters.asObservable().pipe(
-        filter((toRefresh: boolean) => toRefresh),
-        // NOTE This is a workaround, otherwise retrieving filter values returns tha old cached response
-        debounceTime((100)),
-        mergeMap(() => this.retrieveFilterValues(false))
-      ).subscribe()
+      this.retrieveFilterValues().subscribe(),
     );
-    this.retrieveFilterValues().subscribe();
+    this.selectedAppliedFilters$ = this.searchService.getSelectedValuesForFilter(this.filterConfig.name);
   }
 
   /**
@@ -217,9 +211,13 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy {
     }
   }
 
-  protected retrieveFilterValues(useCachedVersionIfAvailable = true): Observable<FacetValues[]> {
+  /**
+   * Retrieves all the filter value suggestion pages that need to be displayed in the facet and combines it into one
+   * list.
+   */
+  protected retrieveFilterValues(): Observable<FacetValues[]> {
     return observableCombineLatest([this.searchOptions$, this.currentPage]).pipe(
-      switchMap(([options, page]: [SearchOptions, number]) => this.searchService.getFacetValuesFor(this.filterConfig, page, options, null, useCachedVersionIfAvailable).pipe(
+      switchMap(([options, page]: [SearchOptions, number]) => this.searchService.getFacetValuesFor(this.filterConfig, page, options).pipe(
         getFirstSucceededRemoteDataPayload(),
         tap((facetValues: FacetValues) => {
           this.isLastPage$.next(hasNoValue(facetValues?.next));
@@ -242,27 +240,12 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy {
         return filterValues;
       }),
       tap((allFacetValues: FacetValues[]) => {
-        this.setAppliedFilter(allFacetValues);
         this.animationState = 'ready';
         this.facetValues$.next(allFacetValues);
       })
     );
   }
 
-  setAppliedFilter(allFacetValues: FacetValues[]): void {
-    const allAppliedFilters: AppliedFilter[] = [].concat(...allFacetValues.map((facetValues: FacetValues) => facetValues.appliedFilters))
-      .filter((appliedFilter: AppliedFilter) => hasValue(appliedFilter));
-
-    this.selectedAppliedFilters$ = this.filterService.getSelectedValuesForFilter(this.filterConfig).pipe(
-      map((selectedValues: string[]) => {
-        const appliedFilters: AppliedFilter[] = selectedValues.map((value: string) => {
-          return allAppliedFilters.find((appliedFilter: AppliedFilter) => appliedFilter.value === stripOperatorFromFilterValue(value));
-        }).filter((appliedFilter: AppliedFilter) => hasValue(appliedFilter));
-        return appliedFilters;
-      }),
-    );
-  }
-
   /**
    * Prevent unnecessary rerendering
    */

src/app/shared/search/search-filters/search-filter/search-filter.component.spec.ts

@@ -1,4 +1,4 @@
-import { ChangeDetectionStrategy, NO_ERRORS_SCHEMA } from '@angular/core';
+import { ChangeDetectionStrategy, CUSTOM_ELEMENTS_SCHEMA } from '@angular/core';
 
 import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
 import { RouterTestingModule } from '@angular/router/testing';
@@ -14,51 +14,28 @@ import { SearchConfigurationServiceStub } from '../../../testing/search-configur
 import { SEARCH_CONFIG_SERVICE } from '../../../../my-dspace-page/my-dspace-page.component';
 import { SequenceService } from '../../../../core/shared/sequence.service';
 import { BrowserOnlyMockPipe } from '../../../testing/browser-only-mock.pipe';
+import { SearchServiceStub } from '../../../testing/search-service.stub';
+import { SearchFilterServiceStub } from '../../../testing/search-filter-service.stub';
 
 describe('SearchFilterComponent', () => {
   let comp: SearchFilterComponent;
   let fixture: ComponentFixture<SearchFilterComponent>;
   const filterName1 = 'test name';
-  const filterName2 = 'test2';
-  const filterName3 = 'another name3';
-  const nonExistingFilter1 = 'non existing 1';
-  const nonExistingFilter2 = 'non existing 2';
+
   const mockFilterConfig: SearchFilterConfig = Object.assign(new SearchFilterConfig(), {
     name: filterName1,
     filterType: FilterType.text,
     hasFacets: false,
     isOpenByDefault: false
   });
-  const mockFilterService = {
-    /* eslint-disable no-empty,@typescript-eslint/no-empty-function */
-    toggle: (filter) => {
-    },
-    collapse: (filter) => {
-    },
-    expand: (filter) => {
-    },
-    initializeFilter: (filter) => {
-    },
-    getSelectedValuesForFilter: (filter) => {
-      return observableOf([filterName1, filterName2, filterName3]);
-    },
-    isFilterActive: (filter) => {
-      return observableOf([filterName1, filterName2, filterName3].indexOf(filter) >= 0);
-    },
-    isCollapsed: (filter) => {
-      return observableOf(true);
-    }
-    /* eslint-enable no-empty, @typescript-eslint/no-empty-function */
-
-  };
-  let filterService;
+  let searchFilterService: SearchFilterServiceStub;
   let sequenceService;
   const mockResults = observableOf(['test', 'data']);
-  const searchServiceStub = {
-    getFacetValuesFor: (filter) => mockResults
-  };
+  let searchService: SearchServiceStub;
 
   beforeEach(waitForAsync(() => {
+    searchFilterService = new SearchFilterServiceStub();
+    searchService = new SearchServiceStub();
     sequenceService = jasmine.createSpyObj('sequenceService', { next: 17 });
 
     TestBed.configureTestingModule({
@@ -68,26 +45,23 @@ describe('SearchFilterComponent', () => {
         BrowserOnlyMockPipe,
       ],
       providers: [
-        { provide: SearchService, useValue: searchServiceStub },
-        {
-          provide: SearchFilterService,
-          useValue: mockFilterService
-        },
+        { provide: SearchService, useValue: searchService },
+        { provide: SearchFilterService, useValue: searchFilterService },
         { provide: SEARCH_CONFIG_SERVICE, useValue: new SearchConfigurationServiceStub() },
         { provide: SequenceService, useValue: sequenceService },
       ],
-      schemas: [NO_ERRORS_SCHEMA]
+      schemas: [CUSTOM_ELEMENTS_SCHEMA],
     }).overrideComponent(SearchFilterComponent, {
       set: { changeDetection: ChangeDetectionStrategy.Default }
     }).compileComponents();
   }));
 
   beforeEach(() => {
+    spyOn(searchService, 'getFacetValuesFor').and.returnValue(mockResults);
     fixture = TestBed.createComponent(SearchFilterComponent);
     comp = fixture.componentInstance; // SearchPageComponent test instance
     comp.filter = mockFilterConfig;
     fixture.detectChanges();
-    filterService = (comp as any).filterService;
   });
 
   it('should generate unique IDs', () => {
@@ -98,54 +72,30 @@ describe('SearchFilterComponent', () => {
 
   describe('when the toggle method is triggered', () => {
     beforeEach(() => {
-      spyOn(filterService, 'toggle');
+      spyOn(searchFilterService, 'toggle');
       comp.toggle();
     });
 
     it('should call toggle with the correct filter configuration name', () => {
-      expect(filterService.toggle).toHaveBeenCalledWith(mockFilterConfig.name);
+      expect(searchFilterService.toggle).toHaveBeenCalledWith(mockFilterConfig.name);
     });
   });
 
   describe('when the initializeFilter method is triggered', () => {
     beforeEach(() => {
-      spyOn(filterService, 'initializeFilter');
+      spyOn(searchFilterService, 'initializeFilter');
       comp.initializeFilter();
     });
 
     it('should call initialCollapse with the correct filter configuration name', () => {
-      expect(filterService.initializeFilter).toHaveBeenCalledWith(mockFilterConfig);
-    });
-  });
-
-  describe('when getSelectedValues is called', () => {
-    let valuesObservable: Observable<string[]>;
-    beforeEach(() => {
-      valuesObservable = (comp as any).getSelectedValues();
-    });
-
-    it('should return an observable containing the existing filters', () => {
-      const sub = valuesObservable.subscribe((values) => {
-        expect(values).toContain(filterName1);
-        expect(values).toContain(filterName2);
-        expect(values).toContain(filterName3);
-      });
-      sub.unsubscribe();
-    });
-
-    it('should return an observable that does not contain the non-existing filters', () => {
-      const sub = valuesObservable.subscribe((values) => {
-        expect(values).not.toContain(nonExistingFilter1);
-        expect(values).not.toContain(nonExistingFilter2);
-      });
-      sub.unsubscribe();
+      expect(searchFilterService.initializeFilter).toHaveBeenCalledWith(mockFilterConfig);
     });
   });
 
   describe('when isCollapsed is called and the filter is collapsed', () => {
     let isActive: Observable<boolean>;
     beforeEach(() => {
-      filterService.isCollapsed = () => observableOf(true);
+      searchFilterService.isCollapsed = () => observableOf(true);
       isActive = (comp as any).isCollapsed();
     });
 
@@ -160,7 +110,7 @@ describe('SearchFilterComponent', () => {
   describe('when isCollapsed is called and the filter is not collapsed', () => {
     let isActive: Observable<boolean>;
     beforeEach(() => {
-      filterService.isCollapsed = () => observableOf(false);
+      searchFilterService.isCollapsed = () => observableOf(false);
       isActive = (comp as any).isCollapsed();
     });