Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MPDX-8418] Add UTF-8 BOM to CSV exports #1173

Merged
merged 1 commit into from
Nov 6, 2024
Merged

[MPDX-8418] Add UTF-8 BOM to CSV exports #1173

merged 1 commit into from
Nov 6, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Nov 4, 2024

Description

Excel does not interpret UTF-8 encoded CSV files correctly. For example, exports contain characters like this instead of Russian characters: "–ï–¥–∞/–ø–æ–¥–∞—Ä–∫–∏ –¥–ª—è –≤—Å—Ç—Ä–µ—á –£–∂–∏–Ω –° –∫–æ–º–∞–Ω–¥–æ–π –∏ —Å —Å—Ç—É–¥–µ–Ω—Ç–∞–º–∏". To fix this, we need to add the Byte Order Mark to hint to Excel that it needs to interpret the file as UTF-8 instead of as ASCII. Fortunately, react-csv has an option for this. We just have to pass true as the second argument to buildURI.

I also migrated the account transactions CSV download from a handrolled CSV export to use react-csv.

Note that the API already adds the byte order mark to server-generated CSV exports.

Testing

  • 14 Month Reports
    • Create or edit a contact to have a non-ASCII character in its name, like Á.
    • Go to a 14 month report and export it
    • Check in Excel (if available) and macOS Numbers that the CSV file is rendered correctly
    • Alternatively, open the CSV file in VS Code and check that it says "UTF-8 with BOM" in the status bar at the bottom of the window
  • Transactions Report
    • Impersonate Toncho's account
    • Go to a responsibility centers transactions page with transactions (I had to push the start date back to see transactions)
    • Click Export CSV button
    • Check in Excel (if available) and macOS Numbers that the CSV file is rendered correctly
    • Alternatively, open the CSV file in VS Code and check that it says "UTF-8 with BOM" in the status bar at the bottom of the window

MPDX-8418

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac added the Preview Environment Add this label to create an Amplify Preview label Nov 4, 2024
@canac canac requested a review from dr-bizz November 4, 2024 20:26
@canac canac self-assigned this Nov 4, 2024
Copy link
Contributor

github-actions bot commented Nov 4, 2024

Preview branch generated at https://8418-csv-utf8.d3dytjb8adxkk5.amplifyapp.com

Copy link
Contributor

github-actions bot commented Nov 4, 2024

Bundle sizes [mpdx-react]

Compared against 758815f

No significant changes found

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

'href',
encodeURI(csvContent),
);
expect(setAttributeSpy).toHaveBeenCalledWith('href', mockBlobUrl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a shame we are losing testing what the href will actually look like, but it makes sense to just mock the buildURI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's actually a good thing that the component unit test isn't concerning itself with CSV formatting and that we're trusting the react-csv to build a valid URL. If we don't trust the library's test coverage or functionality, we could write our own unit tests to directly test buildURI.

@canac canac enabled auto-merge November 6, 2024 16:13
@canac canac merged commit 5bd236b into main Nov 6, 2024
17 checks passed
@canac canac deleted the 8418-csv-utf8 branch November 6, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants