ETT-827: full-text advanced search boolean operator bug fix #136

carylwyatt · 2025-12-05T21:23:01Z

This work-around more or less fixes the bug that was described in the ticket, but there is more under the surface here that I don't 100% understand.

To set the scene: Looking at this form, it's visually obvious that there can be 4 keywords/fields/types and only 3 boolean operators at the maximum. @moseshll discovered that op1/[bools][0] seems to be doing nothing, and I agree, so it gets skipped in this new URL construction. All other search-related arrays are filtered against the lookFors/keyword array so that the related indexes are filtered and reassigned. In a search on prod, searching for "apples" on line 1 and "oranges" on line 4 results in a URL like q1=apples&q4=oranges&op1=AND&op4=AND. This fix changes changes the URL output to something like q1=apples&q2=oranges&op2=AND.

Some history: The version of advanced search before this searched groups of clauses, and I think there is some leftover logic for that hanging out here in a way that isn’t supported in the interface. Lianet’s full-text XML spreadsheet indicates that the param “OP3” is “the operator between second and third search fields, i.e. between groups.” In the old interface, there were two groups of inputs connected with a separate boolean, making it possible to search like (cake AND pie) OR (cake AND ice cream). The op3 param seems to be that connecting OR operator in my example. I'm not sure this makes much of a difference in search results now that I'm re-writing the placement of keywords (e.g. q1=cake&q4=pie&op1=AND&op4=AND is now q1=cake&q2=pie&op2=AND), but in prod currently, if you try to do a search on lines 2 and 3 or 3 and 4 without anything in the first line, it spits out an error about parentheses and/or booleans.

This fix unfortunately creates a new sporadic bug in catalog advanced search. Occasionally, the third boolean radio group will have nothing selected, just two empty radio buttons. This doesn't really affect the searching mechanism or outcome of the search since a user can just select one and move on, but it is annoying. Thankfully (?), it's pretty challenging to get to the catalog version of advanced search, so I'm inclined to ignore this problem until we get complaints about it and/or redesign search.

carylwyatt · 2025-12-10T18:49:32Z

src/js/components/AdvancedSearchForm/index.svelte

+    locationData = [],
+    collid = null,
+    collectionName = null,
+    mockSubmit = null,


New mockSubmit function included here as a prop to pass in as an argument for storybook test.

carylwyatt · 2025-12-10T18:50:11Z

src/js/components/AdvancedSearchForm/index.svelte

  lookFors.fill('');
  let bools = $state(new Array(4));
  bools.fill('AND');
+  bools[0] = null;


Setting op1 as null since it's never used.

carylwyatt · 2025-12-10T18:52:18Z

src/js/components/AdvancedSearchForm/index.svelte

  }

