Bug 25711

Summary: Move ExpireReservesMaxPickUpDelayCharge to the circulation rules
Product: Koha Reporter: Minna Kivinen <minna.kivinen>
Component: CirculationAssignee: Peter Vashchuk <stalkernoid>
Status: Needs Signoff --- QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart+koha, joonas.kylmala, kyle.m.hall, martin.renvoize, nick, nugged, sally.healey, slavashishkin, stalkernoid
Version: unspecifiedKeywords: release-notes-needed
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15523    
Attachments: Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: (QA follow-up) Refactor to clarify logic
Bug 25711: (QA follow-up) Refactor to clarify logic
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: (QA follow-up) Refactor to clarify logic
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug 25711: (QA follow-up) Refactor to clarify logic
Bug 25711: (QA follow-up) Move tests and disallow blank rule
Bug 25711: Wording 'use global' > 'Default'
Bug 25711: fix existing tests and add new one
Bug 25711: move get_effective_expire_charge to Koha::CirculationRules
Bug 25711: add comment to clarify how the method works
Bug 25711: add comment to clarify how the method works
Bug 25711: tests for get_effective_expire_reserves_charge method
Bug 25711: move get_effective_expire_charge to Koha::CirculationRules

Description Minna Kivinen 2020-06-11 04:06:08 UTC
ExpireReservesMaxPickUpDelayCharge allows one charge to be applied to all holds that expire.

It would be better to have ability to define this charge in ciculation rules. There can be different charges for different types of materials for different patron categories.  

Related to this bug report:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18195
Charge for expired holds per patron category
Comment 1 Peter Vashchuk 2020-07-21 11:18:25 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2020-07-24 10:36:21 UTC
Comment on attachment 107144 [details] [review]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules

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

In addition the the other comments, the expire_reserves_charge is missing from installer/data/mysql/kohastructure.sql.

::: Koha/Hold.pm
@@ +372,5 @@
>              C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber });
>  
> +            my $item = $self->item;
> +
> +            my $controlbranch = C4::Context->preference('ReservesControlBranch');

This and the code below should use C4::Reserves::GetReservesControlBranch()

@@ +384,5 @@
> +
> +            my $rule = Koha::CirculationRules->get_effective_rule(
> +                {
> +                    categorycode => $self->borrower->categorycode,
> +                    itemtype     => $item ? $item->effective_itemtype : $self->biblio->itemtype,

Should use instead the syspref item-level_itypes

::: t/db_dependent/Holds/Holdfine.t
@@ +1,1 @@
> +#!/usr/bin/perl

License header and copyright info is missing.
Comment 3 Peter Vashchuk 2020-07-27 09:33:19 UTC Comment hidden (obsolete)
Comment 4 Andrew Nugged 2020-07-31 01:41:54 UTC
JavaScript edit form on smart-rules page wrong behavior spotted,
+1 small fix to come for not to allow inner <td> value to be transferred by JS as field value
Comment 5 Peter Vashchuk 2020-08-03 10:02:22 UTC Comment hidden (obsolete)
Comment 6 Peter Vashchuk 2020-08-05 15:48:09 UTC Comment hidden (obsolete)
Comment 7 Andrew Nugged 2020-08-19 18:23:59 UTC
Created attachment 108689 [details] [review]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules

Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge
in circulation rules per specific items/patron categories.
When value not found in circulation rules it uses global
ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules
feature was introduced.

To check the feature after applying the patch:
    1) Go to /cgi-bin/koha/admin/preferences.pl and set
ExpireReservesMaxPickUpDelay to allow.
    1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table
and notice the newly added "Expire reserve charge" row.
    2) In that row add numerical values to type/patron categories
that you will be using later to test this feature.
    3) Prepare an item for our next step, pick a biblio that has item
with the same item type you altered in smart rules and check it out.
    3) Place a hold on that item and set the expiration date as the
next day.
    4) Check in it after that.
    5) Do SQL request to change expiration date manually to "yesterday":
in "reserves" table, find the id of the hold that you placed and
"UPDATE reserves SET `expirationdate`='2020-07-19' WHERE
`reserve_id`=329;"
 Or alternatively:
    5) Wait for tomorrow, change server time or invent a time machine.
    6) Run cancel_expired_holds.pl cron job.
    7) Check that patron got charged in "accounting" left tab and
"transactions" upper tab: your patron should have debt of that amount
of fee you assigned in that rule
(/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...)
    8) You can repeat this with other rules and different
numbers/biblio/item types to check that different fees apply according
to smart rules.
    9) If there's no matching smart rule or smart rule have an empty
string as a fee - global ExpireReservesMaxPickUpDelayCharge variable
value will be used.
   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

IMPORTANT:
Smart-rules filters only applied to item-level holds, when item-type
for items is known. If there's biblio-level hold, then again global
ExpireReservesMaxPickUpDelayCharge value will be used (as well global
one can be empty or zero).

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 8 Joonas Kylmälä 2021-03-03 12:07:27 UTC
The line

> C4::Reserves::GetReservesControlBranch($item->unblessed, $self->borrower->unblessed);

was uncommented and is needed. The parameters passed to GetReservesControlBranch need to be unblessed for it to return the right branch value.
Comment 9 Joonas Kylmälä 2021-03-03 12:29:28 UTC
(In reply to Joonas Kylmälä from comment #8)
> The line
> 
> > C4::Reserves::GetReservesControlBranch($item->unblessed, $self->borrower->unblessed);
> 
> was uncommented and is needed. The parameters passed to
> GetReservesControlBranch need to be unblessed for it to return the right
> branch value.

Please add also a test to catch this bug.
Comment 10 Joonas Kylmälä 2021-03-03 12:36:09 UTC
I think we should completely remove ExpireReservesMaxPickUpDelayCharge syspref now and move it's value with a DB upgrade to the default circ rule.
Comment 11 Katrin Fischer 2021-03-04 10:07:28 UTC
(In reply to Joonas Kylmälä from comment #10)
> I think we should completely remove ExpireReservesMaxPickUpDelayCharge
> syspref now and move it's value with a DB upgrade to the default circ rule.

For existing installations that will cause no disruption, but for the next person introducing this to their collection it might cause a lot of extra work - even more values to enter and make sure nothing has been missed. I've been wondering about this with other "moves" to the rules as well.

Could we instead handle this like DaysMode, where the default is drawn from the pref, but you can add different values in the rules?
Comment 12 Joonas Kylmälä 2021-03-04 10:43:40 UTC
(In reply to Katrin Fischer from comment #11)
> (In reply to Joonas Kylmälä from comment #10)
> > I think we should completely remove ExpireReservesMaxPickUpDelayCharge
> > syspref now and move it's value with a DB upgrade to the default circ rule.
> 
> For existing installations that will cause no disruption, but for the next
> person introducing this to their collection it might cause a lot of extra
> work - even more values to enter and make sure nothing has been missed. I've
> been wondering about this with other "moves" to the rules as well.

Can you clarify how this would cause more work? I don't understand.


> Could we instead handle this like DaysMode, where the default is drawn from
> the pref, but you can add different values in the rules?

IMHO, it is dangerous to define same setting in two separate places because you only would know this by reading manual or help text next to these two settings and I don't think users usually read manuals.
Comment 13 Joonas Kylmälä 2021-03-04 10:48:59 UTC
(In reply to Joonas Kylmälä from comment #12)
> > Could we instead handle this like DaysMode, where the default is drawn from
> > the pref, but you can add different values in the rules?
> 
> IMHO, it is dangerous to define same setting in two separate places because
> you only would know this by reading manual or help text next to these two
> settings and I don't think users usually read manuals.

Also defining the same rule in syspref and circ rule would make the code more complicated, please see the line

> $charge = $rule_value ne '' ? $rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");

in the patch. And what if next time someone tries to fetch the charge that need to be made then they might miss out that oh, we had a syspref also. So at least we need to have helper method if a syspref is being used but I still think it leaves out the possibility of somebody just fetching the circulation rule because I at least would expect it to be the source of truth for charge value.
Comment 14 Joonas Kylmälä 2021-03-04 10:54:10 UTC
(In reply to Joonas Kylmälä from comment #12)
> (In reply to Katrin Fischer from comment #11)
> > (In reply to Joonas Kylmälä from comment #10)
> > > I think we should completely remove ExpireReservesMaxPickUpDelayCharge
> > > syspref now and move it's value with a DB upgrade to the default circ rule.
> > 
> > For existing installations that will cause no disruption, but for the next
> > person introducing this to their collection it might cause a lot of extra
> > work - even more values to enter and make sure nothing has been missed. I've
> > been wondering about this with other "moves" to the rules as well.
> 
> Can you clarify how this would cause more work? I don't understand.

I think I might understand, so if you have in the default rules multiple different rule lines I think you mean we would have to define the charge for each of those. Hmm, so I wonder if we can allow the value to be undefined and then add a one rule in the end for common rules, or we could maybe make this bug depend on bug 15522 which should make defining the rules a lot simpler.
Comment 15 Joonas Kylmälä 2021-04-15 08:18:32 UTC
*** Bug 18195 has been marked as a duplicate of this bug. ***
Comment 16 Peter Vashchuk 2021-04-19 12:47:58 UTC
Created attachment 119851 [details] [review]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules

Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge
in circulation rules per specific items/patron categories.
When value not found in circulation rules it uses global
ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules
feature was introduced.

To check the feature after applying the patch:
    1) Go to /cgi-bin/koha/admin/preferences.pl and set
ExpireReservesMaxPickUpDelay to allow.
    1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table
and notice the newly added "Expire reserve charge" row.
    2) In that row add numerical values to type/patron categories
that you will be using later to test this feature.
    3) Prepare an item for our next step, pick a biblio that has item
with the same item type you altered in smart rules and check it out.
    3) Place a hold on that item and set the expiration date as the
next day.
    4) Check in it after that.
    5) Do SQL request to change expiration date manually to "yesterday":
in "reserves" table, find the id of the hold that you placed and
"UPDATE reserves SET `expirationdate`='2020-07-19' WHERE
`reserve_id`=329;"
 Or alternatively:
    5) Wait for tomorrow, change server time or invent a time machine.
    6) Run cancel_expired_holds.pl cron job.
    7) Check that patron got charged in "accounting" left tab and
