Bug 12598

*** Bug 5633 has been marked as a duplicate of this bug. ***

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?

(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.

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?

(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.

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.

(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 ; )

(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.

(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.

*** Bug 3340 has been marked as a duplicate of this bug. ***

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

Sorry for the initial screw-up on the order of sign-offs. This is absolutely a valuable addition for automated setups!

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?

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.

(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.

(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? :)

(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!

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

Kyle, could you please rebase this patch?

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.

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.

(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

(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.

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.

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.

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.

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.

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.

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)

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.

Created attachment 51372 [details]
Patrons sample data

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.

Is it ready to be signed?

It is good enough for now. :-)

(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'?

(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.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 52333 [details] [review]
Bug 12598 - Rebase + backport of 16426 plus fixing 16426

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

Created attachment 52540 [details] [review]
Bug 12598: catch warnings raised by import_patrons in tests

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.

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.

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

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?

(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).

(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.

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

Created attachment 64438 [details] [review]
Bug 12598: Update code to use new modules

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.

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

Created attachment 64855 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change

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

Created attachment 65214 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Created attachment 65215 [details] [review]
Bug 12598: Update code to use new modules

Created attachment 65216 [details] [review]
Bug 12598: Fix conflict with bug 16911

Tests will have to be written for this change

Created attachment 65217 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

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

(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.

Created attachment 65819 [details] [review]
New import_borrowers.pl command tool

Created attachment 65820 [details] [review]
Update code to use new modules

Created attachment 65821 [details] [review]
Fis conflict with bug 16911

Created attachment 65822 [details] [review]
Fix conflict with 17829 - GetMember

Created attachment 65823 [details] [review]
Re-add some missing stuff

Signed off these patches after testing

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

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

Created attachment 66353 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Created attachment 66354 [details] [review]
Bug 12598: Update code to use new modules

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

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>

Created attachment 66356 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

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>

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

Created attachment 66359 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import

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

Koha/Patrons/Import.pm

Should have a 'use Koha::Patron::Debarments;'  to avoid runtime errors with undefined subroutine GetDebarments called

Created attachment 66575 [details] [review]
Bug 12598: New misc/import_borrowers.pl command line tool

Created attachment 66576 [details] [review]
Bug 12598: Update code to use new modules

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

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>

Created attachment 66578 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

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>

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

Created attachment 66581 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import

Created attachment 66582 [details] [review]
Bug 12598: Add use Koha::Patron::debarments statement

(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.

Created attachment 67276 [details] [review]
Bug 12598: Re-add bug 18555: Create patron list from patron import

Created attachment 67277 [details] [review]
Bug 12598: Add use Koha::Patron::Debarments statement

Signed off Jonathans patches for Patron lists and import of Debarments

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?

Created attachment 70265 [details] [review]
Bug 12598: Remove warnings from tests and make them pass

Created attachment 70266 [details] [review]
Bug 12598: Use Pod::Usage and make the help more explicit

(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.

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>

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>

Created attachment 71757 [details] [review]
Bug 12598: Update code to use new modules

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

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>

Created attachment 71759 [details] [review]
Bug 12598: Fix conflict with 17829 - GetMember

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

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>

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

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>

Created attachment 71763 [details] [review]
Bug 12598: Add use Koha::Patron::debarments statement

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

Created attachment 71764 [details] [review]
Bug 12598: Remove warnings from tests and make them pass

Created attachment 71765 [details] [review]
Bug 12598: Use Pod::Usage and make the help more explicit

Created attachment 71766 [details] [review]
Bug 12598: Fix POD

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.

Pushed to master for 18.05, thanks to everybody involved!