Bug 18928 - Move `holdallowed`, `hold_fulfillment_policy` and `returnbranch` into the `circulation_rules` table.
Summary: Move `holdallowed`, `hold_fulfillment_policy` and `returnbranch` into the `ci...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18925 22847
Blocks: 18930 23484 25601 25851
  Show dependency treegraph
 
Reported: 2017-07-11 14:04 UTC by Kyle M Hall
Modified: 2021-06-14 21:28 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
**Reports note**: This changes the database schema, reports referencing the `default_branch_circ_rules`, `default_circ_rules`, `default_branch_item_rules` or `branch_item_rules` tables will need to be updated
Version(s) released in:
19.11.00


Attachments
Bug 18928 - Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (61.42 KB, patch)
2017-07-25 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928 - Remove schema files for deleted tables (9.57 KB, patch)
2017-07-25 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928 - Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.35 KB, patch)
2018-01-29 00:07 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 18928 - Remove schema files for deleted tables (9.59 KB, patch)
2018-01-29 00:07 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.37 KB, patch)
2018-10-24 22:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.15 KB, patch)
2018-10-24 22:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Update schema files (12.72 KB, patch)
2018-10-24 22:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.79 KB, patch)
2018-10-24 22:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.37 KB, patch)
2018-12-06 18:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.16 KB, patch)
2018-12-06 18:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Update schema files (12.72 KB, patch)
2018-12-06 18:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.79 KB, patch)
2018-12-06 18:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.43 KB, patch)
2019-02-21 20:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.16 KB, patch)
2019-02-21 20:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.79 KB, patch)
2019-02-21 20:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Update schema files (12.69 KB, patch)
2019-02-21 20:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (5.62 KB, patch)
2019-02-21 20:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.43 KB, patch)
2019-03-06 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.41 KB, patch)
2019-03-06 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.84 KB, patch)
2019-03-06 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (5.65 KB, patch)
2019-03-06 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 18928: DBIC schema update (3.44 KB, patch)
2019-03-06 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.34 KB, patch)
2019-03-06 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.42 KB, patch)
2019-04-24 21:16 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.48 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.21 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.84 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (7.90 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.35 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.42 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 18928: DBIC schema update (3.40 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Use get_effective_rule in GetBranchItemRule (3.30 KB, patch)
2019-04-29 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.50 KB, patch)
2019-05-03 21:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.53 KB, patch)
2019-05-03 21:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones) (2.14 KB, patch)
2019-05-03 21:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add Branches.HasCirculationRules and set branchcode in CirculationRules.Search (5.06 KB, patch)
2019-05-04 07:25 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.54 KB, patch)
2019-05-04 07:30 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.26 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.90 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (7.96 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.40 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.48 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: DBIC schema update (3.44 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Use get_effective_rule in GetBranchItemRule (3.35 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.58 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones) (2.20 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Add Branches.HasCirculationRules and set branchcode in CirculationRules.Search (5.04 KB, patch)
2019-05-04 07:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Remove '(not set)' (1.61 KB, patch)
2019-05-06 17:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Remove '(not set)' (1.66 KB, patch)
2019-05-06 23:23 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.53 KB, patch)
2019-05-07 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.50 KB, patch)
2019-05-07 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.53 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.97 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (8.01 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.46 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.55 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: DBIC schema update (3.51 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: Use get_effective_rule in GetBranchItemRule (3.41 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.66 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones) (2.27 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18928: Remove '(not set)' (1.73 KB, patch)
2019-05-07 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* (4.26 KB, patch)
2019-05-07 15:09 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.58 KB, patch)
2019-05-07 15:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.31 KB, patch)
2019-05-07 15:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.94 KB, patch)
2019-05-07 15:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (8.00 KB, patch)
2019-05-07 15:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.44 KB, patch)
2019-05-07 15:10 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.52 KB, patch)
2019-05-07 15:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: DBIC schema update (3.49 KB, patch)
2019-05-07 15:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Use get_effective_rule in GetBranchItemRule (3.39 KB, patch)
2019-05-07 15:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.63 KB, patch)
2019-05-07 15:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones) (2.24 KB, patch)
2019-05-07 15:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Remove '(not set)' (1.70 KB, patch)
2019-05-07 15:11 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.58 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.31 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Add missing filters (1.94 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (7.98 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.44 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.52 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: DBIC schema update (3.49 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Use get_effective_rule in GetBranchItemRule (3.39 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.63 KB, patch)
2019-05-12 00:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones) (2.24 KB, patch)
2019-05-12 00:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Remove '(not set)' (1.70 KB, patch)
2019-05-12 00:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up 2) Display correctly the values (not the propagated ones) (1.68 KB, patch)
2019-06-11 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules (60.64 KB, patch)
2019-06-28 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: Add DB structure changes (4.36 KB, patch)
2019-06-28 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: Add missing filters (2.00 KB, patch)
2019-06-28 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: Update new occurrences (8.03 KB, patch)
2019-06-28 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: (follow-up) Make DB update idempotent (8.50 KB, patch)
2019-06-28 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: (follow-up) Unset max_holds rule if needed (1.58 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: DBIC schema update (3.54 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: Use get_effective_rule in GetBranchItemRule (3.45 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule (2.68 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones) (2.30 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: Remove '(not set)' (1.76 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: (bug 22847 follow-up 2) Display correctly the values (not the propagated ones) (1.73 KB, patch)
2019-06-28 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18928: (QA follow-up) Remove DBIC files and reference to DefaultCircRule (10.15 KB, patch)
2019-06-28 14:57 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 2017-07-11 14:04:26 UTC

    
Comment 1 Kyle M Hall 2017-07-25 15:28:28 UTC
Created attachment 65241 [details] [review]
Bug 18928 - Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged
Comment 2 Kyle M Hall 2017-07-25 15:28:35 UTC
Created attachment 65242 [details] [review]
Bug 18928 - Remove schema files for deleted tables
Comment 3 Jesse Weaver 2018-01-29 00:07:18 UTC
Created attachment 71005 [details] [review]
Bug 18928 - Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged
Comment 4 Jesse Weaver 2018-01-29 00:07:20 UTC
Created attachment 71006 [details] [review]
Bug 18928 - Remove schema files for deleted tables
Comment 5 Jonathan Druart 2018-10-24 22:40:58 UTC
Created attachment 81096 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged
Comment 6 Jonathan Druart 2018-10-24 22:41:03 UTC
Created attachment 81097 [details] [review]
Bug 18928: Add DB structure changes
Comment 7 Jonathan Druart 2018-10-24 22:41:08 UTC
Created attachment 81098 [details] [review]
Bug 18928: Update schema files
Comment 8 Jonathan Druart 2018-10-24 22:41:12 UTC
Created attachment 81099 [details] [review]
Bug 18928: Add missing filters
Comment 9 Jonathan Druart 2018-12-06 18:51:16 UTC
Created attachment 82933 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged
Comment 10 Jonathan Druart 2018-12-06 18:51:20 UTC
Created attachment 82934 [details] [review]
Bug 18928: Add DB structure changes
Comment 11 Jonathan Druart 2018-12-06 18:51:24 UTC
Created attachment 82935 [details] [review]
Bug 18928: Update schema files
Comment 12 Jonathan Druart 2018-12-06 18:51:30 UTC
Created attachment 82936 [details] [review]
Bug 18928: Add missing filters
Comment 13 Jonathan Druart 2019-02-21 20:28:37 UTC
Created attachment 85487 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged
Comment 14 Jonathan Druart 2019-02-21 20:28:43 UTC
Created attachment 85488 [details] [review]
Bug 18928: Add DB structure changes
Comment 15 Jonathan Druart 2019-02-21 20:28:47 UTC
Created attachment 85489 [details] [review]
Bug 18928: Add missing filters
Comment 16 Jonathan Druart 2019-02-21 20:28:52 UTC
Created attachment 85490 [details] [review]
Bug 18928: Update schema files
Comment 17 Jonathan Druart 2019-02-21 20:28:56 UTC
Created attachment 85491 [details] [review]
Bug 18928: Update new occurrences
Comment 18 Tomás Cohen Arazi 2019-03-06 12:54:01 UTC
Created attachment 86134 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2019-03-06 12:54:06 UTC
Created attachment 86135 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2019-03-06 12:54:10 UTC
Created attachment 86136 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2019-03-06 12:54:14 UTC
Created attachment 86137 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2019-03-06 12:54:18 UTC
Created attachment 86138 [details] [review]
[DO NOT PUSH] Bug 18928: DBIC schema update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2019-03-06 12:54:22 UTC
Created attachment 86139 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2019-03-06 13:18:44 UTC
I've:
- Tried setting global and branch-specific rules
- Upgrade migrated the rules correctly
- Tests pass
- QA scripts happy

Good job!
Comment 25 Josef Moravec 2019-04-24 21:10:03 UTC
Comment on attachment 86134 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Review of attachment 86134 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +1706,5 @@
> +
> +    # Search for rules!
> +    foreach my $rule_name (qw( holdallowed hold_fulfillment_policy returnbranch )) {
> +        foreach my $params (@params) {
> +            my $rule = Koha::CirculationRules->search(

Why do you use ->search instead of get_effective_rule?

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
@@ +720,4 @@
>                      <th>Return policy</th>
>                      <th>&nbsp;</th>
>                  </tr>
> +                [% FOREACH i IN itemtypeloop %]

There is something strange in displaying hold policy for itemtypes. When I have set values in Default checkout, hold and return policy section, i see all item types listed with the same value as set in Default section, even if i set some itemtypes differently...
Comment 26 Josef Moravec 2019-04-24 21:16:07 UTC
Created attachment 88659 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too
Comment 27 Jonathan Druart 2019-04-29 15:21:21 UTC
Created attachment 89017 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Jonathan Druart 2019-04-29 15:21:24 UTC
Created attachment 89018 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Jonathan Druart 2019-04-29 15:21:28 UTC
Created attachment 89019 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2019-04-29 15:21:33 UTC
Created attachment 89020 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Jonathan Druart 2019-04-29 15:21:36 UTC
Created attachment 89021 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Jonathan Druart 2019-04-29 15:21:40 UTC
Created attachment 89022 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too
Comment 33 Jonathan Druart 2019-04-29 15:21:43 UTC
Created attachment 89023 [details] [review]
[DO NOT PUSH] Bug 18928: DBIC schema update
Comment 34 Jonathan Druart 2019-04-29 15:21:47 UTC
Created attachment 89024 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule
Comment 35 Jonathan Druart 2019-04-29 15:22:58 UTC
(In reply to Josef Moravec from comment #25)
> Comment on attachment 86134 [details] [review] [review]
> Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to
> circulation_rules
> 
> Review of attachment 86134 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Circulation.pm
> @@ +1706,5 @@
> > +
> > +    # Search for rules!
> > +    foreach my $rule_name (qw( holdallowed hold_fulfillment_policy returnbranch )) {
> > +        foreach my $params (@params) {
> > +            my $rule = Koha::CirculationRules->search(
> 
> Why do you use ->search instead of get_effective_rule?

Yes you are are right, it seems to make sense to use get_effective_rule. Done in a follow-up.

> ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
> @@ +720,4 @@
> >                      <th>Return policy</th>
> >                      <th>&nbsp;</th>
> >                  </tr>
> > +                [% FOREACH i IN itemtypeloop %]
> 
> There is something strange in displaying hold policy for itemtypes. When I
> have set values in Default checkout, hold and return policy section, i see
> all item types listed with the same value as set in Default section, even if
> i set some itemtypes differently...

I cannot recreate that, can you share screenshots?

I have attached a rebased version of these patches.
Comment 36 Liz Rea 2019-04-30 14:49:19 UTC
Hi,

There is something odd here - the Defaults row seems to always say (not set) even if the rules are propagated?

Deleting the All-All rule from the main table seems to clear the default checkout, hold, and return policy values? Is that expected? You can re-set the checkout hold and return policy rules without having an All-All rule, but deleting it seems to delete both? 

Probably not going to go on testing the actual function until I get an answer on these things, sorry.

Cheers,
Liz
Comment 37 Jonathan Druart 2019-05-03 21:12:34 UTC
Created attachment 89333 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty and maxonsiteissueqty (for now)
Comment 38 Jonathan Druart 2019-05-03 21:14:24 UTC
(In reply to Liz Rea from comment #36)
> Hi,

Hi Liz, thanks a lot for testing!

> There is something odd here - the Defaults row seems to always say (not set)
> even if the rules are propagated?

That's definitely wrong, and is happening before this patch. I am going to investigate and open a new bug report.

> Deleting the All-All rule from the main table seems to clear the default
> checkout, hold, and return policy values? Is that expected? You can re-set
> the checkout hold and return policy rules without having an All-All rule,
> but deleting it seems to delete both? 

Good catch, caused by bug 22679, following up here as it should not have side-effects in master.
Comment 39 Jonathan Druart 2019-05-03 21:19:29 UTC
Created attachment 89334 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty, maxonsiteissueqty and max_holds (for now)
Comment 40 Jonathan Druart 2019-05-03 21:38:46 UTC
(In reply to Jonathan Druart from comment #38)
> (In reply to Liz Rea from comment #36)
> > There is something odd here - the Defaults row seems to always say (not set)
> > even if the rules are propagated?
> 
> That's definitely wrong, and is happening before this patch. I am going to
> investigate and open a new bug report.

See bug 22847.
Comment 41 Jonathan Druart 2019-05-03 21:39:47 UTC
Created attachment 89336 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)
Comment 42 Jonathan Druart 2019-05-03 21:40:20 UTC
And I think we are good now.
Comment 43 Agustín Moyano 2019-05-04 07:25:32 UTC
Created attachment 89338 [details] [review]
Bug 18928: Add Branches.HasCirculationRules and set branchcode in CirculationRules.Search

This patch tackles two issues:
1) Defaults row always says (not set) because "default_rules" param was deleted from smart-rules.pl.
2) When setting standard hold rules for all libraries, hold policy, pickup and return always says 'Not Set' even if you defined values. This is because "current_branch" variable was used in CirculationRules.Search, and current_branch value for default is "*".

Solutions:
1) Add Koha::Template::Plugin::Branches->HasCirculationRules method that returns 1 if defined branch has circulation rules.
2) Switch "current_branch" for "branchcode" whose value is undef for default rules.

To test:
1) Apply previous patches
2) set default values for all libraries
CHECK => hold policy, pickup and return remains in 'Not set'
3) select a library
4) set values for that library
CHECK => first column always says "Defaults (not set)"
5) Apply this patch
SUCCESS => Hold, pickup and return default values for all libraries are now shown
	=> First column of default rules, and of the library selected in step 3 only says "Defaults"
6) Sign off
Comment 44 Agustín Moyano 2019-05-04 07:30:55 UTC
Created attachment 89339 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 45 Agustín Moyano 2019-05-04 07:31:00 UTC
Created attachment 89340 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 46 Agustín Moyano 2019-05-04 07:31:04 UTC
Created attachment 89341 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 47 Agustín Moyano 2019-05-04 07:31:08 UTC
Created attachment 89342 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 48 Agustín Moyano 2019-05-04 07:31:13 UTC
Created attachment 89343 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 49 Agustín Moyano 2019-05-04 07:31:16 UTC
Created attachment 89344 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 50 Agustín Moyano 2019-05-04 07:31:20 UTC
Created attachment 89345 [details] [review]
Bug 18928: DBIC schema update

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 51 Agustín Moyano 2019-05-04 07:31:24 UTC
Created attachment 89346 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 52 Agustín Moyano 2019-05-04 07:31:28 UTC
Created attachment 89347 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty, maxonsiteissueqty and max_holds (for now)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 53 Agustín Moyano 2019-05-04 07:31:32 UTC
Created attachment 89348 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 54 Agustín Moyano 2019-05-04 07:31:36 UTC
Created attachment 89349 [details] [review]
Bug 18928: Add Branches.HasCirculationRules and set branchcode in CirculationRules.Search

This patch tackles two issues:
1) Defaults row always says (not set) because "default_rules" param was deleted from smart-rules.pl.
2) When setting standard hold rules for all libraries, hold policy, pickup and return always says 'Not Set' even if you defined values. This is because "current_branch" variable was used in CirculationRules.Search, and current_branch value for default is "*".

Solutions:
1) Add Koha::Template::Plugin::Branches->HasCirculationRules method that returns 1 if defined branch has circulation rules.
2) Switch "current_branch" for "branchcode" whose value is undef for default rules.

To test:
1) Apply previous patches
2) set default values for all libraries
CHECK => hold policy, pickup and return remains in 'Not set'
3) select a library
4) set values for that library
CHECK => first column always says "Defaults (not set)"
5) Apply this patch
SUCCESS => Hold, pickup and return default values for all libraries are now shown
	=> First column of default rules, and of the library selected in step 3 only says "Defaults"
6) Sign off
Comment 55 Jonathan Druart 2019-05-04 13:18:50 UTC
(In reply to Agustín Moyano from comment #54)
> Created attachment 89349 [details] [review] [review]

Hola Agustín, are you sure this patch is still needed after the 2 follow-ups I attached yesterday?
Comment 56 Agustín Moyano 2019-05-04 15:56:58 UTC
(In reply to Jonathan Druart from comment #55)
> (In reply to Agustín Moyano from comment #54)
> > Created attachment 89349 [details] [review] [review] [review]
> 
> Hola Agustín, are you sure this patch is still needed after the 2 follow-ups
> I attached yesterday?

Hello Jonathan, yesterday night I tested it with the parches, and found those tiny errors.
Comment 57 Agustín Moyano 2019-05-04 22:28:15 UTC
(In reply to Agustín Moyano from comment #56)
> (In reply to Jonathan Druart from comment #55)
> > (In reply to Agustín Moyano from comment #54)
> > > Created attachment 89349 [details] [review] [review] [review] [review]
> > 
> > Hola Agustín, are you sure this patch is still needed after the 2 follow-ups
> > I attached yesterday?
> 
> Hello Jonathan, yesterday night I tested it with the parches, and found
> those tiny errors.

s/parches/patches/g

Sorry.. my mobile spanish corrector :p
Comment 58 Jonathan Druart 2019-05-05 14:07:58 UTC
Agustin, ok I think I am lost then :)

I opened bug 22847 to deal with this behavior, and I thought it was a bug. I am not sure what we expect actually. I am going to mark this one as blocked by 22847 and see how thing goes.

About your patch:
* HasCirculationRules - I would have set the flag from the .pl, not the TT plugin.
* current_branch vs branchcode - my patch was supposed to deal with that.

We will be back to this one later :)
Comment 59 Jonathan Druart 2019-05-05 14:11:19 UTC
(In reply to Jonathan Druart from comment #58)
> I opened bug 22847 to deal with this behavior, and I thought it was a bug. I
> am not sure what we expect actually. I am going to mark this one as blocked
> by 22847 and see how thing goes.

And I have tested on 17.11.x: nothing is displayed if the rule is not defined at library level, so I think bug 22847 is valid and must be fixed asap.
Comment 60 Jonathan Druart 2019-05-06 17:09:58 UTC
So, if we agree that bug 22847 is valid, last patch must be obsoleted.

However the "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default checkout, hold and return policy for Centerville" table), it will be harder to handle it correctly. Not impossible, but if not really needed I would prefer to just remove it. If a rule is not set, the cell is empty or the dropdown list displays the option "Not set", which seems to correct the problem.
Comment 61 Jonathan Druart 2019-05-06 17:12:08 UTC
Created attachment 89377 [details] [review]
Bug 18928: Remove '(not set)'

The "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
checkout, hold and return policy for Centerville" table), it will be harder
to handle it correctly. Not impossible, but if not really needed I would prefer
to just remove it. If a rule is not set, the cell is empty or the dropdown list
displays the option "Not set", which seems to correct the problem.
Comment 62 Agustín Moyano 2019-05-06 18:07:22 UTC
(In reply to Jonathan Druart from comment #61)
> Created attachment 89377 [details] [review] [review]
> Bug 18928: Remove '(not set)'
> 
> The "(not set)" part must be removed IMO, I am not sure I understand its
> goal.
> Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
> checkout, hold and return policy for Centerville" table), it will be harder
> to handle it correctly. Not impossible, but if not really needed I would
> prefer
> to just remove it. If a rule is not set, the cell is empty or the dropdown
> list
> displays the option "Not set", which seems to correct the problem.

Now that bug 22847 is a dependency of this bug, should't attachment 89348 [details] [review] be obsoleted?
Comment 63 Agustín Moyano 2019-05-06 23:23:44 UTC
Created attachment 89402 [details] [review]
Bug 18928: Remove '(not set)'

The "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
checkout, hold and return policy for Centerville" table), it will be harder
to handle it correctly. Not impossible, but if not really needed I would prefer
to just remove it. If a rule is not set, the cell is empty or the dropdown list
displays the option "Not set", which seems to correct the problem.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 64 Kyle M Hall 2019-05-07 14:23:58 UTC
Created attachment 89427 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 65 Kyle M Hall 2019-05-07 14:24:44 UTC
Created attachment 89428 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 66 Kyle M Hall 2019-05-07 14:25:02 UTC
Created attachment 89429 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 67 Kyle M Hall 2019-05-07 14:25:06 UTC
Created attachment 89430 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Kyle M Hall 2019-05-07 14:25:09 UTC
Created attachment 89431 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Kyle M Hall 2019-05-07 14:25:12 UTC
Created attachment 89432 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Kyle M Hall 2019-05-07 14:25:15 UTC
Created attachment 89433 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Kyle M Hall 2019-05-07 14:25:18 UTC
Created attachment 89434 [details] [review]
Bug 18928: DBIC schema update

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall 2019-05-07 14:25:21 UTC
Created attachment 89435 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Kyle M Hall 2019-05-07 14:25:24 UTC
Created attachment 89436 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty, maxonsiteissueqty and max_holds (for now)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Kyle M Hall 2019-05-07 14:25:27 UTC
Created attachment 89437 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Kyle M Hall 2019-05-07 14:25:31 UTC
Created attachment 89438 [details] [review]
Bug 18928: Remove '(not set)'

The "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
checkout, hold and return policy for Centerville" table), it will be harder
to handle it correctly. Not impossible, but if not really needed I would prefer
to just remove it. If a rule is not set, the cell is empty or the dropdown list
displays the option "Not set", which seems to correct the problem.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Liz Rea 2019-05-07 15:09:22 UTC
Created attachment 89440 [details] [review]
Bug 22847: Correctly displayed circ rule values for max_holds and maxissue*

There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank.

I have trouble finding the root of the issue, but I will propose a patch to fix both problem.

Certainly a mismatch between bug 18887 and bug 18925.

Test plan:
0. Do not apply the patch
1. Notice the issue on the circulation rules screen
2. Apply the patch
3. Confirm that the value you set are correctly displayed (and not the propagated ones!)

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 77 Liz Rea 2019-05-07 15:10:37 UTC
Created attachment 89441 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 78 Liz Rea 2019-05-07 15:10:46 UTC
Created attachment 89442 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 79 Liz Rea 2019-05-07 15:10:50 UTC
Created attachment 89443 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 80 Liz Rea 2019-05-07 15:10:53 UTC
Created attachment 89444 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 81 Liz Rea 2019-05-07 15:10:57 UTC
Created attachment 89445 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 82 Liz Rea 2019-05-07 15:11:01 UTC
Created attachment 89446 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 83 Liz Rea 2019-05-07 15:11:04 UTC
Created attachment 89447 [details] [review]
Bug 18928: DBIC schema update

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 84 Liz Rea 2019-05-07 15:11:08 UTC
Created attachment 89448 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 85 Liz Rea 2019-05-07 15:11:11 UTC
Created attachment 89449 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty, maxonsiteissueqty and max_holds (for now)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 86 Liz Rea 2019-05-07 15:11:15 UTC
Created attachment 89450 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 87 Liz Rea 2019-05-07 15:11:18 UTC
Created attachment 89451 [details] [review]
Bug 18928: Remove '(not set)'

The "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
checkout, hold and return policy for Centerville" table), it will be harder
to handle it correctly. Not impossible, but if not really needed I would prefer
to just remove it. If a rule is not set, the cell is empty or the dropdown list
displays the option "Not set", which seems to correct the problem.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 88 Katrin Fischer 2019-05-11 19:18:46 UTC
I a big patch set I am not very brave - can you please rebase?
Comment 89 Jonathan Druart 2019-05-12 00:05:24 UTC
Created attachment 89628 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 90 Jonathan Druart 2019-05-12 00:05:28 UTC
Created attachment 89629 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 91 Jonathan Druart 2019-05-12 00:05:32 UTC
Created attachment 89630 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 92 Jonathan Druart 2019-05-12 00:05:37 UTC
Created attachment 89631 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 93 Jonathan Druart 2019-05-12 00:05:41 UTC
Created attachment 89632 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 94 Jonathan Druart 2019-05-12 00:05:46 UTC
Created attachment 89633 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 95 Jonathan Druart 2019-05-12 00:05:50 UTC
Created attachment 89634 [details] [review]
Bug 18928: DBIC schema update

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 96 Jonathan Druart 2019-05-12 00:05:54 UTC
Created attachment 89635 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 97 Jonathan Druart 2019-05-12 00:05:59 UTC
Created attachment 89636 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty, maxonsiteissueqty and max_holds (for now)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 98 Jonathan Druart 2019-05-12 00:06:04 UTC
Created attachment 89637 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 99 Jonathan Druart 2019-05-12 00:06:08 UTC
Created attachment 89638 [details] [review]
Bug 18928: Remove '(not set)'

The "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
checkout, hold and return policy for Centerville" table), it will be harder
to handle it correctly. Not impossible, but if not really needed I would prefer
to just remove it. If a rule is not set, the cell is empty or the dropdown list
displays the option "Not set", which seems to correct the problem.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 100 Nick Clemens 2019-06-11 12:39:01 UTC
Some feedback:
t/db_dependent/TestBuilder.t - occurrence of default_circ_rules

Set rule:
All-libraries
Default holds policy by item type
Computer files -> From home library -> Item's home libary -> Item floats

Go to a specific branch 'Fairview' - the above rule shows
Setting a branch specific does override and then show correctly


Set rule:
Checkout, hold policy by patron category for Fairview
Teacher -> blank -> blank -> 0
rule does not show as set, check the DB, it is set correctly
It does prevent teacher from placing holds
set to:
Teacher -> blank -> blank -> 1
It shows and is set correctly

Otherwise testing worked, code functions as I believe it is supposed to, overriding and choosing correct rule. We should move this one early this cycle
Comment 101 Jonathan Druart 2019-06-11 15:04:35 UTC
(In reply to Nick Clemens from comment #100)
> Some feedback:
> t/db_dependent/TestBuilder.t - occurrence of default_circ_rules

Sure? I do not have it :)

> Set rule:
> All-libraries
> Default holds policy by item type
> Computer files -> From home library -> Item's home libary -> Item floats
> 
> Go to a specific branch 'Fairview' - the above rule shows
> Setting a branch specific does override and then show correctly

Yes, that's the error we have since the beginning, patch's coming.
Thanks!

> Set rule:
> Checkout, hold policy by patron category for Fairview
> Teacher -> blank -> blank -> 0
> rule does not show as set, check the DB, it is set correctly
> It does prevent teacher from placing holds
> set to:
> Teacher -> blank -> blank -> 1
> It shows and is set correctly
> 
> Otherwise testing worked, code functions as I believe it is supposed to,
> overriding and choosing correct rule. We should move this one early this
> cycle

I'd say it's a master bug, I am investigating it.
Comment 102 Jonathan Druart 2019-06-11 15:06:56 UTC
Created attachment 90501 [details] [review]
Bug 18928: (bug 22847 follow-up 2) Display correctly the values (not the propagated ones)
Comment 103 Jonathan Druart 2019-06-11 15:48:02 UTC
(In reply to Jonathan Druart from comment #101)
> > Set rule:
> > Checkout, hold policy by patron category for Fairview
> > Teacher -> blank -> blank -> 0
> > rule does not show as set, check the DB, it is set correctly
> > It does prevent teacher from placing holds
> > set to:
> > Teacher -> blank -> blank -> 1
> > It shows and is set correctly
> > 
> > Otherwise testing worked, code functions as I believe it is supposed to,
> > overriding and choosing correct rule. We should move this one early this
> > cycle
> 
> I'd say it's a master bug, I am investigating it.

See bug 23095.
Comment 104 Nick Clemens 2019-06-28 14:56:43 UTC
Created attachment 91082 [details] [review]
Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules

Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 105 Nick Clemens 2019-06-28 14:56:47 UTC
Created attachment 91083 [details] [review]
Bug 18928: Add DB structure changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 106 Nick Clemens 2019-06-28 14:56:50 UTC
Created attachment 91084 [details] [review]
Bug 18928: Add missing filters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 107 Nick Clemens 2019-06-28 14:56:54 UTC
Created attachment 91085 [details] [review]
Bug 18928: Update new occurrences

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 108 Nick Clemens 2019-06-28 14:56:58 UTC
Created attachment 91086 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 109 Nick Clemens 2019-06-28 14:57:01 UTC
Created attachment 91087 [details] [review]
Bug 18928: (follow-up) Unset max_holds rule if needed

Test plan:
1) Set all values in Default checkout, hold and return policy section on
circulation rules admin page, try for all libraries and for one specific
library. Save.
2) Click on unset and confitm
--> without patch, the Maximum total holds allowed value remains
--> with patch the unset removex Maximum holds value too

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 110 Nick Clemens 2019-06-28 14:57:06 UTC
Created attachment 91088 [details] [review]
Bug 18928: DBIC schema update

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 111 Nick Clemens 2019-06-28 14:57:09 UTC
Created attachment 91089 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 112 Nick Clemens 2019-06-28 14:57:13 UTC
Created attachment 91090 [details] [review]
Bug 18928: (bug 22679 follow-up) Do not remove all rules along with the issuing rule

We actually want to remove maxissueqty, maxonsiteissueqty and max_holds (for now)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 113 Nick Clemens 2019-06-28 14:57:17 UTC
Created attachment 91091 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 114 Nick Clemens 2019-06-28 14:57:21 UTC
Created attachment 91092 [details] [review]
Bug 18928: Remove '(not set)'

The "(not set)" part must be removed IMO, I am not sure I understand its goal.
Now that we have 1 entry per rule (and not 1 DB row for the whole "Default
checkout, hold and return policy for Centerville" table), it will be harder
to handle it correctly. Not impossible, but if not really needed I would prefer
to just remove it. If a rule is not set, the cell is empty or the dropdown list
displays the option "Not set", which seems to correct the problem.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 115 Nick Clemens 2019-06-28 14:57:25 UTC
Created attachment 91093 [details] [review]
Bug 18928: (bug 22847 follow-up 2) Display correctly the values (not the propagated ones)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 116 Nick Clemens 2019-06-28 14:57:28 UTC
Created attachment 91094 [details] [review]
Bug 18928: (QA follow-up) Remove DBIC files and reference to DefaultCircRule

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 117 Martin Renvoize 2019-07-01 15:19:32 UTC
Nice work!

Pushed to master for 19.11.00
Comment 118 Fridolin Somers 2019-07-25 12:07:37 UTC
Enhancement not pushed to 19.05.x