From 82d699a801e3bb5c41124173ca2b4f71ace32f20 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 14 Dec 2022 11:44:39 +0530 Subject: [PATCH 1/4] refactor: misleading "log and raise" and types --- frappe/utils/scheduler.py | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/frappe/utils/scheduler.py b/frappe/utils/scheduler.py index 9c730eaffa..73ba5a0339 100755 --- a/frappe/utils/scheduler.py +++ b/frappe/utils/scheduler.py @@ -11,6 +11,7 @@ Events: # imports - standard imports import os import time +from typing import NoReturn # imports - module imports import frappe @@ -30,7 +31,7 @@ def cprint(*args, **kwargs): pass -def start_scheduler(): +def start_scheduler() -> NoReturn: """Run enqueue_events_for_all_sites based on scheduler tick. Specify scheduler_interval in seconds in common_site_config.json""" @@ -41,7 +42,7 @@ def start_scheduler(): enqueue_events_for_all_sites() -def enqueue_events_for_all_sites(): +def enqueue_events_for_all_sites() -> None: """Loop through sites and enqueue events that are not already queued""" if os.path.exists(os.path.join(".", ".restarting")): @@ -54,16 +55,13 @@ def enqueue_events_for_all_sites(): for site in sites: try: enqueue_events_for_site(site=site) - except Exception as e: - print(e.__class__, f"Failed to enqueue events for site: {site}") + except Exception: + frappe.logger("scheduler").debug(f"Failed to enqueue events for site: {site}", exc_info=True) -def enqueue_events_for_site(site): - def log_and_raise(): - error_message = "Exception in Enqueue Events for Site {}\n{}".format( - site, frappe.get_traceback() - ) - frappe.logger("scheduler").error(error_message) +def enqueue_events_for_site(site: str) -> None: + def log_exc(): + frappe.logger("scheduler").error(f"Exception in Enqueue Events for Site {site}", exc_info=True) try: frappe.init(site=site) @@ -74,29 +72,26 @@ def enqueue_events_for_site(site): enqueue_events(site=site) frappe.logger("scheduler").debug(f"Queued events for site {site}") - except frappe.db.OperationalError as e: + except Exception as e: if frappe.db.is_access_denied(e): frappe.logger("scheduler").debug(f"Access denied for site {site}") - else: - log_and_raise() - except Exception: - log_and_raise() + log_exc() finally: frappe.destroy() -def enqueue_events(site): +def enqueue_events(site: str) -> None: if schedule_jobs_based_on_activity(): frappe.flags.enqueued_jobs = [] queued_jobs = get_jobs(site=site, key="job_type").get(site) or [] for job_type in frappe.get_all("Scheduled Job Type", ("name", "method"), dict(stopped=0)): if not job_type.method in queued_jobs: # don't add it to queue if still pending - frappe.get_doc("Scheduled Job Type", job_type.name).enqueue() + frappe.get_cached_doc("Scheduled Job Type", job_type.name).enqueue() -def is_scheduler_inactive(): +def is_scheduler_inactive() -> bool: if frappe.local.conf.maintenance_mode: cprint(f"{frappe.local.site}: Maintenance mode is ON") return True @@ -111,7 +106,7 @@ def is_scheduler_inactive(): return False -def is_scheduler_disabled(): +def is_scheduler_disabled() -> bool: if frappe.conf.disable_scheduler: cprint(f"{frappe.local.site}: frappe.conf.disable_scheduler is SET") return True From 2b050f9fc3fb7b01ae420c8f5c0a13d04339db27 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 14 Dec 2022 12:40:06 +0530 Subject: [PATCH 2/4] test: test without adding hacky flags --- .../scheduled_job_type/scheduled_job_type.py | 21 +++++++------------ frappe/tests/test_scheduler.py | 15 ++++++------- frappe/utils/scheduler.py | 13 ++++++++---- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py index 1c178fcee2..3ae3df7693 100644 --- a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py @@ -25,20 +25,13 @@ class ScheduledJobType(Document): def enqueue(self, force=False): # enqueue event if last execution is done if self.is_event_due() or force: - if frappe.flags.enqueued_jobs: - frappe.flags.enqueued_jobs.append(self.method) - - if frappe.flags.execute_job: - self.execute() - else: - if not self.is_job_in_queue(): - enqueue( - "frappe.core.doctype.scheduled_job_type.scheduled_job_type.run_scheduled_job", - queue=self.get_queue_name(), - job_type=self.method, - ) - return True - + if not self.is_job_in_queue(): + enqueue( + "frappe.core.doctype.scheduled_job_type.scheduled_job_type.run_scheduled_job", + queue=self.get_queue_name(), + job_type=self.method, + ) + return True return False def is_event_due(self, current_time=None): diff --git a/frappe/tests/test_scheduler.py b/frappe/tests/test_scheduler.py index 0cefd4843e..4e4b67c603 100644 --- a/frappe/tests/test_scheduler.py +++ b/frappe/tests/test_scheduler.py @@ -34,18 +34,19 @@ class TestScheduler(TestCase): if not frappe.get_all("Scheduled Job Type", limit=1): sync_jobs() + def tearDown(self): + purge_pending_jobs() + def test_enqueue_jobs(self): frappe.db.sql("update `tabScheduled Job Type` set last_execution = '2010-01-01 00:00:00'") - frappe.flags.execute_job = True - enqueue_events(site=frappe.local.site) - frappe.flags.execute_job = False + enqueued_jobs = enqueue_events(site=frappe.local.site) - self.assertTrue("frappe.email.queue.set_expiry_for_email_queue", frappe.flags.enqueued_jobs) - self.assertTrue("frappe.utils.change_log.check_for_update", frappe.flags.enqueued_jobs) - self.assertTrue( + self.assertIn("frappe.email.queue.set_expiry_for_email_queue", enqueued_jobs) + self.assertIn("frappe.utils.change_log.check_for_update", enqueued_jobs) + self.assertIn( "frappe.email.doctype.auto_email_report.auto_email_report.send_monthly", - frappe.flags.enqueued_jobs, + enqueued_jobs, ) def test_queue_peeking(self): diff --git a/frappe/utils/scheduler.py b/frappe/utils/scheduler.py index 73ba5a0339..05399c0e91 100755 --- a/frappe/utils/scheduler.py +++ b/frappe/utils/scheduler.py @@ -81,14 +81,19 @@ def enqueue_events_for_site(site: str) -> None: frappe.destroy() -def enqueue_events(site: str) -> None: +def enqueue_events(site: str) -> list[str] | None: if schedule_jobs_based_on_activity(): - frappe.flags.enqueued_jobs = [] queued_jobs = get_jobs(site=site, key="job_type").get(site) or [] - for job_type in frappe.get_all("Scheduled Job Type", ("name", "method"), dict(stopped=0)): + + enqueued_jobs = [] + for job_type in frappe.get_all("Scheduled Job Type", ("name", "method"), {"stopped": 0}): if not job_type.method in queued_jobs: # don't add it to queue if still pending - frappe.get_cached_doc("Scheduled Job Type", job_type.name).enqueue() + job_type = frappe.get_cached_doc("Scheduled Job Type", job_type.name) + if job_type.enqueue(): + enqueued_jobs.append(job_type.method) + + return enqueued_jobs def is_scheduler_inactive() -> bool: From d13ab320b7248fbbda60434954478de31ff240c0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 14 Dec 2022 12:44:37 +0530 Subject: [PATCH 3/4] refactor: Duplicate enqueue checks scheduled_job_type.enqueue already does check for duplicate job and is more "recent" --- .../doctype/scheduled_job_type/scheduled_job_type.py | 2 +- frappe/utils/scheduler.py | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py index 3ae3df7693..a8a2e25891 100644 --- a/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py +++ b/frappe/core/doctype/scheduled_job_type/scheduled_job_type.py @@ -22,7 +22,7 @@ class ScheduledJobType(Document): # force logging for all events other than continuous ones (ALL) self.create_log = 1 - def enqueue(self, force=False): + def enqueue(self, force=False) -> bool: # enqueue event if last execution is done if self.is_event_due() or force: if not self.is_job_in_queue(): diff --git a/frappe/utils/scheduler.py b/frappe/utils/scheduler.py index 05399c0e91..6a8cf8b5a9 100755 --- a/frappe/utils/scheduler.py +++ b/frappe/utils/scheduler.py @@ -83,15 +83,11 @@ def enqueue_events_for_site(site: str) -> None: def enqueue_events(site: str) -> list[str] | None: if schedule_jobs_based_on_activity(): - queued_jobs = get_jobs(site=site, key="job_type").get(site) or [] - enqueued_jobs = [] for job_type in frappe.get_all("Scheduled Job Type", ("name", "method"), {"stopped": 0}): - if not job_type.method in queued_jobs: - # don't add it to queue if still pending - job_type = frappe.get_cached_doc("Scheduled Job Type", job_type.name) - if job_type.enqueue(): - enqueued_jobs.append(job_type.method) + job_type = frappe.get_cached_doc("Scheduled Job Type", job_type.name) + if _enqueued := job_type.enqueue(): + enqueued_jobs.append(job_type.method) return enqueued_jobs From 4b1f65206f43447e29af5dd7ba1bc811484e40a8 Mon Sep 17 00:00:00 2001 From: Shariq Ansari Date: Wed, 14 Dec 2022 17:29:58 +0530 Subject: [PATCH 4/4] fix: if grid custom button is not set grid has some extra space on top --- frappe/public/scss/common/grid.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frappe/public/scss/common/grid.scss b/frappe/public/scss/common/grid.scss index 43c45ff3b5..b3781d5501 100644 --- a/frappe/public/scss/common/grid.scss +++ b/frappe/public/scss/common/grid.scss @@ -402,6 +402,10 @@ display: inline-flex; } +.grid-custom-buttons:empty { + padding: 0; +} + .grid-footer, .grid-custom-buttons { padding: var(--padding-sm) 0px; background-color: var(--fg-color);