Bug 23104 - Regression (18925) in circ rules - unlimited vs 0
Summary: Regression (18925) in circ rules - unlimited vs 0
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18925
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-12 13:50 UTC by Jonathan Druart
Modified: 2020-06-04 20:33 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 23104: Add tests (1.48 KB, patch)
2019-06-12 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23104: (bug 18925 follow-up): Fix null/empty behavior, again (1.32 KB, patch)
2019-06-12 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23104: Add tests (1.53 KB, patch)
2019-06-12 17:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 23104: (bug 18925 follow-up): Fix null/empty behavior, again (1.37 KB, patch)
2019-06-12 17:45 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 23104: Add tests for maxonsiteissueqty (1.41 KB, patch)
2019-06-13 16:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23104: (follow-up) fix display for 0/Unlimited (1.71 KB, patch)
2019-06-17 13:55 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 23104: Add tests (1.59 KB, patch)
2019-06-17 20:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23104: (bug 18925 follow-up): Fix null/empty behavior, again (1.43 KB, patch)
2019-06-17 20:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23104: Add tests for maxonsiteissueqty (1.46 KB, patch)
2019-06-17 20:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23104: (follow-up) fix display for 0/Unlimited (1.77 KB, patch)
2019-06-17 20:15 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-06-12 13:50:23 UTC
From bug 18925 comment 124:

After updating to 19.05 with this patch included, circulation rules treated "Unlimited" in "Current checkouts allowed" and "Current on-site checkouts allowed" as 0 (zero) instead of infinite. Attempting to checkout items with an "Unlimited" maxissueqty or maxonsiteissueqty resulted in the error "•	Too many checked out. 0 checked out, only are allowed.•	This patron can't check out this item per library circulation policy" 

Circulation was allowed again after changing "Unlimited" to a numerical value. 

Currently, leaving the "Current checkouts allowed"/maxissueqty and "Current on-site checkouts allowed"/maxonsiteissueqty blank defaults to "Unlimited". Should either the default value be changed, or the code changed to treat the default value as infinite again, instead of zero?
Comment 1 Jonathan Druart 2019-06-12 14:15:59 UTC
Created attachment 90532 [details] [review]
Bug 23104: Add tests
Comment 2 Jonathan Druart 2019-06-12 14:16:04 UTC
Created attachment 90533 [details] [review]
Bug 23104: (bug 18925 follow-up): Fix null/empty behavior, again

Not kidding, it's again, and will certainly appear more.
The problem is that we should have a NULL value in DB to make things
more readable and logical. Now we need a quick fix.

Test plan:
Set "Current checkouts allowed" to unlimited (empty string)
Try to check an item out
=> Without this patch we are blocked with a "too many" message
=> With this patch applied the correct behavior is back!
Comment 3 Mark Tompsett 2019-06-12 17:43:24 UTC
Similar problem with "Current on-site checkouts allowed" as well. This only fixes "Current checkouts allowed"
Comment 4 Mark Tompsett 2019-06-12 17:45:25 UTC
Created attachment 90535 [details] [review]
Bug 23104: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Mark Tompsett 2019-06-12 17:45:28 UTC
Created attachment 90536 [details] [review]
Bug 23104: (bug 18925 follow-up): Fix null/empty behavior, again

Not kidding, it's again, and will certainly appear more.
The problem is that we should have a NULL value in DB to make things
more readable and logical. Now we need a quick fix.

Test plan:
Set "Current checkouts allowed" to unlimited (empty string)
Try to check an item out
=> Without this patch we are blocked with a "too many" message
=> With this patch applied the correct behavior is back!

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Jonathan Druart 2019-06-13 16:29:01 UTC
Created attachment 90581 [details] [review]
Bug 23104: Add tests for maxonsiteissueqty
Comment 7 Jonathan Druart 2019-06-13 16:29:32 UTC
(In reply to M. Tompsett from comment #3)
> Similar problem with "Current on-site checkouts allowed" as well. This only
> fixes "Current checkouts allowed"

How that? Could you detail the steps to recreate?
Comment 8 Nick Clemens 2019-06-14 16:00:20 UTC
Are we supposed to be able to set the number to 0? I thought we were

Currently 0 turns into empty turns into unlimited
Comment 9 Katrin Fischer 2019-06-16 20:09:55 UTC
(In reply to Nick Clemens from comment #8)
> Are we supposed to be able to set the number to 0? I thought we were
> 
> Currently 0 turns into empty turns into unlimited

We definitely need 0 too.
Comment 10 Mark Tompsett 2019-06-17 13:55:43 UTC
Created attachment 90666 [details] [review]
Bug 23104: (follow-up) fix display for 0/Unlimited
Comment 11 Katrin Fischer 2019-06-17 20:14:59 UTC
Created attachment 90691 [details] [review]
Bug 23104: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2019-06-17 20:15:04 UTC
Created attachment 90692 [details] [review]
Bug 23104: (bug 18925 follow-up): Fix null/empty behavior, again

Not kidding, it's again, and will certainly appear more.
The problem is that we should have a NULL value in DB to make things
more readable and logical. Now we need a quick fix.

Test plan:
Set "Current checkouts allowed" to unlimited (empty string)
Try to check an item out
=> Without this patch we are blocked with a "too many" message
=> With this patch applied the correct behavior is back!

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2019-06-17 20:15:08 UTC
Created attachment 90693 [details] [review]
Bug 23104: Add tests for maxonsiteissueqty

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2019-06-17 20:15:12 UTC
Created attachment 90694 [details] [review]
Bug 23104: (follow-up) fix display for 0/Unlimited

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Martin Renvoize 2019-06-18 09:07:49 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Fridolin Somers 2019-06-20 12:53:16 UTC
Pushed to 19.05.x for 19.05.01
Comment 17 Lucas Gass 2019-06-26 17:12:51 UTC
missing dependency for 18.11.x, wont backport