Bug 20811 - Fix wrong usage of ModBiblio in bulkmarcimport.pl
Summary: Fix wrong usage of ModBiblio in bulkmarcimport.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 5635
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-24 09:36 UTC by Magnus Enger
Modified: 2019-10-14 19:56 UTC (History)
11 users (show)

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


Attachments
Bug 20811: Fix wrong usage of ModBiblio (1.58 KB, patch)
2018-05-28 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20811: Fix update flag in bulkmarcimport (1.20 KB, patch)
2018-07-11 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20811: Fix wrong usage of ModBiblio (1.58 KB, patch)
2018-07-12 12:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20811: Fix update flag in bulkmarcimport (1.26 KB, patch)
2018-07-12 12:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20811: Fix wrong usage of ModBiblio (1.64 KB, patch)
2018-07-12 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20811: Fix update flag in bulkmarcimport (1.26 KB, patch)
2018-07-12 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20811: Fix wrong usage of ModBiblio (1.73 KB, patch)
2018-07-16 07:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20811: Fix wrong usage of ModBiblio (1.73 KB, patch)
2018-07-20 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20811: (QA follow-up) Make the isbn check work (1.65 KB, patch)
2018-07-20 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20811: (QA follow-up) Prevent calling AddItemBatchFromMarc and ModBiblioMarc without biblionumber (2.09 KB, patch)
2018-07-20 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2018-05-24 09:36:48 UTC
In misc/migration_tools/bulkmarcimport.pl, line  454, ModBiblio is called like so:

  eval { ( $biblionumber, $biblioitemnumber ) = ModBiblio( $record, $biblionumber, GetFrameworkCode($biblionumber) ) };

But the POD for ModBiblio says:

  "Returns 1 on success 0 on failure"

So $biblionumber gets 1 or 0, and $biblioitemnumber never gets anything. This leads to problems when ModBiblioMarc is called on line 487:

  ModBiblioMarc( $clone_record, $biblionumber, $framework );

If the ModBiblio was successfull, this will try to updated the record with biblionumber = 1, which is most likely not what we want.

