From d6a41cd2722d1776da774ab2d7521233b5663116 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 9 Feb 2023 15:49:47 +0530 Subject: [PATCH 01/13] feat: Before/After Request Hooks --- frappe/app.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frappe/app.py b/frappe/app.py index 2fe9991c4c..3acae39fb7 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -119,6 +119,9 @@ def init_request(request): if request.method != "OPTIONS": frappe.local.http_request = HTTPRequest() + for before_request_task in frappe.get_hooks("before_request"): + frappe.call(before_request_task) + def setup_read_only_mode(): """During maintenance_mode reads to DB can still be performed to reduce downtime. This @@ -318,7 +321,10 @@ def handle_exception(e): return response -def after_request(rollback): +def after_request(rollback: bool) -> bool: + for after_request_task in frappe.get_hooks("after_request"): + frappe.call(after_request_task) + # if HTTP method would change server state, commit if necessary if ( frappe.db From 34731d1e9e9fa64fbd64f9266b5ac1b8923f7b02 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Feb 2023 11:37:36 +0530 Subject: [PATCH 02/13] feat: Befor/After Job Hooks --- frappe/utils/background_jobs.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 040a57cc11..3c8a731369 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -168,6 +168,10 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, method_name = cstr(method.__name__) frappe.monitor.start("job", method_name, kwargs) + + for before_job_task in frappe.get_hooks("before_job"): + frappe.call(before_job_task, method=method_name, kwargs=kwargs) + try: method(**kwargs) @@ -202,6 +206,9 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, frappe.db.commit() finally: + for after_job_task in frappe.get_hooks("after_job"): + frappe.call(after_job_task, method=method_name, kwargs=kwargs) + # background job hygiene: release file locks if unreleased # if this breaks something, move it to failed jobs alone - gavin@frappe.io for doc in frappe.local.locked_documents: From 0d547cf71c9057a2d93c5c26902c9403a3b718fb Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Feb 2023 12:55:00 +0530 Subject: [PATCH 03/13] test: Add tests for job sanity + hooks --- frappe/tests/test_background_jobs.py | 83 +++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_background_jobs.py b/frappe/tests/test_background_jobs.py index 1b6680eb9b..72660128cf 100644 --- a/frappe/tests/test_background_jobs.py +++ b/frappe/tests/test_background_jobs.py @@ -1,11 +1,19 @@ import time +from contextlib import contextmanager +from unittest.mock import patch from rq import Queue import frappe from frappe.core.doctype.rq_job.rq_job import remove_failed_jobs from frappe.tests.utils import FrappeTestCase -from frappe.utils.background_jobs import generate_qname, get_redis_conn +from frappe.utils.background_jobs import ( + RQ_JOB_FAILURE_TTL, + RQ_RESULTS_TTL, + execute_job, + generate_qname, + get_redis_conn, +) class TestBackgroundJobs(FrappeTestCase): @@ -44,6 +52,79 @@ class TestBackgroundJobs(FrappeTestCase): # lesser is earlier self.assertTrue(high_priority_job.get_position() < low_priority_job.get_position()) + def test_enqueue_call(self): + with patch.object(Queue, "enqueue_call") as mock_enqueue_call: + frappe.enqueue( + "frappe.handler.ping", + queue="short", + timeout=300, + kwargs={"site": frappe.local.site}, + ) + + mock_enqueue_call.assert_called_once_with( + execute_job, + on_success=None, + on_failure=None, + timeout=300, + kwargs={ + "site": frappe.local.site, + "user": "Administrator", + "method": "frappe.handler.ping", + "event": None, + "job_name": "frappe.handler.ping", + "is_async": True, + "kwargs": {"kwargs": {"site": frappe.local.site}}, + }, + at_front=False, + failure_ttl=RQ_JOB_FAILURE_TTL, + result_ttl=RQ_RESULTS_TTL, + ) + + def test_job_hooks(self): + self.addCleanup(lambda: _test_JOB_HOOK.clear()) + with freeze_local() as locals, frappe.init_site(locals.site), patch( + "frappe.get_hooks", patch_job_hooks + ): + frappe.connect() + self.assertIsNone(_test_JOB_HOOK.get("before_job")) + r = execute_job( + site=frappe.local.site, + user="Administrator", + method="frappe.handler.ping", + event=None, + job_name="frappe.handler.ping", + is_async=True, + kwargs={}, + ) + self.assertEqual(r, "pong") + self.assertLess(_test_JOB_HOOK.get("before_job"), _test_JOB_HOOK.get("after_job")) + def fail_function(): return 1 / 0 + + +_test_JOB_HOOK = {} + + +def before_job(*args, **kwargs): + _test_JOB_HOOK["before_job"] = time.time() + + +def after_job(*args, **kwargs): + _test_JOB_HOOK["after_job"] = time.time() + + +@contextmanager +def freeze_local(): + locals = frappe.local + frappe.local = frappe.Local() + yield locals + frappe.local = locals + + +def patch_job_hooks(event: str): + return { + "before_job": ["frappe.tests.test_background_jobs.before_job"], + "after_job": ["frappe.tests.test_background_jobs.after_job"], + }[event] From 55b31fcdfae7cabfa55b7a55bea29228118295e1 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Feb 2023 13:22:27 +0530 Subject: [PATCH 04/13] test: Added test for before/after request hook --- frappe/tests/test_api.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 1085f4c39e..c345d8fbcf 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -2,6 +2,7 @@ import sys from contextlib import contextmanager from random import choice from threading import Thread +from time import time from unittest.mock import patch import requests @@ -306,3 +307,36 @@ class TestReadOnlyMode(FrappeAPITestCase): response = self.post(self.REQ_PATH, {"description": frappe.mock("paragraph"), "sid": self.sid}) self.assertEqual(response.status_code, 503) self.assertEqual(response.json["exc_type"], "InReadOnlyMode") + + +class TestWSGIApp(FrappeAPITestCase): + def test_request_hooks(self): + self.addCleanup(lambda: _test_REQ_HOOK.clear()) + get_hooks = frappe.get_hooks + + def patch_request_hooks(event: str, *args, **kwargs): + patched_hooks = { + "before_request": ["frappe.tests.test_api.before_request"], + "after_request": ["frappe.tests.test_api.after_request"], + } + if event not in patched_hooks: + return get_hooks(event, *args, **kwargs) + return patched_hooks[event] + + with patch("frappe.get_hooks", patch_request_hooks): + self.assertIsNone(_test_REQ_HOOK.get("before_request")) + self.assertIsNone(_test_REQ_HOOK.get("after_request")) + res = self.get("/api/method/ping") + self.assertEqual(res.json, {"message": "pong"}) + self.assertLess(_test_REQ_HOOK.get("before_request"), _test_REQ_HOOK.get("after_request")) + + +_test_REQ_HOOK = {} + + +def before_request(*args, **kwargs): + _test_REQ_HOOK["before_request"] = time() + + +def after_request(*args, **kwargs): + _test_REQ_HOOK["after_request"] = time() From 83f3cf1991b4c26ba0fbd5e937670e1cfb9e3afb Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Feb 2023 13:23:24 +0530 Subject: [PATCH 05/13] fix(background_jobs): Pass retval in execute_job --- 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 3c8a731369..2ca34141a9 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -153,6 +153,7 @@ def run_doc_method(doctype, name, doc_method, **kwargs): def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, retry=0): """Executes job in a worker, performs commit/rollback and logs if there is any error""" + retval = None if is_async: frappe.connect(site) if os.environ.get("CI"): @@ -173,7 +174,7 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, frappe.call(before_job_task, method=method_name, kwargs=kwargs) try: - method(**kwargs) + retval = method(**kwargs) except (frappe.db.InternalError, frappe.RetryBackgroundJobError) as e: frappe.db.rollback() @@ -204,6 +205,7 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, else: frappe.db.commit() + return retval finally: for after_job_task in frappe.get_hooks("after_job"): From ea62156a6d028ea764ff8a9aabe7406313e1ab89 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Feb 2023 13:28:50 +0530 Subject: [PATCH 06/13] fix: Add request, job events in hooks boilerplate --- frappe/utils/boilerplate.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frappe/utils/boilerplate.py b/frappe/utils/boilerplate.py index ee75c672a8..1cd57f4695 100644 --- a/frappe/utils/boilerplate.py +++ b/frappe/utils/boilerplate.py @@ -458,6 +458,15 @@ app_license = "{app_license}" # ignore_links_on_delete = ["Communication", "ToDo"] +# Request Events +# ---------------- +# before_request = ["{app_name}.utils.before_request"] +# after_request = ["{app_name}.utils.after_request"] + +# Job Events +# ---------- +# before_job = ["{app_name}.utils.before_job"] +# after_job = ["{app_name}.utils.after_job"] # User Data Protection # -------------------- From 2ada98fbdb1d93e7747d15977769b5cc8a1f030c Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Feb 2023 13:31:46 +0530 Subject: [PATCH 07/13] fix: Pass result of job to after_job hooks --- frappe/utils/background_jobs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index 2ca34141a9..a106c3beba 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -209,7 +209,7 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, finally: for after_job_task in frappe.get_hooks("after_job"): - frappe.call(after_job_task, method=method_name, kwargs=kwargs) + frappe.call(after_job_task, method=method_name, kwargs=kwargs, result=retval) # background job hygiene: release file locks if unreleased # if this breaks something, move it to failed jobs alone - gavin@frappe.io From 6d70b5e93440272b241c70b0bd6e68593be5536a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 15 Feb 2023 14:52:08 +0530 Subject: [PATCH 08/13] fix(app): Move after_request hook inside finally block Also, rename after_request fn to sync_database to better match functionality --- frappe/app.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index ff2b95ecb7..0d80d5dc7c 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -74,12 +74,15 @@ def application(request: Request): response = handle_exception(e) else: - rollback = after_request(rollback) + rollback = sync_database(rollback) finally: if request.method in UNSAFE_HTTP_METHODS and frappe.db and rollback: frappe.db.rollback() + for after_request_task in frappe.get_hooks("after_request"): + frappe.call(after_request_task) + frappe.rate_limiter.update() frappe.monitor.stop(response) frappe.recorder.dump() @@ -321,10 +324,7 @@ def handle_exception(e): return response -def after_request(rollback: bool) -> bool: - for after_request_task in frappe.get_hooks("after_request"): - frappe.call(after_request_task) - +def sync_database(rollback: bool) -> bool: # if HTTP method would change server state, commit if necessary if ( frappe.db @@ -338,9 +338,8 @@ def after_request(rollback: bool) -> bool: rollback = False # update session - if getattr(frappe.local, "session_obj", None): - updated_in_db = frappe.local.session_obj.update() - if updated_in_db: + if session := getattr(frappe.local, "session_obj", None): + if session.update(): frappe.db.commit() rollback = False From fe26c542b77697a2a227ec1559f6536a06ba2fc7 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 15 Feb 2023 15:30:02 +0530 Subject: [PATCH 09/13] refactor: Move before/after tasks as hooks Moved before/after tasks in Requests as hooks for: - monitor - rate_limiter - recorder Moved before/after tasks in Jobs as hooks for: - monitor - releasing document locks --- frappe/app.py | 10 ++-------- frappe/hooks.py | 17 +++++++++++++++++ frappe/utils/background_jobs.py | 10 +--------- frappe/utils/file_lock.py | 8 +++++++- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index 0d80d5dc7c..01a5b5f218 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -35,15 +35,13 @@ _sites_path = os.environ.get("SITES_PATH", ".") @Request.application def application(request: Request): response = None + e = None try: rollback = True init_request(request) - frappe.recorder.record() - frappe.monitor.start() - frappe.rate_limiter.apply() frappe.api.validate_auth() if request.method == "OPTIONS": @@ -81,11 +79,7 @@ def application(request: Request): frappe.db.rollback() for after_request_task in frappe.get_hooks("after_request"): - frappe.call(after_request_task) - - frappe.rate_limiter.update() - frappe.monitor.stop(response) - frappe.recorder.dump() + frappe.call(after_request_task, response=response, request=request, exception=e) log_request(request, response) process_response(response) diff --git a/frappe/hooks.py b/frappe/hooks.py index aa6d6f00c7..317439c358 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -393,3 +393,20 @@ ignore_links_on_delete = [ "Integration Request", "Unhandled Email", ] + +# Request Hooks +before_request = [ + "frappe.recorder.record", + "frappe.monitor.start", + "frappe.rate_limiter.apply", +] +after_request = ["frappe.rate_limiter.update", "frappe.monitor.stop", "frappe.recorder.dump"] + +# Background Job Hooks +before_job = [ + "frappe.monitor.start", +] +after_job = [ + "frappe.monitor.stop", + "frappe.utils.file_lock.release_document_locks", +] diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index a106c3beba..fed822700c 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -168,10 +168,8 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, else: method_name = cstr(method.__name__) - frappe.monitor.start("job", method_name, kwargs) - for before_job_task in frappe.get_hooks("before_job"): - frappe.call(before_job_task, method=method_name, kwargs=kwargs) + frappe.call(before_job_task, method=method_name, kwargs=kwargs, transaction_type="job") try: retval = method(**kwargs) @@ -211,12 +209,6 @@ def execute_job(site, method, event, job_name, kwargs, user=None, is_async=True, for after_job_task in frappe.get_hooks("after_job"): frappe.call(after_job_task, method=method_name, kwargs=kwargs, result=retval) - # background job hygiene: release file locks if unreleased - # if this breaks something, move it to failed jobs alone - gavin@frappe.io - for doc in frappe.local.locked_documents: - doc.unlock() - - frappe.monitor.stop() if is_async: frappe.destroy() diff --git a/frappe/utils/file_lock.py b/frappe/utils/file_lock.py index 5be89c42f6..60d8baec5d 100644 --- a/frappe/utils/file_lock.py +++ b/frappe/utils/file_lock.py @@ -11,7 +11,7 @@ Use `frappe.utils.synchroniztion.filelock` for process synchroniztion. import os from time import time -from frappe import _ +import frappe from frappe.utils import get_site_path, touch_file LOCKS_DIR = "locks" @@ -62,3 +62,9 @@ def get_lock_path(name): name = name.lower() lock_path = get_site_path(LOCKS_DIR, name + ".lock") return lock_path + + +def release_document_locks(): + """Unlocks all documents that were locked by the current context.""" + for doc in frappe.local.locked_documents: + doc.unlock() From 110204e2df335ad7e04884b709a7f290deb683fb Mon Sep 17 00:00:00 2001 From: Hussain Nagaria Date: Thu, 16 Feb 2023 20:05:36 +0530 Subject: [PATCH 10/13] feat: publish button in blog post form --- frappe/website/doctype/blog_post/blog_post.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frappe/website/doctype/blog_post/blog_post.js b/frappe/website/doctype/blog_post/blog_post.js index 0266587f2e..5f7268d074 100644 --- a/frappe/website/doctype/blog_post/blog_post.js +++ b/frappe/website/doctype/blog_post/blog_post.js @@ -7,6 +7,8 @@ frappe.ui.form.on("Blog Post", { frm.set_df_property("hide_cta", "hidden", !value); }); + frm.trigger("add_publish_button"); + generate_google_search_preview(frm); }, title: function (frm) { @@ -30,6 +32,12 @@ frappe.ui.form.on("Blog Post", { }); } }, + add_publish_button(frm) { + frm.add_custom_button(frm.doc.published ? __("Unpublish") : __("Publish"), () => { + frm.set_value("published", !frm.doc.published); + frm.save(); + }); + }, }); function generate_google_search_preview(frm) { From 5d0bd512e10bd3b81c241c0f19d851ec5f62aca1 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 17 Feb 2023 11:24:01 +0530 Subject: [PATCH 11/13] fix(after_hook): Don't pass exception object to hook * it can fetch most relevant details via response object * Exceptions not supported by Frappe's WSGI (unsupported HTTP methods) may not be accessible to the after_request hooks - but the lack of active response may be an indicator / and peeking in the request --- frappe/app.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index 01a5b5f218..6b16464d2c 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -35,7 +35,6 @@ _sites_path = os.environ.get("SITES_PATH", ".") @Request.application def application(request: Request): response = None - e = None try: rollback = True @@ -79,7 +78,7 @@ def application(request: Request): frappe.db.rollback() for after_request_task in frappe.get_hooks("after_request"): - frappe.call(after_request_task, response=response, request=request, exception=e) + frappe.call(after_request_task, response=response, request=request) log_request(request, response) process_response(response) From 24d76375382db510b756eedc268b1a73f1921aa7 Mon Sep 17 00:00:00 2001 From: Hussain Nagaria Date: Fri, 17 Feb 2023 11:31:53 +0530 Subject: [PATCH 12/13] fix: hide published checkbox --- frappe/website/doctype/blog_post/blog_post.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/website/doctype/blog_post/blog_post.json b/frappe/website/doctype/blog_post/blog_post.json index 8b5ab54ba8..d41b344464 100644 --- a/frappe/website/doctype/blog_post/blog_post.json +++ b/frappe/website/doctype/blog_post/blog_post.json @@ -53,6 +53,7 @@ "default": "0", "fieldname": "published", "fieldtype": "Check", + "hidden": 1, "label": "Published" }, { @@ -215,7 +216,7 @@ "is_published_field": "published", "links": [], "make_attachments_public": 1, - "modified": "2022-10-18 10:09:10.550734", + "modified": "2023-02-17 11:31:32.223524", "modified_by": "Administrator", "module": "Website", "name": "Blog Post", From 649c211b9bce0fdae7399dc1c0a59dcf8cb4621e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 20 Feb 2023 10:13:49 +0530 Subject: [PATCH 13/13] fix(UX): show message when form is read only (#20077) [skip ci] --- frappe/public/js/frappe/form/form.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index 001279f394..d1fe443f7a 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -406,7 +406,10 @@ frappe.ui.form.Form = class FrappeForm { // read only (workflow) this.read_only = frappe.workflow.is_read_only(this.doctype, this.docname); - if (this.read_only) this.set_read_only(true); + if (this.read_only) { + this.set_read_only(true); + frappe.show_alert(__("This form is not editable due to a Workflow.")); + } // check if doctype is already open if (!this.opendocs[this.docname]) {