Bug 31933 - Import patrons fails when patron attribute value contains a comma
Summary: Import patrons fails when patron attribute value contains a comma
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 21.11
Hardware: All All
: P5 - low minor
Assignee: Jacob O'Mara
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-24 09:18 UTC by Bernard
Modified: 2023-03-17 12:43 UTC (History)
5 users (show)

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


Attachments
patron upload file with the comma (152 bytes, text/csv)
2022-10-24 09:18 UTC, Bernard
Details
Bug 31933: Import patrons fails when patron attribute value contains a comma (2.92 KB, patch)
2023-01-19 12:15 UTC, Jacob O'Mara
Details | Diff | Splinter Review
Bug 31933: Import patrons fails when patron attribute value contains a comma (2.98 KB, patch)
2023-03-14 13:54 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernard 2022-10-24 09:18:07 UTC
Created attachment 142475 [details]
patron upload file with the comma

If patron attribute value contains a comma, the record is not imported
Test:
1. create test.csv with unique cardnumber, mandatory fields surname, branchcode and categorycode and patron attributes
2. Patron attributes to contain a value which includes a comma each key/value pair surrounded by double-quotes 
"COURSE:Government, Economics and Commerce","CSCODE:ABC1234"
3. Ensure patron attribute types COURSE and CSCODE exist
4. Import with default values via patron import tool (cardnumber match)
5. If fails to import 

Import results :
0 imported records
0 overwritten
0 not imported because already in borrowers table and overwrite disabled
1 not imported because they are not in the expected format
1 records parsed
Back to Tools


Feedback:
headerrow : cardnumber, surname, branchcode, categorycode, patron_attributes
attribute string : COURSE:Government, Economics and Commerce
Content-Type : text/csv
Content-Disposition : form-data; name="uploadborrowers"; filename="patron_import_test.csv"
Parsing upload file patron_import_test.csv


Error analysis:
Patron attribute Economics and Commerce is invalid for patron A12364162383.

Repeat above but with file that doesn't have the comma:
"COURSE:Government Economics and Commerce","CSCODE:ABC1234"

Import succeeds

comment: I was able to resolve as follows

$string =~ s/,//g; on line 563 within sub generate_patron_attributes of /usr/share/koha/lib/Koha/Patrons/Import.pm

    # Fixup double quotes in case we are passed smart quotes
    $string =~ s/\xe2\x80\x9c/"/g;
    $string =~ s/\xe2\x80\x9d/"/g;
	+$string =~ s/,//g;
Comment 1 Martin Renvoize (ashimema) 2022-11-28 09:40:38 UTC
We investigated this a little further last week.. It's a bit strange to allow more fields in the rows than in the header that describes which rows are available.

Jacob is working on refining that to give clear errors when we find such cases in the input data and also add a bit more documentation around full quoting with escaped quotes the attributes field.
Comment 2 Jacob O'Mara 2022-11-28 17:26:30 UTC
After some more extensive searching it appears that the issue lies in how smart quotes are being handled within the spreadsheet editor itself prior to uploading to the koha db as I was getting some weird results once saving the csv in LibreOffice and opening in text editor.

The easiest way we discussed to solve this is possibly just to disable smart quotes entirely in the spreadsheet editor and amend the documentation notes to specify the issues between escape quotes and smart quotes. Commas etc then seem to work as expected once disabled and in the format you specified below, bounded within one cell:

"COURSE:Government, Economics and Commerce","CSCODE:ABC1234"

we could probably do some kind of regex fix for the smart quotes issue as you suggested but Martin believes this is something we should discuss further first. 

It appears the error handling does in fact catch the mismatch of columns:values and probably won't need any additional tinkering.

Let me know what you think.
Comment 3 Jacob O'Mara 2023-01-19 12:15:20 UTC
Created attachment 145461 [details] [review]
Bug 31933: Import patrons fails when patron attribute value contains a comma

Following on from our chat I have amended the documentation to reflect the required format for csv files and the issues with smart quotes. I have also included a reference csv download so users have a dummy file they can refer to for the correct format.

Test Plan:
1. Navigate to the Import Patrons tool page
2. Observe the incorrect information in the notes box to the right (blue background)
3. Apply Patch
4. Observe that the documentation has now changed to reflect the above.
5. Test the CSV file download and check is in correct format.
Comment 4 PTFS Europe Sandboxes 2023-03-14 13:54:34 UTC
Created attachment 148166 [details] [review]
Bug 31933: Import patrons fails when patron attribute value contains a comma

Following on from our chat I have amended the documentation to reflect the required format for csv files and the issues with smart quotes. I have also included a reference csv download so users have a dummy file they can refer to for the correct format.

Test Plan:
1. Navigate to the Import Patrons tool page
2. Observe the incorrect information in the notes box to the right (blue background)
3. Apply Patch
4. Observe that the documentation has now changed to reflect the above.
5. Test the CSV file download and check is in correct format.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 5 Sally 2023-03-14 13:55:05 UTC
Works as described, thanks for the patch.
Comment 6 Nick Clemens (kidclamp) 2023-03-17 12:43:10 UTC
The sample attributes is missing a new line when testing in Firefox (on Ubuntu 22.04)

We originally had this section:
The attribute type code and a colon should precede each value. For example: INSTID:12345,LANG:fr or STARTDATE:January 1 2010,TRACK:Day. If an input record has more than one attribute, the fields should either be entered as an unquoted string (previous examples)

Unquoted strings do work in my testing - are you removing this because quoted strings are safer?