Bug 25855 - Add hook to AddRenewal using a new _after_circ_actions method in circulation
Summary: Add hook to AddRenewal using a new _after_circ_actions method in circulation
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 21468 25938 26063 26108
  Show dependency treegraph
 
Reported: 2020-06-23 16:04 UTC by Tomás Cohen Arazi
Modified: 2020-11-12 15:50 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Text to go in the release notes:
This enhancement adds plugin hooks to allow plugins to take action after renewal circulation events.
Version(s) released in:
20.11.00


Attachments
Bug 25855: Unit tests (4.79 KB, patch)
2020-06-23 19:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25855: Add a post_renewal_action plugin hook (2.61 KB, patch)
2020-06-23 19:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25855: Unit tests (4.86 KB, patch)
2020-06-24 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25855: Add a post_renewal_action plugin hook (2.68 KB, patch)
2020-06-24 13:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25855: (QA follow-up) Generalize hook and simplify tests (6.95 KB, patch)
2020-07-03 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25855: Unit tests (4.90 KB, patch)
2020-07-06 07:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25855: Add a post_renewal_action plugin hook (2.73 KB, patch)
2020-07-06 07:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25855: (QA follow-up) Generalize hook and simplify tests (7.00 KB, patch)
2020-07-06 07:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25855: (QA follow-up) Simplify payload (4.25 KB, patch)
2020-07-23 14:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25855: (QA follow-up) Simplify payload (4.33 KB, patch)
2020-07-23 14:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-06-23 16:04:02 UTC
We need a hook to trigger plugin actions after renweals take place.
Comment 1 Tomás Cohen Arazi 2020-06-23 19:29:09 UTC
Created attachment 106215 [details] [review]
Bug 25855: Unit tests

This patch adds tests fr a new circulation hook for plugins.
In this case the post_renewal_action hook,

The tests add the hook to the Test plugin, and verify that all the
required parameters are passed for the plugin hook to use them.
It relies on throwing an exception that is to be caught.

Sponsored-by: ByWater Solutions
Comment 2 Tomás Cohen Arazi 2020-06-23 19:29:13 UTC
Created attachment 106216 [details] [review]
Bug 25855: Add a post_renewal_action plugin hook

This patch adds a new hook to notify plugins about renewal actions. To
test it:

1. Apply the unit tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Circulation_hooks.t
=> FAIL: The tests expect some warnings to show, and they don't (i.e.
        the sample plugin hook is not being called).
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The hook is being called on renewal!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 3 Kyle M Hall 2020-06-24 13:12:58 UTC
Created attachment 106248 [details] [review]
Bug 25855: Unit tests

This patch adds tests fr a new circulation hook for plugins.
In this case the post_renewal_action hook,

The tests add the hook to the Test plugin, and verify that all the
required parameters are passed for the plugin hook to use them.
It relies on throwing an exception that is to be caught.

Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall 2020-06-24 13:13:09 UTC
Created attachment 106249 [details] [review]
Bug 25855: Add a post_renewal_action plugin hook

This patch adds a new hook to notify plugins about renewal actions. To
test it:

1. Apply the unit tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Circulation_hooks.t
=> FAIL: The tests expect some warnings to show, and they don't (i.e.
        the sample plugin hook is not being called).
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The hook is being called on renewal!
5. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Katrin Fischer 2020-06-25 06:50:19 UTC
Please remember to document on the wiki:
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks Thx!
Comment 6 Tomás Cohen Arazi 2020-06-26 15:55:18 UTC
(In reply to Katrin Fischer from comment #5)
> Please remember to document on the wiki:
> https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks Thx!

Done!
Comment 7 Martin Renvoize 2020-07-02 11:27:22 UTC
Comment on attachment 106248 [details] [review]
Bug 25855: Unit tests

Review of attachment 106248 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Koha/Plugins/Circulation_hooks.t
@@ +68,5 @@
> +            'AddBiblio calls the hook with action=create';
> +
> +    warning_like { $item = $builder->build_sample_item({ biblionumber => $biblio->biblionumber }); }
> +            qr/after_item_action called with action: create, ref: Koha::Item/,
> +            'AddItem calls the hook with action=create';

Hmm.. does it make sense to additionally test the AddBiblio and AddItem hooks here.. they're already well tested within Koha/Plugins/Biblio_and_Items_plugin_hooks.t aren't they?

@@ +72,5 @@
> +            'AddItem calls the hook with action=create';
> +
> +    warning_like { AddIssue( $patron->unblessed, $item->barcode ); }
> +            qr/after_item_action called with action: modify, ref: Koha::Item/,
> +            'AddItem calls the hook with action=modify';

'AddIssue' is being tested here.. not AddItem... must admit it feels a little strange that we trigger the item modification hook on an issue.. though I think I understand how that comes about.

@@ +77,5 @@
> +
> +    warnings_like { AddRenewal( $patron->borrowernumber, $item->id, $patron->branchcode ); }
> +            [ qr/after_item_action called with action: modify, ref: Koha::Item/,
> +              qr/post_renewal_action .* DateTime/ ],
> +            'AddRenewal calls the post_renewal_action hook';

I have a vague feeling renewals can be triggered via AddIssue as well as AddRenewal.... might be worth also testing that case to make sure you're always triggering as expected.
Comment 8 Martin Renvoize 2020-07-02 11:32:56 UTC
Comment on attachment 106249 [details] [review]
Bug 25855: Add a post_renewal_action plugin hook

Review of attachment 106249 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +4292,4 @@
>      return 0;
>  }
>  
> +=head3 _post_renewal_actions

