[Bug 26529] New: Holds rules enforced incorrectly when not set at library level
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Bug ID: 26529 Summary: Holds rules enforced incorrectly when not set at library level Change sponsored?: --- Product: Koha Version: 19.11 Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: andrew@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #1 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Nor can I recreate this on 20.05 -- 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=26529 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 110686 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110686&action=edit Bug 26529: Set rule value to undef when not setfor branch defaults -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110686|0 |1 is obsolete| | --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 110687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110687&action=edit Bug 26529: Consider blank rules as undef for default hold policies -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 110695 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110695&action=edit 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 -- 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=26529 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|19.11 |master CC| |nick@bywatersolutions.com Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 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=26529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110687|0 |1 is obsolete| | --- Comment #5 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 110714 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110714&action=edit Bug 26529: Consider blank rules as undef for default hold policies 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=26529 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110695|0 |1 is obsolete| | --- Comment #6 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 110715 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110715&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110714|0 |1 is obsolete| | Attachment #110715|0 |1 is obsolete| | --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 110856 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110856&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 110857 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110857&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #10 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 110859 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110859&action=edit Bug 26529: Remove existing blank rules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 George Williams (NEKLS) <george@nekls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |george@nekls.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110856|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 111251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111251&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110857|0 |1 is obsolete| | --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 111252 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111252&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110859|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 111253 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111253&action=edit Bug 26529: Remove existing blank rules 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=26529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #15 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Should not we then remove those 2 lines? 466 $holdallowed =~ s/\s//g; 467 $holdallowed = undef if $holdallowed !~ /^\d+/; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 111382 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111382&action=edit Bug 26529: (follow-up) Remove other instance of blanking holdallowed from script -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 20.11, 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=26529 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 111471 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111471&action=edit Bug 26529: Remove warning Use of uninitialized value $rule_value in string eq -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #19)
Created attachment 111471 [details] [review] Bug 26529: Remove warning
Use of uninitialized value $rule_value in string eq
Pushed to master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 111511 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111511&action=edit Bug 26529: (follow-up) Remove warning -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #21)
Created attachment 111511 [details] [review] Bug 26529: (follow-up) Remove warning
And, pushed to master! (silly mistake!) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #23 from Lucas Gass <lucas@bywatersolutions.com> --- cannot reproduce this problem in 20.05.x, is it needed? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Niamh Walker-Headon <Niamh.WalkerHeadon@hse.ie> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Niamh.WalkerHeadon@hse.ie -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26529 Thibaud Guillot <thibaud.guillot@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=35458 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org