fix: make partial payments transactional and use core currency setup

[my-dev-jour]

Summary

• replace manual Payment Entry construction with ERPNext's get_payment_entry(...)
• remove helper-level commits from partial payment creation
• rollback the full payment batch to a savepoint if any payment in the request fails

Why

add_payment_to_partial_invoice() currently claims rollback-safe behavior, but create_payment_entry() commits each submitted Payment Entry immediately. If a later payment in the same batch fails, the API returns an error after already creating accounting side effects and then tries to clean up with compensating cancels.

The current code also hardcodes both paid_from_account_currency and paid_to_account_currency from invoice.currency, which diverges from ERPNext's standard Payment Entry setup and is wrong for multi-currency sites.

What changed

• use ERPNext core get_payment_entry(...) so account currencies and references come from the standard path
• keep POSNext-specific fields like mode_of_payment, reference_no, remarks, and explicit payment account selection
• wrap the batch in a database savepoint and roll back to that savepoint on failure
• remove the misleading compensating-cancel flow and helper-level frappe.db.commit()

Validation

• reproduced the non-atomic behavior on ERPNext 16.11.0 staging before the fix: a failed batch still created a submitted Payment Entry and then cancelled it
• applied this patch on the same staging bench and re-tested a failed mixed-valid/invalid payment batch
• after the fix, no Payment Entry Reference rows remained for the invoice after the failed request
• local verification was limited to py_compile in this workspace

Notes

• the rollback behavior in #197 was runtime-validated on staging
• the multi-currency part of #199 is a correctness fix aligned with ERPNext core, but staging is single-currency so that part was not runtime-proven there
• closes [Bug]: Partial payment API is not atomic and can leave Payment Entries after failure #197
• closes [Bug]: Partial payment Payment Entry creation uses wrong account currencies in multi-currency setups #199

[engahmed1190]

Code Review — APPROVE ✅

Summary

Replaces fragile manual Payment Entry construction with ERPNext's get_payment_entry() factory and replaces the broken compensating-cancel rollback with database savepoints.

Test Results

No test file included. The logic change is straightforward and was validated on staging per the PR description.

Correctness

• get_payment_entry() replacement: Correct. The old manual construction hardcoded paid_from_account_currency and paid_to_account_currency from invoice.currency, which is wrong for multi-currency sites. get_payment_entry() handles exchange rates, party accounts, and reference allocation correctly via ERPNext core.
• Savepoint pattern: Significant improvement. The old compensating-cancel approach had a serious problem: cancelling a submitted Payment Entry creates cancellation GL entries, so even after "rollback" the ledger contained submit + cancel noise. With frappe.db.savepoint() + rollback(save_point=...), all database changes are undone at the SQL level — no phantom GL entries.
• Removed frappe.db.commit(): Correct and necessary for savepoints to work. The old per-payment commit made transactional rollback impossible.
• Removed explicit pe.validate(): Fine — pe.insert() already calls validate() internally, and pe.submit() also runs before_submit hooks.

Security

No new concerns. ignore_permissions flag is kept with same justification. Length limits on reference_no (140) and remarks (500) are preserved.

Suggestions (non-blocking)

1. Add frappe.db.release_savepoint(batch_savepoint) after the successful loop completion — currently relies on implicit release at transaction commit, which is fine but explicit is cleaner
2. Consider adding a test for the batch-with-failure scenario (e.g., second payment in a batch fails, verify first payment is also rolled back)
3. The batch_savepoint name could theoretically collide if this function is called recursively — extremely unlikely in practice but a unique suffix would be safer

Verdict

Good improvement to both correctness (core currency setup) and reliability (savepoint atomicity). Ready to merge.

[engahmed1190]

Confirming approval — logic is correct, savepoint pattern is solid, no test failures. Safe to merge.