Fix checkbox facet duplication multi-select facet bug (#4203)

vufind-org · Jan 24, 2025 · b56b22b · b56b22b
1 parent 866ff32
commit b56b22b
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 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
@@ -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
@@ -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
@@ -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);
         }
       }
     }