CIVIPLMMSR-655: Include custom-query handler counts in reconcile summary#28
CIVIPLMMSR-655: Include custom-query handler counts in reconcile summary#28erawat wants to merge 1 commit into
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 improves the accuracy of the reconciliation summary on the Scheduled Jobs screen. Previously, processors that perform internal reconciliation without creating 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for custom-query payment processors (such as GoCardless) that reconcile their own data and report totals directly instead of using individual PaymentAttempt records. It adds methods to ReconcilePaymentAttemptBatchEvent to accumulate and retrieve these counts, integrates them into the run summary within PaymentAttemptReconcileService, and provides thorough test coverage. The review feedback recommends adding validation to prevent negative counts and to disallow mixing the PaymentAttempt-based and custom-query patterns on a single event instance, as well as adding corresponding unit tests for these validation checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
478d302 to
9ca8236
Compare
The reconcile run summary (and the API result shown on the Scheduled Jobs screen) was built solely from PaymentAttempt records reported via setAttemptResult(). Processors that reconcile their own data without PaymentAttempt records — GoCardless — had no way to report their totals, so the summary always showed 0 for them even when work was done. Add reportCounts()/getReportedCounts() to ReconcilePaymentAttemptBatchEvent and fold the reported totals into the per-processor summary. The setAttemptResult() path (Stripe) never calls reportCounts(), so its summary is unchanged.
|
Thanks — two suggestions raised: negative-count validation and a guard to disallow mixing the PaymentAttempt ( We've taken the negative-count validation — We've deliberately not added the mixing guard. A hard throw would forbid a legitimate future case: a processor that reconciles most items through PaymentAttempt records and reports a few extras it discovered by its own query. The two paths are additive by design, and the service folds them together without double-counting (see |
Overview
The "Reconcile stuck payment attempts" scheduled job reconciles GoCardless payments correctly, but the count shown on the Scheduled Jobs screen always reads "0 reconciled", making it look as though the job is doing nothing. This makes the real number appear in the summary.
Before
The reconcile run summary (
reconciled/unchanged/errored/unhandled, returned byPaymentAttemptReconcile.Runand shown on the Scheduled Jobs screen) is built only fromPaymentAttemptrecords reported viasetAttemptResult(). GoCardless reconciles its own data and creates noPaymentAttemptrecords, so it had no way to report totals — the summary always showedreconciled=0for it.Observed on LCC (CIVIPLMMSR-655): all 30 recorded runs report
reconciled=0, while the CiviCRM log for the same days shows ~99/day genuinely reconciled.After
Custom-query handlers report their totals through a new channel on the event, and the service folds them into the run summary, so the screen shows the real number.
N/A for screenshots — backend/telemetry only.
Technical Details
ReconcilePaymentAttemptBatchEvent: addreportCounts()/getReportedCounts()— a processor-agnostic channel for handlers that don't usePaymentAttemptrecords.PaymentAttemptReconcileService::reconcileByProcessor(): fold the reported totals into the per-processor summary before returning.No regression for Stripe (or any
setAttemptResult()handler). The two paths are disjoint and additive:setAttemptResult()and never callsreportCounts()→getReportedCounts()returns all-zeros → the fold adds nothing → the Stripe summary is unchanged.PaymentAttemptrecords → the existingforeach ($attempts …)loop contributes nothing → its counts come only from the new channel.Tests:
testSetAttemptResultPathUnaffectedWhenNoCountsReported— Stripe-style run produces the identical summary and leaves the reported channel empty.testReportedCountsFromCustomQueryHandlerAppearInSummary— custom-query run (0 attempts) surfaces its reported counts.testSetAttemptResultAndReportedCountsCombineWithoutDoubleCounting— mixed run sums correctly, no double-count.Core overrides
None — no CiviCRM core files are overridden or patched.
Comments
Requires the companion GoCardless PR (compucorp/io.compuco.gocardless, branch
CIVIPLMMSR-655-reconcile-summary-reporting) for the handler to callreportCounts(). Safe to merge in either order — the handler guards the call withmethod_exists(), so neither PR breaks without the other.