Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carylwyatt merged 4 commits into from
Dec 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
02156ec
filter out empty form fields in search query and update ls boolean logic
carylwyatt Dec 4, 2025
7513486
refactor form logic for testability; add decorator and tests
carylwyatt Dec 5, 2025
998b40d
update bool logic for ls and catalog searches
carylwyatt Dec 12, 2025
b832cce
add tests for full text variants and catalog
carylwyatt Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 124 additions & 1 deletion src/js/components/AdvancedSearchForm/AdvancedSearchForm.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import AdvancedSearchForm from './index.svelte';
import { userEvent, within } from 'storybook/test';
import AdvancedSearchDecorator from '../../decorators/AdvancedSearchDecorator.svelte'
import { fn, expect, waitFor } from 'storybook/test';

export default {
title: 'AdvancedSearchForm',
component: AdvancedSearchForm,
decorators: [() => AdvancedSearchDecorator],
args: {
mockSubmit: fn(),
},
};

export const MobileDefault = {
Expand All @@ -22,4 +27,122 @@ export const DefaultDesktop = {
isRotated: false,
},
},
play: async ({ args, canvas, userEvent }) => {

await canvas.getByLabelText('Search Term 1').focus();
await userEvent.type(canvas.getByLabelText('Search Term 1'), 'elephant');
await userEvent.click(canvas.getByTestId('advanced-search-submit'));

await waitFor(() => expect(args.mockSubmit).toHaveBeenCalled())

const calledUrl = args.mockSubmit.mock.calls[0][0];
expect(calledUrl).toContain('q1=elephant')

}
};

export const TwoFieldsWithAnd = {
globals: {
viewport: {
value: 'bsXl',
isRotated: false,
},
},
play: async ({ args, canvas, userEvent }) => {

await canvas.getByLabelText('Search Term 1').focus();
await userEvent.type(canvas.getByLabelText('Search Term 1'), 'elephant');
const searchField2 = canvas.getByLabelText('Selected field 2');
await userEvent.selectOptions(searchField2, 'Subject');
await canvas.getByLabelText('Search Term 2').focus();
await userEvent.type(canvas.getByLabelText('Search Term 2'), 'conservation');
await userEvent.click(canvas.getByTestId('advanced-search-submit'));

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=AND')

}
}
export const FullTextFields1And4WithLastOr = {
globals: {
viewport: {
value: 'bsXl',
isRotated: false,
},
},
play: async ({ args, canvas, userEvent }) => {

await canvas.getByLabelText('Search Term 1').focus();
await userEvent.type(canvas.getByLabelText('Search Term 1'), 'elephant');
const searchField4 = canvas.getByLabelText('Selected field 4');
await userEvent.selectOptions(searchField4, 'Subject');
await canvas.getByLabelText('Search Term 4').focus();
await userEvent.type(canvas.getByLabelText('Search Term 4'), 'conservation');
const thirdRadioGroupOr = canvas.getAllByRole('radio', { name: 'OR' })[2];
await userEvent.click(thirdRadioGroupOr);
await userEvent.click(canvas.getByTestId('advanced-search-submit'));

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')
Copy link
Member

@aelkiss aelkiss Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


}
}

export const FullTextFields1And4WithFirstOr = {
Copy link
Member Author

@carylwyatt carylwyatt Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

globals: {
viewport: {
value: 'bsXl',
isRotated: false,
},
},
play: async ({ args, canvas, userEvent }) => {

await canvas.getByLabelText('Search Term 1').focus();
await userEvent.type(canvas.getByLabelText('Search Term 1'), 'elephant');
const firstRadioGroupOr = canvas.getAllByRole('radio', { name: 'OR' })[0];
await userEvent.click(firstRadioGroupOr);
const searchField4 = canvas.getByLabelText('Selected field 4');
await userEvent.selectOptions(searchField4, 'Subject');
await canvas.getByLabelText('Search Term 4').focus();
await userEvent.type(canvas.getByLabelText('Search Term 4'), 'conservation');
await userEvent.click(canvas.getByTestId('advanced-search-submit'));

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')

}
}
export const CatalogFields1And4WithLastOr = {
Copy link
Member Author

@carylwyatt carylwyatt Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

globals: {
viewport: {
value: 'bsXl',
isRotated: false,
},
},
play: async ({ args, canvas, userEvent }) => {

const searchField1 = canvas.getByLabelText('Selected field 1');
await userEvent.selectOptions(searchField1, 'Title');
await canvas.getByLabelText('Search Term 1').focus();
await userEvent.type(canvas.getByLabelText('Search Term 1'), 'apple');
const searchField4 = canvas.getByLabelText('Selected field 4');
await userEvent.selectOptions(searchField4, 'Author');
await canvas.getByLabelText('Search Term 4').focus();
await userEvent.type(canvas.getByLabelText('Search Term 4'), 'orange');
const thirdRadioGroupOr = canvas.getAllByRole('radio', { name: 'OR' })[2];
await userEvent.click(thirdRadioGroupOr);
await userEvent.click(canvas.getByTestId('advanced-search-submit'));

await waitFor(() => expect(args.mockSubmit).toHaveBeenCalled())

const calledUrl = args.mockSubmit.mock.calls[0][0];
expect(calledUrl).toContain('adv=1&setft=true&ft=ft&lookfor%5B%5D=apple&lookfor%5B%5D=orange&type%5B%5D=title&type%5B%5D=author&bool%5B%5D=OR')

}
}
88 changes: 67 additions & 21 deletions src/js/components/AdvancedSearchForm/index.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@
*/