"transactions" upper tab: your patron should have debt of that amount
of fee you assigned in that rule
(/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...)
    8) You can repeat this with other rules and different
numbers/biblio/item types to check that different fees apply according
to smart rules.
    9) If there's no matching smart rule or smart rule have an empty
string as a fee - global ExpireReservesMaxPickUpDelayCharge variable
value will be used.
   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

IMPORTANT:
Smart-rules filters only applied to item-level holds when item-type
for items is known. If there's biblio-level hold, in that case global
ExpireReservesMaxPickUpDelayCharge value will be used).

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 17 Martin Renvoize 2021-04-20 14:18:53 UTC
Created attachment 119911 [details] [review]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules

Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge
in circulation rules per specific items/patron categories.
When value not found in circulation rules it uses global
ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules
feature was introduced.

To check the feature after applying the patch:
    1) Go to /cgi-bin/koha/admin/preferences.pl and set
ExpireReservesMaxPickUpDelay to allow.
    1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table
and notice the newly added "Expire reserve charge" row.
    2) In that row add numerical values to type/patron categories
that you will be using later to test this feature.
    3) Prepare an item for our next step, pick a biblio that has item
with the same item type you altered in smart rules and check it out.
    3) Place a hold on that item and set the expiration date as the
next day.
    4) Check in it after that.
    5) Do SQL request to change expiration date manually to "yesterday":
in "reserves" table, find the id of the hold that you placed and
"UPDATE reserves SET `expirationdate`='2020-07-19' WHERE
`reserve_id`=329;"
 Or alternatively:
    5) Wait for tomorrow, change server time or invent a time machine.
    6) Run cancel_expired_holds.pl cron job.
    7) Check that patron got charged in "accounting" left tab and
"transactions" upper tab: your patron should have debt of that amount
of fee you assigned in that rule
(/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...)
    8) You can repeat this with other rules and different
numbers/biblio/item types to check that different fees apply according
to smart rules.
    9) If there's no matching smart rule or smart rule have an empty
string as a fee - global ExpireReservesMaxPickUpDelayCharge variable
value will be used.
   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

IMPORTANT:
Smart-rules filters only applied to item-level holds when item-type
for items is known. If there's biblio-level hold, in that case global
ExpireReservesMaxPickUpDelayCharge value will be used).

