Bug 22709

Summary: Add hooks to notify plugins of biblio and item changes
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Plugin architectureAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: new feature    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, josef.moravec, katrin.fischer, kyle, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29273
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36542
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This new feature allows plugin authors to implement `after_biblio_action` and `after_item_action` methods which may be used to take various actions upon biblio and item creations, modifications and deletions. **Warning**: Care should be taken when installing any plugins and only plugins you trust should be used.
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 23395, 34786, 26470, 29274    
Attachments: Bug 22709: Unit tests
Bug 22709: Add after biblio/item action hooks
Bug 22709: (follow-up) Mock enable_plugins config entry
Bug 22709: Unit tests
Bug 22709: Add after biblio/item action hooks
Bug 22709: (follow-up) Mock enable_plugins config entry
Bug 22709: (follow-up) Move new test file into a Plugins subdirectory
Bug 22709: Unit tests
Bug 22709: Add after biblio/item action hooks
Bug 22709: (follow-up) Mock enable_plugins config entry
Bug 22709: (follow-up) Move new test file into a Plugins subdirectory

Description Tomás Cohen Arazi 2019-04-15 16:17:25 UTC
We should have a way to make

*Biblio
*Item

methods trigger actions on plugins. Maybe if we have a message queue, then enqueue messages for registered plugins (i.e. to avoid synchronous calls).
Comment 1 Tomás Cohen Arazi 2019-04-15 16:18:55 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2019-06-24 14:57:11 UTC
Created attachment 90950 [details] [review]
Bug 22709: Unit tests

This patch introduces tests for new plugin hooks added to
C4::Biblio::{Add|Mod|Del}Biblio and C4::Items::{Add|Mod|Del}Item.

They are tested together as for creating an item you need to first create
a biblio and tests looks basically the same.

The testing strategy is
- Make sure the plugin is passed the right params
- Throw an exception at plugin-level, to be trapped by the C4::Biblio
and C4::Items lib, and converted into a warning. So we test for the
warning.
- Also, the fact that C4::Biblio and C4::Items warns, means the
exception was cought, and then Koha won't break on faulty plugins or
fatal errors from plugins.
Comment 3 Tomás Cohen Arazi 2019-06-24 14:57:14 UTC
Created attachment 90951 [details] [review]
Bug 22709: Add after biblio/item action hooks

This patch introduces after-action hooks for
- C4::Biblio::{Add|Mod|Del}Biblio
- C4::Items::{Add|Mod|Del}Biblio

