Bug 12598

Summary: New misc/import_borrowers.pl command line tool
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Command-line UtilitiesAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: abl, amy, benjamin.rokseth, bgkriegel, colin.campbell, dschust1, florent.mara, jonathan.druart, katrin.fischer, kholten, kyle.m.hall, m.de.rooy, magnus, martin.renvoize, mtj, nicole, paul.poulain
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13607
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16426
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Koha now has a command line tool for importing patron CSV files of the same format the web-based tool uses. This tool allows a user to specify a matchpoint, set default values for non-existing fields, decide if a match should be overwritten, and if extended attributes should be preserved! For more details, run "misc/import_patrons.pl -h"
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13717, 13704, 21465, 22136, 27586    
Attachments: Bug 12598 - Allow tools/import_borrowers.pl to be used from the command line.
Bug 12598 - Allow tools/import_borrowers.pl to be used from the command line.
Bug 12598 - Allow tools/import_borrowers.pl to be used from the command line.
Bug 12598 - Allow tools/import_borrowers.pl to be used from the command line.
Bug 12598 - Allow tools/import_borrowers.pl to be used from the command line.
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 - Move Koha::Patrons::Import to Koha::Borrower::Import
[SIGNED-OFF] Bug 12598 - Tidy import_borrowers.pl
[SIGNED-OFF] Bug 12598 - Move importing code to a subroutine
[SIGNED-OFF] Bug 12598 - Update command line script to use patron import subroutine
[SIGNED-OFF] Bug 12598 - Move Koha::Patrons::Import to Koha::Borrower::Import
[SIGNED-OFF] Bug 12598: New misc/import_borrowers.pl command line tool
[SIGNED-OFF] Bug 12598: New misc/import_borrowers.pl command line tool
[SIGNED-OFF] Bug 12598 - Tidy import_borrowers.pl
[SIGNED-OFF] Bug 12598 - Move importing code to a subroutine
[SIGNED-OFF] Bug 12598 - Update command line script to use patron import subroutine
[SIGNED-OFF] Bug 12598 - Move Koha::Patrons::Import to Koha::Borrower::Import
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 [QA Followup]
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 [QA Followup]
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 [QA Followup]
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 [QA Followup]
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl
Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package
Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import
Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl
Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Bug 12598: New misc/import_borrowers.pl command line tool
Attachment to Bug 12598 - New misc/import_borrowers.pl command line tool
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 [QA Followup]
Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl
Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package
Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import
Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl
Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing
Bug 12598 - Rebase + backport of 16426 plus fixing 16426
Patrons sample data
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598 - Tidy import_borrowers.pl
Bug 12598 - Move importing code to a subroutine
Bug 12598 - Update command line script to use patron import subroutine
Bug 12598 [QA Followup]
Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl
Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package
Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import
Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl
Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing
Bug 12598 - Rebase + backport of 16426 plus fixing 16426
Bug 12598: catch warnings raised by import_patrons in tests
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: Update code to use new modules
Bug 12598: Fix conflict with bug 16911
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: Update code to use new modules
Bug 12598: Fix conflict with bug 16911
Bug 12598: Fix conflict with 17829 - GetMember
Bug 12598: Re-add some missing stuffs
New import_borrowers.pl command tool
Update code to use new modules
Fis conflict with bug 16911
Fix conflict with 17829 - GetMember
Re-add some missing stuff
Allow matched patron to be updated
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: Update code to use new modules
Bug 12598: Fix conflict with bug 16911
Bug 12598: Fix conflict with 17829 - GetMember
Bug 12598: Re-add some missing stuffs
Bug 12598 Allow matched patron to be updated
Bug 12598: Re-add bug 18555: Create patron list from patron import
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: Update code to use new modules
Bug 12598: Fix conflict with bug 16911
Bug 12598: Fix conflict with 17829 - GetMember
Bug 12598: Re-add some missing stuffs
Bug 12598 Allow matched patron to be updated
Bug 12598: Re-add bug 18555: Create patron list from patron import
Bug 12598: Add use Koha::Patron::debarments statement
Bug 12598: Re-add bug 18555: Create patron list from patron import
Bug 12598: Add use Koha::Patron::Debarments statement
Bug 12598: Remove warnings from tests and make them pass
Bug 12598: Use Pod::Usage and make the help more explicit
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: New misc/import_borrowers.pl command line tool
Bug 12598: Update code to use new modules
Bug 12598: Fix conflict with bug 16911
Bug 12598: Fix conflict with 17829 - GetMember
Bug 12598: Re-add some missing stuffs
Bug 12598 Allow matched patron to be updated
Bug 12598: Re-add bug 18555: Create patron list from patron import
Bug 12598: Add use Koha::Patron::debarments statement
Bug 12598: Remove warnings from tests and make them pass
Bug 12598: Use Pod::Usage and make the help more explicit
Bug 12598: Fix POD

Description Kyle M Hall 2014-07-17 18:21:03 UTC
Many libraries have the need to automate the importing of patrons into Koha from external sources. It would be nice if we could use Koha's own import_borrowers.pl to do that from the command line, in the same way as tools/export.pl can be used from both the staff interface and the command line.
Comment 1 Kyle M Hall 2014-07-17 18:23:21 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-07-17 18:31:07 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-07-17 18:35:33 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-07-20 21:03:53 UTC
*** Bug 5633 has been marked as a duplicate of this bug. ***
Comment 5 Katrin Fischer 2014-07-20 21:12:09 UTC
Hi Kyle, I think this is a duplicate to bug 5633. 

