Fix balance corruption when IPN completes a partially paid contribution - #302
Open
magnolia61 wants to merge 1 commit into
Open
Fix balance corruption when IPN completes a partially paid contribution#302magnolia61 wants to merge 1 commit into
magnolia61 wants to merge 1 commit into
Conversation
…al payment
completeTransaction() assumes the IPN represents the full, first payment
against a contribution: it always marks it Completed and effectively treats
the full original contribution amount as paid. That's wrong when the
contribution already carries an outstanding balance smaller than the total —
e.g. an earlier partial payment was recorded (status Partially paid), or the
contribution was created pay-later (Pending) and is now being settled.
In both cases the transaction ends up recorded for the full original amount
rather than the amount the processor actually reports for this transaction,
corrupting the payment ledger even though the contribution status itself
ends up correct.
This is confirmed by civicrm-core dev/financial#174 ("Partial payment
records amount of full contribution disrupting balance"), reported in 2021
and still open: contribution 155, initial payment 55, remaining 100 paid via
the user-dashboard Pay Now button -> contribution correctly shows Completed,
but the payment transaction is recorded as the full 155 instead of 100.
Fix: when the contribution is Partially paid or Pending at IPN time, route
through Payment::create with the amount the processor actually reports
(capped defensively at the outstanding balance) instead of
completetransaction. Payment::create records only that amount and lets core
flip the contribution to Completed once the balance reaches zero. The
ordinary case (fresh, first, full payment) is unaffected -- it still goes
through completetransaction exactly as before.
Also adds CRM_Utils_System::civiExit() after $response->redirect() in
doPayment(), so execution doesn't continue past a redirect that has already
sent output.
Contributor
Author
|
Opened #303 with the reproduction scenario for anyone who wants context outside the diff. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the IPN handler's
handlePaymentNotification()receives a successful response for a contribution that isn't yetCompleted, it unconditionally callscompletetransaction, which assumes this IPN represents the first and only payment against the contribution. That's correct for the common case (fresh registration, paid in full, first attempt). It's wrong whenever the contribution already carries an outstanding balance smaller than its total — e.g.:Partially paid), and this IPN is for the remaining balance;Pending) and this IPN settles it later.In both cases
completetransactionrecords the transaction as being for the full original contribution amount, not the amount the processor actually reports for this transaction. The contribution ends upCompleted, but the payment ledger shows more money received than was actually charged — the balance is corrupted.This is a confirmed, still-open core issue: dev/financial#174 — "Partial payment records amount of full contribution disrupting balance", reported 2021, reproduced with exactly this scenario (contribution €155, initial payment €55, remaining €100 paid via the user-dashboard Pay Now button → contribution correctly shows
Completed, but the payment transaction is recorded as the full €155 instead of the €100 actually paid). Checked currentmaster: the blindcompletetransactioncall is still present, unchanged, 5+ years later.This affects any site where a contribution can receive more than one payment through this extension — event registrations with instalments, membership balance top-ups, pledges, or any pay-later flow settled online later.
Fix
Before completing the transaction, check the contribution's current status. If it is
Partially paidorPendingwith an outstanding balance, route throughPayment::createwith the amount the processor actually reports ($response->getAmount()), capped defensively at the outstanding balance.Payment::createrecords only that amount, adjusts the balance correctly, and lets core flip the contribution toCompletedonce the balance reaches zero.For the ordinary case (no prior payment), behaviour is unchanged —
completetransactionstill runs exactly as before. Risk of regression for the vast majority of installs (single, full, first-time payments) should be minimal.Also bundles a small, unrelated defensive fix:
CRM_Utils_System::civiExit()after$response->redirect()indoPayment(), so execution doesn't continue past a redirect that has already sent output. Scoped only to thedoPayment()redirect path; the separatedoPreApproval()redirect flow is untouched.Test plan
Verified against a live CiviCRM 6.15 instance via
Payment::createdirectly (the same call this PR routes the IPN handler to):Partially paid, balance reduced by exactly the paid amount (not the full total).Completed, balance 0, no over-recording at any point.These match the reproduction case described in dev/financial#174.
Related reports
doPayment()/redirect area but a different concern (trxn-reference persistence for transparent-redirect processors); no overlap.🤖 Generated with Claude Code