Bug 5379 - import_borrowers.pl fails with db insert/update errors
Summary: import_borrowers.pl fails with db insert/update errors
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Assignee: Chris Nighswonger
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-09 15:34 UTC by Chris Nighswonger
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Example of a CSV which causes db errors (1.03 KB, text/csv)
2010-11-12 13:21 UTC, Chris Nighswonger
Details
csv file with example record (513 bytes, text/plain)
2010-11-15 13:58 UTC, Katrin Fischer
Details
Corrected CSV (518 bytes, text/plain)
2010-11-16 14:49 UTC, Chris Nighswonger
Details
[SIGNED OFF] Bug 5379 - import_borrowers.pl fails with db insert/update errors (6.85 KB, patch)
2011-06-15 16:55 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Nighswonger 2010-11-09 15:34:07 UTC
I'm not sure what is going on here exactly; the gonenoaddress, lost, debarred, guarantorid, and flags db fields default to NULL, but for some reason when the insert/update occurs, DBD returns an error like "Incorrect integer value: '' for column 'foo'..." It appears save to default these to zero for now.

This patch also adds more logging of errors and a notice to the user to check the logs for errors when they occur.
Comment 1 Chris Nighswonger 2010-11-09 15:38:42 UTC
Patch can be had here:

http://git.koha-community.org/gitweb/?p=wip/koha-fbc.git;a=shortlog;h=refs/heads/k_bug_5379
Comment 2 Galen Charlton 2010-11-12 12:45:10 UTC
Regarding gonenoaddress, lost, etc., not being set to a default value of zero, I have not been able to reproduce that problem.  Are you testing with MariaDB by any chance?
Comment 3 Chris Nighswonger 2010-11-12 13:20:26 UTC
(In reply to comment #2)
> Regarding gonenoaddress, lost, etc., not being set to a default value of zero,
> I have not been able to reproduce that problem.  Are you testing with MariaDB
> by any chance?

You are right. I will break out that part of this patch and submit it under bug 5366.

I am also attaching a copy of the CSV causing errors to this bug to help ensure proper replication of the problem.
Comment 4 Chris Nighswonger 2010-11-12 13:21:13 UTC
Created attachment 2742 [details]
Example of a CSV which causes db errors
Comment 5 Chris Nighswonger 2010-11-12 13:32:32 UTC
(In reply to comment #0)
> I'm not sure what is going on here exactly; the gonenoaddress, lost, debarred,
> guarantorid, and flags db fields default to NULL, but for some reason when the
> insert/update occurs, DBD returns an error like "Incorrect integer value: ''
> for column 'foo'..." It appears save to default these to zero for now.
> 
> This patch also adds more logging of errors and a notice to the user to check
> the logs for errors when they occur.

DB errors are also thrown for extended patron attributes when entered in the form:

"ATTRIB1:value1", "ATTRIB2:value2", etc.

This is due to a problem introduced by smart quotes.
Comment 6 Chris Cormack 2010-11-14 20:44:20 UTC
Patches pushed to new/awaiting_qa/bug_5379, which is a merge onto master of the k_bug_5379 branch of Chris Nighswongers repository.

Will continue to push any patches to pull requests to this branch.
Comment 7 Katrin Fischer 2010-11-15 09:01:52 UTC
Not sure I tested the right thing, but here are my results ;)

I tested on current HEAD and on the new branch new/awaiting_qa/bug_5379 with different test cases for 2 patron attributes. The file I used was loaded successfully before in 3.01.61.

I used a unique patron attribute as matching key during import, overwriting matching records, replacing all patron attributes.

"BORROWER#:222,SECOND_ID:222"
Overwriting to existing patrons with attributes works without problems.

--- 

"BORROWER#:223,SECOND_ID:222"
For testing I changed one of the matching keys, so that one patron would be added as new to the database. I got following error message for this patron:
1 not imported because they are not in the expected format (last...)

I think the error is due to the already existing cardnumber in my database. After I changed the cardnubmer too, the patron loaded without problems.

A better error message would be good.

---

"BORROWER#:222","SECOND_ID:224"
I changed my patron attributes in an attempt to follow the help instructions.

This resulted in deletion of my second patron attribute from the overwritten patron record. No error message was displayed, although the number of fields was not matching my header row.

The help seems not correct here or is misleading?

---

"BORROWER#:222,'SECOND_ID:224, test'"
Deleted both patron attributes from the existing patron.

---

"'BORROWER#:222','SECOND_ID:224, test'"
1 not imported because they are not in the expected format ..


What is the right syntax to import more than 1 patron attribute with patron attributes containing commas?
Comment 8 Chris Nighswonger 2010-11-15 13:15:10 UTC
(In reply to comment #7)
> Not sure I tested the right thing, but here are my results ;)

<snip>

> 
> What is the right syntax to import more than 1 patron attribute with patron
> attributes containing commas?

Take a look at the csv attached to this bug. The record in it has two patron attributes, the first of which contains a comma in the date format.

I agree that the help on this screen is confusing. I'll follow up with a patch to hopefully help clarify it later this week.
Comment 9 Chris Nighswonger 2010-11-15 13:19:37 UTC
(In reply to comment #7)
> Not sure I tested the right thing, but here are my results ;)
> 

<snip>

> 
> "BORROWER#:222","SECOND_ID:224"
> I changed my patron attributes in an attempt to follow the help instructions.
> 
> This resulted in deletion of my second patron attribute from the overwritten
> patron record. No error message was displayed, although the number of fields
> was not matching my header row.
> 
> The help seems not correct here or is misleading?
> 

What where the "Patron Attributes" settings (near the bottom of the import tool screen) set to?
Comment 10 Katrin Fischer 2010-11-15 13:27:17 UTC
I used these settings for all of my tests:

- overwrite matching records
- replace all patron attributes
Comment 11 Katrin Fischer 2010-11-15 13:58:56 UTC
Created attachment 2755 [details]
csv file with example record

File attached will have following results:
"BORROWER#:222","SECOND_ID:224"

- patron already exists: overwrite and replace all will delete second_id from the records
- patron is new: only the first patron_attribute will be created

Help file should be changed. I wondered why the program didn't complain about the difference between header and provided fields.
Comment 12 Katrin Fischer 2010-11-15 14:18:59 UTC
Ok, I have to correct my former report: the patron attribute is not deleted but written into the next field of the header row. 

More tests with small changes to the provided csv file:

"'BORROWER#:222','SECOND_ID:224'"
1 not imported because they are not in the expected format (last was Fischer / AddMember)

'"BORROWER#:222","SECOND_ID:224"'
1 not imported because they are not in the expected format

And one last change, to match the format in chris_n's example file:
"“STARTDATE:January 1, 2010”,”TRACK:Day”" changed to:
"“BORROWER#:222”,”SECOND_ID:222, 444”"
And that works.
Comment 13 Chris Nighswonger 2010-11-16 14:49:29 UTC
Created attachment 2760 [details]
Corrected CSV
Comment 14 Chris Nighswonger 2010-11-16 14:55:46 UTC
The problem is most certainly due to poorly worded help.

According to RFC-4180:

If double-quotes are used to enclose fields, then a double-quote appearing inside a field must be escaped by preceding it with another double quote.  For example:

       "aaa","b""bb","ccc"

We are essentially embedding a CSV inside of a single field of another CSV, so we must properly escape our double quotes. This means in a plain text editor our patron attribute field should follow this form:

"""ATTRIB_1:VALUE_1"",""ATTRIB_2:VALUE_2"",""ATTRIB_3:VALUE_3"",... ""ATTRIB_N:VALUE_N"""

In a spreadsheet editor this would be:

""ATTRIB_1:VALUE_1"",""ATTRIB_2:VALUE_2"",""ATTRIB_3:VALUE_3"",... ""ATTRIB_N:VALUE_N""

My testing shows that this syntax works.

NOTE: Excel *does not* double quote fields afaict, so it may be necessary to follow the first example when using Excel. This needs to be tested.

I will follow up with a patch to clarify the help in this tool.
Comment 15 Chris Nighswonger 2010-11-16 15:08:26 UTC
Adding a link to RFC-4180

http://www.rfc-editor.org/rfc/rfc4180.txt
Comment 16 Katrin Fischer 2010-11-22 12:19:47 UTC
I tested the uploaded csv file. New import of record and update of existing record worked correctly. It's also possible to import patron attributes containing commas.

Example:
"""BORROWER#:222"",""SECOND_ID:224, test"""

The help file and the hints on the page should be very clear about the right syntax, so people know exactly how the file has to look like.
Comment 18 Liz Rea 2011-06-15 16:55:06 UTC
Created attachment 4486 [details] [review]
[SIGNED OFF] Bug 5379 - import_borrowers.pl fails with db insert/update errors

Some spreadsheet programs use smart quotes which causes the db to throw
an error when an insert/update is attempted due to improper processing
of the CSV file. This patch adds code to check for smart quotes and change
them to "dumb" quotes.

This patch also adds more logging of errors and a notice to the user to check
the logs for errors when they occur.
Comment 19 Chris Cormack 2011-07-04 23:16:20 UTC
Works as designed, pushing