(I probably won't have the time to fix this soon, so please beat me to it!)
Comment 1 Jonathan Druart 2018-05-28 18:21:35 UTC
Created attachment 75599 [details] [review]
Bug 20811: Fix wrong usage of ModBiblio

Since
  commit cefa7c21e28b88351ee8ae0dfefb80a515323df9
  Bug 5635: bulkmarcimport new parameters & features

AddBiblio call has been replaced with ModBiblio, but the return values
are different. We should not replace the value of $biblionumber with
what returns this subroutine.

Test plan:
If you are familiar with bulkmarcimport.pl you should know what to test,
I am not.
Comment 2 Mark Tompsett 2018-06-04 13:48:47 UTC
What about other places?
misc/batchRepairMissingBiblionumbers.pl
I believe that has a bad ModBiblio line too.
Changing to In Discussion, until we determine whether to fix in other bug, or creep the scope to include all bad ModBiblio calls.
Comment 3 Jonathan Druart 2018-06-04 14:01:07 UTC
batchRepairMissingBiblionumbers.pl uses ModBiblioMarc, not ModBiblio.
Comment 4 Katrin Fischer 2018-07-08 18:18:54 UTC
Need some reinforcements here, I think we are looking at a bigger issue. 

With and without the patch I don't get it to work with the -update option:

$ ./bulkmarcimport.pl -b -file ../../data/Katrin-biblios.mrc -v -update
Characteristic MARC flavour: MARC21
.DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`biblio_metadata`, CONSTRAINT `record_metadata_fk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `biblio_metadata` ( `format`, `marcflavour`, `metadata`) VALUES ( ?, ?, ? )" with ParamValues: 0='marcxml', 1='MARC21', 2="<?xml version="1.0" encoding="UTF-8"?>
<record
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd"
    xmlns="http://www.loc.gov/MARC21/slim">

  <leader>00647cas a2200229   4500</leader>
  <controlfield tag="001">012079189</controlfield>
  <controlfield tag="003">DE-576</controlfield>
  <controlfield tag="005">20180708032646.0</controlfield>
  <controlfield tag="007">tu</controlfield>
  <controlfield tag="008">860818cuuuuuuuuxx    m            0ita c</controlfield>
  <datafield tag="016" ind1=" " ind2=" ">
    <subfield code="a">(OCoLC)72970470</subfield>
  </datafield>
  <datafield tag="035" ind1=" " ind2=" ">
    <subfield code="a">(DE-599)BSZ012079189</subfield>
  </datafield>
  <datafield tag="040" ind1=" " ind2=" ">
    <subfield code="a">DE-576</subfield>
    <subfield code="b">ger</subfield>
    <subfield code="c">DE-576</subfield>
    <subfield code="e"..."] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832, <GEN27> line 1.
Use of uninitialized value in sprintf at /home/vagrant/kohaclone/Koha/Exceptions/Exception.pm line 19, <GEN27> line 1.
Invalid parameter passed, biblionumber= does not exist

I've also tried using it with match:
$ ./bulkmarcimport.pl -b -file ../../data/Katrin-biblios.mrc -v -match Control-number,001 -update

The result is always the same.
Comment 5 Katrin Fischer 2018-07-08 18:19:54 UTC
Should add: tests were done on a freshly reset_all MARC21 database.
Comment 6 Jonathan Druart 2018-07-11 13:48:20 UTC
> Invalid parameter passed, biblionumber= does not exist

This sounds like another bug, I would prefer to discuss it on a separate bug report.

Try without the --update flag.
Comment 7 Katrin Fischer 2018-07-11 13:53:45 UTC
The ModBiblio call is inside the check for a set update flag...

if ($update)...
Comment 8 Jonathan Druart 2018-07-11 13:54:49 UTC
heh yes :)
Comment 9 Jonathan Druart 2018-07-11 14:18:29 UTC
Created attachment 76861 [details] [review]
Bug 20811: Fix update flag in bulkmarcimport

I have no idea why we have these two variables $id and $originalid, it
seems that they should not cohabit.

It feels like Users and original authors of this script should provide an
update and refresh it.
Comment 10 Martin Renvoize 2018-07-12 12:21:17 UTC
Created attachment 76885 [details] [review]
Bug 20811: Fix wrong usage of ModBiblio

Since
  commit cefa7c21e28b88351ee8ae0dfefb80a515323df9
  Bug 5635: bulkmarcimport new parameters & features

AddBiblio call has been replaced with ModBiblio, but the return values
are different. We should not replace the value of $biblionumber with
what returns this subroutine.

Test plan:
If you are familiar with bulkmarcimport.pl you should know what to test,
I am not.
Comment 11 Martin Renvoize 2018-07-12 12:21:21 UTC
Created attachment 76886 [details] [review]
Bug 20811: Fix update flag in bulkmarcimport

I have no idea why we have these two variables $id and $originalid, it
seems that they should not cohabit.

It feels like Users and original authors of this script should provide an
update and refresh it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-07-12 12:22:15 UTC
Created attachment 76887 [details] [review]
Bug 20811: Fix wrong usage of ModBiblio

Since
  commit cefa7c21e28b88351ee8ae0dfefb80a515323df9
  Bug 5635: bulkmarcimport new parameters & features

AddBiblio call has been replaced with ModBiblio, but the return values
are different. We should not replace the value of $biblionumber with
what returns this subroutine.

Test plan:
If you are familiar with bulkmarcimport.pl you should know what to test,
I am not.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-07-12 12:22:18 UTC
Created attachment 76888 [details] [review]
Bug 20811: Fix update flag in bulkmarcimport

I have no idea why we have these two variables $id and $originalid, it
seems that they should not cohabit.

It feels like Users and original authors of this script should provide an
update and refresh it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-07-12 12:23:17 UTC
Works as expected.. the $id and $originalid may both be populated if $match is used.. some of the logging in there is rather odd but that shouldn't require any fixes in this bug.

Signing Off
Comment 15 Marcel de Rooy 2018-07-13 06:03:21 UTC
Hmm. The first patch is obviously needed.
But I have my doubts on the second patch. Can we move it to another report to expedite handling the first ?

About the second:
> I have no idea why we have these two variables $id and $originalid, it
> seems that they should not cohabit.
> It feels like Users and original authors of this script should provide an
> update and refresh it.
I do not know either. But because of that it seems to me that we need more proof that this change is valid. Why do you change it? How did you test it? etc.
The only assignment to $id that I see is line 328 when the condition $match is true and @$results == 1. Does $id here become the biblionumber of the only match/duplicate? Apparently this record is updated by the script.
$originalid is the 999c or 001 from the imported batch record. So it seems to me that it should not be a valid biblionumber. And certainly no candidate for $biblionumber.
Look also at the remaining lines testing if there is a biblionumber. 
I have the strong feeling that this is a very bad change !
Comment 16 Jonathan Druart 2018-07-13 10:53:41 UTC
I do not have time to work more on this script.
Comment 17 Marcel de Rooy 2018-07-16 07:51:03 UTC
Created attachment 76969 [details] [review]
Bug 20811: Fix wrong usage of ModBiblio

Since
  commit cefa7c21e28b88351ee8ae0dfefb80a515323df9
  Bug 5635: bulkmarcimport new parameters & features

AddBiblio call has been replaced with ModBiblio, but the return values
are different. We should not replace the value of $biblionumber with
what returns this subroutine.

Test plan:
If you are familiar with bulkmarcimport.pl you should know what to test,
I am not.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2018-07-16 07:52:46 UTC
(In reply to Jonathan Druart from comment #16)
> I do not have time to work more on this script.

OK I will obsolete the second patch. With the first patch we fix something ugly. If more scripts need attention, we can do so on a new report.
Comment 19 Jonathan Druart 2018-07-16 13:11:06 UTC
First patch is useless without the second patch...
Comment 20 Marcel de Rooy 2018-07-16 13:18:56 UTC
(In reply to Jonathan Druart from comment #19)
> First patch is useless without the second patch...

Not so sure about that. Please explain.
Comment 21 Jonathan Druart 2018-07-16 13:23:42 UTC
(In reply to Marcel de Rooy from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > First patch is useless without the second patch...
> 
> Not so sure about that. Please explain.

Comments 6 to 9
Comment 22 Marcel de Rooy 2018-07-16 13:26:13 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Marcel de Rooy from comment #20)
> > (In reply to Jonathan Druart from comment #19)
> > > First patch is useless without the second patch...
> > 
> > Not so sure about that. Please explain.
> 
> Comments 6 to 9

I saw them. But that does not make this patch useless. We fix a wrong call. If it is in enclosed by if($update), the fix is conditional.
Comment 23 Katrin Fischer 2018-07-16 13:50:01 UTC
It doesn't work, see my tests.
Comment 24 Katrin Fischer 2018-07-16 13:54:30 UTC
We fix a wrong call... but the feature will remain broken. Might just be hiding the real problem. Maybe we should change this bug to 'update option is broken in bulkmarcimport.pl' for now.
Comment 25 Jonathan Druart 2018-07-16 13:55:53 UTC
Removing from PQA for now.
Comment 26 Marcel de Rooy 2018-07-17 12:16:48 UTC
(In reply to Katrin Fischer from comment #23)
> It doesn't work, see my tests.

Sure? Not a configuration issue on a fresh dev box?
Comment 27 Marcel de Rooy 2018-07-17 12:18:34 UTC
(In reply to Katrin Fischer from comment #24)
> We fix a wrong call... but the feature will remain broken. Might just be
> hiding the real problem. Maybe we should change this bug to 'update option
> is broken in bulkmarcimport.pl' for now.

I will test this a bit further coming Friday. No need to change anything before that.
Comment 28 Katrin Fischer 2018-07-17 12:35:19 UTC
It's only the update opption that I can't get to work. I've documented my testing in comment#4. I've done quite somte tests with bulkmarkimport lately and it worked just fine otherwise.
Comment 29 Marcel de Rooy 2018-07-17 12:40:33 UTC
(In reply to Katrin Fischer from comment #28)
> It's only the update opption that I can't get to work. I've documented my
> testing in comment#4. I've done quite somte tests with bulkmarkimport lately
> and it worked just fine otherwise.

Can imagine btw that the error message about a wrong biblionumber (foreign key) is caused by the wrong ModBiblio call and how its results are saved. But I will have a look.
Comment 30 Marcel de Rooy 2018-07-20 07:32:02 UTC
QA: Looking here again now
Comment 31 Marcel de Rooy 2018-07-20 09:53:28 UTC
(In reply to Katrin Fischer from comment #28)
> It's only the update opption that I can't get to work. I've documented my
> testing in comment#4. I've done quite somte tests with bulkmarkimport lately
> and it worked just fine otherwise.

Since [cefa7c21e28b88351ee8ae0dfefb80a515323df9] Bug 5635: bulkmarcimport new parameters & features (version 3.11.00.109) the code contains the wrong ModBiblio call.
If I use the -update parameter now with the -match parameter (e.g. pass "Control-number,001"), the fix works fine.

But that is not the complete story.
If you use the -update option without the -match parameter, you won't reach the ModBiblio call, but you will get errors from AddItemBatchFromMarc (called without bibionumber) and from ModBiblioMarc.
If you use the -update option with -isbn, there will be no match on isbn due to a bug with the $isbn variable.

Will add two simple follow-ups to overcome both situations.
Comment 32 Marcel de Rooy 2018-07-20 09:57:17 UTC
Created attachment 77154 [details] [review]
Bug 20811: Fix wrong usage of ModBiblio

Since
  commit cefa7c21e28b88351ee8ae0dfefb80a515323df9
  Bug 5635: bulkmarcimport new parameters & features

AddBiblio call has been replaced with ModBiblio, but the return values
are different. We should not replace the value of $biblionumber with
what returns this subroutine.

Test plan:
If you are familiar with bulkmarcimport.pl you should know what to test,
I am not.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2018-07-20 09:57:22 UTC
Created attachment 77155 [details] [review]
Bug 20811: (QA follow-up) Make the isbn check work

The following code was never reached, since $isbn was not filled.
    if (!$biblionumber && $isbn_check && $isbn) {
        $sth_isbn->execute($isbn);
        ($biblionumber,$biblioitemnumber) = $sth_isbn->fetchrow;
    }
Solution: Fix the code with two $isbn declarations. Move the checkisbn
condition a level deeper.

Test plan:
Run misc/migration_tools/bulkmarcimport.pl -file bib726.utf8 --update -isbn
Since you do not match on biblionumber, the ISBN should match.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2018-07-20 09:57:26 UTC
Created attachment 77156 [details] [review]
Bug 20811: (QA follow-up) Prevent calling AddItemBatchFromMarc and ModBiblioMarc without biblionumber

If you use -update but do not find matches (or did not want to match), you
should not call those routines. We should warn and skip this record.

Adding a warn at the start that the choice of options may not be smart.
Note that this needs further attention somewhere else. You could mix
-update with -insert for instance and still see some problems. (May depend
on items with unique barcode etc.)

Test plan:
Run -update without match or isbn.
Or run -update -isbn with a non-matching ISBN.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2018-07-20 10:00:18 UTC
Adding back Jonathan as assignee :) He wrote the original fix.
Note that this bug survived quite some releases (3.12 - 18.05)
With the two follow-ups we fix a bad call and improve the use of -update.
Further improvements are welcome on new reports.
Comment 36 Nick Clemens 2018-07-23 15:38:29 UTC
Awesome work all!

Pushed to master for 18.11
Comment 37 Martin Renvoize 2018-08-02 14:19:11 UTC
Pushed to 18.05.x for 18.05.03
Comment 38 Fridolin Somers 2018-08-28 13:10:56 UTC
Pushed to 17.11.x for 17.11.09
Comment 39 Fridolin Somers 2018-09-24 13:04:42 UTC
Pushed to 17.05.x for 17.05.14