Bug 25711 - Add ability to define expiredpickup charge for patron category and item type
Summary: Add ability to define expiredpickup charge for patron category and item type
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 20.05
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Peter Vashchuk
QA Contact: Nick Clemens
URL:
Keywords:
: 18195 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-06-11 04:06 UTC by Minna Kivinen
Modified: 2021-07-30 13:59 UTC (History)
9 users (show)

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


Attachments
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (18.66 KB, patch)
2020-07-21 11:18 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (18.36 KB, patch)
2020-07-27 09:33 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (18.02 KB, patch)
2020-08-03 10:02 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (17.50 KB, patch)
2020-08-05 15:48 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (17.56 KB, patch)
2020-08-19 18:23 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (20.04 KB, patch)
2021-04-19 12:47 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (20.10 KB, patch)
2021-04-20 14:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25711: (QA follow-up) Refactor to clarify logic (2.39 KB, patch)
2021-04-20 14:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25711: (QA follow-up) Refactor to clarify logic (2.44 KB, patch)
2021-04-26 08:01 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (19.98 KB, patch)
2021-07-30 12:14 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: (QA follow-up) Refactor to clarify logic (2.44 KB, patch)
2021-07-30 12:14 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules (19.98 KB, patch)
2021-07-30 13:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25711: (QA follow-up) Refactor to clarify logic (2.44 KB, patch)
2021-07-30 13:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25711: (QA follow-up) Move tests and disallow blank rule (21.15 KB, patch)
2021-07-30 13:59 UTC, Nick Clemens
Details | Diff | Splinter Review

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