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.
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 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.
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!
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.
Oh, and thanks for the advice Katrin - I'm still getting to grips with Koha and git (been an RCS/SVN user for years!).
Thx for the follow-up!
Hi Jon, Could you resubmit your patch following the requirements on the wiki page https://wiki.koha-community.org/wiki/Commit_messages
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.
Created attachment 63851 [details] [review] Revised patch with commit information reformatted.
Sounds good, thanks Jon! Do not forget to change the status accordingly :)
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)!
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
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>
Thanks Katrin!
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.
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.
Created attachment 66379 [details] [review] Added error if there are multiple MARC templates with the same name.
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.
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.
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>
Created attachment 68587 [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>
Created attachment 68588 [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>
Created attachment 68589 [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
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>
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>
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>
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.
(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.
Pushed to master for 17.11, thanks to everybody involved!
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.
(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 :(
Ignore the sad smiley, just something to keep in mind. :)