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

refactor: Use status_updater as a class property #46600

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

marination
Copy link
Collaborator

@marination marination commented Mar 18, 2025

Issue

  • The current design entirely relies on __init__ to resolve/set the status_updater attribute
  • Any changes to this attribute are later manually done elsewhere before evaluating the status_updater (on submit/cancel)
  • This works fine via desk because each save/submit/cancel action creates a new instance of the class and so status_updater is (re)initialized with the doc's newer values
  • This does not work fine/is not intuitive via scripts/tests. I discovered this while writing a test

Example:

Consider the status_updater in delivery_note.py where:

1 def __init__(self):
2	# Watered down for obvious reasons
3	...
4	self.status_updater = [1, 2]
5	if cint(self.is_return):
6		self.status_updater.extend([3,4])

On instantiation of class DeliveryNote if it does not have the document values, line 5 does not evaluate to True ever.

# Creates an instance of a Delivery Note -> runs `__init__` -> `self.status_updater` is computed and set
# `self.is_return` is falsy. So even though we intend to create a return document the status updater has an incorrect state
1	delivery_note_return = frappe.new_doc("Delivery Note")
# `self.status_updater` is already initialized so this has no automatic impact
2	delivery_note_return.is_return = 1
3	...
# Does not back update some fields because [3,4] are missing
4	delivery_note.submit()

_Our solution here is to modify this attribute explicitly, which now has to be done in tests as well._

Solution

  • Switch this attribute to a property that dynamically determines status_updater based on the document's current state
  • Behaviours are now predictable and status_updater's value definition is in one place

A pattern like:

def __init__(self):
	...
	self.status_updater = [1]

def on_submit(self):
	if self.some_value:
		self.status_updater = []
	self.status_updater.append(2)

	self.update_prevdoc_status()  # Applies the status updater

def on_cancel(self):
	if self.some_value:
		self.status_updater = []
	self.status_updater.append(2)

	self.update_prevdoc_status()  # Applies the status updater

Is rewritten like this:

def __init__(self):
	...

@property
def status_updater(self):
	updater = [2]
	if not self.some_value:
		updater.append(1)
	return updater

def on_submit(self):
	self.update_prevdoc_status()  # Applies the status updater

def on_cancel(self):
	self.update_prevdoc_status()  # Applies the status updater

Todo:

  • Add a setter for the property as other apps might use self.status_updater = ...

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Mar 18, 2025
@marination
Copy link
Collaborator Author

Buggy behaviour in tests is highlighted even more with breaking tests 😄
Eg:

  • In many tests, return documents (like a return delivery note), did not update the linked previous delivery note
  • Now that it does (it behaves exactly like it does in desk!), we have timestamp errors because the first DN doc instance needs to be reloaded before cancellation (return DN modified it)

@marination marination added the CI-failing Unit tests or patch tests are failing. label Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-failing Unit tests or patch tests are failing. needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant