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

Created attachment 65242 [details] [review]
Bug 18928 - Remove schema files for deleted tables

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

Created attachment 71006 [details] [review]
Bug 18928 - Remove schema files for deleted tables

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

Created attachment 81097 [details] [review]
Bug 18928: Add DB structure changes

Created attachment 81098 [details] [review]
Bug 18928: Update schema files

Created attachment 81099 [details] [review]
Bug 18928: Add missing filters

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

Created attachment 82934 [details] [review]
Bug 18928: Add DB structure changes

Created attachment 82935 [details] [review]
Bug 18928: Update schema files

Created attachment 82936 [details] [review]
Bug 18928: Add missing filters

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

Created attachment 85488 [details] [review]
Bug 18928: Add DB structure changes

Created attachment 85489 [details] [review]
Bug 18928: Add missing filters

Created attachment 85490 [details] [review]
Bug 18928: Update schema files

Created attachment 85491 [details] [review]
Bug 18928: Update new occurrences

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>

Created attachment 86135 [details] [review]
Bug 18928: Add DB structure changes

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

Created attachment 86136 [details] [review]
Bug 18928: Add missing filters

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

Created attachment 86137 [details] [review]
Bug 18928: Update new occurrences

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

Created attachment 86138 [details] [review]
[DO NOT PUSH] Bug 18928: DBIC schema update

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

Created attachment 86139 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

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

I've:
- Tried setting global and branch-specific rules
- Upgrade migrated the rules correctly
- Tests pass
- QA scripts happy

Good job!

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...

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

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>

Created attachment 89018 [details] [review]
Bug 18928: Add DB structure changes

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

Created attachment 89019 [details] [review]
Bug 18928: Add missing filters

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

Created attachment 89020 [details] [review]
Bug 18928: Update new occurrences

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

Created attachment 89021 [details] [review]
Bug 18928: (follow-up) Make DB update idempotent

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

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

Created attachment 89023 [details] [review]
[DO NOT PUSH] Bug 18928: DBIC schema update

Created attachment 89024 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

(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.

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

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)

(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.

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)

(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.

Created attachment 89336 [details] [review]
Bug 18928: (bug 22847 follow-up) Display correctly the values (not the propagated ones)

And I think we are good now.

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

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>

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>

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>

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>

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>

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>

Created attachment 89345 [details] [review]
Bug 18928: DBIC schema update

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

Created attachment 89346 [details] [review]
Bug 18928: Use get_effective_rule in GetBranchItemRule

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

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>

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>

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

(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?

(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.

(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

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 :)

(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.

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.

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.

(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?

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

I a big patch set I am not very brave - can you please rebase?

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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

(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.

Created attachment 90501 [details] [review]
Bug 18928: (bug 22847 follow-up 2) Display correctly the values (not the propagated ones)

(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.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Nice work!

Pushed to master for 19.11.00

Enhancement not pushed to 19.05.x