Skip to content

Commit

Permalink
fix: scheduled type syncing
Browse files Browse the repository at this point in the history
- Scheduled Job sync when type was changed from scheduled to some other
  type didn't work.
- It updates on every save with message, bad DX IMO (can't save script
  and edit without dismissing)
- This was because of complex walrus which was triggering rest of code
  even when nothing changed. Maybe walrus opponents were onto something.
- `Truthy` couples two different operations and hence makes code
  complicated. In most cases where these checks are required it's not
  performance critical, we can do 1 more function call to avoid this
  coupling of change + actual value.
  • Loading branch information
ankush committed May 20, 2024
1 parent 9ad376a commit fe05661
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 37 deletions.
37 changes: 21 additions & 16 deletions frappe/core/doctype/server_script/server_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,24 +112,29 @@ def scheduled_jobs(self) -> list[dict[str, str]]:

def sync_scheduled_job_type(self):
"""Create or update Scheduled Job Type documents for Scheduler Event Server Scripts"""
if self.script_type != "Scheduler Event" or (
(previous_script_type := self.has_value_changed("script_type"))
# True will be sent if its a new record
and previous_script_type.value not in (True, "Scheduler Event")

def get_scheduled_job() -> "ScheduledJobType":
if scheduled_script := frappe.db.get_value("Scheduled Job Type", {"server_script": self.name}):
return frappe.get_doc("Scheduled Job Type", scheduled_script)
else:
return frappe.get_doc({"doctype": "Scheduled Job Type", "server_script": self.name})

previous_script_type = self.get_value_before_save("script_type")
if (
previous_script_type != self.script_type
and self.get_value_before_save("script_type") == "Scheduled Event"
):
get_scheduled_job().update({"stopped": 1}).save()
return

if scheduled_script := frappe.db.get_value("Scheduled Job Type", {"server_script": self.name}):
scheduled_job_type: "ScheduledJobType" = frappe.get_doc("Scheduled Job Type", scheduled_script)
else:
scheduled_job_type: "ScheduledJobType" = frappe.get_doc(
{
"doctype": "Scheduled Job Type",
"server_script": self.name,
}
)

scheduled_job_type.update(
if self.script_type != "Scheduler Event" or not (
self.has_value_changed("event_frequency")
or self.has_value_changed("cron_format")
or self.has_value_changed("disabled")
):
return

get_scheduled_job().update(
{
"method": frappe.scrub(f"{self.name}-{self.event_frequency}"),
"frequency": self.event_frequency,
Expand All @@ -138,7 +143,7 @@ def sync_scheduled_job_type(self):
}
).save()

frappe.msgprint(_("Scheduled execution for script {0} has updated").format(self.name))
frappe.msgprint(_("Scheduled execution for script {0} has updated").format(self.name), alert=True)

def check_if_compilable_in_restricted_context(self):
"""Check compilation errors and send them back as warnings."""
Expand Down
16 changes: 13 additions & 3 deletions frappe/model/document.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from frappe.model.utils import is_virtual_doctype
from frappe.model.workflow import set_workflow_state_on_action, validate_workflow
from frappe.types import DF
from frappe.utils import Truthy, compare, cstr, date_diff, file_lock, flt, now
from frappe.utils import compare, cstr, date_diff, file_lock, flt, now
from frappe.utils.data import get_absolute_url, get_datetime, get_timedelta, getdate
from frappe.utils.global_search import update_global_search

Expand Down Expand Up @@ -468,7 +468,7 @@ def has_value_changed(self, fieldname):
previous = self.get_doc_before_save()

if not previous:
return Truthy(context="New Document")
return True

previous_value = previous.get(fieldname)
current_value = self.get(fieldname)
Expand All @@ -481,10 +481,20 @@ def has_value_changed(self, fieldname):
current_value = get_timedelta(current_value)

if previous_value != current_value:
return Truthy(value=previous_value)
return True

return False

def get_value_before_save(self, fieldname):
"""Returns value of a field before saving
Note: This function only works in save context like doc.save, doc.submit.
"""
previous = self.get_doc_before_save()
if not previous:
return
return previous.get(fieldname)

def set_new_name(self, force=False, set_name=None, set_child_names=True):
"""Calls `frappe.naming.set_new_name` for parent and child docs."""

Expand Down
18 changes: 0 additions & 18 deletions frappe/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1168,21 +1168,3 @@ def run(self):

def reset(self):
self._functions.clear()


class Truthy:
def __init__(self, value=True, context=UNSET):
self.value = value
self.context = context

def __bool__(self):
return True

def __eq__(self, other: object) -> bool:
return True == other # noqa: E712

def __repr__(self) -> str:
_val = "UNSET" if self.value is UNSET else self.value
_ctx = "UNSET" if self.context is UNSET else self.context

return f"Truthy(value={_val}, context={_ctx})"

0 comments on commit fe05661

Please sign in to comment.