After the action has taken place, a call to a helper method is done,
which loops through all plugins implementing the hooks, and calls the
plugin method. The related object is passed, along with an 'action'
string specifying the action that took place, and the object id (which
is specially important for the 'Del' case).

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Biblio_and_Items_plugin_hooks.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 4 Kyle M Hall 2019-07-01 18:40:58 UTC
Unit tests are failing.
Comment 5 Tomás Cohen Arazi 2019-07-02 13:07:45 UTC
Created attachment 91186 [details] [review]
Bug 22709: (follow-up) Mock enable_plugins config entry

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2019-07-02 13:08:37 UTC
(In reply to Kyle M Hall from comment #4)
> Unit tests are failing.

Thanks for testing! I've added a follow-up to fix the issue. I didn't notice I forgot to mock the enable_plugins config entry.
Comment 7 Kyle M Hall 2019-07-03 12:19:53 UTC
Created attachment 91223 [details] [review]
Bug 22709: Unit tests

This patch introduces tests for new plugin hooks added to
C4::Biblio::{Add|Mod|Del}Biblio and C4::Items::{Add|Mod|Del}Item.

They are tested together as for creating an item you need to first create
a biblio and tests looks basically the same.

The testing strategy is
- Make sure the plugin is passed the right params
- Throw an exception at plugin-level, to be trapped by the C4::Biblio
and C4::Items lib, and converted into a warning. So we test for the
warning.
- Also, the fact that C4::Biblio and C4::Items warns, means the
exception was cought, and then Koha won't break on faulty plugins or
fatal errors from plugins.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2019-07-03 12:20:04 UTC
Created attachment 91224 [details] [review]
Bug 22709: Add after biblio/item action hooks

This patch introduces after-action hooks for
- C4::Biblio::{Add|Mod|Del}Biblio
- C4::Items::{Add|Mod|Del}Biblio

After the action has taken place, a call to a helper method is done,
which loops through all plugins implementing the hooks, and calls the
plugin method. The related object is passed, along with an 'action'
string specifying the action that took place, and the object id (which
is specially important for the 'Del' case).

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Biblio_and_Items_plugin_hooks.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2019-07-03 12:20:06 UTC
Created attachment 91225 [details] [review]
Bug 22709: (follow-up) Mock enable_plugins config entry

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2019-07-03 12:20:10 UTC
Created attachment 91226 [details] [review]
Bug 22709: (follow-up) Move new test file into a Plugins subdirectory

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Tomás Cohen Arazi 2019-07-03 13:06:17 UTC
(In reply to Kyle M Hall from comment #10)
> Created attachment 91226 [details] [review] [review]
> Bug 22709: (follow-up) Move new test file into a Plugins subdirectory
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Good idea!
Comment 12 Josef Moravec 2019-07-03 21:19:24 UTC
Created attachment 91244 [details] [review]
Bug 22709: Unit tests

This patch introduces tests for new plugin hooks added to
C4::Biblio::{Add|Mod|Del}Biblio and C4::Items::{Add|Mod|Del}Item.

They are tested together as for creating an item you need to first create
a biblio and tests looks basically the same.

The testing strategy is
- Make sure the plugin is passed the right params
- Throw an exception at plugin-level, to be trapped by the C4::Biblio
and C4::Items lib, and converted into a warning. So we test for the
warning.
- Also, the fact that C4::Biblio and C4::Items warns, means the
exception was cought, and then Koha won't break on faulty plugins or
fatal errors from plugins.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2019-07-03 21:19:29 UTC
Created attachment 91245 [details] [review]
Bug 22709: Add after biblio/item action hooks

This patch introduces after-action hooks for
- C4::Biblio::{Add|Mod|Del}Biblio
- C4::Items::{Add|Mod|Del}Biblio

After the action has taken place, a call to a helper method is done,
which loops through all plugins implementing the hooks, and calls the
plugin method. The related object is passed, along with an 'action'
string specifying the action that took place, and the object id (which
is specially important for the 'Del' case).

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Biblio_and_Items_plugin_hooks.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2019-07-03 21:19:33 UTC
Created attachment 91246 [details] [review]
Bug 22709: (follow-up) Mock enable_plugins config entry

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2019-07-03 21:19:37 UTC
Created attachment 91247 [details] [review]
Bug 22709: (follow-up) Move new test file into a Plugins subdirectory

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Martin Renvoize 2019-07-04 12:57:58 UTC
Nice work!

Pushed to master for 19.11.00
Comment 17 Katrin Fischer 2019-07-05 17:31:46 UTC
Do we have documentation for this and other new hooks? Should this be added to the kitchen sink plugin?
Comment 18 Tomás Cohen Arazi 2019-07-05 17:32:42 UTC
(In reply to Katrin Fischer from comment #17)
> Do we have documentation for this and other new hooks? Should this be added
> to the kitchen sink plugin?

Good point. Will add them to KitchenSink ASAP.
Comment 19 Katrin Fischer 2019-07-05 18:28:25 UTC
Started a wiki page too... content to be added ;)
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Comment 20 Martin Renvoize 2019-07-09 10:30:56 UTC
Nice work!

Pushed to master for 19.11.00
Comment 21 Martin Renvoize 2019-07-15 11:43:42 UTC
Nice work!

Pushed to master for 19.11.00
Comment 22 Fridolin Somers 2019-07-23 16:01:29 UTC
Awesome \o/
Comment 23 David Cook 2019-07-24 05:12:36 UTC
Nice! A few years ago, I was thinking it would be great to add hooks like this, but never got around to doing it.
Comment 24 Fridolin Somers 2019-07-25 13:31:16 UTC
I could backport to stable but maybe this needs the plugins performance patch.
Comment 25 Tomás Cohen Arazi 2019-07-25 13:43:02 UTC
(In reply to Fridolin SOMERS from comment #24)
> I could backport to stable but maybe this needs the plugins performance
> patch.

Indeed. And we noticed a slight performance hit we need to address.
Comment 26 Kyle M Hall 2019-07-25 15:02:35 UTC
(In reply to Tomás Cohen Arazi from comment #25)
> (In reply to Fridolin SOMERS from comment #24)
> > I could backport to stable but maybe this needs the plugins performance
> > patch.
> 
> Indeed. And we noticed a slight performance hit we need to address.

Agreed, with the performance patch, the addition time is essentially negligible.