MAE-1384: Record the payment processor when completing a contribution - #32
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Stripe Checkout payments completed through this extension were not refundable from CiviCRM due to the absence of a recorded payment processor ID. The core problem stemmed from the ContributionCompletionService not passing this crucial ID during transaction completion. The solution involves modifying the completion service to correctly record the payment processor ID, either by explicit provision or by resolving it from associated payment attempts or recurring contributions. Additionally, a robust backfill mechanism is implemented via an upgrade step to repair historical payment records, ensuring that all past and future payments are correctly linked to their processors, thereby enabling proper refund functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to resolve and record the payment processor ID on financial transactions when completing contributions, including a new PaymentProcessorBackfillService and an upgrade step to retroactively backfill missing processor IDs on historical transactions. Feedback on these changes highlights three key areas for improvement: adding an is_array() guard on the APIv4 first() result to prevent PHP warnings when no payment attempt is found, simplifying the backfill SQL query to avoid an expensive subquery and GROUP BY anti-pattern, and lowering the log level from warning to info when a processor cannot be determined to prevent log spam during normal back-office payment flows.
f305904 to
d424194
Compare
Payments completed through this extension were recorded without the payment processor that took them, because ContributionCompletionService did not pass payment_processor_id to Contribution.completetransaction. Core then left the column empty on the financial transaction it created, and Finance Extras, which reads the processor back off that transaction to decide whether a refund is possible, offered no refund action at all. Staff could not refund a Stripe Checkout payment from CiviCRM. Callers can now pass the processor. When they do not, it is resolved from the contribution's payment attempt, and failing that from its recurring contribution, so no caller has to change and every processor using this service is fixed. Upgrade step 1001 repairs the payments already recorded without a processor, taking it from the payment attempt alongside each one. It only touches payment transactions that have no processor, so it is safe to run again.
d424194 to
61dace0
Compare
2 participants
Overview
Staff could not refund a Stripe Checkout payment from CiviCRM — the Refund action was never offered on the contribution, so there was no way to start a refund at all.
Finance Extras decides whether to offer a refund by reading the payment processor off the payment. Payments completed by this extension were recorded without one, so every one of them looked unrefundable. This PR records the processor when the payment is completed, and repairs the payments already recorded without it.
Jira: https://compucorp.atlassian.net/browse/MAE-1384 (P0)
Test write-up: https://compucorp.atlassian.net/wiki/spaces/CN/pages/6574833665
Before
No Refund action on a Completed Stripe Checkout payment — the menu goes straight from "Duplicate As New Pending Contribution" to "Add Credit Note".
After
"Submit Credit Card Refund" is offered on the same payment.
And it opens the refund form against that payment, with the Stripe charge and the amount available to refund.
Payments taken before this fix are repaired by the upgrade step, so they become refundable too — same contribution as the "Before" shot, after running it.
Technical Details
Why the processor went missing
Contribution.completetransactionalready acceptspayment_processor_idand passes it all the way through (completeOrder()sets$contributionParams['payment_processor'], whichrecordFinancialAccounts()writes to the financial transaction, and unsets when it is empty).ContributionCompletionService::completeTransaction()simply never sent it, so core dropped the column.The fix
ContributionCompletionService::complete()takes an optional$paymentProcessorId. When a caller does not pass one it is resolved from:payment_processor_id;Resolution never blocks completion: if it cannot find a processor the contribution still completes, just without one recorded, which is what back office payments do today.
This means no caller had to change — the Stripe extension's five call sites all run behind a payment attempt — and GoCardless gets the same fix for free.
Backfill
upgrade_1001runsCRM_Paymentprocessingcore_Upgrader_PaymentProcessorBackfill, which takes the processor from the payment attempt alongside each payment. It only touches payment transactions (is_payment = 1) that have no processor, so it is safe to run again — a second run reports 0.It sits under
CRM/Paymentprocessingcore/Upgrader/rather than inCivi/.../Service/: it runs once, from one caller, so it is migration code rather than something the extension uses at runtime. Keeping it in its own class under the upgrader leavesupgrade_1001()thin and lets the repair be tested on its own.A note on the payment attempt table
contribution_idis unique oncivicrm_payment_attempt, so a contribution only ever has one attempt. The backfill joins straight to it, and the constant carries a comment saying the join depends on that index — if it is ever dropped, this needs a deterministic way to choose between a contribution's attempts.Core overrides
None.
Comments
Side effect worth a look. Once a processor is passed, core also sets the contribution's
payment_instrument_idfrom that processor when it is empty (CRM_Contribute_BAO_Contribution::completeOrder()). For Stripe that is Credit Card either way, but it now applies to every processor using this service.testContributionTakesItsPaymentInstrumentFromTheProcessorpins the behaviour so a future change in core is noticed rather than silently absorbed.Verified on a real site (compuclient dev, CiviCRM 6.4.1, Finance Extras installed):
isEligibleForRefund()trueupgrade_1001isEligibleForRefund()false → trueTests: 338 pass, 23 of them new (14 on the completion service, 7 on the backfill, 2 on the upgrade step wiring). phpcs clean. PHPStan level 9 adds no new errors.
The diff adds
EntityFinancialTrxntostubs/CiviApi4.stub.php. The entity is real — its class lives in thecivi_contributecore extension — butphpstan.neononly scanscivicrm/Civi,civicrm/CRMandcivicrm/api, so nothing underext/is visible to it. That is the same reasonContribution,ContributionRecurandPaymentTokenare already stubbed there.Companion PR with the end-to-end test that produced the screenshots above: https://github.com/compucorp/uk.co.compucorp.stripe/pull/300
Release timing. This extension releases on its own cycle, so that is what gates when clients actually get the fix — worth agreeing given the ticket is P0.