Scc 4167/all items

[charmingduchess]

preliminary PR for all_items. Currently includes:

• params parsing to include all_items and separate ES body construction from other findByUri queries
  • body also removes the exclusion of *_sort properties so we can...
• sorts items on enumerationChronology_sort
• tests!
  • testing that params are parsed and overrides happen
  • ensure that sorting happens properly (see comment in the code about this)
  • ensure that electronic item shenanigans happen properly

I'd love input on any other tests that should be added for this.
I'm also curious about if there is some stale code we can remove from the innerHitsProperties method.

[charmingduchess]

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.

[nonword]

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.

[charmingduchess]

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.

[nonword]

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.

[charmingduchess]

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

[nonword]

Looks good to me!

[nonword]

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.

[nonword]

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.