We need a hook to trigger plugin actions after renweals take place.
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
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
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>
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>
Please remember to document on the wiki: https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks Thx!
(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 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 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?
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.
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>
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>
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>
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>
Excellent work, Thanks Tomas Works well, fully tested and passes QA scripts. Passing QA
Please remember to update the wiki :)
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?
(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?
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 :)
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.
Additional question: what happens (and what is the expected behaviour) if the plugin fails?
(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
(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.
(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.
At this point I'd privilege robustness and maintainability over extra calls :)
Created attachment 107251 [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>
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>
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.
Pushed to master for 20.11, thanks to everybody involved!
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.
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.
Failures reported on bug 26108
I will backport this but first I will wait until bZ26108 is pushed to master
after consideringthis bug for 20.05.x I have decided not to backport this enhancement