PROBLEM 1 If a record as some than one ISBN, the values are recorded in database like 'isbn1 | isbn2 | isbn3'. In the script the ISBN match are only checking for exact ISBN matching. my $sth_isbn = $dbh->prepare("SELECT biblionumber,biblioitemnumber FROM biblioitems WHERE isbn=? OR isbn like ? OR isbn like ? OR isbn like ?"); The script should verify if the ISBN is a exact match or if it appears in start, in the middle or in the end of the database isbn column: my $sth_isbn = $dbh->prepare("SELECT biblionumber,biblioitemnumber FROM biblioitems WHERE isbn=? OR isbn like ? OR isbn like ? OR isbn like ?"); The query execution should also be changed: $sth_isbn->execute($isbn, $isbn." | %", "% | ".$isbn, "% | ".$isbn." | %"); PROBLEM 2 The ISBN is not being saved in the variable $isbn because of the following line: my $isbn = $field && $field->subfield('a'); The "my" before the variable should be removed. PROBLEM 3 The $biblionumber and $biblioitemnumber are being lost after the record modification in the case of being a duplicate record. ModBiblio returns 1 if everything works right in the record modification. The following line code should be modified: eval { ( $biblionumber, $biblioitemnumber ) = ModBiblio( $record, $biblionumber, GetFrameworkCode($biblionumber) ) }; The result should be: eval { ModBiblio( $record, $biblionumber, GetFrameworkCode($biblionumber) ) }; PROBLEM 4 When modifying a record, maybe we want to append the items to the existing record. So we should pass a clone of the record to the method ModBiblio, because that method strips all items in the record. This should be done with a new argument. -updateadditems The line: eval { ModBiblio( $record, $biblionumber, GetFrameworkCode($biblionumber) ) }; Is changed to: my $clone = $updateadditems ? $record->clone() : $record; eval { ModBiblio( $clone, $biblionumber, GetFrameworkCode($biblionumber) ) };
Created attachment 40362 [details] [review] bulkmarcimport.pl
Hi Vitor, can you please add a test plan? Some sample records might also be helpful to make this easier to test.
Ah, also make sure test plan and problem description are part of your commit message!
Created attachment 41338 [details] [review] Bug 14420 : bulkmarcimport.pl - Some problems http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14420 Created by: Vitor Fernandes <vfernandes@keep.pt> Sponsored by: KEEP SOLUTIONS
Created attachment 41339 [details] [review] Bug 14420 : bulkmarcimport.pl - Some problems http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14420 Test plan: - Add one record with two ISBN using bulkmarcimport - Add the same record using bulkmarcimport with ISBN comparing - The comparing of ISBN isn't working because the bulkmarcimport are trying to check if isbn1 is in the column isbn of biblio table (check if equal). The column isbn of the first record is "isbn1 | isbn2". - Add one record with one ISBN using bulkmarcimport - Add the same record using bulkmarcimport and ISBN comparing - The first record will be updated but the items of the second record will not be added to the first one - Apply the patch - Add one record with two ISBN using bulkmarcimport - Add the same record using bulkmarcimport with ISBN comparing - The comparing of ISBN will work because the bulkmarcimport will use "like" comparing instead of "equal" - Add one record with one ISBN using bulkmarcimport - Add the same record using bulkmarcimport with ISBN comparing and updateadditems - The first record will be updated and the items of the second record will be added to the first one Created by: Vitor Fernandes <vfernandes@keep.pt> Sponsored by: KEEP SOLUTIONS
(In reply to Vitor Fernandes from comment #0) > PROBLEM 1 > > If a record as some than one ISBN, the values are recorded in database like > 'isbn1 | isbn2 | isbn3'. > > In the script the ISBN match are only checking for exact ISBN matching. This is because: 020 - International Standard Book Number (R) -- The 020 field is repeatable. This is clearly bad data, but yes, we should attempt to parse for it. I don't like the multiple likes fix provided here. I would prefer a: my @isbns = parseIsbnField($isbn_field_value); foreach my $isbn (@isbns) { search for the individual isbn ... } sub parseIsbnField { my ($isbn_data) = @_; see if we can split out some ISBNs. ... return an array of the results. }
Can you please rebase your patch it is currently not applying cleanly on master, as the below output shows: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14420 : bulkmarcimport.pl - Some problems Using index info to reconstruct a base tree... M misc/migration_tools/bulkmarcimport.pl Falling back to patching base and 3-way merge... Auto-merging misc/migration_tools/bulkmarcimport.pl CONFLICT (content): Merge conflict in misc/migration_tools/bulkmarcimport.pl Failed to merge in the changes. Patch failed at 0001 Bug 14420 : bulkmarcimport.pl - Some problems The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-14420--bulkmarcimportpl---Some-problems-T8e2dA.patch You have new mail in /var/mail/vagrant