From 6f9e8bd3bf4a7606e9ac8523748833f5efb1bda3 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Tue, 17 Nov 2020 13:15:24 +0100 Subject: [PATCH 1/4] 74606: Ensure the correct operators are used for filter queries --- .../search-authority-filter.component.ts | 18 -------- .../search-facet-option.component.ts | 17 ++----- ...earch-facet-selected-option.component.html | 2 +- .../search-facet-selected-option.component.ts | 15 +------ .../search-facet-filter.component.ts | 5 ++- .../search-text-filter.component.ts | 21 +++++++++ .../search-labels/search-labels.component.ts | 13 +++++- src/app/shared/search/search.utils.ts | 44 +++++++++++++++++++ 8 files changed, 86 insertions(+), 49 deletions(-) create mode 100644 src/app/shared/search/search.utils.ts diff --git a/src/app/shared/search/search-filters/search-filter/search-authority-filter/search-authority-filter.component.ts b/src/app/shared/search/search-filters/search-filter/search-authority-filter/search-authority-filter.component.ts index 5dc930f67f..7a20fd5ff0 100644 --- a/src/app/shared/search/search-filters/search-filter/search-authority-filter/search-authority-filter.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-authority-filter/search-authority-filter.component.ts @@ -1,9 +1,7 @@ import { Component, OnInit } from '@angular/core'; - import { FilterType } from '../../../filter-type.model'; import { facetLoad, SearchFacetFilterComponent } from '../search-facet-filter/search-facet-filter.component'; import { renderFacetFor } from '../search-filter-type-decorator'; -import { FacetValue } from '../../../facet-value.model'; @Component({ selector: 'ds-search-authority-filter', @@ -17,20 +15,4 @@ import { FacetValue } from '../../../facet-value.model'; */ @renderFacetFor(FilterType.authority) export class SearchAuthorityFilterComponent extends SearchFacetFilterComponent implements OnInit { - - /** - * TODO to review after https://github.com/DSpace/dspace-angular/issues/368 is resolved - * Retrieve facet value from search link - */ - protected getFacetValue(facet: FacetValue): string { - const search = facet._links.search.href; - const hashes = search.slice(search.indexOf('?') + 1).split('&'); - const params = {}; - hashes.map((hash) => { - const [key, val] = hash.split('='); - params[key] = decodeURIComponent(val) - }); - - return params[this.filterConfig.paramName]; - } } diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts index 04e810edda..dac98e7627 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts @@ -10,6 +10,7 @@ import { SearchConfigurationService } from '../../../../../../core/shared/search import { hasValue } from '../../../../../empty.util'; import { FilterType } from '../../../../filter-type.model'; import { currentPath } from '../../../../../utils/route.utils'; +import { getFacetValueForType } from '../../../../search.utils'; @Component({ selector: 'ds-search-facet-option', @@ -102,7 +103,7 @@ export class SearchFacetOptionComponent implements OnInit, OnDestroy { */ private updateAddParams(selectedValues: FacetValue[]): void { this.addQueryParams = { - [this.filterConfig.paramName]: [...selectedValues.map((facetValue: FacetValue) => facetValue.label), this.getFacetValue()], + [this.filterConfig.paramName]: [...selectedValues.map((facetValue: FacetValue) => getFacetValueForType(facetValue, this.filterConfig)), this.getFacetValue()], page: 1 }; } @@ -112,19 +113,7 @@ export class SearchFacetOptionComponent implements OnInit, OnDestroy { * Retrieve facet value related to facet type */ private getFacetValue(): string { - if (this.filterConfig.type === FilterType.authority) { - const search = this.filterValue._links.search.href; - const hashes = search.slice(search.indexOf('?') + 1).split('&'); - const params = {}; - hashes.map((hash) => { - const [key, val] = hash.split('='); - params[key] = decodeURIComponent(val) - }); - - return params[this.filterConfig.paramName]; - } else { - return this.filterValue.value; - } + return getFacetValueForType(this.filterValue, this.filterConfig); } /** diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.html b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.html index a27a5d3d86..4bcfc02966 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.html +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.html @@ -3,6 +3,6 @@ [queryParams]="removeQueryParams" queryParamsHandling="merge"> - {{ 'search.filters.' + filterConfig.name + '.' + selectedValue.value | translate: {default: selectedValue.value} }} + {{ 'search.filters.' + filterConfig.name + '.' + selectedValue.value | translate: {default: selectedValue.label} }} diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts index f58a903b0c..e54b228f7f 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts @@ -9,6 +9,7 @@ import { SearchConfigurationService } from '../../../../../../core/shared/search import { FacetValue } from '../../../../facet-value.model'; import { FilterType } from '../../../../filter-type.model'; import { currentPath } from '../../../../../utils/route.utils'; +import { getFacetValueForType } from '../../../../search.utils'; @Component({ selector: 'ds-search-facet-selected-option', @@ -101,19 +102,7 @@ export class SearchFacetSelectedOptionComponent implements OnInit, OnDestroy { * Retrieve facet value related to facet type */ private getFacetValue(facetValue: FacetValue): string { - if (this.filterConfig.type === FilterType.authority) { - const search = facetValue._links.search.href; - const hashes = search.slice(search.indexOf('?') + 1).split('&'); - const params = {}; - hashes.map((hash) => { - const [key, val] = hash.split('='); - params[key] = decodeURIComponent(val) - }); - - return params[this.filterConfig.paramName]; - } else { - return facetValue.value; - } + return getFacetValueForType(facetValue, this.filterConfig); } /** diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts index df0c53f543..af4a92781d 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts @@ -25,6 +25,7 @@ import { InputSuggestion } from '../../../../input-suggestions/input-suggestions import { SearchOptions } from '../../../search-options.model'; import { SEARCH_CONFIG_SERVICE } from '../../../../../+my-dspace-page/my-dspace-page.component'; import { currentPath } from '../../../../utils/route.utils'; +import { getFacetValueForType, stripOperatorFromFilterValue } from '../../../search.utils'; @Component({ selector: 'ds-search-facet-filter', @@ -148,7 +149,7 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy { if (hasValue(fValue)) { return fValue; } - return Object.assign(new FacetValue(), { label: value, value: value }); + return Object.assign(new FacetValue(), { label: stripOperatorFromFilterValue(value), value: value }); }); }) ); @@ -303,7 +304,7 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy { * Retrieve facet value */ protected getFacetValue(facet: FacetValue): string { - return facet.value; + return getFacetValueForType(facet, this.filterConfig); } /** diff --git a/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts b/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts index 678cbbfdd5..379c73157f 100644 --- a/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts @@ -5,6 +5,10 @@ import { SearchFacetFilterComponent } from '../search-facet-filter/search-facet-filter.component'; import { renderFacetFor } from '../search-filter-type-decorator'; +import { + addOperatorToFilterValue, + stripOperatorFromFilterValue +} from '../../../search.utils'; /** * This component renders a simple item page. @@ -24,4 +28,21 @@ import { renderFacetFor } from '../search-filter-type-decorator'; */ @renderFacetFor(FilterType.text) export class SearchTextFilterComponent extends SearchFacetFilterComponent implements OnInit { + /** + * Submits a new active custom value to the filter from the input field + * Overwritten method from parent component, adds the "query" operator to the received data before passing it on + * @param data The string from the input field + */ + onSubmit(data: any) { + super.onSubmit(addOperatorToFilterValue(data, 'query')); + } + + /** + * On click, set the input's value to the clicked data + * Overwritten method from parent component, strips any operator from the received data before passing it on + * @param data The value of the option that was clicked + */ + onClick(data: any) { + super.onClick(stripOperatorFromFilterValue(data)); + } } diff --git a/src/app/shared/search/search-labels/search-labels.component.ts b/src/app/shared/search/search-labels/search-labels.component.ts index 154b888269..2322c80acf 100644 --- a/src/app/shared/search/search-labels/search-labels.component.ts +++ b/src/app/shared/search/search-labels/search-labels.component.ts @@ -3,6 +3,8 @@ import { SEARCH_CONFIG_SERVICE } from '../../../+my-dspace-page/my-dspace-page.c import { Observable } from 'rxjs'; import { Params, Router } from '@angular/router'; import { SearchConfigurationService } from '../../../core/shared/search/search-configuration.service'; +import { map } from 'rxjs/operators'; +import { stripOperatorFromFilterValue } from '../search.utils'; @Component({ selector: 'ds-search-labels', @@ -30,6 +32,15 @@ export class SearchLabelsComponent { constructor( protected router: Router, @Inject(SEARCH_CONFIG_SERVICE) public searchConfigService: SearchConfigurationService) { - this.appliedFilters = this.searchConfigService.getCurrentFrontendFilters(); + this.appliedFilters = this.searchConfigService.getCurrentFrontendFilters().pipe( + map((params) => { + const labels = {}; + Object.keys(params) + .forEach((key) => { + labels[key] = [...params[key].map((value) => stripOperatorFromFilterValue(value))]; + }); + return labels; + }) + ); } } diff --git a/src/app/shared/search/search.utils.ts b/src/app/shared/search/search.utils.ts new file mode 100644 index 0000000000..05a9711435 --- /dev/null +++ b/src/app/shared/search/search.utils.ts @@ -0,0 +1,44 @@ +import { FacetValue } from './facet-value.model'; +import { SearchFilterConfig } from './search-filter-config.model'; +import { isNotEmpty } from '../empty.util'; + +/** + * Get a facet's value by matching its parameter in the search href, this will include the operator of the facet value + * If the {@link FacetValue} doesn't contain a search link, its raw value will be returned as a fallback + * @param facetValue + * @param searchFilterConfig + */ +export function getFacetValueForType(facetValue: FacetValue, searchFilterConfig: SearchFilterConfig): string { + const regex = new RegExp(`[?|&]${searchFilterConfig.paramName}=(${facetValue.value}[^&]*)`, 'g'); + if (isNotEmpty(facetValue._links)) { + const values = regex.exec(facetValue._links.search.href); + if (isNotEmpty(values)) { + return values[1]; + } + } + return addOperatorToFilterValue(facetValue.value, 'equals'); +} + +/** + * Strip the operator from a filter value + * Warning: This expects the value to end with an operator, otherwise it might strip unwanted content + * @param value + */ +export function stripOperatorFromFilterValue(value: string) { + if (value.lastIndexOf(',') > -1) { + return value.substring(0, value.lastIndexOf(',')); + } + return value; +} + +/** + * Add an operator to a string + * @param value + * @param operator + */ +export function addOperatorToFilterValue(value: string, operator: string) { + if (!value.endsWith(`,${operator}`)) { + return `${value},${operator}`; + } + return value; +} From 05406e6919b2d55ec1e061ef84d2836d64de31a9 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Tue, 17 Nov 2020 15:43:05 +0100 Subject: [PATCH 2/4] 74606: Search Utils test cases + test fixes --- .../search-facet-option.component.spec.ts | 4 +- .../search-facet-filter.component.spec.ts | 2 +- src/app/shared/search/search.utils.spec.ts | 52 +++++++++++++++++++ 3 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 src/app/shared/search/search.utils.spec.ts diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.spec.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.spec.ts index 4f85411d85..ba6eceeede 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.spec.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.spec.ts @@ -130,7 +130,7 @@ describe('SearchFacetOptionComponent', () => { comp.addQueryParams = {}; (comp as any).updateAddParams(selectedValues); expect(comp.addQueryParams).toEqual({ - [mockFilterConfig.paramName]: [value1, value.value], + [mockFilterConfig.paramName]: [`${value1},${operator}`, value.value + ',equals'], page: 1 }); }); @@ -145,7 +145,7 @@ describe('SearchFacetOptionComponent', () => { comp.addQueryParams = {}; (comp as any).updateAddParams(selectedValues); expect(comp.addQueryParams).toEqual({ - [mockAuthorityFilterConfig.paramName]: [value1, `${value2},${operator}`], + [mockAuthorityFilterConfig.paramName]: [value1 + ',equals', `${value2},${operator}`], page: 1 }); }); diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.spec.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.spec.ts index 376aa7ba3a..54548b0ddf 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.spec.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.spec.ts @@ -208,7 +208,7 @@ describe('SearchFacetFilterComponent', () => { it('should call navigate on the router with the right searchlink and parameters', () => { expect(router.navigate).toHaveBeenCalledWith(searchUrl.split('/'), { - queryParams: { [mockFilterConfig.paramName]: [...selectedValues, testValue] }, + queryParams: { [mockFilterConfig.paramName]: [...selectedValues.map((value) => `${value},equals`), testValue] }, queryParamsHandling: 'merge' }); }); diff --git a/src/app/shared/search/search.utils.spec.ts b/src/app/shared/search/search.utils.spec.ts new file mode 100644 index 0000000000..a14085c2d3 --- /dev/null +++ b/src/app/shared/search/search.utils.spec.ts @@ -0,0 +1,52 @@ +import { FacetValue } from './facet-value.model'; +import { SearchFilterConfig } from './search-filter-config.model'; +import { addOperatorToFilterValue, getFacetValueForType, stripOperatorFromFilterValue } from './search.utils'; + +describe('Search Utils', () => { + describe('getFacetValueForType', () => { + let facetValueWithSearchHref: FacetValue; + let facetValueWithoutSearchHref: FacetValue; + let searchFilterConfig: SearchFilterConfig; + + beforeEach(() => { + facetValueWithSearchHref = Object.assign(new FacetValue(), { + value: 'Value with search href', + _links: { + search: { + href: 'rest/api/search?f.otherFacet=Other facet value,operator&f.facetName=Value with search href,operator' + } + } + }); + facetValueWithoutSearchHref = Object.assign(new FacetValue(), { + value: 'Value without search href' + }); + searchFilterConfig = Object.assign(new SearchFilterConfig(), { + name: 'facetName' + }); + }); + + it('should retrieve the correct value from the search href', () => { + expect(getFacetValueForType(facetValueWithSearchHref, searchFilterConfig)).toEqual('Value with search href,operator'); + }); + + it('should return the facet value with an equals operator by default', () => { + expect(getFacetValueForType(facetValueWithoutSearchHref, searchFilterConfig)).toEqual('Value without search href,equals'); + }); + }); + + describe('stripOperatorFromFilterValue', () => { + it('should strip the operator from the value', () => { + expect(stripOperatorFromFilterValue('value,operator')).toEqual('value'); + }); + }); + + describe('addOperatorToFilterValue', () => { + it('should add the operator to the value', () => { + expect(addOperatorToFilterValue('value', 'operator')).toEqual('value,operator'); + }); + + it('shouldn\'t add the operator to the value if it already contains the operator', () => { + expect(addOperatorToFilterValue('value,operator', 'operator')).toEqual('value,operator'); + }); + }); +}); From cb6381f7ee9e4f835b16a207c06e92c4adaa20a5 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Wed, 18 Nov 2020 17:59:22 +0100 Subject: [PATCH 3/4] 74606: Move stripping of suggestions to SearchFacetFilter --- .../search-facet-filter.component.ts | 2 +- .../search-text-filter/search-text-filter.component.ts | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts index af4a92781d..9ff8663a65 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts @@ -288,7 +288,7 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy { return rd.payload.page.map((facet) => { return { displayValue: this.getDisplayValue(facet, data), - value: this.getFacetValue(facet) + value: stripOperatorFromFilterValue(this.getFacetValue(facet)) } }) } diff --git a/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts b/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts index 379c73157f..b2b44ce219 100644 --- a/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-text-filter/search-text-filter.component.ts @@ -7,7 +7,6 @@ import { import { renderFacetFor } from '../search-filter-type-decorator'; import { addOperatorToFilterValue, - stripOperatorFromFilterValue } from '../../../search.utils'; /** @@ -36,13 +35,4 @@ export class SearchTextFilterComponent extends SearchFacetFilterComponent implem onSubmit(data: any) { super.onSubmit(addOperatorToFilterValue(data, 'query')); } - - /** - * On click, set the input's value to the clicked data - * Overwritten method from parent component, strips any operator from the received data before passing it on - * @param data The value of the option that was clicked - */ - onClick(data: any) { - super.onClick(stripOperatorFromFilterValue(data)); - } } From 67de6538e5fae61a607fa5077de9d6bfa50cc642 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Tue, 1 Dec 2020 14:47:03 +0100 Subject: [PATCH 4/4] 74606: LGTM issue fixes --- .../search-facet-option/search-facet-option.component.ts | 1 - .../search-facet-selected-option.component.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts index dac98e7627..7148682b0e 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-option/search-facet-option.component.ts @@ -8,7 +8,6 @@ import { SearchService } from '../../../../../../core/shared/search/search.servi import { SearchFilterService } from '../../../../../../core/shared/search/search-filter.service'; import { SearchConfigurationService } from '../../../../../../core/shared/search/search-configuration.service'; import { hasValue } from '../../../../../empty.util'; -import { FilterType } from '../../../../filter-type.model'; import { currentPath } from '../../../../../utils/route.utils'; import { getFacetValueForType } from '../../../../search.utils'; diff --git a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts index e54b228f7f..4fd9a62bf6 100644 --- a/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts +++ b/src/app/shared/search/search-filters/search-filter/search-facet-filter-options/search-facet-selected-option/search-facet-selected-option.component.ts @@ -7,7 +7,6 @@ import { SearchFilterService } from '../../../../../../core/shared/search/search import { hasValue } from '../../../../../empty.util'; import { SearchConfigurationService } from '../../../../../../core/shared/search/search-configuration.service'; import { FacetValue } from '../../../../facet-value.model'; -import { FilterType } from '../../../../filter-type.model'; import { currentPath } from '../../../../../utils/route.utils'; import { getFacetValueForType } from '../../../../search.utils';