From b56b22ba8246dc0e4604af3a9f7f9f35b0437148 Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Fri, 24 Jan 2025 08:31:36 -0500 Subject: [PATCH 1/2] Fix checkbox facet duplication multi-select facet bug (#4203) --- .../src/VuFindTest/Mink/SearchFacetsTest.php | 47 +++++++++++++------ themes/bootstrap3/js/facets.js | 27 +++++++---- themes/bootstrap5/js/facets.js | 27 +++++++---- 3 files changed, 70 insertions(+), 31 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..8890d9b4966 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,13 @@ public function testMultiSelectOnAdvancedSearch(bool $changeLanguage): void $this->findCssAndGetText($page, '.adv_search_terms strong') ); + // 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; + $this->assertCount($expectedFilterCount, $queryArray['filter']); // If configured, flip-flop language again to potentially modify filter params: if ($changeLanguage) { diff --git a/themes/bootstrap3/js/facets.js b/themes/bootstrap3/js/facets.js index e64198a4d6c..2a474596301 100644 --- a/themes/bootstrap3/js/facets.js +++ b/themes/bootstrap3/js/facets.js @@ -178,23 +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))) { - initialParams.append(normalizeSearchQueryKey(key), normalizeValue(key, value)); + appendNormalizedValue(initialParams, key, value); } /** @@ -249,19 +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)) { - globalAddedParams.append(normalizeSearchQueryKey(key), 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)) { - globalRemovedParams.append(normalizeSearchQueryKey(key), value); + appendNormalizedValue(globalRemovedParams, key, value); } } } diff --git a/themes/bootstrap5/js/facets.js b/themes/bootstrap5/js/facets.js index e64198a4d6c..2a474596301 100644 --- a/themes/bootstrap5/js/facets.js +++ b/themes/bootstrap5/js/facets.js @@ -178,23 +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))) { - initialParams.append(normalizeSearchQueryKey(key), normalizeValue(key, value)); + appendNormalizedValue(initialParams, key, value); } /** @@ -249,19 +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)) { - globalAddedParams.append(normalizeSearchQueryKey(key), 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)) { - globalRemovedParams.append(normalizeSearchQueryKey(key), value); + appendNormalizedValue(globalRemovedParams, key, value); } } } From af2d1d38f0eb64a81d5ad95bc358944a789050f8 Mon Sep 17 00:00:00 2001 From: Demian Katz Date: Fri, 24 Jan 2025 08:37:44 -0500 Subject: [PATCH 2/2] Style: add missing semi-colons to generated JS code. (#4206) --- themes/bootstrap3/templates/layout/layout.phtml | 4 ++-- themes/bootstrap5/templates/layout/layout.phtml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/themes/bootstrap3/templates/layout/layout.phtml b/themes/bootstrap3/templates/layout/layout.phtml index 3235bc478d3..6f871f13db9 100644 --- a/themes/bootstrap3/templates/layout/layout.phtml +++ b/themes/bootstrap3/templates/layout/layout.phtml @@ -120,12 +120,12 @@ if (!empty($lightboxParent)) { $lightboxParent = json_encode((string)$lightboxParent); - $appendScripts[] = 'VuFind.lightbox.parent = ' . $lightboxParent; + $appendScripts[] = 'VuFind.lightbox.parent = ' . $lightboxParent . ';'; } if (!empty($lightboxChild)) { $lightboxChild = json_encode((string)$lightboxChild); - $appendScripts[] = 'VuFind.lightbox.child = ' . $lightboxChild; + $appendScripts[] = 'VuFind.lightbox.child = ' . $lightboxChild . ';'; } $this->headScript()->appendScript(implode("\n", $appendScripts)); diff --git a/themes/bootstrap5/templates/layout/layout.phtml b/themes/bootstrap5/templates/layout/layout.phtml index 46e73bb82ff..185a01d47f5 100644 --- a/themes/bootstrap5/templates/layout/layout.phtml +++ b/themes/bootstrap5/templates/layout/layout.phtml @@ -120,12 +120,12 @@ if (!empty($lightboxParent)) { $lightboxParent = json_encode((string)$lightboxParent); - $appendScripts[] = 'VuFind.lightbox.parent = ' . $lightboxParent; + $appendScripts[] = 'VuFind.lightbox.parent = ' . $lightboxParent . ';'; } if (!empty($lightboxChild)) { $lightboxChild = json_encode((string)$lightboxChild); - $appendScripts[] = 'VuFind.lightbox.child = ' . $lightboxChild; + $appendScripts[] = 'VuFind.lightbox.child = ' . $lightboxChild . ';'; } $this->headScript()->appendScript(implode("\n", $appendScripts));