-
-
Notifications
You must be signed in to change notification settings - Fork 794
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
[13.0][IMP] report_csv: add encoding option #711
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AungKoKoLin1997 Can you please add simple tests with encodings with 'cp932' and something random like 'xyz'?
report_csv/models/ir_report.py
Outdated
@@ -9,6 +9,9 @@ class ReportAction(models.Model): | |||
_inherit = "ir.actions.report" | |||
|
|||
report_type = fields.Selection(selection_add=[("csv", "csv")]) | |||
encoding = fields.Char( | |||
help="Encoding to be applied to the generated CSV file. " "e.g. cp932" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help="Encoding to be applied to the generated CSV file. " "e.g. cp932" | |
help="Encoding to be applied to the generated CSV file. e.g. cp932" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed to include this in the previous review.
@@ -46,6 +46,9 @@ def create_csv_report(self, docids, data): | |||
file = csv.DictWriter(file_data, **self.csv_report_options()) | |||
self.generate_csv_report(file, data, objs) | |||
file_data.seek(0) | |||
encoding = self._context.get("encoding") | |||
if encoding: | |||
return file_data.read().encode(encoding), "csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return file_data.read().encode(encoding), "csv" | |
try: | |
return file_data.read().encode(encoding), "csv" | |
except Exception as e: | |
raise UserError(_("Failed to encode the data with the encoding set in the report.")) |
Please also update USAGE.rst to include a comment about encoding. |
151896d
to
d5bd55f
Compare
report_csv/readme/USAGE.rst
Outdated
<report | ||
id="partner_csv" | ||
model="res.partner" | ||
string="Print to CSV" | ||
report_type="csv" | ||
encoding="cp932" | ||
name="module_name.report_name" | ||
file="res_partner" | ||
attachment_use="False" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the report tag doesn't take the encoding parameter. Did you try this yourself?
We may simply add the following comment instead of a lengthy explanation:
Update encoding
with an appropriate value (e.g. cp932
) as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. You are right. We can only pass value to encoding when we create record with "ir.actions.report".
936b1d6
to
516a4a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍
report_csv/readme/CONFIGURE.rst
Outdated
In order to add encode error handing in csv report you need to choose | ||
encode error handling in reports. | ||
|
||
* Go to Settings > Technical > Reports > Your csv report | ||
* Add encoding first | ||
* Choose encode error handling | ||
|
||
Note: You can't see encode error handling if you don't add encoding first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to add encode error handing in csv report you need to choose | |
encode error handling in reports. | |
* Go to Settings > Technical > Reports > Your csv report | |
* Add encoding first | |
* Choose encode error handling | |
Note: You can't see encode error handling if you don't add encoding first. | |
In case the exported CSV report should be encoded in another system than UTF-8, following fields of the report record (*Settings > Technical > Reports*) should be populated accordingly. | |
* Encoding: set an encoding system (such as cp932) | |
* Encode Error Handling: select 'Ignore' or 'Replace' as necessary. | |
* 'Ignore': in case of an encoding error, the problematic character will be removed from the exported file. | |
* 'Replace': in case of an encoding error, the problematic character will be replaced with '?' symbol. | |
* Leaving the field blank: in case of an encoding error, the report generation fails with an error message. |
@AungKoKoLin1997 Can you go ahead and create PRs for the upstream versions? |
410eda6
to
06257cc
Compare
report_csv/report/report_csv.py
Outdated
if error_handling: | ||
return file_data.read().encode(encoding, errors=error_handling), "csv" | ||
try: | ||
logging.info("Encdonig__________") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging.info("Encdonig__________") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well. Can you update the forward ports with the tests too?
This PR has the |
@AungKoKoLin1997 Can you squash commits? Then I'll merge. |
4237c97
to
9bf1b70
Compare
@thomaspaulb I squashed the commits. Please merge it. Thank you. |
/ocabot merge patch |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 5b91680. Thanks a lot for contributing to OCA. ❤️ |
@qrtl
This PR make an enhancement for generating CSV files with encoding.