Bug 34943 - Add a pre-save plugin hook for biblios
Summary: Add a pre-save plugin hook for biblios
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 29392 31791
  Show dependency treegraph
 
Reported: 2023-09-28 11:05 UTC by Tomás Cohen Arazi
Modified: 2024-04-30 10:49 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 34943: Unit tests (3.31 KB, patch)
2023-12-08 20:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Implement before_record_store() plugin hook (1.27 KB, patch)
2023-12-08 20:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.33 KB, patch)
2023-12-08 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Implement before_record_store() plugin hook (1.28 KB, patch)
2023-12-08 20:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.35 KB, patch)
2023-12-08 20:44 UTC, David Nind
Details | Diff | Splinter Review
Bug 34943: Implement before_record_store() plugin hook (1.31 KB, patch)
2023-12-08 20:44 UTC, David Nind
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.33 KB, patch)
2023-12-08 20:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Implement `before_biblio_metadata_store` plugin hook (1.29 KB, patch)
2023-12-08 20:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.37 KB, patch)
2023-12-08 21:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 34943: Implement `before_biblio_metadata_store` plugin hook (1.33 KB, patch)
2023-12-08 21:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.37 KB, patch)
2024-01-31 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34943: Implement `before_biblio_metadata_store` plugin hook (1.40 KB, patch)
2024-01-31 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param (2.67 KB, patch)
2024-03-11 17:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.44 KB, patch)
2024-03-18 13:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34943: Implement `before_biblio_metadata_store` plugin hook (1.40 KB, patch)
2024-03-18 13:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param (2.73 KB, patch)
2024-03-18 13:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34943: Unit tests (3.43 KB, patch)
2024-03-22 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: Implement `before_biblio_metadata_store` plugin hook (1.40 KB, patch)
2024-03-22 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param (2.73 KB, patch)
2024-03-22 15:16 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 2023-09-28 11:05:56 UTC
It would be handy if we could have a hook that is passed the MARC::Record to be saved (on ModBiblioMarc) right before saving it. This way we could add machine generated fields using plugins.
Comment 1 Tomás Cohen Arazi 2023-09-28 11:07:28 UTC
I forgot to mention, right now we have the 'after_biblio_action' hook, but using is for the aforementioned purpose would generate a loop.
Comment 2 Kyle M Hall 2023-09-28 14:36:36 UTC
That sounds like an excellent idea!
Comment 3 Tomás Cohen Arazi 2023-12-08 20:30:25 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2023-12-08 20:30:28 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2023-12-08 20:43:11 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2023-12-08 20:43:13 UTC Comment hidden (obsolete)
Comment 7 David Nind 2023-12-08 20:44:28 UTC Comment hidden (obsolete)
Comment 8 David Nind 2023-12-08 20:44:30 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2023-12-08 20:45:48 UTC
David, we overlapped. I'm renaming the hook for clarity. Let me re-submit and you test again!
Comment 10 Tomás Cohen Arazi 2023-12-08 20:47:21 UTC
Created attachment 159686 [details] [review]
Bug 34943: Unit tests
Comment 11 Tomás Cohen Arazi 2023-12-08 20:47:24 UTC
Created attachment 159687 [details] [review]
Bug 34943: Implement `before_biblio_metadata_store` plugin hook

This patch implements a hook allowing record modification right before
they are written on the DB. The idea is that a plugin could be used to
add machine-generated fields/subfields.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
=> FAIL: Tests fail! The hook is not implemented so the desired results
don't appear (added fields/subfields).
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works!
5. Run:
  k$ qa -c 2
=> SUCCESS: All green!
6. Sign off :-D

Sponsored-by: Theke Solutions
Comment 12 Tomás Cohen Arazi 2023-12-08 20:48:12 UTC
I decided to rename for hook for better scoping it (authorities could have a hook too) taking the idea from bug 29392, which feels valuable as well.
Comment 13 David Nind 2023-12-08 21:26:30 UTC
Created attachment 159690 [details] [review]
Bug 34943: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2023-12-08 21:26:33 UTC
Created attachment 159691 [details] [review]
Bug 34943: Implement `before_biblio_metadata_store` plugin hook

