[16.0][REF][ADD] report_csv, report_format_option #864

AungKoKoLin1997 · 2024-03-28T01:23:41Z

This PR refactors the encoding part of the report_csv module into a new module to enable the use of encoding features across multiple reporting modules by adding this new module as a dependency.

@qrtl QT4120

report_format_option/models/ir_actions_report.py

+    show_encoding = fields.Boolean(compute="_compute_show_encoding")
+    line_ending = fields.Char(
+        help="Line ending to be applied to the generated report. e.g., crlf"
+    )


yostashiro · 2024-03-29T13:59:36Z

report_format_option/models/ir_actions_report.py

+    _inherit = "ir.actions.report"
+
+    encoding = fields.Char(
+        help="Encoding to be applied to the generated CSV file. " "e.g. cp932"


Suggested change

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"

yostashiro · 2024-03-29T14:01:19Z

report_format_option/models/ir_actions_report.py

+    )
+    show_encoding = fields.Boolean(compute="_compute_show_encoding")
+    line_ending = fields.Char(
+        help="Line ending to be applied to the generated report. e.g., crlf"


nitpick

Suggested change

help="Line ending to be applied to the generated report. e.g., crlf"

help="Line ending to be applied to the generated report. e.g. crlf"

yostashiro · 2024-03-29T14:01:35Z

report_format_option/readme/DESCRIPTION.rst

@@ -0,0 +1,2 @@
+This is a technical module to add encoding fields to ir.actions.report model and it does nothing by itself.
+The module is expected to be a dependency of other reporting modules (e.g., report_csv).


nitpick

Suggested change

The module is expected to be a dependency of other reporting modules (e.g., report_csv).

The module is expected to be a dependency of other reporting modules (e.g. report_csv).

yostashiro

Code review + checked presentation in runboat.

yostashiro · 2024-04-01T15:02:03Z

report_format_option/models/ir_actions_report.py

+        compute="_compute_show_encoding",
+        help="Technical field to control the visibility of the encoding field.",
+    )
+    line_ending = fields.Char(


@AungKoKoLin1997 Can we actually make it a selection field, pre-defining 'LF', 'CRLF' and 'CR'?

I think we can also extend method _render_qweb_text() in this module to handle the conversion of line ending and encoding.

report_format_option/models/ir_actions_report.py

+    def _compute_show_encoding(self):
+        """Extend this method to show the encoding field in the report form."""
+        for report in self:
+            report.show_encoding = False


report_format_option/models/ir_actions_report.py

+    def _compute_show_line_ending(self):
+        """Extend this method to show the line ending field in the report form."""
+        for report in self:
+            report.show_line_ending = False


yostashiro · 2024-04-02T03:34:14Z

report_format_option/models/ir_actions_report.py

+        [("LF", "LF (\\n)"), ("CRLF", "CRLF (\\r\\n)"), ("CR", "CR (\\r)")],
+        default="LF",
+        help="Select the type of line ending.",


Suggested change

[("LF", "LF (\\n)"), ("CRLF", "CRLF (\\r\\n)"), ("CR", "CR (\\r)")],

default="LF",

help="Select the type of line ending.",

[("crlf", "CRLF (\\r\\n)"), ("cr", "CR (\\r)")],

help="Select the type of line ending in case the report needs to be output with other line ending than 'LF'.",

yostashiro · 2024-04-02T03:46:10Z

report_format_option/models/ir_actions_report.py

+        report = self._get_report(report_ref)
+        # If specific encoding is set on the report, use it; otherwise, fallback to utf-8
+        encoding = report.encoding or "utf-8"
+        # Decode content assuming it is 'utf-8' to work with it as a string
+        content_str = content.decode("utf-8")
+        # Adjust line endings if specified, while content is still a string
+        line_ending = report.line_ending or ""
+        if line_ending == "CRLF":
+            content_str = content_str.replace("\n", "\r\n")
+        elif report.line_ending == "CR":
+            content_str = content_str.replace("\n", "\r")
+        # Then encode with the desired encoding
+        encode_options = {}
+        if report.encode_error_handling:
+            encode_options["errors"] = report.encode_error_handling
+        # Re-encode the content with or without the errors parameter
+        content_encoded = content_str.encode(encoding, **encode_options)
+        return content_encoded, content_type


Suggested change

report = self._get_report(report_ref)

# If specific encoding is set on the report, use it; otherwise, fallback to utf-8

encoding = report.encoding or "utf-8"

# Decode content assuming it is 'utf-8' to work with it as a string

content_str = content.decode("utf-8")

# Adjust line endings if specified, while content is still a string

line_ending = report.line_ending or ""

if line_ending == "CRLF":

content_str = content_str.replace("\n", "\r\n")

elif report.line_ending == "CR":

content_str = content_str.replace("\n", "\r")

# Then encode with the desired encoding

encode_options = {}

if report.encode_error_handling:

encode_options["errors"] = report.encode_error_handling

# Re-encode the content with or without the errors parameter

content_encoded = content_str.encode(encoding, **encode_options)

return content_encoded, content_type

if not report.encoding and not report.line_ending:

return content, content_type

report = self._get_report(report_ref)

content_str = content.decode("utf-8")

if report.line_ending == "crlf":

content_str = content_str.replace("\n", "\r\n")

elif report.line_ending == "cr":

content_str = content_str.replace("\n", "\r")

# If specific encoding is set on the report, use it; otherwise, fallback to utf-8

encoding = report.encoding or "utf-8"

encode_options = {}

if report.encode_error_handling:

encode_options["errors"] = report.encode_error_handling

content = content_str.encode(encoding, **encode_options)

return content, content_type

yostashiro · 2024-04-02T03:52:31Z

@AungKoKoLin1997 Can you please add tests to report_format_option?

yostashiro

Please apply the suggested change, squash commits and update the commit message with rationale.

yostashiro · 2024-04-03T05:05:48Z

report_format_option/readme/CONFIGURE.rst

+* Line Ending: Select the type of line ending—'CRLF', 'CR' or leave blank, as necessary.
+  * 'CRLF': Stands for 'Carriage Return' + 'Line Feed'. It's used as a line ending in Windows, signifying the end of a line and the start of a new one.
+  * 'CR': Stands for 'Carriage Return'. It's used in classic Mac OS as a line ending, moving the cursor to the beginning of the line.
+  * Leaving this field blank will default to using 'LF' (Line Feed), which is the standard line ending in Unix/Linux systems, denoting the end of a line.


Let's make it less verbose (and more maintainable).

Suggested change

* Line Ending: Select the type of line ending—'CRLF', 'CR' or leave blank, as necessary.

* 'CRLF': Stands for 'Carriage Return' + 'Line Feed'. It's used as a line ending in Windows, signifying the end of a line and the start of a new one.

* 'CR': Stands for 'Carriage Return'. It's used in classic Mac OS as a line ending, moving the cursor to the beginning of the line.

* Leaving this field blank will default to using 'LF' (Line Feed), which is the standard line ending in Unix/Linux systems, denoting the end of a line.

* Line Ending: Select the type of line ending, 'CRLF' or 'CR', as necessary.

* 'CRLF': 'Carriage Return' + 'Line Feed' (Windows)

* 'CR': 'Carriage Return' (classic Mac OS)

* Leaving this field blank defaults to using 'LF' (Line Feed), the default behavior of Odoo.

yostashiro

Code review. LGTM.

kanda999 · 2024-04-03T09:54:42Z

report_format_option/demo/report_demo.xml

+
+    <!-- Demo report template -->
+    <template id="demo_report_template">
+        <t t-call="web.html_container">


Does this text report need to be html report?

Yes. You right. We don't need it.

This commit refactors the encoding part of the report_csv module into a new module(report_format_option) to enable the use of encoding features across multiple reporting modules by adding report_format_option as a dependency.

kanda999

Functinal and code review

OCA-git-bot · 2024-04-04T02:16:52Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

HviorForgeFlow · 2024-05-27T09:35:31Z

Question @pedrobaeza
How should we merge this PR? Seems that report_csv needs some major version bump while the new module does not.

pedrobaeza

Well, there's no debate, as you can't do such change.

pedrobaeza · 2024-05-27T09:37:58Z

report_csv/__manifest__.py

@@ -8,9 +8,8 @@
    "category": "Reporting",
    "version": "16.0.2.0.0",
    "license": "AGPL-3",
-    "depends": ["base", "web"],
+    "depends": ["base", "web", "report_format_option"],


This is incorrect, as you can't change dependencies of a merged module.

AFAIK the purpose is to split a feature to be reusable by other report modules out of the scope of the CSV.
There should be a way to achieve this change.

@pedrobaeza Do you mean that this goes against the stable policy and report_csv should stay as is? In that case, we will try to do this in the 17.0 migration, and we may just propose to add report_format_option in 16.0 anyway.

Yes, you can't do it on an existing version, or you will break existing installations. I also see that the extracted code is not worth for such "reusing", but that's only my opinion.

Fine by me. We need report_format_option anyway for a client to apply some format conversions on text reports. That's why we proposed to split the common feature off report_csv to avoid redundancy. We will try to do this in 17.0.

@AungKoKoLin1997 Can you please close this PR and create another one just to add report_format_option?

AungKoKoLin1997 force-pushed the 16.0-ref-report_csv branch from 38e13fe to e09e97b Compare March 28, 2024 01:28

yostashiro reviewed Mar 29, 2024

View reviewed changes

yostashiro force-pushed the 16.0-ref-report_csv branch from 1dca959 to 10e001e Compare March 30, 2024 07:32

yostashiro approved these changes Mar 30, 2024

View reviewed changes

yostashiro reviewed Apr 1, 2024

View reviewed changes

AungKoKoLin1997 requested a review from yostashiro April 2, 2024 03:24

yostashiro suggested changes Apr 2, 2024

View reviewed changes

yostashiro reviewed Apr 3, 2024

View reviewed changes

AungKoKoLin1997 changed the title ~~[16.0][REF] report_csv: refactor encoding part into new module~~ [16.0][REF][ADD] report_csv, report_format_option Apr 3, 2024

AungKoKoLin1997 force-pushed the 16.0-ref-report_csv branch 2 times, most recently from ededd9a to 9ae9c03 Compare April 3, 2024 06:05

yostashiro approved these changes Apr 3, 2024

View reviewed changes

kanda999 reviewed Apr 3, 2024

View reviewed changes

[REF][ADD] report_csv, report_format_option

23fc5df

This commit refactors the encoding part of the report_csv module into a new module(report_format_option) to enable the use of encoding features across multiple reporting modules by adding report_format_option as a dependency.

AungKoKoLin1997 force-pushed the 16.0-ref-report_csv branch from 9ae9c03 to 23fc5df Compare April 4, 2024 02:04

kanda999 approved these changes Apr 4, 2024

View reviewed changes

OCA-git-bot added approved ready to merge labels Apr 4, 2024

yostashiro mentioned this pull request Apr 18, 2024

[17.0][MIG] report_csv #856

Open

pedrobaeza requested changes May 27, 2024

View reviewed changes

OCA-git-bot removed approved ready to merge labels May 27, 2024

pedrobaeza added this to the 16.0 milestone May 27, 2024

AungKoKoLin1997 closed this May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][REF][ADD] report_csv, report_format_option #864

