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
Looking good so far.. a couple of comments We have a fairly standard 'skip_index' name for the search engine index skip param.. we should stick to that here too (I bet your patch pre-dates that, but it shouldn't be too onerous to update for consistency) However, there's a more challenging thing to resolve. If 'BiblioAddsAuthorities' is enabled the index skip is going to cause issues as the linker relies of updated indexes to do it's job... we possibly need to move where the BiblioAddsAuthories code runs for such cases to run at the end after a first run of indexing has taken place and then run a second index run to catch the link changes it makes? Conundrum.
Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 29440: Refactor and clean up bulkmarcimport.pl Using index info to reconstruct a base tree... M C4/Biblio.pm Falling back to patching base and 3-way merge... Auto-merging C4/Biblio.pm CONFLICT (content): Merge conflict in C4/Biblio.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 29440: Refactor and clean up bulkmarcimport.pl hint: Use 'git am --show-current-patch=diff' to see the failed 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-29440-Refactor-and-clean-up-bulkmarcimportpl-uQYeFb.patch
Created attachment 140822 [details] [review] Bug 29440: defer biblio autolinking until records are indexed
Created attachment 140823 [details] [review] Bug 29440: defer biblio autolinking until records are indexed
(In reply to Martin Renvoize from comment #38) > Looking good so far.. a couple of comments > > We have a fairly standard 'skip_index' name for the search engine index skip > param.. we should stick to that here too (I bet your patch pre-dates that, > but it shouldn't be too onerous to update for consistency) > > However, there's a more challenging thing to resolve. > > If 'BiblioAddsAuthorities' is enabled the index skip is going to cause > issues as the linker relies of updated indexes to do it's job... we possibly > need to move where the BiblioAddsAuthories code runs for such cases to run > at the end after a first run of indexing has taken place and then run a > second index run to catch the link changes it makes? > > Conundrum. Good catch. I'm a little bit confused about the first point. I only see skip_index being used in rebuild_zebra.pl and not really sure what it does. bulkmarkimport.pl will always defer indexing (but only for elasticsearch) and bulk-index for each batch. There is not option to disable this as there is no apparent downside not to do this. The second issue should now be fixed, it turned out to be pretty straightforward as only needed to add the option to skip linking for AddBiblio (the option was already present in ModBiblio), and then run BiblioAutoLink on all records in the batch after indexing them successfully (if I'm not missing anything). The patch is still not rebased but will now have a look at that.
Created attachment 140903 [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 140904 [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 140905 [details] [review] Bug 29440: Fix QA-tool issues
Created attachment 140906 [details] [review] Bug 29440: defer biblio autolinking until records are indexed
Have not rebased against latest master. @Martin Renvoize while rebasing I also understood what was referring to about the "skip_index" have changed the option name accordingly. Will perform some more testing in case some regression has been introduced during rebase.
Typo: Should have been "Have *now* rebased against master".
Discovered some issues after rebase so setting to failed until these are fixed (will probably have to wait until Monday).
Created attachment 140924 [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 140925 [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 140926 [details] [review] Bug 29440: Fix QA-tool issues
Created attachment 140927 [details] [review] Bug 29440: defer biblio autolinking until records are indexed
Created attachment 140928 [details] [review] Bug 29440: replace usage of removed GetMarcBiblio
Fixed the issues, but will test some more next week before ready for signoff.
Created attachment 140987 [details] [review] Bug 29440: Remove pointless call to _strip_item_fields
Created attachment 140988 [details] [review] Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options This variable actually contains ModBibio options and ModifyBiblioMarc accepts a subset of these
Created attachment 140989 [details] [review] Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion
Created attachment 143375 [details] [review] Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios
Sorry, patch does not apply on current master: $ git bz apply 29440 Bug 29440 - Refactor/clean up bulkmarcimport.pl 140924 - Bug 29440: Refactor and clean up bulkmarcimport.pl 140925 - Bug 29440: Make authorities import behavior consistent with biblios 140926 - Bug 29440: Fix QA-tool issues 140927 - Bug 29440: defer biblio autolinking until records are indexed 140928 - Bug 29440: replace usage of removed GetMarcBiblio 140987 - Bug 29440: Remove pointless call to _strip_item_fields 140988 - Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options 140989 - Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion 143375 - Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 29440: Refactor and clean up bulkmarcimport.pl Using index info to reconstruct a base tree... M C4/Biblio.pm Falling back to patching base and 3-way merge... Auto-merging C4/Biblio.pm CONFLICT (content): Merge conflict in C4/Biblio.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 29440: Refactor and clean up bulkmarcimport.pl hint: Use 'git am --show-current-patch=diff' to see the failed 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-29440-Refactor-and-clean-up-bulkmarcimportpl-i5itTa.patch
Created attachment 151293 [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 151294 [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 151295 [details] [review] Bug 29440: Fix QA-tool issues
Created attachment 151296 [details] [review] Bug 29440: defer biblio autolinking until records are indexed
Created attachment 151297 [details] [review] Bug 29440: replace usage of removed GetMarcBiblio
Created attachment 151298 [details] [review] Bug 29440: Remove pointless call to _strip_item_fields
Created attachment 151299 [details] [review] Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options This variable actually contains ModBibio options and ModifyBiblioMarc accepts a subset of these
Created attachment 151300 [details] [review] Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion
Created attachment 151301 [details] [review] Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios
Created attachment 157798 [details] [review] Bug 29440: Fix AddBiblio/ModBiblio options args
I rebased this patch and Bug 25539 in our library and successfully used it to update a lot of biblio and auth records. My feedback is that it generally works, and marc overlay rules are also applied. There's one biggest issue I've ran into: I've had issues with the -match option with auths. For biblios, I could use `-match Other-control-number,035a` perfectly fine. But for auths, using `-match LC-card-number,010a` just didn't work, so I had to resort to `-match Any,010a`. It works, but not really ideal. After playing with yaz-client and other tools, I can say this wasn't really indexing issue it seems. I've also ran: > xsltproc /etc/koha/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl <(sudo koha-mysql biblioteka <<< "select marcxml from auth_header where ExtractValue( marcxml, '//datafield[@tag=010]/subfield[@code=\"a\"]' ) != '' limit 1\\G" | sed -n 's/marcxml: //;2,$p' ) | grep 2005134806 > <z:index name="LC-card-number:w LC-card-number:p">n 2005134806</z:index> > <z:index name="Any:w Any:p">n 2005134806</z:index> > <z:index name="Any:w Any:p">http://nukat.edu.pl/aut/n 2005134806</z:index> It seems that the control field is indexed as "LC-card-number", but using it with the scripts always yielded no matches, and with "-update" a warning that insertion of new record was skipped. From my memory of debugging this a few days ago, the $query variable had a value of `LC-card-number="n 2005134806"`. I also have two more tiny feedback items: - In the if commented as `#Skip if authority in database is the same or newer than the incoming record`, it skips the record with `next;` with no feedback in logs, neither warn, nor entry in the log like "skipped" similar to "updated"/"inserted". I think it might be a good idea to add some log line here. - In the final logs it says `n MARC records done in x seconds`. The `n` stands for number of all records that were checked from the file, including those that were skipped. I think it'd be nice if it said something like `n/n MARC records done`, where first `n` would be the number of records actually inserted or updated into the database, while the second `n` would remain the current number. This improvement alone would also partially rectify the problem presented with the first suggestion above. All in all though it's a very nice changeset, we appreciate that it can load biblio records while respecting MARC overlay rules, it already works for us pretty much as-is, so it'd be nice if it could be merged to 23.11 version already, and maybe tiny details improved later on with time, without blocking this. The way I see it, this changeset already makes this script MUCH better, fixes broken behavior in it, and doesn't make anything more broken than it already was before, so I definitely vouch for it... Not sure if I have any authority to review code or make a sign-off, but we used it to import/update thousands of biblio and auth records now, without noticing any issues (I paid special attention to MARC export of biblio before/after).
(In reply to M from comment #72) > I rebased this patch and Bug 25539 in our library and successfully used it > to update a lot of biblio and auth records. My feedback is that it generally > works, and marc overlay rules are also applied. > > There's one biggest issue I've ran into: I've had issues with the -match > option with auths. For biblios, I could use `-match > Other-control-number,035a` perfectly fine. But for auths, using `-match > LC-card-number,010a` just didn't work, so I had to resort to `-match > Any,010a`. It works, but not really ideal. > > After playing with yaz-client and other tools, I can say this wasn't really > indexing issue it seems. I've also ran: > > xsltproc /etc/koha/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl <(sudo koha-mysql biblioteka <<< "select marcxml from auth_header where ExtractValue( marcxml, '//datafield[@tag=010]/subfield[@code=\"a\"]' ) != '' limit 1\\G" | sed -n 's/marcxml: //;2,$p' ) | grep 2005134806 > > <z:index name="LC-card-number:w LC-card-number:p">n 2005134806</z:index> > > <z:index name="Any:w Any:p">n 2005134806</z:index> > > <z:index name="Any:w Any:p">http://nukat.edu.pl/aut/n 2005134806</z:index> > > It seems that the control field is indexed as "LC-card-number", but using it > with the scripts always yielded no matches, and with "-update" a warning > that insertion of new record was skipped. From my memory of debugging this a > few days ago, the $query variable had a value of `LC-card-number="n > 2005134806"`. > > I also have two more tiny feedback items: > > - In the if commented as `#Skip if authority in database is the same or > newer than the incoming record`, it skips the record with `next;` with no > feedback in logs, neither warn, nor entry in the log like "skipped" similar > to "updated"/"inserted". I think it might be a good idea to add some log > line here. > > - In the final logs it says `n MARC records done in x seconds`. The `n` > stands for number of all records that were checked from the file, including > those that were skipped. I think it'd be nice if it said something like `n/n > MARC records done`, where first `n` would be the number of records actually > inserted or updated into the database, while the second `n` would remain the > current number. > This improvement alone would also partially rectify the problem presented > with the first suggestion above. > > All in all though it's a very nice changeset, we appreciate that it can load > biblio records while respecting MARC overlay rules, it already works for us > pretty much as-is, so it'd be nice if it could be merged to 23.11 version > already, and maybe tiny details improved later on with time, without > blocking this. > > The way I see it, this changeset already makes this script MUCH better, > fixes broken behavior in it, and doesn't make anything more broken than it > already was before, so I definitely vouch for it... > > Not sure if I have any authority to review code or make a sign-off, but we > used it to import/update thousands of biblio and auth records now, without > noticing any issues (I paid special attention to MARC export of biblio > before/after). It sounds like you should sign-off on this then. Do you know how to do it? You can use sandboxes or if you use ktd use the git so command. If you need help, please let us know.
Created attachment 160148 [details] [review] Bug 29440: Refactor and clean up bulkmarcimport.pl
Created attachment 160149 [details] [review] Bug 29440: Make authorities import behavior consistent with biblios
Created attachment 160150 [details] [review] Bug 29440: Fix QA-tool issues
Created attachment 160151 [details] [review] Bug 29440: defer biblio autolinking until records are indexed
Created attachment 160152 [details] [review] Bug 29440: replace usage of removed GetMarcBiblio
Created attachment 160153 [details] [review] Bug 29440: Remove pointless call to _strip_item_fields
Created attachment 160154 [details] [review] Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options
Created attachment 160155 [details] [review] Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion
Created attachment 160156 [details] [review] Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios
Created attachment 160157 [details] [review] Bug 29440: Fix AddBiblio/ModBiblio options args
I've signed off on all patches after reviewing them. They improve the situation over what's currently in master, fix inconsistent behavior and don't seem to regress on anything.
I also signed off Bug 25539, and I can suggest testing them together for the final fixed behavior of this script... (my personal tests were done with both applied) I noticed some additional issues during my review (existing ones, not regressions!), but those can be dealt with in some new bug later on (after these two are merged to master, to prevent even bigger dependency mess)
Could you detail the issues you found in some new bugs and link them to this please? It would help me confidently QA this long standing improvement.
I plan to make such a bug soon where I'd list more of them, but the most important functional ones are: * if more than one match is found with -match option, it won't skip or update all such records, but rather create yet another duplicate (which I think is wrong, as they can keep multiplying forever) * matching but skipped records aren't logged, but I think they should be (it currently skips updating authority records if their date indicates that they're equal or newer in the Koha instance already) Others are just some rather minor imperfections in the code, but the above may matter for QA. On top of that, the issue with `-match LC-card-number,010a` for authorities remains, but I believe it's an issue with Zebra rather than this script, as I think I had the same problem while trying to debug the search with yaz-client, although not 100% sure (also that can be mitigated with Zebra with Any index, but this sometimes created an issue of multiple matches for us, so that's not a great workaround). The issue isn't present at all after switching to Elasticsearch though...
Created attachment 160477 [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 Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160478 [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> Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160479 [details] [review] Bug 29440: Fix QA-tool issues Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160480 [details] [review] Bug 29440: defer biblio autolinking until records are indexed Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160481 [details] [review] Bug 29440: replace usage of removed GetMarcBiblio Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160482 [details] [review] Bug 29440: Remove pointless call to _strip_item_fields Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160483 [details] [review] Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options This variable actually contains ModBibio options and ModifyBiblioMarc accepts a subset of these Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160484 [details] [review] Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160485 [details] [review] Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160486 [details] [review] Bug 29440: Fix AddBiblio/ModBiblio options args Signed-off-by: Michał Kula <148193449+mkibp@users.noreply.github.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 160487 [details] [review] Bug 29440: (QA follow-up) Fix tidy
Happy new year.. Passing QA at last.
This still applies, but please note that I am very suspicious of any patch sets labelled "enhancements" "improvements" or other general terms, as I usually suspect they would be much easier to manage as a series of bugs with smaller changes instead. 1) Please unit tests for disable_autolink in AddBiblio 2) Please add a summary of the changes to the "Text to go in the release notes" field here on the bug I realize this has been sitting a while and I a not going to block it any longer, but please follow-up on these requests!
Pushed for 24.05! Well done everyone, thank you!
Enhancement not pushed to 23.11.x
(In reply to Katrin Fischer from comment #100) > This still applies, but please note that I am very suspicious of any patch > sets labelled "enhancements" "improvements" or other general terms, as I > usually suspect they would be much easier to manage as a series of bugs with > smaller changes instead. > > 1) Please unit tests for disable_autolink in AddBiblio > 2) Please add a summary of the changes to the "Text to go in the release > notes" field here on the bug > > I realize this has been sitting a while and I a not going to block it any > longer, but please follow-up on these requests! *ping*
It looks like this might have unintentionally caused issues with big files (bug 37020). This is a big issue as it will hinder migrations. Can you please take a look there?
(In reply to Katrin Fischer from comment #104) > It looks like this might have unintentionally caused issues with big files > (bug 37020). This is a big issue as it will hinder migrations. Can you > please take a look there? Never got any reply here :( But we have the other bug. Removing keyword.
True, I still have it in my backlog to poke around that script sometime, but an opportunity could come within 2-3 months if we'll be refactoring our import scripts... (I also had the idea in my head to add some second simpler script to manipulate/swap just a single MARC record with external tooling/scripts)