Bug 29440 - Refactor/clean up bulkmarcimport.pl
Summary: Refactor/clean up bulkmarcimport.pl
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 30822
Blocks: 25539 29597 29654
  Show dependency treegraph
 
Reported: 2021-11-09 12:29 UTC by David Gustafsson
Modified: 2024-02-23 07:03 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
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
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.21 KB, patch)
2021-12-23 15:45 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.95 KB, patch)
2021-12-23 15:45 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.23 KB, patch)
2022-03-01 14:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.95 KB, patch)
2022-03-01 14:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.21 KB, patch)
2022-03-01 14:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.95 KB, patch)
2022-03-01 14:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Fix QA-tool issues (1.24 KB, patch)
2022-04-04 14:41 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (5.48 KB, patch)
2022-09-21 15:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (5.50 KB, patch)
2022-09-21 15:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (47.21 KB, patch)
2022-09-23 13:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.95 KB, patch)
2022-09-23 13:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Fix QA-tool issues (1.24 KB, patch)
2022-09-23 13:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (4.07 KB, patch)
2022-09-23 13:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (47.21 KB, patch)
2022-09-23 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.95 KB, patch)
2022-09-23 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Fix QA-tool issues (1.24 KB, patch)
2022-09-23 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (4.08 KB, patch)
2022-09-23 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: replace usage of removed GetMarcBiblio (1.45 KB, patch)
2022-09-23 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Remove pointless call to _strip_item_fields (1.56 KB, patch)
2022-09-26 14:56 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options (2.81 KB, patch)
2022-09-26 14:56 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion (2.92 KB, patch)
2022-09-26 14:56 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios (1.67 KB, patch)
2022-11-07 16:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.76 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (7.95 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Fix QA-tool issues (1.24 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (4.05 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: replace usage of removed GetMarcBiblio (1.45 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Remove pointless call to _strip_item_fields (1.56 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options (2.81 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion (2.92 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios (1.68 KB, patch)
2023-05-16 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Fix AddBiblio/ModBiblio options args (1.67 KB, patch)
2023-10-25 12:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (47.96 KB, patch)
2023-12-20 18:25 UTC, M
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (8.19 KB, patch)
2023-12-20 18:26 UTC, M
Details | Diff | Splinter Review
Bug 29440: Fix QA-tool issues (1.36 KB, patch)
2023-12-20 18:26 UTC, M
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (4.24 KB, patch)
2023-12-20 18:27 UTC, M
Details | Diff | Splinter Review
Bug 29440: replace usage of removed GetMarcBiblio (1.57 KB, patch)
2023-12-20 18:27 UTC, M
Details | Diff | Splinter Review
Bug 29440: Remove pointless call to _strip_item_fields (1.68 KB, patch)
2023-12-20 18:28 UTC, M
Details | Diff | Splinter Review
Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options (2.95 KB, patch)
2023-12-20 18:28 UTC, M
Details | Diff | Splinter Review
Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion (3.07 KB, patch)
2023-12-20 18:28 UTC, M
Details | Diff | Splinter Review
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios (1.80 KB, patch)
2023-12-20 18:29 UTC, M
Details | Diff | Splinter Review
Bug 29440: Fix AddBiblio/ModBiblio options args (1.80 KB, patch)
2023-12-20 18:30 UTC, M
Details | Diff | Splinter Review
Bug 29440: Refactor and clean up bulkmarcimport.pl (46.98 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Make authorities import behavior consistent with biblios (8.13 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Fix QA-tool issues (1.46 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: defer biblio autolinking until records are indexed (4.27 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: replace usage of removed GetMarcBiblio (1.67 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Remove pointless call to _strip_item_fields (1.78 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options (3.03 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Only log items has been inserted if actually have been so, and remove extra out of place logging of record insertion (3.14 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios (1.90 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: Fix AddBiblio/ModBiblio options args (1.89 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29440: (QA follow-up) Fix tidy (1.27 KB, patch)
2024-01-03 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review

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
Comment 29 Frank Hansen 2021-12-23 15:45:37 UTC
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
Comment 30 Frank Hansen 2021-12-23 15:45:41 UTC
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.
Comment 31 David Gustafsson 2022-03-01 14:25:20 UTC
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
Comment 32 David Gustafsson 2022-03-01 14:25:24 UTC
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.
Comment 33 David Gustafsson 2022-03-01 14:38:17 UTC
Rebased against master
Comment 34 David Gustafsson 2022-03-01 14:52:07 UTC
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
Comment 35 David Gustafsson 2022-03-01 14:52:11 UTC
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.
Comment 36 Nick Clemens 2022-03-31 15:56:44 UTC
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.
Comment 37 David Gustafsson 2022-04-04 14:41:20 UTC
Created attachment 132942 [details] [review]
Bug 29440: Fix QA-tool issues
Comment 38 Martin Renvoize 2022-05-20 11:44:32 UTC
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.
Comment 39 Katrin Fischer 2022-06-25 16:36:46 UTC
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
Comment 40 David Gustafsson 2022-09-21 15:44:12 UTC
Created attachment 140822 [details] [review]
Bug 29440: defer biblio autolinking until records are indexed
Comment 41 David Gustafsson 2022-09-21 15:49:01 UTC
Created attachment 140823 [details] [review]
Bug 29440: defer biblio autolinking until records are indexed
Comment 42 David Gustafsson 2022-09-21 16:04:20 UTC
(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.
Comment 43 David Gustafsson 2022-09-23 13:25:32 UTC
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
Comment 44 David Gustafsson 2022-09-23 13:25:37 UTC
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.
Comment 45 David Gustafsson 2022-09-23 13:25:42 UTC
Created attachment 140905 [details] [review]
Bug 29440: Fix QA-tool issues
Comment 46 David Gustafsson 2022-09-23 13:25:47 UTC
Created attachment 140906 [details] [review]
Bug 29440: defer biblio autolinking until records are indexed
Comment 47 David Gustafsson 2022-09-23 13:31:04 UTC
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.
Comment 48 David Gustafsson 2022-09-23 13:31:47 UTC
Typo: Should have been "Have *now* rebased against master".
Comment 49 David Gustafsson 2022-09-23 15:09:38 UTC
Discovered some issues after rebase so setting to failed until these are fixed (will probably have to wait until Monday).
Comment 50 David Gustafsson 2022-09-23 15:33:29 UTC
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
Comment 51 David Gustafsson 2022-09-23 15:33:34 UTC
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.
Comment 52 David Gustafsson 2022-09-23 15:33:41 UTC
Created attachment 140926 [details] [review]
Bug 29440: Fix QA-tool issues
Comment 53 David Gustafsson 2022-09-23 15:33:46 UTC
Created attachment 140927 [details] [review]
Bug 29440: defer biblio autolinking until records are indexed
Comment 54 David Gustafsson 2022-09-23 15:33:51 UTC
Created attachment 140928 [details] [review]
Bug 29440: replace usage of removed GetMarcBiblio
Comment 55 David Gustafsson 2022-09-23 15:35:27 UTC
Fixed the issues, but will test some more next week before ready for signoff.
Comment 56 David Gustafsson 2022-09-26 14:56:32 UTC
Created attachment 140987 [details] [review]
Bug 29440: Remove pointless call to _strip_item_fields
Comment 57 David Gustafsson 2022-09-26 14:56:37 UTC
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
Comment 58 David Gustafsson 2022-09-26 14:56:42 UTC
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
Comment 59 David Gustafsson 2022-11-07 16:59:20 UTC
Created attachment 143375 [details] [review]
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios
Comment 60 Magnus Enger 2023-03-27 10:35:14 UTC
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
Comment 61 David Gustafsson 2023-05-16 14:44:13 UTC
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
Comment 62 David Gustafsson 2023-05-16 14:44:16 UTC
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.
Comment 63 David Gustafsson 2023-05-16 14:44:19 UTC
Created attachment 151295 [details] [review]
Bug 29440: Fix QA-tool issues
Comment 64 David Gustafsson 2023-05-16 14:44:22 UTC
Created attachment 151296 [details] [review]
Bug 29440: defer biblio autolinking until records are indexed
Comment 65 David Gustafsson 2023-05-16 14:44:25 UTC
Created attachment 151297 [details] [review]
Bug 29440: replace usage of removed GetMarcBiblio
Comment 66 David Gustafsson 2023-05-16 14:44:28 UTC
Created attachment 151298 [details] [review]
Bug 29440: Remove pointless call to _strip_item_fields
Comment 67 David Gustafsson 2023-05-16 14:44:31 UTC
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
Comment 68 David Gustafsson 2023-05-16 14:44:34 UTC
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
Comment 69 David Gustafsson 2023-05-16 14:44:37 UTC
Created attachment 151301 [details] [review]
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios
Comment 70 David Gustafsson 2023-05-16 14:44:55 UTC
Rebased against master
Comment 71 David Gustafsson 2023-10-25 12:44:09 UTC
Created attachment 157798 [details] [review]
Bug 29440: Fix AddBiblio/ModBiblio options args
Comment 72 M 2023-11-08 12:53:31 UTC
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).
Comment 73 Katrin Fischer 2023-11-09 21:05:43 UTC
(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.
Comment 74 M 2023-12-20 18:25:28 UTC
Created attachment 160148 [details] [review]
Bug 29440: Refactor and clean up bulkmarcimport.pl
Comment 75 M 2023-12-20 18:26:24 UTC
Created attachment 160149 [details] [review]
Bug 29440: Make authorities import behavior consistent with biblios
Comment 76 M 2023-12-20 18:26:47 UTC
Created attachment 160150 [details] [review]
Bug 29440: Fix QA-tool issues
Comment 77 M 2023-12-20 18:27:16 UTC
Created attachment 160151 [details] [review]
Bug 29440: defer biblio autolinking until records are indexed
Comment 78 M 2023-12-20 18:27:43 UTC
Created attachment 160152 [details] [review]
Bug 29440: replace usage of removed GetMarcBiblio
Comment 79 M 2023-12-20 18:28:07 UTC
Created attachment 160153 [details] [review]
Bug 29440: Remove pointless call to _strip_item_fields
Comment 80 M 2023-12-20 18:28:38 UTC
Created attachment 160154 [details] [review]
Bug 29440: Rename $modify_biblio_marc_options to $mod_bilbio_options
Comment 81 M 2023-12-20 18:28:59 UTC
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
Comment 82 M 2023-12-20 18:29:35 UTC
Created attachment 160156 [details] [review]
Bug 29440: Replace removed syspref BiblioAddsAuthorities with AutoLinkBiblios
Comment 83 M 2023-12-20 18:30:01 UTC
Created attachment 160157 [details] [review]
Bug 29440: Fix AddBiblio/ModBiblio options args
Comment 84 M 2023-12-20 18:31:23 UTC
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.
Comment 85 M 2023-12-20 18:53:21 UTC
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)
Comment 86 Martin Renvoize 2023-12-20 20:00:21 UTC
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.
Comment 87 M 2023-12-21 02:57:54 UTC
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...
Comment 88 Martin Renvoize 2024-01-03 16:25:03 UTC
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>
Comment 89 Martin Renvoize 2024-01-03 16:25:06 UTC
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>
Comment 90 Martin Renvoize 2024-01-03 16:25:10 UTC
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>
Comment 91 Martin Renvoize 2024-01-03 16:25:13 UTC
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>
Comment 92 Martin Renvoize 2024-01-03 16:25:16 UTC
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>
Comment 93 Martin Renvoize 2024-01-03 16:25:20 UTC
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>
Comment 94 Martin Renvoize 2024-01-03 16:25:23 UTC
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>
Comment 95 Martin Renvoize 2024-01-03 16:25:26 UTC
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>
Comment 96 Martin Renvoize 2024-01-03 16:25:30 UTC
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>
Comment 97 Martin Renvoize 2024-01-03 16:25:34 UTC
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>
Comment 98 Martin Renvoize 2024-01-03 16:25:37 UTC
Created attachment 160487 [details] [review]
Bug 29440: (QA follow-up) Fix tidy
Comment 99 Martin Renvoize 2024-01-03 16:25:59 UTC
Happy new year.. Passing QA at last.