This new hook allows to overwrite the koha standard calculation of fines. This can be useful if your library has a fine policy with graduated fines per overdue letter (very popular in german academic libraries) This hook is implemented in the CalcFine method which is defined in overdue.pm
Created attachment 179582 [details] [review] Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm This new hook allows to overwrite the koha standard calculation of fines. This can be useful if your library has a fine policy with graduated fines per overdue letter. To test: 1) Apply patch. 2) Run prove -v t/db_dependent/Circulation/CalcFine.t 3) Run prove -v t/db_dependent/Koha/Plugins/Overdues.t 4) Sign off. Sponsored-by: Karlsruhe Institute of Technology (KIT)
Please also document the new hook on the wiki and ask for a commit to the kitchen-sink-plugin on github.
Note: Ideally we should return the same structure than CalCFine (ie. an array, not an arrayref). But we cannot: Koha/Plugins.pm 80 sub call { 95 my $response = eval { $plugin->$method(@args) };
For discussion: The first parameter "item" is confusing, it's not a Koha::Item object. Maybe we should pass something that is not too tight to the current signature of CalcFine (so the plugin won't break when/if we change it). If you need item's info, then pass the item object (or the item's id?)
there should be a check on the circulation rules definition page, if any installed plugin implement this hook. You could check if any plugin implement the hook by using this code (found in C4/Letters.pm) ---- if ( C4::Context->config("enable_plugins") ) { my @plugins = Koha::Plugins->new->GetPlugins( { method => 'before_send_messages', } ); ---- That could be in another dependent bug :)
(In reply to Jonathan Druart from comment #4) > For discussion: > The first parameter "item" is confusing, it's not a Koha::Item object. > > Maybe we should pass something that is not too tight to the current > signature of CalcFine (so the plugin won't break when/if we change it). > > If you need item's info, then pass the item object (or the item's id?) I talked with Raphael and Paul. We will pass the itemnumber and get the Koha:item into the plugin.
Created attachment 180095 [details] [review] Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm This new hook allows to overwrite the koha standard calculation of fines. This can be useful if your library has a fine policy with graduated fines per overdue letter. To test: 1) Apply patch. 2) Run prove -v t/db_dependent/Circulation/CalcFine.t 3) Run prove -v t/db_dependent/Koha/Plugins/Overdues.t 4) Sign off. Sponsored-by: Karlsruhe Institute of Technology (KIT)
(In reply to Clemens Tubach from comment #6) > We will pass the itemnumber and get the Koha:item > into the plugin. A new patch that does this is attached.
Created attachment 180111 [details] [review] Bug 39405: (follow-up) Simplify code for choosing the first fine values This patch avoids the introduction of a `foreach` loop and a `return` call inside of it, by using grep on the array assignment, and then just returning the first element on it, if any. No behavior change is expected. The code becomes easier to read and spot what is doing. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Hi Ralph. Good job here. A couple QA remarks: - In the tests file you introduce, you correctly add a subtest that wraps all actions inside a DB transaction. [GOOD] - Even though you wrap the subtest inside a transaction, some data is generated OUTSIDE the transaction i.e. `$branch` and `$category`. Those two should be generated inside the subtest and within the transaction. Otherwise running the test pollutes the DB [BLOCKER] - The hook is added to Koha::Plugin::Test [GOOD] - The fact that your code skips 'undefined result' plugin hook calls is not tested [BLOCKER] - The fact your code will pick the first available value is not tested [BLOCKER] I will submit a patch now, creating two new sample plugins you can use for writing those tests.
Created attachment 180127 [details] [review] Bug 39405: Add more plugin examples to build tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi (tcohen) from comment #10) > I will submit a patch now, creating two new sample plugins you can use for > writing those tests. Added 3 plugins, actually.
(In reply to Tomás Cohen Arazi (tcohen) from comment #10) > - The fact that your code skips 'undefined result' plugin hook calls is not > tested [BLOCKER] > - The fact your code will pick the first available value is not tested > [BLOCKER] I think, we cannot test these two facts as long as Koha::Plugins->call() calls the plugin hooks and returns the results in an arbitrary order.
Created attachment 180342 [details] [review] Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm This new hook allows to overwrite the koha standard calculation of fines. This can be useful if your library has a fine policy with graduated fines per overdue letter. To test: 1) Apply patch. 2) Run prove -v t/db_dependent/Circulation/CalcFine.t 3) Run prove -v t/db_dependent/Koha/Plugins/Overdues.t 4) Sign off. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 180343 [details] [review] Bug 39405: (follow-up) Simplify code for choosing the first fine values This patch avoids the introduction of a `foreach` loop and a `return` call inside of it, by using grep on the array assignment, and then just returning the first element on it, if any. No behavior change is expected. The code becomes easier to read and spot what is doing. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 180344 [details] [review] Bug 39405: Add more plugin examples to build tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I added the requested tests aiming to help this patchset make it to the crossing line. I still think, as discussed in person at Marseille, that some warning should be displayed on the circulation page when a plugin overrides rules. As Arthur pointed out, it is not a terribly hard thing to do. I leave it to the QA person to decide if it should be done here or in a separate bug.
Created attachment 180433 [details] [review] Bug 39405: (follow-up) Add the parameter borrowernumber to the overwrite_calc_fine plugin hook This patch adds the borrowernumber as a parameter to the overwrite_calc_fine plugin hook. E.g., the borrower is needed for calculating graduated fines. Sponsored-by: Karlsruhe Institute of Technology (KIT)
I added another patch to add the borrowernumber as a parameter to the plugin hook. We need the borrowernumber to implement graduated fines.
(In reply to Tomás Cohen Arazi (tcohen) from comment #17) > I added the requested tests aiming to help this patchset make it to the > crossing line. Thanks. > I still think, as discussed in person at Marseille, that some warning should > be displayed on the circulation page when a plugin overrides rules. > > As Arthur pointed out, it is not a terribly hard thing to do. I'll look into it.
Created attachment 180439 [details] [review] Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm This new hook allows to overwrite the koha standard calculation of fines. This can be useful if your library has a fine policy with graduated fines per overdue letter. To test: 1) Apply patch. 2) Run prove -v t/db_dependent/Circulation/CalcFine.t 3) Run prove -v t/db_dependent/Koha/Plugins/Overdues.t 4) Sign off. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 180440 [details] [review] Bug 39405: (follow-up) Simplify code for choosing the first fine values This patch avoids the introduction of a `foreach` loop and a `return` call inside of it, by using grep on the array assignment, and then just returning the first element on it, if any. No behavior change is expected. The code becomes easier to read and spot what is doing. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 180441 [details] [review] Bug 39405: Add more plugin examples to build tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 180442 [details] [review] Bug 39405: (follow-up) Add the parameter borrowernumber to the overwrite_calc_fine plugin hook This patch adds the borrowernumber as a parameter to the overwrite_calc_fine plugin hook. E.g., the borrower is needed for calculating graduated fines. Sponsored-by: Karlsruhe Institute of Technology (KIT)
(In reply to Raphael Straub from comment #20) > > I still think, as discussed in person at Marseille, that some warning should > > be displayed on the circulation page when a plugin overrides rules. > > > > As Arthur pointed out, it is not a terribly hard thing to do. > I'll look into it. I filed bug 39540 for this. Lets discuss it there.
Created attachment 180486 [details] [review] Bug 39405: Add a warning to the circulation rules page if CalcFine is overwritten. This patch adds a warning to the "Circulation and fine rules" page in "Koha administration" if there is an enabled plugin that implements the overwrite_calc_fine plugin hook. To test: 1) Apply the patch. 2) In your browser go to /cgi-bin/koha/admin/smart-rules.pl and check that there is no warning on the top of the page. 3) Install a plugin that implements overwrite_calc_fine. 4) Reload /cgi-bin/koha/admin/smart-rules.pl and check that now there is a warning that displays the installed plugin's name. Sponsored-by: Karlsruhe Institute of Technology (KIT)
(In reply to Tomás Cohen Arazi (tcohen) from comment #25) > I filed bug 39540 for this. Lets discuss it there. I accidentally attached the patch here before I saw the new bug. Is there a way to delete the patch in this bug?
(In reply to Raphael Straub from comment #27) > (In reply to Tomás Cohen Arazi (tcohen) from comment #25) > > I filed bug 39540 for this. Lets discuss it there. > > I accidentally attached the patch here before I saw the new bug. Is there a > way to delete the patch in this bug? I marked it as obsolete so you can upload the patch to Bug 39540
Created attachment 180505 [details] [review] Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm This new hook allows to overwrite the koha standard calculation of fines. This can be useful if your library has a fine policy with graduated fines per overdue letter. To test: 1) Apply patch. 2) Run prove -v t/db_dependent/Circulation/CalcFine.t 3) Run prove -v t/db_dependent/Koha/Plugins/Overdues.t 4) Sign off. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 180506 [details] [review] Bug 39405: (follow-up) Simplify code for choosing the first fine values This patch avoids the introduction of a `foreach` loop and a `return` call inside of it, by using grep on the array assignment, and then just returning the first element on it, if any. No behavior change is expected. The code becomes easier to read and spot what is doing. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 180507 [details] [review] Bug 39405: Add more plugin examples to build tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 180508 [details] [review] Bug 39405: (follow-up) Add the parameter borrowernumber to the overwrite_calc_fine plugin hook This patch adds the borrowernumber as a parameter to the overwrite_calc_fine plugin hook. E.g., the borrower is needed for calculating graduated fines. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
The perfectionist in me would love to adjust the parameters we pass to the plugin here.. I'd love to see a Koha::Checkout object passed which could then easily be used to fetch cached patron, item etc etc information. However.. the pragmatist in me knows that's out of scope and would mean adding either unrequited DB hits within CalcFine itself or reworking the callers to pass this information instead of passing plain hashes and strings. So.. with that in mind I'm going to PQA.
Thanks for all the work and help with this one! 1) Please remember to update wiki 2) Not sure yet if this will break tests now, but I believe it might need fixing after bug 39367: FAIL t/lib/plugins/Koha/Plugin/1_CalcFineEmpty.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 FAIL t/lib/plugins/Koha/Plugin/2_CalcFineNotEmpty.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 FAIL t/lib/plugins/Koha/Plugin/3_CalcFineBadValue.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
Pushed for 25.05! Well done everyone, thank you!
Could this please be backported?
Nice work everyone! Pushed to 24.11.x for 24.11.04 (v24.11.04-2)