Hmm.. could we perhaps be a bit more forward-thinking here and adopt a more generic approach akin to that taken in C4/Biblio.pm where we have '_after_biblio_actions_hooks' which accepts and action param.. perhaps we could have `_after_circ_action_hooks` here and then in followup bugs we can extend it easily to support the other actions available within the module?
Comment 9 Tomás Cohen Arazi 2020-07-03 13:25:18 UTC
Taking Martin's suggestion to generalize this hook I'm renaming it 'after_circ_action' and made the params be 'action' ('renewal' in this case) and 'payload' for all we want to pass the plugin.
Comment 10 Tomás Cohen Arazi 2020-07-03 13:40:10 UTC
Created attachment 106513 [details] [review]
Bug 25855: (QA follow-up) Generalize hook and simplify tests

This patch generalizes the hook so it can be used by other circulation
actions.

Tests are also simplified by mocking some of the (extensive) plugin
hooks.

To test:
1. Repeat the test plan on the original patch
=> SUCCESS: All good
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Martin Renvoize 2020-07-06 07:35:21 UTC
Created attachment 106568 [details] [review]
Bug 25855: Unit tests

This patch adds tests fr a new circulation hook for plugins.
In this case the post_renewal_action hook,

The tests add the hook to the Test plugin, and verify that all the
required parameters are passed for the plugin hook to use them.
It relies on throwing an exception that is to be caught.

Sponsored-by: ByWater Solutions
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-07-06 07:35:24 UTC
Created attachment 106569 [details] [review]
Bug 25855: Add a post_renewal_action plugin hook

This patch adds a new hook to notify plugins about renewal actions. To
test it:

1. Apply the unit tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Circulation_hooks.t
=> FAIL: The tests expect some warnings to show, and they don't (i.e.
        the sample plugin hook is not being called).
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! The hook is being called on renewal!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-07-06 07:35:27 UTC
Created attachment 106570 [details] [review]
Bug 25855: (QA follow-up) Generalize hook and simplify tests

This patch generalizes the hook so it can be used by other circulation
actions.

Tests are also simplified by mocking some of the (extensive) plugin
hooks.

To test:
1. Repeat the test plan on the original patch
=> SUCCESS: All good
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-07-06 07:35:58 UTC
Excellent work, Thanks Tomas

Works well, fully tested and passes QA scripts.

Passing QA
Comment 15 Katrin Fischer 2020-07-06 07:40:37 UTC
Please remember to update the wiki :)
Comment 16 Martin Renvoize 2020-07-06 08:37:25 UTC
Hmm, One final question actually.. now I've started to look into utilising the hook for AddIssue.

Do we need to add a 'type' parameter into the payload to distinguish between OPAC, Staff, SIP and Autorenewals?  It feels like something a plugin may want to know about and handle distinctly?
Comment 17 Tomás Cohen Arazi 2020-07-06 10:28:04 UTC
(In reply to Martin Renvoize from comment #16)
> Hmm, One final question actually.. now I've started to look into utilising
> the hook for AddIssue.
> 
> Do we need to add a 'type' parameter into the payload to distinguish between
> OPAC, Staff, SIP and Autorenewals?  It feels like something a plugin may
> want to know about and handle distinctly?

That's an excellent idea. Are you adding a follow-up here?
Comment 18 Martin Renvoize 2020-07-06 13:23:46 UTC
I'll file a bug... it's not a trivial addition and currently, we don thave a clear use case for it.. just a 'nice to have'.

Leaving this as PQA :)
Comment 19 Jonathan Druart 2020-07-23 10:02:33 UTC
A couple of questions:

