To test: 1 - set maxreserves to 1 2 - set all/all circ rule to unlimited holds allowed total, holds allowed daily, and holds per record 3 - confirm you can place 1 hold for a patron via staff interface, but the second is blocked 4 - confirm you can place 1 for a patron via API, but the second is blocked 5 - set maxreserves to 0 6 - confirm you can place any number of holds for the patron via the staff interface 7 - confirm you can place 0 holds for your patron via the API 8 - set maxreserves to blank 9 - confirm you can place any number of holds for the patron 10 - confirm you can place 0 holds for your patron via the API We should make the API treat zero and blank as un-set, as the staff interface does.
Created attachment 129411 [details] [review] Bug 29877: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129412 [details] [review] Bug 29877: Make POST /holds handle maxreserves correctly The current implementation doesn't consider the following values for the syspref: undef and 0. The tests mistakenly didn't cover them. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail, obvious warnings about comparing undefined values too. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. You can try on Postman as well => SUCCESS: Behavior is correct! 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129415 [details] [review] Bug 29877: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 129416 [details] [review] Bug 29877: Make POST /holds handle maxreserves correctly The current implementation doesn't consider the following values for the syspref: undef and 0. The tests mistakenly didn't cover them. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail, obvious warnings about comparing undefined values too. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. You can try on Postman as well => SUCCESS: Behavior is correct! 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Shouldn't we fix the staff interface's behaviour instead? Considering 0 as infinite looks like a bug.
(In reply to Jonathan Druart from comment #5) > Shouldn't we fix the staff interface's behaviour instead? > Considering 0 as infinite looks like a bug. I see your point, but that would break untold numbers of Koha installs. There is long historical precedent of having zero = blank = feature is disabled. If you want to change that, it should be on a separate bug report and will need a database update and many large warnings all over the upgrade notes.
(In reply to Kyle M Hall from comment #6) > (In reply to Jonathan Druart from comment #5) > > Shouldn't we fix the staff interface's behaviour instead? > > Considering 0 as infinite looks like a bug. > > I see your point, but that would break untold numbers of Koha installs. > There is long historical precedent of having zero = blank = feature is > disabled. If you want to change that, it should be on a separate bug report > and will need a database update and many large warnings all over the upgrade > notes. I am actually confused with this one... for holds, we here always assumed 0 to mean 'no holds allowed' - otherwise there would be no way to turn this off. I agree that leaving empty is often meant to mean "infinite", like for the number of checkouts. But also 0 there means no checkouts. So there is definitely an inconsistency. I believe we totally need a way to say 'no holds' in the rules - and also believe that it has worked at some point... are we sure there this was not a regression with switch to circulation_rules or similar?
Hm, I see this is about the system preference... while I was looking at the circulation rules. Still, maybe we could make the behavior consistent with a database update and update to the pref description? (make 0 work as one might expect and update the existing installations having 0 to empty for unlimited)
Bug 29875, just pushed to master, updates the system preference text to reflect the current behavior in which 0 disables the feature. We found this bug because we have libraries with MaxReserves=0 with the expectation that this will disable the system preference, as it has done for years. So while I appreciate that one *might* expect a value of 0 to mean no holds are allowed, I would counter than many users definitively *do* expect a value of 0 to mean this feature is disabled and to change that would be highly disruptive.
(In reply to Andrew Fuerste-Henry from comment #9) > Bug 29875, just pushed to master, updates the system preference text to > reflect the current behavior in which 0 disables the feature. > > We found this bug because we have libraries with MaxReserves=0 with the > expectation that this will disable the system preference, as it has done for > years. So while I appreciate that one *might* expect a value of 0 to mean no > holds are allowed, I would counter than many users definitively *do* expect > a value of 0 to mean this feature is disabled and to change that would be > highly disruptive. I understand your argument. I often vote for consistence over small interruptions, because I think long term it makes Koha easier to use. In this case it would have mimicked what empty means for max holds, max checkouts etc in circulation rules. As long as we can do so without causing a disruptive behavior change. I guess a workaround for disabling holds for users would be OPACHoldRequests and in the circulation rules, so we might not need a global one. (And can close this.)
Hm or not (close) - we do still make them behave the same, right?
Hi, I would like us to not get distracted by Koha lacking things or not ideal, and focus on the fact that this is the current behavior, and the API is not respecting it and is affecting stable. I'm more than happy to rewrite it all on a follow-up bug, add an on/off switch for holds and remove maxreserves too :-D On a separate bug :-D
Sorry, was not clear enough maybe: not super happy, but ok with the patch as is. ;)
(In reply to Tomás Cohen Arazi from comment #12) > I'm more than happy to rewrite it all on a follow-up bug, add an on/off > switch for holds and remove maxreserves too :-D On a separate bug :-D Yes please :3. I had some thoughts on this at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15565#c266 but didn't have funds/scope to do anything about it...
I forgot to mention bug 28529. We spotted it in the wild some time ago.
Created attachment 131189 [details] [review] Bug 29877: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131190 [details] [review] Bug 29877: Make POST /holds handle maxreserves correctly The current implementation doesn't consider the following values for the syspref: undef and 0. The tests mistakenly didn't cover them. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail, obvious warnings about comparing undefined values too. 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. You can try on Postman as well => SUCCESS: Behavior is correct! 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I think we're all in agreement here that this bug fix restores/corrects the functionality of the API to make it consistent with existing behaviours elsewhere and that further actions should take place in their own bug. As such, and as I can't spot any real regressions resulting from this.. Passing QA
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.04
Pushed to 21.05.x for 21.05.12
Backported: Pushed to 20.11.x branch for 20.11.17
Does this need to be backported to 19.11.x?