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

Use REDASH_CSV_WRITER_ENCODING for downloading CSV #5826

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kaibadash
Copy link
Contributor

@kaibadash kaibadash commented Sep 13, 2022

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

REDASH_CSV_WRITER_ENCODING environment variable is ignored and always downloads as UTF-8.
We, engineers may not understand what the problem is that.

The administrative staffs in Japan like to edit the CSV directly in Excel and upload it to another system. Excel needs to be encoded in CP932(Shift-JIS) to edit it directly, so we need to be able to set the character encoding.

I think all be solved if Excel supported UTF-8, but Microsoft never do that and Japanese administrative staffs can only use Excel... 😫

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Sorry for the Japanese documentation. There are people who are having the same problem as I am.
https://qiita.com/koike_moyashi/items/d0d5dc37a93b398f0aaa

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@@ -135,42 +135,6 @@ def build_url(request, host, path):
return "{}://{}{}".format(request.scheme, host, path)


class UnicodeWriter:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UnicodeWriter no longer appears to be in use.

@@ -37,6 +38,7 @@
serialize_job,
)

WRITER_ENCODING = os.environ.get("REDASH_CSV_WRITER_ENCODING", "utf-8")
Copy link
Member

Choose a reason for hiding this comment

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

We should put it in the settings.

Copy link
Member

Choose a reason for hiding this comment

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

@junnplus I guess you mean the list of environment variable strings?

https://redash.io/help/open-source/admin-guide/env-vars-settings

If so, then definitely yes. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

@junnplus junnplus force-pushed the fix_using_redash_csv_writer_encoding branch from 7a5530b to ff188a6 Compare July 7, 2023 23:32
@guidopetri
Copy link
Contributor

@kaibadash thank you for your contribution! Would you mind resolving the merge conflicts, as well as adding the env var in the documentation here?

@kaibadash kaibadash force-pushed the fix_using_redash_csv_writer_encoding branch 2 times, most recently from 36060bd to 73c25bd Compare August 6, 2023 03:55
@kaibadash
Copy link
Contributor Author

kaibadash commented Aug 6, 2023

Thank you for reviewing. I resolved conflicts.

adding the env var in the documentation here?

I created a pull request.
getredash/website#679

@konnectr
Copy link
Collaborator

konnectr commented Aug 6, 2023

@kaibadash Could you add tests ?

@konnectr
Copy link
Collaborator

konnectr commented Aug 6, 2023

You need to run make format

@@ -37,6 +38,7 @@
serialize_job,
)

WRITER_ENCODING = os.environ.get("REDASH_CSV_WRITER_ENCODING", "utf-8")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaibadash kaibadash marked this pull request as draft October 16, 2023 06:13
…badash/redash into fix_using_redash_csv_writer_encoding
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #5826 (2286624) into master (6b98197) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master    #5826   +/-   ##
=======================================
  Coverage   61.26%   61.26%           
=======================================
  Files         158      158           
  Lines       12889    12890    +1     
  Branches     1755     1755           
=======================================
+ Hits         7896     7897    +1     
  Misses       4743     4743           
  Partials      250      250           
Files Coverage Δ
redash/settings/__init__.py 98.66% <100.00%> (+<0.01%) ⬆️
redash/handlers/query_results.py 81.57% <0.00%> (ø)

@kaibadash
Copy link
Contributor Author

kaibadash commented Oct 19, 2023

I tried to add tests in test_query_results.py . I'm not a pythonista, that was difficult for me... Could someone help me?

class TestMakeCsvResponse(BaseTestCase):
    # TODO: How can I mock CSV_WRITER_ENCODING ?
    def test_make_csv_response_with_cp932_encoding(self):
        # TODO: How can I get query_result with multi byte string?
        query_result = self.factory.create_query_result()
        response = self.make_csv_response(query_result)
        # TODO: How can I get result ?
        self.assertEqual(response["TODO"], "こんにちは means hello in Japanese".encode("cp932"))

@justinclift
Copy link
Member

@guidopetri Do you have time to help out here? 😄

@guidopetri guidopetri self-assigned this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants