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.
Nor can I recreate this on 20.05
Created attachment 110686 [details] [review] Bug 26529: Set rule value to undef when not setfor branch defaults
Created attachment 110687 [details] [review] Bug 26529: Consider blank rules as undef for default hold policies
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
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>
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>
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?
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>
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
(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
Created attachment 110859 [details] [review] Bug 26529: Remove existing blank rules
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>
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>
Created attachment 111253 [details] [review] Bug 26529: Remove existing blank rules Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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
Should not we then remove those 2 lines? 466 $holdallowed =~ s/\s//g; 467 $holdallowed = undef if $holdallowed !~ /^\d+/;
Created attachment 111382 [details] [review] Bug 26529: (follow-up) Remove other instance of blanking holdallowed from script
Pushed to master for 20.11, thanks to everybody involved!
Created attachment 111471 [details] [review] Bug 26529: Remove warning Use of uninitialized value $rule_value in string eq
(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.
Created attachment 111511 [details] [review] Bug 26529: (follow-up) Remove warning
(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!)
cannot reproduce this problem in 20.05.x, is it needed?