Mentored-by: Andrew Nugged <nugged@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-04-20 14:18:57 UTC
Created attachment 119912 [details] [review]
Bug 25711: (QA follow-up) Refactor to clarify logic

At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick
in, we should already have an item waiting (else what's triggered a pickup
delay). As such we can drop the fall through default of using the syspref
when we don't yet have an item.

We can also save us some calculations and database hits by only
calculating the charge value when we've actually been asked to charge.
So we also move the block inside the 'charge_cancel_fee' block.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-04-20 14:20:38 UTC
I believe this works as described.. I did a little followup to clean up the code a little whilst I was wrapping my head around the various failure clauses.

Signing off so Joonas can move on to QA ;).. He's certainly the expert in these parts at the moment and looks to be progressing nicely with a code review.. so I'll encourage him to continue :)
Comment 20 Peter Vashchuk 2021-04-26 08:01:04 UTC Comment hidden (obsolete)
Comment 21 Joonas Kylmälä 2021-06-15 11:19:16 UTC
Unsetting myself from QA contact, I don't think I am able to find time reviewing this any time soon.
Comment 22 Peter Vashchuk 2021-07-30 12:14:07 UTC
Created attachment 123309 [details] [review]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules

Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge
in circulation rules per specific items/patron categories.
When value not found in circulation rules it uses global
ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules
feature was introduced.

To check the feature after applying the patch:
    1) Go to /cgi-bin/koha/admin/preferences.pl and set
ExpireReservesMaxPickUpDelay to allow.
    1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table
and notice the newly added "Expire reserve charge" row.
    2) In that row add numerical values to type/patron categories
that you will be using later to test this feature.
    3) Prepare an item for our next step, pick a biblio that has item
with the same item type you altered in smart rules and check it out.
    3) Place a hold on that item and set the expiration date as the
next day.
    4) Check in it after that.
    5) Do SQL request to change expiration date manually to "yesterday":
in "reserves" table, find the id of the hold that you placed and
"UPDATE reserves SET `expirationdate`='2020-07-19' WHERE
`reserve_id`=329;"
 Or alternatively:
    5) Wait for tomorrow, change server time or invent a time machine.
    6) Run cancel_expired_holds.pl cron job.
    7) Check that patron got charged in "accounting" left tab and
"transactions" upper tab: your patron should have debt of that amount
of fee you assigned in that rule
(/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...)
    8) You can repeat this with other rules and different
numbers/biblio/item types to check that different fees apply according
to smart rules.
    9) If there's no matching smart rule or smart rule have an empty
string as a fee - global ExpireReservesMaxPickUpDelayCharge variable
value will be used.
   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

IMPORTANT:
Smart-rules filters only applied to item-level holds when item-type
for items is known. If there's biblio-level hold, in that case global
ExpireReservesMaxPickUpDelayCharge value will be used).

Mentored-by: Andrew Nugged <nugged@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Peter Vashchuk 2021-07-30 12:14:11 UTC
Created attachment 123310 [details] [review]
Bug 25711: (QA follow-up) Refactor to clarify logic

At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick
in, we should already have an item waiting (else what's triggered a pickup
delay). As such we can drop the fall through default of using the syspref
when we don't yet have an item.

We can also save us some calculations and database hits by only
calculating the charge value when we've actually been asked to charge.
So we also move the block inside the 'charge_cancel_fee' block.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 24 Peter Vashchuk 2021-07-30 12:14:44 UTC
Rebased on the latest master
Comment 25 Nick Clemens 2021-07-30 13:59:42 UTC
Created attachment 123326 [details] [review]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules

Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge
in circulation rules per specific items/patron categories.
When value not found in circulation rules it uses global
ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules
feature was introduced.

To check the feature after applying the patch:
    1) Go to /cgi-bin/koha/admin/preferences.pl and set
ExpireReservesMaxPickUpDelay to allow.
    1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table
and notice the newly added "Expire reserve charge" row.
    2) In that row add numerical values to type/patron categories
that you will be using later to test this feature.
    3) Prepare an item for our next step, pick a biblio that has item
with the same item type you altered in smart rules and check it out.
    3) Place a hold on that item and set the expiration date as the
next day.
    4) Check in it after that.
    5) Do SQL request to change expiration date manually to "yesterday":
