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

Override a function to improve error msg #205

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

viralkansodiya
Copy link
Contributor


def validate_allocated_amount_with_latest_data(self):
if self.references:
uniq_vouchers = {(x.reference_doctype, x.reference_name) for x in self.references}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a matter of personal taste, but I'm not a big fan of this kind of abbreviated variable naming. In this context, I'd argue that vouchers is fine, all_vouchers would be good if there's a concern about reusing vouchers (mypy might complain about reassignment and that's how you ended up here). unique_vouchers is only two characters longer and improves readability by being a complete and recognizable word, where "uniq" is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rename variable 'uniq_vouchers' to 'unique_vouchers' in override payment_entry.py

@@ -53,6 +57,144 @@ def get_valid_reference_doctypes(self):
elif self.party_type == "Employee":
return ("Journal Entry", "Expense Claim") # Expense Claim

# validation msg improvement
Copy link
Owner

@agritheory agritheory Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have to copy both of these function in order to improve the error message, it's probably good to have a little bit longer note here about why.

"""
Because Check Run processes multiple payment entries in a background queue, errors generally do not include
enough data to identify the problem since there were written and remain appropriate for the context of an individual
Payment Entry. This code is copied from: 

https://github.com/frappe/erpnext/blob/version-14/erpnext/accounts/doctype/payment_entry/payment_entry.py#L164

https://github.com/frappe/erpnext/blob/version-14/erpnext/accounts/doctype/payment_entry/payment_entry.py#L194
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment as you suggest

@agritheory agritheory changed the base branch from version-14 to staging February 6, 2024 16:06
@agritheory agritheory merged commit ddfde87 into agritheory:staging Feb 6, 2024
agritheory pushed a commit that referenced this pull request Feb 6, 2024
* Allocated ammount validation override for msg improvement

* comment add

* add a comment on function

* Refactor: Rename 'uniq_vouchers' to 'unique_vouchers' in payment_entry.py

---------

Co-authored-by: viralpatel15 <[email protected]>
@viralkansodiya viralkansodiya deleted the PEchanges branch February 9, 2024 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants