Bug 32332

This is a bit weird, but now I can't reproduce the exact behaviour described in the first comment. On the first run with -import, the 999 field is missing. But if I the exact same command once more, the 999 goes away. And then I can re-run the -import as many times as I want, and no 999 is added. So run bulkmarcimport.pl with -import twice to reproduce the problem.

Done some more digging, and I think I found the problem. The POD for bulkmarcimport.pl reads:

    -insert
        if set, only insert when possible

    -update
        if set, only updates (any biblio should have a matching record)

    -all
        if set, do whatever is required

The way I interpret this, if -insert is provided, records that are not present in the database (based on the matching provided by -match) should be inserted, and records that are already present should be skipped and not updated. 

If the script is run with -match and -insert, it will look for an existing record. If one is found, then the following logic happens: 

            if ($biblionumber) {
                eval{
                    $biblioitemnumber = Koha::Biblios->find( $biblionumber )->biblioitem->biblioitemnumber;
                };
                if ($update) {
                    # If run with -update, but this is not what we are looking for
                } else {
                    # If run with -insert, nothing happens, except some logging
                    printlog( { id => $id || $originalid || $biblionumber, op => "insert", status => "warning : already in database" } ) if ($logfile);
                }
            } else {
                # An existing record was not found
            }

This looks OK, nothing happens if there is a match and -insert is provided. But then, further down in the script, ModBiblioMarc gets run for all records:

            ModBiblioMarc( $clone_record, $biblionumber );

This updates biblio_metadata.metadata, even if we did not want to update records that have a match! To check this behviour, run the "bulkmarcimport.pl -import" command I gave in the first comment, then log into the database and run this: 

MariaDB [koha_kohadev]> select timestamp from biblio_metadata where biblionumber = 439;

(Replace 439 with the biblionumber you got, if you got another biblionumber.)

Then run "bulkmarcimport.pl -import" again and check the timestamp again. It will be different from the first timestamp. 

This could have worked (sort of), if the 999 field was added in ModBiblioMarc, but since bug 29486 that has not been the case. 

I think the solution here is to not run ModBiblioMarc for records that have a match, when run with -insert?

I am not familiar with this script, but I am wondering: should not we skip to the next record way before we reach those lines?

Maybe we need a "next" in the "search for a match" block?

iff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl
index d2dfe1ad56a..45a7c94e39c 100755
--- a/misc/migration_tools/bulkmarcimport.pl
+++ b/misc/migration_tools/bulkmarcimport.pl
@@ -349,6 +349,7 @@ RECORD: while (  ) {
                     next;
                 }
             }
+            next if $insert;
         } elsif ( $results && scalar(@$results) > 1 ) {
             $logger->debug("more than one match for $query");
         } else {

But not knowing much this script it's hard to tell if it won't introduce regressions.

Or, another option, more inline with your diagnostic, could be to 

 diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl
index d2dfe1ad56a..e95b1fa199e 100755
--- a/misc/migration_tools/bulkmarcimport.pl
+++ b/misc/migration_tools/bulkmarcimport.pl
@@ -480,7 +480,7 @@ RECORD: while (  ) {
             C4::Biblio::_strip_item_fields($clone_record, '');
             # This sets the marc fields if there was an error, and also calls
             # defer_marc_save.
-            ModBiblioMarc( $clone_record, $biblionumber );
+            ModBiblioMarc( $clone_record, $biblionumber ) if $insert;
             if ( $error_adding ) {
                 warn "ERROR: Adding items to bib $biblionumber failed: $error_adding";
                                printlog({id=>$id||$originalid||$biblionumber, op=>"insertitem",status=>"ERROR"}) if ($logfile);

Thanks for looking at this! 

I find it kind of hard to get my head around the logic of this script too. 

> Maybe we need a "next" in the "search for a match" block?

I *think* it makes sense to skip to the next record if we have -insert, and we have found a matching record. But if we skip as early as line ~351, we miss the logging that happens on line 457:

printlog( { id => $id || $originalid || $biblionumber, op => "insert", status => "warning : already in database" } ) if ($logfile);

So that should probably be moved to before the "next". 

> Or, another option, more inline with your diagnostic, could be to...
> 
> -            ModBiblioMarc( $clone_record, $biblionumber );
> +            ModBiblioMarc( $clone_record, $biblionumber ) if $insert;

Just before this we do this: 

my $clone_record = $record->clone();

This seems to be unnecessary if are not updating anything anyway.

Magnus, is there an easy way to get around this with a fix?

(In reply to Hans Pålsson from comment #5)
> Magnus, is there an easy way to get around this with a fix?

It's been a long time since I looked at this, but from the comments above it sounds like it should be possible to add a "next" and avoid the problem.

In our test environment I still have the same issue, on 23.05.04.

(In reply to Magnus Enger from comment #6)
> (In reply to Hans Pålsson from comment #5)
> > Magnus, is there an easy way to get around this with a fix?
> 
> It's been a long time since I looked at this, but from the comments above it
> sounds like it should be possible to add a "next" and avoid the problem.

Magnus, we actually did find a strange thing in our installation. By removing the plugin Biblibre Dibspayment and the db-tables allocated to it posts actually get imported (!) with 999 correctly. So Insert seems to work. However Update removes 999 from posts which is not wanted behavior. We will try to protect 999 using MarcOverlayRules. I think this should be known since Dibs payment-plugin has nothing to do with import of posts.

(In reply to Hans Pålsson from comment #8)
> Magnus, we actually did find a strange thing in our installation. By
> removing the plugin Biblibre Dibspayment and the db-tables allocated to it
> posts actually get imported (!) with 999 correctly. So Insert seems to work.

Reminded me of bug 35930, but guess they are not related. 

> However Update removes 999 from posts which is not wanted behavior. We will
> try to protect 999 using MarcOverlayRules. I think this should be known
> since Dibs payment-plugin has nothing to do with import of posts.

How did the MarcOverlayRules work?

(In reply to Magnus Enger from comment #9)
> How did the MarcOverlayRules work?

It did not. :)