Bug 18389

Summary: Allow using MARC modification templates in bulkmarcimport.pl
Product: Koha Reporter: Jon Knight <J.P.Knight>
Component: MARC Bibliographic record staging/importAssignee: Jon Knight <J.P.Knight>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: J.P.Knight, jonathan.druart, katrin.fischer, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Patch for bulkmarcimport.pl
The same patch as above only generated within git and without touch existing whitespace formatting oddities in the script.
Additional patch to remove use strict/warn and add POD
Revised patch with commit information reformatted.
[SIGNED OFF] Bug 18690: Use MARC modification templates with bulkmarcimport.pl
Added error if there are multiple MARC templates with the same name.
Added error if there are multiple MARC templates with the same name.
Bug 18690: Use MARC modification templates with bulkmarcimport.pl
Added error if there are multiple MARC templates with the same name.
Bug 18389: QA Followup - Fix tabs, note used template, info on verbose
Bug 18690: Use MARC modification templates with bulkmarcimport.pl
Added error if there are multiple MARC templates with the same name.
Bug 18389: QA Followup - Fix tabs, note used template, info on verbose

Description Jon Knight 2017-04-05 16:21:49 UTC
Created attachment 61907 [details] [review]
Patch for bulkmarcimport.pl

When importing large numbers of MARC records from a legacy LMS to Koha we needed to convert some of 852 fields to 952 fields for item holdings.  I'd created MARC modification templates in the web administration front end, but the bulkmarcimport.pl migration tool doesn't use these. 

Joubu on the #koha IRC channel told me that this was because bulkmarcimport.pl was written before C4::MarcModificationTemplates.pm Perl module was available.

The change is actually relatively easy and I've attached a diff patch to this case for others to contemplate and critic.
Comment 1 Jon Knight 2017-05-10 15:19:39 UTC
Created attachment 63353 [details] [review]
The same patch as above only generated within git and without touch existing whitespace formatting oddities in the script.

When I mentioned this bug/patch on the Koha IRC channel originally I was told I needed to read up on how to submit bugs/patch because I'd done something wrong. I've read a load of wiki pages, etc now and I think the issue might have been that I tidied up the white space formatting in another part of the same script and didn't have the patch generated by git (rather I just used context diffing on our non-git installation). Hopefully this patch will find more favour. :-)
Comment 2 Katrin Fischer 2017-05-10 21:46:21 UTC
Comment on attachment 63353 [details] [review]
The same patch as above only generated within git and without touch existing whitespace formatting oddities in the script.

Review of attachment 63353 [details] [review]:
-----------------------------------------------------------------

Hi Jon,

the patch is looking good, thx for your contribution! Looking at the code, I am missing documentation of the new option - could you add it please so it shows up when bulkmarcimport is run without params or with -h?

Also noticed another small thing:

::: misc/migration_tools/bulkmarcimport.pl
@@ +3,4 @@
>  
>  use strict;
>  use warnings;
> +use Modern::Perl;

use Modern::Perl; replaces use strict; and use warnings; so the 2 lines above should be deleted.
Comment 3 Katrin Fischer 2017-05-10 21:47:05 UTC
Oh, and also don't forget to switch to "Needs sign-off" when you upload a patch for testing! Spotted this in my mailbox, but it will now show on the Dashboard etc. otherwise!
Comment 4 Jon Knight 2017-05-15 09:51:54 UTC
Created attachment 63469 [details] [review]
Additional patch to remove use strict/warn and add POD

This is an additional patch to remove the use strict and use warn lines before the use Perl::Modern, and add some POD for the help option.
Comment 5 Jon Knight 2017-05-15 10:16:33 UTC
Oh, and thanks for the advice Katrin - I'm still getting to grips with Koha and git (been an RCS/SVN user for years!).
Comment 6 Katrin Fischer 2017-05-16 06:05:01 UTC
Thx for the follow-up!
Comment 7 Jonathan Druart 2017-05-30 15:13:34 UTC
Hi Jon,
Could you resubmit your patch following the requirements on the wiki page https://wiki.koha-community.org/wiki/Commit_messages
Comment 8 Jon Knight 2017-05-30 16:04:34 UTC
OK.  This may take some time as I'm new to git and I've not idea how to recommit something I've already committed.
Comment 9 Jon Knight 2017-05-30 18:05:51 UTC
Created attachment 63851 [details] [review]
Revised patch with commit information reformatted.
Comment 10 Jonathan Druart 2017-05-30 18:50:54 UTC
Sounds good, thanks Jon!
Do not forget to change the status accordingly :)
Comment 11 Katrin Fischer 2017-08-17 10:55:10 UTC
Hi Jon, it would be great if this could also be included in the stage_file.pl/commit_file.pl scripts that are command line tools of the staged MARC import. Of course, separate bug (not trying to scope creep)!
Comment 12 Katrin Fischer 2017-08-17 11:10:11 UTC
The QA tools point out some small issues with tabs vs. spaces - something to keep in mind for your next patches!

As this works great, I am fixing them in the signed-off patch :)

 FAIL	misc/migration_tools/bulkmarcimport.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 126)
		forbidden pattern: tab char (line 128)
		forbidden pattern: tab char (line 285)
		forbidden pattern: tab char (line 125)
		forbidden pattern: tab char (line 131)
		forbidden pattern: tab char (line 127)
		forbidden pattern: tab char (line 286)
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid
Comment 13 Katrin Fischer 2017-08-17 11:11:50 UTC
Created attachment 66144 [details] [review]
[SIGNED OFF] Bug 18690: Use MARC modification templates with bulkmarcimport.pl

When importing large numbers of MARC records from a legacy LMS to Koha
using bulkmarcimport.pl, it did not make use of the MARC modification
templates in the system (which can be useful for coversion of 852
fields to 952 fields for item holdings for example). This patch allows
MARC modification templates to be used with bulkmarcimport.pl.

To test:
1) Apply patch.
2) Set up a MARC modification template (in Home > Tools > MARC
modification templates) to make some changes to imported MARC
records (for example copy a subfield).
3) Take a test set of MARC records that have fields matching the
template and import them using the bulkmarcimport.pl tool. For example
if these MARC records are in testrecords.mrc and the MARC modification
template is called testtemplate use something like:

   perl misc/migration_tools/bulkmarcimport.pl -commit 1000 \\
     -file testrecords.mrc -marcmodtemplate testtemplate

4) Check the imported records in Koha to see that the required
modifications have been applied when the MARC records are imported.
5) Sign off.

https://bugs.koha-community.org/show_bug.cgi?id=18389

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Jon Knight 2017-08-22 13:22:56 UTC
Thanks Katrin!
Comment 15 Tomás Cohen Arazi 2017-08-22 17:38:53 UTC
Hi Jon, the enhancement works as advertised and things look ok :-D

I was wondering if using the template name was the best option, given the 'name' column is not a UNIQUE id.
I saw the UI doesn't show the real DB ID for the MARC modification template, so I'm not sure what's the best solution.

