Bug 27066

Summary: Implement hooks for patrons updates
Product: Koha Reporter: Eric Bégin <eric.begin>
Component: Plugin architectureAssignee: Maryse Simard <maryse.simard>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: black23, dcook, fridolin.somers, jonathan.druart, martin.renvoize, maryse.simard, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Adds a new plugin hook `after_patron_action` which is called after creation, modification or deletion of a patron.
Version(s) released in:
Attachments: Bug 27066: Add plugin hook after_patron_action
Bug 27066: Unit tests for after_patron_action hook
Bug 27066: Add plugin hook after_patron_action
Bug 27066: Unit tests for after_patron_action hook

Description Eric Bégin 2020-11-20 15:16:08 UTC
Some of our clients use external systems to populate Koha's patron information.

They can use Koha's API to update patron info, however, we don't have any way (that I know of) to notify an external system when a patron info is changed in Koha.

In order to allow bidirectional communications, we are planning to add hooks.

This will allow creating plugins to send messages to external systems in order to fulfil the following requirements :
- Update patron information stored in the borrowers table (name, address, email, etc.)
- Notify an external system when a restriction is placed on a user (fines, manual restrictions, overdues, etc.)
Comment 1 Fridolin Somers 2020-11-25 14:00:00 UTC
Since we have hooks after_item_action and after_biblio_action,
I'd say add after_patron_action hook.


See https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Comment 2 Maryse Simard 2021-01-29 06:32:32 UTC
Created attachment 116007 [details] [review]
Bug 27066: Add plugin hook after_patron_action

The hook is called after patron manipulations. The value of the
'action' parameter can be 'create', 'modify' or 'delete'.

To test:
1. Write a simple plugin that implements at least the new
   after_patron_action hook.
2. Install the plugin and enable it.
3. Create, modify or delete a patron and verify that the plugin method
   for the hook has been called with the right parameters.
Comment 3 Maryse Simard 2021-01-29 06:32:35 UTC
Created attachment 116008 [details] [review]
Bug 27066: Unit tests for after_patron_action hook

Adds unit tests for the new hook.

To test:
    prove -v t/db_dependent/Koha/Plugins/Patron.t
Comment 4 Fridolin Somers 2021-03-05 15:44:03 UTC
Created attachment 117867 [details] [review]
Bug 27066: Add plugin hook after_patron_action

The hook is called after patron manipulations. The value of the
'action' parameter can be 'create', 'modify' or 'delete'.

To test:
1. Write a simple plugin that implements at least the new
   after_patron_action hook.
2. Install the plugin and enable it.
3. Create, modify or delete a patron and verify that the plugin method
   for the hook has been called with the right parameters.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 5 Fridolin Somers 2021-03-05 15:44:17 UTC
Created attachment 117868 [details] [review]
Bug 27066: Unit tests for after_patron_action hook

Adds unit tests for the new hook.

To test:
    prove -v t/db_dependent/Koha/Plugins/Patron.t

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 6 Fridolin Somers 2021-03-05 15:55:14 UTC
Works has expected, looks a lot like "after_item_action"
Comment 7 Katrin Fischer 2021-03-06 12:56:40 UTC
Please document on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks

Looking at the code and comparing with the similar after_item_action hooks I wonder about the placement in the code.

The implementation on bug 22709 adds the hooks before the logs are written, this adds them after.

