From 4b7ac81b7a21b11114694fba6b5a50fbeb260ff9 Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Thu, 23 Jan 2025 08:37:07 -0500 Subject: [PATCH 1/4] Create test to capture checkbox duplication bug. --- .../src/VuFindTest/Mink/SearchFacetsTest.php | 46 +++++++++++++------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php b/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php index d4b40c76be5..773e9ec788f 100644 --- a/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php +++ b/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php @@ -1381,32 +1381,37 @@ protected function assertNoResetFiltersButton(Element $page): void public static function multiSelectOnAdvancedSearchProvider(): array { return [ - 'with language switch' => [true], - 'without language switch' => [false], + 'with language switch / with checkbox' => [true, true], + 'without language switch / with checkbox' => [false, true], + 'with language switch / without checkbox' => [true, false], + 'without language switch / without checkbox' => [false, false], ]; } /** * Test applying multi-facet selection to advanced search results, with or without changing the - * language setting first. + * language setting first and/or including a pre-existing checkbox filter. * - * @param bool $changeLanguage Should we change the language before applying the facets? + * @param bool $changeLanguage Should we change the language before applying the facets? + * @param bool $includeCheckbox Should we apply a checkbox prior to multi-selection? * * @dataProvider multiSelectOnAdvancedSearchProvider * * @return void */ - public function testMultiSelectOnAdvancedSearch(bool $changeLanguage): void + public function testMultiSelectOnAdvancedSearch(bool $changeLanguage, bool $includeCheckbox): void { - $this->changeConfigs( - [ - 'facets' => [ - 'Results_Settings' => [ - 'multiFacetsSelection' => true, - ], - ], - ] - ); + $facets = [ + 'Results_Settings' => [ + 'multiFacetsSelection' => true, + ], + ]; + if ($includeCheckbox) { + // Create a pointless checkbox filter that will not impact the result set size + // (we're just testing that it applies to the URL correctly): + $facets['CheckboxFacets']['title:*'] = 'Has Title'; + } + $this->changeConfigs(compact('facets')); $path = '/Search/Advanced'; $session = $this->getMinkSession(); $session->visit($this->getVuFindUrl() . $path); @@ -1416,11 +1421,17 @@ public function testMultiSelectOnAdvancedSearch(bool $changeLanguage): void $this->findCssAndSetValue($page, '#search_lookfor0_1', 'history'); $this->findCss($page, '[type=submit]')->press(); + if ($includeCheckbox) { + $link = $this->findAndAssertLink($page, 'Has Title'); + $link->click(); + $this->waitForPageLoad($page); + } + if ($changeLanguage) { $this->flipflopLanguage($page); } - // Activate the first two facet values: + // Activate the first two facet values (and the checkbox filter, if requested): $this->clickCss($page, '.js-user-selection-multi-filters'); $this->clickCss($page, '.facet__list__item a'); $this->clickCss($page, '.facet__list__item a', index: 1); @@ -1433,7 +1444,12 @@ public function testMultiSelectOnAdvancedSearch(bool $changeLanguage): void $this->findCssAndGetText($page, '.adv_search_terms strong') ); + // Make sure we have the expected number of filters applied: $this->assertCount(2, $page->findAll('css', '.facet.active')); + $query = parse_url($session->getCurrentUrl(), PHP_URL_QUERY); + parse_str($query, $queryArray); + $expectedFilterCount = $includeCheckbox ? 3 : 2; + $this->assertCount($expectedFilterCount, $queryArray['filter']); // If configured, flip-flop language again to potentially modify filter params: if ($changeLanguage) { From 56e11a747ef1fd53775022413b1cd85dc42de398 Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Thu, 23 Jan 2025 08:45:19 -0500 Subject: [PATCH 2/4] More consistent normalization. --- themes/bootstrap3/js/facets.js | 9 ++++++--- themes/bootstrap5/js/facets.js | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/themes/bootstrap3/js/facets.js b/themes/bootstrap3/js/facets.js index e64198a4d6c..aac9d208221 100644 --- a/themes/bootstrap3/js/facets.js +++ b/themes/bootstrap3/js/facets.js @@ -194,7 +194,8 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { for (const [key, value] of (new URLSearchParams(window.location.search))) { - initialParams.append(normalizeSearchQueryKey(key), normalizeValue(key, value)); + const normalizedKey = normalizeSearchQueryKey(key); + initialParams.append(normalizedKey, normalizeValue(normalizedKey, value)); } /** @@ -255,13 +256,15 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { for (const [key, value] of elemParams) { // URLSearchParams.has(key, value) seems to be broken on iOS 16, so check with our own method: if (!VuFind.inURLSearchParams(initialParams, key, value)) { - globalAddedParams.append(normalizeSearchQueryKey(key), value); + const normalizedKey = normalizeSearchQueryKey(key); + globalAddedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); } } // Remove parameters that this URL no longer has: for (const [key, value] of initialParams) { if (!VuFind.inURLSearchParams(elemParams, key, value)) { - globalRemovedParams.append(normalizeSearchQueryKey(key), value); + const normalizedKey = normalizeSearchQueryKey(key); + globalRemovedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); } } } diff --git a/themes/bootstrap5/js/facets.js b/themes/bootstrap5/js/facets.js index e64198a4d6c..aac9d208221 100644 --- a/themes/bootstrap5/js/facets.js +++ b/themes/bootstrap5/js/facets.js @@ -194,7 +194,8 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { for (const [key, value] of (new URLSearchParams(window.location.search))) { - initialParams.append(normalizeSearchQueryKey(key), normalizeValue(key, value)); + const normalizedKey = normalizeSearchQueryKey(key); + initialParams.append(normalizedKey, normalizeValue(normalizedKey, value)); } /** @@ -255,13 +256,15 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { for (const [key, value] of elemParams) { // URLSearchParams.has(key, value) seems to be broken on iOS 16, so check with our own method: if (!VuFind.inURLSearchParams(initialParams, key, value)) { - globalAddedParams.append(normalizeSearchQueryKey(key), value); + const normalizedKey = normalizeSearchQueryKey(key); + globalAddedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); } } // Remove parameters that this URL no longer has: for (const [key, value] of initialParams) { if (!VuFind.inURLSearchParams(elemParams, key, value)) { - globalRemovedParams.append(normalizeSearchQueryKey(key), value); + const normalizedKey = normalizeSearchQueryKey(key); + globalRemovedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); } } } From 26e2a3a6f022d3bbbc29b0745032ef45a6f3b423 Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Thu, 23 Jan 2025 08:50:20 -0500 Subject: [PATCH 3/4] Improve test. --- .../integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php b/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php index 773e9ec788f..8890d9b4966 100644 --- a/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php +++ b/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php @@ -1444,8 +1444,9 @@ public function testMultiSelectOnAdvancedSearch(bool $changeLanguage, bool $incl $this->findCssAndGetText($page, '.adv_search_terms strong') ); - // Make sure we have the expected number of filters applied: + // Make sure we have the expected number of filters applied on screen and in the URL query: $this->assertCount(2, $page->findAll('css', '.facet.active')); + $this->assertCount($includeCheckbox ? 1 : 0, $page->findAll('css', '.checkbox-filter [data-checked="true"]')); $query = parse_url($session->getCurrentUrl(), PHP_URL_QUERY); parse_str($query, $queryArray); $expectedFilterCount = $includeCheckbox ? 3 : 2; From cb2bfc0c7279edfe93ee56a15b1340b669fa1947 Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Thu, 23 Jan 2025 09:02:31 -0500 Subject: [PATCH 4/4] Refactor for clarity. --- themes/bootstrap3/js/facets.js | 30 +++++++++++++++++++----------- themes/bootstrap5/js/facets.js | 30 +++++++++++++++++++----------- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/themes/bootstrap3/js/facets.js b/themes/bootstrap3/js/facets.js index aac9d208221..2a474596301 100644 --- a/themes/bootstrap3/js/facets.js +++ b/themes/bootstrap3/js/facets.js @@ -178,24 +178,34 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { } /** - * Normalize key names in a set of search parameters + * Append a normalized value to a normalized key in a set of parameters + * + * @param {URLSearchParams} params Parameters to update + * @param {string} key Key name + * @param {string} value Value to set + */ + function appendNormalizedValue(params, key, value) { + const normalizedKey = normalizeSearchQueryKey(key); + params.append(normalizedKey, normalizeValue(normalizedKey, value)); + } + + /** + * Normalize keys and values in a set of search parameters * * @param {URLSearchParams} params Parameters to normalize * * @returns URLSearchParams */ - function normalizeSearchQueryKeys(params) { + function normalizeSearchQueryKeysAndValues(params) { const normalized = new URLSearchParams(); for (const [key, value] of params) { - normalized.append(normalizeSearchQueryKey(key), value); + appendNormalizedValue(normalized, key, value); } return normalized; } - for (const [key, value] of (new URLSearchParams(window.location.search))) { - const normalizedKey = normalizeSearchQueryKey(key); - initialParams.append(normalizedKey, normalizeValue(normalizedKey, value)); + appendNormalizedValue(initialParams, key, value); } /** @@ -250,21 +260,19 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { for (const elem of elems) { const href = elem.getAttribute('href'); const p = href.indexOf('?'); - const elemParams = normalizeSearchQueryKeys(new URLSearchParams(p >= 0 ? href.substring(p + 1) : '')); + const elemParams = normalizeSearchQueryKeysAndValues(new URLSearchParams(p >= 0 ? href.substring(p + 1) : '')); // Add parameters that did not initially exist: for (const [key, value] of elemParams) { // URLSearchParams.has(key, value) seems to be broken on iOS 16, so check with our own method: if (!VuFind.inURLSearchParams(initialParams, key, value)) { - const normalizedKey = normalizeSearchQueryKey(key); - globalAddedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); + appendNormalizedValue(globalAddedParams, key, value); } } // Remove parameters that this URL no longer has: for (const [key, value] of initialParams) { if (!VuFind.inURLSearchParams(elemParams, key, value)) { - const normalizedKey = normalizeSearchQueryKey(key); - globalRemovedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); + appendNormalizedValue(globalRemovedParams, key, value); } } } diff --git a/themes/bootstrap5/js/facets.js b/themes/bootstrap5/js/facets.js index aac9d208221..2a474596301 100644 --- a/themes/bootstrap5/js/facets.js +++ b/themes/bootstrap5/js/facets.js @@ -178,24 +178,34 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { } /** - * Normalize key names in a set of search parameters + * Append a normalized value to a normalized key in a set of parameters + * + * @param {URLSearchParams} params Parameters to update + * @param {string} key Key name + * @param {string} value Value to set + */ + function appendNormalizedValue(params, key, value) { + const normalizedKey = normalizeSearchQueryKey(key); + params.append(normalizedKey, normalizeValue(normalizedKey, value)); + } + + /** + * Normalize keys and values in a set of search parameters * * @param {URLSearchParams} params Parameters to normalize * * @returns URLSearchParams */ - function normalizeSearchQueryKeys(params) { + function normalizeSearchQueryKeysAndValues(params) { const normalized = new URLSearchParams(); for (const [key, value] of params) { - normalized.append(normalizeSearchQueryKey(key), value); + appendNormalizedValue(normalized, key, value); } return normalized; } - for (const [key, value] of (new URLSearchParams(window.location.search))) { - const normalizedKey = normalizeSearchQueryKey(key); - initialParams.append(normalizedKey, normalizeValue(normalizedKey, value)); + appendNormalizedValue(initialParams, key, value); } /** @@ -250,21 +260,19 @@ VuFind.register('multiFacetsSelection', function multiFacetsSelection() { for (const elem of elems) { const href = elem.getAttribute('href'); const p = href.indexOf('?'); - const elemParams = normalizeSearchQueryKeys(new URLSearchParams(p >= 0 ? href.substring(p + 1) : '')); + const elemParams = normalizeSearchQueryKeysAndValues(new URLSearchParams(p >= 0 ? href.substring(p + 1) : '')); // Add parameters that did not initially exist: for (const [key, value] of elemParams) { // URLSearchParams.has(key, value) seems to be broken on iOS 16, so check with our own method: if (!VuFind.inURLSearchParams(initialParams, key, value)) { - const normalizedKey = normalizeSearchQueryKey(key); - globalAddedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); + appendNormalizedValue(globalAddedParams, key, value); } } // Remove parameters that this URL no longer has: for (const [key, value] of initialParams) { if (!VuFind.inURLSearchParams(elemParams, key, value)) { - const normalizedKey = normalizeSearchQueryKey(key); - globalRemovedParams.append(normalizedKey, normalizeValue(normalizedKey, value)); + appendNormalizedValue(globalRemovedParams, key, value); } } }