fix: deprecate adhoc payment request#112
Conversation
📝 WalkthroughWalkthroughThis PR deprecates and removes the Bank Payment Request doctype (including Python class, JSON definition, JavaScript handlers, and list view configuration) and the Payment Type doctype entirely. It refactors the Payment Order reference allocation logic to support Payment Terms Template-based splitting and allocation of invoice amounts. Deprecated fields (is_adhoc, tax withholding, payment_type) are reorganized or marked read-only via a new database patch. Custom field definitions are updated, and a deprecation notice is added to the Payment Request form warning users that ad-hoc payment requests are no longer supported. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
venkat102
force-pushed
the
deprecate-adhoc-payment-request
branch
from
2d709ce to
7495694
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
india_banking/india_banking/doctype/bank_payment_request/bank_payment_request.py (1)
127-135:⚠️ Potential issue | 🟡 MinorAvoid blanket
except Exceptionwhen checking bank account workflow state.The code catches all exceptions to handle the missing workflow case, but
frappe.db.get_value()returnsNonewhen a record doesn't exist—it does not raise an exception. This blanket handler masks legitimate errors like database or permission issues and surfaces a misleading message. Instead, explicitly check forNoneand let unexpected exceptions surface for proper debugging.🛠️ Suggested refactor
- try: - status = frappe.db.get_value( - "Bank Account", self.bank_account, "workflow_state" - ) - - if self.mode_of_payment == "Wire Transfer" and status != "Approved": - frappe.throw(_("Cannot proceed with un-approved bank account")) - except Exception: - frappe.throw(_("Workflow Not Found for Bank Account")) + status = frappe.db.get_value( + "Bank Account", self.bank_account, "workflow_state" + ) + if status is None: + frappe.throw(_("Workflow Not Found for Bank Account")) + if self.mode_of_payment == "Wire Transfer" and status != "Approved": + frappe.throw(_("Cannot proceed with un-approved bank account"))
🤖 Fix all issues with AI agents
In `@india_banking/india_banking/doc_events/payment_order.py`:
- Around line 129-212: The code uses Payment Request.net_total for
reference_amount which can over-allocate when the Payment Order Reference was
manually reduced; change the initial reference_amount assignment to prefer the
Payment Order Reference's own amount (e.g., reference.get("amount") or
reference.amount) and fall back to frappe.db.get_value("Payment Request",
reference.payment_request, "net_total") if missing, ensure subsequent math
clamps reference_amount >= 0 and continues to decrement that reference_amount
when allocations are appended via _append_reference so you never allocate more
than the Payment Order Reference's amount; update references inside the
payment-term branch and the else branches to use this new reference_amount
variable.
In
`@india_banking/india_banking/doctype/bank_payment_request/bank_payment_request.js`:
- Around line 7-16: The deprecation banner in refresh(frm) currently points
users to Payment Request; update the message passed to frm.dashboard.add_comment
so it directs users to the current workflow: create a Payment Entry and then use
a Payment Order (pull the Payment Entry into a Payment Order) instead of using
Payment Request. Edit the string in the refresh function (where
frm.disable_form() and frm.dashboard.add_comment are called) to replace the old
guidance with a concise instruction mentioning Payment Entry and Payment Order.
In `@india_banking/install.py`:
- Around line 173-189: The three commented-out references to the removed field
tax_withholding_category break layout and visibility: update the two dicts that
use "insert_after": "payment_term" and "insert_after": "hold_gst_payables" to
point at existing neighboring field names in the current sequence (e.g., use
"currency" or "payment_term" and then the next real field after that) and fix
the taxes_deducted dict's "depends_on" which currently references the removed
field by changing it to a valid condition (for example "depends_on":
"eval:doc.apply_tax_withholding_amount" or remove the depends_on entirely if it
should always show). Locate the dicts by their fieldnames
"tax_withholding_category", "hold_gst_payables", and "taxes_deducted" and update
their "insert_after" and "depends_on" properties accordingly so they reference
existing fields/conditions.
In `@india_banking/overrides/payment_entry.py`:
- Around line 142-145: The list comprehension building order_entry in
payment_entry.py uses a redundant conditional that always selects
pe.payment_entry; change it to prefer payment_entry_reference when present by
replacing the conditional expression with (pe.payment_entry_reference or
pe.payment_entry) so referenced entries are used and payment_entry is the
fallback; update the expression where order_entry is defined to use those
attributes on the payment entry objects (pe.payment_entry_reference and
pe.payment_entry).
- Around line 71-84: The code currently hardcodes "Wire Transfer" for the
"mode_of_payment" field when `reference` is falsy; change it to use the Payment
Entry's actual `mode_of_payment` (`source.mode_of_payment`) instead (or fall
back to "Wire Transfer" only if `source.mode_of_payment` is empty) so the dict
built alongside keys like "reference_doctype", "reference_name", "amount",
"party_type", and "party" preserves the real payment method.
🧹 Nitpick comments (2)
india_banking/india_banking/doctype/bank_payment_request/bank_payment_request.py (1)
399-431: Remove the commented-out helper block.Keeping dead code commented out adds noise; VCS history already preserves it.
🧹 Proposed cleanup
-# def get_existing_payment_request_amount( -# ref_dt, ref_dn, submitted=True, update=None, payment_term=None -# ): -# """ -# Get the existing Bank payment request which are unpaid or partially paid for payment channel other than Phone -# and get the summation of existing paid Bank payment request for Phone payment channel. -# """ - -# docstatus = 1 if submitted else 0 - -# where_conditions = ( -# "AND payment_term = '{0}'".format(payment_term.replace("%", "%%")) -# if payment_term -# else "AND payment_term is null" -# ) - -# existing_payment_request_amount = frappe.db.sql( -# """ -# select sum(net_total) -# from `tabBank Payment Request` -# where -# name != %s -# and reference_doctype = %s -# and reference_name = %s -# and docstatus = %s {0} -# """.format(where_conditions), -# (update or "", ref_dt, ref_dn, docstatus), -# ) -# return ( -# flt(existing_payment_request_amount[0][0]) -# if existing_payment_request_amount -# else 0 -# )india_banking/india_banking/doc_events/payment_order.py (1)
95-116: Cache the settings lookup outside the summary loop.The settings fetch occurs per summary row; caching it once avoids repeated DB calls.
♻️ Proposed refactor
- for row in payment_order_doc.summary: + use_po_date = frappe.get_single( + "India Banking Settings" + ).use_payment_order_date_as_payment_entry_date + for row in payment_order_doc.summary: ... - if frappe.get_single( - "India Banking Settings" - ).use_payment_order_date_as_payment_entry_date: + if use_po_date: pe.posting_date = payment_order_doc.posting_date
india_banking/india_banking/doctype/bank_payment_request/bank_payment_request.js
Outdated
Show resolved
Hide resolved
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
india_banking/install.py (1)
395-408:⚠️ Potential issue | 🟠 MajorInconsistency between patch and install script for
is_adhocfield on Payment Order Reference.The
is_adhocfield for Payment Order Reference is commented out in install.py (lines 395-401, marked as deprecated) and won't be created on fresh installs. However, the patch scriptupdate_deprecated_fields.py(lines 35-40) attempts to mark this same field asread_onlyfor Payment Order Reference. This creates a mismatch:
- Fresh installs: field won't exist
- Existing installs: patch tries to update a field that may not exist
For consistency, either:
- Keep the field definition uncommented with
"hidden": 1, "read_only": 1applied in install.py, or- Remove the Payment Order Reference
is_adhocentry from the patch script since fresh installs won't have this field
🤖 Fix all issues with AI agents
In `@india_banking/india_banking/doc_events/payment_order.py`:
- Around line 187-200: The code computes per, invoice_amount and to_be_pay using
frappe.db.get_value which can return None; wrap the retrieved values with flt()
to guard against TypeError: call flt(frappe.db.get_value("Payment Term",
term_row.get("payment_term"), "invoice_portion"))/100 for per and
flt(frappe.db.get_value(reference.reference_doctype, reference.reference_name,
"grand_total")) for invoice_amount, then compute to_be_pay = per *
invoice_amount so per, invoice_amount and the multiplication are safe; update
the occurrences in payment_order.py around the per/invoice_amount/to_be_pay
logic accordingly.
- Around line 170-212: When splited_invoice_rows exists and is_term_applied is
true, any leftover reference_amount after the term_row loop is never allocated;
after the for term_row in splited_invoice_rows loop completes, check if
reference_amount > 0 and call _append_reference(pe, reference, invoice_amount,
allocated_amount=reference_amount) (or omit payment_term / pass None) then set
reference_amount = 0 so the remaining amount is allocated as a fallback;
reference the loop over splited_invoice_rows, the is_term_applied condition, the
reference_amount variable, and the _append_reference function when making the
change.
In `@india_banking/overrides/payment_entry.py`:
- Line 136: Replace the direct dict access that assigns existing_payment_entries
from filters (the expression using filters["existing_payment_entries"] or [])
with a safer lookup using filters.get for the "existing_payment_entries" key and
a default empty list; update the assignment in payment_entry.py so
existing_payment_entries uses filters.get("existing_payment_entries", []) (or
equivalent) to avoid KeyError when the key is missing.
🧹 Nitpick comments (4)
india_banking/overrides/payment_entry.py (1)
67-67: Unusedreferenceparameter in_get_reference_data.The
referenceparameter is declared but never used within the function body. All values are derived from thesourceclosure variable. Consider removing this unused parameter.Suggested fix
- def _get_reference_data(reference=None): + def _get_reference_data():india_banking/patches/v16_0/update_deprecated_fields.py (1)
43-44: Silent failure when Custom Field doesn't exist.
frappe.db.set_valuewith a filters dict will silently do nothing if no matching Custom Field record exists. This could happen on fresh installations where these deprecated fields were never created. Consider adding existence checks or usingupdate_modified=Falseto avoid updating timestamps on these read-only fields.♻️ Proposed improvement with existence check
for i in fields: - frappe.db.set_value("Custom Field", i["filters"], i["property"]) + field_name = frappe.db.get_value("Custom Field", i["filters"]) + if field_name: + frappe.db.set_value("Custom Field", field_name, i["property"], update_modified=False)india_banking/public/js/payment_request.js (1)
15-22: Consider showing the deprecation warning conditionally.The warning is displayed on every Payment Request, but the message specifically mentions "Adhoc Payment Request feature is deprecated." Users working with standard Payment Requests (created from Purchase Invoices, etc.) might be confused by this warning.
Consider adding a condition to show the warning only for adhoc payment requests (e.g., checking
frm.doc.is_adhoc), or revise the message to clarify which workflow is deprecated.♻️ Suggested conditional display
frm.dashboard.clear_headline(); + if (frm.doc.is_adhoc) { frm.dashboard.add_comment( __( "<b>Warning: </b>The Adhoc Payment Request feature is deprecated. Please create a Payment Entry and pull it into the Payment Order instead." ), "yellow", true ); + }india_banking/india_banking/doc_events/payment_order.py (1)
155-164: Consider caching the document fetch to avoid repeated lookups.The
frappe.get_doc()call for the reference document is executed for each reference in the loop. If multiple references point to the same document, this results in redundant database queries.♻️ Suggested caching approach
# Before the loop, add a cache dict doc_cache = {} # Then in the loop: doc_key = (reference.reference_doctype, reference.reference_name) if doc_key not in doc_cache: doc_cache[doc_key] = frappe.get_doc(reference.reference_doctype, reference.reference_name) # Use cached document exc_rates={reference.reference_name: doc_cache[doc_key]}
2 participants
This PR deprecates the Ad Hoc Payment Request feature. As a workaround, users should create a Payment Entry and then pull that entry into the Payment Order instead.