Bug 27066 - Implement hooks for patrons updates
Summary: Implement hooks for patrons updates
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Maryse Simard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-20 15:16 UTC by Eric Bégin
Modified: 2021-03-10 16:47 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
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 (2.16 KB, patch)
2021-01-29 06:32 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 27066: Unit tests for after_patron_action hook (3.29 KB, patch)
2021-01-29 06:32 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 27066: Add plugin hook after_patron_action (2.22 KB, patch)
2021-03-05 15:44 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27066: Unit tests for after_patron_action hook (3.35 KB, patch)
2021-03-05 15:44 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.