This patch implements a hook allowing record modification right before
they are written on the DB. The idea is that a plugin could be used to
add machine-generated fields/subfields.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
=> FAIL: Tests fail! The hook is not implemented so the desired results
don't appear (added fields/subfields).
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works!
5. Run:
  k$ qa -c 2
=> SUCCESS: All green!
6. Sign off :-D

Sponsored-by: Theke Solutions
Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Martin Renvoize 2024-01-31 16:08:21 UTC
This feels like the perfect place for a plugin hook to accomplish bug 20065  :)
Comment 16 Martin Renvoize 2024-01-31 16:09:55 UTC
Created attachment 161695 [details] [review]
Bug 34943: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Martin Renvoize 2024-01-31 16:09:57 UTC
Created attachment 161696 [details] [review]
Bug 34943: Implement `before_biblio_metadata_store` plugin hook

This patch implements a hook allowing record modification right before
they are written on the DB. The idea is that a plugin could be used to
add machine-generated fields/subfields.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
=> FAIL: Tests fail! The hook is not implemented so the desired results
don't appear (added fields/subfields).
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works!
5. Run:
  k$ qa -c 2
=> SUCCESS: All green!
6. Sign off :-D

Sponsored-by: Theke Solutions
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2024-01-31 16:11:22 UTC
Works as described, fully tested and no regressions :)
Comment 19 Martin Renvoize 2024-01-31 16:34:33 UTC
Actually.. reading a little deeper I have questions.