There it was pointed out that normal scripts should not be command line scripts at the same time. Could you move this functionality into a separate command line script?
Comment 6 Kyle M Hall 2014-07-21 17:50:37 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2014-07-21 17:52:54 UTC
(In reply to Katrin Fischer from comment #5)
> Hi Kyle, I think this is a duplicate to bug 5633. 
> 
> There it was pointed out that normal scripts should not be command line
> scripts at the same time. Could you move this functionality into a separate
> command line script?

I was not aware of that. Thanks for letting me know! Here is a new variant that is command line only and lives in misc.
Comment 8 Bernardo Gonzalez Kriegel 2014-08-09 14:17:29 UTC
Hi, testing

$ misc/import_borrowers.pl --help
Can't locate object method "new" via package "CGI" at misc/import_borrowers.pl line 47

Why use CGI on a command line version?
Comment 9 Kyle M Hall 2014-08-13 12:07:21 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #8)
> Hi, testing
> 
> $ misc/import_borrowers.pl --help
> Can't locate object method "new" via package "CGI" at
> misc/import_borrowers.pl line 47
> 
> Why use CGI on a command line version?

That's a leftover, I'll remove that.
Comment 10 Kyle M Hall 2014-08-13 12:42:27 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2014-08-14 00:55:01 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-08-22 10:45:14 UTC
Kyle,
I am sorry but this patch c/p almost all the code from tools/import_borrowers.pl.
I am not sure this is the right way to do.

The script should also use Pod2Usage (instead of the print_help routine).

I am completely aware of the quantity of work to provide to reach a correct quality of core here: refactoring, unit tests, etc.

But I am pretty sure this script should not be pushed as it.

I let this bug report in the NQA queue in order to get more feedback.
Comment 13 Kyle M Hall 2014-08-22 15:08:04 UTC
(In reply to Jonathan Druart from comment #12)
> Kyle,
> I am sorry but this patch c/p almost all the code from
> tools/import_borrowers.pl.
> I am not sure this is the right way to do.

That is why my initial patch allowed the existing tools/import_borrowers.pl script to be used from the command line. However, I was told that is no longer allowed ( tools/export.pl has this behavior ), though it is not stated in the coding guidelines.

> The script should also use Pod2Usage (instead of the print_help routine).

Is this in the coding guidelines?

> I am completely aware of the quantity of work to provide to reach a correct
> quality of core here: refactoring, unit tests, etc.

Not sure why this would be required for code we already know works...

While I agree in general, this is proven code, and your proposal would add an order of magnitude to the development with no real gains. I would much prefer we allow tools/import_borrowers.pl to be used from the command line for this exact reason.

> But I am pretty sure this script should not be pushed as it.

I would respectfully disagree ; )
Comment 14 Jonathan Druart 2014-08-22 15:28:07 UTC
(In reply to Kyle M Hall from comment #13)
> That is why my initial patch allowed the existing tools/import_borrowers.pl
> script to be used from the command line. However, I was told that is no
> longer allowed ( tools/export.pl has this behavior ), though it is not
> stated in the coding guidelines.

We all know it's a very bad idea. tools/export.pl is the perfect example, it's a pain to maintain it.

> > The script should also use Pod2Usage (instead of the print_help routine).
> 
> Is this in the coding guidelines?

I don't know, all new scripts in misc use Pod2Usage.
I think it's a good way to follow, don't you think?

> > I am completely aware of the quantity of work to provide to reach a correct
> > quality of core here: refactoring, unit tests, etc.
> 
> Not sure why this would be required for code we already know works...

If the code is moved to a new subroutine, it will require to be covered by UT.

> While I agree in general, this is proven code, and your proposal would add
> an order of magnitude to the development with no real gains. I would much
> prefer we allow tools/import_borrowers.pl to be used from the command line
> for this exact reason.

The gain is the "maintainability" (this word exist?) of the code.
In the current state, the code is duplicated.
Comment 15 Kyle M Hall 2014-08-25 14:05:44 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Kyle M Hall from comment #13)
> > That is why my initial patch allowed the existing tools/import_borrowers.pl
> > script to be used from the command line. However, I was told that is no
> > longer allowed ( tools/export.pl has this behavior ), though it is not
> > stated in the coding guidelines.
> 
> We all know it's a very bad idea. tools/export.pl is the perfect example,
> it's a pain to maintain it.
> 
> > > The script should also use Pod2Usage (instead of the print_help routine).
> > 
> > Is this in the coding guidelines?
> 
> I don't know, all new scripts in misc use Pod2Usage.
> I think it's a good way to follow, don't you think?

Agreed, but I've already implemented it a different way. Please suggest to the RM that this be put in the coding guidelines. We can vote on it at the next meeting.

> 
> > > I am completely aware of the quantity of work to provide to reach a correct
> > > quality of core here: refactoring, unit tests, etc.
> > 
> > Not sure why this would be required for code we already know works...
> 
> If the code is moved to a new subroutine, it will require to be covered by
> UT.

I do agree, but I think it's a heavy load to put on one developer. Put it this way, I'd be willing to move the code to shared subroutines if someone else would volunteer to write unit tests.

> > While I agree in general, this is proven code, and your proposal would add
> > an order of magnitude to the development with no real gains. I would much
> > prefer we allow tools/import_borrowers.pl to be used from the command line
> > for this exact reason.
> 
> The gain is the "maintainability" (this word exist?) of the code.
> In the current state, the code is duplicated.

Yep, that's the correct word. And that is why I prefer to have tools/import_borrowers.pl work from the command line. I think it's the best compromise. I don't think these changes introduce any serious maintenance issues. If you inspect the code the changes are almost entirely if/else's for passing output messages to the template or to the command line depending on how it is run.
Comment 16 Magnus Enger 2014-09-02 08:56:34 UTC
*** Bug 3340 has been marked as a duplicate of this bug. ***
Comment 17 Marcel de Rooy 2014-10-13 12:12:21 UTC
QA Comment:
I agree with Jonathan that we should move shared code into a module.
That should not be so much work.
If you would not have unit tests rightaway, I could live with that argumenting that we do not have these unit tests at this moment also and you are just moving code.
If you could write them or at least a simple start, you would receive bonus points of course ;)

Failed QA
Comment 18 Kyle M Hall 2015-01-27 16:15:08 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2015-01-27 16:15:33 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2015-01-27 16:15:40 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2015-01-27 16:15:44 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2015-02-11 16:09:32 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2015-02-11 16:09:40 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-02-11 16:09:43 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-02-11 16:09:46 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-02-17 12:06:17 UTC Comment hidden (obsolete)
Comment 27 Benjamin Rokseth 2015-03-06 11:17:00 UTC Comment hidden (obsolete)
Comment 28 Benjamin Rokseth 2015-03-06 11:17:08 UTC Comment hidden (obsolete)
Comment 29 Benjamin Rokseth 2015-03-06 11:17:21 UTC Comment hidden (obsolete)
Comment 30 Benjamin Rokseth 2015-03-06 11:17:33 UTC Comment hidden (obsolete)
Comment 31 Benjamin Rokseth 2015-03-06 11:19:18 UTC Comment hidden (obsolete)
Comment 32 Benjamin Rokseth 2015-03-06 11:38:04 UTC Comment hidden (obsolete)
Comment 33 Benjamin Rokseth 2015-03-06 11:38:16 UTC Comment hidden (obsolete)
Comment 34 Benjamin Rokseth 2015-03-06 11:38:24 UTC Comment hidden (obsolete)
Comment 35 Benjamin Rokseth 2015-03-06 11:38:35 UTC Comment hidden (obsolete)
Comment 36 Benjamin Rokseth 2015-03-06 11:38:55 UTC Comment hidden (obsolete)
Comment 37 Benjamin Rokseth 2015-03-06 11:43:03 UTC
Sorry for the initial screw-up on the order of sign-offs. This is absolutely a valuable addition for automated setups!
Comment 38 Katrin Fischer 2015-04-06 21:30:24 UTC
Patch fails the QA script:

1)
 FAIL	Koha/Borrower/Import.pm
   FAIL	  critic
		Package declaration must match filename at line 1, column 1. Correct the filename or package statement.
   OK	  forbidden patterns
   OK	  pod
   OK	  valid

2) Patch removes the recently added matching option on userid:
-                    <option value="userid">Username</option>

3) Is the copyright for the new file import_borrowers.pl correct?
+# Copyright 2007 Liblime
+# Parts copyright 2010 BibLibre
+# Parts copyright 2014 ByWater Solutions

4) c is used for csv file here, but as confirm in other scripts. Again... not sure what we finally agreed to, but should we change this and maybe implement a 'confirm'?

Can you please fix?
Comment 39 Kyle M Hall 2015-05-13 13:22:59 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2015-05-13 13:23:07 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2015-05-13 13:23:10 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2015-05-13 13:23:13 UTC Comment hidden (obsolete)
Comment 43 Kyle M Hall 2015-05-13 13:23:16 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2015-06-04 10:42:04 UTC
Kyle,

Would you mind if I squash the patches?

Don't you think it would make more sense to have Koha::Import::Patrons instead of Koha::Patrons::Import? I can imagine easily a Koha::Import module with some useful subroutines to import some kind of stuffs.
Comment 45 Kyle M Hall 2015-06-16 15:49:37 UTC
(In reply to Jonathan Druart from comment #44)
> Kyle,
> 
> Would you mind if I squash the patches?
> 
> Don't you think it would make more sense to have Koha::Import::Patrons
> instead of Koha::Patrons::Import? I can imagine easily a Koha::Import module
> with some useful subroutines to import some kind of stuffs.

Squash away!

I have no problem if you want to change the namespace.
Comment 46 Jonathan Druart 2015-06-16 16:05:51 UTC
(In reply to Kyle M Hall from comment #45)
> I have no problem if you want to change the namespace.

Do you agree with my thought or you just said that to make me happy? :)
Comment 47 Kyle M Hall 2015-07-17 10:48:29 UTC
(In reply to Jonathan Druart from comment #46)
> (In reply to Kyle M Hall from comment #45)
> > I have no problem if you want to change the namespace.
> 
> Do you agree with my thought or you just said that to make me happy? :)

Lol! Why not both? I have no strong opinion on the matter so if you think a different namespace would be better I'm fine with that. If you submit a patch to change the namespace I'd be more than happy to sign it off!
Comment 48 Alan 2015-08-12 13:12:20 UTC
Hi All, 

When do you anticipate this being available for use? We had used the command line tool in the past, so now are in a mode of manual uploads until this is ready. 

Thanks for all of the work you are doing!

Alan
Comment 49 Jonathan Druart 2015-08-19 13:22:44 UTC
Kyle, could you please rebase this patch?
Comment 50 Kyle M Hall 2015-08-21 18:56:04 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2015-08-21 18:56:13 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2015-08-21 18:56:16 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2015-08-21 18:56:19 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2015-08-21 18:56:22 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2015-10-27 13:48:24 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2015-10-27 13:48:32 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2015-10-27 13:48:36 UTC Comment hidden (obsolete)
Comment 58 Kyle M Hall 2015-10-27 13:48:39 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2015-10-27 13:48:43 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2015-10-27 13:49:48 UTC
I would really like to see this qa'ed for a fast push after the 3.22 release. The longer this sits here the more likely the new subroutine will fall out of sync with the current import code in master.
Comment 61 Kyle M Hall 2016-03-07 15:27:35 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2016-03-07 15:27:44 UTC Comment hidden (obsolete)
Comment 63 Kyle M Hall 2016-03-07 15:27:47 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2016-03-07 15:27:51 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2016-03-07 15:27:55 UTC Comment hidden (obsolete)
Comment 66 Florent Mara 2016-04-18 22:39:11 UTC
As part of an effort to improve performance when importing borrowers, I have started working on top of the current patches. 
Steps proposed are as follows:

(1) Resolve merge conflicts from pre-existing patches. (Completed)
(2) Add regression tests to new Koha::Patrons::Import. (Completed)
(3) Refactor Koha::Patrons::Import + further regression testing. ( In progress )
(4) Review import_borrowers.pl and Koha::Patrons::Import for further improvements. (To do)

Any feedback welcomed.
Comment 67 Jonathan Druart 2016-04-19 06:42:01 UTC
(In reply to Florent Mara from comment #66)
> As part of an effort to improve performance when importing borrowers, I have
> started working on top of the current patches. 
> Steps proposed are as follows:
> 
> (1) Resolve merge conflicts from pre-existing patches. (Completed)
> (2) Add regression tests to new Koha::Patrons::Import. (Completed)
> (3) Refactor Koha::Patrons::Import + further regression testing. ( In
> progress )
> (4) Review import_borrowers.pl and Koha::Patrons::Import for further
> improvements. (To do)
> 
> Any feedback welcomed.

Good to heard that :)
Don't forget to share your patches ASAP
Comment 68 Kyle M Hall 2016-04-19 10:05:20 UTC
(In reply to Florent Mara from comment #66)
> As part of an effort to improve performance when importing borrowers, I have
> started working on top of the current patches. 
> Steps proposed are as follows:
> 
> (1) Resolve merge conflicts from pre-existing patches. (Completed)
> (2) Add regression tests to new Koha::Patrons::Import. (Completed)
> (3) Refactor Koha::Patrons::Import + further regression testing. ( In
> progress )
> (4) Review import_borrowers.pl and Koha::Patrons::Import for further
> improvements. (To do)
> 
> Any feedback welcomed.

Please submit your patches as soon as you can! I'll hold off on updating this patch set. There is little point to rebasing it until we can take advantage of your new module.
Comment 69 Florent Mara 2016-04-21 04:26:55 UTC
Many thanks for the feedback. If Chris has the time next Tuesday, I will kindly ask his advice on the next step for my work. It might be good enough to be in a patch but want to make sure.

Other than rebasing, tests exposed some defects which I have fixed while refactoring.
Comment 70 Florent Mara 2016-04-26 02:31:26 UTC Comment hidden (obsolete)
Comment 71 Florent Mara 2016-04-26 02:32:31 UTC Comment hidden (obsolete)
Comment 72 Florent Mara 2016-04-26 02:33:33 UTC Comment hidden (obsolete)
Comment 73 Florent Mara 2016-04-26 02:34:23 UTC Comment hidden (obsolete)
Comment 74 Florent Mara 2016-04-26 02:34:58 UTC Comment hidden (obsolete)
Comment 75 Florent Mara 2016-04-26 02:37:05 UTC Comment hidden (obsolete)
Comment 76 Florent Mara 2016-04-26 02:38:03 UTC Comment hidden (obsolete)
Comment 77 Florent Mara 2016-04-26 02:39:18 UTC Comment hidden (obsolete)
Comment 78 Florent Mara 2016-04-26 02:59:21 UTC
I might have done the right thing when creating my patch. (or not, please let me know if it is the case)

The plan is to continue refactoring the Patrons::Import module. Regression tests have shown further defects that will require moving code around and more changes.
Comment 79 Bernardo Gonzalez Kriegel 2016-04-26 03:43:27 UTC
Hi Florent,

with simple csv
cardnumber,surname,firstname,categorycode,branchcode,password,userid
12344321,Doe,John,S,CP,test,john

'misc/import_patrons.pl -f test.csv -c -m cardnumber -v' gives:

No file handle passed in! at misc/import_patrons.pl line 51.
Use of uninitialized value $alreadyindb in addition (+) at misc/import_patrons.pl line 69.

etc.

On your last patch you put
sub import_patrons {
    my ($self, $params) = @_;

but $self has the params and $params is undef.

Removing the last patch I get

Import complete:
Imported:    0
Overwritten: 0
Skipped:     0
Invalid:     1
Total:       1

which is better but invalid, I don't know why.

Test pass, no koha-qa errors so far.
Comment 80 Florent Mara 2016-04-26 03:57:06 UTC
Hasty addition of Moo on my part. Fixing right now.

But I am very glad you were able to test my patches / fixes ... and new defects.
Comment 81 Florent Mara 2016-04-26 04:00:58 UTC Comment hidden (obsolete)
Comment 82 Florent Mara 2016-05-03 04:27:45 UTC Comment hidden (obsolete)
Comment 83 Florent Mara 2016-05-03 04:38:15 UTC
The latest patch includes the following:

(1) Further refactoring and regression tests in Koha::Patrons::Import module.

(2) A minor bug fix for header row parser by which it was possible to fail the header row parsing (status equal to 0) yet call on Text::CSV->field() which would create an undef entry in both the @csvcolumns and %csvkeycol variables.

(3) A more important bug fix related to dateofbirth, dateenrolled and dateexpiry imports by which the failure to format the dates would see these being silently ignored/replaced with default dates. This defect was caused by the @missing_criticals being populated but ignored in the remainder of the loop. This defect is still present in master.
Comment 84 Katrin Fischer 2016-05-03 06:22:47 UTC
Hi Florent, please be careful not to let this grow too big on a single bug report, might be got so split issues out into several bugs at some point :)
We will also need a test plan for the testers.
Is the status correct? (Patch doesn't apply)
Comment 85 Florent Mara 2016-05-03 10:15:27 UTC
I agree 100% Katrin. Being merge master at work, I know the pain and have debated the options several times. It was already a bit patch when I found it, only made it bigger ...

Overall, I doubt more than 30 lines of 'working' code has been added so far. Most of my contributions are tests + bug fixes. ~95% of my refactoring is taking chunks of existing code from the main script and putting it in methods. Then adding pedantic tests to validate equivalence of output.
The other 5% are defect fixes. 

I thought of stopping last week then found a defect so kept going (I intended to create a specific bug report for the dates defect but ran out of time today). Then a second - minor - defect while at it today. 

Ultimately, none of my work is intended to change code behavior - unless defect. This refactoring + tests work is in preparation for my task of improving the importer performance which is hardly possible while working on a 300 lines untested script.

For test plan, I use the 1000 entries file from the importer performance bug, would it be worth giving more details here? I loaded plenty of patrons today, it would be good for someone else to give it a try and update the status.
Comment 86 Florent Mara 2016-05-10 02:30:47 UTC Comment hidden (obsolete)
Comment 87 Florent Mara 2016-05-10 02:34:52 UTC Comment hidden (obsolete)
Comment 88 Florent Mara 2016-05-10 02:51:38 UTC Comment hidden (obsolete)
Comment 89 Florent Mara 2016-05-10 03:04:00 UTC Comment hidden (obsolete)
Comment 90 Florent Mara 2016-05-10 03:33:17 UTC Comment hidden (obsolete)
Comment 91 Florent Mara 2016-05-10 03:35:29 UTC Comment hidden (obsolete)
Comment 92 Florent Mara 2016-05-10 03:36:29 UTC Comment hidden (obsolete)
Comment 93 Florent Mara 2016-05-10 03:38:27 UTC Comment hidden (obsolete)
Comment 94 Florent Mara 2016-05-10 03:42:06 UTC Comment hidden (obsolete)
Comment 95 Florent Mara 2016-05-10 03:43:36 UTC Comment hidden (obsolete)
Comment 96 Florent Mara 2016-05-10 03:44:24 UTC Comment hidden (obsolete)
Comment 97 Florent Mara 2016-05-10 03:45:17 UTC Comment hidden (obsolete)
Comment 98 Florent Mara 2016-05-10 03:46:01 UTC Comment hidden (obsolete)
Comment 99 Florent Mara 2016-05-10 03:48:11 UTC Comment hidden (obsolete)
Comment 100 Florent Mara 2016-05-10 03:50:01 UTC Comment hidden (obsolete)
Comment 101 Florent Mara 2016-05-10 03:56:12 UTC
Created attachment 51372 [details]
Patrons sample data
Comment 102 Florent Mara 2016-05-10 04:10:43 UTC
This patch has been rebased today. Bug 16426 was backported and fixed.

Test plan:
1) Apply this patch. 
2) Set dateformat to mm/dd/yyyy. (Koha home->Administration->search for dateformat -> select and save)
3) run 'prove t/db_dependent/Koha/Patrons/Import.t' which should result in 2 file handle warnings and 122 tests passing.
4) Run the online importer ( Koha home->tools->Import patrons) Load PatronDataCSV1.csv sample file with default import parameters. It should result in 165 imported records and 835 not imported because they are not in the expected format.
5) Set dateformat to dd/mm/yyyy.
6) Run the new command line importer 'perl misc/import_patrons.pl -f ~/Downloads/PatronDataCSV1.csv -c -m 'cardnumber' -v'
It should load the remaining 835 entries from the sample file.
Import complete:
Imported:    835
Overwritten: 0
Skipped:     0
Invalid:     165
Total:       1000
7) Optional. Run 'perl misc/import_patrons.pl --help' because you can.
Comment 103 Bernardo Gonzalez Kriegel 2016-05-10 10:58:17 UTC
Is it ready to be signed?
Comment 104 Florent Mara 2016-05-10 19:30:16 UTC
It is good enough for now. :-)
Comment 105 Bernardo Gonzalez Kriegel 2016-05-10 20:18:59 UTC
(In reply to Florent Mara from comment #104)
> It is good enough for now. :-)

