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.

Sign up for GitHub

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

Jump to bottom

Override a function to improve error msg #205

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Override a function to improve error msg #205

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
345ee53
Allocated ammount validation override for msg improvement
viralpatel15 Feb 5, 2024
566deed
comment add
viralpatel15 Feb 5, 2024
bac1984
add a comment on function
viralpatel15 Feb 6, 2024
6883c44
Refactor: Rename 'uniq_vouchers' to 'unique_vouchers' in payment_entr…
viralpatel15 Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 144 additions & 2 deletions check_run/overrides/payment_entry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@
# For license information, please see license.txt

import frappe
from frappe.utils import get_link_to_form, comma_and
from erpnext.accounts.doctype.payment_entry.payment_entry import PaymentEntry
from frappe.utils import get_link_to_form, comma_and, flt
from erpnext.accounts.general_ledger import make_gl_entries, process_gl_map
from frappe.utils.data import getdate
from erpnext.accounts.doctype.payment_entry.payment_entry import (
PaymentEntry,
get_outstanding_reference_documents,
)
from frappe import _
import json


Expand Down Expand Up @@ -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 •
edited
Loading

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

def validate_allocated_amount(self):
if self.payment_type == "Internal Transfer":
return

if self.party_type in ("Customer", "Supplier"):
self.validate_allocated_amount_with_latest_data()
else:
fail_message = _(
"{0} Row {1} / {2}: Allocated Amount of {3} cannot be greater than outstanding amount of {4}."
)
for d in self.get("references"):
if (flt(d.allocated_amount)) > 0 and flt(d.allocated_amount) > flt(d.outstanding_amount):
frappe.throw(
fail_message.format(
self.party_name,
d.idx,
get_link_to_form(d.reference_doctype, d.reference_name),
d.allocated_amount,
d.outstanding_amount,
)
)

# Check for negative outstanding invoices as well
if flt(d.allocated_amount) < 0 and flt(d.allocated_amount) < flt(d.outstanding_amount):
frappe.throw(
fail_message.format(
self.party_name,
d.idx,
get_link_to_form(d.reference_doctype, d.reference_name),
d.allocated_amount,
d.outstanding_amount,
)
)

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

vouchers = [frappe._dict({"voucher_type": x[0], "voucher_no": x[1]}) for x in uniq_vouchers]
latest_references = get_outstanding_reference_documents(
{
"posting_date": self.posting_date,
"company": self.company,
"party_type": self.party_type,
"payment_type": self.payment_type,
"party": self.party,
"party_account": self.paid_from if self.payment_type == "Receive" else self.paid_to,
"get_outstanding_invoices": True,
"get_orders_to_be_billed": True,
"vouchers": vouchers,
}
)

# Group latest_references by (voucher_type, voucher_no)
latest_lookup = {}
for d in latest_references:
d = frappe._dict(d)
latest_lookup.setdefault((d.voucher_type, d.voucher_no), frappe._dict())[d.payment_term] = d

for idx, d in enumerate(self.get("references"), start=1):
latest = latest_lookup.get((d.reference_doctype, d.reference_name)) or frappe._dict()

# If term based allocation is enabled, throw
if (
d.payment_term is None or d.payment_term == ""
) and self.term_based_allocation_enabled_for_reference(
d.reference_doctype, d.reference_name
):
frappe.throw(
_(
"{0} has Payment Term based allocation enabled. Select a Payment Term for Row #{1} in Payment References section"
).format(frappe.bold(d.reference_name), frappe.bold(idx))
)

# if no payment template is used by invoice and has a custom term(no `payment_term`), then invoice outstanding will be in 'None' key
latest = latest.get(d.payment_term) or latest.get(None)
# The reference has already been fully paid
if not latest:
frappe.throw(
_("{0} {1} has already been fully paid.").format(_(d.reference_doctype), d.reference_name)
)
# The reference has already been partly paid
elif (
latest.outstanding_amount < latest.invoice_amount
and flt(d.outstanding_amount, d.precision("outstanding_amount"))
!= flt(latest.outstanding_amount, d.precision("outstanding_amount"))
and d.payment_term == ""
):
frappe.throw(
_(
"{0} {1} has already been partly paid. Please use the 'Get Outstanding Invoice' or the 'Get Outstanding Orders' button to get the latest outstanding amounts."
).format(_(d.reference_doctype), d.reference_name)
)

fail_message = _(
"<b>Row #{1}</b> {0} / {2}: Allocated Amount of {3} cannot be greater than outstanding amount of {4}."
)

if (
d.payment_term
and (
(flt(d.allocated_amount)) > 0
and latest.payment_term_outstanding
and (flt(d.allocated_amount) > flt(latest.payment_term_outstanding))
)
and self.term_based_allocation_enabled_for_reference(d.reference_doctype, d.reference_name)
):
frappe.throw(
_(
"Row #{0}: Allocated amount:{1} is greater than outstanding amount:{2} for Payment Term {3}"
).format(
d.idx, d.allocated_amount, latest.payment_term_outstanding, d.payment_term
)
)

if (flt(d.allocated_amount)) > 0 and flt(d.allocated_amount) > flt(latest.outstanding_amount):
frappe.throw(
fail_message.format(
self.party_name,
d.idx,
get_link_to_form(d.reference_doctype, d.reference_name),
d.allocated_amount,
d.outstanding_amount,
)
)

# Check for negative outstanding invoices as well
if flt(d.allocated_amount) < 0 and flt(d.allocated_amount) < flt(latest.outstanding_amount):
frappe.throw(
fail_message.format(
self.party_name,
d.idx,
get_link_to_form(d.reference_doctype, d.reference_name),
d.allocated_amount,
d.outstanding_amount,
)
)


@frappe.whitelist()
def update_check_number(doc: PaymentEntry, method: str | None = None) -> None:
Expand Down