[16.0][REF][ADD] report_csv, report_format_option #864

AungKoKoLin1997 commented Mar 28, 2024

This comment was marked as outdated.

yostashiro Mar 29, 2024

yostashiro Mar 29, 2024

yostashiro Mar 29, 2024

yostashiro left a comment

yostashiro Apr 1, 2024 •

edited

This comment was marked as resolved.

This comment was marked as resolved.

yostashiro Apr 2, 2024

yostashiro Apr 2, 2024

yostashiro commented Apr 2, 2024

yostashiro left a comment

yostashiro Apr 3, 2024

yostashiro left a comment

kanda999 Apr 3, 2024

AungKoKoLin1997 Apr 4, 2024

kanda999 left a comment

OCA-git-bot commented Apr 4, 2024

HviorForgeFlow commented May 27, 2024

pedrobaeza left a comment

pedrobaeza May 27, 2024

HviorForgeFlow May 27, 2024

yostashiro May 27, 2024

pedrobaeza May 27, 2024

yostashiro May 27, 2024

	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"

	help="Line ending to be applied to the generated report. e.g., crlf"
	help="Line ending to be applied to the generated report. e.g. crlf"

		@@ -0,0 +1,2 @@
		This is a technical module to add encoding fields to ir.actions.report model and it does nothing by itself.
		The module is expected to be a dependency of other reporting modules (e.g., report_csv).

[16.0][REF][ADD] report_csv, report_format_option #864

[16.0][REF][ADD] report_csv, report_format_option #864

Conversation

AungKoKoLin1997 commented Mar 28, 2024

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro left a comment

Choose a reason for hiding this comment

yostashiro Apr 1, 2024 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro commented Apr 2, 2024

yostashiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanda999 left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Apr 4, 2024

HviorForgeFlow commented May 27, 2024

pedrobaeza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro Apr 1, 2024 •

edited