From f8b67dfbf61a837fd3269e6bf64897635ebf1402 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Thu, 3 Mar 2022 20:28:28 +0100 Subject: [PATCH 1/6] feat: remove unused parameter "columns" --- frappe/desk/query_report.py | 6 ++---- .../email/doctype/auto_email_report/auto_email_report.py | 4 ++-- frappe/tests/test_query_report.py | 9 +-------- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index a0b0aec255..61913f3e05 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -352,14 +352,12 @@ def export_query(): ) return - columns = get_columns_dict(data.columns) - from frappe.utils.xlsxutils import make_xlsx data["result"] = handle_duration_fieldtype_values( data.get("result"), data.get("columns") ) - xlsx_data, column_widths = build_xlsx_data(columns, data, visible_idx, include_indentation) + xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation) xlsx_file = make_xlsx(xlsx_data, "Query Report", column_widths=column_widths) frappe.response["filename"] = report_name + ".xlsx" @@ -399,7 +397,7 @@ def handle_duration_fieldtype_values(result, columns): return result -def build_xlsx_data(columns, data, visible_idx, include_indentation, ignore_visible_idx=False): +def build_xlsx_data(data, visible_idx, include_indentation, ignore_visible_idx=False): result = [[]] column_widths = [] diff --git a/frappe/email/doctype/auto_email_report/auto_email_report.py b/frappe/email/doctype/auto_email_report/auto_email_report.py index 682f0df7cf..5ffde0c37b 100644 --- a/frappe/email/doctype/auto_email_report/auto_email_report.py +++ b/frappe/email/doctype/auto_email_report/auto_email_report.py @@ -104,7 +104,7 @@ class AutoEmailReport(Document): report_data['columns'] = columns report_data['result'] = data - xlsx_data, column_widths = build_xlsx_data(columns, report_data, [], 1, ignore_visible_idx=True) + xlsx_data, column_widths = build_xlsx_data(report_data, [], 1, ignore_visible_idx=True) xlsx_file = make_xlsx(xlsx_data, "Auto Email Report", column_widths=column_widths) return xlsx_file.getvalue() @@ -113,7 +113,7 @@ class AutoEmailReport(Document): report_data['columns'] = columns report_data['result'] = data - xlsx_data, column_widths = build_xlsx_data(columns, report_data, [], 1, ignore_visible_idx=True) + xlsx_data, column_widths = build_xlsx_data(report_data, [], 1, ignore_visible_idx=True) return to_csv(xlsx_data) else: diff --git a/frappe/tests/test_query_report.py b/frappe/tests/test_query_report.py index 656894fc9b..09596ba44c 100644 --- a/frappe/tests/test_query_report.py +++ b/frappe/tests/test_query_report.py @@ -12,13 +12,6 @@ class TestQueryReport(unittest.TestCase): def test_xlsx_data_with_multiple_datatypes(self): """Test exporting report using rows with multiple datatypes (list, dict)""" - # Describe the columns - columns = { - 0: {"label": "Column A", "fieldname": "column_a"}, - 1: {"label": "Column B", "fieldname": "column_b"}, - 2: {"label": "Column C", "fieldname": "column_c"} - } - # Create mock data data = frappe._dict() data.columns = [ @@ -37,7 +30,7 @@ class TestQueryReport(unittest.TestCase): visible_idx = [0, 2, 3] # Build the result - xlsx_data, column_widths = build_xlsx_data(columns, data, visible_idx, include_indentation=0) + xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation=0) self.assertEqual(type(xlsx_data), list) self.assertEqual(len(xlsx_data), 4) # columns + data From 3c4aa2a3ddeca1f8e11a7697e628d3da0289b77e Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Thu, 3 Mar 2022 20:28:57 +0100 Subject: [PATCH 2/6] refactor: Simplify logic in `handle_duration_fieldtype_values` --- frappe/desk/query_report.py | 40 +++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index 61913f3e05..c9c2d4a4cc 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -371,28 +371,30 @@ def handle_duration_fieldtype_values(result, columns): if isinstance(col, str): col = col.split(":") if len(col) > 1: - if col[1]: - fieldtype = col[1] - if "/" in fieldtype: - fieldtype, options = fieldtype.split("/") - else: - fieldtype = "Data" + if not col[1]: + continue + + fieldtype = col[1] + if "/" in fieldtype: + fieldtype = fieldtype.split("/")[0] else: fieldtype = col.get("fieldtype") - if fieldtype == "Duration": - for entry in range(0, len(result)): - row = result[entry] - if isinstance(row, dict): - val_in_seconds = row[col.fieldname] - if val_in_seconds: - duration_val = format_duration(val_in_seconds) - row[col.fieldname] = duration_val - else: - val_in_seconds = row[i] - if val_in_seconds: - duration_val = format_duration(val_in_seconds) - row[i] = duration_val + if fieldtype != "Duration": + continue + + for entry in range(0, len(result)): + row = result[entry] + if isinstance(row, dict): + val_in_seconds = row[col.fieldname] + if val_in_seconds: + duration_val = format_duration(val_in_seconds) + row[col.fieldname] = duration_val + else: + val_in_seconds = row[i] + if val_in_seconds: + duration_val = format_duration(val_in_seconds) + row[i] = duration_val return result From 8830141faf5b86ae7847330c642df8c2b82c6fea Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Mon, 7 Mar 2022 11:10:21 +0100 Subject: [PATCH 3/6] refactor: reduce code duplication Co-authored-by: gavin --- frappe/desk/query_report.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index c9c2d4a4cc..d9bf4f70fa 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -383,18 +383,11 @@ def handle_duration_fieldtype_values(result, columns): if fieldtype != "Duration": continue - for entry in range(0, len(result)): - row = result[entry] - if isinstance(row, dict): - val_in_seconds = row[col.fieldname] - if val_in_seconds: - duration_val = format_duration(val_in_seconds) - row[col.fieldname] = duration_val - else: - val_in_seconds = row[i] - if val_in_seconds: - duration_val = format_duration(val_in_seconds) - row[i] = duration_val + for row in result: + index = col.fieldname if isinstance(row, dict) else i + val_in_seconds = row[index] + if val_in_seconds: + row[index] = format_duration(val_in_seconds) return result From 39ea6a2074c72156a95fbd6565cd7c6d6610fce5 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Mon, 7 Mar 2022 12:12:13 +0100 Subject: [PATCH 4/6] test: improve test data for query report --- frappe/tests/test_query_report.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/frappe/tests/test_query_report.py b/frappe/tests/test_query_report.py index 09596ba44c..bd01ec2b31 100644 --- a/frappe/tests/test_query_report.py +++ b/frappe/tests/test_query_report.py @@ -15,15 +15,16 @@ class TestQueryReport(unittest.TestCase): # Create mock data data = frappe._dict() data.columns = [ - {"label": "Column A", "fieldname": "column_a"}, - {"label": "Column B", "fieldname": "column_b", "width": 150}, - {"label": "Column C", "fieldname": "column_c", "width": 100} + {"label": "Column A", "fieldname": "column_a", "fieldtype": "Float"}, + {"label": "Column B", "fieldname": "column_b", "width": 100, "fieldtype": "Float"}, + {"label": "Column C", "fieldname": "column_c", "width": 150, "fieldtype": "Duration"}, + "Column D:Data:100" ] data.result = [ - [1.0, 3.0, 5.5], - {"column_a": 22.1, "column_b": 21.8, "column_c": 30.2}, - {"column_b": 5.1, "column_c": 9.5, "column_a": 11.1}, - [3.0, 1.5, 7.5], + [1.0, 3.0, 600, "Test1"], + {"column_a": 22.1, "column_b": 21.8, "column_c": 86412, "column_d": "Test2"}, + {"column_b": 5.1, "column_c": 53234, "column_a": 11.1, "column_d": "Test3"}, + [3.0, 1.5, 333, "Test4"], ] # Define the visible rows @@ -35,7 +36,7 @@ class TestQueryReport(unittest.TestCase): self.assertEqual(type(xlsx_data), list) self.assertEqual(len(xlsx_data), 4) # columns + data # column widths are divided by 10 to match the scale that is supported by openpyxl - self.assertListEqual(column_widths, [0, 15, 10]) + self.assertListEqual(column_widths, [0, 10, 150, 0]) for row in xlsx_data: self.assertEqual(type(row), list) From 2956f0be117321755550fbd7d17ea6625d871e5a Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Mon, 7 Mar 2022 12:28:35 +0100 Subject: [PATCH 5/6] feat: drop support of column definitions as string --- frappe/desk/query_report.py | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index ba26f8affa..f5f50b14fe 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -354,9 +354,7 @@ def export_query(): from frappe.utils.xlsxutils import make_xlsx - data["result"] = handle_duration_fieldtype_values( - data.get("result"), data.get("columns") - ) + format_duration_fields(data) xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation) xlsx_file = make_xlsx(xlsx_data, "Query Report", column_widths=column_widths) @@ -365,31 +363,15 @@ def export_query(): frappe.response["type"] = "binary" -def handle_duration_fieldtype_values(result, columns): - for i, col in enumerate(columns): - fieldtype = None - if isinstance(col, str): - col = col.split(":") - if len(col) > 1: - if not col[1]: - continue - - fieldtype = col[1] - if "/" in fieldtype: - fieldtype = fieldtype.split("/")[0] - else: - fieldtype = col.get("fieldtype") - - if fieldtype != "Duration": +def format_duration_fields(data: frappe._dict) -> None: + for i, col in enumerate(data.columns): + if col.get("fieldtype") != "Duration": continue - for row in result: + for row in data.result: index = col.fieldname if isinstance(row, dict) else i - val_in_seconds = row[index] - if val_in_seconds: - row[index] = format_duration(val_in_seconds) - - return result + if row[index]: + row[index] = format_duration(row[index]) def build_xlsx_data(data, visible_idx, include_indentation, ignore_visible_idx=False): From 45cbacf7b035232de973e1efa8695f96b133124e Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Mon, 7 Mar 2022 12:29:36 +0100 Subject: [PATCH 6/6] test: remove string column --- frappe/tests/test_query_report.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/frappe/tests/test_query_report.py b/frappe/tests/test_query_report.py index bd01ec2b31..2117bc830e 100644 --- a/frappe/tests/test_query_report.py +++ b/frappe/tests/test_query_report.py @@ -18,13 +18,12 @@ class TestQueryReport(unittest.TestCase): {"label": "Column A", "fieldname": "column_a", "fieldtype": "Float"}, {"label": "Column B", "fieldname": "column_b", "width": 100, "fieldtype": "Float"}, {"label": "Column C", "fieldname": "column_c", "width": 150, "fieldtype": "Duration"}, - "Column D:Data:100" ] data.result = [ - [1.0, 3.0, 600, "Test1"], - {"column_a": 22.1, "column_b": 21.8, "column_c": 86412, "column_d": "Test2"}, - {"column_b": 5.1, "column_c": 53234, "column_a": 11.1, "column_d": "Test3"}, - [3.0, 1.5, 333, "Test4"], + [1.0, 3.0, 600], + {"column_a": 22.1, "column_b": 21.8, "column_c": 86412}, + {"column_b": 5.1, "column_c": 53234, "column_a": 11.1}, + [3.0, 1.5, 333], ] # Define the visible rows @@ -36,7 +35,7 @@ class TestQueryReport(unittest.TestCase): self.assertEqual(type(xlsx_data), list) self.assertEqual(len(xlsx_data), 4) # columns + data # column widths are divided by 10 to match the scale that is supported by openpyxl - self.assertListEqual(column_widths, [0, 10, 150, 0]) + self.assertListEqual(column_widths, [0, 10, 15]) for row in xlsx_data: self.assertEqual(type(row), list)