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
Created attachment 48164 [details] pic
(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 ""
(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
:(
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.
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
What is meant by 'still used' - can you explain?
(In reply to Katrin Fischer from comment #7) > What is meant by 'still used' - can you explain? s/still/already !
>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.
(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.
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.
Created attachment 48205 [details] [review] 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: Import a csv file with the same userid used Without this patch, you will get a DBIx::class exception "Duplicate entry ' for key 'userid'" With this patch applied, these patrons won't be inserted but an error will be nicely displayed in the error list. Note that it would be easy to insert the patrons anyway and generate a userid based on the surname and firstname.
I can't duplicate this. Could you provide sample user data which causes explosion?
I have tried with a csv file containing 3 lines: headers, then 2 lines with patron data. The patron data contained the same userid.
iirc, you need bug 15842.
Created attachment 48303 [details] My test patron to attempt with import -> cardnumber -> ignore import -> cardnumber -> overwrite import -> username -> ignore import -> username -> overwrite All I get is the annoying "[Mon Feb 22 11:49:14.646982 2016] [cgi:error] [pid 2623] [client 192.168.71.222:54060] AH01215: [Mon Feb 22 11:49:14 2016] import_borrowers.pl: CGI::param called in list context from package C4::Auth line 401, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436., referer: http://library-admin.debiankoha.ca/cgi-bin/koha/tools/import_borrowers.pl" flooding my log, and no explosions. (see bug 15747 for that fix)
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).
(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.
Created attachment 48306 [details] Patron CSV to trigger Sample Data to trigger issue.
Created attachment 48307 [details] [review] [SIGNED-OFF] 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>
> 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?
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>
Note: it seems without the patch the patrons will silently be assigend new userids when duplicate - I couldn't reproduce the perl error.
Pushed to Master - Should be in the May 2016 release. Thanks!
Patch pushed in 3.22.x, will be in 3.22.5
Pushed to 3.20.x, will be in 3.20.10
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
(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!