Right now, the values for holdallowed are numeric, 1 = from home library, 2 = from any library, 3 = from local hold group. I think it would help developers if we replaced the numbers with those strings, so 1 => "from_home_library", 2 => "from_any_library", and 3 => "from_local_hold_group".
Holds rules are complex enough to get lost fairly easily. No need to ALSO remember what each integer means :-D
Created attachment 118252 [details] [review] Bug 27069: Adjust tests
Created attachment 118253 [details] [review] Bug 27069: Adapt uses of holdallowed
Created attachment 118254 [details] [review] Bug 27069: Update existing rules This patch adds an atomic update file that takes care of translating existing rules. It constrains the rules to translate to those currently expected by Koha and leaves any other value out. To test: 1. Have some manually added rules: $ koha-mysql kohadev > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', -1); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 1); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 2); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 3); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 0); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 4); 2. Apply this patch 3. Run: $ updatedatabase => SUCCESS: It doesn't explode 4. Verify the created rules were updated correctly: > SELECT * FROM circulation_rules WHERE rule_name='holdallowed'; 5: Verify all the tests that dealt with this rule still pass! $ kshell k$ git diff origin/master --name-only | grep -e '\.t$' | \ xargs prove => SUCCESS: Tests pass! 6. Verify the UI handles setting things correctly 7. Sign off :-D
Created attachment 118324 [details] [review] Bug 27069: Adjust tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118325 [details] [review] Bug 27069: Adapt uses of holdallowed Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118326 [details] [review] Bug 27069: Update existing rules This patch adds an atomic update file that takes care of translating existing rules. It constrains the rules to translate to those currently expected by Koha and leaves any other value out. To test: 1. Have some manually added rules: $ koha-mysql kohadev > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', -1); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 1); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 2); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 3); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 0); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 4); 2. Apply this patch 3. Run: $ updatedatabase => SUCCESS: It doesn't explode 4. Verify the created rules were updated correctly: > SELECT * FROM circulation_rules WHERE rule_name='holdallowed'; 5: Verify all the tests that dealt with this rule still pass! $ kshell k$ git diff origin/master --name-only | grep -e '\.t$' | \ xargs prove => SUCCESS: Tests pass! 6. Verify the UI handles setting things correctly 7. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice improvement, works as expected.. Signing off
Created attachment 119223 [details] [review] Bug 27069: Adjust tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119224 [details] [review] Bug 27069: Adapt uses of holdallowed Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 119225 [details] [review] Bug 27069: Update existing rules This patch adds an atomic update file that takes care of translating existing rules. It constrains the rules to translate to those currently expected by Koha and leaves any other value out. To test: 1. Have some manually added rules: $ koha-mysql kohadev > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', -1); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 1); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 2); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 3); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 0); > INSERT INTO circulation_rules (rule_name,rule_value) VALUES ('holdallowed', 4); 2. Apply this patch 3. Run: $ updatedatabase => SUCCESS: It doesn't explode 4. Verify the created rules were updated correctly: > SELECT * FROM circulation_rules WHERE rule_name='holdallowed'; 5: Verify all the tests that dealt with this rule still pass! $ kshell k$ git diff origin/master --name-only | grep -e '\.t$' | \ xargs prove => SUCCESS: Tests pass! 6. Verify the UI handles setting things correctly 7. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 21.05, thanks to everybody involved!
Created attachment 119454 [details] [review] Bug 27069: (follow-up) Only display rules if one is set Testing that the rule is != 'hold_allowed' is true for each itemtype where the rule is not set. This means it appears that no holds are allowed for any item type if no rules are set To test: 1 - With a fresh testing DB or no holdallowed rule set, you can use SQl below to clear: DELETE from circuation_rules WHERE rule_name='holdallowed'; 2 - Load smart-rules.pl (Administration->Circulation and fines rules) 3 - Scroll to "Default holds policy by item type" 4 - Note all item types appear as 'No holds allowed' 5 - Apply patch 6 - Reload 7 - All clear 8 - Confirm adding rules displays correctly
Follow-up pushed to master.
Enhancement not pushed to 20.11.x