Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scheduled type syncing #26490

Merged
merged 1 commit into from
May 20, 2024
Merged

fix: scheduled type syncing #26490

merged 1 commit into from
May 20, 2024

Conversation

ankush
Copy link
Member

@ankush ankush commented May 20, 2024

  • 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.

There are two distinct cases that need to be handled while syncing:

  1. Switching away from scheduled type - Stop script and nothing
    else (not handled correctly)
  2. Switching other script fields -> sync script. (handled right
    now but overly executed)

@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label May 20, 2024
@ankush ankush force-pushed the exec_script branch 5 times, most recently from 5ae6053 to 6eda7bb Compare May 20, 2024 13:28
@ankush ankush marked this pull request as ready for review May 20, 2024 13:28
- 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.
@ankush ankush added do not backport and removed add-test-cases Add test case to validate fix or enhancement labels May 20, 2024
@ankush ankush enabled auto-merge (squash) May 20, 2024 13:34
@ankush ankush merged commit 0257276 into frappe:develop May 20, 2024
23 checks passed
@ankush ankush deleted the exec_script branch May 20, 2024 13:43
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant