Bug 26529 - Holds rules enforced incorrectly when not set at library level
Summary: Holds rules enforced incorrectly when not set at library level
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-24 15:07 UTC by Andrew Fuerste-Henry
Modified: 2020-11-10 22:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 26529: Set rule value to undef when not setfor branch defaults (3.05 KB, patch)
2020-09-24 18:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Consider blank rules as undef for default hold policies (1.41 KB, patch)
2020-09-24 18:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Define some rules as not able to be blank (4.46 KB, patch)
2020-09-24 18:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Consider blank rules as undef for default hold policies (1.47 KB, patch)
2020-09-24 22:42 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26529: Define some rules as not able to be blank (3.92 KB, patch)
2020-09-24 22:42 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26529: Define some rules as not able to be blank (3.30 KB, patch)
2020-09-28 13:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Unit tests (2.22 KB, patch)
2020-09-28 13:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Remove existing blank rules (1.06 KB, patch)
2020-09-28 13:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Define some rules as not able to be blank (3.37 KB, patch)
2020-10-05 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26529: Unit tests (2.28 KB, patch)
2020-10-05 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26529: Remove existing blank rules (1.12 KB, patch)
2020-10-05 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26529: (follow-up) Remove other instance of blanking holdallowed from script (872 bytes, patch)
2020-10-08 17:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26529: Remove warning (995 bytes, patch)
2020-10-12 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26529: (follow-up) Remove warning (978 bytes, patch)
2020-10-12 12:49 UTC, Jonathan Druart
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 2020-09-24 15:07:22 UTC
To recreate:
1 - Set ReservesControl to item home library
2 - confirm your circ rules allow holds at both the All Libraries and library-specific levels
3 - confirm Default checkout, hold and return policy is totally unset at both the All Libraries and library-specific levels
4 - place a hold on an item at Branch A for pickup at Branch A
5 - check item in, confirm hold triggers
6 - cancel hold
7 - go to Default checkout, hold and return policy for Branch A
8 - enter 1 for Total current checkouts allowed, save that with no other values entered
9 - re-place hold, confirm no error when placing hold
10 - check item in, hold does not trigger
11 - set Hold policy and Hold pickup library match at to Any Library at All Libraries level
12 - confirm hold still doesn't capture
set Hold policy and Hold pickup library match at to Any Library for Branch A
13 - confirm hold now captures

If these rules aren't set at the branch level we should be falling back to All Libraries. If they're not set anywhere, we should be assuming holds are allowed is the circ rules allow them.

Starting from default testing docker data, this is what my db shows at step 7:
MySQL [koha_kohadev]> select * from circulation_rules;
+----+------------+--------------+----------+-------------------+------------+
| id | branchcode | categorycode | itemtype | rule_name         | rule_value |
+----+------------+--------------+----------+-------------------+------------+
|  1 | NULL       | NULL         | NULL     | maxissueqty       | 5          |
|  2 | NULL       | NULL         | NULL     | maxonsiteissueqty | 5          |
+----+------------+--------------+----------+-------------------+------------+

And this is what it shows at step 8:
MySQL [koha_kohadev]> select * from circulation_rules;
+----+------------+--------------+----------+--------------------------+------------+
| id | branchcode | categorycode | itemtype | rule_name                | rule_value |
+----+------------+--------------+----------+--------------------------+------------+
|  1 | NULL       | NULL         | NULL     | maxissueqty              | 5          |
|  2 | NULL       | NULL         | NULL     | maxonsiteissueqty        | 5          |
|  8 | CPL        | NULL         | NULL     | patron_maxissueqty       | 1          |
|  9 | CPL        | NULL         | NULL     | returnbranch             |            |
| 10 | CPL        | NULL         | NULL     | hold_fulfillment_policy  |            |
| 11 | CPL        | NULL         | NULL     | patron_maxonsiteissueqty |            |
| 12 | CPL        | NULL         | NULL     | max_holds                |            |
+----+------------+--------------+----------+--------------------------+------------+

I cannot recreate this on master.
Comment 1 Andrew Fuerste-Henry 2020-09-24 16:51:41 UTC
Nor can I recreate this on 20.05
Comment 2 Nick Clemens 2020-09-24 18:11:34 UTC
Created attachment 110686 [details] [review]
Bug 26529: Set rule value to undef when not setfor branch defaults
Comment 3 Nick Clemens 2020-09-24 18:13:43 UTC
Created attachment 110687 [details] [review]
Bug 26529: Consider blank rules as undef for default hold policies
Comment 4 Nick Clemens 2020-09-24 18:46:26 UTC
Created attachment 110695 [details] [review]
Bug 26529: Define some rules as not able to be blank

This patch uses the existing hash that controls the scope of rules and adds a
new parameter 'can_be_blank'

To test:
1 - Goto to Admin -> Circulation and fines rules and select a library
2 - Under "Default checkout, hold, and return policy" set:
    Total current checkouts allowed: 1
    Hold policy: Not set
    Hold pickup library match: Not set
    Return policay: Not set
3 - Save the rule
4 - Check the DB
    SELECT * FROM circulation_rules WHERE branchcode = 'BRANCHCODE'
    using the branchcode for the library you chode
5 - Note holdallowed, returnbranch, and hold_fulfillment_policy rules exist with blank value
6 - Apply patch and restart all the things
7 - Reload the page and save the rules as before
8 - Check the DB and note the rules no longer exist
9 - pr0ve -v t/db_dependent/Koha/CirculationRules.t
Comment 5 Andrew Fuerste-Henry 2020-09-24 22:42:15 UTC
Created attachment 110714 [details] [review]
Bug 26529: Consider blank rules as undef for default hold policies

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 6 Andrew Fuerste-Henry 2020-09-24 22:42:19 UTC
Created attachment 110715 [details] [review]
Bug 26529: Define some rules as not able to be blank

This patch uses the existing hash that controls the scope of rules and adds a
new parameter 'can_be_blank'

To test:
1 - Goto to Admin -> Circulation and fines rules and select a library
2 - Under "Default checkout, hold, and return policy" set:
    Total current checkouts allowed: 1
    Hold policy: Not set
    Hold pickup library match: Not set
    Return policay: Not set
3 - Save the rule
4 - Check the DB
    SELECT * FROM circulation_rules WHERE branchcode = 'BRANCHCODE'
    using the branchcode for the library you chode
5 - Note holdallowed, returnbranch, and hold_fulfillment_policy rules exist with blank value
6 - Apply patch and restart all the things
7 - Reload the page and save the rules as before
8 - Check the DB and note the rules no longer exist
9 - pr0ve -v t/db_dependent/Koha/CirculationRules.t

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Katrin Fischer 2020-09-26 20:39:54 UTC
This one is confusing to me...

1) Andrew said he could not reproduce the problem neither in master nor in 20.05.
2) The test plan of the patch does only show the difference in the data, but doesn't explain why this change is important and how it related to the initial hold issue.
3) Same for the test - it only verifies that no rules with "" are entered.
4) There is no database update to remove problmatic(?) empty rules.

Can we have some more information here?
Comment 8 Nick Clemens 2020-09-28 13:54:50 UTC
Created attachment 110856 [details] [review]
Bug 26529: Define some rules as not able to be blank

Currently, the 'Default checkout, hold, and return policy' have a conditional to blank
'holdallowed' when not set, but other rules don't get the same treatment

This patch uses the existing hash that controls the scope of rules and adds a
new parameter 'can_be_blank' Default is assumed to be true, can_be_blank, but if set to '0' then
blank rule values will not be save to the database

To test:
 0 - Set CircControl to 'the library the item is from'
 1 - Goto to Admin -> Circulation and fines rules and select a library
 2 - Under "Default checkout, hold, and return policy" set:
    Total current checkouts allowed: 1
    Hold policy: Not set
    Hold pickup library match: Not set
    Return policy: Not set
 3 - Save the rule
 4 - Check the DB
    SELECT * FROM circulation_rules WHERE branchcode = 'BRANCHCODE'
    using the branchcode for the library you chode
 5 - Note returnbranch, and hold_fulfillment_policy rules exist with blank value
 6 - Set 'All libraries'->'Default checkout, hold and return policy'->'Item floats'
 7 - Checkin an item from the library with blank rules at another library
 8 - It returns home rather than floating
 9 - Apply patch and restart all the things
10 - Reload the page and save the rules as before
11 - Check the DB and note the rules no longer exist
12 - Check in the item, it floats!
13 - prove -v t/db_dependent/Koha/CirculationRules.t

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 9 Nick Clemens 2020-09-28 13:54:55 UTC
Created attachment 110857 [details] [review]
Bug 26529: Unit tests

This tests tries to set some rules to blank, and then fetch the effective rule - nothing should be
returned as these values are 'Not set'

To test:
1 - Apply just this patch
2 - Tests fail
3 - Apply second patch
4 - Tests pass
Comment 10 Nick Clemens 2020-09-28 13:56:41 UTC
(In reply to Katrin Fischer from comment #7)
> This one is confusing to me...
> 
> 1) Andrew said he could not reproduce the problem neither in master nor in
> 20.05.