in "reserves" table, find the id of the hold that you placed and
"UPDATE reserves SET `expirationdate`='2020-07-19' WHERE
`reserve_id`=329;"
 Or alternatively:
    5) Wait for tomorrow, change server time or invent a time machine.
    6) Run cancel_expired_holds.pl cron job.
    7) Check that patron got charged in "accounting" left tab and
"transactions" upper tab: your patron should have debt of that amount
of fee you assigned in that rule
(/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...)
    8) You can repeat this with other rules and different
numbers/biblio/item types to check that different fees apply according
to smart rules.
    9) If there's no matching smart rule or smart rule have an empty
string as a fee - global ExpireReservesMaxPickUpDelayCharge variable
value will be used.
   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

IMPORTANT:
Smart-rules filters only applied to item-level holds when item-type
for items is known. If there's biblio-level hold, in that case global
ExpireReservesMaxPickUpDelayCharge value will be used).

Mentored-by: Andrew Nugged <nugged@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Nick Clemens 2021-07-30 13:59:46 UTC
Created attachment 123327 [details] [review]
Bug 25711: (QA follow-up) Refactor to clarify logic

At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick
in, we should already have an item waiting (else what's triggered a pickup
delay). As such we can drop the fall through default of using the syspref
when we don't yet have an item.

We can also save us some calculations and database hits by only
calculating the charge value when we've actually been asked to charge.
So we also move the block inside the 'charge_cancel_fee' block.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 27 Nick Clemens 2021-07-30 13:59:49 UTC
Created attachment 123328 [details] [review]
Bug 25711: (QA follow-up) Move tests and disallow blank rule

The tests can go where the existing cancellation charge tests are.
If we define the default rule we do not need to delete all circulation rules
as the itemtypes and branches are all created for the tests

The rule should not be saved if blank, this allows simplifying the logic
when getting the charge
Comment 28 Jonathan Druart 2021-08-04 08:33:12 UTC
Created attachment 123452 [details] [review]
Bug 25711: Wording 'use global' > 'Default'

We use 'Default' for useDaysMode already
Comment 29 Jonathan Druart 2021-08-04 08:42:38 UTC
1. Tiny wording change suggestion, do you agree with the last patch?

2. You need a new method in Koha::CirculationRules to retrieve the effective value of the new rule (have a look at 'sub get_effective_*')