If it's ready, could you please change status to 'Needs Signoff'?
Comment 106 Florent Mara 2016-05-14 03:44:47 UTC
(Thank you Bernardo for the advice on trying to progress this) 

Here are the changes introduces in each of the 6 patches:
Patch 1 - Replace deprecated C4::Dates with Koha::DateUtils and backport fix from master for entries with empty userids. This fix is deprecated in my last commit due to bug 16426.

Patch 2 - Add missing dependency C4::Members::Attributes and some pod. Add the first regression tests - none were present previously - with basic tests for empty parameters variables and valid input file. 

Patch 3 - Introduced Moo. Refactored setting of column keys, missing borrower category code and missing borrower branch code into 3 new methods. Features are identical as prior. Each method is 100% test covered with subtests. 

Patch 4 - Update misc/import_patrons.pl and tools/import_borrowers.pl to use Koha::Patrons::Import in an object oriented way.

Patch 5 - Adds missing dependency C4::Members::AttributeTypes. Refactor setting attributes types, prepare columns and dates formatting into 3 new methods. Features for attributes type and prepare column are identical. 

Date formatting was changes as follows after discussion with Chris.
* The previous implementation let parsing of dates silently failed then be overwritten with system default values during import as the error was push to @missing_criticals after this array was last considered. 
* The new implementation formats dates and uses @missing_criticals before it is used to bail out of importing the entry. 

Each new method is 100% test covered with subtests. 

Patch 6 - Backport changes from bug 16426 and fixes the defect it introduces. As per my comment in 16426, the fix was introduced at the wrong place in the code which caused it to never be reached and prevents checks on duplicate userid . 

Test plan:
1) Apply this patch. 
2) Set dateformat to mm/dd/yyyy. (Koha home->Administration->search for dateformat -> select and save) 
3) run 'prove t/db_dependent/Koha/Patrons/Import.t'. All 122 tests should pass. 
These regression tests validate (1) Koha::Patrons::Import compiles, (2) all fields output from the main import() method are identical before and after refactoring (equivalence of output), (3) all 6 new methods provide the expected feature, (4) backport duplicate userid fix. 
4) Run the online importer ( Koha home->tools->Import patrons) Load PatronDataCSV1.csv sample file with default import parameters. It should result in 165 imported records and 835 not imported because they are not in the expected format.
This step validates that (1) tools/import_borrowers.pl works as previously and wrongly formatted dates are now correctly being reported as such. 
5) Set dateformat to dd/mm/yyyy.
6) Run the new command line importer 'perl misc/import_patrons.pl -f PatronDataCSV1.csv -c -m 'cardnumber' -v'
It should load the remaining 835 entries from the sample file.
Import complete:
Imported:    835
Overwritten: 0
Skipped:     0
Invalid:     165
Total:       1000
This step validates the new command line tool misc/import_patrons works and confirm date validation is effective.

While this is a big patch, it contains only one feature change to fix dates being silently overwritten and is coming with a large number of tests with extensive test coverage for all changes made. It also contains a regression tested fix for bug 16426.
Comment 107 Kyle M Hall 2016-06-13 16:14:10 UTC
Created attachment 52323 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Test Plan:
1) Apply this patch
2) Test importing patrons from command line,
   options are availble with --help.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Tested with minimal csv
(cardnumber,surname,firstname,categorycode,branchcode,password,userid)

Overwrite does not change category or branch.
Patrons are loaded, userid & password works

Updated license to GPLv3
No other koha-qa errors.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 108 Kyle M Hall 2016-06-13 16:14:20 UTC
Created attachment 52324 [details] [review]
Bug 12598 - Tidy import_borrowers.pl

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 109 Kyle M Hall 2016-06-13 16:14:25 UTC
Created attachment 52325 [details] [review]
Bug 12598 - Move importing code to a subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 110 Kyle M Hall 2016-06-13 16:14:29 UTC
Created attachment 52326 [details] [review]
Bug 12598 - Update command line script to use patron import subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 111 Kyle M Hall 2016-06-13 16:14:33 UTC
Created attachment 52327 [details] [review]
Bug 12598 [QA Followup]

* Fix copyright on import_borrowers.pl
* Changes -c --csv to -f --file
* Adds -c --confirm option
* Renames misc/import_borrowers.pl to misc/import_patrons.pl
* Restore userid matchpoint option

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 112 Kyle M Hall 2016-06-13 16:14:38 UTC
Created attachment 52328 [details] [review]
Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 113 Kyle M Hall 2016-06-13 16:14:42 UTC
Created attachment 52329 [details] [review]
Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 114 Kyle M Hall 2016-06-13 16:14:47 UTC
Created attachment 52330 [details] [review]
Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 115 Kyle M Hall 2016-06-13 16:14:51 UTC
Created attachment 52331 [details] [review]
Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 116 Kyle M Hall 2016-06-13 16:14:56 UTC
Created attachment 52332 [details] [review]
Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 117 Kyle M Hall 2016-06-13 16:15:24 UTC
Created attachment 52333 [details] [review]
Bug 12598 - Rebase + backport of 16426 plus fixing 16426

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 118 Jonathan Druart 2016-06-19 12:03:56 UTC
Created attachment 52540 [details] [review]
Bug 12598: catch warnings raised by import_patrons in tests
Comment 119 Jonathan Druart 2016-06-19 12:06:18 UTC
2 Tests are failing
t/db_dependent/Koha/Patrons/Import.t .. 2/124     
    #   Failed test 'Got the expected array size from set column keys with undef extended'
    #   at t/db_dependent/Koha/Patrons/Import.t line 398.
    #          got: '67'
    #     expected: '66'
    
    #   Failed test 'Got the expected array size from set column keys with extended'
    #   at t/db_dependent/Koha/Patrons/Import.t line 405.
    #          got: '68'
    #     expected: '67'
    # Looks like you failed 2 tests of 2

I guess it's because a new column has been added to the borrowers table.
Maybe it would be better to test that
  @columnkeys_1 = @columnkeys_0 + 1
