Bug 27069 - Change holdallowed values from numbers to strings
Summary: Change holdallowed values from numbers to strings
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 1 vote (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-20 17:54 UTC by Kyle M Hall
Modified: 2021-04-14 08:05 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 27069: Adjust tests (30.18 KB, patch)
2021-03-15 18:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27069: Adapt uses of holdallowed (14.80 KB, patch)
2021-03-15 18:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27069: Update existing rules (2.42 KB, patch)
2021-03-15 18:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27069: Adjust tests (30.23 KB, patch)
2021-03-16 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27069: Adapt uses of holdallowed (14.85 KB, patch)
2021-03-16 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27069: Update existing rules (2.48 KB, patch)
2021-03-16 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27069: Adjust tests (30.30 KB, patch)
2021-04-06 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27069: Adapt uses of holdallowed (14.92 KB, patch)
2021-04-06 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27069: Update existing rules (2.55 KB, patch)
2021-04-06 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27069: (follow-up) Only display rules if one is set (1.83 KB, patch)
2021-04-12 13:26 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.