Bug 29877 - MaxReserves should be enforced consistently between staff interface and API
Summary: MaxReserves should be enforced consistently between staff interface and API
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-01-13 14:46 UTC by Andrew Fuerste-Henry
Modified: 2022-03-21 00:40 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04,21.05.13,20.11.17


Attachments
Bug 29877: Regression tests (1.50 KB, patch)
2022-01-13 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29877: Make POST /holds handle maxreserves correctly (1.42 KB, patch)
2022-01-13 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29877: Regression tests (1.56 KB, patch)
2022-01-13 16:10 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29877: Make POST /holds handle maxreserves correctly (1.49 KB, patch)
2022-01-13 16:10 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29877: Regression tests (1.62 KB, patch)
2022-03-01 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29877: Make POST /holds handle maxreserves correctly (1.55 KB, patch)
2022-03-01 15:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-01-13 14:46:33 UTC
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.
Comment 1 Tomás Cohen Arazi 2022-01-13 15:16:12 UTC
Created attachment 129411 [details] [review]
Bug 29877: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-01-13 15:16:20 UTC
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>
Comment 3 Andrew Fuerste-Henry 2022-01-13 16:10:52 UTC
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>
Comment 4 Andrew Fuerste-Henry 2022-01-13 16:10:56 UTC
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>
Comment 5 Jonathan Druart 2022-01-17 13:39:48 UTC
Shouldn't we fix the staff interface's behaviour instead?
Considering 0 as infinite looks like a bug.
Comment 6 Kyle M Hall 2022-01-18 15:28:38 UTC
(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.
Comment 7 Katrin Fischer 2022-01-20 20:54:39 UTC
(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?
Comment 8 Katrin Fischer 2022-01-20 21:01:38 UTC
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)
Comment 9 Andrew Fuerste-Henry 2022-01-31 15:11:32 UTC
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.
Comment 10 Katrin Fischer 2022-01-31 19:34:55 UTC
(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.)
Comment 11 Katrin Fischer 2022-01-31 19:35:24 UTC
Hm or not (close) - we do still make them behave the same, right?
Comment 12 Tomás Cohen Arazi 2022-02-08 19:15:58 UTC
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
Comment 13 Katrin Fischer 2022-02-08 23:22:21 UTC
Sorry, was not clear enough maybe: not super happy, but ok with the patch as is. ;)
Comment 14 David Cook 2022-02-09 00:55:31 UTC
(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...
Comment 15 Tomás Cohen Arazi 2022-02-21 17:28:39 UTC
I forgot to mention bug 28529. We spotted it in the wild some time ago.
Comment 16 Martin Renvoize 2022-03-01 15:17:53 UTC
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>
Comment 17 Martin Renvoize 2022-03-01 15:17:57 UTC
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>
Comment 18 Martin Renvoize 2022-03-01 15:19:08 UTC
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
Comment 19 Fridolin Somers 2022-03-04 02:12:10 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 20 Kyle M Hall 2022-03-04 14:33:42 UTC
Pushed to 21.11.x for 21.11.04
Comment 21 Andrew Fuerste-Henry 2022-03-07 19:35:00 UTC
Pushed to 21.05.x for 21.05.12
Comment 22 Victor Grousset/tuxayo 2022-03-18 02:36:05 UTC
Backported: Pushed to 20.11.x branch for 20.11.17
Comment 23 wainuiwitikapark 2022-03-21 00:40:15 UTC
Does this need to be backported to 19.11.x?