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
Created attachment 107144 [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 it’s undefined in circulation rules it uses global ExpireReservesMaxPickUpDelayCharge which was used before this 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 itemtype 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 no smart rule will be found or smart rules have an "empty" fee – global ExpireReservesMaxPickUpDelayCharge variable value will be used. IMPORTANT: Different actions when rules applied to biblio and item level holds: - if there is biblio-level hold ("next available") then biblio item type used for rule preselection - if there is direct item-level hold for particular barcode, then item "item type" will be used for rule preselection In most cases we have the same items as biblio item types, but in case there's different item types: be accurate and attentive with rules creation and overlapping. Mentored-by: Andrew Nugged <nugged@gmail.com>
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.
Created attachment 107407 [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>
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
Created attachment 107683 [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>
Created attachment 107844 [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>
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>
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.
(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.
I think we should completely remove ExpireReservesMaxPickUpDelayCharge syspref now and move it's value with a DB upgrade to the default circ rule.
(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?
(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.
(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.
(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.
*** Bug 18195 has been marked as a duplicate of this bug. ***
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>
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>
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>
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 :)
Created attachment 120164 [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>
Unsetting myself from QA contact, I don't think I am able to find time reviewing this any time soon.
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>
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>
Rebased on the latest master
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>
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>
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
Created attachment 123452 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already
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.
I think we lost part of the logic after changes (as I remember it was tricky): The goal was: when we calling "get_effective_rule" for "expire_reserves_charge" we expect to have any number _including_ zero as value for real charge value, and in case we have '' (blank) in the rule, we get "ExpireReservesMaxPickUpDelayCharge" instead, as default. Look for commit description test/check scenario: ... 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. but now: there is the replacement in patch 123328: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123328&action=diff#a/Koha/Hold.pm_sec1 This: my $rule_value = $rule && $rule->rule_value // ''; $charge = $rule_value ne '' ? $rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); is replaced by: $charge = $rule ? $rule->rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); and the new version goes only for "Perl truth" so it doesn't differentiate between 0 and '' (under as well) and all goes then to "Default", but we should have 0 instead of Default if we have zero in $rule->rule_value. Petro, let's check why tests are passing :)...
> 1. Tiny wording change suggestion, do you agree with the last patch? yes, "Default" is good
(In reply to Jonathan Druart from comment #29) > 3. > - } else if ( i == 5 || i == 6 || i == 26 || i == 27 > || i == 28 ) { > + } else if ( i == 5 || i == 6 || i == 17 || i == 27 > || i == 28 || i == 29 ) { > > I think there is something wrong here. Why did you remove 26?... A new 17th column was added, so the number of columns increased, and relative numbers for the next ones shifted, in short, 26th column wasn't removed, it just became 27th, so 26 27 28 are 27 28 29 accordingly.
> 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.
We tested once more with Petro: Nick's update works, so it's ok, I was confused that we might overlap 0 in "$rule ? $rule->rule_value : C4::Context->preference..." but we before "?" checking for RULE as _object_, not rule value (my confusion :) ), so even if rule_value is 0, object anyway will be truth, so zero returned, and logic works. And in the "Default" case in rule, we store "undef", which leads to full rule absence in DB, so again: checking for the rule as object fails and we falling back to Default. All ok with logic, yeah.
Created attachment 123456 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object.
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!)
Created attachment 123498 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule.
+ 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.
(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 )
(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!
Created attachment 123568 [details] [review] Bug 25711: add comment to clarify how the method works
Created attachment 123610 [details] [review] Bug 25711: add comment to clarify how the method works
Created attachment 123653 [details] [review] Bug 25711: tests for get_effective_expire_reserves_charge method new tests that test logic of new get_effective_expire_reserves_charge method that was added to Koha::CirculationRules to retrieve the effective value of the rule.
Created attachment 123661 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic.
Created attachment 135060 [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>
Created attachment 135061 [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>
Created attachment 135062 [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
Created attachment 135063 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already
Created attachment 135064 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object.
Created attachment 135065 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic.
Rebased on latest master
Martin, can you retest this one?
Created attachment 135156 [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>
Created attachment 135157 [details] [review] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135158 [details] [review] Bug 25711: (QA follow-up) Move tests and disallow blank rule The tests can go where the existing cancellation charge tests are. If we define the default rule we do not need to delete all circulation rules as the itemtypes and branches are all created for the tests The rule should not be saved if blank, this allows simplifying the logic when getting the charge Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135159 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135160 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135161 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All working well still.. added my SO lines to the subsequent patches that have come in :)
Created attachment 135950 [details] [review] Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge in circulation rules per specific items/patron categories. When value not found in circulation rules it uses global ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules feature was introduced. To check the feature after applying the patch: 1) Go to /cgi-bin/koha/admin/preferences.pl and set ExpireReservesMaxPickUpDelay to allow. 1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table and notice the newly added "Expire reserve charge" row. 2) In that row add numerical values to type/patron categories that you will be using later to test this feature. 3) Prepare an item for our next step, pick a biblio that has item with the same item type you altered in smart rules and check it out. 3) Place a hold on that item and set the expiration date as the next day. 4) Check in it after that. 5) Do SQL request to change expiration date manually to "yesterday": in "reserves" table, find the id of the hold that you placed and "UPDATE reserves SET `expirationdate`='2020-07-19' WHERE `reserve_id`=329;" Or alternatively: 5) Wait for tomorrow, change server time or invent a time machine. 6) Run cancel_expired_holds.pl cron job. 7) Check that patron got charged in "accounting" left tab and "transactions" upper tab: your patron should have debt of that amount of fee you assigned in that rule (/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...) 8) You can repeat this with other rules and different numbers/biblio/item types to check that different fees apply according to smart rules. 9) If there's no matching smart rule or smart rule have an empty string as a fee - global ExpireReservesMaxPickUpDelayCharge variable value will be used. 10) But in case if matching rule has zero ("0") as a value - that allows to have no-fee smart-rule results, as a feature. IMPORTANT: Smart-rules filters only applied to item-level holds when item-type for items is known. If there's biblio-level hold, in that case global ExpireReservesMaxPickUpDelayCharge value will be used). Mentored-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 135951 [details] [review] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 135952 [details] [review] Bug 25711: (QA follow-up) Move tests and disallow blank rule The tests can go where the existing cancellation charge tests are. If we define the default rule we do not need to delete all circulation rules as the itemtypes and branches are all created for the tests The rule should not be saved if blank, this allows simplifying the logic when getting the charge Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 135953 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 135954 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 135955 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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)
Created attachment 136769 [details] [review] Bug 25711: Deprecate ExpireReservesMaxPickUpDelayCharge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
Created attachment 140377 [details] [review] Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge in circulation rules per specific items/patron categories. When value not found in circulation rules it uses global ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules feature was introduced. To check the feature after applying the patch: 1) Go to /cgi-bin/koha/admin/preferences.pl and set ExpireReservesMaxPickUpDelay to allow. 1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table and notice the newly added "Expire reserve charge" row. 2) In that row add numerical values to type/patron categories that you will be using later to test this feature. 3) Prepare an item for our next step, pick a biblio that has item with the same item type you altered in smart rules and check it out. 3) Place a hold on that item and set the expiration date as the next day. 4) Check in it after that. 5) Do SQL request to change expiration date manually to "yesterday": in "reserves" table, find the id of the hold that you placed and "UPDATE reserves SET `expirationdate`='2020-07-19' WHERE `reserve_id`=329;" Or alternatively: 5) Wait for tomorrow, change server time or invent a time machine. 6) Run cancel_expired_holds.pl cron job. 7) Check that patron got charged in "accounting" left tab and "transactions" upper tab: your patron should have debt of that amount of fee you assigned in that rule (/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...) 8) You can repeat this with other rules and different numbers/biblio/item types to check that different fees apply according to smart rules. 9) If there's no matching smart rule or smart rule have an empty string as a fee - global ExpireReservesMaxPickUpDelayCharge variable value will be used. 10) But in case if matching rule has zero ("0") as a value - that allows to have no-fee smart-rule results, as a feature. IMPORTANT: Smart-rules filters only applied to item-level holds when item-type for items is known. If there's biblio-level hold, in that case global ExpireReservesMaxPickUpDelayCharge value will be used). Mentored-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 140378 [details] [review] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 140379 [details] [review] Bug 25711: (QA follow-up) Move tests and disallow blank rule The tests can go where the existing cancellation charge tests are. If we define the default rule we do not need to delete all circulation rules as the itemtypes and branches are all created for the tests The rule should not be saved if blank, this allows simplifying the logic when getting the charge Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 140380 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 140381 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 140382 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 140383 [details] [review] Bug 25711: Deprecate ExpireReservesMaxPickUpDelayCharge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Rebased over current master. @Petro, yes, let's kick this up, I need this in production!
Created attachment 141270 [details] [review] Bug 25711: (QA follow-up) Remove leftover code and fix atomic update This patch finalizes another one which deprecates ExpireReservesMaxPickUpDelayCharge. Because we had multiple smart-rules with undefined "expire_reserves_charge" values, this patch updates actomic update to set not only the general All/All smart-rule row to deprecated ExpireReservesMaxPickUpDelayCharge value but also every smart-rule that fall back to default value in "expire_reserves_charge". Also removed additional leftovers in template which earlier expected to show hyperlink to syspref ExpireReservesMaxPickUpDelay. To check if atomic update functions correctly: 1) Go to syspref and set ExpireReservesMaxPickUpDelay as "allow", set ExpireReservesMaxPickUpDelayCharge to some numerical value (that will be your default value), for example "42". 2) Go to smart rules page and create three rules: a rule with empty "Expire reserve charge" value, then create another one rule with "Expire reserve charge" vaue "0", and then another rule with "Expire reserve charge" value set to any number, for example "12". 3) Apply the patch, and run the atomic update via the installer page. 4) Return to the smart rules page and check those rules you created in step 2: - That one rule that you set as empty, now should have "Expire reserve charge" equal 42, i.e. that one you set to the value you set ExpireReservesMaxPickUpDelayCharge in step 1. - All rules that had numerical values set previously should have been exist untouched.
Sorry chaps, this needs another rebase.. I managed the first one fairly easily, but then I got sh1 issues and missing hunks. Any chance you could rebase it for us?
Still on your horizon Andrii?
Created attachment 157632 [details] [review] Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge in circulation rules per specific items/patron categories. When value not found in circulation rules it uses global ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules feature was introduced. To check the feature after applying the patch: 1) Go to /cgi-bin/koha/admin/preferences.pl and set ExpireReservesMaxPickUpDelay to allow. 1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table and notice the newly added "Expire reserve charge" row. 2) In that row add numerical values to type/patron categories that you will be using later to test this feature. 3) Prepare an item for our next step, pick a biblio that has item with the same item type you altered in smart rules and check it out. 3) Place a hold on that item and set the expiration date as the next day. 4) Check in it after that. 5) Do SQL request to change expiration date manually to "yesterday": in "reserves" table, find the id of the hold that you placed and "UPDATE reserves SET `expirationdate`='2020-07-19' WHERE `reserve_id`=329;" Or alternatively: 5) Wait for tomorrow, change server time or invent a time machine. 6) Run cancel_expired_holds.pl cron job. 7) Check that patron got charged in "accounting" left tab and "transactions" upper tab: your patron should have debt of that amount of fee you assigned in that rule (/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...) 8) You can repeat this with other rules and different numbers/biblio/item types to check that different fees apply according to smart rules. 9) If there's no matching smart rule or smart rule have an empty string as a fee - global ExpireReservesMaxPickUpDelayCharge variable value will be used. 10) But in case if matching rule has zero ("0") as a value - that allows to have no-fee smart-rule results, as a feature. IMPORTANT: Smart-rules filters only applied to item-level holds when item-type for items is known. If there's biblio-level hold, in that case global ExpireReservesMaxPickUpDelayCharge value will be used). Mentored-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 157633 [details] [review] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 157634 [details] [review] Bug 25711: (QA follow-up) Move tests and disallow blank rule The tests can go where the existing cancellation charge tests are. If we define the default rule we do not need to delete all circulation rules as the itemtypes and branches are all created for the tests The rule should not be saved if blank, this allows simplifying the logic when getting the charge Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 157635 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 157636 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 157637 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 157638 [details] [review] Bug 25711: Deprecate ExpireReservesMaxPickUpDelayCharge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 157639 [details] [review] Bug 25711: (QA follow-up) Remove leftover code and fix atomic update This patch finalizes another one which deprecates ExpireReservesMaxPickUpDelayCharge. Because we had multiple smart-rules with undefined "expire_reserves_charge" values, this patch updates actomic update to set not only the general All/All smart-rule row to deprecated ExpireReservesMaxPickUpDelayCharge value but also every smart-rule that fall back to default value in "expire_reserves_charge". Also removed additional leftovers in template which earlier expected to show hyperlink to syspref ExpireReservesMaxPickUpDelay. To check if atomic update functions correctly: 1) Go to syspref and set ExpireReservesMaxPickUpDelay as "allow", set ExpireReservesMaxPickUpDelayCharge to some numerical value (that will be your default value), for example "42". 2) Go to smart rules page and create three rules: a rule with empty "Expire reserve charge" value, then create another one rule with "Expire reserve charge" vaue "0", and then another rule with "Expire reserve charge" value set to any number, for example "12". 3) Apply the patch, and run the atomic update via the installer page. 4) Return to the smart rules page and check those rules you created in step 2: - That one rule that you set as empty, now should have "Expire reserve charge" equal 42, i.e. that one you set to the value you set ExpireReservesMaxPickUpDelayCharge in step 1. - All rules that had numerical values set previously should have been exist untouched.
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.
Do you need again a signoff or it's good ? Because I can't find the field "Expire reserve charge" when I want to create smart rules.
This one needs a rebase for current master.
Any update here?
Created attachment 171054 [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>
Created attachment 171055 [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>
Created attachment 171056 [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
Created attachment 171057 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already
Created attachment 171058 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object.
Created attachment 171059 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic.
Rebased all changes onto the current master.
Do you have a branch for this somewhere public Petro? I'm getting sha1 errors on trying to apply.
Created attachment 176243 [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>
Created attachment 176244 [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>
Created attachment 176245 [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
Created attachment 176246 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already
Created attachment 176247 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object.
Created attachment 176248 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic.
Rebased the changes on the current main and fixed sha1 errors
Created attachment 176408 [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>
Created attachment 176409 [details] [review] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176410 [details] [review] Bug 25711: (QA follow-up) Move tests and disallow blank rule The tests can go where the existing cancellation charge tests are. If we define the default rule we do not need to delete all circulation rules as the itemtypes and branches are all created for the tests The rule should not be saved if blank, this allows simplifying the logic when getting the charge Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176411 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176412 [details] [review] Bug 25711: fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176413 [details] [review] Bug 25711: move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176414 [details] [review] Bug 25711: (QA follow-up) Re-base fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176415 [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>
Created attachment 176416 [details] [review] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176417 [details] [review] Bug 25711: (QA follow-up) Move tests and disallow blank rule The tests can go where the existing cancellation charge tests are. If we define the default rule we do not need to delete all circulation rules as the itemtypes and branches are all created for the tests The rule should not be saved if blank, this allows simplifying the logic when getting the charge Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176418 [details] [review] Bug 25711: Wording 'use global' > 'Default' We use 'Default' for useDaysMode already Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176419 [details] [review] Bug 25711: Fix existing tests and add new one Add a new test with circulation rule for library_C_code that has set expire_reserves_charge which is 0. In this case 0 should be used as the charge. Also use build_object instead of build to build the koha object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176420 [details] [review] Bug 25711: Move get_effective_expire_charge to Koha::CirculationRules Move get_effective_expire_reserves_charge as a new method in Koha::CirculationRules to retrieve the effective value of the rule, which means it will get the value from existing circulation rule and if it doesn't exist or set to undefined it will get the default value from the preferences. Also added tests for this logic. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176421 [details] [review] Bug 25711: (QA follow-up) Re-base fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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.
Pushed for 25.05! Well done everyone, thank you!