Bug 25711

Summary: Move ExpireReservesMaxPickUpDelayCharge to the circulation rules
Product: Koha Reporter: Minna Kivinen <minna.kivinen>
Component: CirculationAssignee: Peter Vashchuk <stalkernoid>
Status: Patch doesn't apply --- QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, joonas.kylmala, kyle.m.hall, kyle, lucas, martin.renvoize, matthias.le-gac, nick, nugged, sally.healey, slavashishkin, stalkernoid, tomascohen
Version: unspecifiedKeywords: release-notes-needed
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
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
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 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 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: Deprecate ExpireReservesMaxPickUpDelayCharge
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: Deprecate ExpireReservesMaxPickUpDelayCharge
Bug 25711: (QA follow-up) Remove leftover code and fix atomic update
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: Deprecate ExpireReservesMaxPickUpDelayCharge
Bug 25711: (QA follow-up) Remove leftover code and fix atomic update

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 Andrii 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 Andrii Nugged 2020-08-19 18:23:59 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 17 Martin Renvoize 2021-04-20 14:18:53 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2021-04-20 14:18:57 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 23 Peter Vashchuk 2021-07-30 12:14:11 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 26 Nick Clemens 2021-07-30 13:59:46 UTC Comment hidden (obsolete)
Comment 27 Nick Clemens 2021-07-30 13:59:49 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2021-08-04 08:33:12 UTC Comment hidden (obsolete)
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 Andrii Nugged 2021-08-04 09:01:14 UTC Comment hidden (obsolete)
Comment 31 Andrii 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 Andrii 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 Andrii 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 Comment hidden (obsolete)
Comment 36 Andrii 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 Comment hidden (obsolete)
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 Andrii 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 Comment hidden (obsolete)
Comment 45 Peter Vashchuk 2022-05-17 12:03:30 UTC Comment hidden (obsolete)
Comment 46 Peter Vashchuk 2022-05-17 12:03:37 UTC Comment hidden (obsolete)
Comment 47 Peter Vashchuk 2022-05-17 12:03:42 UTC Comment hidden (obsolete)
Comment 48 Peter Vashchuk 2022-05-17 12:03:47 UTC Comment hidden (obsolete)
Comment 49 Peter Vashchuk 2022-05-17 12:03:52 UTC Comment hidden (obsolete)
Comment 50 Peter Vashchuk 2022-05-17 12:04:00 UTC Comment hidden (obsolete)
Comment 51 Peter Vashchuk 2022-05-17 12:04:30 UTC
Rebased on latest master
Comment 52 Jonathan Druart 2022-05-18 08:18:52 UTC
Martin, can you retest this one?
Comment 53 Martin Renvoize 2022-05-18 15:58:48 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2022-05-18 15:58:54 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2022-05-18 15:58:59 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize 2022-05-18 15:59:04 UTC Comment hidden (obsolete)
Comment 57 Martin Renvoize 2022-05-18 15:59:09 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2022-05-18 15:59:14 UTC Comment hidden (obsolete)
Comment 59 Martin Renvoize 2022-05-18 15:59:52 UTC
All working well still.. added my SO lines to the subsequent patches that have come in :)
Comment 60 Kyle M Hall 2022-06-10 16:43:24 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2022-06-10 16:43:39 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2022-06-10 16:43:43 UTC Comment hidden (obsolete)
Comment 63 Kyle M Hall 2022-06-10 16:43:47 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2022-06-10 16:43:51 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2022-06-10 16:43:55 UTC Comment hidden (obsolete)
Comment 66 Tomás Cohen Arazi 2022-06-28 14:22:45 UTC
I agree with this 99%. But it is missing the 'Move' part. I think we should:

1. Migrate the syspref value into a global default using an atomicupdate
2. Delete the syspref
3. Adapt the code that uses the syspref (git grep ExpireReservesMaxPickUpDelayCharge)
Comment 67 Tomás Cohen Arazi 2022-06-30 13:10:13 UTC Comment hidden (obsolete)
Comment 68 Tomás Cohen Arazi 2022-06-30 13:13:54 UTC
Petro et al, please look at the follow-up I've added. I think there's still some little work to do here. In particular, running:

    $ git grep ExpireReservesMaxPickUpDelayCharge

showed that some spots need thought still. Places where templates still check the syspref when they should be checking the "effective value", even without my follow-up.

Looking forward to the upcoming follow-ups for pushing this ASAP.

Thanks!

