Bug 32332 - bulkmarcimport.pl -insert fails to add MARC field 999
Summary: bulkmarcimport.pl -insert fails to add MARC field 999
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-23 09:56 UTC by Magnus Enger
Modified: 2024-04-05 10:11 UTC (History)
6 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2022-11-23 09:56:00 UTC
We have several libraries that have problems with their batch imports/updates from external sources after upgrading to 22.05.06. The problem is that records that are imported and/or updated via bulkmarcimport.pl do not get a 999 field, with the biblionumber in $c and $d. No 999 field is added at all. 

To reproduce in koha-testing-docker:

- Edit koha/t/db_dependent/Record/testrecords/marcxml_utf8.xml and remove the 952 field (otherwise Koha will complain about an undefined branchcode)

- Import that file: 
$ sudo koha-shell -c "perl koha/misc/migration_tools/bulkmarcimport.pl -b -v -m=MARCXML -match=Control-number,001 -insert -l=/tmp/import.log -file koha/t/db_dependent/Record/testrecords/marcxml_utf8.xml" kohadev

- Check the result: 
$ sudo koha-mysql kohadev
MariaDB [koha_kohadev]> SELECT biblionumber, ExtractValue(metadata, 'count(//datafield[@tag="999"])') AS count999 FROM biblio_metadata HAVING count999 = 0;

This should give output like this: 

+--------------+----------+
| biblionumber | count999 |
+--------------+----------+
|          439 | 0        |
+--------------+----------+
1 row in set (0.013 sec)

...meaning biblionumber 439 does not have a 999 field. 

- Updating the same record: 
$ sudo koha-shell -c "perl koha/misc/migration_tools/bulkmarcimport.pl -b -v -m=MARCXML -match=Control-number,001 -update -l=/tmp/import.log -file koha/t/db_dependent/Record/testrecords/marcxml_utf8.xml" kohadev

...seems to add the 999 and make the problem go away: 
$ sudo koha-mysql kohadev
MariaDB [koha_kohadev]> SELECT biblionumber, ExtractValue(metadata, 'count(//datafield[@tag="999"])') AS count999 FROM biblio_metadata HAVING count999 = 0;
Empty set (0.009 sec)

BUT on live servers (running 22.05.06, not master like in ktd) it looks like updating a record via bulkmarcimport.pl can cause an existing record to loose its 999 field.
Comment 1 Magnus Enger 2022-11-23 14:00:29 UTC
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.
Comment 2 Magnus Enger 2022-11-23 14:46:19 UTC
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?
Comment 3 Jonathan Druart 2022-11-25 06:50:55 UTC
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);
Comment 4 Magnus Enger 2022-12-05 08:26:49 UTC
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.
Comment 5 Hans Pålsson 2023-09-14 13:54:48 UTC
Magnus, is there an easy way to get around this with a fix?
Comment 6 Magnus Enger 2023-09-15 06:26:03 UTC
(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.
Comment 7 Hans Pålsson 2023-11-01 10:26:15 UTC
In our test environment I still have the same issue, on 23.05.04.
Comment 8 Hans Pålsson 2023-11-20 12:27:22 UTC
(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.
Comment 9 Magnus Enger 2024-02-23 09:49:09 UTC
(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?
Comment 10 Hans Pålsson 2024-04-05 10:11:22 UTC
(In reply to Magnus Enger from comment #9)
> How did the MarcOverlayRules work?

It did not. :)