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

Add support to display FR column headings in reports CSVs #1681

Merged
merged 11 commits into from
Oct 4, 2023

Conversation

whabanks
Copy link
Contributor

@whabanks whabanks commented Sep 28, 2023

Summary | Résumé

This PR adds support for displaying French translated column headers in downloaded report CSVs. It also cleans up the headers for consistency, converting email_address and phone_number to Email address and Phone number for consistent case and styling in the headings.

Screenshot 2023-10-03 at 10 17 54 AM

Screenshot 2023-10-03 at 10 18 45 AM

Test instructions | Instructions pour tester la modification

Test 1 - Basic sends

  1. Create and send two bulk sends, one Email and one SMS
  2. Navigate to the jobs page and switch the language to French
  3. Download the report
  4. Note that the column headings are in French and the case and spacing for email address and phone number are correct
  5. Note that the column values for Status and Type are in French

Test 2 - Custom columns

  1. Create and send two bulk sends, one Email and one SMS both with custom headings
  2. Navigate to the jobs page and switch the language to French
  3. Download the report
  4. Note that the column headings are in French and the Case and spacing for email address and phone number are correct
  5. Note that custom column headings are preserved as they were input by the user

Test Downloaded CSVs

  1. Open both an SMS and Email CSV in MS Excel.
  2. Note that accented characters are rendered properly
  3. Check for any other encoding issues related to accented FR characters.

- Converts email_address to Email address
- Converts phone_number to Phone number
@whabanks whabanks changed the title Add support to display FR column headings in reports csvs Add support to display FR column headings in reports CSVs Sep 28, 2023
@github-actions
Copy link

@whabanks whabanks marked this pull request as ready for review October 3, 2023 14:51
- Fetch translations using fr.csv
Copy link
Member

@andrewleith andrewleith 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. Tested and scenarios come out as described!

One side note (probably outside the scope of this PR) is that when you open up these CSV files in excel, the french characters get garbled and it looks pretty rough. It seems like excel doesnt think the file is in the right encoding:

image

@andrewleith
Copy link
Member

andrewleith commented Oct 3, 2023

One side note (probably outside the scope of this PR) is that when you open up these CSV files in excel, the french characters get garbled and it looks pretty rough. It seems like excel doesnt think the file is in the right encoding:

For future work: I was playing around trying to fix this and it seems like if you yield the BOM before the rest of the CSV content this gets fixed, not sure if there are any negative side effects though. I added the following line to utils.py/generate_notifications_csv(): yield u'\uFEFF'.encode('utf-8') before the first yield statement.

Source: https://stackoverflow.com/questions/37013433/proper-rendering-of-flask-produced-csv-files-in-excel

@whabanks
Copy link
Contributor Author

whabanks commented Oct 3, 2023

One side note (probably outside the scope of this PR) is that when you open up these CSV files in excel, the french characters get garbled and it looks pretty rough. It seems like excel doesnt think the file is in the right encoding:

For future work: I was playing around trying to fix this and it seems like if you yield the BOM before the rest of the CSV content this gets fixed, not sure if there are any negative side effects though. I added the following line to utils.py/generate_notifications_csv(): yield u'\uFEFF'.encode('utf-8') before the first yield statement.

Source: https://stackoverflow.com/questions/37013433/proper-rendering-of-flask-produced-csv-files-in-excel

Sweet catch, I will play around with this - having garbled text like that is less than ideal and a bad user experience. Seems like a straightforward enough fix to include with this PR.

@whabanks whabanks requested a review from andrewleith October 4, 2023 17:58
@jzbahrai
Copy link
Contributor

jzbahrai commented Oct 4, 2023

For placeholder, this is the template:
Screenshot 2023-10-04 at 2 56 20 PM

But the spreadsheet didnt preserve the placeholder name?
Screenshot 2023-10-04 at 2 57 12 PM

@jzbahrai
Copy link
Contributor

jzbahrai commented Oct 4, 2023

Works for excel file!

@whabanks whabanks merged commit 646c359 into main Oct 4, 2023
@whabanks whabanks deleted the localize-job-report-csv branch October 4, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants