Bug 17359 - Patron import results use wrong encoding
Summary: Patron import results use wrong encoding
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-27 12:39 UTC by Mirko Tietgen
Modified: 2019-11-15 09:26 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
Input file in UTF8 without BOM format (647 bytes, text/plain)
2019-10-09 14:20 UTC, Anke
Details
Import summary (25.02 KB, image/png)
2019-10-09 14:24 UTC, Anke
Details
Patron list (44.02 KB, image/png)
2019-10-09 14:24 UTC, Anke
Details
Patron details (28.43 KB, image/png)
2019-10-09 14:24 UTC, Anke
Details
User import file (202 bytes, text/csv)
2019-10-09 20:38 UTC, Caroline Cyr La Rose
Details
Bug 17359: Add tests (5.96 KB, patch)
2019-10-16 08:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17359: Correct encoding when displaying patron import summary (4.19 KB, patch)
2019-10-16 08:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Test file (with import issue, surname is missing for line 1) (1.83 KB, text/csv)
2019-10-16 08:53 UTC, Jonathan Druart
Details
Bug 17359: Add tests (6.10 KB, patch)
2019-10-28 13:51 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 17359: Correct encoding when displaying patron import summary (4.24 KB, patch)
2019-10-28 13:51 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 17359: Use a unicode character in tests (1.18 KB, patch)
2019-11-05 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17359: Add tests (6.20 KB, patch)
2019-11-05 15:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17359: Correct encoding when displaying patron import summary (4.35 KB, patch)
2019-11-05 15:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17359: Use a unicode character in tests (1.57 KB, patch)
2019-11-05 15:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mirko Tietgen 2016-09-27 12:39:34 UTC
When using the patron import tool, the summary at the end uses wrong character encoding.

Upload a csv with a few fields and include some ü ö ä. To trigger an error, not providing a surname in the file should work, or a non numeric cardnumber.

You will see broken Umlauts on the results page.

(The imported data is ok, only the summary is not)
Comment 1 Mirko Tietgen 2016-09-27 12:47:01 UTC
Card numbers can be non numeric, use a too long one instead. (Limit is 16 I think)
Comment 2 Jonathan Druart 2016-09-28 08:03:10 UTC
Where do you see broken chars on the results page?
I see
"1 imported records (last was ü ö ä / 8422)"
with "ü ö ä" the surname.
Comment 3 Anke 2019-09-27 12:18:31 UTC
As I noticed, the German Umlauts aren't displayed correctly after patron import even in patron names, patron attributes and other fields.
Comment 4 Anke 2019-09-27 12:41:31 UTC
(In reply to Anke from comment #3)
> As I noticed, the German Umlauts aren't displayed correctly after patron
> import even in patron names, patron attributes and other fields.

Update - sorry, got this wrong! If UTF 8 (without BOM") is chosen for encoding, it works.
Comment 5 Anke 2019-09-27 12:59:21 UTC
(In reply to Anke from comment #4)
> (In reply to Anke from comment #3)
> > As I noticed, the German Umlauts aren't displayed correctly after patron
> > import even in patron names, patron attributes and other fields.

Even with UTF 8 chosen for encoding, as mentioned in the documentation, the umlaute are shown correctly in the import "log" list, but in the "real" patron entries they are still shown as question marks.
Comment 6 Katrin Fischer 2019-09-30 09:25:28 UTC
Hi Anke,
could you share a broken sample file here (Add an attachment)? Which version are you testing on?
Comment 7 Anke 2019-10-08 13:41:24 UTC
(In reply to Katrin Fischer from comment #6)
> Hi Anke,
> could you share a broken sample file here (Add an attachment)? Which version
> are you testing on?

Dear Katrin, because of the personal data it seems a bit delicate to send a sample; with some more time I can try to import some fantasy patrons and send them.

Some more infos for now:
- The problem affects not only German umlauts but also accents. Thus, Timón becomes Tim?n in Koha.

- It is not limited to the patrons' names but also to self-defined attributes 
(<koha-url>/cgi-bin/koha/admin/patron-attr-types.pl); e.g.: I defined an attribute "Gruppe" which, for some users has a name with an umlaut as content, e.g. Gruppe: Müller. This shows as Gruppe: M?ller

I thought there might be something wrong with my browser's encoding, but even when I changed this from UTF8 to "Westeuropäisch", the question marks instead of umlauts and accents remain. I also tried a second browser (IE) to make sure my Firefox doesn't cause the problem.

My Koha version is 19.05.02.000.

I got the data from a csv file which I edited with Notepad++, set the encoding to "UTF8 ohne BOM" (just UTF8 will result in an error message when loading the patrons into Koha).

Hope this helps to identify the cause for the bug.
Comment 8 Katrin Fischer 2019-10-08 15:42:16 UTC
Hi Anke, I meant a made up example "Karin Mustermann" or "Donald Duck" of course :) - just to see about the encoding of the file and if there are any differences to how I ususally format it. I'll try to have a look and see if I can replicate.
Comment 9 Anke 2019-10-09 14:20:22 UTC
Created attachment 93926 [details]
Input file in UTF8 without BOM format

Patron import .csv file with fantasy names
Comment 10 Anke 2019-10-09 14:21:39 UTC
OK, I imported some fantasy users into my Koha; I used a comma separated .csv file See attached .csv as well as a screenshot of the import summary (where the umlauts and accents look good) and two more screenshots of the list of patrons and a detailed patron information showing that here, the umlauts and accents aren't displayed correctly.
Comment 11 Anke 2019-10-09 14:24:02 UTC
Created attachment 93927 [details]
Import summary
Comment 12 Anke 2019-10-09 14:24:25 UTC
Created attachment 93928 [details]
Patron list
Comment 13 Anke 2019-10-09 14:24:41 UTC
Created attachment 93929 [details]
Patron details
Comment 14 Caroline Cyr La Rose 2019-10-09 20:37:27 UTC
I was able to reproduce the problem mentionned by Mirko, i.e. that the error summary has encoding problems.
The user's name was Chloé and it came out as Chloé

BUT once imported, that user's file was fine. So I can't reproduce Anke's problem.

For reference, I'm including a one-line file with the name and other information changed, but I kept the same firstname. You can try to import it as is and the error will be that the branchcode and categorycodes don't exist.
Comment 15 Caroline Cyr La Rose 2019-10-09 20:38:08 UTC
Created attachment 93942 [details]
User import file
Comment 16 Katrin Fischer 2019-10-09 20:43:52 UTC
I downloaded Anke's sample file, but my editor (Geany) and LibreOffice both recognize it as "Western ISO-8859-1". So I think something might have gone wrong with encoding conversion, hoping Bugzilla is not to blame here. 

Could you please try this: 

- Open the file in Notepad++
- Check the setting it displays under 'Encoding'
- If it displays as something other than ANSI, change it to ANSI
- Use Encoding > Convert to UTF-8 (without BOM) in the lower half of the menu
- Verify if your Umlauts look correct and that the encoding setting displays right as UTF-8
- Save

I could only check the menu options in the Notepad++ Linux version, so they might be named slightly different in the German/Windows version.

Caroline's file behaves well and displays as UTF-8 in all my programs.
Comment 17 Caroline Cyr La Rose 2019-10-09 20:56:21 UTC
Your file also comes up as ISO 8859-1 on my computer
Comment 18 Anke 2019-10-10 08:43:23 UTC
Thanks to Katrin and Caroline and all!

It was really a question of the encoding I tried to do with Notepad++, so no problem in Koha; encoding as described by Katrin (post of 2019-10-09 20:43:52 UTC) worked, and I apologize for bothering everybody! Sorry indeed!
Comment 19 Katrin Fischer 2019-10-10 08:55:44 UTC
No problem at all, glad we could solve it!
Comment 20 Katrin Fischer 2019-10-15 17:52:39 UTC
(In reply to Caroline Cyr La Rose from comment #14)
> I was able to reproduce the problem mentionned by Mirko, i.e. that the error
> summary has encoding problems.
> The user's name was Chloé and it came out as Chloé

Hi Jonathan, reopening for this issue, something still seems a little odd there.
Comment 21 Jonathan Druart 2019-10-16 08:53:07 UTC
Created attachment 94261 [details] [review]
Bug 17359: Add tests
Comment 22 Jonathan Druart 2019-10-16 08:53:10 UTC
Created attachment 94262 [details] [review]
Bug 17359: Correct encoding when displaying patron import summary

There is an encoding issue when the patron import summry, the string
must be UTF8 decoded before being sent to the template.

Test plan:
- Create a patron import CSV file with import issues. Use UTF8 characters.
See file attached to the bug report 'patron_import.csv'
=> The screen should display the line correctly
- Correct the import issue (add a surname for Chloé)
=> The imported patrons should contain the correct values.
Comment 23 Jonathan Druart 2019-10-16 08:53:56 UTC
Created attachment 94263 [details]
Test file (with import issue, surname is missing for line 1)
Comment 24 Séverine Queune 2019-10-28 13:51:37 UTC
Created attachment 94809 [details] [review]
Bug 17359: Add tests

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 25 Séverine Queune 2019-10-28 13:51:40 UTC
Created attachment 94810 [details] [review]
Bug 17359: Correct encoding when displaying patron import summary

There is an encoding issue when the patron import summry, the string
must be UTF8 decoded before being sent to the template.

Test plan:
- Create a patron import CSV file with import issues. Use UTF8 characters.
See file attached to the bug report 'patron_import.csv'
=> The screen should display the line correctly
- Correct the import issue (add a surname for Chloé)
=> The imported patrons should contain the correct values.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 26 Marcel de Rooy 2019-11-01 08:48:36 UTC
QAing
Comment 27 Marcel de Rooy 2019-11-01 10:12:49 UTC
QA Comment:

my $surname ='Chloé';
my $surname_utf8 = encode_utf8($surname);
=> This name is not the best example for UTF-8. Could be Latin too.
=> You dont do anything with this encoded string.
=> When I am adding some Chinese character in the surname, the test fails.

decode_utf8($borrowerline)
Instead of adding this all over the place, we should try to fix at the source or at least closer.
LINE: while ( my $borrowerline = <$handle> ) {
You could add it here, which would be closer but thats not all. Is the assumption correct that it is UTF-8? We just got a handle from the caller. On the report I am reading about people uploading Latin data.
Actually, we see in tools/import_borrowers.pl:
use CGI qw ( -utf8 );
and
my $handle   = $input->upload('uploadborrowers');
CGI tells me that "binary upload data ... are left untouched".
Do we actually need another parameter for the file encoding?

Looks like this needs some more attention.
Comment 28 Jonathan Druart 2019-11-05 09:29:54 UTC
Created attachment 95042 [details] [review]
Bug 17359: Use a unicode character in tests

And remove an unused variable.
Comment 29 Jonathan Druart 2019-11-05 09:33:15 UTC
(In reply to Marcel de Rooy from comment #27)

Hi Marcel, thanks for the review.

> my $surname ='Chloé';
> my $surname_utf8 = encode_utf8($surname);
> => This name is not the best example for UTF-8. Could be Latin too.
> => You dont do anything with this encoded string.
> => When I am adding some Chinese character in the surname, the test fails.

Looks correct for me, see the last patch. Which tests fail?

> decode_utf8($borrowerline)
> Instead of adding this all over the place, we should try to fix at the
> source or at least closer.
> LINE: while ( my $borrowerline = <$handle> ) {
> You could add it here, which would be closer but thats not all. Is the
> assumption correct that it is UTF-8? We just got a handle from the caller.
> On the report I am reading about people uploading Latin data.
> Actually, we see in tools/import_borrowers.pl:
> use CGI qw ( -utf8 );
> and
> my $handle   = $input->upload('uploadborrowers');
> CGI tells me that "binary upload data ... are left untouched".
> Do we actually need another parameter for the file encoding?

As it seems that there is no encoding issue during the import process I tried to provide a patch that will not have impact on it. That's why I preferred to only modify the lines where @missing_criticals was used.
Comment 30 Marcel de Rooy 2019-11-05 15:26:22 UTC
(In reply to Jonathan Druart from comment #28)
> Created attachment 95042 [details] [review] [review]
> Bug 17359: Use a unicode character in tests
> 
> And remove an unused variable.

 t/db_dependent/Koha/Patrons/Import.t | 2 +-

Where did you remove it? :)
Comment 31 Marcel de Rooy 2019-11-05 15:28:49 UTC
(In reply to Jonathan Druart from comment #29)
> Looks correct for me, see the last patch. Which tests fail?

my $surname ='Chloé覂';

And now it does not fail. So what can I say?
Comment 32 Marcel de Rooy 2019-11-05 15:36:46 UTC
(In reply to Jonathan Druart from comment #29)
> As it seems that there is no encoding issue during the import process I
> tried to provide a patch that will not have impact on it. That's why I
> preferred to only modify the lines where @missing_criticals was used.

I think that Text::CSV resolves the issue for us by guessing the encoding correctly. Formally, we cannot even pass it the encoding because we don't know.
So I can understand your point. And will pass QA.
Comment 33 Marcel de Rooy 2019-11-05 15:39:14 UTC
Created attachment 95057 [details] [review]
Bug 17359: Add tests

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2019-11-05 15:39:18 UTC
Created attachment 95058 [details] [review]
Bug 17359: Correct encoding when displaying patron import summary

There is an encoding issue when the patron import summry, the string
must be UTF8 decoded before being sent to the template.

Test plan:
- Create a patron import CSV file with import issues. Use UTF8 characters.
See file attached to the bug report 'patron_import.csv'
=> The screen should display the line correctly
- Correct the import issue (add a surname for Chloé)
=> The imported patrons should contain the correct values.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2019-11-05 15:39:23 UTC
Created attachment 95059 [details] [review]
Bug 17359: Use a unicode character in tests

And remove an unused variable.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT]
Now really removed it.
Comment 36 Jonathan Druart 2019-11-05 15:50:53 UTC
(In reply to Marcel de Rooy from comment #30)
> (In reply to Jonathan Druart from comment #28)
> > Created attachment 95042 [details] [review] [review] [review]
> > Bug 17359: Use a unicode character in tests
> > 
> > And remove an unused variable.
> 
>  t/db_dependent/Koha/Patrons/Import.t | 2 +-
> 
> Where did you remove it? :)

You... lack imagination! ;)

Thanks!
Comment 37 Martin Renvoize 2019-11-06 17:10:11 UTC
Nice work!

Pushed to master for 19.11.00
Comment 38 Fridolin SOMERS 2019-11-15 09:26:52 UTC
Pushed to 19.05.x for 19.05.05