Skip to content
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.

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

WEBDEV-6909 Add radio search backend #54

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

latonv
Copy link
Contributor

@latonv latonv commented Jun 20, 2024

Adds a new search backend for requesting radio caption hits from the PPS, updating models to account for this new search type.

Copy link
Contributor

@iisa iisa left a comment

Choose a reason for hiding this comment

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

looks good! nice and easily loaded in. thanks for the test suite! had questions,comments

src/models/hit-types/hit.ts Show resolved Hide resolved
test/search-service.test.ts Show resolved Hide resolved
window.history.replaceState({}, '', url.toString());
});

it('includes reCache param from URL if not provided', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('includes reCache param from URL if not provided', async () => {
it('includes reCache param from URL if provided', async () => {

looks like recache is set in stub

Copy link
Contributor Author

@latonv latonv Jul 9, 2024

Choose a reason for hiding this comment

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

Thanks, I've rewritten the test descriptions to be clearer around this. "Provided" here was supposed to mean "provided as an option" which is now explicitly spelled out. The purpose of these tests is: if we don't receive a caching option from the caller, check whether there is a cache directive in the URL and use that instead if so.

test/search-backend/radio-search-backend.test.ts Outdated Show resolved Hide resolved
test/search-backend/radio-search-backend.test.ts Outdated Show resolved Hide resolved
test/search-backend/radio-search-backend.test.ts Outdated Show resolved Hide resolved
const response = await backend.performSearch({ query: 'foo' });
expect(response).to.exist; // No error thrown
expect(response.error).to.be.instanceof(SearchServiceError);
expect(response.error?.type).to.equal(SearchServiceErrorType.networkError);
Copy link
Contributor

Choose a reason for hiding this comment

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

is network error the default error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in general, no. But in this test, we are mocking window.fetch calls to return an error, which results in the networkError code from the service.

async performSearch(
params: SearchParams
): Promise<Result<any, SearchServiceError>> {
if (this.debuggingEnabled && params.debugging === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do !params.debugging ?

Copy link
Contributor Author

@latonv latonv Jul 9, 2024

Choose a reason for hiding this comment

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

debugging is allowed to be explicitly set to true or false on a search, which overrides any global default setting in the search service. So this line is just saying "if no debugging value was provided as an option, use the default".

!params.debugging would change the logic, as it would no longer be possible to override the default by providing false.

@iisa
Copy link
Contributor

iisa commented Jul 3, 2024

noting build is failing

@latonv latonv force-pushed the webdev6909-radio-search-backend branch from 2b4fec5 to f8ec5ad Compare July 10, 2024 01:11
Copy link

PR Preview Action v1.4.7
🚀 Deployed preview to https://internetarchive.github.io/iaux-search-service/pr/pr-54/
on branch gh-pages at 2024-07-10 01:12 UTC

@latonv
Copy link
Contributor Author

latonv commented Jul 10, 2024

Build issues fixed -- the node version in our CI config was outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants