Escape characters in CSV

[emartynov]

It is fix for #383.

Changes:

1. Refactored CSV export
2. Escaped '\n', '\r', '"', ',' when generating CSV
3. Extracted one more string constant
4. Added some missing tests

No changes here:

1. Using async task
2. Resume export after user allows write permission

Please review and comment.

[emartynov]

@AndyScherzinger can you review?

[coveralls]

Coverage increased (+1.002%) to 20.148% when pulling a57e9d0 on 383-fix-for-csv-export into 54dd90f on develop.

[AndyScherzinger]

@emartynov sure, I'll give the branch a test drive :)

[AndyScherzinger]

SYMBOLS_TO_ESCAPE actually contains more symbols that get checked (and lead to a return true) than the following line will actually escape.

[emartynov]

Can you elaborate more? I don't get the comment, sorry.

Did I make some logical error?

[AndyScherzinger]

I'll try :) containsSymbolsToEscape checks for a certain set of characters while not all of them get escaped then.

[emartynov]

Can you be more specific? What characters are not escaped?

[AndyScherzinger]

I'll try :)

 if (containsSymbolsToEscape(value)) {
    return "\"" + value.replace("\"", "\"\"") + "\"";
} 

escaping is done for " while the condition containsSymbolsToEscape(value) needs to be met of course. Thus I expected the condition to check for the same pattern. But it actually checks for:

private boolean containsSymbolsToEscape(String value) {
        for (char c : SYMBOLS_TO_ESCAPE) {
            if (value.indexOf(c) > -1) {
                return true;
            }
        }

        return false;
    }

while SYMBOLS_TO_ESCAPE contains:

private static final char[] SYMBOLS_TO_ESCAPE = new char[]{',', '"', '\n', '\r'};

So I would have expected the actual ascaping to also then escape the other "symbols to escape".

[AndyScherzinger]

other than that also when \n has been found the check return true and you are escaping for "

[emartynov]

If , or \n or \r is detected then escaping is wrapping the whole string in the single quotes ". However, if the string contains single quote it should be escaped as double quote "". I hope it helps.

[AndyScherzinger]

Ah 👍
I didn't think of that, now makes send. Thanks for the explanation and your patience 😃

[emartynov]

No issues, I also didn't know it by heart and should, of course, mention it in PR description.

[AndyScherzinger]

Looks good to me code wise, just a minor comment. Will test the export later today after work.

[AndyScherzinger]

Like mentioned in the in line comment only " and ' seem to be escaped, line breaks still exist and won't be escaped. This leads to new lines which will be treated as new rows, e.g. the following export:

Date,Time,Concentration,Preferred Glucose unit,Measured,Notes
1/22/18,21:37,25,mg/dL,After dinner,"Yet
Another
'
""
Test"
1/22/18,21:36,23.5,mg/dL,After dinner,"This, is
A
New line
Test"

When imported into MS Excel (komma delimiter, "-String key):

So I would guess an in-app import of it would still fail (don't know how to do an import :/)

[emartynov]

We don't have import, only export :(

I was quickly checking https://tools.ietf.org/html/rfc4180. It looks like MS Excel is wrong with importing CSV.

I will dive more :)

[AndyScherzinger]

I also send it to my gmail adress and opened it as preview within gmail and there it works fine... so yeah, maybe we just have to ignore Excel...

[emartynov]

Conclusion on this PR?

[AndyScherzinger]

@emartynov fine by me and as discussed excel has its cvs issues but that is excel not glucose :)

[AndyScherzinger]

Hi @emartynov I tried to explain it again, sorry for not getting it clearly explained :( Hope this time I got it right :)

[AndyScherzinger]

All fine by me. Any feedback @paolorotolo?

[paolorotolo]

I'll try it ASAP

[paolorotolo]

Works fine in LibreOffice, good work