PS It also felt like we could move ExpireReservesMaxPickupDelay and put them both on its own section below the big matrix. We can do it on a follow-up bug of course.
Comment 69 Andrii Nugged 2022-09-09 17:07:14 UTC
Created attachment 140377 [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>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Andrii Nugged 2022-09-09 17:07:20 UTC
Created attachment 140378 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Andrii Nugged 2022-09-09 17:07:26 UTC
Created attachment 140379 [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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Andrii Nugged 2022-09-09 17:07:31 UTC
Created attachment 140380 [details] [review]
Bug 25711: Wording 'use global' > 'Default'

We use 'Default' for useDaysMode already

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Andrii Nugged 2022-09-09 17:07:36 UTC
Created attachment 140381 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Andrii Nugged 2022-09-09 17:07:42 UTC
Created attachment 140382 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Andrii Nugged 2022-09-09 17:07:47 UTC
Created attachment 140383 [details] [review]
Bug 25711: Deprecate ExpireReservesMaxPickUpDelayCharge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 76 Andrii Nugged 2022-09-09 17:08:17 UTC
Rebased over current master.

@Petro, yes, let's kick this up, I need this in production!
Comment 77 Peter Vashchuk 2022-10-03 14:52:26 UTC
Created attachment 141270 [details] [review]
Bug 25711: (QA follow-up) Remove leftover code and fix atomic update

This patch finalizes another one which deprecates ExpireReservesMaxPickUpDelayCharge.
Because we had multiple smart-rules with undefined "expire_reserves_charge" values, this patch updates actomic update to set not only the general All/All smart-rule row to deprecated ExpireReservesMaxPickUpDelayCharge value but also every smart-rule that fall back to default value in "expire_reserves_charge".

Also removed additional leftovers in template which earlier expected to show hyperlink to syspref ExpireReservesMaxPickUpDelay.

To check if atomic update functions correctly:
1) Go to syspref and set ExpireReservesMaxPickUpDelay as "allow", set ExpireReservesMaxPickUpDelayCharge to some numerical value (that will be your default value), for example "42".
2) Go to smart rules page and create three rules: a rule with empty "Expire reserve charge" value, then create another one rule with "Expire reserve charge" vaue "0", and then another rule with "Expire reserve charge" value set to any number, for example "12".
3) Apply the patch, and run the atomic update via the installer page.
4) Return to the smart rules page and check those rules you created in step 2:
- That one rule that you set as empty, now should have "Expire reserve charge" equal 42, i.e. that one you set to the value you set ExpireReservesMaxPickUpDelayCharge in step 1.
- All rules that had numerical values set previously should have been exist untouched.
Comment 78 Martin Renvoize 2023-06-21 10:04:02 UTC
Sorry chaps, this needs another rebase.. I managed the first one fairly easily, but then I got sh1 issues and missing hunks.

Any chance you could rebase it for us?
Comment 79 Martin Renvoize 2023-09-20 12:59:10 UTC
Still on your horizon Andrii?
Comment 80 Andrii Nugged 2023-10-23 08:45:30 UTC
Created attachment 157632 [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>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 81 Andrii Nugged 2023-10-23 08:45:35 UTC
Created attachment 157633 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 82 Andrii Nugged 2023-10-23 08:45:39 UTC
Created attachment 157634 [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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 83 Andrii Nugged 2023-10-23 08:45:42 UTC
Created attachment 157635 [details] [review]
Bug 25711: Wording 'use global' > 'Default'

We use 'Default' for useDaysMode already

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 84 Andrii Nugged 2023-10-23 08:45:46 UTC
Created attachment 157636 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 85 Andrii Nugged 2023-10-23 08:45:49 UTC
Created attachment 157637 [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.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 86 Andrii Nugged 2023-10-23 08:45:53 UTC
Created attachment 157638 [details] [review]
Bug 25711: Deprecate ExpireReservesMaxPickUpDelayCharge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Andrii Nugged 2023-10-23 08:45:57 UTC
Created attachment 157639 [details] [review]
Bug 25711: (QA follow-up) Remove leftover code and fix atomic update

This patch finalizes another one which deprecates ExpireReservesMaxPickUpDelayCharge.
Because we had multiple smart-rules with undefined "expire_reserves_charge" values, this patch updates actomic update to set not only the general All/All smart-rule row to deprecated ExpireReservesMaxPickUpDelayCharge value but also every smart-rule that fall back to default value in "expire_reserves_charge".

Also removed additional leftovers in template which earlier expected to show hyperlink to syspref ExpireReservesMaxPickUpDelay.

To check if atomic update functions correctly:
1) Go to syspref and set ExpireReservesMaxPickUpDelay as "allow", set ExpireReservesMaxPickUpDelayCharge to some numerical value (that will be your default value), for example "42".
2) Go to smart rules page and create three rules: a rule with empty "Expire reserve charge" value, then create another one rule with "Expire reserve charge" vaue "0", and then another rule with "Expire reserve charge" value set to any number, for example "12".
3) Apply the patch, and run the atomic update via the installer page.
4) Return to the smart rules page and check those rules you created in step 2:
- That one rule that you set as empty, now should have "Expire reserve charge" equal 42, i.e. that one you set to the value you set ExpireReservesMaxPickUpDelayCharge in step 1.
- All rules that had numerical values set previously should have been exist untouched.
Comment 88 Andrii Nugged 2023-10-23 08:47:58 UTC
Lost it from my POV here (sorry)
we are using this code in our productions (even!), (in the older form where ExpireReservesMaxPickUpDelayCharge syspref exists)

... so here is the rebased on the latest master working code.

I think after FINAL confirmation that it is okay and what it does in that way might be proposed for master - Petro might rewrite all commits to less to clean up from "changes over changes" in this code.
Comment 89 Matthias Le Gac 2024-01-09 20:44:19 UTC
Do you need again a signoff or it's good ? 
Because I can't find the field "Expire reserve charge" when I want to create smart rules.
Comment 90 Lucas Gass 2024-01-11 00:51:40 UTC
This one needs a rebase for current master.