feat: create SEPA Payment Order from Expense Claim#352
feat: create SEPA Payment Order from Expense Claim#352MarcCon wants to merge 11 commits intoversion-15-hotfixfrom
Conversation
@MarcCon what do you think, should we keep this part analogous to Purchase Invoice as well? This way we could avoid the creation of duplicate payment orders and filter expense claims by payment status. |
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
barredterra
left a comment
There was a problem hiding this comment.
Functionality works fine! Two small remaining issues.
Greptile SummaryThis PR adds SEPA Payment Order creation from Expense Claims, following the same general architecture as the existing Purchase Invoice flow. A new Confidence Score: 4/5Safe to merge with low risk, but the missing outstanding-amount guard can produce unprocessable 0-amount SEPA Payment Orders in edge cases. One P1 finding (zero-amount payment guard) describes a present defect on the changed path. The remaining findings are P2. Score is 4 rather than 5 because of the P1 finding. banking/custom/expense_claim.py — the row-condition logic and the missing duplicate-order guard need review before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant JS as Expense Claim JS
participant PY as expense_claim.py
participant Mapper as get_mapped_doc
participant Order as SEPA Payment Order
participant Hooks as doc_events hooks
User->>JS: Click Create SEPA Payment Order
JS->>PY: make_sepa_payment_order(source_name)
PY->>Mapper: get_mapped_doc with validation rules
Note over Mapper: docstatus=1, approval_status=Approved, status!=Paid
Mapper->>Mapper: condition row.idx==1 picks first Expense Claim Detail
Mapper->>PY: process_payment callback
PY->>PY: _get_recipients_bank_account(claim)
PY->>PY: get_sepa_payment_amount = grand_total - total_amount_reimbursed
Mapper->>PY: set_missing_values fills company bank account
PY-->>JS: Unsaved SEPA Payment Order doc
JS->>User: Open SEPA Payment Order form
Order->>Hooks: after_insert notify DRAFT
Hooks->>PY: sepa_payment_order_status_changed doc DRAFT
PY->>PY: doc.sepa_payment_order_status = Draft
Order->>Hooks: on_submit notify APPROVED
Hooks->>PY: sepa_payment_order_status_changed doc APPROVED
Order->>Hooks: on_update_after_submit notify TRANSMITTED
Hooks->>PY: sepa_payment_order_status_changed doc TRANSMITTED
Order->>Hooks: on_cancel or on_trash notify CANCELLED
Hooks->>PY: sepa_payment_order_status_changed doc empty string
Note over PY: Status cleared, Create button reappears
Reviews (1): Last reviewed commit: "fix(Expense Claim): get iban from employ..." | Re-trigger Greptile |
| "condition": lambda row: row.idx == 1, | ||
| "postprocess": process_payment, | ||
| }, |
There was a problem hiding this comment.
Missing outstanding-amount guard allows 0-amount payment
The condition for Expense Claim Detail is lambda row: row.idx == 1, which always selects the first row regardless of the outstanding amount. In contrast, the analogous Purchase Invoice condition is lambda payment: round(payment.outstanding, 2) > 0, which prevents row selection when nothing is owed.
If an Expense Claim reaches total_amount_reimbursed == grand_total but ERPNext hasn't yet set status = "Paid" (e.g., partial reimbursement edge cases, or a timing gap between payment and status update), the server-side get_mapped_doc validation passes, a SEPA Payment row is created, and get_sepa_payment_amount returns max(0, 0) = 0. The resulting SEPA Payment Order will fail at XML generation time because the SEPA spec does not allow zero-amount transfers, leaving the user with an unprocessable order.
Add an outstanding-amount guard to the condition so no row — and therefore no payment — is created when there is nothing to pay:
"condition": lambda row: row.idx == 1
and flt(row.parent_doc.grand_total) - flt(row.parent_doc.total_amount_reimbursed) > 0,Or, equivalently, add a server-side check in set_missing_values / process_payment that throws a user-friendly error when the computed amount is 0, mirroring the frappe.throw already present for the missing-IBAN case.
| return get_mapped_doc( | ||
| "Expense Claim", | ||
| source_name, | ||
| { | ||
| "Expense Claim": { | ||
| "doctype": "SEPA Payment Order", | ||
| "validation": { | ||
| "docstatus": ("=", 1), | ||
| "approval_status": ("=", "Approved"), | ||
| "status": ("!=", "Paid"), | ||
| }, | ||
| }, | ||
| "Expense Claim Detail": { | ||
| "doctype": "SEPA Payment", | ||
| "field_map": { | ||
| "name": "reference_row_name", | ||
| "parent": "reference_name", | ||
| "parenttype": "reference_doctype", | ||
| }, | ||
| "condition": lambda row: row.idx == 1, | ||
| "postprocess": process_payment, | ||
| }, | ||
| }, | ||
| target_doc, | ||
| postprocess=set_missing_values, | ||
| ) |
There was a problem hiding this comment.
No server-side guard against duplicate SEPA Payment Orders
The get_mapped_doc validation checks docstatus = 1, approval_status = Approved, and status != Paid, but does not check sepa_payment_order_status. A direct POST call to banking.custom.expense_claim.make_sepa_payment_order (bypassing the client-side !frm.doc.sepa_payment_order_status guard in expense_claim.js) can create a second SEPA Payment Order for the same claim, overwriting the status field and potentially triggering a double payment.
Consider adding a server-side check at the start of make_sepa_payment_order:
if frappe.db.get_value("Expense Claim", source_name, "sepa_payment_order_status"):
frappe.throw(_("A SEPA Payment Order already exists for Expense Claim {0}.").format(source_name))| const old_onload = frappe.listview_settings["Expense Claim"].onload; | ||
| const old_add_fields = | ||
| frappe.listview_settings["Expense Claim"].add_fields || []; | ||
| frappe.listview_settings["Expense Claim"].add_fields = [ |
There was a problem hiding this comment.
Top-level access to
frappe.listview_settings["Expense Claim"] may throw if HRMS is absent
Lines 1–4 access frappe.listview_settings["Expense Claim"] at module evaluation time. If this key does not exist (e.g., HRMS is not installed or its list-view script hasn't run yet), line 1 throws a TypeError and the entire file fails to load, silently disabling the bulk action for all users.
A defensive guard is worth adding:
frappe.listview_settings["Expense Claim"] =
frappe.listview_settings["Expense Claim"] || {};
const old_onload = frappe.listview_settings["Expense Claim"].onload;
Create SEPA Payment Orders from Expense Claims.
Follows the same general logic as for Purchase Invoices.
Key differences compared to Purchase Invoice:
Closes #349