3. 
-                        } else if ( i == 5 || i == 6 || i == 26 || i == 27 || i == 28 ) {
+                        } else if ( i == 5 || i == 6 || i == 17 || i == 27 || i == 28 || i == 29 ) {

I think there is something wrong here. Why did you remove 26? Do you understand what this part of the code does? It's displaying "Unlimited" for integer without value. But you want "use global/Default" to be displayed if the rule is empty.
I am not sure we need this line to change.

4. (not blocker)
Not a bit deal but in tests you can use $builder->build_object instead of ->build, to return a Koha::Object-based object.
Comment 30 Andrew Nugged 2021-08-04 09:01:14 UTC Comment hidden (obsolete)
Comment 31 Andrew Nugged 2021-08-04 09:04:40 UTC
> 1. Tiny wording change suggestion, do you agree with the last patch?

yes, "Default" is good
Comment 32 Peter Vashchuk 2021-08-04 09:11:33 UTC
(In reply to Jonathan Druart from comment #29)
 > 3. 
> -                        } else if ( i == 5 || i == 6 || i == 26 || i == 27
> || i == 28 ) {
> +                        } else if ( i == 5 || i == 6 || i == 17 || i == 27
> || i == 28 || i == 29 ) {
> 
> I think there is something wrong here. Why did you remove 26?...

A new 17th column was added, so the number of columns increased, and relative numbers for the next ones shifted, in short, 26th column wasn't removed, it just became 27th, so 26 27 28 are 27 28 29 accordingly.
Comment 33 Andrew Nugged 2021-08-04 09:22:36 UTC
> 4. (not blocker)
> Not a bit deal but in tests you can use $builder->build_object instead of
> ->build, to return a Koha::Object-based object.

and, as I figured out too, seems tests needed to protect numerical zero in rule value not to be overlapped with default:

   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

... and this is not covered by tests now. So, Petro, you can update tests also taking into account this JD's p4 advice.

As well as I see roll-back for this https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123328&action=diff#a/Koha/Hold.pm_sec1
needed to restore this "no-fee (0) for particular items when the default is not zero" feature.
Comment 34 Andrew Nugged 2021-08-04 10:40:26 UTC
We tested once more with Petro: Nick's update works, so it's ok,

I was confused that we might overlap 0 in

    "$rule ? $rule->rule_value : C4::Context->preference..."

but we before "?" checking for RULE as _object_, not rule value (my confusion :) ), so even if rule_value is 0, object anyway will be truth, so zero returned, and logic works.

And in the "Default" case in rule, we store "undef", which leads to full rule absence in DB, so again: checking for the rule as object fails and we falling back to Default.

All ok with logic, yeah.
Comment 35 Peter Vashchuk 2021-08-04 12:20:45 UTC
Created attachment 123456 [details] [review]
Bug 25711: fix existing tests and add new one

Add a new test with circulation rule for library_C_code that has
set expire_reserves_charge which is 0. In this case 0 should be used
as the charge.
Also use build_object instead of build to build the koha object.
Comment 36 Andrew Nugged 2021-08-04 13:09:23 UTC
Petro, 
  regarding this from JD:

> 2. You need a new method in Koha::CirculationRules to retrieve the effective value of the new rule (have a look at 'sub get_effective_*')

Let's create +1 more patch and move get_effective_expire_reserves_charge as for example get_effective_daysmode created and move "default detection" logic there, so it will be the proper place for re-usable code.

(JD: thanks for the explanation!)
Comment 37 Peter Vashchuk 2021-08-05 13:38:20 UTC
Created attachment 123498 [details] [review]
Bug 25711: move get_effective_expire_charge to Koha::CirculationRules

Move get_effective_expire_reserves_charge as a new method
in Koha::CirculationRules to retrieve the effective value of the rule.
Comment 38 Jonathan Druart 2021-08-05 14:39:49 UTC
+    return $expire_reserves_charge_rule ? $expire_reserves_charge_rule->rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");

Is this correct if the rule is "0"?

Note that it won't pass QA because of missing tests.
Comment 39 Andrew Nugged 2021-08-06 09:49:09 UTC
(In reply to Jonathan Druart from comment #38)
> +    return $expire_reserves_charge_rule ?
> $expire_reserves_charge_rule->rule_value :
> C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
> 
> Is this correct if the rule is "0"?

if the rule has 0 as a stored value but is found,
then $expire_reserves_charge_rule is the object,

in case we saving an empty line to form for that rule item,
it transfers undef to DBIC so even no record is created,
then there's not an object and all, "not found".

(same as I misled myself above, and figured out here:
 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711#c34 )
Comment 40 Jonathan Druart 2021-08-06 10:27:35 UTC
(In reply to Andrew Nugged from comment #39)
> (In reply to Jonathan Druart from comment #38)
> > +    return $expire_reserves_charge_rule ?
> > $expire_reserves_charge_rule->rule_value :
> > C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
> > 
> > Is this correct if the rule is "0"?
> 
> if the rule has 0 as a stored value but is found,
> then $expire_reserves_charge_rule is the object,
> 
> in case we saving an empty line to form for that rule item,
> it transfers undef to DBIC so even no record is created,
> then there's not an object and all, "not found".
> 
> (same as I misled myself above, and figured out here:
>  https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711#c34 )

Right, I misread!
Comment 41 Peter Vashchuk 2021-08-06 11:37:32 UTC Comment hidden (obsolete)
Comment 42 Peter Vashchuk 2021-08-09 08:01:03 UTC Comment hidden (obsolete)
Comment 43 Peter Vashchuk 2021-08-09 14:57:52 UTC Comment hidden (obsolete)
Comment 44 Peter Vashchuk 2021-08-10 08:42:02 UTC
Created attachment 123661 [details] [review]
Bug 25711: move get_effective_expire_charge to Koha::CirculationRules

Move get_effective_expire_reserves_charge as a new method
in Koha::CirculationRules to retrieve the effective value of the rule,
which means it will get the value from existing circulation rule and
if it doesn't exist or set to undefined it will get the default
value from the preferences.

Also added tests for this logic.