Bug 29392 - Add plugin hooks before merging biblios or authorities
Summary: Add plugin hooks before merging biblios or authorities
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 8 votes (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on: 33036 34943
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-02 14:33 UTC by Julian Maurice
Modified: 2024-03-19 11:00 UTC (History)
5 users (show)

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


Attachments
Bug 29392: Add plugin hooks before merging biblios or authorities (5.12 KB, patch)
2021-11-02 14:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 29392: Add plugin hooks before merging biblios or authorities (5.10 KB, patch)
2024-03-18 13:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29392: Improve hook consistency. (2.02 KB, patch)
2024-03-18 13:47 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2021-11-02 14:33:05 UTC
For when we want to modify the final record of a merge operation
Comment 1 Julian Maurice 2021-11-02 14:33:53 UTC
Created attachment 127217 [details] [review]
Bug 29392: Add plugin hooks before merging biblios or authorities

Hooks are called 'before_biblio_action' and 'before_authority_action'
and they both take an 'action' parameter whose value is 'merge'.
Other parameters include the record id (biblionumber or authid), the
object itself (Koha::Biblio or Koha::Authority), the MARC::Record about
to be saved (so plugins can modify it), and the list of record ids that
will be deleted after the merge.

Test plan:
1. Create a plugin with these two methods. For instance:

    sub before_biblio_action {
        my ($self, $args) = @_;

        my $action = $args->{action};
        return unless $action eq 'merge';

        my $record = $args->{record};
        my $merged_biblio_ids = $args->{merged_biblio_ids};
        if ($record && $merged_biblio_ids) {
            my @fields = map {
                MARC::Field->new('035', ' ', ' ', 'z' => $_)
            } @$merged_biblio_ids;
            $record->insert_fields_ordered(@fields);
        }
    }

    sub before_authority_action {
        my ($self, $args) = @_;

        my $action = $args->{action};
        return unless $action eq 'merge';

        my $record = $args->{record};
        my $merged_authority_ids = $args->{merged_authority_ids};
        if ($record && $merged_authority_ids) {
            my @fields = map {
                MARC::Field->new('035', ' ', ' ', 'z' => $_)
            } @$merged_authority_ids;
            $record->insert_fields_ordered(@fields);
        }
    }

  This will copy the identifier of deleted records inside 035$z of the
  final record.
  (035$z is for "Canceled/invalid system control number")

2. Add the plugin path to your $KOHA_CONF
3. Run misc/devel/install_plugins.pl. It should say:
   Installed <YOUR_PLUGIN_NAME> version <YOUR_PLUGIN_VERSION>
4. Apply patch and restart starman
5. Create two or more biblios with the same title. Make sure they are
   indexed in your search engine.
6. Search for them. In the results check their corresponding boxes and
   click on "Edit -> Merge records"
7. Merge them and verify that your plugin code was called. If you used
   the example above, verify that it added a 035$z for each deleted
   record.
8. Do the same for authorities
Comment 2 Fridolin Somers 2022-06-01 20:45:07 UTC
Added to https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Comment 3 Fridolin Somers 2022-06-01 20:46:35 UTC
Maybe create a KPZ to ease testing
Comment 4 Katrin Fischer 2022-06-25 11:57:21 UTC
(In reply to Fridolin Somers from comment #3)
> Maybe create a KPZ to ease testing

Yes, if have never created a plugin, having one supplied to ease testing here would certainly help to speed up things! 

Maybe a variation to the kitchen sink plugin that could later be a merge request for it?
Comment 5 Tomás Cohen Arazi 2023-12-08 20:37:08 UTC
I just noticed this one, after submitting bug 34943.

The chosen name for the hook is not specific enough, given the fact it is only called on merging records.

Regarding testing it, I would say the best is to write tests for it :-D
Comment 6 Martin Renvoize 2024-01-31 16:33:37 UTC
Actually.. I think this is more in keeping with the existing 'after_biblio_action' found in C4::Biblio Tomas.. we should probably rename yours and pass an 'action' to be consistent.
Comment 7 Martin Renvoize 2024-03-18 13:47:40 UTC
Created attachment 163329 [details] [review]
Bug 29392: Add plugin hooks before merging biblios or authorities

Hooks are called 'before_biblio_action' and 'before_authority_action'
and they both take an 'action' parameter whose value is 'merge'.
Other parameters include the record id (biblionumber or authid), the
object itself (Koha::Biblio or Koha::Authority), the MARC::Record about
to be saved (so plugins can modify it), and the list of record ids that
will be deleted after the merge.

Test plan:
1. Create a plugin with these two methods. For instance:

    sub before_biblio_action {
        my ($self, $args) = @_;

        my $action = $args->{action};
        return unless $action eq 'merge';

        my $record = $args->{record};
        my $merged_biblio_ids = $args->{merged_biblio_ids};
        if ($record && $merged_biblio_ids) {
            my @fields = map {
                MARC::Field->new('035', ' ', ' ', 'z' => $_)
            } @$merged_biblio_ids;
            $record->insert_fields_ordered(@fields);
        }
    }

    sub before_authority_action {
        my ($self, $args) = @_;

        my $action = $args->{action};
        return unless $action eq 'merge';

        my $record = $args->{record};
        my $merged_authority_ids = $args->{merged_authority_ids};
        if ($record && $merged_authority_ids) {
            my @fields = map {
                MARC::Field->new('035', ' ', ' ', 'z' => $_)
            } @$merged_authority_ids;
            $record->insert_fields_ordered(@fields);
        }
    }

  This will copy the identifier of deleted records inside 035$z of the
  final record.
  (035$z is for "Canceled/invalid system control number")

2. Add the plugin path to your $KOHA_CONF
3. Run misc/devel/install_plugins.pl. It should say:
   Installed <YOUR_PLUGIN_NAME> version <YOUR_PLUGIN_VERSION>
4. Apply patch and restart starman
5. Create two or more biblios with the same title. Make sure they are
   indexed in your search engine.
6. Search for them. In the results check their corresponding boxes and
   click on "Edit -> Merge records"
7. Merge them and verify that your plugin code was called. If you used
   the example above, verify that it added a 035$z for each deleted
   record.
8. Do the same for authorities

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2024-03-18 13:47:43 UTC
Created attachment 163330 [details] [review]
Bug 29392: Improve hook consistency.

It would appear we've adopted 'payload' for passing additional data into
action hooks.
Comment 9 Martin Renvoize 2024-03-18 13:49:57 UTC
Rebased and added a consistency follow-up... we aught to have unit tests, but as we're at the controller level that's not quite as trivial as our other hook unit tests.

I'm opting to PQA for now but hope in the future the logic of these merge op's will make it's way into the Model level.
Comment 10 Martin Renvoize 2024-03-18 14:34:27 UTC
With bug 33036 having just been pushed, I think we need to move the before_biblio hooks here.
Comment 11 Martin Renvoize 2024-03-18 14:46:25 UTC
(In reply to Martin Renvoize from comment #10)
> With bug 33036 having just been pushed, I think we need to move the
> before_biblio hooks here.

Hmm.. kinda annoying.. we're allowing for both metadata and biblio changes here in the hook.. that will be more difficult if we move within the merge_with routing of Koha::Biblio as we don't handle metadata in that merge routine at all.
Comment 12 Martin Renvoize 2024-03-19 11:00:06 UTC
Julian, do you have these hooks in use already? I'm wondering about the use cases and whether a mix of hooks for 'save' and 'merge' might make sense?

Tomas's bug 34943 allows for hooking on the biblio metadata change action, whilst a hook added to the merge_with method introduced by bug 33036 would allow for hooking on the biblio and item level data combination during merge.