/** @type {Props} */
let { formatData = [], languageData = [], locationData = [], collid = null, collectionName = null } = $props();
let {
formatData = [],
languageData = [],
locationData = [],
collid = null,
collectionName = null,
mockSubmit = null,
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} = $props();

// export let useAnyAll = true;

Expand Down Expand Up @@ -58,13 +65,12 @@
let lang = $state([]);
let format = $state([]);
let originalLocation = $state('');
let modal;
// let types = new Array(4); types.fill('ocr');
let types = $state(['ocr', 'all', 'title', 'author']);
let lookFors = $state(new Array(4));
lookFors.fill('');
let bools = $state(new Array(4));
bools.fill('AND');
bools[0] = null;
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting op1 as null since it's never used.

let anyalls = $state(new Array(4));
anyalls.fill('all');
let isFullView = $state(true);
Expand All @@ -88,7 +94,7 @@
}
}

function submitForm(event) {
function buildSearchUrl(event) {
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// which are we targeting?
errors.lookFors = false;
errors.yop = false;
Expand All @@ -103,6 +109,36 @@
break;
}
}

const filteredLookFors = [];
Copy link
Member Author

@carylwyatt carylwyatt Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const filteredTypes = [];
const filteredAnyAlls = [];
const filteredBools = [null]; //first will be null

let lastValidIndex = null;

for (let i = 0; i < lookFors.length; i++) {
if (lookFors[i] !== null && lookFors[i] !== '') {
filteredLookFors.push(lookFors[i]);
filteredTypes.push(types[i]);
filteredAnyAlls.push(anyalls[i]);

// use OR if it's the connecting bool between two lines, otherwise use AND
if (lastValidIndex !== null) {
Copy link
Member Author

@carylwyatt carylwyatt Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Image

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?

Copy link

@moseshll moseshll Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let connectingBool = 'AND'; //default to AND
for (let j = lastValidIndex + 1; j <= i; j++) {
if (bools[j] === 'OR') {
connectingBool = 'OR';
break;
}
}
filteredBools.push(connectingBool);
}

lastValidIndex = i;
}
}

if (target == 'catalog') {
url = new URL(`${protocol}//${HT.catalog_domain}/Search/Home`);
let searchParams = new URLSearchParams();
Expand Down Expand Up @@ -166,11 +202,8 @@
}
}

bools.forEach((value, idx) => {
if (idx === 0) {
return;
}
if (value && lookFors[idx] && lookFors[idx - 1]) {
filteredBools.forEach((value, idx) => {
Copy link
Member Author

@carylwyatt carylwyatt Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the catalog bools to use the filteredBools array.

if (value && filteredLookFors[idx]) {
searchParams.append('bool[]', value);
}
});
Expand Down Expand Up @@ -201,27 +234,30 @@
}

let hasSearchTerms = false;
lookFors.forEach((value, idx) => {
filteredLookFors.forEach((value, idx) => {
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting params based on the filtered arrays.

if (value) {
searchParams.set(`q${idx + 1}`, value);
hasSearchTerms = true;
}
});
types.forEach((value, idx) => {
if (value && lookFors[idx]) {
filteredTypes.forEach((value, idx) => {
if (value && filteredLookFors[idx]) {
searchParams.set(`field${idx + 1}`, value == 'everything' ? 'ocr' : value);
}
});
anyalls.forEach((value, idx) => {
if (value && lookFors[idx]) {
filteredAnyAlls.forEach((value, idx) => {
if (value && filteredLookFors[idx]) {
searchParams.set(`anyall${idx + 1}`, value);
}
});
bools.forEach((value, idx) => {
if (value && lookFors[idx]) {
searchParams.set(`op${idx + 1}`, value);
// if there's only one keyword, we don't need a boolean
if (filteredLookFors.length > 1) {
for (let idx = 1; idx < filteredBools.length; idx++) {
if (filteredBools[idx] && filteredLookFors[idx]) {
searchParams.set(`op${idx + 1}`, filteredBools[idx]);
}
}
});
}

if (!hasSearchTerms) {
errors.lookFors = true;
Expand Down Expand Up @@ -275,7 +311,16 @@
return;
}

location.assign(url.toString());
return url.toString();
}

function submitForm(event) {
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const url = buildSearchUrl(event);
if (mockSubmit) {
mockSubmit(url);
} else {
location.assign(url);
}
}

onMount(() => {
Expand Down Expand Up @@ -341,6 +386,7 @@
lookFors[0] = params.get('q1');
types[0] = params.get('field1') || 'ocr';
anyalls[0] = params.get('anyall1') || 'all';
bools[0] = null;
lookFors[1] = params.get('q2');
types[1] = params.get('field2') || 'all';
anyalls[1] = params.get('anyall2') || 'all';
Expand All @@ -352,7 +398,7 @@
lookFors[3] = params.get('q4');
types[3] = params.get('field4') || 'author';
anyalls[3] = params.get('anyall4') || 'all';
bools[4] = params.get('op3') || 'AND';
bools[3] = params.get('op4') || 'AND';
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@aelkiss aelkiss Dec 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (params.get('facet_lang')) {
params.getAll('facet_lang').forEach((value) => {
Expand Down Expand Up @@ -579,7 +625,7 @@
{/each}

<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">
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<span>Advanced Search</span>
<i class="fa-solid fa-arrow-up" aria-hidden="true"></i>
</button>
Expand Down
13 changes: 13 additions & 0 deletions src/js/decorators/AdvancedSearchDecorator.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<script>
Copy link
Member Author

@carylwyatt carylwyatt Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let { children } = $props();

const HT = {};
HT.service_domain = 'babel.hathitrust.org';
HT.catalog_domain = 'catalog.hathitrust.org';

globalThis.HT = HT;
</script>

<div>
{@render children()}
</div>