From 91377658039880e1090c9d968e9fe6a154f7c3e6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 16:58:18 +0530 Subject: [PATCH 1/8] fix: halve prepared report threshold Users can still enable/disable on their own after last refactor. --- frappe/core/doctype/report/report.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index dba82bada5..89cbc1b2f5 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -121,7 +121,7 @@ class Report(Document): def execute_script_report(self, filters): # save the timestamp to automatically set to prepared - threshold = 30 + threshold = 15 res = [] start_time = datetime.datetime.now() From b81aff3237152a5319c9d95e90d0b788af33b8af Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 17:09:15 +0530 Subject: [PATCH 2/8] test: simplify prepared report tests --- .../prepared_report/test_prepared_report.py | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/frappe/core/doctype/prepared_report/test_prepared_report.py b/frappe/core/doctype/prepared_report/test_prepared_report.py index a864ea73f8..6c2458d8a1 100644 --- a/frappe/core/doctype/prepared_report/test_prepared_report.py +++ b/frappe/core/doctype/prepared_report/test_prepared_report.py @@ -5,7 +5,7 @@ import time import frappe from frappe.desk.query_report import generate_report_result, get_report_doc -from frappe.tests.utils import FrappeTestCase +from frappe.tests.utils import FrappeTestCase, timeout class TestPreparedReport(FrappeTestCase): @@ -16,7 +16,18 @@ class TestPreparedReport(FrappeTestCase): frappe.db.commit() - def create_prepared_report(self, commit=False): + @timeout(seconds=20) + def wait_for_completion(self, report, status="Completed"): + frappe.db.commit() # Flush changes first + while True: + frappe.db.rollback() # read new data + report.reload() + if report.status == status: + break + # Cheap blocking behaviour + time.sleep(0.5) + + def create_prepared_report(self, commit=True): doc = frappe.get_doc( { "doctype": "Prepared Report", @@ -30,23 +41,21 @@ class TestPreparedReport(FrappeTestCase): return doc def test_queueing(self): - doc_ = self.create_prepared_report() - self.assertEqual("Queued", doc_.status) - self.assertTrue(doc_.queued_at) + doc = self.create_prepared_report() + self.assertEqual("Queued", doc.status) + self.assertTrue(doc.queued_at) - frappe.db.commit() - time.sleep(5) + self.wait_for_completion(doc) - doc_ = frappe.get_last_doc("Prepared Report") - self.assertEqual("Completed", doc_.status) - self.assertTrue(doc_.job_id) - self.assertTrue(doc_.report_end_time) + doc = frappe.get_last_doc("Prepared Report") + self.assertTrue(doc.job_id) + self.assertTrue(doc.report_end_time) def test_prepared_data(self): - doc_ = self.create_prepared_report(commit=True) - time.sleep(5) + doc = self.create_prepared_report() + self.wait_for_completion(doc) - prepared_data = json.loads(doc_.get_prepared_data().decode("utf-8")) + prepared_data = json.loads(doc.get_prepared_data().decode("utf-8")) generated_data = generate_report_result(get_report_doc("Database Storage Usage By Tables")) self.assertEqual(len(prepared_data["columns"]), len(generated_data["columns"])) self.assertEqual(len(prepared_data["result"]), len(generated_data["result"])) From 869f2194185bdac416726a8c2d522556d46b0454 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 17:17:10 +0530 Subject: [PATCH 3/8] fix: dont export prepared_report and letterhead --- frappe/core/doctype/report/report.py | 4 ++++ .../database_storage_usage_by_tables.json | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/report/report.py b/frappe/core/doctype/report/report.py index 89cbc1b2f5..8bd22b173e 100644 --- a/frappe/core/doctype/report/report.py +++ b/frappe/core/doctype/report/report.py @@ -49,6 +49,10 @@ class Report(Document): def on_update(self): self.export_doc() + def before_export(self): + self.letterhead = None + self.prepared_report = 0 + def on_trash(self): if ( self.is_standard == "Yes" diff --git a/frappe/core/report/database_storage_usage_by_tables/database_storage_usage_by_tables.json b/frappe/core/report/database_storage_usage_by_tables/database_storage_usage_by_tables.json index 20deb78ad6..773cb7771f 100644 --- a/frappe/core/report/database_storage_usage_by_tables/database_storage_usage_by_tables.json +++ b/frappe/core/report/database_storage_usage_by_tables/database_storage_usage_by_tables.json @@ -9,7 +9,6 @@ "filters": [], "idx": 0, "is_standard": "Yes", - "letter_head": "abc", "modified": "2022-10-19 02:59:00.365307", "modified_by": "Administrator", "module": "Core", @@ -25,4 +24,4 @@ "role": "System Manager" } ] -} \ No newline at end of file +} From f72643781d3c21723dba56210de55c6dbd594e78 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 17:22:43 +0530 Subject: [PATCH 4/8] feat: "Started" status in prepared reports --- .../prepared_report/prepared_report.json | 4 +-- .../prepared_report/prepared_report.py | 18 +++++++--- .../prepared_report/test_prepared_report.py | 34 ++++++++++++++++--- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/frappe/core/doctype/prepared_report/prepared_report.json b/frappe/core/doctype/prepared_report/prepared_report.json index fb3809e481..135b6a996c 100644 --- a/frappe/core/doctype/prepared_report/prepared_report.json +++ b/frappe/core/doctype/prepared_report/prepared_report.json @@ -35,7 +35,7 @@ "in_list_view": 1, "in_standard_filter": 1, "label": "Status", - "options": "Error\nQueued\nCompleted", + "options": "Error\nQueued\nCompleted\nStarted", "read_only": 1 }, { @@ -104,7 +104,7 @@ ], "in_create": 1, "links": [], - "modified": "2023-05-19 15:41:03.428589", + "modified": "2023-07-01 17:18:40.183106", "modified_by": "Administrator", "module": "Core", "name": "Prepared Report", diff --git a/frappe/core/doctype/prepared_report/prepared_report.py b/frappe/core/doctype/prepared_report/prepared_report.py index ed7f4711aa..7182fa4920 100644 --- a/frappe/core/doctype/prepared_report/prepared_report.py +++ b/frappe/core/doctype/prepared_report/prepared_report.py @@ -58,7 +58,7 @@ class PreparedReport(Document): def generate_report(prepared_report): - update_job_id(prepared_report, get_current_job().id) + update_job_id(prepared_report) instance = frappe.get_doc("Prepared Report", prepared_report) report = frappe.get_doc("Report", instance.report_name) @@ -95,8 +95,18 @@ def generate_report(prepared_report): ) -def update_job_id(prepared_report, job_id): - frappe.db.set_value("Prepared Report", prepared_report, "job_id", job_id, update_modified=False) +def update_job_id(prepared_report): + job = get_current_job() + + frappe.db.set_value( + "Prepared Report", + prepared_report, + { + "job_id": job and job.id, + "status": "Started", + }, + ) + frappe.db.commit() @@ -132,7 +142,7 @@ def get_reports_in_queued_state(report_name, filters): filters={ "report_name": report_name, "filters": process_filters_for_prepared_report(filters), - "status": "Queued", + "status": ("in", ("Queued", "Started")), "owner": frappe.session.user, }, ) diff --git a/frappe/core/doctype/prepared_report/test_prepared_report.py b/frappe/core/doctype/prepared_report/test_prepared_report.py index 6c2458d8a1..b8dd3d67d0 100644 --- a/frappe/core/doctype/prepared_report/test_prepared_report.py +++ b/frappe/core/doctype/prepared_report/test_prepared_report.py @@ -2,6 +2,7 @@ # License: MIT. See LICENSE import json import time +from contextlib import contextmanager import frappe from frappe.desk.query_report import generate_report_result, get_report_doc @@ -17,7 +18,7 @@ class TestPreparedReport(FrappeTestCase): frappe.db.commit() @timeout(seconds=20) - def wait_for_completion(self, report, status="Completed"): + def wait_for_status(self, report, status): frappe.db.commit() # Flush changes first while True: frappe.db.rollback() # read new data @@ -27,11 +28,11 @@ class TestPreparedReport(FrappeTestCase): # Cheap blocking behaviour time.sleep(0.5) - def create_prepared_report(self, commit=True): + def create_prepared_report(self, report=None, commit=True): doc = frappe.get_doc( { "doctype": "Prepared Report", - "report_name": "Database Storage Usage By Tables", + "report_name": report or "Database Storage Usage By Tables", } ).insert() @@ -45,7 +46,7 @@ class TestPreparedReport(FrappeTestCase): self.assertEqual("Queued", doc.status) self.assertTrue(doc.queued_at) - self.wait_for_completion(doc) + self.wait_for_status(doc, "Completed") doc = frappe.get_last_doc("Prepared Report") self.assertTrue(doc.job_id) @@ -53,10 +54,33 @@ class TestPreparedReport(FrappeTestCase): def test_prepared_data(self): doc = self.create_prepared_report() - self.wait_for_completion(doc) + self.wait_for_status(doc, "Completed") prepared_data = json.loads(doc.get_prepared_data().decode("utf-8")) generated_data = generate_report_result(get_report_doc("Database Storage Usage By Tables")) self.assertEqual(len(prepared_data["columns"]), len(generated_data["columns"])) self.assertEqual(len(prepared_data["result"]), len(generated_data["result"])) self.assertEqual(len(prepared_data), len(generated_data)) + + def test_start_status(self): + if frappe.db.db_type == "postgres": + return + + with test_report(report_type="Query Report", query="select sleep(5)") as report: + doc = self.create_prepared_report(report.name) + self.wait_for_status(doc, "Started") + + +@contextmanager +def test_report(**args): + try: + report = frappe.new_doc("Report") + report.update(args) + if not report.report_name: + report.report_name = frappe.generate_hash() + if not report.ref_doctype: + report.ref_doctype = "ToDo" + report.insert() + yield report + finally: + report.delete() From 58501760666bdce4348eaa72df51eef6ec834698 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 18:05:30 +0530 Subject: [PATCH 5/8] fix: Stop running prepared reports when report is deleted --- .../doctype/prepared_report/prepared_report.py | 10 ++++++++++ .../prepared_report/test_prepared_report.py | 18 ++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/frappe/core/doctype/prepared_report/prepared_report.py b/frappe/core/doctype/prepared_report/prepared_report.py index 7182fa4920..1d768cb6a1 100644 --- a/frappe/core/doctype/prepared_report/prepared_report.py +++ b/frappe/core/doctype/prepared_report/prepared_report.py @@ -3,6 +3,7 @@ import json +from contextlib import suppress from typing import Any from rq import get_current_job @@ -38,6 +39,15 @@ class PreparedReport(Document): def before_insert(self): self.status = "Queued" + def on_trash(self): + # If job is running then send stop signal. + if self.status != "Started": + return + + with suppress(Exception): + job = frappe.get_doc("RQ Job", self.job_id) + job.stop_job() + def after_insert(self): enqueue( generate_report, diff --git a/frappe/core/doctype/prepared_report/test_prepared_report.py b/frappe/core/doctype/prepared_report/test_prepared_report.py index b8dd3d67d0..dbd1294cbb 100644 --- a/frappe/core/doctype/prepared_report/test_prepared_report.py +++ b/frappe/core/doctype/prepared_report/test_prepared_report.py @@ -6,6 +6,8 @@ from contextlib import contextmanager import frappe from frappe.desk.query_report import generate_report_result, get_report_doc +from frappe.query_builder.utils import db_type_is +from frappe.tests.test_query_builder import run_only_if from frappe.tests.utils import FrappeTestCase, timeout @@ -25,7 +27,6 @@ class TestPreparedReport(FrappeTestCase): report.reload() if report.status == status: break - # Cheap blocking behaviour time.sleep(0.5) def create_prepared_report(self, report=None, commit=True): @@ -62,13 +63,17 @@ class TestPreparedReport(FrappeTestCase): self.assertEqual(len(prepared_data["result"]), len(generated_data["result"])) self.assertEqual(len(prepared_data), len(generated_data)) - def test_start_status(self): - if frappe.db.db_type == "postgres": - return - - with test_report(report_type="Query Report", query="select sleep(5)") as report: + @run_only_if(db_type_is.MARIADB) + def test_start_status_and_kill_jobs(self): + with test_report(report_type="Query Report", query="select sleep(10)") as report: doc = self.create_prepared_report(report.name) self.wait_for_status(doc, "Started") + job_id = doc.job_id + + doc.delete() + time.sleep(1) + job = frappe.get_doc("RQ Job", job_id) + self.assertEqual(job.status, "stopped") @contextmanager @@ -81,6 +86,7 @@ def test_report(**args): if not report.ref_doctype: report.ref_doctype = "ToDo" report.insert() + frappe.db.commit() yield report finally: report.delete() From 4ece3da47d7a664de56b2e48dfdea2fd6003a1a3 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 13:14:00 +0530 Subject: [PATCH 6/8] test: publish_progress ui test --- cypress/integration/socket_updates.js | 7 +++++++ frappe/tests/ui_test_helpers.py | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/cypress/integration/socket_updates.js b/cypress/integration/socket_updates.js index 38d13a500e..9f8c55b938 100644 --- a/cypress/integration/socket_updates.js +++ b/cypress/integration/socket_updates.js @@ -65,6 +65,13 @@ context("Realtime updates", () => { cy.get("@callback").should("be.called"); }); }); + + it("Progress bar", { scrollBehavior: false }, () => { + const title = "RealTime Progress"; + cy.call("frappe.tests.ui_test_helpers.publish_progress", { title }).then(() => { + cy.contains(title).should("be.visible"); + }); + }); }); function publish_realtime(args) { diff --git a/frappe/tests/ui_test_helpers.py b/frappe/tests/ui_test_helpers.py index fd0fdbdc5b..8920872069 100644 --- a/frappe/tests/ui_test_helpers.py +++ b/frappe/tests/ui_test_helpers.py @@ -642,3 +642,19 @@ def publish_realtime( docname=docname, task_id=task_id, ) + + +@whitelist_for_tests +def publish_progress(duration=3, title=None, doctype=None, docname=None): + # This should consider session user and only show it to current user. + frappe.enqueue(slow_task, duration=duration, title=title, doctype=doctype, docname=docname) + + +def slow_task(duration, title, doctype, docname): + import time + + steps = 10 + + for i in range(steps + 1): + frappe.publish_progress(i * 10, title=title, doctype=doctype, docname=docname) + time.sleep(int(duration) / steps) From 2e8ea0202810c3a68b4946cccb2ef9e9b6875509 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 18:24:57 +0530 Subject: [PATCH 7/8] fix: Expire stalled reports Mark any report that take more than 60 minutes as failed --- .../prepared_report/prepared_report.py | 23 +++++++++++++++++-- frappe/hooks.py | 6 ++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/frappe/core/doctype/prepared_report/prepared_report.py b/frappe/core/doctype/prepared_report/prepared_report.py index 1d768cb6a1..30efa8eb91 100644 --- a/frappe/core/doctype/prepared_report/prepared_report.py +++ b/frappe/core/doctype/prepared_report/prepared_report.py @@ -13,9 +13,13 @@ from frappe.desk.form.load import get_attachments from frappe.desk.query_report import generate_report_result from frappe.model.document import Document from frappe.monitor import add_data_to_monitor -from frappe.utils import gzip_compress, gzip_decompress +from frappe.utils import add_to_date, gzip_compress, gzip_decompress, now from frappe.utils.background_jobs import enqueue +# If prepared report runs for longer than this time it's automatically considered as failed +FAILURE_THRESHOLD = 60 * 60 +REPORT_TIMEOUT = 25 * 60 + class PreparedReport(Document): @property @@ -53,7 +57,7 @@ class PreparedReport(Document): generate_report, queue="long", prepared_report=self.name, - timeout=1500, + timeout=REPORT_TIMEOUT, enqueue_after_commit=True, ) @@ -171,6 +175,21 @@ def get_completed_prepared_report(filters, user, report_name): ) +def expire_stalled_report(): + frappe.db.set_value( + "Prepared Report", + { + "status": "Started", + "modified": ("<", add_to_date(now(), seconds=-FAILURE_THRESHOLD, as_datetime=True)), + }, + { + "status": "Failed", + "error_message": frappe._("Report timed out."), + }, + update_modified=False, + ) + + @frappe.whitelist() def delete_prepared_reports(reports): reports = frappe.parse_json(reports) diff --git a/frappe/hooks.py b/frappe/hooks.py index 85a28feb39..fe430918d0 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -196,9 +196,9 @@ scheduler_events = { "frappe.email.doctype.email_account.email_account.pull", ], # Hourly but offset by 30 minutes - # "30 * * * *": [ - # - # ], + "30 * * * *": [ + "frappe.core.doctype.prepared_report.prepared_report.expire_stalled_report", + ], # Daily but offset by 45 minutes "45 0 * * *": [ "frappe.core.doctype.log_settings.log_settings.run_log_clean_up", From e342f7bd9cd14a951d2a7b0b0795fc079eca07a2 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 1 Jul 2023 18:29:38 +0530 Subject: [PATCH 8/8] perf: index status and report names Two commonly used fields to filter by --- frappe/core/doctype/prepared_report/prepared_report.json | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frappe/core/doctype/prepared_report/prepared_report.json b/frappe/core/doctype/prepared_report/prepared_report.json index 135b6a996c..b64b91c4ef 100644 --- a/frappe/core/doctype/prepared_report/prepared_report.json +++ b/frappe/core/doctype/prepared_report/prepared_report.json @@ -25,7 +25,8 @@ "fieldtype": "Data", "label": "Report Name", "read_only": 1, - "reqd": 1 + "reqd": 1, + "search_index": 1 }, { "default": "Queued", @@ -36,7 +37,8 @@ "in_standard_filter": 1, "label": "Status", "options": "Error\nQueued\nCompleted\nStarted", - "read_only": 1 + "read_only": 1, + "search_index": 1 }, { "fieldname": "column_break_4", @@ -104,7 +106,7 @@ ], "in_create": 1, "links": [], - "modified": "2023-07-01 17:18:40.183106", + "modified": "2023-07-01 18:29:12.700239", "modified_by": "Administrator", "module": "Core", "name": "Prepared Report",