Please add your comments on how could we make it rock solid.
Comment 16 Jon Knight 2017-08-23 09:05:47 UTC
I'd prefer to not expose the database ID to the end user as I don't think that's terribly user friendly, nor portable. Recently I've been writing Koha SQL reports for use with an external system and having the IDs change between our dev box and the (hosted) live Koha is a bit of a pain there (I'm tempted to suggest a bug enhancement to allow SQL reports to be selected by name rather than ID!).

Should being able to generate MARC modification templates with identical names be viewed as a bug in the UI perhaps? Its going to be confusing there too.

I'll put a check in to detect multiple MARC modification templates with the same name and refuse to run until they are renamed.
Comment 17 Jon Knight 2017-08-23 09:09:38 UTC
Created attachment 66379 [details] [review]
Added error if there are multiple MARC templates with the same name.
Comment 18 Jon Knight 2017-08-23 09:13:45 UTC
Test for this additional patch is to use the UI to create two MARC modification templates with exactly the same name, and then try to use that name with the bulkmarcimport.pl script.  It should fail with an error message suggesting that the templates are renamed.
Comment 19 Katrin Fischer 2017-08-26 22:04:38 UTC
Just a note: You can't actually rename the templates at the moment, but you can duplicate with a new name and delete the old one.
Comment 20 Katrin Fischer 2017-08-26 22:05:43 UTC
Created attachment 66498 [details] [review]
Added error if there are multiple MARC templates with the same name.

https://bugs.koha-community.org/show_bug.cgi?id=18389
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Nick Clemens 2017-10-26 10:49:24 UTC Comment hidden (obsolete)
Comment 22 Nick Clemens 2017-10-26 10:49:28 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens 2017-10-26 10:49:32 UTC Comment hidden (obsolete)
Comment 24 Nick Clemens 2017-10-26 10:50:14 UTC
Created attachment 68590 [details] [review]
Bug 18690: Use MARC modification templates with bulkmarcimport.pl

When importing large numbers of MARC records from a legacy LMS to Koha
using bulkmarcimport.pl, it did not make use of the MARC modification
templates in the system (which can be useful for coversion of 852
fields to 952 fields for item holdings for example). This patch allows
MARC modification templates to be used with bulkmarcimport.pl.

To test:
1) Apply patch.
2) Set up a MARC modification template (in Home > Tools > MARC
modification templates) to make some changes to imported MARC
records (for example copy a subfield).
3) Take a test set of MARC records that have fields matching the
template and import them using the bulkmarcimport.pl tool. For example
if these MARC records are in testrecords.mrc and the MARC modification
template is called testtemplate use something like:

   perl misc/migration_tools/bulkmarcimport.pl -commit 1000 \\
     -file testrecords.mrc -marcmodtemplate testtemplate

4) Check the imported records in Koha to see that the required
modifications have been applied when the MARC records are imported.
5) Sign off.

https://bugs.koha-community.org/show_bug.cgi?id=18389

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2017-10-26 10:50:17 UTC
Created attachment 68591 [details] [review]
Added error if there are multiple MARC templates with the same name.

https://bugs.koha-community.org/show_bug.cgi?id=18389
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Nick Clemens 2017-10-26 10:50:21 UTC
Created attachment 68592 [details] [review]
Bug 18389: QA Followup - Fix tabs, note used template, info on verbose

Fixed whitespace for QA tools
Added a verbose note when template found
Only print 'Modifying MARC' if verbose

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2017-10-26 10:51:58 UTC
Added a small followup, Tomas can you check it?

Ideally we would fix the naming issue in the staff side, but I think is beyond scope here.
Comment 28 Tomás Cohen Arazi 2017-10-27 12:16:04 UTC
(In reply to Nick Clemens from comment #27)
> Added a small followup, Tomas can you check it?
> 
> Ideally we would fix the naming issue in the staff side, but I think is
> beyond scope here.

I like it, Nick. Go ahead.
Comment 29 Jonathan Druart 2017-10-27 17:14:09 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 30 Jonathan Druart 2017-10-27 17:18:37 UTC
Note: I did not catch before the wrong bug number in the commit message. It has been pushed with "bug 18690: ".
Please do not be dyslexic with bug numbers.
Comment 31 Katrin Fischer 2017-10-28 10:00:19 UTC
(In reply to Jonathan Druart from comment #30)
> Note: I did not catch before the wrong bug number in the commit message. It
> has been pushed with "bug 18690: ".
> Please do not be dyslexic with bug numbers.

Will have to be manually fixed on release notes :(
Comment 32 Katrin Fischer 2017-10-28 10:01:12 UTC
Ignore the sad smiley, just something to keep in mind. :)