Bug 27069

Summary: Change holdallowed values from numbers to strings
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: agustinmoyano, fridolin.somers, jonathan.druart, kyle, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Bug Depends on:    
Bug Blocks: 28169    
Attachments: Bug 27069: Adjust tests
Bug 27069: Adapt uses of holdallowed
Bug 27069: Update existing rules
Bug 27069: Adjust tests
Bug 27069: Adapt uses of holdallowed
Bug 27069: Update existing rules
Bug 27069: Adjust tests
Bug 27069: Adapt uses of holdallowed
Bug 27069: Update existing rules
Bug 27069: (follow-up) Only display rules if one is set

Description Kyle M Hall 2020-11-20 17:54:45 UTC
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".
Comment 1 Tomás Cohen Arazi 2021-03-15 18:42:39 UTC
Holds rules are complex enough to get lost fairly easily. No need to ALSO remember what each integer means :-D
Comment 2 Tomás Cohen Arazi 2021-03-15 18:44:38 UTC
Created attachment 118252 [details] [review]
Bug 27069: Adjust tests
Comment 3 Tomás Cohen Arazi 2021-03-15 18:44:43 UTC
Created attachment 118253 [details] [review]
Bug 27069: Adapt uses of holdallowed
Comment 4 Tomás Cohen Arazi 2021-03-15 18:44:48 UTC
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
Comment 5 Martin Renvoize 2021-03-16 14:20:37 UTC
Created attachment 118324 [details] [review]
Bug 27069: Adjust tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-03-16 14:20:40 UTC
Created attachment 118325 [details] [review]
Bug 27069: Adapt uses of holdallowed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-03-16 14:20:43 UTC
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>
Comment 8 Martin Renvoize 2021-03-16 14:21:03 UTC
Nice improvement, works as expected.. Signing off
Comment 9 Kyle M Hall 2021-04-06 15:14:13 UTC
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>
Comment 10 Kyle M Hall 2021-04-06 15:14:25 UTC
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>
Comment 11 Kyle M Hall 2021-04-06 15:14:27 UTC
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>
Comment 12 Jonathan Druart 2021-04-07 14:38:24 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 13 Nick Clemens 2021-04-12 13:26:06 UTC
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
Comment 14 Jonathan Druart 2021-04-14 08:05:45 UTC
Follow-up pushed to master.
Comment 15 Fridolin Somers 2021-04-19 10:06:01 UTC
Enhancement not pushed to 20.11.x