Bug 15840 - Import borrowers tool explodes if userid already exists
Summary: Import borrowers tool explodes if userid already exists
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 1861 15842
Blocks: 16426
  Show dependency treegraph
 
Reported: 2016-02-17 12:43 UTC by Mason James
Modified: 2016-12-05 21:25 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
pic (228.26 KB, image/png)
2016-02-17 12:47 UTC, Mason James
Details
Bug 15840: Catch errors if userid already exists when importing patrons (2.26 KB, patch)
2016-02-18 10:56 UTC, Jonathan Druart
Details | Splinter Review
My test patron to attempt with (887 bytes, application/vnd.ms-excel)
2016-02-22 16:53 UTC, M. Tompsett
Details
Patron CSV to trigger (1.05 KB, application/vnd.ms-excel)
2016-02-23 00:25 UTC, M. Tompsett
Details
[SIGNED-OFF] Bug 15840: Catch errors if userid already exists when importing patrons (2.51 KB, patch)
2016-02-23 00:31 UTC, M. Tompsett
Details | Splinter Review
[PASSED QA] Bug 15840: Catch errors if userid already exists when importing patrons (2.57 KB, patch)
2016-03-20 15:38 UTC, Katrin Fischer
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mason James 2016-02-17 12:43:40 UTC
to replicate the bug

1/ import a borrowers.csv file containing any row with a blank 'userid' value,
    - use the 'overwrite' option.

2/ repeat step 1/


koha dies with error..

DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '' for key 'userid' at /path/to/yr/koha/C4/Members.pm line 683


