Bug 39405

Summary: Add plugin hook `overwrite_calc_fine` to override fine calculation
Product: Koha Reporter: Michaela Sieber <michaela.sieber>
Component: Plugin architectureAssignee: Raphael Straub <raphael.straub>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: arthur.suzuki, clemens.tubach, jan.kissig, jonathan.druart, katrin.fischer, martin.renvoize, nick, paul.derscheid, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: Sponsored
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
This new hook allows to overwrite the internal Koha standard calculation of fines. This can be useful if your library needs to implement its own algorithm. For example, if it has a fine policy with graduated fines per overdue letter.
Version(s) released in:
25.05.00,24.11.04
Circulation function:
Bug Depends on:    
Bug Blocks: 39540    
Attachments: Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm
Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm
Bug 39405: (follow-up) Simplify code for choosing the first fine values
Bug 39405: Add more plugin examples to build tests
Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm
Bug 39405: (follow-up) Simplify code for choosing the first fine values
Bug 39405: Add more plugin examples to build tests
Bug 39405: (follow-up) Add the parameter borrowernumber to the overwrite_calc_fine plugin hook
Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm
Bug 39405: (follow-up) Simplify code for choosing the first fine values
Bug 39405: Add more plugin examples to build tests
Bug 39405: (follow-up) Add the parameter borrowernumber to the overwrite_calc_fine plugin hook
Bug 39405: Add a warning to the circulation rules page if CalcFine is overwritten.
Bug 39405: Add hook overwrite_calc_fine to override CalcFine in Overdues.pm
Bug 39405: (follow-up) Simplify code for choosing the first fine values
Bug 39405: Add more plugin examples to build tests
Bug 39405: (follow-up) Add the parameter borrowernumber to the overwrite_calc_fine plugin hook

Description Michaela Sieber 2025-03-21 07:53:41 UTC
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
Comment 1 Raphael Straub 2025-03-21 10:41:43 UTC
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)
Comment 2 Paul Derscheid 2025-03-31 08:08:24 UTC
Please also document the new hook on the wiki and ask for a commit to the kitchen-sink-plugin on github.
Comment 3 Jonathan Druart 2025-03-31 08:41:25 UTC
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-&gt;$method(@args) };
Comment 4 Jonathan Druart 2025-03-31 08:45:18 UTC
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?)
Comment 5 Arthur Suzuki 2025-03-31 08:46:44 UTC
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 :)
Comment 6 Clemens Tubach 2025-03-31 13:38:11 UTC
(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.
Comment 7 Raphael Straub 2025-04-01 08:28:52 UTC
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)
Comment 8 Raphael Straub 2025-04-01 08:33:26 UTC
(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.
Comment 9 Tomás Cohen Arazi (tcohen) 2025-04-01 09:57:02 UTC
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>
Comment 10 Tomás Cohen Arazi (tcohen) 2025-04-01 10:03:08 UTC
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.
Comment 11 Tomás Cohen Arazi (tcohen) 2025-04-01 10:12:30 UTC
Created attachment 180127 [details] [review]
Bug 39405: Add more plugin examples to build tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi (tcohen) 2025-04-01 10:13:59 UTC
(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.
Comment 13 Raphael Straub 2025-04-01 12:53:13 UTC
(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.
Comment 14 Tomás Cohen Arazi (tcohen) 2025-04-02 13:13:54 UTC
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>
Comment 15 Tomás Cohen Arazi (tcohen) 2025-04-02 13:13:56 UTC
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>
Comment 16 Tomás Cohen Arazi (tcohen) 2025-04-02 13:13:59 UTC
Created attachment 180344 [details] [review]
Bug 39405: Add more plugin examples to build tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi (tcohen) 2025-04-02 13:16:58 UTC
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.
Comment 18 Raphael Straub 2025-04-03 07:52:45 UTC
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)
Comment 19 Raphael Straub 2025-04-03 07:55:40 UTC
I added another patch to add the borrowernumber as a parameter to the plugin hook. We need the borrowernumber to implement graduated fines.
Comment 20 Raphael Straub 2025-04-03 08:03:56 UTC
(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.
Comment 21 Tomás Cohen Arazi (tcohen) 2025-04-03 08:18:11 UTC
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>
Comment 22 Tomás Cohen Arazi (tcohen) 2025-04-03 08:18:14 UTC
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>
Comment 23 Tomás Cohen Arazi (tcohen) 2025-04-03 08:18:17 UTC
Created attachment 180441 [details] [review]
Bug 39405: Add more plugin examples to build tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi (tcohen) 2025-04-03 08:18:20 UTC
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)
Comment 25 Tomás Cohen Arazi (tcohen) 2025-04-03 08:24:18 UTC
(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.
Comment 26 Raphael Straub 2025-04-03 09:59:50 UTC
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)
Comment 27 Raphael Straub 2025-04-03 10:15:30 UTC
(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?
Comment 28 Michaela Sieber 2025-04-03 12:11:38 UTC
(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
Comment 29 Martin Renvoize (ashimema) 2025-04-03 12:57:25 UTC
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>
Comment 30 Martin Renvoize (ashimema) 2025-04-03 12:57:29 UTC
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>
Comment 31 Martin Renvoize (ashimema) 2025-04-03 12:57:33 UTC
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>
Comment 32 Martin Renvoize (ashimema) 2025-04-03 12:57:37 UTC
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>
Comment 33 Martin Renvoize (ashimema) 2025-04-03 13:03:42 UTC
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.
Comment 34 Katrin Fischer 2025-04-25 06:39:21 UTC
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
Comment 35 Katrin Fischer 2025-04-25 06:49:12 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 36 Michaela Sieber 2025-04-25 09:03:27 UTC
Could this please be backported?
Comment 37 Paul Derscheid 2025-05-17 18:37:10 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.04 (v24.11.04-2)