From 52d5a52ae588ea6174470ed24cad9310d4b073ed Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 2 Feb 2023 20:16:49 +0000 Subject: [PATCH] Bug 32804: [22.05.x] Do not adjust biblionumber when replacing items during import This patch adjust the item matching at import to confirm that for a duplicate itemnumber or barcode matches an existing item in the DB and uses the original biblionumber when updating the item. When ordering in a consortium the items may be moved around, duplicate biblios added, and various matches found. We should not allow importing of items to move them from 1 biblio to another, but we should allow the imports to succeed and assume itemnumber or barcode matches are authoritative. The responsibility for correct matching of items to biblio should fall on the creator of the files To test: 1 - Be using the sample data in koha testing docker 2 - Stage the sample file on this report 3 - Match on KohaBiblio(999$c) / Item processing: replace 4 - Note the biblio match is "The complete novels / Austen, Jane" 5 - View the staged marc, note the barcode 39999000004090 in an item 6 - Search for that barcode 7 - You find biblio "Five novels : complete and unabridged / Gustave Flaubert" 8 - Import the file 9 - Check the db: SELECT * FROM items WHERE biblionumber != biblioitemnumber; 10 - Note the mismatch 11 - Fix the item and set it as 'Music' type UPDATE items SET biblionumber = biblioitemnumber, itype='MU' WHERE biblionumber != biblioitemnumber; 12 - Apply patch, restart all 13 - Stage and import the file with the same settings 14 - Confirm the item is modified on its original biblio (99) and that item type is Book again 15 - Change itemnumber to avoid itemnumber match and reset type UPDATE items SET itype='MU', itemnumber=999 WHERE itemnumber=212; 16 - Stage and import with the same setttings 17 - Confirm the marcode match worked and item is updated to book on original record 18 - Delete the original item 19 - Stage and import the file with the same settings 20 - The item is successfully created 21 - Stage and import, but item processing option is 'add' 22 - Confirm 1 item ignored 23 - Check the db SELECT * FROM import_items WHERE barcode=39999000004090 24 - Confirm there is a line with 'error' and duplicate_barcode JD amended patch - # We assume that when replaicing tiems we do not want to move them - the onus is on the importer to + # We assume that when replacing items we do not want to move them - the onus is on the importer to Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Marcel de Rooy Bug 32804: Add unit tests JD amended patch - is( $item1->homebranch, $library->branchcode, "Item was overlayed succesfully" ); + is( $item1->homebranch, $library->branchcode, "Item was overlaid successfully" ); Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Marcel de Rooy Bug 32804: (follow-up) Test that id's have not changed Rather than assuming biblionumber and biblioitemnumber are the same, we should test that they have not changed form their original values Signed-off-by: Marcel de Rooy Bug 32804: (QA follow-up) Typo ahs and fix ImportBatch.t Resolve: # Failed test 'Item's biblioitemnumber has not changed' # at t/db_dependent/ImportBatch.t line 407. # # got: '4261' # expected: '2371' Do not compare $item1->biblionumber with $original_biblioitemnumber :) Signed-off-by: Marcel de Rooy --- C4/ImportBatch.pm | 28 +++++++++++++++----- t/db_dependent/ImportBatch.t | 51 +++++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index 1dd75382f9..c146671b94 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -716,13 +716,20 @@ sub BatchCommitItems { my $item = TransformMarcToKoha( $item_marc ); - my $duplicate_barcode = exists( $item->{'barcode'} ) && Koha::Items->find({ barcode => $item->{'barcode'} }); + my $item_match; + my $duplicate_barcode = exists( $item->{'barcode'} ); my $duplicate_itemnumber = exists( $item->{'itemnumber'} ); + # We assume that when replacing items we do not want to move them - the onus is on the importer to + # ensure the correct items/records are being updated my $updsth = $dbh->prepare("UPDATE import_items SET status = ?, itemnumber = ?, import_error = ? WHERE import_items_id = ?"); - if ( $action eq "replace" && $duplicate_itemnumber ) { + if ( + $action eq "replace" && + $duplicate_itemnumber && + ( $item_match = Koha::Items->find( $item->{itemnumber} )) + ) { # Duplicate itemnumbers have precedence, that way we can update barcodes by overlaying - ModItemFromMarc( $item_marc, $biblionumber, $item->{itemnumber} ); + ModItemFromMarc( $item_marc, $item_match->biblionumber, $item->{itemnumber}); $updsth->bind_param( 1, 'imported' ); $updsth->bind_param( 2, $item->{itemnumber} ); $updsth->bind_param( 3, undef ); @@ -730,9 +737,12 @@ sub BatchCommitItems { $updsth->execute(); $updsth->finish(); $num_items_replaced++; - } elsif ( $action eq "replace" && $duplicate_barcode ) { - my $itemnumber = $duplicate_barcode->itemnumber; - ModItemFromMarc( $item_marc, $biblionumber, $itemnumber ); + } elsif ( + $action eq "replace" && + $duplicate_barcode && + ( $item_match = Koha::Items->find({ barcode => $item->{'barcode'} }) ) + ) { + ModItemFromMarc( $item_marc, $item_match->biblionumber, $item_match->itemnumber ); $updsth->bind_param( 1, 'imported' ); $updsth->bind_param( 2, $item->{itemnumber} ); $updsth->bind_param( 3, undef ); @@ -740,7 +750,11 @@ sub BatchCommitItems { $updsth->execute(); $updsth->finish(); $num_items_replaced++; - } elsif ($duplicate_barcode) { + } elsif ( + # We aren't replacing, but the incoming file has a barcode, we need to check if it exists + $duplicate_barcode && + ( $item_match = Koha::Items->find({ barcode => $item->{'barcode'} }) ) + ) { $updsth->bind_param( 1, 'error' ); $updsth->bind_param( 2, undef ); $updsth->bind_param( 3, 'duplicate item barcode' ); diff --git a/t/db_dependent/ImportBatch.t b/t/db_dependent/ImportBatch.t index c93bc53019..414ffdd5db 100755 --- a/t/db_dependent/ImportBatch.t +++ b/t/db_dependent/ImportBatch.t @@ -1,7 +1,7 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 18; +use Test::More tests => 19; use utf8; use File::Basename; use File::Temp qw/tempfile/; @@ -328,6 +328,55 @@ subtest "_get_commit_action" => sub { }; + +subtest "Do not adjust biblionumber when replacing items during import" => sub { + plan tests => 7; + + my $item1 = $builder->build_sample_item; + my $original_biblionumber = $item1->biblionumber; + my $original_biblioitemnumber = $item1->biblioitemnumber; + my $item2 = $builder->build_sample_item; + + my $library = $builder->build_object({ class => 'Koha::Libraries' }); + + my $import_item = $builder->build_object({ class => 'Koha::Import::Record::Items', value => { + marcxml => qq{ + + + + 00000 a + + ${\($library->branchcode)} + ${\($library->branchcode)} + GEN + ${\($item1->barcode)} + BK + + + + }, + }}); + + isnt( $item1->homebranch, $library->branchcode, "Item's homebranch is currently not the same as our created branch's branchcode" ); + + my ( $num_items_added, $num_items_replaced, $num_items_errored ) = + C4::ImportBatch::BatchCommitItems( $import_item->import_record_id, $item2->biblionumber, 'replace' ); + + $item1->discard_changes(); + + is( $num_items_errored, 0, 'Item was replaced' ); + $import_item->discard_changes(); + is( $import_item->status, 'imported', 'Import was successful'); + is( $import_item->import_error, undef, 'No error was reported' ); + + is( $item1->biblionumber, $original_biblionumber, "Item's biblionumber has not changed" ); + is( $item1->biblioitemnumber, $original_biblioitemnumber, "Item's biblioitemnumber has not changed" ); + is( $item1->homebranch, $library->branchcode, "Item was overlaid successfully" ); +}; + sub get_import_record { my $id_import_batch = shift; return $dbh->do('SELECT * FROM import_records WHERE import_batch_id = ?', undef, $id_import_batch); -- 2.30.2