Cancel buttons for holds

[olli-gold]

This PR fixes a minor problem in the MyResearchController. Normally the buttons to cancel a Hold should get hidden if no Hold can be cancelled. This did not work (at least not in my VuFind instance), and I hope, this little PR fixes that.

[demiankatz]

I revised this PR slightly -- if you have an empty() check, you don't need an isset() check, since empty() performs the same task for you automatically.

However, I'm not sure if this is the best solution to your problem -- or at least, this fix may be masking some deeper issue. According to the spec, getCancelHoldDetails() returns either an arbitrary string (to allow cancellation) or an empty string (to indicate that cancellation is not possible). If an empy string is returned, this logic is supposed to prevent the value from being set at all, which should allow the existing isset() check to work correctly:

https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/Plugin/Holds.php#L63

Unless I'm missing something, this suggests that your ILS driver is returning a value that is not an empty string but that causes empty() to evaluate to true -- like perhaps a '0' character. I don't think we want to treat that as a "disabled" value, since theoretically "0" might be a valid identifier. Any thoughts?

[olli-gold]

Well, this is what I get as $current in the foreach loop in MyResearchController (https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/MyResearchController.php#L916):

array (size=11)
  'item_id' => string 'http://uri.gbv.de/document/opac-de-830:bar:830$34096983' (length=55)
  'cancel_details' => string '' (length=0)
  'id' => string '40445623' (length=8)
  'type' => string 'provided' (length=8)
  'location' => string 'Test-Theke' (length=10)
  'position' => null
  'available' => boolean true
  'title' => string 'Praktikum über Entwurf und Manipulation von Datenbanken : SQL/DS (IBM), UDS (Siemens) und MEMODAX / Vossen, Gottfried (1986)' (length=125)
  'callnumber' => string '34:3409-6983' (length=12)
  'create' => string '17.06.2016' (length=10)
  'expire' => string '' (length=0)

As you see, cancel_details is an empty string as expected.
This if-clause in https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/MyResearchController.php#L922 is returning true.

However changing the if-clause in https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/MyResearchController.php#L922 like that:
&& !empty($current['cancel_details'])
is returning false.

Thus your revision of the PR is working fine for me while the original version is not working fine. I don't see the driver returning anything wrong here, do you?

[demiankatz]

Thanks for the clarification -- looks like this is the right fix for this context. I'll take a closer look (and most likely merge) when I get back from vacation!

[demiankatz]

I took another look at this, and I still think something is not quite right somewhere.

If I understand things correctly, when the controller retrieves hold information from the ILS driver here:

https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/MyResearchController.php#L913

... there should be no cancel_details set anywhere.

It is this code in the loop which adds the cancel_details element:

https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/MyResearchController.php#L917

That calls the Holds controller plugin, and this code in the plugin should prevent cancel_details from being set when getCancelHoldDetails returns an empty string:

https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/Controller/Plugin/Holds.php#L63

So either your ILS driver is setting cancel_details PRIOR to the getCancelHoldDetails call (which is a violation of the spec, though I can understand why it would be a worthwhile feature if supported since it would save some time -- we can discuss adding support if that's the case) or else the getCancelHoldDetails function is misbehaving for some reason.

Could you take another look at this and let me know what you find?

Thanks!

[olli-gold]

You are right, the driver is setting the cancel_details prior to the getCancelHoldDetails call and therefore violates the spec. @lahmann: Should we change the PAIA driver to match the spec at this point?

[demiankatz]

@lahmann , @olli-gold : the alternative would be to add cancel_details as an optional feature of the spec and change the logic to skip calling the controller plugin when the value is already present (but then the driver would also need to be changed to NOT set cancel_details when the value was an empty string).

[demiankatz]

(Actually, my off-the-cuff proposed solution has some problems ... if we call getCancelHoldDetails when cancel_details is absent, but don't set cancel_details when cancel is unsupported, that will cause some unnecessary extra ILS driver calls... and that gets us back to perhaps wanting an empty() check instead of isset()... but I'm still a little wary of that because empty() also evaluates to true for 0, etc.)

[olli-gold]

I would appreciate to change the spec and to add setting cancel_details in the driver as a new (optional) feature. This won't break anything and perhaps can keep things simple in the PAIA driver (and perhaps open up a new way to simplify other drivers as well). But perhaps there is a downside of that somewhere, that I am missing?

[demiankatz]

I would think the main downside is the performance hit involved in providing this data as part of the initial call, since it may not be needed at all. Historically this has been an expensive operation. However, if it is low-cost in some cases, I don't think it hurts to revise the spec to allow it -- but we do need to pin down exactly how we want to implement that support, since that will definitely require a few changes beyond those found in this PR. Would you like to take a stab at it?

[olli-gold]

I'd love to take this task, but I'm not sure when I'll find the time to do that. Unfortunately I'm still stuck at #608 and will need to do some other tasks besides VuFind in the next weeks.
But if this is something that can wait for a while, I'm absolutely willing to do it.

[demiankatz]

It can wait. If I find extra time, I might try working on an implementation for you to test... but at the moment, I have a lot of other things to catch up on as well, so this may just have to wait. If you don't mind the wait, it's fine with me too!

[demiankatz]

@olli-gold, I finally had a bit of time to look into this, and I think I've come up with a flexible solution.

Using the code in this PR, the ILS driver can set cancel_details to the getCancelHoldDetails string up front in the getMyHolds() return value up front. This can be either a blank string (indicating unsupported) or whatever internal token is used. If cancel_details is unset, getCancelHoldDetails will be called as usual. Regardless of which mechanism is used, the output of addCancelDetails() will ensure that cancel_details is only set when cancel is supported, and it is unset otherwise.

I've tested this with the existing Voyager driver and it seems to work fine... but I haven't tested any of the cases where tokens are provided up-front in getMyHolds. Can you take a look at my logic, test it with your driver, and let me know if this seems like a reasonable solution? If so, I think I can go ahead and update the spec and merge this.

Thanks!

[olli-gold]

Thank you for reviewing and improving this. I tested it with our PAIA driver and its working for me. I think you can go ahead, though!

Thank you!

[demiankatz]

Thanks for double-checking. I've gone ahead and merged this!