|
Description
Tomás Cohen Arazi (tcohen)
2023-09-28 11:05:56 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. That sounds like an excellent idea! Created attachment 159680 [details] [review] Bug 34943: Unit tests Created attachment 159681 [details] [review] Bug 34943: Implement before_record_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 Created attachment 159682 [details] [review] Bug 34943: Unit tests Created attachment 159683 [details] [review] Bug 34943: Implement before_record_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 Created attachment 159684 [details] [review] Bug 34943: Unit tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 159685 [details] [review] Bug 34943: Implement before_record_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> David, we overlapped. I'm renaming the hook for clarity. Let me re-submit and you test again! Created attachment 159686 [details] [review] Bug 34943: Unit tests 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 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. Created attachment 159690 [details] [review] Bug 34943: Unit tests Signed-off-by: David Nind <david@davidnind.com> 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> This feels like the perfect place for a plugin hook to accomplish bug 20065 :) Created attachment 161695 [details] [review] Bug 34943: Unit tests Signed-off-by: David Nind <david@davidnind.com> 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> Works as described, fully tested and no regressions :) 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? (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. (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. 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> Martin, can you please check if my follow-up is what you were envisioning? Thanks! 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? (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. Good catch, thanks for highlighting that Tomas. Oh the fun of trying to stay consistent when we're already rather inconsistent aye. 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> 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> 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> 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 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> 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> 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> Please update the wiki page! https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks Pushed for 24.05! Well done everyone, thank you! Backported (big word) to 22.11.x for 22.11.16 (In reply to Katrin Fischer from comment #34) > Please update the wiki page! > https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks *ping* (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 (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 :) Not backported to 23.11.x |