Add getDefaultPickUpLocation() to FOLIO driver

[dltj]

TODO

• Document new circulation-storage.request-preferences.collection.get permission requirement in wiki and changelog when merging.

[demiankatz]

This looks reasonable to me from a code review alone, but since my library doesn't have multiple pickup locations, I can't easily test it. @maccabeelevine, are you in the same situation, or are you in a better position to try this out?

Regardless of that, a few thoughts:

1.) This looks like a good candidate for adding unit test coverage; it's not too complicated, so it wouldn't be too difficult. If you're interested in learning how to do it, I'd be happy to work with you on it. If you don't have time, maybe I can find a way to help add the tests.

2.) Does this require VuFind's FOLIO account to have any new/additional permissions? If so, we should be sure to document that.

[maccabeelevine]

This works nicely! I will have to do some refactoring in #3930 but that's fine and it makes my own PR better to handle this (more normal) case first.

If you want to be 100% safe, I think there should also be a check for $pref->holdShelf (== true) indicating the user is able to pick things up from any service point. I don't think there is any way in the UI to disable that, and for all I know the API won't let you do so either, so this is mostly pedantic, but the spec is what it is.

And yes it will need a permission circulation-storage.request-preferences.collection.get

[demiankatz]

Thanks, @maccabeelevine! A couple more follow-up thoughts:

1.) I'm not sure I understand the relevance of the $pref->holdShelf check to this PR; if the user has a default pickup set, it will be selected in the list. If they don't, the first option will be the default. I'm not sure how that setting would impact things one way or the other, but I also confess I didn't look at the spec, so maybe this is just going over my head. :-)

2.) I guess there's a question of whether we want to get a fatal error if the permission is missing, or if we should have a try..catch here and log a warning if the permission is missing. Usually when we add things that require new permissions, it's tied to new functionality and is unlikely to break existing installations... but in this case, I could see updating to use this code breaking requests unexpectedly if admins fail to pay attention to change notes. Maybe we should insulate against that, though I don't insist upon it. In any case, I'll add a TODO checkbox so we remember to document this one way or another.

[dltj]

Adding unit test coverage makes sense to me, and I'll have to read up on how to do that. (I'll make a first pass, Demian.) For that reason, I'm going to move this pull request to "draft".

I'm not entirely sure the new permission is necessary. I've been able to use this API call without it. Will need to test.

[maccabeelevine]

1.) I'm not sure I understand the relevance of the $pref->holdShelf check to this PR; if the user has a default pickup set, it will be selected in the list. If they don't, the first option will be the default. I'm not sure how that setting would impact things one way or the other, but I also confess I didn't look at the spec, so maybe this is just going over my head. :-)

It is possible (via the FOLIO API, though not the UI) for a user's request preference to have a defaultServicePointId set but have holdShelf = false, which would imply that they used to have a hold shelf preference but lost the permission to pick things up there. Theoretically because again, the FOLIO UI doesn't support it, so we can ignore if we like. But in the VuFind context of getDefaultPickUpLocation, we care specifically about pickups -- at least so far -- so it seems like we should confirm the user can actually do pickups. I'm thinking about this because in #3930 I expand that function to consider delivery fulfillment, and then check for the parallel delivery boolean, so it seems like this one should be parallel. But we can always discuss both points in #3930.

[demiankatz]

Adding unit test coverage makes sense to me, and I'll have to read up on how to do that. (I'll make a first pass, Demian.) For that reason, I'm going to move this pull request to "draft".

Thanks, @dltj! If you look at the existing test class you should get an idea of the basic patterns. The key is to turn on json_log_file in your Folio.ini (pointing at a file that's writable by Apache) and utilize the method. This will create a recording that you can then edit to replace any identifying information with fake values and then "play back" as part of the test. Let me know if you run into any trouble!

[demiankatz]

It is possible (via the FOLIO API, though not the UI) for a user's request preference to have a defaultServicePointId set but have holdShelf = false, which would imply that they used to have a hold shelf preference but lost the permission to pick things up there. Theoretically because again, the FOLIO UI doesn't support it, so we can ignore if we like. But in the VuFind context of getDefaultPickUpLocation, we care specifically about pickups -- at least so far -- so it seems like we should confirm the user can actually do pickups. I'm thinking about this because in #3930 I expand that function to consider delivery fulfillment, and then check for the parallel delivery boolean, so it seems like this one should be parallel. But we can always discuss both points in #3930.

Perhaps the question is whether this check needs to happen at a higher level. If the user has no hold permission but has a default location set, it probably doesn't really matter what their default is -- we should block the whole thing before we ask about defaults. In any case, I have no objection to making this smarter if there's an obvious way to do it, but it also sounds like waiting until #3930 might make everything more clear.

[maccabeelevine]

Perhaps the question is whether this check needs to happen at a higher level. If the user has no hold permission but has a default location set, it probably doesn't really matter what their default is -- we should block the whole thing before we ask about defaults. In any case, I have no objection to making this smarter if there's an obvious way to do it, but it also sounds like waiting until #3930 might make everything more clear.

Yeah I agree on both points -- better probably to do at a higher level, and easier to understand the tradeoffs once we are talking #3930.

[dltj]

Huh. So I'm puzzling over behavior I didn't expect. VuFind isn't actually using this driver method. Or, rather, it isn't using this method unless the VuFind MySQL record for the user has an empty home_library field. As soon as there is a value in the MySQL home_library field, it ignores anything in the user's FOLIO record. At least that is why my experimentation has show so far.

[dltj]

Ah, interesting! MyResearchController::profileAction() has a call to updateUserHomeLibrary(), if it exists in the ILS driver. So I think if I were to add that method (and perhaps add any appropriate permissions to the VuFind user in FOLIO), then the home library would get pushed back.

I'm looking 8 lines down from that updateUserHomeLibrary method, and I'm now concerned about how the VuFind database home_library column gets updated if the FOLIO defaultServicePointId field changes. If I'm reading the code correctly, my experience is confirmed...if there is a value in the VuFind database, the data from FOLIO is ignored.

Summarized this way, I think:

1. If there is no home_library value in the VuFind table, but there is a defaultServicePointId in FOLIO, then the user is shown the defaultServicePointId (and that location UUID is stored in the VuFind table for the user record).
2. If there is a home_library value in the VuFind table, the defaultServicePointId in FOLIO is not used.
3. If the user updates the home library in their Profile, the value is stored in the VuFind database but not sent back to VuFind (likely until I implement updateUserHomeLibrary() in the driver.
4. If the default service point is updated in FOLIO, it is never reflected in VuFind.

[demiankatz]

@dltj, I think this behavior can vary depending on whether the user chooses "Best available option" or "Always ask me" from the "Preferred Library" drop-down. Since Villanova has only one pickup location, this doesn't really apply to us, so I can't remember all the nuances. Perhaps @EreMaijala can comment more intelligently than me when he returns from the Summit.