Should we not be consistent with 'after_biblio_action' and call the hook 'before_biblio_action' and pass through an 'action' parameter?
Comment 20 Tomás Cohen Arazi 2024-01-31 18:39:38 UTC
(In reply to Martin Renvoize from comment #19)
> Actually.. reading a little deeper I have questions.
> 
> Should we not be consistent with 'after_biblio_action' and call the hook
> 'before_biblio_action' and pass through an 'action' parameter?

You might be right. I had my own doubts about it :-D

I will expose my thinking so we can discuss on top of it: let's think we move things to Koha::*

I would imagine we implement:

Koha::Biblio->update_from_marc
Koha::Biblio->new_from_marc

and let's stop at `update_from_marc` (at some point our MARC editor(s) could be replaced by a UI that PUTs application/marc-in-json, but right now we do HTML form -> MARC::Record -> ModBiblio which is conceptually the same).

In plain english, the sequence of actions would be:

1. You got passed a MARC::Record, didn't do anything yet.
2. Some sanitization takes place (as in Koha::Biblio::Metadata->store or ModBiblio.
3. Some data is extracted and stored on biblio and biblioitems.
4. The sanitized metadata is stored.

This hook is intended to be called between (3) and (4). I would accept any naming change, but need to be sure we are looking at the same thing. To me, `before_biblio_action` might fit before (3) but could as well fit before (2).

Hope it clarifies the thinking behind of it.
Comment 21 Tomás Cohen Arazi 2024-03-11 14:08:33 UTC
(In reply to Martin Renvoize from comment #19)
> Actually.. reading a little deeper I have questions.
> 
> Should we not be consistent with 'after_biblio_action' and call the hook
> 'before_biblio_action' and pass through an 'action' parameter?

I think I misread this :-D Sending a follow-up patch.
Comment 22 Tomás Cohen Arazi 2024-03-11 17:36:51 UTC
Created attachment 163048 [details] [review]
Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param

This patch harmonizes the hook name and parameters with the rest of the
codebase.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All looks green, and tests still pass (i.e. they were
correctly adjusted to the new schema).
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2024-03-11 17:50:34 UTC
Martin, can you please check if my follow-up is what you were envisioning?

Thanks!
Comment 24 Martin Renvoize 2024-03-18 12:48:29 UTC
Look good to me.. I'm still comparing to the _after_biblio_action_hooks sub though and the signature looks a little different with a top level hash vs a hash under a 'payload' key.. thoughts on that?
Comment 25 Tomás Cohen Arazi 2024-03-18 12:59:05 UTC
(In reply to Martin Renvoize from comment #24)
> Look good to me.. I'm still comparing to the _after_biblio_action_hooks sub
> though and the signature looks a little different with a top level hash vs a
> hash under a 'payload' key.. thoughts on that?

Yeah, I looked at C4::Reserves and C4::Circulation hooks and found those were all following this pattern. I'm not attached to any pattern, but the one I picked seems to be used more.
Comment 26 Martin Renvoize 2024-03-18 13:23:22 UTC
Good catch, thanks for highlighting that Tomas.

Oh the fun of trying to stay consistent when we're already rather inconsistent aye.
Comment 27 Martin Renvoize 2024-03-18 13:26:12 UTC
Created attachment 163325 [details] [review]
Bug 34943: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2024-03-18 13:26:14 UTC
Created attachment 163326 [details] [review]
Bug 34943: Implement `before_biblio_metadata_store` plugin hook

This patch implements a hook allowing record modification right before
they are written on the DB. The idea is that a plugin could be used to
add machine-generated fields/subfields.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
=> FAIL: Tests fail! The hook is not implemented so the desired results
don't appear (added fields/subfields).
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works!
5. Run:
  k$ qa -c 2
=> SUCCESS: All green!
6. Sign off :-D

Sponsored-by: Theke Solutions
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2024-03-18 13:26:17 UTC
Created attachment 163327 [details] [review]
Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param

This patch harmonizes the hook name and parameters with the rest of the
codebase.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All looks green, and tests still pass (i.e. they were
correctly adjusted to the new schema).
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Katrin Fischer 2024-03-22 14:33:07 UTC
Something going wrong here, can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 34943: Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
Applying: Bug 34943: Implement `before_biblio_metadata_store` plugin hook
Applying: Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param
error: sha1 information is lacking or useless (C4/Biblio.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-34943-QA-follow-up-Use-beforebiblioaction-and--c2vv4mt2.patch


Please also add to the release notes field and the wiki page for documentation:
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Comment 31 Tomás Cohen Arazi 2024-03-22 15:16:14 UTC
Created attachment 163711 [details] [review]
Bug 34943: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Tomás Cohen Arazi 2024-03-22 15:16:17 UTC
Created attachment 163712 [details] [review]
Bug 34943: Implement `before_biblio_metadata_store` plugin hook

This patch implements a hook allowing record modification right before
they are written on the DB. The idea is that a plugin could be used to
add machine-generated fields/subfields.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t
=> FAIL: Tests fail! The hook is not implemented so the desired results
don't appear (added fields/subfields).
3. Apply this patch
4. Repeat 2
=> SUCCESS: It works!
5. Run:
  k$ qa -c 2
=> SUCCESS: All green!
6. Sign off :-D

Sponsored-by: Theke Solutions
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Tomás Cohen Arazi 2024-03-22 15:16:20 UTC
Created attachment 163713 [details] [review]
Bug 34943: (QA follow-up) Use `before_biblio_action` and an `action` param

This patch harmonizes the hook name and parameters with the rest of the
codebase.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All looks green, and tests still pass (i.e. they were
correctly adjusted to the new schema).
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Katrin Fischer 2024-03-25 08:39:12 UTC
Please update the wiki page! https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Comment 35 Katrin Fischer 2024-03-25 08:54:18 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 36 Frédéric Demians 2024-03-26 14:47:40 UTC
Backported (big word) to 22.11.x for 22.11.16
Comment 37 Katrin Fischer 2024-04-26 20:45:57 UTC
(In reply to Katrin Fischer from comment #34)
> Please update the wiki page!
> https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks

*ping*
Comment 38 Martin Renvoize 2024-04-30 10:48:06 UTC
(In reply to Katrin Fischer from comment #37)
> (In reply to Katrin Fischer from comment #34)
> > Please update the wiki page!
> > https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> 
> *ping*

Done
Comment 39 Katrin Fischer 2024-04-30 10:49:00 UTC
(In reply to Martin Renvoize from comment #38)
> (In reply to Katrin Fischer from comment #37)
> > (In reply to Katrin Fischer from comment #34)
> > > Please update the wiki page!
> > > https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> > 
> > *ping*
> 
> Done

Thx! Also removed additional_work_needed now :)