SCC-4348: callnumber= and standard_number= param #420
Conversation
routes/resources.js
Outdated
There was a problem hiding this comment.
so glad to see the gatherParams situation refactored.
| // We're specifically interested in params that match supported search- | ||
| // scopes because we can build a whole ES query just using the existing | ||
| // logic for that search scope: | ||
| if (this.request.advancedSearchParamsThatAreAlsoScopes()) { |
There was a problem hiding this comment.
what are the advanced search params that are not also scopes? is that just filters?
There was a problem hiding this comment.
I realize this is older work, but I think this method wants a different name. It's not the search_scopeness of the params we're interested in, but rather that they're valid single value query params (as opposed to a compound q=spaghetti&search_scope=title format). the method itself isn't actually verifying that they are also search scopes, just that they are in the approved array of advanced search params.
There was a problem hiding this comment.
The word "advanced" in "advanced search params" is just a reminder that this is for supporting params used in RC Adv Search, but actually there are many params besides q= that are not filters. For example isbn=, issn=. In the future there could be more. The fact that these params are also valid search_scope values is actually important because we're leveraging the existing search-scope logic to build the sub-query. We very much want each of the adv search params to individually work the same as the namesake search scopes do, so we do want to use the same logic. The method thus does verify that they are also search-scopes (recent update here). In the future, there may be other Adv Search params (e.g. 'ISBN'), which would be included in an "approved array of advanced search params" but wold not have a namesake search-scope. To support that, we would have to add custom handler code to add the extra clauses for that new adv-search param. The fact that all non-filter adv search params happen to have equivalent search-scopes right now is a convenience, but not one we assume will remain, hence the awkward function name.
| .forEach((advSearchParam) => { | ||
| const advSearchValue = this.request.params[advSearchParam] | ||
| // Build a new ApiRequest object for this search-scope: | ||
| const request = ApiRequest.fromParams({ |
There was a problem hiding this comment.
is this pattern being used elsewhere? something about it makes me uneasy...
There was a problem hiding this comment.
Because ApiRequest feels like something that should only ever wrap a real user-initiated API request? It's a light-weight model representing a user query. The Adv Search page essentially allows you to find the intersection of multiple queries, so I think it's fine, but can you say more about what is unsettling? Conceptually we do want the same query to be generated from q=foo&search_scope=title as is generated from title=foo, so the two routes need to ultimately share some code. Do you see a clearer way?
Add support for
callnumber=andstandard_number=params (for use in Adv Search).Removes use of
gatherParamsutil inlib/routesbecause it was redundant given that each controller was also explicitly parsing specific params from the query string. We should now only need to define params once.Changed how adv-search params are incorporated into the greater query (specifically for
title=,contributor=, andsubject=). Previously, due to an oversight, we only included the multi-match query associated with each of these params. With this update, we include the whole relevant search-scope query, including boosting. This means that all of the boosting applied to seach-scope queries is also applied to namesake adv-search queries (i.e.search_scope=title&q=fooshould return the same results astitle=foo,search_scope=contributor&q=barshould return the same results ascontributor=barandtitle=foo&contribotor=barshould return results that are boosted by the combination of boosting andmustclauses used in both, likely returning narrower results)https://newyorkpubliclibrary.atlassian.net/browse/SCC-4348