don't you think?
It would avoid these 2 tests to fail each time a new column is added/removed to this table.
Comment 120 Jonathan Druart 2016-06-19 12:07:47 UTC
FAIL   t/db_dependent/Koha/Patrons/Import.t
  FAIL   critic
    "$headerrow_0" is declared but not used at line 313, column 5. Unused variables clutter code and make it harder to read.
Comment 121 Jonathan Druart 2016-06-19 12:40:23 UTC
1/ + use namespace::clean;

Why do you need it?

2/ + use Moo;

You are going to start a neverending debate using Moo: it is not marked as mandatory and is not used from a CGI script. If you persist, you will have to start the discussion during the next dev meeting.

3/ On bug 15451, I have created a Koha::Exporter::Record file, which will export records.
Here you created a Koha::Patrons::Import. We will need to uniformise the namespace.

4/ You should use pod2usage to display the help of the command line script

5/ Doc of the script is wrong:
  import_patrons.pl -c /path/to/patrons.csv -m cardnumber
-f is missing

6/ Data::Dumper is not imported

7/ The pod does not explain that the script supports several levels of verbosity
Comment 122 Florent Mara 2016-06-21 03:23:51 UTC
I agree with most of it - also I can't reproduce the failed tests - but I can fail other tests so some of these need to be reconsidered. 
$headerrow_0 shouldn't be there. Namespace uniformity sounds like an excellent idea, any suggestion/guidance? 

