[Bug 15840] New: Fix 'import borrowers' tool to not die when importing a file with blank 'userid' values
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Bug ID: 15840 Summary: Fix 'import borrowers' tool to not die when importing a file with blank 'userid' values Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Database Assignee: gmcharlt@gmail.com Reporter: mtj@kohaaloha.com QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #1 from Mason James <mtj@kohaaloha.com> --- Created attachment 48164 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48164&action=edit pic -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |1861 See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=1861, | |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=8397 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=1861 [Bug 1861] Unique patrons logins not (totally) enforced -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |major Priority|P5 - low |P3 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #2 from Mason James <mtj@kohaaloha.com> --- (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 "" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #3 from Mason James <mtj@kohaaloha.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- :( -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15842 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- What is meant by 'still used' - can you explain? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Katrin Fischer from comment #7)
What is meant by 'still used' - can you explain?
s/still/already ! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> ---
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=5771 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Fix 'import borrowers' tool |Import borrowers tool |to not die when importing a |explodes if userid already |file with blank 'userid' |exists |values | Assignee|gmcharlt@gmail.com |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 48205 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48205&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- I can't duplicate this. Could you provide sample user data which causes explosion? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I have tried with a csv file containing 3 lines: headers, then 2 lines with patron data. The patron data contained the same userid. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15842 See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15842 | --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- iirc, you need bug 15842. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15842 [Bug 15842] Cannot import patrons if the csv file does not contain privacy_guarantor_checkouts -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 48303 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48303&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #18 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #48303|0 |1 is obsolete| | --- Comment #19 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 48306 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48306&action=edit Patron CSV to trigger Sample Data to trigger issue. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #48205|0 |1 is obsolete| | --- Comment #20 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 48307 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48307&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bob@calyx.net.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Bug 15840 depends on bug 15842, which changed state. Bug 15842 Summary: Cannot import patrons if the csv file does not contain privacy_guarantor_checkouts https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15842 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> ---
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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #48307|0 |1 is obsolete| | --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 49330 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49330&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 --- Comment #23 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Note: it seems without the patch the patrons will silently be assigend new userids when duplicate - I couldn't reproduce the perl error. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |brendan@bywatersolutions.co | |m --- Comment #24 from Brendan Gallagher <brendan@bywatersolutions.com> --- Pushed to Master - Should be in the May 2016 release. Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |julian.maurice@biblibre.com --- Comment #25 from Julian Maurice <julian.maurice@biblibre.com> --- Patch pushed in 3.22.x, will be in 3.22.5 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #26 from Frédéric Demians <frederic@tamil.fr> --- Pushed to 3.20.x, will be in 3.20.10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #27 from Nick Clemens <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16426 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15840 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16426 See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16426 | --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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! Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16426 [Bug 16426] Import borrowers tool warns for blank and/or existing userids -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org