If testing only 'holdallowed' the problem is not apparent as we catch that case in the script

> 2) The test plan of the patch does only show the difference in the data, but
> doesn't explain why this change is important and how it related to the
> initial hold issue.

Updated test plan with a test to illustrate the problem

> 3) Same for the test - it only verifies that no rules with "" are entered.

It also verifies that those rules aren't returned - run the tests on their own and they fail on master

> 4) There is no database update to remove problmatic(?) empty rules.

Added
Comment 11 Nick Clemens 2020-09-28 13:57:23 UTC
Created attachment 110859 [details] [review]
Bug 26529: Remove existing blank rules
Comment 12 Martin Renvoize 2020-10-05 16:04:05 UTC
Created attachment 111251 [details] [review]
Bug 26529: Define some rules as not able to be blank

Currently, the 'Default checkout, hold, and return policy' have a conditional to blank
'holdallowed' when not set, but other rules don't get the same treatment

This patch uses the existing hash that controls the scope of rules and adds a
new parameter 'can_be_blank' Default is assumed to be true, can_be_blank, but if set to '0' then
blank rule values will not be save to the database

To test:
 0 - Set CircControl to 'the library the item is from'
 1 - Goto to Admin -> Circulation and fines rules and select a library
 2 - Under "Default checkout, hold, and return policy" set:
    Total current checkouts allowed: 1
    Hold policy: Not set
    Hold pickup library match: Not set
    Return policy: Not set
 3 - Save the rule
 4 - Check the DB
    SELECT * FROM circulation_rules WHERE branchcode = 'BRANCHCODE'
    using the branchcode for the library you chode
 5 - Note returnbranch, and hold_fulfillment_policy rules exist with blank value
 6 - Set 'All libraries'->'Default checkout, hold and return policy'->'Item floats'
 7 - Checkin an item from the library with blank rules at another library
 8 - It returns home rather than floating
 9 - Apply patch and restart all the things
10 - Reload the page and save the rules as before
11 - Check the DB and note the rules no longer exist
12 - Check in the item, it floats!
13 - prove -v t/db_dependent/Koha/CirculationRules.t

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-10-05 16:04:08 UTC
Created attachment 111252 [details] [review]
Bug 26529: Unit tests

This tests tries to set some rules to blank, and then fetch the effective rule - nothing should be
returned as these values are 'Not set'

To test:
1 - Apply just this patch
2 - Tests fail
3 - Apply second patch
4 - Tests pass

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-10-05 16:04:12 UTC
Created attachment 111253 [details] [review]
Bug 26529: Remove existing blank rules

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-10-05 16:06:16 UTC
This all works well and I'm surprised we didn't already handle such cases.

Unit tests pass and we're dealing with the obvious cases where rules should not be blank.

I suspect there may be a few other cases crop up in the future where we've been allowing errant blank rules in, but we can handle them as we find them.

Passing QA
Comment 16 Jonathan Druart 2020-10-08 08:13:23 UTC
Should not we then remove those 2 lines?

466     $holdallowed =~ s/\s//g;                                                                         
467     $holdallowed = undef if $holdallowed !~ /^\d+/;
Comment 17 Nick Clemens 2020-10-08 17:49:18 UTC
Created attachment 111382 [details] [review]
Bug 26529: (follow-up) Remove other instance of blanking holdallowed from script
Comment 18 Jonathan Druart 2020-10-09 11:02:38 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 19 Jonathan Druart 2020-10-12 07:40:49 UTC
Created attachment 111471 [details] [review]
Bug 26529: Remove warning

Use of uninitialized value $rule_value in string eq
Comment 20 Jonathan Druart 2020-10-12 09:36:29 UTC
(In reply to Jonathan Druart from comment #19)
> Created attachment 111471 [details] [review] [review]
> Bug 26529: Remove warning
> 
> Use of uninitialized value $rule_value in string eq

Pushed to master.
Comment 21 Jonathan Druart 2020-10-12 12:49:11 UTC
Created attachment 111511 [details] [review]
Bug 26529: (follow-up) Remove warning
Comment 22 Jonathan Druart 2020-10-12 12:50:35 UTC
(In reply to Jonathan Druart from comment #21)
> Created attachment 111511 [details] [review] [review]
> Bug 26529: (follow-up) Remove warning

And, pushed to master! (silly mistake!)
Comment 23 Lucas Gass 2020-11-10 22:23:38 UTC
cannot reproduce this problem in 20.05.x, is it needed?