[Bug 29877] New: MaxReserves should be enforced consistently between staff interface and API
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Bug ID: 29877 Summary: MaxReserves should be enforced consistently between staff interface and API Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: andrew@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit | |y.org | Status|NEW |ASSIGNED Component|Hold requests |REST API CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 129411 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129411&action=edit Bug 29877: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 129412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129412&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129411|0 |1 is obsolete| | --- Comment #3 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129415 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129415&action=edit Bug 29877: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129412|0 |1 is obsolete| | --- Comment #4 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 129416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129416&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Shouldn't we fix the staff interface's behaviour instead? Considering 0 as infinite looks like a bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm or not (close) - we do still make them behave the same, right? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Sorry, was not clear enough maybe: not super happy, but ok with the patch as is. ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I forgot to mention bug 28529. We spotted it in the wild some time ago. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129415|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 131189 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=131189&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #129416|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 131190 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=131190&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #18 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|22.05.00 |22.05.00,21.11.04 released in| | --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.11.x for 21.11.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00,21.11.04 |22.05.00,21.11.04,21.05.12 released in| | Status|Pushed to stable |Pushed to oldstable --- Comment #21 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.12 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00,21.11.04,21.05.12 |22.05.00,21.11.04,21.05.12, released in| |20.11.17 CC| |victor@tuxayo.net Status|Pushed to oldstable |Pushed to oldoldstable --- Comment #22 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Backported: Pushed to 20.11.x branch for 20.11.17 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00,21.11.04,21.05.12, |22.05.00,21.11.04,21.05.13, released in|20.11.17 |20.11.17 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29877 wainuiwitikapark@catalyst.net.nz changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #23 from wainuiwitikapark@catalyst.net.nz --- Does this need to be backported to 19.11.x? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org