Bug 29440 - Refactor/clean up bulkmarcimport.pl
Summary: Refactor/clean up bulkmarcimport.pl
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25539 29597 29654
  Show dependency treegraph
 
Reported: 2021-11-09 12:29 UTC by David Gustafsson
Modified: 2021-12-08 12:56 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-09 12:45 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.89 KB, patch)
2021-11-09 12:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-09 12:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (45.31 KB, patch)
2021-11-09 13:21 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-09 13:21 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (45.15 KB, patch)
2021-11-10 11:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-10 11:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (45.10 KB, patch)
2021-11-15 13:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-15 13:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (45.12 KB, patch)
2021-11-18 12:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-18 12:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (45.12 KB, patch)
2021-11-18 13:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.50 KB, patch)
2021-11-18 13:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.43 KB, patch)
2021-11-23 15:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.85 KB, patch)
2021-11-23 15:53 UTC, David Gustafsson
Details | Diff | Splinter Review
authorities.xml (1.33 KB, application/xml)
2021-11-23 15:56 UTC, David Gustafsson
Details
biblios.mrc (14.21 KB, application/octet-stream)
2021-11-23 15:58 UTC, David Gustafsson
Details
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.21 KB, patch)
2021-11-29 18:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.85 KB, patch)
2021-11-29 18:32 UTC, David Gustafsson
Details | Diff | Splinter Review
biblios.marc.tar.gz (4.24 KB, application/gzip)
2021-12-08 12:56 UTC, David Gustafsson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2021-11-09 12:29:27 UTC
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).
Comment 1 David Gustafsson 2021-11-09 12:45:09 UTC
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.
Comment 2 David Gustafsson 2021-11-09 12:46:41 UTC
Created attachment 127480 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 3 David Gustafsson 2021-11-09 12:46:45 UTC
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.
Comment 4 David Gustafsson 2021-11-09 12:57:31 UTC
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.
Comment 5 David Gustafsson 2021-11-09 13:21:07 UTC
Created attachment 127485 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 6 David Gustafsson 2021-11-09 13:21:10 UTC
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.
Comment 7 Katrin Fischer 2021-11-10 08:47:50 UTC
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!
Comment 8 David Gustafsson 2021-11-10 11:15:38 UTC
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.
Comment 9 David Gustafsson 2021-11-10 11:17:23 UTC
Created attachment 127508 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 10 David Gustafsson 2021-11-10 11:17:27 UTC
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.
Comment 11 David Gustafsson 2021-11-15 13:59:37 UTC
Created attachment 127651 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 12 David Gustafsson 2021-11-15 13:59:41 UTC
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.
Comment 13 David Gustafsson 2021-11-18 12:52:07 UTC
Created attachment 127791 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 14 David Gustafsson 2021-11-18 12:52:12 UTC
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.
Comment 15 David Gustafsson 2021-11-18 13:08:50 UTC
Created attachment 127795 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 16 David Gustafsson 2021-11-18 13:08:54 UTC
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.
Comment 17 Katrin Fischer 2021-11-18 13:40:53 UTC
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?
Comment 18 David Gustafsson 2021-11-22 13:50:28 UTC
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.
Comment 19 David Gustafsson 2021-11-23 15:52:56 UTC
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
Comment 20 David Gustafsson 2021-11-23 15:53:02 UTC
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.
Comment 21 David Gustafsson 2021-11-23 15:56:29 UTC
Created attachment 127961 [details]
authorities.xml
Comment 22 David Gustafsson 2021-11-23 15:58:01 UTC
Created attachment 127963 [details]
biblios.mrc
Comment 23 Martin Renvoize 2021-11-26 16:06:49 UTC
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
Comment 24 David Gustafsson 2021-11-29 18:32:21 UTC
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
Comment 25 David Gustafsson 2021-11-29 18:32:24 UTC
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.
Comment 26 David Gustafsson 2021-11-29 18:46:24 UTC
(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.
Comment 27 David Gustafsson 2021-12-02 18:35:42 UTC
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.
Comment 28 David Gustafsson 2021-12-08 12:56:12 UTC
Created attachment 128362 [details]
biblios.marc.tar.gz