Add vufind_sort option to [Loans] in Folio.ini#5109
Conversation
…ort options in [Loans] in Folio.ini
|
I honestly had more trouble coming up with the clearest explanation for this feature to add in the |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @meganschanz!
One obvious problem: how do you reconcile VuFind-level sorting with pagination when there is a large set of values? Each page will be sorted independently, but the overall result set may not make sense. Do we need to add a note about that? Disable some sort options when the result set is too big?
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @meganschanz -- see below for one comment related to the latest round of changes.
Beyond that, what are your thoughts on the pagination issue I mentioned earlier?
Well that feels obvious in retrospect, particularly considering the pagination was how I noticed the first sorting issue, but I didn't think through the pagination issue this would cause initially! After spending the afternoon thinking about, talking with the team and FOLIO developer there is no ideal solution for our specific end goal of enabling title sort on checked out items. When chatting with our FOLIO developer, there isn't a way to sort by title via the API and there likely wouldn't be support for adding it (due to the general policy of not wanting to do joins cross modules -- mod-circulation and mod-inventory). The FOLIO interface is able to do the sorting client-side because they have no pagination (they set the limit on the API call to an arbitrarily high number). For VuFind the only solutions I can see all have issues:
I think I'll ask this in the Slack to see if others in the community have ideas. Otherwise it might be preferable to just merge only the bug fix portion of this ticket to update the |
|
Thanks for the detailed analysis, @meganschanz. I agree that it might be beneficial to separate out the bug fix portion (maybe even as a PR against release-11.0, to get the fix in place faster) while we think through the full implications of this. It's possible that doing the "paginate through the results and then slice the appropriate page from the accumulated results" approach, combined with a large page size, would yield good results for the vast majority of cases. I think it's just a matter of watching out for situations where the page size gets so large that the Solr lookup associated with the request becomes too memory-expensive. In any case, tapping into the wisdom of Slack sounds like a good next step. Let me know if there's more I can do to help in the meantime! |
demiankatz
left a comment
There was a problem hiding this comment.
@meganschanz, now that #5121 is merged, there are some conflicts to resolve here... Of course, no rush if we're not even sure if we want to move forward with this yet. Just commenting to track the current state of things. :-)
|
I decided to look at this again. I got it to a working state, we just need to decide if we're comfortable with the potential performance implications of this feature. I did add a note about it in the config. This of course is not at all urgent, but maybe something we could get opinions on at an upcoming community call since Slack didn't get any feedback. I feel like it is a pretty common feature to be able to want to sort your checked out items by title and FOLIO's API is not going to add the ability to do it natively due to where the data is located cross-modules, so if we want to offer it it, this is likely the best approach. |
|
Thanks, @meganschanz -- I'll try to review this again when I'm a little more caught up. In the meantime, I've added this to the next Community Call agenda so I don't forget to bring it up there. My bottom line is that as long as the option is off by default and its trade-offs clearly documented, I don't think it's harmful to provide it if we think it will remain useful to some users on an ongoing basis. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @meganschanz, see below for a couple of thoughts from a very quick review!
| ; the same for non-unique values. | ||
| ;sort[dueDate/sort.ascending itemId/sort.ascending] = sort_due_date_asc | ||
| ;sort[dueDate/sort.descending itemId/sort.ascending] = sort_due_date_desc | ||
| sort[dueDate/sort.ascending itemId/sort.ascending] = sort_due_date_asc |
There was a problem hiding this comment.
Was it a conscious choice to uncomment the previously commented-out defaults?
There was a problem hiding this comment.
Good catch. That was unintentional. I just pushed a commit to undo that change.
|
|
||
| ; Default sort (note: this must be defined in the sort[] options above to be applied!) | ||
| ;default_sort="dueDate/sort.ascending itemId/sort.ascending" | ||
| default_sort="dueDate/sort.ascending itemId/sort.ascending" |
| * | ||
| * @return array | ||
| */ | ||
| protected function sortArray(array $data, string $sortField): array |
There was a problem hiding this comment.
Do we do this anywhere else? If so, I wonder if a trait would make sense. But I didn't check, so maybe that's a premature suggestion. :-)
There was a problem hiding this comment.
There isn't anywhere else in the code that currently does a strnatcasecmp. We have other sorting functions, but not ones that are sorting associative arrays based on a specific column (at least that I could find in my searching). I lean towards not making a trait until we have another use for it, but I could be convinced otherwise.
There was a problem hiding this comment.
I agree, no sense in overengineering this prematurely. I just thought there might be another similar function somewhere else. Thanks for confirming that there is not! :-)
demiankatz
left a comment
There was a problem hiding this comment.
@meganschanz, I had time for another closer look at the code, and the only thing I found this time was a typo in a comment.
I'd be happy to approve and merge this once that is fixed, but I can also leave it open until after the Community Call if you want to allow more time for feedback before closing things out. Let me know how you would like to proceed!
| $count = $this->getResultCount('/circulation/loans', compact('query')); | ||
| } | ||
|
|
||
| // Apply local sorted if requested |
There was a problem hiding this comment.
| // Apply local sorted if requested | |
| // Apply local sorting if requested |
This includes two updates to checked out items for FOLIO users:
vufind_sort[]option to[Loans]section of theFolio.inito enable adding VuFind side only sort options for checked out items. For example, sorting by a field that is not sortable in the FOLIO API (like title).sort[]options within the[Loans]section of theFolio.inito ensure consistent sort order in the event that a patron has more transactions with the samedueDatevalue than thepageSizeconfigured. FOLIO does not ensure the order of the items returned between pages in consistent in this case and can cause the same item to appear on multiple pages (and even items to be missing).