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.

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

Scc 4167/all items #401

Merged
merged 9 commits into from
Sep 10, 2024
Merged

Scc 4167/all items #401

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e67a0f8
preliminary query passes preliminary test
charmingduchess Sep 7, 2024
30adf57
add request body to test
charmingduchess Sep 7, 2024
cc540e7
update queries
charmingduchess Sep 9, 2024
06944dc
add enum chron sort
charmingduchess Sep 9, 2024
53f1b0e
change sort order
charmingduchess Sep 9, 2024
9dac068
add resources response test for new yorker mag
charmingduchess Sep 10, 2024
949ad54
add test for electronic items bib
charmingduchess Sep 10, 2024
38d7e01
rename processInnerHits params
charmingduchess Sep 10, 2024
2140cd2
update comment
charmingduchess Sep 10, 2024
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
52 changes: 28 additions & 24 deletions lib/resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ module.exports = function (app, _private = null) {
app.resources.findByUri = function (params, opts = {}, request) {
// Parse all params we support:
params = parseParams(params, {
all_items: { type: 'boolean', default: false },
uri: { type: 'string' },
itemUri: { type: 'string' },
items_size: { type: 'int', default: 100, range: [0, 200] },
Expand Down Expand Up @@ -228,6 +229,9 @@ module.exports = function (app, _private = null) {
.then((recapBarcodesByStatus) => {
// Establish base query:
let body = {
_source: {
excludes: EXCLUDE_FIELDS
},
size: 1,
query: {
bool: {
Expand All @@ -239,38 +243,39 @@ module.exports = function (app, _private = null) {
}
]
}
},
_source: {
excludes: EXCLUDE_FIELDS.concat(['items'])
}
}

// No specific item requested, so add pagination and matching params:
const itemsOptions = {
size: params.items_size,
from: params.items_from,
merge_checkin_card_items: params.merge_checkin_card_items,
query: {
volume: params.item_volume,
date: params.item_date,
format: params.item_format,
location: params.item_location,
status: params.item_status,
itemUri: params.itemUri
},
unavailable_recap_barcodes: recapBarcodesByStatus['Not Available']
if (params.all_items) {
body._source.excludes = EXCLUDE_FIELDS.filter((field) => field !== '*_sort')
}
if (!params.all_items) {
// No specific item requested, so add pagination and matching params:
const itemsOptions = {
size: params.items_size,
from: params.items_from,
merge_checkin_card_items: params.merge_checkin_card_items,
query: {
volume: params.item_volume,
date: params.item_date,
format: params.item_format,
location: params.item_location,
status: params.item_status,
itemUri: params.itemUri
},
unavailable_recap_barcodes: recapBarcodesByStatus['Not Available']
}
body = addInnerHits(body, itemsOptions)
body._source = {
excludes: EXCLUDE_FIELDS.concat(['items'])
}
}
body = addInnerHits(body, itemsOptions)

if (params.include_item_aggregations) {
body.aggregations = ITEM_FILTER_AGGREGATIONS
}

app.logger.debug('Resources#findByUri', body)
return app.esClient.search(body)
.then((resp) => {
resp = resp.body

// Mindfully throw errors for known issues:
if (!resp || !resp.hits) {
throw new Error('Error connecting to index')
Expand Down Expand Up @@ -723,11 +728,10 @@ module.exports = function (app, _private = null) {
return app.esClient.search(body)
.then((resp) => {
resp = resp.body

const massagedResponse = new ResponseMassager(resp)
return massagedResponse.massagedResponse(request)
.catch((e) => {
// If error hitting HTC, just return response un-modified:
// If error hitting HTC, just return response un-modified:
return resp
})
.then((updatedResponse) => ResourceResultsSerializer.serialize(updatedResponse, opts))
Expand Down
10 changes: 6 additions & 4 deletions lib/response_massager.js
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't leave a comment on the exact line, but around line 34-38 it looks like we can get rid of some code.

Copy link
Member

Choose a reason for hiding this comment

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

You mean the part where we populate hit._source.numItemsTotal? I believe that's there to ensure that we get an accurate items count even if we have active item filters. When there are active item filters we insert a second inner_hits call without any item filters so that we can use that count for numItemsTotal. In the all_items case, there are no item filters by definition, so the allItems inner_hits won't be triggered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opted for something in the middle - renamed the param in processInnerHitsProperties to sound more agnostic to what is triggering it.

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ const parallelFieldsExtractor = require('./parallel-fields-extractor')
const { isAeonUrl } = require('../lib/util')
const FulfillmentResolver = require('./fulfillment_resolver')
const RequestabilityResolver = require('./requestability_resolver')
// const addNumItemsMatched = require('./item-match-numerator')

class ResponseMassager {
constructor (responseReceived) {
Expand All @@ -19,8 +18,11 @@ class ResponseMassager {
*
* Also copies ".total" properties into convenient places for serialization.
*/
processInnerHitsProperties (response) {
processInnerHitsProperties (response, allItemsBibQuery) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the args here because I didn't see a clearer way to branch the logic. I don't love this update, but let me know if you see any alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

I hear you. I think it's fine on balance. This seems like the right place to sort items. What triggers the sorting is the question. An alternative to adding a "do the sorting" param would be to change the check on line 23 to if (hit._source.items.some((i) => i.enumerationChronology_sort)) since items will only have that property when we've specifically requested it for manual sorting. But what you've done is prob clearer.

response.hits.hits.forEach((hit) => {
if (allItemsBibQuery) {
hit._source.items.sort((a, b) => a.enumerationChronology_sort[0] > b.enumerationChronology_sort[0] ? -1 : 1)
}
// Process "items" inner_hits
if (hit.inner_hits && hit.inner_hits.items) {
// Reassign items inner_hits to .items
Expand Down Expand Up @@ -58,10 +60,10 @@ class ResponseMassager {

massagedResponse (request, options = {}) {
let response = this.elasticSearchResponse

const allItemsBibQuery = request?.query?.all_items
// Inspect response inner_hits queries and move properties around to ease
// serialization:
response = this.processInnerHitsProperties(response)
response = this.processInnerHitsProperties(response, allItemsBibQuery)

// Rename parallel fields:
response = parallelFieldsExtractor(response)
Expand Down
25 changes: 23 additions & 2 deletions routes/resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,28 @@ module.exports = function (app) {
next()
})

const standardParams = ['page', 'per_page', 'q', 'filters', 'expandContext', 'ext', 'field', 'sort', 'sort_direction', 'search_scope', 'items_size', 'items_from', 'contributor', 'title', 'subject', 'isbn', 'issn', 'lccn', 'oclc', 'merge_checkin_card_items', 'include_item_aggregations']
const standardParams = ['page',
'per_page',
'q',
'filters',
'expandContext',
'ext',
'field',
'sort',
'sort_direction',
'search_scope',
'all_items',
'items_size',
'items_from',
'contributor',
'title',
'subject',
'isbn',
'issn',
'lccn',
'oclc',
'merge_checkin_card_items',
'include_item_aggregations']

const respond = (res, _resp, params) => {
let contentType = 'application/ld+json'
Expand Down Expand Up @@ -104,7 +125,7 @@ module.exports = function (app) {
* e.g. discovery/resources/b1234
*/
app.get(`/api/v${VER}/discovery/resources/:uri.:ext?`, function (req, res) {
const gatheredParams = gatherParams(req, ['uri', 'items_size', 'items_from', 'merge_checkin_card_items', 'include_item_aggregations'])
const gatheredParams = gatherParams(req, ['uri', 'items_size', 'items_from', 'merge_checkin_card_items', 'include_item_aggregations', 'all_items'])
const params = Object.assign({}, req.query, { uri: req.params.uri })

if (Number.isInteger(parseInt(gatheredParams.items_size))) params.items_size = gatheredParams.items_size
Expand Down
Loading
Loading