The part that confuses me is around Moo usage and I assume the comment refers to Koha::Patrons::Import. The module has 2 attributes ('today_iso' and 'text_csv'), can these exist without importing Moo?
Comment 123 Jonathan Druart 2016-06-26 07:42:36 UTC
(In reply to Florent Mara from comment #122)
> I agree with most of it - also I can't reproduce the failed tests - but I
> can fail other tests so some of these need to be reconsidered. 
> $headerrow_0 shouldn't be there. Namespace uniformity sounds like an
> excellent idea, any suggestion/guidance? 

Actually it was added by bug 14722 which is already pushed.
I have added the topic to the next dev meeting, just in case (https://wiki.koha-community.org/wiki/Development_IRC_meeting_13_July_2016).
I have already talked about this to someone, but I don't find anything.

> The part that confuses me is around Moo usage and I assume the comment
> refers to Koha::Patrons::Import. The module has 2 attributes ('today_iso'
> and 'text_csv'), can these exist without importing Moo?

Yes of course.
What you are doing is just initializing them, so you can do that in the constructor (new).
Comment 124 Jonathan Druart 2016-06-26 07:43:23 UTC
(In reply to Jonathan Druart from comment #123)
> What you are doing is just initializing them, so you can do that in the
> constructor (new).

Florent, let me know if you need more info about how to do it.
Comment 125 Jonathan Druart 2017-06-19 18:11:28 UTC
Created attachment 64437 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Test Plan:
1) Apply this patch
2) Test importing patrons from command line,
   options are availble with --help.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Tested with minimal csv
(cardnumber,surname,firstname,categorycode,branchcode,password,userid)

Overwrite does not change category or branch.
Patrons are loaded, userid & password works

Updated license to GPLv3
No other koha-qa errors.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Tidy import_borrowers.pl

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Move importing code to a subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Update command line script to use patron import subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 [QA Followup]

* Fix copyright on import_borrowers.pl
* Changes -c --csv to -f --file
* Adds -c --confirm option
* Renames misc/import_borrowers.pl to misc/import_patrons.pl
* Restore userid matchpoint option

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Rebase + backport of 16426 plus fixing 16426

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598: catch warnings raised by import_patrons in tests
Comment 126 Jonathan Druart 2017-06-19 18:11:45 UTC
Created attachment 64438 [details] [review]
Bug 12598: Update code to use new modules
Comment 127 Jonathan Druart 2017-06-19 18:14:34 UTC
I tried to revive these patches, I had to squash all patches to rebase them (authorship lost, sorry!).

I have pushed a branch with all previous patches applied on https://github.com/joubu/Koha/tree/old/20160701_bug_12598 (so not up-to-date).

I switched the status to Needs Signoff to get feedback and know/estimate what need to be done to get something ready for push.
Comment 128 Colin Campbell 2017-06-23 07:36:09 UTC
Problem occured in testing

Koha/Patrons/Import.pm calls undefined GetExpiryDate method

Undefined subroutine &Koha::Patrons::Import::GetExpiryDate called at /usr/share/koha/lib/Koha/Patrons/Import.pm line 161
Comment 129 Jonathan Druart 2017-07-06 15:49:53 UTC
Created attachment 64855 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change
Comment 130 Colin Campbell 2017-07-20 14:55:40 UTC
Code in Koha/Patrons?Import.pm does not reflect latest version of existing import. When we tested we fell foul of the fact that it effectively undid the changes introduced by bug 12543 - it rejects all records matching on userid. As that is probably the most common matchpoint in the sites that do regular offline updates its a showstopper. Maybe that other functionality present in current load is missing
Comment 131 Jonathan Druart 2017-07-25 13:39:47 UTC
Created attachment 65214 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool
Comment 132 Jonathan Druart 2017-07-25 13:39:55 UTC
Created attachment 65215 [details] [review]
Bug 12598: Update code to use new modules
Comment 133 Jonathan Druart 2017-07-25 13:40:02 UTC
Created attachment 65216 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change
Comment 134 Jonathan Druart 2017-07-25 13:40:09 UTC
Created attachment 65217 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember
Comment 135 Jonathan Druart 2017-07-25 13:40:15 UTC
Created attachment 65218 [details] [review]
Bug 12598: Re-add some missing stuffs

This patch will have to be squashed with previous ones, but for
readability I prefer to let it alone for now.

It reintroduces 2 missing changes:
  commit fd123903651435c56ca14d88897e9ce4d7a38bb5
    Bug 15010: Import patron tool creates 'duplicate' restrictions (
    debarments )
  commit 011d8e9855ffeb1779f5c1aadc9f4367c298cee7
    Bug 12543 - Add userid as matchpoint for "Import patrons" tool
Comment 136 Jonathan Druart 2017-07-25 13:43:48 UTC
(In reply to Colin Campbell from comment #130)
> Code in Koha/Patrons?Import.pm does not reflect latest version of existing
> import. When we tested we fell foul of the fact that it effectively undid
> the changes introduced by bug 12543 - it rejects all records matching on
> userid. As that is probably the most common matchpoint in the sites that do
> regular offline updates its a showstopper. Maybe that other functionality
> present in current load is missing

Hi Colin, thanks for catching that. I have rebased the patches and provide other follow-ups.
Comment 137 Colin Campbell 2017-08-10 14:41:19 UTC
Created attachment 65819 [details] [review]
New import_borrowers.pl command tool
Comment 138 Colin Campbell 2017-08-10 14:42:40 UTC
Created attachment 65820 [details] [review]
Update code to use new modules
Comment 139 Colin Campbell 2017-08-10 14:43:39 UTC
Created attachment 65821 [details] [review]
Fis conflict with bug 16911
Comment 140 Colin Campbell 2017-08-10 14:45:23 UTC
Created attachment 65822 [details] [review]
Fix conflict with 17829 - GetMember
Comment 141 Colin Campbell 2017-08-10 14:47:27 UTC
Created attachment 65823 [details] [review]
Re-add some missing stuff

Signed off these patches after testing
Comment 142 Colin Campbell 2017-08-10 14:55:48 UTC
Created attachment 65824 [details] [review]
Allow matched patron to be updated

Additional patch to be included - currently if matching is on userid and there is a match the update is rejected. Historically the addition of the userid match point and the guard against adding duplicate userids were separate patches merged around the same time without taking account of each other. This rectifies the problem. - Will submit a patch for the existing code separately but will also need including in this enhancement
Comment 143 Katrin Fischer 2017-08-16 16:33:33 UTC
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12598: New misc/import_borrowers.pl command line tool
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt
M	tools/import_borrowers.pl
Falling back to patching base and 3-way merge...
Auto-merging tools/import_borrowers.pl
CONFLICT (content): Merge conflict in tools/import_borrowers.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 12598: New misc/import_borrowers.pl command line tool
The copy of the patch that failed is found in: .git/rebase-apply/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/New-importborrowerspl-command-tool-MjxqyB.patch
Comment 144 Jonathan Druart 2017-08-22 16:35:26 UTC
Created attachment 66353 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool
Comment 145 Jonathan Druart 2017-08-22 16:35:33 UTC
Created attachment 66354 [details] [review]
Bug 12598: Update code to use new modules

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 146 Jonathan Druart 2017-08-22 16:35:41 UTC
Created attachment 66355 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 147 Jonathan Druart 2017-08-22 16:35:49 UTC
Created attachment 66356 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 148 Jonathan Druart 2017-08-22 16:35:56 UTC
Created attachment 66357 [details] [review]
Bug 12598: Re-add some missing stuffs

This patch will have to be squashed with previous ones, but for
readability I prefer to let it alone for now.

It reintroduces 2 missing changes:
  commit fd123903651435c56ca14d88897e9ce4d7a38bb5
    Bug 15010: Import patron tool creates 'duplicate' restrictions (
    debarments )
  commit 011d8e9855ffeb1779f5c1aadc9f4367c298cee7
    Bug 12543 - Add userid as matchpoint for "Import patrons" tool

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 149 Jonathan Druart 2017-08-22 16:36:03 UTC
Created attachment 66358 [details] [review]
Bug 12598 Allow matched patron to be updated

When a test was introduced to not add duplicate userids
it overlooked that if you were matching on userid you
do not want the update to be rejected.

Add a test to overcome this logical flaw if updating
by matching userid
Comment 150 Jonathan Druart 2017-08-22 16:36:11 UTC
Created attachment 66359 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import
Comment 151 Colin Campbell 2017-08-24 13:18:05 UTC
The recently applied to master patch

 Bug 17829: (follow-up) Fix import patron

needs to be reverted inorder for the first of these patch to still apply
Comment 152 Colin Campbell 2017-08-24 14:44:18 UTC
Koha/Patrons/Import.pm

Should have a 'use Koha::Patron::Debarments;'  to avoid runtime errors with undefined subroutine GetDebarments called
Comment 153 Jonathan Druart 2017-08-29 13:32:21 UTC
Created attachment 66575 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool
Comment 154 Jonathan Druart 2017-08-29 13:32:29 UTC
Created attachment 66576 [details] [review]
Bug 12598: Update code to use new modules

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 155 Jonathan Druart 2017-08-29 13:32:36 UTC
Created attachment 66577 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 156 Jonathan Druart 2017-08-29 13:32:43 UTC
Created attachment 66578 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 157 Jonathan Druart 2017-08-29 13:32:49 UTC
Created attachment 66579 [details] [review]
Bug 12598: Re-add some missing stuffs

This patch will have to be squashed with previous ones, but for
readability I prefer to let it alone for now.

It reintroduces 2 missing changes:
  commit fd123903651435c56ca14d88897e9ce4d7a38bb5
    Bug 15010: Import patron tool creates 'duplicate' restrictions (
    debarments )
  commit 011d8e9855ffeb1779f5c1aadc9f4367c298cee7
    Bug 12543 - Add userid as matchpoint for "Import patrons" tool

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 158 Jonathan Druart 2017-08-29 13:32:56 UTC
Created attachment 66580 [details] [review]
Bug 12598 Allow matched patron to be updated

When a test was introduced to not add duplicate userids
it overlooked that if you were matching on userid you
do not want the update to be rejected.

Add a test to overcome this logical flaw if updating
by matching userid
Comment 159 Jonathan Druart 2017-08-29 13:33:04 UTC
Created attachment 66581 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import
Comment 160 Jonathan Druart 2017-08-29 13:33:10 UTC
Created attachment 66582 [details] [review]
Bug 12598: Add use Koha::Patron::debarments statement
Comment 161 Jonathan Druart 2017-08-29 13:55:46 UTC
(In reply to Colin Campbell from comment #151)
> The recently applied to master patch
> 
>  Bug 17829: (follow-up) Fix import patron
> 
> needs to be reverted inorder for the first of these patch to still apply

First patch has been rebased to fix this conflict.

(In reply to Colin Campbell from comment #152)
> Koha/Patrons/Import.pm
> 
> Should have a 'use Koha::Patron::Debarments;'  to avoid runtime errors with
> undefined subroutine GetDebarments called

Fixed in the last patch.
Comment 162 Colin Campbell 2017-09-21 13:22:11 UTC
Created attachment 67276 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import
Comment 163 Colin Campbell 2017-09-21 13:24:30 UTC
Created attachment 67277 [details] [review]
Bug 12598: Add use Koha::Patron::Debarments statement
Comment 164 Colin Campbell 2017-09-21 13:26:28 UTC
Signed off Jonathans patches for Patron lists and import of Debarments
Comment 165 Katrin Fischer 2018-01-03 23:44:59 UTC
I've taken a look at the patches for a first QA review. 

1) Currently fails QA script:

 FAIL	t/db_dependent/Koha/Patrons/Import.t
   FAIL	  critic
		"$headerrow_0" is declared but not used at line 320, column 5. Unused variables clutter code and make it harder to read.

2) Tests fail:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ perl t/db_dependent/Koha/Patrons/Import.t
1..124
ok 1 - use Koha::Patrons::Import;
ok 2 - An object of class 'Koha::Patrons::Import' isa 'Koha::Patrons::Import'
# Subtest: test_methods
    1..1
    ok 1 - Koha::Patrons::Import->can(...)
ok 3 - test_methods
# Subtest: test_attributes
    1..1
    ok 1 - Koha::Patrons::Import->can(...)
ok 4 - test_attributes
ok 5 -  Koha::Patrons::Import->import_patrons carps if no file handle is passed
ok 6 - Got the expected undef from import_patrons with nothing much
ok 7 -  Koha::Patrons::Import->import_patrons carps if no file handle is passed
ok 8 - Got the expected undef from import_patrons with no file handle
Use of uninitialized value $matchpoint in string eq at /home/vagrant/kohaclone/Koha/Patrons/Import.pm line 170, <$handle_1> line 2.
# Looks like your test exited with 255 just after 8.

3) Might be me, but trying to run the script always gives me the help, no error or anything. Used -c and -f with a random file.

Can you please take a look?
Comment 166 Jonathan Druart 2018-01-04 13:07:04 UTC
Created attachment 70265 [details] [review]
Bug 12598: Remove warnings from tests and make them pass
Comment 167 Jonathan Druart 2018-01-04 13:07:19 UTC
Created attachment 70266 [details] [review]
Bug 12598: Use Pod::Usage and make the help more explicit
Comment 168 Jonathan Druart 2018-01-04 13:09:55 UTC
(In reply to Katrin Fischer from comment #165)
> I've taken a look at the patches for a first QA review. 

1 & 2 have been fixed.

> 3) Might be me, but trying to run the script always gives me the help, no
> error or anything. Used -c and -f with a random file.

I improved the POD for the script, --matchpoint is required.
Comment 169 Kyle M Hall 2018-02-16 13:45:16 UTC
Created attachment 71755 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Test Plan:
1) Apply this patch
2) Test importing patrons from command line,
   options are availble with --help.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Tested with minimal csv
