https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |stalkernoid@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #4 from Andrew Nugged <nugged@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107407|0 |1 is obsolete| | --- Comment #5 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 107683 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107683&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107844|0 |1 is obsolete| | --- Comment #7 from Andrew Nugged <nugged@gmail.com> --- Created attachment 108689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108689&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #13 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- *** Bug 18195 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108689|0 |1 is obsolete| | --- Comment #16 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 119851 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119851&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 119912 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119912&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #119912|0 |1 is obsolete| | --- Comment #20 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 120164 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120164&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|joonas.kylmala@helsinki.fi |testopia@bugs.koha-communit | |y.org --- Comment #21 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Unsetting myself from QA contact, I don't think I am able to find time reviewing this any time soon. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #23 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 123310 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123310&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123309|0 |1 is obsolete| | Attachment #123310|0 |1 is obsolete| | --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 123326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123326&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #27 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 123328 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123328&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15523 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15523 [Bug 15523] Move a number of sysprefs to circulation rules (Omnibus) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #29 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #31 from Andrew Nugged <nugged@gmail.com> ---

1. Tiny wording change suggestion, do you agree with the last patch?

yes, "Default" is good -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #33 from Andrew Nugged <nugged@gmail.com> ---

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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #36 from Andrew Nugged <nugged@gmail.com> --- 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!) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #38 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- + 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #40 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123568|0 |1 is obsolete| | --- Comment #42 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 123610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123610&action=edit Bug 25711: add comment to clarify how the method works -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123498|0 |1 is obsolete| | Attachment #123610|0 |1 is obsolete| | Attachment #123653|0 |1 is obsolete| | --- Comment #44 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 123661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123661&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #46 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 135061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135061&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #48 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 135063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135063&action=edit Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #50 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 135065 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135065&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #52 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Martin, can you retest this one? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135060|0 |1 is obsolete| | --- Comment #53 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135156 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135156&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135062|0 |1 is obsolete| | --- Comment #55 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135158 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135158&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135064|0 |1 is obsolete| | --- Comment #57 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135160 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135160&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #59 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- All working well still.. added my SO lines to the subsequent patches that have come in :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com QA Contact| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135156|0 |1 is obsolete| | Attachment #135157|0 |1 is obsolete| | Attachment #135158|0 |1 is obsolete| | Attachment #135159|0 |1 is obsolete| | Attachment #135160|0 |1 is obsolete| | Attachment #135161|0 |1 is obsolete| | --- Comment #60 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 135950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135950&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #62 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 135952 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135952&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #64 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 135954 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135954&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #66 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #68 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #70 from Andrew Nugged <nugged@gmail.com> --- Created attachment 140378 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140378&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #72 from Andrew Nugged <nugged@gmail.com> --- Created attachment 140380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140380&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #74 from Andrew Nugged <nugged@gmail.com> --- Created attachment 140382 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140382&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #76 from Andrew Nugged <nugged@gmail.com> --- Rebased over current master. @Petro, yes, let's kick this up, I need this in production! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #77 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 141270 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141270&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #79 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Still on your horizon Andrii? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Andrii Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #140377|0 |1 is obsolete| | Attachment #140378|0 |1 is obsolete| | Attachment #140379|0 |1 is obsolete| | Attachment #140380|0 |1 is obsolete| | Attachment #140381|0 |1 is obsolete| | Attachment #140382|0 |1 is obsolete| | Attachment #140383|0 |1 is obsolete| | Attachment #141270|0 |1 is obsolete| | --- Comment #80 from Andrii Nugged <nugged@gmail.com> --- Created attachment 157632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157632&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #82 from Andrii Nugged <nugged@gmail.com> --- Created attachment 157634 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157634&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #84 from Andrii Nugged <nugged@gmail.com> --- Created attachment 157636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157636&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #86 from Andrii Nugged <nugged@gmail.com> --- Created attachment 157638 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157638&action=edit Bug 25711: Deprecate ExpireReservesMaxPickUpDelayCharge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #88 from Andrii Nugged <nugged@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Needs Signoff |Patch doesn't apply --- Comment #90 from Lucas Gass <lucas@bywatersolutions.com> --- This one needs a rebase for current master. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157633|0 |1 is obsolete| | --- Comment #93 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 171055 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171055&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157635|0 |1 is obsolete| | --- Comment #95 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 171057 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171057&action=edit Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157637|0 |1 is obsolete| | --- Comment #97 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 171059 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171059&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157638|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Peter Vashchuk <stalkernoid@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171054|0 |1 is obsolete| | Attachment #171055|0 |1 is obsolete| | Attachment #171056|0 |1 is obsolete| | Attachment #171057|0 |1 is obsolete| | Attachment #171058|0 |1 is obsolete| | Attachment #171059|0 |1 is obsolete| | --- Comment #100 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 176243 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176243&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #102 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 176245 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176245&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #104 from Peter Vashchuk <stalkernoid@gmail.com> --- Created attachment 176247 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176247&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #106 from Peter Vashchuk <stalkernoid@gmail.com> --- Rebased the changes on the current main and fixed sha1 errors -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176243|0 |1 is obsolete| | --- Comment #107 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176408 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176408&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176245|0 |1 is obsolete| | --- Comment #109 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176410 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176410&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176247|0 |1 is obsolete| | --- Comment #111 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176412&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #113 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176414&action=edit Bug 25711: (QA follow-up) Re-base fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176409|0 |1 is obsolete| | --- Comment #115 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176416&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176411|0 |1 is obsolete| | --- Comment #117 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176418 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176418&action=edit Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #119 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176420 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176420&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176408|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176413|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |Main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #121 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 1) Terminology I know this is rather old code now a this has been filed in 2020 and the first time set to NSO in the same year... but I wonder if not some of the reserves could have been turned into holds to get us somewhat closer to the goal of better more consistent terminology. a) circulation rule value For example the circulation rules: onshelfholds holds_per_record holds_per_day opacitemholds ... and reservesallowed. ... and with this patch: expire_reserves_charge b) GUI (fixed in follow-up) We are not going to have reserve used in the GUI. Updated "Expire reserve charge" to "Expired hold charge" 2) Validate input (fixed in follow-up) We have since added some validation on the circulation rules form. 3) Use $Price for display (fixed in follow-up) We need to display the saved charge formatted. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 --- Comment #122 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Documentation| |Caroline Cyr La Rose contact| | Documentation| |https://gitlab.com/koha-com submission| |munity/koha-manual/-/merge_ | |requests/1083 Keywords|release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25711 Aude Charillon <aude.charillon@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aude.charillon@openfifth.co | |.uk Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #124 from Aude Charillon <aude.charillon@openfifth.co.uk> --- Manual updated by Caroline Cyr La Rose. -- You are receiving this mail because: You are watching all bug changes.