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. 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. batchRepairMissingBiblionumbers.pl uses ModBiblioMarc, not ModBiblio. 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.
Should add: tests were done on a freshly reset_all MARC21 database. > 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.
The ModBiblio call is inside the check for a set update flag... if ($update)... heh yes :) 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. 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. 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> 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> 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> 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 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 !
I do not have time to work more on this script. 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> (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. First patch is useless without the second patch... (In reply to Jonathan Druart from comment #19) > First patch is useless without the second patch... Not so sure about that. Please explain. (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 (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. It doesn't work, see my tests. 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. Removing from PQA for now. (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? (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. 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. (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. QA: Looking here again now (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. 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> 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> 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> 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. Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.03 Pushed to 17.11.x for 17.11.09 Pushed to 17.05.x for 17.05.14 |
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!)