Attachment 127599 Details for Bug 25539 – Bug 25539: Remove AddBiblio option "defer_marc_save"

[patch] Bug 25539: Remove AddBiblio option "defer_marc_save"

-Bug-25539-Remove-AddBiblio-option-defermarcsave.patch (text/plain), 8.99 KB, created by David Gustafsson on 2021-11-12 22:45:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Gustafsson

Created: 2021-11-12 22:45:04 UTC

Size: 8.99 KB

patch

obsolete

>From f214a9d9d69e42e3c08c54e0fba63c67966f0861 Mon Sep 17 00:00:00 2001
>From: David Gustafsson <glasklas@gmail.com>
>Date: Fri, 12 Nov 2021 23:16:55 +0100
>Subject: [PATCH]  Bug 25539: Remove AddBiblio option "defer_marc_save"
>
>Items are no longer embedded in the MARCXML and because of this the MARC data
>does not need to be saved once more after changing record items data.
>The "defer_marc_save" is no longer needed since bulkmarcimport.pl was the only
>place this option was utilized in order to resave MARC data after possibly changing
>items data. There is also a bug bulkmarcimport.pl where the record data is re-saved
>without stripping items if duplicate items are found and the dedup barcodes
>option is enabled that is resolved by this change.
>---
> C4/Biblio.pm                           | 17 +---------
> misc/migration_tools/bulkmarcimport.pl | 46 ++++++++++----------------
> 2 files changed, 19 insertions(+), 44 deletions(-)
>
>diff --git a/C4/Biblio.pm b/C4/Biblio.pm
>index f057c80207..bc4fc160f0 100644
>--- a/C4/Biblio.pm
>+++ b/C4/Biblio.pm
>@@ -187,31 +187,16 @@ The first argument is a C<MARC::Record> object containing the
> bib to add, while the second argument is the desired MARC
> framework code.
> 
>-This function also accepts a third, optional argument: a hashref
>-to additional options.  The only defined option is C<defer_marc_save>,
>-which if present and mapped to a true value, causes C<AddBiblio>
>-to omit the call to save the MARC in C<biblio_metadata.metadata>
>-This option is provided B<only>
>-for the use of scripts such as C<bulkmarcimport.pl> that may need
>-to do some manipulation of the MARC record for item parsing before
>-saving it and which cannot afford the performance hit of saving
>-the MARC record twice.  Consequently, do not use that option
>-unless you can guarantee that C<ModBiblioMarc> will be called.
>-
> =cut
> 
> sub AddBiblio {
>     my $record          = shift;
>     my $frameworkcode   = shift;
>     my $options         = @_ ? shift : undef;
>-    my $defer_marc_save = 0;
>     if (!$record) {
>         carp('AddBiblio called with undefined record');
>         return;
>     }
>-    if ( defined $options and exists $options->{'defer_marc_save'} and $options->{'defer_marc_save'} ) {
>-        $defer_marc_save = 1;
>-    }
> 
>     my $schema = Koha::Database->schema;
>     my ( $biblionumber, $biblioitemnumber );
>@@ -292,7 +277,7 @@ sub AddBiblio {
>             }
> 
>             # now add the record
>-            ModBiblioMarc( $record, $biblionumber ) unless $defer_marc_save;
>+            ModBiblioMarc( $record, $biblionumber );
> 
>             # update OAI-PMH sets
>             if(C4::Context->preference("OAI-PMH:AutoUpdateSets")) {
>diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl
>index 3409e00608..43467a756c 100755
>--- a/misc/migration_tools/bulkmarcimport.pl
>+++ b/misc/migration_tools/bulkmarcimport.pl
>@@ -502,7 +502,7 @@ RECORD: foreach my $record (@{$marc_records}) {
>             }
>         }
>         else {
>-            my ($biblioitemnumber, $itemnumbers_ref, $errors_ref, $record_id);
>+            my ($biblioitemnumber, $itemnumbers, $errors, $record_id);
>             # check for duplicate, based on ISBN (skip it if we already have found a duplicate with match parameter
>             if (!$matched_record_id && $isbn_check && $isbn) {
>                 $sth_isbn->execute($isbn);
>@@ -551,7 +551,7 @@ RECORD: foreach my $record (@{$marc_records}) {
>                 }
>             }
>             elsif ($insert) {
>-                eval { ($record_id, $biblioitemnumber) = AddBiblio($record, $framework, { defer_marc_save => 1 }) };
>+                eval { ($record_id, $biblioitemnumber) = AddBiblio($record, $framework) };
>                 if ($@) {
>                     warn "ERROR: Insert biblio $originalid failed: $@\n";
>                     printlog( { id => $originalid, op => "insert", status => "ERROR" } ) if ($logfile);
>@@ -570,16 +570,9 @@ RECORD: foreach my $record (@{$marc_records}) {
>             if ($record_id) {
>                 $yamlhash->{$originalid} = $record_id if $yamlfile;
>                 # TODO: Add option for skipping items?
>-                eval { ($itemnumbers_ref, $errors_ref) = AddItemBatchFromMarc($record, $record_id, $biblioitemnumber, $framework); };
>-                $record_has_added_items = @{$itemnumbers_ref};
>+                eval { ($itemnumbers, $errors) = AddItemBatchFromMarc($record, $record_id, $biblioitemnumber, $framework); };
>                 my $error_adding = $@;
>-                # Work on a clone so that if there are real errors, we can maybe
>-                # fix them up later.
>-                my $clone_record = $record->clone();
>-                C4::Biblio::_strip_item_fields($clone_record, $framework);
>-                # This sets the marc fields if there was an error, and also calls
>-                # defer_marc_save.
>-                ModBiblioMarc($clone_record, $record_id, $modify_biblio_marc_options);
>+                $record_has_added_items = @{$itemnumbers};
>                 if ($error_adding) {
>                     warn "ERROR: Adding items to bib $record_id failed: $error_adding";
>                     printlog({ id => $record_id, op => "insert items", status => "ERROR"}) if ($logfile);
>@@ -590,20 +583,20 @@ RECORD: foreach my $record (@{$marc_records}) {
>                 else {
>                     printlog({ id => $record_id, op => "insert items", status => "ok" }) if ($logfile);
>                 }
>-                if ($dedup_barcode && grep { exists $_->{error_code} && $_->{error_code} eq 'duplicate_barcode' } @$errors_ref) {
>+                if ($dedup_barcode && grep { exists $_->{error_code} && $_->{error_code} eq 'duplicate_barcode' } @{$errors}) {
>                     # Find the record called 'barcode'
>                     my ($tag, $sub) = C4::Biblio::GetMarcFromKohaField('items.barcode');
>                     # Now remove any items that didn't have a duplicate_barcode error,
>                     # erase the barcodes on items that did, and re-add those items.
>                     my %dupes;
>                     # FIXME: This could cause array out of bounds because shifting down rest of items on array item delete?
>-                    foreach my $i (0 .. $#{$errors_ref}) {
>-                        my $ref = $errors_ref->[$i];
>-                        if ($ref && ($ref->{error_code} eq 'duplicate_barcode')) {
>-                            $dupes{$ref->{item_sequence}} = 1;
>+                    foreach my $i (0 .. $#{$errors}) {
>+                        my $error = $errors->[$i];
>+                        if ($error && ($error->{error_code} eq 'duplicate_barcode')) {
>+                            $dupes{$error->{item_sequence}} = 1;
>                             # Delete the error message because we're going to
>                             # retry this one.
>-                            delete $errors_ref->[$i];
>+                            delete $errors->[$i];
>                         }
>                     }
>                     my $seq = 0;
>@@ -621,26 +614,23 @@ RECORD: foreach my $record (@{$marc_records}) {
>                     }
>                     # Now re-add the record as before, adding errors to the prev list
>                     my $more_errors;
>-                    eval { ($itemnumbers_ref, $more_errors) = AddItemBatchFromMarc($record, $record_id, $biblioitemnumber, ''); };
>-                    $record_has_added_items ||= @{$itemnumbers_ref};
>-                    if ($@) {
>-                        warn "ERROR: Adding items to bib $record_id failed: $@\n";
>+                    eval { ($itemnumbers, $more_errors) = AddItemBatchFromMarc($record, $record_id, $biblioitemnumber, ''); };
>+                    $error_adding = $@;
>+                    $record_has_added_items ||= @{$itemnumbers};
>+                    if ($error_adding) {
>+                        warn "ERROR: Adding items to bib $record_id failed: $error_adding\n";
>                         printlog({ id => $record_id, op => "insert items", status => "ERROR" }) if ($logfile);
>                         # if we failed because of an exception, assume that
>                         # the MARC columns in biblioitems were not set.
>-
>-                        # @FIXME: Why do we save here without stripping items? Besides,
>-                        # save with stripped items has already been performed
>-                        ModBiblioMarc($record, $record_id, $modify_biblio_marc_options);
>                         next RECORD;
>                     }
>                     else {
>                         printlog({ id => $record_id, op => "insert", status => "ok" }) if ($logfile);
>                     }
>-                    push @$errors_ref, @{$more_errors};
>+                    push @{$errors}, @{$more_errors};
>                 }
>-                if (@{$errors_ref}) {
>-                    report_item_errors($record_id, $errors_ref);
>+                if (@{$errors}) {
>+                    report_item_errors($record_id, $errors);
>                 }
>                 C4::Biblio::_strip_item_fields($record, $framework);
>                 if ($record_has_added_items || $matched_record_id) {
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 25539: 105065 | 105066 | 109789 | 110260 | 110261 | 127599 | 127600 | 127601 | 127602 | 127603 | 127976 | 127977 | 127978 | 127979 | 127980 | 127981 | 133030 | 133031 | 133032 | 133034 | 133035 | 133036 | 140992 | 140993 | 140994 | 156224 | 156225 | 156226 | 160158 | 160159 | 160160 | 160489 | 160490 | 160491