1. Why do we have the hook before UpdateStats and logaction?
We could imagine a plugin that want to deal with them, and so get up-to-date info

2. A renewal is identified by an issue_id, why don't we only pass the issue_id to the plugin?
It could then retrieve the info it wants.
We should limit the number of params we pass, we only need to inform the plugin "yeah, we've just renewed this!".
It will make the plugins' api more robust and maintainable IMO.
Comment 20 Jonathan Druart 2020-07-23 10:05:10 UTC
Additional question: what happens (and what is the expected behaviour) if the plugin fails?
Comment 21 Jonathan Druart 2020-07-23 10:25:37 UTC
(In reply to Jonathan Druart from comment #20)
> Additional question: what happens (and what is the expected behaviour) if
> the plugin fails?

Related with patch "Bug 24031: Add safety checks in Koha::Plugins::call"
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105697
Comment 22 Tomás Cohen Arazi 2020-07-23 10:37:51 UTC
(In reply to Jonathan Druart from comment #20)
> Additional question: what happens (and what is the expected behaviour) if
> the plugin fails?

They are eval-ed.
Comment 23 Tomás Cohen Arazi 2020-07-23 10:46:05 UTC
(In reply to Jonathan Druart from comment #19)
> A couple of questions:
> 
> 1. Why do we have the hook before UpdateStats and logaction?
> We could imagine a plugin that want to deal with them, and so get up-to-date
> info
>
> 2. A renewal is identified by an issue_id, why don't we only pass the
> issue_id to the plugin?
> It could then retrieve the info it wants.
> We should limit the number of params we pass, we only need to inform the
> plugin "yeah, we've just renewed this!".
> It will make the plugins' api more robust and maintainable IMO.

Those are valid questions. About the stats calls, I only copied what we already do. 

The most relevant information is the issue_id and the new date due. My impression was that we could send all the info we already had. In my case I needed the itemnumber and date due so my plugin notified a third party system and no extra call to the DB is required.
Comment 24 Jonathan Druart 2020-07-23 10:51:24 UTC
At this point I'd privilege robustness and maintainability over extra calls :)
Comment 25 Tomás Cohen Arazi 2020-07-23 14:04:25 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2020-07-23 14:21:25 UTC
Created attachment 107257 [details] [review]
Bug 25855: (QA follow-up) Simplify payload

This patch makes the hook be passed the Koha::Checkout object instead
of a hand-crafted list of attributes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2020-07-23 14:22:44 UTC
I decided to keep the 'payload' parameter so if bug 25938 adds more information we don't change the API. I hope it fulfils your requirement, Jonathan.
Comment 28 Jonathan Druart 2020-07-30 15:47:01 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 29 Jonathan Druart 2020-07-30 16:28:29 UTC
kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/SIP/Message.t
t/db_dependent/SIP/Message.t .. 2/7     # Looks like you planned 29 tests but ran 5.

#   Failed test 'Checkin V2'
#   at t/db_dependent/SIP/Message.t line 74.
Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 2209.


The issue may not exist. Please fix.
Comment 30 Jonathan Druart 2020-07-30 16:30:40 UTC
Also

1. t/db_dependent/Reserves.t .. 2/63 Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 2209.


2. t/db_dependent/Circulation/issue.t .. 21/46 Use of uninitialized value $resfound in string eq at /kohadevbox/koha/C4/Circulation.pm line 2127.
Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 2209.

3. #   Failed test 'No tests run for subtest "checkin_lost"'
#   at t/db_dependent/SIP/Transaction.t line 360.
Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 2209.

4.  #   Failed test 'bug 22877'
    #   at t/db_dependent/Circulation.t line 2455.
    # Looks like you planned 8 tests but ran 5.
    # Looks like you failed 1 test of 5 run.
t/db_dependent/Circulation.t .. 31/49 
#   Failed test 'AddReturn | is_overdue'
#   at t/db_dependent/Circulation.t line 2669.
Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 2209.
Comment 31 Jonathan Druart 2020-07-31 07:49:34 UTC
Failures reported on bug 26108
Comment 32 Lucas Gass 2020-07-31 21:32:37 UTC
I will backport this but first I will wait until bZ26108 is pushed to master
Comment 33 Lucas Gass 2020-08-13 16:21:55 UTC
after consideringthis bug for 20.05.x I have decided not to backport this enhancement