-  function submitForm(event) {
+  function buildSearchUrl(event) {


I refactored this function and renamed as buildSearchUrl. I extracted the part that does the form "submission" so I could test the URL that is constructed in this step.

carylwyatt · 2025-12-10T18:53:37Z

src/js/components/AdvancedSearchForm/index.svelte

+      const filteredAnyAlls = [];
+      const filteredBools = [];
+
+      for (let i = 0; i < lookFors.length; i++) {


The loop that filters the search arrays depending on whether that line in the search form was filled in.

carylwyatt · 2025-12-10T18:53:56Z

src/js/components/AdvancedSearchForm/index.svelte

+
      let hasSearchTerms = false;
-      lookFors.forEach((value, idx) => {
+      filteredLookFors.forEach((value, idx) => {


Setting params based on the filtered arrays.

carylwyatt · 2025-12-10T18:56:03Z

src/js/components/AdvancedSearchForm/index.svelte

+    return url.toString();
+  }
+
+  function submitForm(event) {


Extracted function that "submits" the form. It actually just reassigns the page's URL to the constructed URL. The storybook test is the only file that uses mockSubmit.

carylwyatt · 2025-12-10T18:57:12Z

src/js/components/AdvancedSearchForm/index.svelte

      types[3] = params.get('field4') || 'author';
      anyalls[3] = params.get('anyall4') || 'all';
-      bools[4] = params.get('op3') || 'AND';
+      bools[3] = params.get('op4') || 'AND';


The bools needed some work. @moseshll pointed out the mismatch in an earlier comment and this rearranged the indexes to have them match.

This definitely seems like a case where it would be easier to reason about and work with if the params had more structure directly (i.e. were expressed in JSON), but not feasible to change now without making changes in ls.

carylwyatt · 2025-12-10T18:58:18Z

src/js/components/AdvancedSearchForm/index.svelte


        <div class="d-flex mb-3 justify-content-end">
-          <button class="btn btn-primary btn-lg" type="submit">
+          <button data-testid="advanced-search-submit" class="btn btn-primary btn-lg" type="submit">


Gayathri , Ange, and I have all lamented that these buttons don't make sense and that advanced search in general needs a redesign. Why are there 2 "Advanced Search" buttons on this page? I don't know, but it was too hard to select this button for storybook without giving it a test ID.

carylwyatt · 2025-12-10T18:58:58Z

src/js/decorators/AdvancedSearchDecorator.svelte

@@ -0,0 +1,12 @@
+<script>


I needed to add a decorator to this story to account for the HT global variable in the component.

aelkiss

Having tests in storybook for this is great; as I've commented, in many ways we're limited by the interface that full-text search has given us. I think it would be tricky to do much more refactoring here in a way that makes it easier to understand. One benefit of what's here is that it makes some of the "data clumpiness" a little more apparent for consideration/rework in a future redesign of advanced search.

aelkiss · 2025-12-11T18:52:30Z

src/js/components/AdvancedSearchForm/AdvancedSearchForm.stories.js

+    await waitFor(() => expect(args.mockSubmit).toHaveBeenCalled())
+
+    const calledUrl = args.mockSubmit.mock.calls[0][0];
+    expect(calledUrl).toContain('lmt=ft&a=srchls&adv=1&q1=elephant&q2=conservation&field1=ocr&field2=subject&anyall1=all&anyall2=all&op2=OR')


Stories look good; I verified that these URLs in full-text search (i.e. prepending this with https://babel.hathitrust.org/cgi/ls?) appears to work as expected.

aelkiss · 2025-12-11T18:54:23Z

src/js/components/AdvancedSearchForm/index.svelte

        searchParams.set('c', collid);
      }

+      const filteredLookFors = [];


This seems like one of those "data clump" cases as we were discussing with 99 bottles, BUT it's very dependent on the interface with full-text search -- so this seems like more of a design opportunity to rethink the data structure here (maybe passing something with more structure as JSON) in the future, rather than doing anything different here right now.

aelkiss · 2025-12-11T18:55:56Z

src/js/components/AdvancedSearchForm/index.svelte

      types[3] = params.get('field4') || 'author';
      anyalls[3] = params.get('anyall4') || 'all';
-      bools[4] = params.get('op3') || 'AND';
+      bools[3] = params.get('op4') || 'AND';


This definitely seems like a case where it would be easier to reason about and work with if the params had more structure directly (i.e. were expressed in JSON), but not feasible to change now without making changes in ls.

carylwyatt · 2025-12-11T20:55:25Z

@moseshll discovered a bug with catalog searches, so I'm going to fix that up before going for a second review from him.

carylwyatt · 2025-12-12T19:23:46Z

src/js/components/AdvancedSearchForm/AdvancedSearchForm.stories.js

+  }
+}
+
+export const FullTextFields1And4WithFirstOr = {


New test for fields 1 and 4 if connected with the first OR (this wasn't working before).

carylwyatt · 2025-12-12T19:24:20Z

src/js/components/AdvancedSearchForm/AdvancedSearchForm.stories.js

+
+  }
+}
+export const CatalogFields1And4WithLastOr = {


New test for constructing a catalog search URL with fields 1 and 4 connected with the third boolean OR.

carylwyatt · 2025-12-12T19:25:06Z

src/js/components/AdvancedSearchForm/index.svelte

      }
    }
+
+    const filteredLookFors = [];


Moved these filtered arrays to outside the ls scope so catalog URL construction could access filteredBools, too.

carylwyatt · 2025-12-12T19:31:22Z

src/js/components/AdvancedSearchForm/index.svelte

+        filteredAnyAlls.push(anyalls[i]);
+
+        // use OR if it's the connecting bool between two lines, otherwise use AND
+        if (lastValidIndex !== null) {


I kept running into an issue while working on this where I could get the value for the first OR or the last OR but never both of them.

I used Claude to help figure this out. What Ange really wanted was to always default to an 'OR' if the user selected it somewhere:

would it make sense to write a rule where, because AND is the default, then selecting OR should have priority over AND when they come into potential conflict?

I agree, since it takes effort to select the non-default.

carylwyatt · 2025-12-12T19:31:54Z

src/js/components/AdvancedSearchForm/index.svelte

-          return;
-        }
-        if (value && lookFors[idx] && lookFors[idx - 1]) {
+      filteredBools.forEach((value, idx) => {


Refactored the catalog bools to use the filteredBools array.

moseshll

Checked the latest on dev-3 and I am happy with the result. APPROVE

carylwyatt added 2 commits December 5, 2025 15:43

filter out empty form fields in search query and update ls boolean logic

02156ec

refactor form logic for testability; add decorator and tests

7513486

carylwyatt commented Dec 10, 2025

View reviewed changes

carylwyatt requested review from aelkiss and moseshll December 10, 2025 18:59

aelkiss approved these changes Dec 11, 2025

View reviewed changes

aelkiss mentioned this pull request Dec 12, 2025

ETT-1154: don't submit search if there are errors #138

Merged

update bool logic for ls and catalog searches

998b40d

carylwyatt commented Dec 12, 2025

View reviewed changes

add tests for full text variants and catalog

b832cce

carylwyatt force-pushed the ETT-827-advanced-search branch from a2eead3 to b832cce Compare December 12, 2025 19:32

moseshll approved these changes Dec 12, 2025

View reviewed changes

carylwyatt merged commit c659da1 into main Dec 16, 2025
5 checks passed

carylwyatt deleted the ETT-827-advanced-search branch December 16, 2025 14:15

ETT-827: full-text advanced search boolean operator bug fix #136

ETT-827: full-text advanced search boolean operator bug fix #136

Uh oh!

Conversation

carylwyatt commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelkiss Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelkiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelkiss Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carylwyatt commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moseshll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carylwyatt commented Dec 5, 2025 •

edited

Loading

aelkiss Dec 11, 2025 •

edited

Loading

aelkiss Dec 11, 2025 •

edited

Loading