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

Added to https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks

Maybe create a KPZ to ease testing

(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?

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

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.

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>

Created attachment 163330 [details] [review]
Bug 29392: Improve hook consistency.

It would appear we've adopted 'payload' for passing additional data into
action hooks.

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.

With bug 33036 having just been pushed, I think we need to move the before_biblio hooks here.

(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.

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.

(In reply to Martin Renvoize from comment #12)
> 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?
Our use case is almost like what is described in the test plan: store the ID of to-be-deleted records in the kept record, and store the ID of the kept record in the to-be-deleted records.

How would you use the 'save' hook in that case ?