SCC-5490: Update item location filter#772
Conversation
7emansell
changed the title
| }] | ||
| } | ||
| } | ||
| return null |
There was a problem hiding this comment.
Since buildingLocation is calculated from holdingLocation, partner items will continue to return nothing for both. This case is already accounted for on the frontend
There was a problem hiding this comment.
ignore all the fixture updates sorry
There was a problem hiding this comment.
item format filter is being screwy on these not sure why
There was a problem hiding this comment.
I'd expect most of these to not change, should we investigate why updating some of the filter logic may have changed what is returned? Or is this a change to the underlying data?
There was a problem hiding this comment.
Went back and checked this specific bib on QA– item formats and counts did genuinely update, but updated to look for Book/text where previously I had just had Text
mwbenowitz
left a comment
mwbenowitz
left a comment
There was a problem hiding this comment.
Overall this looks good, just had a few questions!
| terms: { | ||
| size: 100, | ||
| script: { | ||
| source: "def locs = doc['items.holdingLocation.id']; if (locs.size() == 0) return null; def loc = locs.value; int colonIdx = loc.indexOf(':'); if (colonIdx == -1) return null; def locId = loc.substring(colonIdx + 1); if (locId.length() < 2) return null; def parentLoc = locId.substring(0, 2); if (['ma', 'pa', 'sc', 'rc', 'bu'].contains(parentLoc)) { return parentLoc; } return null;" |
There was a problem hiding this comment.
A few questions about this script!
- This instance and the one below seem substantially similar. Is there in value in refactoring them out to a
constand then just changing the returns? Would make things a bit easier to maintain - Any performance concerns with executing such a complicated Painless script? I think there are other ways to achieve this but they might not be possible with how the index is currently configured.
There was a problem hiding this comment.
Moved out to util.js. There is a performance cost (doesn't have to compile every time but it's still slower than a native ES query), and yes wouldn't have to do this if we indexed the building location ID on the item. Decided on this approach since it'll be faster to get out than a full reindex but could put that approach on the docket for upcoming reindexes?
There was a problem hiding this comment.
I'd expect most of these to not change, should we investigate why updating some of the filter logic may have changed what is returned? Or is this a change to the underlying data?
|
Not merging this into QA until status messaging work is closer to release (as is this will break the item location filters) |
4 participants
buildingLocationcalculated from the item's holding location IDdata/buildingLocationsbuildingLocationfield to put on theItemfor ease of display on the frontend (still need the inline script though since we're not indexing items with this)shortLabels to thedata/buildingLocationssince these strings don't seem to exist anywhere else. Still need full labels for other places on the frontendsupporting evidence:
Now ending up with item aggs that look like this: