https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25273 --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Julian Maurice from comment #12)
Hi Nick,
Thanks for the explanation, it is much more clear to me now. The change makes sense and the patch works as expected.
However I would like to see some changes in the patch before validating it:
1)
- ModZebra( $authid, 'specialUpdate', 'authorityserver', $record ); + ModZebra( $authid, 'specialUpdate', 'authorityserver', { record => $record, authtypecode => $authtypecode } );
This change is confusing. In ModZebra we now have a $record variable which is a hash that contain a 'record' key. Even with this simple patch I had to ask myself several times « what's this $record variable I'm looking at ? The hash or the MARC::Record ? ». Look at this line for instance:
+ $record = $record->{record};
At some point in the subroutine, $record was a hasref, now it's a MARC::Record. This is the kind of things that make code hard to read, and make it easier for bugs to appear.
And I think it is not needed to pass the authtypecode to ModZebra, since it can be obtained from the MARC::Record.
2)
- unless ($record) { + if ($record) { + $indexer->update_index_background( [$biblionumber], [$record] ); + } else { $record = GetMarcBiblio({ biblionumber => $biblionumber, embed_items => 1 }); + $indexer->update_index_background( [$biblionumber], [{ record => $record }] ); } - my $records = [$record]; - $indexer->update_index_background( [$biblionumber], [$record] );
I think it was easier to read before : unless there is a record, fetch it; in all cases call update_index_background Now it's : if there is a record, call update_index_background, otherwise fetch the record and call update_index_background. This change was not needed, so why ? :)
3)
- my $id = $record->id // $record->authid; + my $id = $record->id;
Again this change is not needed, but this time it causes a bug. Try this : misc/search_tools/rebuild_elasticsearch.pl -a -ai X (replace X by an existing authid)
1 - Yes, was trying to save a call to GuessAuthType, reverted 2 - This was a result of above, turning record into a hash if needed, undone 3 - It made sense at the time, but yes, it breaks, removed -- You are receiving this mail because: You are watching all bug changes.