bug confirmed on 3.18.9 -> master 43bcc1c
Comment 1 Mason James 2016-02-17 12:47:15 UTC
Created attachment 48164 [details]
pic
Comment 2 Mason James 2016-02-17 13:27:07 UTC
(In reply to Mason James from comment #0)
> to replicate the bug
> 
> 1/ import a borrowers.csv file containing any row with a blank 'userid'
> value,
>     - use the 'overwrite' option.
> 

actually the bug is triggered using *any* duplicated 'userid' value, not just ""
Comment 3 Mason James 2016-02-17 13:29:34 UTC
(In reply to Mason James from comment #2)
> (In reply to Mason James from comment #0)
> > to replicate the bug
> > 
> > 1/ import a borrowers.csv file containing any row with a blank 'userid'
> > value,
> >     - use the 'overwrite' option.
> > 
> 
> actually the bug is triggered using *any* duplicated 'userid' value, not
> just ""

..which means if you import any .csv file twice (with overwrite option), the error is triggered
Comment 4 Katrin Fischer 2016-02-17 13:30:31 UTC
:(
Comment 5 Jonathan Druart 2016-02-17 14:41:34 UTC
I don't reproduce, because the import patron tool is broken at the moment, by bug 15842.
But looking at the code, I don't understand how it could happen, there is:

 733     $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} )
 734       if ( $data{'userid'} eq '' || !Check_Userid( $data{'userid'} ) );

in C4::Members::AddMember which should avoid that.
Comment 6 Jonathan Druart 2016-02-17 16:21:13 UTC
Ok, got it.
I almost have a patch, what should we do if the userid is still used?
1/ Generate a new one, insert the patron and display a warning
or
2/ Do not insert the patron and display an error
Comment 7 Katrin Fischer 2016-02-17 16:23:00 UTC
What is meant by 'still used' - can you explain?
Comment 8 Jonathan Druart 2016-02-17 16:58:45 UTC
(In reply to Katrin Fischer from comment #7)
> What is meant by 'still used' - can you explain?

s/still/already !
Comment 9 Katrin Fischer 2016-02-18 08:28:34 UTC
>1/ Generate a new one, insert the patron and display a warning
>or
>2/ Do not insert the patron and display an error

Currently you only get the name/borrowernumber of the last patron who couldn't be loaded - which is really not helpful when you try to find out who are the missing ones. I think I a fine with both solutions - but the error/warning should show for each and help you identify them.
Comment 10 Jonathan Druart 2016-02-18 10:00:33 UTC
(In reply to Katrin Fischer from comment #9)
> >1/ Generate a new one, insert the patron and display a warning
> >or
> >2/ Do not insert the patron and display an error
> 
> Currently you only get the name/borrowernumber of the last patron who
> couldn't be loaded - which is really not helpful when you try to find out
> who are the missing ones. I think I a fine with both solutions - but the
> error/warning should show for each and help you identify them.

It's exactly the purpose of bug 5771.
Comment 11 Katrin Fischer 2016-02-18 10:17:28 UTC
I know... but that's been there for a while :(

If we wanted to stick with current the behaviour, I think the user should not be added.
Comment 12 Jonathan Druart 2016-02-18 10:56:47 UTC Comment hidden (obsolete)
Comment 13 M. Tompsett 2016-02-22 05:34:57 UTC
I can't duplicate this.
Could you provide sample user data which causes explosion?
Comment 14 Jonathan Druart 2016-02-22 08:31:14 UTC
I have tried with a csv file containing 3 lines: headers, then 2 lines with patron data.
The patron data contained the same userid.
Comment 15 Jonathan Druart 2016-02-22 08:31:58 UTC
iirc, you need bug 15842.
Comment 16 M. Tompsett 2016-02-22 16:53:54 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-02-22 20:29:39 UTC
Sorry, something was missing in the test plan.
It happens when you modify a patron, when adding, there is a check on the userid (if blank or already exist, generate another one).
Comment 18 M. Tompsett 2016-02-23 00:23:10 UTC
(In reply to Jonathan Druart from comment #17)
> Sorry, something was missing in the test plan.
> It happens when you modify a patron, when adding, there is a check on the
> userid (if blank or already exist, generate another one).

BINGO! Got it. Thanks.
Comment 19 M. Tompsett 2016-02-23 00:25:23 UTC
Created attachment 48306 [details]
Patron CSV to trigger

Sample Data to trigger issue.
Comment 20 M. Tompsett 2016-02-23 00:31:55 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2016-03-20 15:31:11 UTC
> Note that it would be easy to insert the patrons anyway and generate a
> userid based on the surname and firstname.

The current patch keeps the existing behaviour, but it might be nice to do what you suggest here. My only worry is about finding out which patrons have had userids generated by the import. Could we log that maybe?
Comment 22 Katrin Fischer 2016-03-20 15:38:33 UTC
Created attachment 49330 [details] [review]
[PASSED QA] Bug 15840: Catch errors if userid already exists when importing patrons

Import borrowers tool explodes if userid already exists. This error
should be caught to avoid the script to explodes.

TEST PLAN
---------
1) back up DB
2) drop DB, create empty DB
3) Apply patch 15842
4) web install all the sample data
5) login as system user and import patron data
   -- sample provided to trigger error.
   -- you will get a DBIx::class exception
      "Duplicate entry ' for key 'userid'"
6) Apply this patch
7) Repeat steps 2-5
   -- patrons won't be inserted but an error
      will be nicely displayed in the error list.
8) run koha qa test tools

Note that it would be easy to insert the patrons anyway and generate a
userid based on the surname and firstname.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2016-03-20 15:39:37 UTC
Note: it seems without the patch the patrons will silently be assigend new userids when duplicate - I couldn't reproduce the perl error.
Comment 24 Brendan Gallagher 2016-03-21 21:25:05 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 25 Julian Maurice 2016-03-23 07:07:31 UTC
Patch pushed in 3.22.x, will be in 3.22.5
Comment 26 Frédéric Demians 2016-03-23 12:51:36 UTC
Pushed to 3.20.x, will be in 3.20.10
Comment 27 Nick Clemens 2016-04-21 00:49:41 UTC
I think I am seeing a problem here, that error trips whenever there is no username or the username exists.  

1 - We should be able to import files with no username,
2 - Username existence should also check that the existing name doesn't belong to the matching patron
Comment 28 Jonathan Druart 2016-05-03 08:10:06 UTC
(In reply to Nick Clemens from comment #27)
> I think I am seeing a problem here, that error trips whenever there is no
> username or the username exists.  
> 
> 1 - We should be able to import files with no username,
> 2 - Username existence should also check that the existing name doesn't
> belong to the matching patron

Frédéric & Julian, I have submited a patch on bug 16426 to fix this issue, stay tuned for the backport!