|
Description
Andrew Fuerste-Henry
2020-09-24 15:07:22 UTC
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? |