|
Description
Barton Chittenden
2018-08-26 16:35:32 UTC
*** Bug 14768 has been marked as a duplicate of this bug. *** Created attachment 155038 [details] [review] Bug 21272: MARC import should warn about mis-matched branch and item-type fields in 952 during staged import This plugin add a warning message to show missing branches during staged import, it also ignore item with wrong banches. To test Before make sure backgroundjobs process is running (/misc/workers/background_jobs_worker.pl --queue long_tasks) 1- Go to Cataloging -> Stage records for import 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or homelibraary set and is not present in your installation 3- Click on Stage for import 4- Click on View batch --> There is no message for the missing branches 5- Click on "Import this batch into the catalog" --> The importation failed 6- Apply the patch 7- Run prove -t t/db_dependent/ImportBatch.t 8- Click on "Manage imported batch" --> There is a warning message showing the missing branches in a link 9- Click on "Import this batch into the catalog" again --> The records are imported --> The items with wrong branches are ignored https://bugs.koha-community.org/show_bug.cgi?id=12532 Created attachment 155039 [details] [review] Bug 21272: Unit test https://bugs.koha-community.org/show_bug.cgi?id=12532 Created attachment 155043 [details]
Exported Marc file that contains items with branche code
Created attachment 155523 [details] [review] Bug 21272: MARC import should warn about mis-matched branch and item-type fields in 952 during staged import This plugin add a warning message to show missing branches during staged import, it also ignore item with wrong banches. To test Before make sure backgroundjobs process is running (/misc/workers/background_jobs_worker.pl --queue long_tasks) 1- Go to Cataloging -> Stage records for import 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or homelibraary set and is not present in your installation 3- Click on Stage for import 4- Click on View batch --> There is no message for the missing branches 5- Click on "Import this batch into the catalog" --> The importation failed 6- Apply the patch 7- Run prove -t t/db_dependent/ImportBatch.t 8- Click on "Manage imported batch" --> There is a warning message showing the missing branches in a link 9- Click on "Import this batch into the catalog" again --> The records are imported --> The items with wrong branches are ignored Signed-off-by: David Nind <david@davidnind.com> Created attachment 155524 [details] [review] Bug 21272: Unit test Signed-off-by: David Nind <david@davidnind.com> Thanks for adding a sample MARC file! I amended the commit messages to remove the link to bug 12532 (hope I have done this correctly!). I will add a follow-up to fix terminology and formatting: - In US English for the warning message: no space between the end of the warning message and the colon - "...missing branches...": changed to "...missing libraries..." (see https://wiki.koha-community.org/wiki/Terminology#B) For me, this did not seem to show a warning message when the item types in 952 don't match. I'm assuming these patches only cover library code mismatches. Created attachment 155525 [details] [review] Bug 21272: (follow-up) Terminology and formatting Change warning message to "..missing libraries...". Remove space before colon for the warning message. Signed-off-by: David Nind <david@davidnind.com> Comment on attachment 155523 [details] [review] Bug 21272: MARC import should warn about mis-matched branch and item-type fields in 952 during staged import Review of attachment 155523 [details] [review]: ----------------------------------------------------------------- ::: C4/ImportBatch.pm @@ +819,5 @@ > + my ( $holdingbranchfield, $holdingbranchsubfield ) = GetMarcFromKohaField('items.holdingbranch'); > + my $holdingbranch = $item_marc->subfield( $holdingbranchfield, $holdingbranchsubfield ); > + > + my @branches = map { $_->branchcode } Koha::Libraries->search->as_list; > + my %branches = map { $_ => 1 } @branches; why not use https://perldoc.perl.org/List::Util#any and get rid of %branches @@ +1235,5 @@ > + WHERE import_batch_id = ? > + ORDER BY import_items_id"; > + my @params; > + push( @params, $batch_id ); > + my $sth = $dbh->prepare_cached($query); Should this be a cached querey? Created attachment 158683 [details] [review] Bug 21272: (follow-up) using List::MoreUtils#any to get rid of %branches and fixing GetImportItemsBranches in ImportBatch.pm Please also fix the bug description, I think this bug only covers one half of it. I don't feel that the language "There are some missing libraries in your installation:" is good or sound. I think the better presumption is that the record has incorrect values, rather than those values should be created to accommodate those records. It just feels a bit wild to have a link to create new branches from the cataloging module. Are catalogers often charged with creating new branches? It looks like there is no permission check for this link either. I this feature simply needs to report the record values that don't match up with anything already in the database. Created attachment 163646 [details] [review] Bug 21272: Change warning message To test Before make sure backgroundjobs process is running (/misc/workers/background_jobs_worker.pl --queue long_tasks) 1- Go to Cataloging -> Stage records for import 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or homelibraary set and is not present in your installation 3- Click on Stage for import 4- Click on View batch --> There is no message for the missing branches 5- Click on "Import this batch into the catalog" --> The importation failed 6- Apply the patch 7- Run prove -t t/db_dependent/ImportBatch.t 8- Click on "Manage imported batch" --> There is a warning message showing the missing branches in a link 9- Click on "Import this batch into the catalog" again --> The records are imported --> The items with wrong branches are ignored Created attachment 163648 [details] [review] Bug 21272: Change warning message To test Before make sure backgroundjobs process is running (/misc/workers/background_jobs_worker.pl --queue long_tasks) 1- Go to Cataloging -> Stage records for import 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or homelibraary set and is not present in your installation 3- Click on Stage for import 4- Click on View batch --> There is no message for the missing branches 5- Click on "Import this batch into the catalog" --> The importation failed 6- Apply the patch 7- Run prove -t t/db_dependent/ImportBatch.t 8- Click on "Manage imported batch" --> There is a warning message showing the missing branches in a link 9- Click on "Import this batch into the catalog" again --> The records are imported --> The items with wrong branches are ignored I tested this. I had 5 bibliographic records with attached items. Each bibliographic record had multiple copies/items. I edited the first item on the first bibliographic record in the import file to have an incorrect branch code. When staging, I selected the option to always add the items. After staging the file, I received the warning that an item has an incorrect branchcode (nice!). I proceeded to import the file. None of the bibliographic records imported any items. I did not see a link in the warning message? I would have expected the one item with the incorrect branch code to be ignored. I could even expect the bibliographic record that contained this item would not have any items brought in. But I would expect the rest of the bibliographic records would import their items. It would be even more helpful if the warning message was more specific. If this was a large import file with a few hundred items, finding the one item with the bad branch code would be quite time consuming. Would it be possible for you to make the file you import available? Because when I run the test with the file provided by Hammat, it works correctly. I agree with you, I'll see if I can improve the display. Created attachment 164143 [details] [review] Bug 21272: Improve warning message To test Before make sure backgroundjobs process is running (/misc/workers/background_jobs_worker.pl --queue long_tasks) 1- Go to Cataloging -> Stage records for import 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or homelibraary set and is not present in your installation 3- Click on Stage for import 4- Click on View batch --> There is no message for the missing branches 5- Click on "Import this batch into the catalog" --> The importation failed 6- Apply the patch 7- Run prove -t t/db_dependent/ImportBatch.t 8- Click on "Manage imported batch" --> There is a warning message showing the missing branches --> There is a warning message showing the titles of the wrong items 9- Click on "Import this batch into the catalog" again --> The records are imported --> The items with wrong branches are ignored I have to add tests for the two new methods I added in Import.pm but tell me if the warning is better ? Created attachment 164147 [details]
Import file with an attached items and bad branch code on one
Here is the import file I tested with. Only one item on one bibliographic record has a bad branch code. That bibliographic record is Working effectively with legacy code, the first item has a branch code of MPAL instead of MPL.
I tested again and the error message is improved. It would be nice if the error message was retained in some way so that it can be referenced again after the import in the Manage Stage records for the file. Hammat's file works and the items with bad codes are ignored and the other items are brought in. My import file contains bibliographic records with multiple items for each. The error message was triggered correctly for the one item with a bad branch code. However, none of the items were brought in. Created attachment 164148 [details] [review] Bug 21272: Improve warning message To test Before make sure backgroundjobs process is running (/misc/workers/background_jobs_worker.pl --queue long_tasks) 1- Go to Cataloging -> Stage records for import 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or homelibraary set and is not present in your installation 3- Click on Stage for import 4- Click on View batch --> There is no message for the missing branches 5- Click on "Import this batch into the catalog" --> The importation failed 6- Apply the patch 7- Run prove -t t/db_dependent/ImportBatch.t 8- Click on "Manage imported batch" --> There is a warning message showing the missing branches --> There is a warning message showing the titles of the wrong items 9- Click on "Import this batch into the catalog" again --> The records are imported --> The items with wrong branches are ignored Current status: Failed QA (In reply to Matthias Le Gac from comment #21) > Created attachment 164148 [details] [review] [review] > Bug 21272: Improve warning message > > To test > > Before make sure backgroundjobs process is running > (/misc/workers/background_jobs_worker.pl --queue long_tasks) > > 1- Go to Cataloging -> Stage records for import > 2 - Upload a MARC file (koha.mrc) that contain items with homebranch and/or > homelibraary set and is not present in your installation > 3- Click on Stage for import > 4- Click on View batch > --> There is no message for the missing branches > 5- Click on "Import this batch into the catalog" > --> The importation failed > 6- Apply the patch > 7- Run prove -t t/db_dependent/ImportBatch.t > 8- Click on "Manage imported batch" > --> There is a warning message showing the missing branches > --> There is a warning message showing the titles of the wrong items > 9- Click on "Import this batch into the catalog" again > --> The records are imported > --> The items with wrong branches are ignored > > > Current status: Failed QA Just make tideness (In reply to Esther Melander from comment #20) > I tested again and the error message is improved. It would be nice if the > error message was retained in some way so that it can be referenced again > after the import in the Manage Stage records for the file. > > Hammat's file works and the items with bad codes are ignored and the other > items are brought in. > > My import file contains bibliographic records with multiple items for each. > The error message was triggered correctly for the one item with a bad branch > code. However, none of the items were brought in. Ok I will check with your file Created attachment 164149 [details] [review] Bug 21272: Unit test for methods GetBadBranchesImportItems and GetTitleImportRecord Current status: Failed QA I tested with your file and the items that are good imported well and those with bad branches did not. For the record "Working effectively with legacy code" I have the item with homebranch "FPL" but not the item with homebranch "MPAL". |