From 26534118e44131117afae01fd50cd8797ab9e105 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Fri, 28 Oct 2022 19:54:03 +0200 Subject: [PATCH 01/72] feat: CSV params for report view --- frappe/desk/reportview.py | 10 ++++--- .../js/frappe/views/reports/report_view.js | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 679b052baf..d5ac82ac40 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -14,7 +14,7 @@ from frappe.model import child_table_fields, default_fields, optional_fields from frappe.model.base_document import get_controller from frappe.model.db_query import DatabaseQuery from frappe.model.utils import is_virtual_doctype -from frappe.utils import add_user_info, cstr, format_duration +from frappe.utils import add_user_info, cint, cstr, format_duration @frappe.whitelist() @@ -349,7 +349,11 @@ def export_query(): add_totals_row = None file_format_type = form_params["file_format_type"] title = title or doctype + csv_delimiter = cstr(form_params.get("csv_delimiter", ",")) + csv_quoting = cint(form_params.get("csv_quoting", 2)) + del form_params["csv_delimiter"] + del form_params["csv_quoting"] del form_params["doctype"] del form_params["file_format_type"] @@ -384,14 +388,12 @@ def export_query(): data = handle_duration_fieldtype_values(doctype, data, db_query.fields) if file_format_type == "CSV": - - # convert to csv import csv from frappe.utils.xlsxutils import handle_html f = StringIO() - writer = csv.writer(f) + writer = csv.writer(f, quoting=csv_quoting, delimiter=csv_delimiter) for r in data: # encode only unicode type strings and not int, floats etc. writer.writerow([handle_html(frappe.as_unicode(v)) if isinstance(v, str) else v for v in r]) diff --git a/frappe/public/js/frappe/views/reports/report_view.js b/frappe/public/js/frappe/views/reports/report_view.js index af7e518678..9976b34544 100644 --- a/frappe/public/js/frappe/views/reports/report_view.js +++ b/frappe/public/js/frappe/views/reports/report_view.js @@ -1489,6 +1489,27 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { options: ["Excel", "CSV"], default: "Excel", }, + { + fieldtype: "Data", + label: __("Delimiter"), + fieldname: "csv_delimiter", + default: ",", + length: 1, + depends_on: "eval:doc.file_format_type=='CSV'", + }, + { + fieldtype: "Select", + label: __("Quoting"), + fieldname: "csv_quoting", + options: [ + { value: 0, label: "Minimal" }, + { value: 1, label: "All" }, + { value: 2, label: "Non-numeric" }, + { value: 3, label: "None" }, + ], + default: 2, + depends_on: "eval:doc.file_format_type=='CSV'", + }, ]; if (this.total_count > this.count_without_children || args.page_length) { @@ -1508,6 +1529,11 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { args.file_format_type = data.file_format_type; args.title = this.report_name || this.doctype; + if (data.file_format_type == "CSV") { + args.csv_delimiter = data.csv_delimiter; + args.csv_quoting = data.csv_quoting; + } + if (this.add_totals_row) { args.add_totals_row = 1; } From 40593ac1be492efb1d8bad180b82ff9ed1fac9bd Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Fri, 28 Oct 2022 19:54:33 +0200 Subject: [PATCH 02/72] feat: CSV params for query report --- frappe/desk/query_report.py | 66 +++++++++------ .../js/frappe/views/reports/query_report.js | 83 +++++++++++-------- 2 files changed, 87 insertions(+), 62 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index 877fdbe5bc..f8f296f747 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -5,6 +5,7 @@ import datetime import json import os from datetime import timedelta +from io import StringIO import frappe import frappe.desk.reportview @@ -332,47 +333,60 @@ def get_prepared_report_result(report, filters, dn="", user=None): @frappe.whitelist() def export_query(): """export from query reports""" - data = frappe._dict(frappe.local.form_dict) - data.pop("cmd", None) - data.pop("csrf_token", None) + form_params = frappe._dict(frappe.local.form_dict) + form_params.pop("cmd", None) + form_params.pop("csrf_token", None) - if isinstance(data.get("filters"), str): - filters = json.loads(data["filters"]) + if isinstance(form_params.get("filters"), str): + filters = json.loads(form_params["filters"]) - if data.get("report_name"): - report_name = data["report_name"] + if form_params.get("report_name"): + report_name = form_params["report_name"] frappe.permissions.can_export( frappe.get_cached_value("Report", report_name, "ref_doctype"), raise_exception=True, ) - file_format_type = data.get("file_format_type") - custom_columns = frappe.parse_json(data.get("custom_columns", "[]")) - include_indentation = data.get("include_indentation") - visible_idx = data.get("visible_idx") + file_format_type = form_params.get("file_format_type") + custom_columns = frappe.parse_json(form_params.get("custom_columns", "[]")) + include_indentation = form_params.get("include_indentation") + visible_idx = form_params.get("visible_idx") + csv_delimiter = cstr(form_params.get("csv_delimiter", ",")) + csv_quoting = cint(form_params.get("csv_quoting", 2)) if isinstance(visible_idx, str): visible_idx = json.loads(visible_idx) - if file_format_type == "Excel": - data = run(report_name, filters, custom_columns=custom_columns) - data = frappe._dict(data) - if not data.columns: - frappe.respond_as_web_page( - _("No data to export"), - _("You can try changing the filters of your report."), - ) - return + data = run(report_name, filters, custom_columns=custom_columns) + data = frappe._dict(data) + if not data.columns: + frappe.respond_as_web_page( + _("No data to export"), + _("You can try changing the filters of your report."), + ) + return + format_duration_fields(data) + xlsx_data, column_widths = build_xlsx_data(data, visible_idx, include_indentation) + + if file_format_type == "CSV": + import csv + + file = StringIO() + writer = csv.writer(file, quoting=csv_quoting, delimiter=csv_delimiter) + writer.writerows(xlsx_data) + content = file.getvalue().encode("utf-8") + file_extension = "csv" + elif file_format_type == "Excel": from frappe.utils.xlsxutils import make_xlsx - 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) + file = make_xlsx(xlsx_data, "Query Report", column_widths=column_widths) + file_extension = "xlsx" + content = file.getvalue() - frappe.response["filename"] = report_name + ".xlsx" - frappe.response["filecontent"] = xlsx_file.getvalue() - frappe.response["type"] = "binary" + frappe.response["filename"] = f"{report_name}.{file_extension}" + frappe.response["filecontent"] = content + frappe.response["type"] = "binary" def format_duration_fields(data: frappe._dict) -> None: diff --git a/frappe/public/js/frappe/views/reports/query_report.js b/frappe/public/js/frappe/views/reports/query_report.js index 1febf11b6c..5db7e14f9b 100644 --- a/frappe/public/js/frappe/views/reports/query_report.js +++ b/frappe/public/js/frappe/views/reports/query_report.js @@ -1421,6 +1421,27 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { default: "Excel", reqd: 1, }, + { + fieldtype: "Data", + label: __("Delimiter"), + fieldname: "csv_delimiter", + default: ",", + length: 1, + depends_on: "eval:doc.file_format=='CSV'", + }, + { + fieldtype: "Select", + label: __("Quoting"), + fieldname: "csv_quoting", + options: [ + { value: 0, label: "Minimal" }, + { value: 1, label: "All" }, + { value: 2, label: "Non-numeric" }, + { value: 3, label: "None" }, + ], + default: 2, + depends_on: "eval:doc.file_format=='CSV'", + }, ]; if (this.tree_report) { @@ -1433,45 +1454,35 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { this.export_dialog = frappe.prompt( export_dialog_fields, - ({ file_format, include_indentation }) => { + ({ file_format, include_indentation, csv_delimiter, csv_quoting }) => { this.make_access_log("Export", file_format); - if (file_format === "CSV") { - const column_row = this.columns.reduce((acc, col) => { - if (!col.hidden) { - acc.push(__(col.label)); - } - return acc; - }, []); - const data = this.get_data_for_csv(include_indentation); - const out = [column_row].concat(data); - frappe.tools.downloadify(out, null, this.report_name); - } else { - let filters = this.get_filter_values(true); - if (frappe.urllib.get_dict("prepared_report_name")) { - filters = Object.assign( - frappe.urllib.get_dict("prepared_report_name"), - filters - ); - } - - const visible_idx = this.datatable.bodyRenderer.visibleRowIndices; - if (visible_idx.length + 1 === this.data.length) { - visible_idx.push(visible_idx.length); - } - - const args = { - cmd: "frappe.desk.query_report.export_query", - report_name: this.report_name, - custom_columns: this.custom_columns.length ? this.custom_columns : [], - file_format_type: file_format, - filters: filters, - visible_idx, - include_indentation, - }; - - open_url_post(frappe.request.url, args); + let filters = this.get_filter_values(true); + if (frappe.urllib.get_dict("prepared_report_name")) { + filters = Object.assign( + frappe.urllib.get_dict("prepared_report_name"), + filters + ); } + + const visible_idx = this.datatable.bodyRenderer.visibleRowIndices; + if (visible_idx.length + 1 === this.data.length) { + visible_idx.push(visible_idx.length); + } + + const args = { + cmd: "frappe.desk.query_report.export_query", + report_name: this.report_name, + custom_columns: this.custom_columns.length ? this.custom_columns : [], + file_format_type: file_format, + filters: filters, + visible_idx, + csv_delimiter, + csv_quoting, + include_indentation, + }; + + open_url_post(frappe.request.url, args); }, __("Export Report: {0}", [this.report_name]), __("Download") From 685eb419eb800a971179266c5e1cade5d7af4ab6 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Sat, 29 Oct 2022 17:17:05 +0200 Subject: [PATCH 03/72] refactor: utility function get_export_dialog --- .../js/frappe/views/reports/query_report.js | 58 +++++------------ .../js/frappe/views/reports/report_utils.js | 44 +++++++++++++ .../js/frappe/views/reports/report_view.js | 62 +++++-------------- 3 files changed, 77 insertions(+), 87 deletions(-) diff --git a/frappe/public/js/frappe/views/reports/query_report.js b/frappe/public/js/frappe/views/reports/query_report.js index 5db7e14f9b..f71391a1c3 100644 --- a/frappe/public/js/frappe/views/reports/query_report.js +++ b/frappe/public/js/frappe/views/reports/query_report.js @@ -1412,48 +1412,20 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { return; } - let export_dialog_fields = [ - { - label: __("Select File Format"), - fieldname: "file_format", - fieldtype: "Select", - options: ["Excel", "CSV"], - default: "Excel", - reqd: 1, - }, - { - fieldtype: "Data", - label: __("Delimiter"), - fieldname: "csv_delimiter", - default: ",", - length: 1, - depends_on: "eval:doc.file_format=='CSV'", - }, - { - fieldtype: "Select", - label: __("Quoting"), - fieldname: "csv_quoting", - options: [ - { value: 0, label: "Minimal" }, - { value: 1, label: "All" }, - { value: 2, label: "Non-numeric" }, - { value: 3, label: "None" }, - ], - default: 2, - depends_on: "eval:doc.file_format=='CSV'", - }, - ]; - + let extra_fields = null; if (this.tree_report) { - export_dialog_fields.push({ - label: __("Include indentation"), - fieldname: "include_indentation", - fieldtype: "Check", - }); + extra_fields = [ + { + label: __("Include indentation"), + fieldname: "include_indentation", + fieldtype: "Check", + }, + ]; } - this.export_dialog = frappe.prompt( - export_dialog_fields, + this.export_dialog = frappe.report_utils.get_export_dialog( + __(this.report_name), + extra_fields, ({ file_format, include_indentation, csv_delimiter, csv_quoting }) => { this.make_access_log("Export", file_format); @@ -1483,10 +1455,12 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { }; open_url_post(frappe.request.url, args); - }, - __("Export Report: {0}", [this.report_name]), - __("Download") + + this.export_dialog.hide(); + } ); + + this.export_dialog.show(); } get_data_for_csv(include_indentation) { diff --git a/frappe/public/js/frappe/views/reports/report_utils.js b/frappe/public/js/frappe/views/reports/report_utils.js index b2444ce07f..904443e403 100644 --- a/frappe/public/js/frappe/views/reports/report_utils.js +++ b/frappe/public/js/frappe/views/reports/report_utils.js @@ -158,4 +158,48 @@ frappe.report_utils = { }; return get_result[fn](values); }, + + get_export_dialog(report_name, extra_fields, callback) { + const fields = [ + { + label: __("Select File Format"), + fieldname: "file_format", + fieldtype: "Select", + options: ["Excel", "CSV"], + default: "Excel", + reqd: 1, + }, + { + fieldtype: "Data", + label: __("Delimiter"), + fieldname: "csv_delimiter", + default: ",", + length: 1, + depends_on: "eval:doc.file_format=='CSV'", + }, + { + fieldtype: "Select", + label: __("Quoting"), + fieldname: "csv_quoting", + options: [ + { value: 0, label: "Minimal" }, + { value: 1, label: "All" }, + { value: 2, label: "Non-numeric" }, + { value: 3, label: "None" }, + ], + default: 2, + depends_on: "eval:doc.file_format=='CSV'", + }, + ]; + if (extra_fields) { + fields.push(...extra_fields); + } + + return new frappe.ui.Dialog({ + title: __("Export Report: {0}", [report_name]), + fields: fields, + primary_action_label: __("Download"), + primary_action: callback, + }); + }, }; diff --git a/frappe/public/js/frappe/views/reports/report_view.js b/frappe/public/js/frappe/views/reports/report_view.js index 9976b34544..511d2ca945 100644 --- a/frappe/public/js/frappe/views/reports/report_view.js +++ b/frappe/public/js/frappe/views/reports/report_view.js @@ -1481,55 +1481,27 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { action: () => { const args = this.get_args(); const selected_items = this.get_checked_items(true); - let fields = [ - { - fieldtype: "Select", - label: __("Select File Type"), - fieldname: "file_format_type", - options: ["Excel", "CSV"], - default: "Excel", - }, - { - fieldtype: "Data", - label: __("Delimiter"), - fieldname: "csv_delimiter", - default: ",", - length: 1, - depends_on: "eval:doc.file_format_type=='CSV'", - }, - { - fieldtype: "Select", - label: __("Quoting"), - fieldname: "csv_quoting", - options: [ - { value: 0, label: "Minimal" }, - { value: 1, label: "All" }, - { value: 2, label: "Non-numeric" }, - { value: 3, label: "None" }, - ], - default: 2, - depends_on: "eval:doc.file_format_type=='CSV'", - }, - ]; - if (this.total_count > this.count_without_children || args.page_length) { - fields.push({ - fieldtype: "Check", - fieldname: "export_all_rows", - label: __("Export All {0} rows?", [(this.total_count + "").bold()]), - }); + let extra_fields = null; + if (this.total_count > (this.count_without_children || args.page_length)) { + extra_fields = [ + { + fieldtype: "Check", + fieldname: "export_all_rows", + label: __("Export All {0} rows?", [`${this.total_count}`]), + }, + ]; } - const d = new frappe.ui.Dialog({ - title: __("Export Report: {0}", [__(this.doctype)]), - fields: fields, - primary_action_label: __("Download"), - primary_action: (data) => { + const d = frappe.report_utils.get_export_dialog( + __(this.doctype), + extra_fields, + (data) => { args.cmd = "frappe.desk.reportview.export_query"; - args.file_format_type = data.file_format_type; + args.file_format_type = data.file_format; args.title = this.report_name || this.doctype; - if (data.file_format_type == "CSV") { + if (data.file_format == "CSV") { args.csv_delimiter = data.csv_delimiter; args.csv_quoting = data.csv_quoting; } @@ -1553,8 +1525,8 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { open_url_post(frappe.request.url, args); d.hide(); - }, - }); + } + ); d.show(); }, From a232e7f0c17212833adcd50b88ecd2f61238c654 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Sat, 29 Oct 2022 18:05:52 +0200 Subject: [PATCH 04/72] fix: don't parse CSV params for Excel --- frappe/desk/reportview.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index d5ac82ac40..e392816d35 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -349,11 +349,12 @@ def export_query(): add_totals_row = None file_format_type = form_params["file_format_type"] title = title or doctype - csv_delimiter = cstr(form_params.get("csv_delimiter", ",")) - csv_quoting = cint(form_params.get("csv_quoting", 2)) + if file_format_type == "CSV": + csv_delimiter = cstr(form_params.get("csv_delimiter", ",")) + csv_quoting = cint(form_params.get("csv_quoting", 2)) + del form_params["csv_delimiter"] + del form_params["csv_quoting"] - del form_params["csv_delimiter"] - del form_params["csv_quoting"] del form_params["doctype"] del form_params["file_format_type"] From 62d68a8be644412915e5e53b8e392aeb4a8ac7eb Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Sat, 29 Oct 2022 18:07:51 +0200 Subject: [PATCH 05/72] refactor: reuse functions from reportview --- frappe/desk/query_report.py | 39 +++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index f8f296f747..1f8233510c 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -11,6 +11,7 @@ import frappe import frappe.desk.reportview from frappe import _ from frappe.core.utils import ljust_list +from frappe.desk.reportview import clean_params, parse_json from frappe.model.utils import render_include from frappe.modules import get_module_path, scrub from frappe.monitor import add_data_to_monitor @@ -334,30 +335,24 @@ def get_prepared_report_result(report, filters, dn="", user=None): def export_query(): """export from query reports""" form_params = frappe._dict(frappe.local.form_dict) - form_params.pop("cmd", None) - form_params.pop("csrf_token", None) + clean_params(form_params) + parse_json(form_params) - if isinstance(form_params.get("filters"), str): - filters = json.loads(form_params["filters"]) + report_name = form_params.report_name + frappe.permissions.can_export( + frappe.get_cached_value("Report", report_name, "ref_doctype"), + raise_exception=True, + ) - if form_params.get("report_name"): - report_name = form_params["report_name"] - frappe.permissions.can_export( - frappe.get_cached_value("Report", report_name, "ref_doctype"), - raise_exception=True, - ) - - file_format_type = form_params.get("file_format_type") - custom_columns = frappe.parse_json(form_params.get("custom_columns", "[]")) - include_indentation = form_params.get("include_indentation") - visible_idx = form_params.get("visible_idx") - csv_delimiter = cstr(form_params.get("csv_delimiter", ",")) - csv_quoting = cint(form_params.get("csv_quoting", 2)) + file_format_type = form_params.file_format_type + custom_columns = frappe.parse_json(form_params.custom_columns or "[]") + include_indentation = form_params.include_indentation + visible_idx = form_params.visible_idx if isinstance(visible_idx, str): visible_idx = json.loads(visible_idx) - data = run(report_name, filters, custom_columns=custom_columns) + data = run(report_name, form_params.filters, custom_columns=custom_columns) data = frappe._dict(data) if not data.columns: frappe.respond_as_web_page( @@ -373,7 +368,13 @@ def export_query(): import csv file = StringIO() - writer = csv.writer(file, quoting=csv_quoting, delimiter=csv_delimiter) + writer = csv.writer( + file, + quoting=csv.QUOTE_NONNUMERIC + if form_params.csv_quoting is None + else cint(form_params.csv_quoting), + delimiter=cstr(form_params.csv_delimiter or ","), + ) writer.writerows(xlsx_data) content = file.getvalue().encode("utf-8") file_extension = "csv" From 87298f5d76de129ce619f1696f1663a8ea6cac31 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Thu, 3 Nov 2022 12:02:16 +0100 Subject: [PATCH 06/72] test: export query report as CSV --- frappe/tests/test_query_report.py | 41 ++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_query_report.py b/frappe/tests/test_query_report.py index 7db4d0b4de..434eefb5d5 100644 --- a/frappe/tests/test_query_report.py +++ b/frappe/tests/test_query_report.py @@ -3,7 +3,7 @@ import frappe import frappe.utils -from frappe.desk.query_report import build_xlsx_data +from frappe.desk.query_report import build_xlsx_data, export_query from frappe.tests.utils import FrappeTestCase from frappe.utils.xlsxutils import make_xlsx @@ -69,3 +69,42 @@ class TestQueryReport(FrappeTestCase): for row in xlsx_data: # column_b should be 'str' even with composite cell value self.assertEqual(type(row[1]), str) + + def test_csv(self): + from csv import QUOTE_ALL, QUOTE_MINIMAL, QUOTE_NONE, QUOTE_NONNUMERIC, DictReader + from io import StringIO + + REPORT_NAME = "Test CSV Report" + REF_DOCTYPE = "DocType" + REPORT_COLUMNS = ["name", "module", "issingle"] + + if not frappe.db.exists("Report", REPORT_NAME): + report = frappe.new_doc("Report") + report.report_name = REPORT_NAME + report.ref_doctype = "User" + report.report_type = "Query Report" + report.query = frappe.qb.from_(REF_DOCTYPE).select(*REPORT_COLUMNS).limit(10).get_sql() + report.is_standard = "No" + report.save() + + for delimiter in (",", ";", "\t", "|"): + for quoting in (QUOTE_ALL, QUOTE_MINIMAL, QUOTE_NONE, QUOTE_NONNUMERIC): + frappe.local.form_dict = { + "report_name": REPORT_NAME, + "file_format_type": "CSV", + "csv_quoting": quoting, + "csv_delimiter": delimiter, + "include_indentation": 0, + "visible_idx": [0, 1, 2], + } + export_query() + + self.assertTrue(frappe.response["filename"].endswith(".csv")) + self.assertEqual(frappe.response["type"], "binary") + with StringIO(frappe.response["filecontent"].decode("utf-8")) as result: + reader = DictReader(result, delimiter=delimiter, quoting=quoting) + row = reader.__next__() + for column in REPORT_COLUMNS: + self.assertIn(column, row) + + frappe.delete_doc("Report", REPORT_NAME, delete_permanently=True) From d955a29ce8657426408baa8ded46af295fef31e3 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 15 Nov 2022 00:06:08 +0100 Subject: [PATCH 07/72] feat: add translation context --- .../js/frappe/views/reports/report_utils.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/frappe/public/js/frappe/views/reports/report_utils.js b/frappe/public/js/frappe/views/reports/report_utils.js index 904443e403..2dd8ac5160 100644 --- a/frappe/public/js/frappe/views/reports/report_utils.js +++ b/frappe/public/js/frappe/views/reports/report_utils.js @@ -162,7 +162,7 @@ frappe.report_utils = { get_export_dialog(report_name, extra_fields, callback) { const fields = [ { - label: __("Select File Format"), + label: __("Select File Format", null, "Export report"), fieldname: "file_format", fieldtype: "Select", options: ["Excel", "CSV"], @@ -171,7 +171,7 @@ frappe.report_utils = { }, { fieldtype: "Data", - label: __("Delimiter"), + label: __("CSV Delimiter", null, "Export report"), fieldname: "csv_delimiter", default: ",", length: 1, @@ -179,13 +179,13 @@ frappe.report_utils = { }, { fieldtype: "Select", - label: __("Quoting"), + label: __("CSV Quoting", null, "Export report"), fieldname: "csv_quoting", options: [ - { value: 0, label: "Minimal" }, - { value: 1, label: "All" }, - { value: 2, label: "Non-numeric" }, - { value: 3, label: "None" }, + { value: 0, label: __("Minimal", null, "Export report") }, + { value: 1, label: __("All", null, "Export report") }, + { value: 2, label: __("Non-numeric", null, "Export report") }, + { value: 3, label: __("None", null, "Export report") }, ], default: 2, depends_on: "eval:doc.file_format=='CSV'", @@ -196,9 +196,9 @@ frappe.report_utils = { } return new frappe.ui.Dialog({ - title: __("Export Report: {0}", [report_name]), + title: __("Export Report: {0}", [report_name], "Export report"), fields: fields, - primary_action_label: __("Download"), + primary_action_label: __("Download", null, "Export report"), primary_action: callback, }); }, From 29465c1e3afe6dde2371fb9a3ada3a96d2f09372 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 15 Nov 2022 00:07:19 +0100 Subject: [PATCH 08/72] feat: add csv preview --- .../js/frappe/views/reports/report_utils.js | 83 ++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/views/reports/report_utils.js b/frappe/public/js/frappe/views/reports/report_utils.js index 2dd8ac5160..cbbb49fb72 100644 --- a/frappe/public/js/frappe/views/reports/report_utils.js +++ b/frappe/public/js/frappe/views/reports/report_utils.js @@ -190,16 +190,97 @@ frappe.report_utils = { default: 2, depends_on: "eval:doc.file_format=='CSV'", }, + { + fieldtype: "Small Text", + label: __("CSV Preview", null, "Export report"), + fieldname: "csv_preview", + read_only: 1, + depends_on: "eval:doc.file_format=='CSV'", + }, ]; + if (extra_fields) { fields.push(...extra_fields); } - return new frappe.ui.Dialog({ + const dialog = new frappe.ui.Dialog({ title: __("Export Report: {0}", [report_name], "Export report"), fields: fields, primary_action_label: __("Download", null, "Export report"), primary_action: callback, }); + + function update_csv_preview(dialog) { + PREVIEW_DATA = [ + [ + __("Text", null, "Export report"), + __("Number", null, "Export report"), + __("Float", null, "Export report"), + __("Check", null, "Export report"), + ], + [__("Hello, World", null, "Export report"), 42, 3.14, 0], + [__("Hello World", null, "Export report"), 0, 99.99, 1], + ]; + dialog.set_value( + "csv_preview", + frappe.report_utils.get_csv_preview( + PREVIEW_DATA, + dialog.get_value("csv_quoting"), + dialog.get_value("csv_delimiter") + ) + ); + } + + dialog.fields_dict["file_format"].df.onchange = () => update_csv_preview(dialog); + dialog.fields_dict["csv_quoting"].df.onchange = () => update_csv_preview(dialog); + dialog.fields_dict["csv_delimiter"].df.onchange = () => update_csv_preview(dialog); + + return dialog; + }, + + get_csv_preview(data, quoting, delimiter) { + // data: array of arrays + // quoting: 0 - minimal, 1 - all, 2 - non-numeric, 3 - none + // delimiter: any single character + quoting = cint(quoting); + const QUOTING = { + Minimal: 0, + All: 1, + NonNumeric: 2, + None: 3, + }; + + if (delimiter.length > 1) { + frappe.throw(__("Delimiter must be a single character")); + } + + if (0 > quoting || quoting > 3) { + frappe.throw(__("Quoting must be between 0 and 3")); + } + + return data + .map((row) => { + return row + .map((col) => { + if (typeof col == "string" && col.includes('"')) { + col = col.replace(/"/g, '""'); + } + + switch (quoting) { + case QUOTING.Minimal: + return typeof col === "string" && col.includes(delimiter) + ? `"${col}"` + : `${col}`; + case QUOTING.All: + return `"${col}"`; + case QUOTING.NonNumeric: + return isNaN(col) ? `"${col}"` : `${col}`; + case QUOTING.None: + return `${col}`; + } + }) + .join(delimiter); + }) + .join("\n"); }, }; From cb434d728bca984b954540b2971001c097477f57 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Tue, 15 Nov 2022 00:48:44 +0100 Subject: [PATCH 09/72] feat: preview real data --- .../js/frappe/views/reports/report_utils.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/frappe/public/js/frappe/views/reports/report_utils.js b/frappe/public/js/frappe/views/reports/report_utils.js index cbbb49fb72..faf5c4af4c 100644 --- a/frappe/public/js/frappe/views/reports/report_utils.js +++ b/frappe/public/js/frappe/views/reports/report_utils.js @@ -211,16 +211,18 @@ frappe.report_utils = { }); function update_csv_preview(dialog) { + const is_query_report = frappe.get_route()[0] === "query-report"; + const report = is_query_report ? frappe.query_report : cur_list; + const columns = report.columns.filter((col) => col.hidden !== 1); PREVIEW_DATA = [ - [ - __("Text", null, "Export report"), - __("Number", null, "Export report"), - __("Float", null, "Export report"), - __("Check", null, "Export report"), - ], - [__("Hello, World", null, "Export report"), 42, 3.14, 0], - [__("Hello World", null, "Export report"), 0, 99.99, 1], + columns.map((col) => __(is_query_report ? col.label : col.name)), + ...report.data + .slice(0, 3) + .map((row) => + columns.map((col) => row[is_query_report ? col.fieldname : col.field]) + ), ]; + dialog.set_value( "csv_preview", frappe.report_utils.get_csv_preview( From 8cc504709bcc0e4ce8eff95a1738185b416bfa3f Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 23 Nov 2022 23:00:48 +0530 Subject: [PATCH 10/72] fix: only accept string values for `key` --- frappe/www/printview.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/www/printview.py b/frappe/www/printview.py index 3ba73fe348..faf6a02067 100644 --- a/frappe/www/printview.py +++ b/frappe/www/printview.py @@ -335,8 +335,8 @@ def validate_print_permission(doc): if frappe.has_permission(doc.doctype, ptype, doc) or frappe.has_website_permission(doc): return - key = frappe.form_dict.get("key") - if key: + key = frappe.form_dict.key + if key and isinstance(key, str): validate_key(key, doc) else: raise frappe.PermissionError(_("You do not have permission to view this document")) From 920606f10eea0b4d318c3ea8a850a5c10a3234c7 Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Thu, 24 Nov 2022 06:24:46 +0100 Subject: [PATCH 11/72] fix: handle "No Letterhead" in new print format builder (#18990) [skip ci] --- frappe/utils/weasyprint.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frappe/utils/weasyprint.py b/frappe/utils/weasyprint.py index d670cf96f5..540149b2e8 100644 --- a/frappe/utils/weasyprint.py +++ b/frappe/utils/weasyprint.py @@ -4,6 +4,7 @@ import click import frappe +from frappe import _ @frappe.whitelist() @@ -49,7 +50,11 @@ class PrintFormatGenerator: self.base_url = frappe.utils.get_url() self.print_format = frappe.get_doc("Print Format", print_format) self.doc = doc + + if letterhead == _("No Letterhead"): + letterhead = None self.letterhead = frappe.get_doc("Letter Head", letterhead) if letterhead else None + self.build_context() self.layout = self.get_layout(self.print_format) self.context.layout = self.layout From 73f0256aa8b02f888663a3e6778c7c149970c18b Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Thu, 24 Nov 2022 08:45:24 +0100 Subject: [PATCH 12/72] feat: add param letterhead to frappe.get_print (#18989) * feat: add param letterhead to frappe.get_print --- frappe/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/__init__.py b/frappe/__init__.py index 2d491ca068..8424fd0071 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1987,6 +1987,7 @@ def get_print( no_letterhead=0, password=None, pdf_options=None, + letterhead=None, ): """Get Print Format for given document. @@ -2005,6 +2006,7 @@ def get_print( local.form_dict.style = style local.form_dict.doc = doc local.form_dict.no_letterhead = no_letterhead + local.form_dict.letterhead = letterhead pdf_options = pdf_options or {} if password: From 575d32ec35f136fa2f98541113f804bee3107b20 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Nov 2022 15:22:12 +0530 Subject: [PATCH 13/72] fix(db_query): Space resilient matching --- frappe/model/db_query.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index e689f91ddd..5026b04999 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -384,14 +384,19 @@ class DatabaseQuery: _raise_exception() for field in self.fields: + lower_field = field.lower() + function = lower_field.split("(", 1)[0].rstrip() + + if function in blacklisted_functions: + frappe.throw(_("Use of function {0} in field is restricted").format(function)) + if SUB_QUERY_PATTERN.match(field): - if any(f"({keyword}" in field.lower() for keyword in blacklisted_keywords): - _raise_exception() + if field[0] == "(": + subquery_token = lower_field[1:].lstrip().split(" ", 1)[0] + if subquery_token in blacklisted_keywords: + _raise_exception() - if any(f"{keyword}(" in field.lower() for keyword in blacklisted_functions): - _raise_exception() - - if "@" in field.lower(): + if "@" in lower_field: # prevent access to global variables _raise_exception() @@ -407,7 +412,7 @@ class DatabaseQuery: if STRICT_FIELD_PATTERN.match(field): frappe.throw(_("Illegal SQL Query")) - if STRICT_UNION_PATTERN.match(field.lower()): + if STRICT_UNION_PATTERN.match(lower_field): frappe.throw(_("Illegal SQL Query")) def extract_tables(self): From 1f913248aa61958be77a97221b1db8b43210aba8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Nov 2022 15:30:38 +0530 Subject: [PATCH 14/72] test: Add more tests for illegal subquery and fn usage --- frappe/tests/test_db_query.py | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 162d3e9d8a..79bfacdcf9 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -418,6 +418,43 @@ class TestReportview(FrappeTestCase): ) self.assertTrue("date_diff" in data[0]) + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=["name", "issingle", "if (issingle=1, (select name from tabUser), count(name))"], + limit_start=0, + limit_page_length=1, + ) + + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=["name", "issingle", "if(issingle=1, (select name from tabUser), count(name))"], + limit_start=0, + limit_page_length=1, + ) + + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=[ + "name", + "issingle", + "( select name from `tabUser` where `tabDocType`.owner = `tabUser`.name )", + ], + limit_start=0, + limit_page_length=1, + ignore_permissions=True, + ) + + with self.assertRaises(frappe.ValidationError): + DatabaseQuery("DocType").execute( + fields=[ + "name", + "issingle", + "(select name from `tabUser` where `tabDocType`.owner = `tabUser`.name )", + ], + limit_start=0, + limit_page_length=1, + ) + def test_nested_permission(self): frappe.set_user("Administrator") create_nested_doctype() From c7461a05c66a28d1215c74ca9657676f4ba77b1b Mon Sep 17 00:00:00 2001 From: Himanshu Shivhare Date: Thu, 24 Nov 2022 16:25:11 +0530 Subject: [PATCH 15/72] chore: Typo correction (#18997) [skip ci] --- frappe/commands/scheduler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/commands/scheduler.py b/frappe/commands/scheduler.py index e481676088..fba7aac4fa 100755 --- a/frappe/commands/scheduler.py +++ b/frappe/commands/scheduler.py @@ -183,7 +183,7 @@ def start_scheduler(): @click.option("-u", "--rq-username", default=None, help="Redis ACL user") @click.option("-p", "--rq-password", default=None, help="Redis ACL user password") def start_worker(queue, quiet=False, rq_username=None, rq_password=None): - """Site is used to find redis credentals.""" + """Site is used to find redis credentials.""" from frappe.utils.background_jobs import start_worker start_worker(queue, quiet=quiet, rq_username=rq_username, rq_password=rq_password) From 1a5e5f546b7151ec5994cc4631e697c44b355880 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 24 Nov 2022 16:21:39 +0530 Subject: [PATCH 16/72] fix: Move function check inside subquery --- frappe/model/db_query.py | 10 ++++++---- frappe/tests/test_db_query.py | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 5026b04999..f56e777ad9 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -385,10 +385,6 @@ class DatabaseQuery: for field in self.fields: lower_field = field.lower() - function = lower_field.split("(", 1)[0].rstrip() - - if function in blacklisted_functions: - frappe.throw(_("Use of function {0} in field is restricted").format(function)) if SUB_QUERY_PATTERN.match(field): if field[0] == "(": @@ -396,6 +392,12 @@ class DatabaseQuery: if subquery_token in blacklisted_keywords: _raise_exception() + function = lower_field.split("(", 1)[0].rstrip() + if function in blacklisted_functions: + frappe.throw( + _("Use of function {0} in field is restricted").format(function), exc=frappe.DataError + ) + if "@" in lower_field: # prevent access to global variables _raise_exception() diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 79bfacdcf9..52f18f5963 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -418,21 +418,21 @@ class TestReportview(FrappeTestCase): ) self.assertTrue("date_diff" in data[0]) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=["name", "issingle", "if (issingle=1, (select name from tabUser), count(name))"], limit_start=0, limit_page_length=1, ) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=["name", "issingle", "if(issingle=1, (select name from tabUser), count(name))"], limit_start=0, limit_page_length=1, ) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=[ "name", @@ -444,7 +444,7 @@ class TestReportview(FrappeTestCase): ignore_permissions=True, ) - with self.assertRaises(frappe.ValidationError): + with self.assertRaises(frappe.DataError): DatabaseQuery("DocType").execute( fields=[ "name", From b1703117fccbb49a9949d480b905e9f4dbbdddf3 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 25 Nov 2022 06:29:04 +0000 Subject: [PATCH 17/72] chore: remove unused whitelisting from API (#19000) --- frappe/desk/form/linked_with.py | 1 - 1 file changed, 1 deletion(-) diff --git a/frappe/desk/form/linked_with.py b/frappe/desk/form/linked_with.py index cb2b5508ce..5bb07aefe5 100644 --- a/frappe/desk/form/linked_with.py +++ b/frappe/desk/form/linked_with.py @@ -403,7 +403,6 @@ def get_exempted_doctypes(): return auto_cancel_exempt_doctypes -@frappe.whitelist() def get_linked_docs(doctype: str, name: str, linkinfo: dict | None = None) -> dict[str, list]: if isinstance(linkinfo, str): # additional fields are added in linkinfo From b7cef3ae71c3a78a3af6d4f393d15777b1067d22 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 24 Nov 2022 14:37:12 +0530 Subject: [PATCH 18/72] fix: prioritize short queue when using all queues --- frappe/utils/background_jobs.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 6ce79d59e4..3e6da14878 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -34,9 +34,11 @@ def get_queues_timeout(): custom_workers_config = common_site_config.get("workers", {}) default_timeout = 300 + # Note: Order matters here + # If no queues are specified then RQ prioritizes queues in specified order return { - "default": default_timeout, "short": default_timeout, + "default": default_timeout, "long": 1500, **{ worker: config.get("timeout", default_timeout) From 40b2929c0d73e763c39bd8604d28146fb79dad4f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 24 Nov 2022 13:38:17 +0530 Subject: [PATCH 19/72] feat(workers): allow consuming multiple queues --- frappe/commands/scheduler.py | 6 +++++- frappe/utils/background_jobs.py | 12 ++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/frappe/commands/scheduler.py b/frappe/commands/scheduler.py index fba7aac4fa..d998cc85fe 100755 --- a/frappe/commands/scheduler.py +++ b/frappe/commands/scheduler.py @@ -178,7 +178,11 @@ def start_scheduler(): @click.command("worker") -@click.option("--queue", type=str) +@click.option( + "--queue", + type=str, + help="Queue to consume from. Multiple queues can be specified using comma-separated string. If not specified all queues are consumed.", +) @click.option("--quiet", is_flag=True, default=False, help="Hide Log Outputs") @click.option("-u", "--rq-username", default=None, help="Redis ACL user") @click.option("-p", "--rq-password", default=None, help="Redis ACL user password") diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 3e6da14878..f321a62c77 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -3,7 +3,7 @@ import socket import time from collections import defaultdict from functools import lru_cache -from typing import TYPE_CHECKING, Any, Union +from typing import TYPE_CHECKING, Any, NoReturn, Union from uuid import uuid4 import redis @@ -211,11 +211,19 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, frappe.destroy() -def start_worker(queue=None, quiet=False, rq_username=None, rq_password=None): +def start_worker( + queue: str | None = None, + quiet: bool = False, + rq_username: str | None = None, + rq_password: str | None = None, +) -> NoReturn: """Wrapper to start rq worker. Connects to redis and monitors these queues.""" with frappe.init_site(): # empty init is required to get redis_queue from common_site_config.json redis_connection = get_redis_conn(username=rq_username, password=rq_password) + + if queue: + queue = [q.strip() for q in queue.split(",")] queues = get_queue_list(queue, build_queue_name=True) queue_name = queue and generate_qname(queue) From 0ebd3945ff37fd8928c9818f014ddd77fa2274c4 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 24 Nov 2022 14:59:05 +0530 Subject: [PATCH 20/72] refactor: consider multi-queue consumption --- frappe/core/doctype/rq_worker/rq_worker.json | 6 +++--- frappe/core/doctype/rq_worker/rq_worker.py | 9 ++++++--- frappe/utils/background_jobs.py | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/frappe/core/doctype/rq_worker/rq_worker.json b/frappe/core/doctype/rq_worker/rq_worker.json index d9a5a23f67..18441377c9 100644 --- a/frappe/core/doctype/rq_worker/rq_worker.json +++ b/frappe/core/doctype/rq_worker/rq_worker.json @@ -76,13 +76,13 @@ { "fieldname": "queue", "fieldtype": "Data", - "label": "Queue" + "label": "Queue(s)" }, { "fieldname": "queue_type", "fieldtype": "Select", "in_list_view": 1, - "label": "Queue Type", + "label": "Queue Type(s)", "options": "default\nlong\nshort" }, { @@ -113,7 +113,7 @@ "in_create": 1, "is_virtual": 1, "links": [], - "modified": "2022-11-14 15:35:32.786012", + "modified": "2022-11-24 14:50:48.511706", "modified_by": "Administrator", "module": "Core", "name": "RQ Worker", diff --git a/frappe/core/doctype/rq_worker/rq_worker.py b/frappe/core/doctype/rq_worker/rq_worker.py index 3de0c8f7fc..52397cd5cc 100644 --- a/frappe/core/doctype/rq_worker/rq_worker.py +++ b/frappe/core/doctype/rq_worker/rq_worker.py @@ -16,8 +16,10 @@ class RQWorker(Document): def load_from_db(self): all_workers = get_workers() - worker = [w for w in all_workers if w.pid == cint(self.name)][0] - d = serialize_worker(worker) + workers = [w for w in all_workers if w.pid == cint(self.name)] + if not workers: + raise frappe.DoesNotExistError + d = serialize_worker(workers[0]) super(Document, self).__init__(d) @@ -53,10 +55,11 @@ class RQWorker(Document): def serialize_worker(worker: Worker) -> frappe._dict: queue = ", ".join(worker.queue_names()) + queue_types = ",".join(q.rsplit(":", 1)[1] for q in worker.queue_names()) return frappe._dict( name=worker.pid, queue=queue, - queue_type=queue.rsplit(":", 1)[1], + queue_type=queue_types, worker_name=worker.name, status=worker.get_state(), pid=worker.pid, diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index f321a62c77..873a91dd14 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -377,6 +377,8 @@ def generate_qname(qtype: str) -> str: qnames are useful to define namespaces of customers. """ + if isinstance(qtype, list): + qtype = ",".join(qtype) return f"{get_bench_id()}:{qtype}" From aece93fbc54dae49238f280b58e22ad70d82a040 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 24 Nov 2022 16:02:01 +0530 Subject: [PATCH 21/72] feat: burst mode in workers https://python-rq.org/docs/workers/#burst-mode --- frappe/commands/scheduler.py | 5 +++-- frappe/core/doctype/rq_job/test_rq_job.py | 10 ++++++++++ frappe/utils/background_jobs.py | 5 +++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/frappe/commands/scheduler.py b/frappe/commands/scheduler.py index d998cc85fe..be16450c54 100755 --- a/frappe/commands/scheduler.py +++ b/frappe/commands/scheduler.py @@ -186,11 +186,12 @@ def start_scheduler(): @click.option("--quiet", is_flag=True, default=False, help="Hide Log Outputs") @click.option("-u", "--rq-username", default=None, help="Redis ACL user") @click.option("-p", "--rq-password", default=None, help="Redis ACL user password") -def start_worker(queue, quiet=False, rq_username=None, rq_password=None): +@click.option("--burst", is_flag=True, default=False, help="Run Worker in Burst mode.") +def start_worker(queue, quiet=False, rq_username=None, rq_password=None, burst=False): """Site is used to find redis credentials.""" from frappe.utils.background_jobs import start_worker - start_worker(queue, quiet=quiet, rq_username=rq_username, rq_password=rq_password) + start_worker(queue, quiet=quiet, rq_username=rq_username, rq_password=rq_password, burst=burst) @click.command("ready-for-migration") diff --git a/frappe/core/doctype/rq_job/test_rq_job.py b/frappe/core/doctype/rq_job/test_rq_job.py index 460aa08941..654cdcd21f 100644 --- a/frappe/core/doctype/rq_job/test_rq_job.py +++ b/frappe/core/doctype/rq_job/test_rq_job.py @@ -10,6 +10,7 @@ from rq.job import Job import frappe from frappe.core.doctype.rq_job.rq_job import RQJob, remove_failed_jobs, stop_job from frappe.tests.utils import FrappeTestCase, timeout +from frappe.utils import cstr, execute_in_shell from frappe.utils.background_jobs import is_job_queued @@ -92,6 +93,15 @@ class TestRQJob(FrappeTestCase): self.check_status(actual_job, "finished") self.assertFalse(is_job_queued(job_name)) + @timeout(20) + def test_multi_queue_burst_consumption(self): + for _ in range(3): + for q in ["default", "short"]: + frappe.enqueue(self.BG_JOB, sleep=1, queue=q) + + _, stderr = execute_in_shell("bench worker --queue short,default --burst", check_exit_code=True) + self.assertIn("quitting", cstr(stderr)) + def test_func(fail=False, sleep=0): if fail: diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 873a91dd14..1457a821ec 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -216,7 +216,8 @@ def start_worker( quiet: bool = False, rq_username: str | None = None, rq_password: str | None = None, -) -> NoReturn: + burst: bool = False, +) -> NoReturn | None: """Wrapper to start rq worker. Connects to redis and monitors these queues.""" with frappe.init_site(): # empty init is required to get redis_queue from common_site_config.json @@ -234,7 +235,7 @@ def start_worker( logging_level = "INFO" if quiet: logging_level = "WARNING" - Worker(queues, name=get_worker_name(queue_name)).work(logging_level=logging_level) + Worker(queues, name=get_worker_name(queue_name)).work(logging_level=logging_level, burst=burst) def get_worker_name(queue): From a8bf86ef75dd55fd4afe5627dfa57cb529288621 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 24 Nov 2022 16:27:21 +0530 Subject: [PATCH 22/72] feat: support dequeuing strategies for worker --- frappe/commands/scheduler.py | 20 +++++++++++++++++--- frappe/commands/site.py | 2 +- frappe/utils/background_jobs.py | 13 +++++++++++-- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/frappe/commands/scheduler.py b/frappe/commands/scheduler.py index be16450c54..ad1635cb57 100755 --- a/frappe/commands/scheduler.py +++ b/frappe/commands/scheduler.py @@ -187,11 +187,25 @@ def start_scheduler(): @click.option("-u", "--rq-username", default=None, help="Redis ACL user") @click.option("-p", "--rq-password", default=None, help="Redis ACL user password") @click.option("--burst", is_flag=True, default=False, help="Run Worker in Burst mode.") -def start_worker(queue, quiet=False, rq_username=None, rq_password=None, burst=False): - """Site is used to find redis credentials.""" +@click.option( + "--strategy", + required=False, + type=click.Choice(["round_robbin", "random"]), + help="dequeuing strategy to use.", +) +def start_worker( + queue, quiet=False, rq_username=None, rq_password=None, burst=False, strategy=None +): from frappe.utils.background_jobs import start_worker - start_worker(queue, quiet=quiet, rq_username=rq_username, rq_password=rq_password, burst=burst) + start_worker( + queue, + quiet=quiet, + rq_username=rq_username, + rq_password=rq_password, + burst=burst, + strategy=strategy, + ) @click.command("ready-for-migration") diff --git a/frappe/commands/site.py b/frappe/commands/site.py index 13f642ea76..37b1a36c6c 100644 --- a/frappe/commands/site.py +++ b/frappe/commands/site.py @@ -1119,7 +1119,7 @@ def build_search_index(context): @click.command("clear-log-table") -@click.option("--doctype", default="text", type=click.Choice(LOG_DOCTYPES), help="Log DocType") +@click.option("--doctype", required=True, type=click.Choice(LOG_DOCTYPES), help="Log DocType") @click.option("--days", type=int, help="Keep records for days") @click.option("--no-backup", is_flag=True, default=False, help="Do not backup the table") @pass_context diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 1457a821ec..3d1c33d258 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -3,13 +3,14 @@ import socket import time from collections import defaultdict from functools import lru_cache -from typing import TYPE_CHECKING, Any, NoReturn, Union +from typing import TYPE_CHECKING, Any, Literal, NoReturn, Union from uuid import uuid4 import redis from redis.exceptions import BusyLoadingError, ConnectionError from rq import Connection, Queue, Worker from rq.logutils import setup_loghandlers +from rq.worker import RandomWorker, RoundRobinWorker from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed import frappe @@ -217,8 +218,11 @@ def start_worker( rq_username: str | None = None, rq_password: str | None = None, burst: bool = False, + strategy: Literal["round_robbin", "random"] | None = None, ) -> NoReturn | None: """Wrapper to start rq worker. Connects to redis and monitors these queues.""" + DEQUEUE_STRATEGIES = {"round_robbin": RoundRobinWorker, "random": RandomWorker} + with frappe.init_site(): # empty init is required to get redis_queue from common_site_config.json redis_connection = get_redis_conn(username=rq_username, password=rq_password) @@ -231,11 +235,16 @@ def start_worker( if os.environ.get("CI"): setup_loghandlers("ERROR") + WorkerKlass = Worker + if strategy and strategy in DEQUEUE_STRATEGIES: + WorkerKlass = DEQUEUE_STRATEGIES[strategy] + with Connection(redis_connection): logging_level = "INFO" if quiet: logging_level = "WARNING" - Worker(queues, name=get_worker_name(queue_name)).work(logging_level=logging_level, burst=burst) + worker = WorkerKlass(queues, name=get_worker_name(queue_name)) + worker.work(logging_level=logging_level, burst=burst) def get_worker_name(queue): From ed2862602193cad5478e48fac8a264a310492137 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 25 Nov 2022 12:02:51 +0530 Subject: [PATCH 23/72] refactor: simpler deque strat selection Co-authored-by: Ritwik Puri --- frappe/commands/scheduler.py | 2 +- frappe/core/doctype/rq_worker/rq_worker.py | 6 ++++-- frappe/utils/background_jobs.py | 4 +--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frappe/commands/scheduler.py b/frappe/commands/scheduler.py index ad1635cb57..bbc7008321 100755 --- a/frappe/commands/scheduler.py +++ b/frappe/commands/scheduler.py @@ -191,7 +191,7 @@ def start_scheduler(): "--strategy", required=False, type=click.Choice(["round_robbin", "random"]), - help="dequeuing strategy to use.", + help="Dequeuing strategy to use", ) def start_worker( queue, quiet=False, rq_username=None, rq_password=None, burst=False, strategy=None diff --git a/frappe/core/doctype/rq_worker/rq_worker.py b/frappe/core/doctype/rq_worker/rq_worker.py index 52397cd5cc..1d24001fc3 100644 --- a/frappe/core/doctype/rq_worker/rq_worker.py +++ b/frappe/core/doctype/rq_worker/rq_worker.py @@ -53,9 +53,11 @@ class RQWorker(Document): def serialize_worker(worker: Worker) -> frappe._dict: - queue = ", ".join(worker.queue_names()) + queue_names = worker.queue_names() + + queue = ", ".join(queue_names) + queue_types = ",".join(q.rsplit(":", 1)[1] for q in queue_names) - queue_types = ",".join(q.rsplit(":", 1)[1] for q in worker.queue_names()) return frappe._dict( name=worker.pid, queue=queue, diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 3d1c33d258..8bfe244c93 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -235,9 +235,7 @@ def start_worker( if os.environ.get("CI"): setup_loghandlers("ERROR") - WorkerKlass = Worker - if strategy and strategy in DEQUEUE_STRATEGIES: - WorkerKlass = DEQUEUE_STRATEGIES[strategy] + WorkerKlass = DEQUEUE_STRATEGIES.get(strategy, Worker) with Connection(redis_connection): logging_level = "INFO" From 35827af1729f4d05cdb277d367849439cbd90e13 Mon Sep 17 00:00:00 2001 From: gavin Date: Fri, 25 Nov 2022 12:39:11 +0530 Subject: [PATCH 24/72] fix: Strip white spaces on lower cased field value Co-authored-by: Ankush Menat --- frappe/model/db_query.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index f56e777ad9..4b9318ab03 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -384,10 +384,10 @@ class DatabaseQuery: _raise_exception() for field in self.fields: - lower_field = field.lower() + lower_field = field.lower().strip() if SUB_QUERY_PATTERN.match(field): - if field[0] == "(": + if lower_field[0] == "(": subquery_token = lower_field[1:].lstrip().split(" ", 1)[0] if subquery_token in blacklisted_keywords: _raise_exception() From d00b98f460eb0db58ef8eccbcf3e53667ed58076 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 25 Nov 2022 13:10:43 +0530 Subject: [PATCH 25/72] chore: typo --- frappe/commands/scheduler.py | 2 +- frappe/utils/background_jobs.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/commands/scheduler.py b/frappe/commands/scheduler.py index bbc7008321..0c8278dcbf 100755 --- a/frappe/commands/scheduler.py +++ b/frappe/commands/scheduler.py @@ -190,7 +190,7 @@ def start_scheduler(): @click.option( "--strategy", required=False, - type=click.Choice(["round_robbin", "random"]), + type=click.Choice(["round_robin", "random"]), help="Dequeuing strategy to use", ) def start_worker( diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 8bfe244c93..ea7eefc44f 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -218,10 +218,10 @@ def start_worker( rq_username: str | None = None, rq_password: str | None = None, burst: bool = False, - strategy: Literal["round_robbin", "random"] | None = None, + strategy: Literal["round_robin", "random"] | None = None, ) -> NoReturn | None: """Wrapper to start rq worker. Connects to redis and monitors these queues.""" - DEQUEUE_STRATEGIES = {"round_robbin": RoundRobinWorker, "random": RandomWorker} + DEQUEUE_STRATEGIES = {"round_robin": RoundRobinWorker, "random": RandomWorker} with frappe.init_site(): # empty init is required to get redis_queue from common_site_config.json From 375de8647a5b0e2340da673b4e1160444e6e81e5 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 25 Nov 2022 14:13:05 +0530 Subject: [PATCH 26/72] fix: discovery and styling issues in grid buttons --- frappe/public/js/frappe/form/grid.js | 97 ++++++++++++++-------------- frappe/public/scss/common/grid.scss | 9 ++- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/frappe/public/js/frappe/form/grid.js b/frappe/public/js/frappe/form/grid.js index d0c352d784..3d81744f59 100644 --- a/frappe/public/js/frappe/form/grid.js +++ b/frappe/public/js/frappe/form/grid.js @@ -61,57 +61,56 @@ export default class Grid { make() { let template = ` - - -

-
-
-
-
-
-
-
- Grid Empty State - ${__("No Data")} +
+ + +

+
+
+
+
+
+
+
+ Grid Empty State + ${__("No Data")} +
-
-