I could imagine that plugins could actually change the data, so would result in a different logging result. So I think "before" would be better. What do you think?
Comment 8 Fridolin Somers 2021-03-08 08:08:59 UTC
(In reply to Katrin Fischer from comment #7)
> Please document on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> 
> Looking at the code and comparing with the similar after_item_action hooks I
> wonder about the placement in the code.
> 
> The implementation on bug 22709 adds the hooks before the logs are written,
> this adds them after.
> 
> I could imagine that plugins could actually change the data, so would result
> in a different logging result. So I think "before" would be better. What do
> you think?

I agree we must have same behavior.

But I see _after_item_action_hooks is at the very end of store() :
https://git.koha-community.org/Koha-community/Koha/src/commit/4f96b6767a1be022a4e38a856491b2b1eae782fc/Koha/Item.pm#L202

After action logs and search engine indexing.

Note that a main difference is that patron store() is in a DB transaction.
Comment 9 Tomás Cohen Arazi 2021-03-08 12:16:13 UTC
(In reply to Eric Bégin from comment #0)
> Some of our clients use external systems to populate Koha's patron
> information.
> 
> They can use Koha's API to update patron info, however, we don't have any
> way (that I know of) to notify an external system when a patron info is
> changed in Koha.
> 
> In order to allow bidirectional communications, we are planning to add hooks.
> 
> This will allow creating plugins to send messages to external systems in
> order to fulfil the following requirements :
> - Update patron information stored in the borrowers table (name, address,
> email, etc.)
> - Notify an external system when a restriction is placed on a user (fines,
> manual restrictions, overdues, etc.)

Hi, maybe it is an interesting opportunity to look at adding hooks for more fine grained things. Because in that hook, it seems you will end up checking all those things (DB access). And if the hooks were added to other places like when a debarment is generated, then you would have the debarment object in context, and no DB access would be required.
Comment 10 Martin Renvoize 2021-03-10 08:17:07 UTC
(In reply to Katrin Fischer from comment #7)
> Please document on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> 
> Looking at the code and comparing with the similar after_item_action hooks I
> wonder about the placement in the code.
> 
> The implementation on bug 22709 adds the hooks before the logs are written,
> this adds them after.
> 
> I could imagine that plugins could actually change the data, so would result
> in a different logging result. So I think "before" would be better. What do
> you think?

Hmmm, 

tl;dr I don't think 'before' or 'after' hooks should directly affect the data of the action.

We probably want 'before' and 'after' hooks.. and maybe even an 'around'.

Generally, a 'before' hook is for triggering actions prior to store (but not affecting the data for the store itself).. 'after' is for triggering actions after the store (like triggering zebraqueue.. again, it shouldn't affect the data of the store itself) and an around wraps the store and returns the updated data for the store to then act upon.

At least, that's my vague understanding of 'before', 'after', 'around' in other frameworks?
Comment 11 Martin Renvoize 2021-03-10 08:24:02 UTC
(In reply to Eric Bégin from comment #0)
> - Update patron information stored in the borrowers table (name, address,
> email, etc.)

I would say that this plugin works perfectly for the case where you want to trigger the notification of other systems.. but I do not think, as it stands, it works for the above statement (and I'm not sure it should.. we should implement an 'around' for that in my opinion.)
Comment 12 Martin Renvoize 2021-03-10 08:24:53 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> (In reply to Eric Bégin from comment #0)
>
> Hi, maybe it is an interesting opportunity to look at adding hooks for more
> fine grained things. Because in that hook, it seems you will end up checking
> all those things (DB access). And if the hooks were added to other places
> like when a debarment is generated, then you would have the debarment object
> in context, and no DB access would be required.

Also agree with this.. it would be nice to add more specific hooks.. but I don't see that as a blocker for this one.
Comment 13 Katrin Fischer 2021-03-10 12:04:55 UTC
(In reply to Katrin Fischer from comment #7)
> Please document on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> 
> Looking at the code and comparing with the similar after_item_action hooks I
> wonder about the placement in the code.
> 
> The implementation on bug 22709 adds the hooks before the logs are written,
> this adds them after.
> 
> I could imagine that plugins could actually change the data, so would result
> in a different logging result. So I think "before" would be better. What do
> you think?

To come back to the original question - do we care if the plugin calls are before of after logaction and should we be consistent? :)
Comment 14 Martin Renvoize 2021-03-10 16:47:22 UTC
(In reply to Katrin Fischer from comment #13)
> (In reply to Katrin Fischer from comment #7)
> > Please document on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> > 
> > Looking at the code and comparing with the similar after_item_action hooks I
> > wonder about the placement in the code.
> > 
> > The implementation on bug 22709 adds the hooks before the logs are written,
> > this adds them after.
> > 
> > I could imagine that plugins could actually change the data, so would result
> > in a different logging result. So I think "before" would be better. What do
> > you think?
> 
> To come back to the original question - do we care if the plugin calls are
> before of after logaction and should we be consistent? :)

The questions are related ;)

If 'before' and 'after' actions don't affect the data of the object.. then they wont affect the actionlogs call.. so it shouldn't matter if they're before or after.  Personally.. I prefer to have them as late as possible, so after, but that's just a style thing.
Comment 15 Nick Clemens 2022-09-15 15:07:46 UTC
(In reply to Martin Renvoize from comment #14)
> If 'before' and 'after' actions don't affect the data of the object.. then
> they wont affect the actionlogs call.. so it shouldn't matter if they're
> before or after.  Personally.. I prefer to have them as late as possible, so
> after, but that's just a style thing.

I agree - the log should come before the after action hook - as the log is that state when Koha is done with its work

The after action hook shouldn't change the data, but it seems more stable to put our log before passing the object, in case something outside does change it - we can at least record what Koha thinks the object should be
Comment 16 Fridolin Somers 2024-03-20 13:00:52 UTC
How do we continue here ?
Comment 17 David Cook 2024-03-20 22:58:18 UTC
(In reply to Martin Renvoize from comment #10)
> tl;dr I don't think 'before' or 'after' hooks should directly affect the
> data of the action.
> 
> We probably want 'before' and 'after' hooks.. and maybe even an 'around'.
> 
> Generally, a 'before' hook is for triggering actions prior to store (but not
> affecting the data for the store itself).. 'after' is for triggering actions
> after the store (like triggering zebraqueue.. again, it shouldn't affect the
> data of the store itself) and an around wraps the store and returns the
> updated data for the store to then act upon.
> 
> At least, that's my vague understanding of 'before', 'after', 'around' in
> other frameworks?

That's an interesting point. I hadn't thought of that...