In order to address various issues with and add some new features to bulkmarcimport.pl I found it necessary to first clean up the script. At our library we run a modified version of bulkmarcimport.pl that for practical reasons was squashed into one commit. I have split this commit into various feature branches, all which depends on the clean up version of the script. So in the first commit of this issue I tried to isolate all the changes that should not affect current script behavior and are mostly related to code formatting, structure and optimizations. The next commits (for this issue) will contain changes that do affect script behavior, but that I would argue should be the default behavior and where the current could be considered confusing and/or buggy. I will later create new related issues for the newly added features mentioned above (and it's possible there is stuff in this patch that should have been broken out and I have overlooked).
Created attachment 127479 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Created attachment 127480 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 127481 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
To make this easier to review I will make a rough summary/motivation of the major changes, just did not have the time to do so right now.
Created attachment 127485 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 127486 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Hi David, that reads great. If you have a lot of patches coming, you could also use this as an omnibus bug and link the bugs that belong to it using the blocks and depends on fields. You can also use these fields to build a hierarchy that will be used when applying patches using git bz. Thx for your work, I know that script has grown a lot over the years!
Yes, I will make sure to mark the other patches (will probably only be one or two more besides 25539) as depending on this one, as far as I understand this is all I have to do to make this an omnibus bug? For reviewers of this patch, here is a short summary of the changes: Added an option 'defer_search_engine_indexing' to ModBiblio/Modbiblio marc, which is used in the script for avoiding indexing each item individually and to do so in bulk when changes are committed to the database. This is more efficient, and you don't run the same risk of ending up with junk data in the index if the script crashes and the current database transaction is rolled back. Instead of processing records in one loop, the processing is split up into two loops. The first one for pre-processing tasks (transcoding, filtering out broken records etc) that does not touch the database. This makes the code in the import loop cleaner, and it's also nice to have the option of bailing out before any database changes are committed (could for example add the option to abort the whole import if contains one or more invalid records). The rest of the changes more or less of fixing formatting/indentation, renaming some variables for clarity purposes, making error/log messages more consistent and generally just cleaning up the code.
Created attachment 127508 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 127509 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Created attachment 127651 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 127652 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Created attachment 127791 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 127792 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Created attachment 127795 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 127796 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Hi David, could you please also add a test plan that highlights the changes for testers? They usually should be added to the commit message. Also this is still marked as NEW - should it be Needs signoff to call for testers?
Hi! As it wasn't really ready for signoff I did not change the status. Have done some manual testing to go through most of the options and catch possible issues after splitting the code into feature branches. There are no (serious at least) bugs fixed, and no features added, just a clean up, so what need to make sure is that the script works as before. There is no test-suite for bulkmarkimport.pl so it's a lot of work for reviewers to go through all the options and fully test the script, but I can at least provide some basic steps wit the most common options.
Created attachment 127959 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl To test: 1) Import biblios using ./bulkmarcimport.pl -v -b -file biblios.mrc -insert -update -c=MARC21 -match "control-number,001" -n=1 -l "/tmp/import.log" 2) Verify in /tmp/import.log that one biblio has been inserted 3) Import again without the "-n" option, verify that one biblio has been updated and the rest inserted 4) In the staff interface search the catalog for some string appearing in the imported biblios to verify records have been indexed 5) View/edit some of the biblios the staff interface to very have been properly imported 6) Import authorities using ./bulkmarcimport.pl -v -m=MARCXML -a -file authorities.xml -insert -update -c=MARC21 -keepids="024a" -match "heading-main,100a" -l "/tmp/import.log" -yaml="/tmp/ids.yml" 7) Go to the authorities page and find the imported authority by for example search for a 400 or 100 field value. Verify that the 001 field of the incoming record has been moved to 024a. Also check that the value of 024a appear as a key in /tmp/ids.yml. 8) Edit the authorities.xml file and change the value of field 400a 9) Import authorities again using the same options 10) Verify that the 400a field of the authority in Koha has not been updated (the import was skippped since revision did not change) 11) Edit authorities.xml, replace the value of field 005 in the file with field 005 of the authority in Koha incremented by one 12) Import again with the same options 13) Verify that the 400a field of the authority in Koha now have been updated
Created attachment 127960 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
Created attachment 127961 [details] authorities.xml
Created attachment 127963 [details] biblios.mrc
Nice work David.. On with the review: 1. You appear to introduce a few new modules, but I'm not seeing some of them actually used. [List::MoreUtils qw(any), Koha::Plugins::Handler;] 2. With such an extensive tidy up.. I'd probably go ahead and run it through perltidy as a final patch.. (I did this on the before and after script to make comparing simpler) Otherwise, this is generally looking very reasonable to me. I'm going to run a few more test runs against it and ask our migrations team to have a play
Created attachment 128086 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl To test: 1) Import biblios using ./bulkmarcimport.pl -v -b -file biblios.mrc -insert -update -c=MARC21 -match "control-number,001" -n=1 -l "/tmp/import.log" 2) Verify in /tmp/import.log that one biblio has been inserted 3) Import again without the "-n" option, verify that one biblio has been updated and the rest inserted 4) In the staff interface search the catalog for some string appearing in the imported biblios to verify records have been indexed 5) View/edit some of the biblios the staff interface to very have been properly imported 6) Import authorities using ./bulkmarcimport.pl -v -m=MARCXML -a -file authorities.xml -insert -update -c=MARC21 -keepids="024a" -match "heading-main,100a" -l "/tmp/import.log" -yaml="/tmp/ids.yml" 7) Go to the authorities page and find the imported authority by for example search for a 400 or 100 field value. Verify that the 001 field of the incoming record has been moved to 024a. Also check that the value of 024a appear as a key in /tmp/ids.yml. 8) Edit the authorities.xml file and change the value of field 400a 9) Import authorities again using the same options 10) Verify that the 400a field of the authority in Koha has not been updated (the import was skippped since revision did not change) 11) Edit authorities.xml, replace the value of field 005 in the file with field 005 of the authority in Koha incremented by one 12) Import again with the same options 13) Verify that the 400a field of the authority in Koha now have been updated
Created attachment 128087 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case.
(In reply to Martin Renvoize from comment #23) > Nice work David.. > > On with the review: > 1. You appear to introduce a few new modules, but I'm not seeing some of > them actually used. [List::MoreUtils qw(any), Koha::Plugins::Handler;] > 2. With such an extensive tidy up.. I'd probably go ahead and run it through > perltidy as a final patch.. (I did this on the before and after script to > make comparing simpler) > > Otherwise, this is generally looking very reasonable to me. > > I'm going to run a few more test runs against it and ask our migrations team > to have a play Thanks. Yes those imports where part of add features I removed (but forgot to remove the imports). I'm not a regular user of perltidy, but for me the output-file is the same as the original file. Perhaps I'm doing something wrong.
Discovered looks like there is encoding issues with "biblios.mrc", double encoded most likely, I suspect the file was re-encoded either on download/upload as the original file correctly encoded. If requested I can upload another file and tar it beforehand.
Created attachment 128362 [details] biblios.marc.tar.gz
Created attachment 128890 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl To test: 1) Import biblios using ./bulkmarcimport.pl -v -b -file biblios.mrc -insert -update -c=MARC21 -match "control-number,001" -n=1 -l "/tmp/import.log" 2) Verify in /tmp/import.log that one biblio has been inserted 3) Import again without the "-n" option, verify that one biblio has been updated and the rest inserted 4) In the staff interface search the catalog for some string appearing in the imported biblios to verify records have been indexed 5) View/edit some of the biblios the staff interface to very have been properly imported 6) Import authorities using ./bulkmarcimport.pl -v -m=MARCXML -a -file authorities.xml -insert -update -c=MARC21 -keepids="024a" -match "heading-main,100a" -l "/tmp/import.log" -yaml="/tmp/ids.yml" 7) Go to the authorities page and find the imported authority by for example search for a 400 or 100 field value. Verify that the 001 field of the incoming record has been moved to 024a. Also check that the value of 024a appear as a key in /tmp/ids.yml. 8) Edit the authorities.xml file and change the value of field 400a 9) Import authorities again using the same options 10) Verify that the 400a field of the authority in Koha has not been updated (the import was skippped since revision did not change) 11) Edit authorities.xml, replace the value of field 005 in the file with field 005 of the authority in Koha incremented by one 12) Import again with the same options 13) Verify that the 400a field of the authority in Koha now have been updated
Created attachment 128891 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case. Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se> The patch works fine for me. Signing off.
Created attachment 131176 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl To test: 1) Import biblios using ./bulkmarcimport.pl -v -b -file biblios.mrc -insert -update -c=MARC21 -match "control-number,001" -n=1 -l "/tmp/import.log" 2) Verify in /tmp/import.log that one biblio has been inserted 3) Import again without the "-n" option, verify that one biblio has been updated and the rest inserted 4) In the staff interface search the catalog for some string appearing in the imported biblios to verify records have been indexed 5) View/edit some of the biblios the staff interface to very have been properly imported 6) Import authorities using ./bulkmarcimport.pl -v -m=MARCXML -a -file authorities.xml -insert -update -c=MARC21 -keepids="024a" -match "heading-main,100a" -l "/tmp/import.log" -yaml="/tmp/ids.yml" 7) Go to the authorities page and find the imported authority by for example search for a 400 or 100 field value. Verify that the 001 field of the incoming record has been moved to 024a. Also check that the value of 024a appear as a key in /tmp/ids.yml. 8) Edit the authorities.xml file and change the value of field 400a 9) Import authorities again using the same options 10) Verify that the 400a field of the authority in Koha has not been updated (the import was skippped since revision did not change) 11) Edit authorities.xml, replace the value of field 005 in the file with field 005 of the authority in Koha incremented by one 12) Import again with the same options 13) Verify that the 400a field of the authority in Koha now have been updated
Created attachment 131177 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case. Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se> The patch works fine for me. Signing off.
Rebased against master
Created attachment 131180 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl To test: 1) Import biblios using ./bulkmarcimport.pl -v -b -file biblios.mrc -insert -update -c=MARC21 -match "control-number,001" -n=1 -l "/tmp/import.log" 2) Verify in /tmp/import.log that one biblio has been inserted 3) Import again without the "-n" option, verify that one biblio has been updated and the rest inserted 4) In the staff interface search the catalog for some string appearing in the imported biblios to verify records have been indexed 5) View/edit some of the biblios the staff interface to very have been properly imported 6) Import authorities using ./bulkmarcimport.pl -v -m=MARCXML -a -file authorities.xml -insert -update -c=MARC21 -keepids="024a" -match "heading-main,100a" -l "/tmp/import.log" -yaml="/tmp/ids.yml" 7) Go to the authorities page and find the imported authority by for example search for a 400 or 100 field value. Verify that the 001 field of the incoming record has been moved to 024a. Also check that the value of 024a appear as a key in /tmp/ids.yml. 8) Edit the authorities.xml file and change the value of field 400a 9) Import authorities again using the same options 10) Verify that the 400a field of the authority in Koha has not been updated (the import was skippped since revision did not change) 11) Edit authorities.xml, replace the value of field 005 in the file with field 005 of the authority in Koha incremented by one 12) Import again with the same options 13) Verify that the 400a field of the authority in Koha now have been updated
Created attachment 131181 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios The current behavior for when importing authorities, when match option is enabled, is to insert as a new authority fetching from db fails. This is both a bit confusing and not consistent with the biblio case where an error is generated. Also clean up error and log messages so both messages/errors and when messages/errors are issued are more consistent for the biblio and authority case. Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se> The patch works fine for me. Signing off.
Test plan works, some failures from QA tool: Processing files after patches |========================>| 2 / 2 (100.00%) OK C4/Biblio.pm FAIL misc/migration_tools/bulkmarcimport.pl FAIL critic # Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s). FAIL spelling unkown ==> unknown The move of ES indexing is very nice. I am leaving this for another pair of eyes as I am not confident I understand this whole routine well enough.
Created attachment 132942 [details] [review] Bug 29440: Fix QA-tool issues