(cardnumber,surname,firstname,categorycode,branchcode,password,userid)

Overwrite does not change category or branch.
Patrons are loaded, userid & password works

Updated license to GPLv3
No other koha-qa errors.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Tidy import_borrowers.pl

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Move importing code to a subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Update command line script to use patron import subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 [QA Followup]

* Fix copyright on import_borrowers.pl
* Changes -c --csv to -f --file
* Adds -c --confirm option
* Renames misc/import_borrowers.pl to misc/import_patrons.pl
* Restore userid matchpoint option

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Rebase + backport of 16426 plus fixing 16426

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598: catch warnings raised by import_patrons in tests

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 170 Kyle M Hall 2018-02-16 13:45:51 UTC
Created attachment 71756 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Test Plan:
1) Apply this patch
2) Test importing patrons from command line,
   options are availble with --help.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Tested with minimal csv
(cardnumber,surname,firstname,categorycode,branchcode,password,userid)

Overwrite does not change category or branch.
Patrons are loaded, userid & password works

Updated license to GPLv3
No other koha-qa errors.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Tidy import_borrowers.pl

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Move importing code to a subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Update command line script to use patron import subroutine

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 [QA Followup]

* Fix copyright on import_borrowers.pl
* Changes -c --csv to -f --file
* Adds -c --confirm option
* Renames misc/import_borrowers.pl to misc/import_patrons.pl
* Restore userid matchpoint option

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Fix merge to master. Backport 3 updates from latest import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Started regression tests. Fix missing C4::Members::Attributes package

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - More refactoring and regression tests in Koha::Patrons::Import

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Creating objects in misc/import_patrons.pl and tools/import_borrowers.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Refactoring Koha::Patrons::Import includes bug fixed for critical date types and header column parsing

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598 - Rebase + backport of 16426 plus fixing 16426

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 12598: catch warnings raised by import_patrons in tests

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 171 Kyle M Hall 2018-02-16 13:46:03 UTC
Created attachment 71757 [details] [review]
Bug 12598: Update code to use new modules

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 172 Kyle M Hall 2018-02-16 13:46:08 UTC
Created attachment 71758 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 173 Kyle M Hall 2018-02-16 13:46:14 UTC
Created attachment 71759 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 174 Kyle M Hall 2018-02-16 13:46:20 UTC
Created attachment 71760 [details] [review]
Bug 12598: Re-add some missing stuffs

This patch will have to be squashed with previous ones, but for
readability I prefer to let it alone for now.

It reintroduces 2 missing changes:
  commit fd123903651435c56ca14d88897e9ce4d7a38bb5
    Bug 15010: Import patron tool creates 'duplicate' restrictions (
    debarments )
  commit 011d8e9855ffeb1779f5c1aadc9f4367c298cee7
    Bug 12543 - Add userid as matchpoint for "Import patrons" tool

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 175 Kyle M Hall 2018-02-16 13:46:25 UTC
Created attachment 71761 [details] [review]
Bug 12598 Allow matched patron to be updated

When a test was introduced to not add duplicate userids
it overlooked that if you were matching on userid you
do not want the update to be rejected.

Add a test to overcome this logical flaw if updating
by matching userid
Comment 176 Kyle M Hall 2018-02-16 13:46:30 UTC
Created attachment 71762 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 177 Kyle M Hall 2018-02-16 13:46:36 UTC
Created attachment 71763 [details] [review]
Bug 12598: Add use Koha::Patron::debarments statement

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 178 Kyle M Hall 2018-02-16 13:46:41 UTC
Created attachment 71764 [details] [review]
Bug 12598: Remove warnings from tests and make them pass
Comment 179 Kyle M Hall 2018-02-16 13:46:47 UTC
Created attachment 71765 [details] [review]
Bug 12598: Use Pod::Usage and make the help more explicit
Comment 180 Kyle M Hall 2018-02-16 13:46:53 UTC
Created attachment 71766 [details] [review]
Bug 12598: Fix POD
Comment 181 Jonathan Druart 2018-02-16 16:09:09 UTC
It would have been better to get another QA point of view but I am going push this patch set anyway, it has been waiting for too long.
Comment 182 Jonathan Druart 2018-02-16 17:06:26 UTC
Pushed to master for 18.05, thanks to everybody involved!