From 9ca8236f72f7ef5db16ff54f5a644cf2ef8462c4 Mon Sep 17 00:00:00 2001 From: Erawat Chamanont Date: Thu, 2 Jul 2026 18:07:57 +0100 Subject: [PATCH] CIVIPLMMSR-655: Include custom-query handler counts in reconcile summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../ReconcilePaymentAttemptBatchEvent.php | 55 ++++++++- .../PaymentAttemptReconcileService.php | 10 ++ .../ReconcilePaymentAttemptBatchEventTest.php | 71 ++++++++++++ .../PaymentAttemptReconcileServiceTest.php | 107 ++++++++++++++++++ 4 files changed, 239 insertions(+), 4 deletions(-) diff --git a/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEvent.php b/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEvent.php index d5148bc..0ed756c 100644 --- a/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEvent.php +++ b/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEvent.php @@ -12,10 +12,12 @@ * their API for the real status of stuck payments and report results. * * Two usage patterns: - * - Stripe: Uses getAttempts() to iterate PaymentAttempt records, calls - * setAttemptResult() for each. - * - GoCardless: Uses getProcessorType()/getThresholdDays()/getRemainingBudget() - * as trigger + config; queries its own data internally. + * - PaymentAttempt-based (e.g. Stripe): iterate getAttempts() and call + * setAttemptResult() for each; the core builds the run summary from those + * results. + * - Custom-query (e.g. GoCardless): use getProcessorType()/getThresholdDays()/ + * getRemainingBudget() as trigger + config, reconcile own data internally, and + * report totals via reportCounts() so they are included in the run summary. */ class ReconcilePaymentAttemptBatchEvent extends GenericHookEvent { @@ -31,6 +33,18 @@ class ReconcilePaymentAttemptBatchEvent extends GenericHookEvent { */ private array $results = []; + /** + * Counts reported directly by custom-query handlers. + * + * @var array{reconciled: int, unchanged: int, errored: int, unhandled: int} + */ + private array $reportedCounts = [ + 'reconciled' => 0, + 'unchanged' => 0, + 'errored' => 0, + 'unhandled' => 0, + ]; + /** * Constructor. * @@ -151,4 +165,37 @@ public function hasAttemptResult(int $attemptId): bool { return array_key_exists($attemptId, $this->results); } + /** + * Report reconciliation counts directly, for handlers that do not use + * PaymentAttempt records (the custom-query pattern, e.g. GoCardless). + * + * Counts are additive, so a handler may call this more than once. Handlers + * using setAttemptResult() must not call this — the core counts their results + * separately, and mixing the two would double-count. + * + * @param int $reconciled + * @param int $unchanged + * @param int $errored + * @param int $unhandled + */ + public function reportCounts(int $reconciled, int $unchanged = 0, int $errored = 0, int $unhandled = 0): void { + if ($reconciled < 0 || $unchanged < 0 || $errored < 0 || $unhandled < 0) { + throw new \InvalidArgumentException('Reconciliation counts must be non-negative'); + } + + $this->reportedCounts['reconciled'] += $reconciled; + $this->reportedCounts['unchanged'] += $unchanged; + $this->reportedCounts['errored'] += $errored; + $this->reportedCounts['unhandled'] += $unhandled; + } + + /** + * Get the counts reported by custom-query handlers. + * + * @return array{reconciled: int, unchanged: int, errored: int, unhandled: int} + */ + public function getReportedCounts(): array { + return $this->reportedCounts; + } + } diff --git a/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileService.php b/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileService.php index 5c3836d..77ec418 100644 --- a/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileService.php +++ b/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileService.php @@ -269,6 +269,16 @@ private function reconcileByProcessor(string $processorType, int $thresholdDays, } } + // Fold in counts reported by custom-query handlers (e.g. GoCardless), which + // reconcile their own data and report totals directly rather than through + // PaymentAttempt records. Handlers on the setAttemptResult() path (e.g. + // Stripe) do not call reportCounts(), so this adds zero for them. + $reported = $event->getReportedCounts(); + $summary['reconciled'] += $reported['reconciled']; + $summary['unchanged'] += $reported['unchanged']; + $summary['errored'] += $reported['errored']; + $summary['unhandled'] += $reported['unhandled']; + return $summary; } diff --git a/tests/phpunit/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEventTest.php b/tests/phpunit/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEventTest.php index 46417cc..d47a9fb 100644 --- a/tests/phpunit/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEventTest.php +++ b/tests/phpunit/Civi/Paymentprocessingcore/Event/ReconcilePaymentAttemptBatchEventTest.php @@ -242,4 +242,75 @@ public function testGetCompletionDataReturnsNullByDefault(): void { $this->assertNull($result->getCompletionData()); } + // ------------------------------------------------------------------------- + // Reported-counts tests (custom-query pattern, e.g. GoCardless) + // ------------------------------------------------------------------------- + + /** + * Tests reported counts default to zero. + */ + public function testReportedCountsDefaultToZero(): void { + $event = new ReconcilePaymentAttemptBatchEvent('GoCardless', [], 7, 50); + + $this->assertEquals( + ['reconciled' => 0, 'unchanged' => 0, 'errored' => 0, 'unhandled' => 0], + $event->getReportedCounts() + ); + } + + /** + * Tests reportCounts records the reported totals. + */ + public function testReportCountsRecordsTotals(): void { + $event = new ReconcilePaymentAttemptBatchEvent('GoCardless', [], 7, 50); + + $event->reportCounts(99, 1, 2, 3); + + $this->assertEquals( + ['reconciled' => 99, 'unchanged' => 1, 'errored' => 2, 'unhandled' => 3], + $event->getReportedCounts() + ); + } + + /** + * Tests reportCounts is additive across multiple calls. + */ + public function testReportCountsIsAdditive(): void { + $event = new ReconcilePaymentAttemptBatchEvent('GoCardless', [], 7, 50); + + $event->reportCounts(10, 1, 0, 0); + $event->reportCounts(5, 0, 2, 1); + + $this->assertEquals( + ['reconciled' => 15, 'unchanged' => 1, 'errored' => 2, 'unhandled' => 1], + $event->getReportedCounts() + ); + } + + /** + * Tests reportCounts defaults the optional buckets to zero. + */ + public function testReportCountsDefaultsOptionalBucketsToZero(): void { + $event = new ReconcilePaymentAttemptBatchEvent('GoCardless', [], 7, 50); + + $event->reportCounts(4); + + $this->assertEquals( + ['reconciled' => 4, 'unchanged' => 0, 'errored' => 0, 'unhandled' => 0], + $event->getReportedCounts() + ); + } + + /** + * Tests reportCounts rejects negative counts. + */ + public function testReportCountsRejectsNegativeValues(): void { + $event = new ReconcilePaymentAttemptBatchEvent('GoCardless', [], 7, 50); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Reconciliation counts must be non-negative'); + + $event->reportCounts(-1); + } + } diff --git a/tests/phpunit/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileServiceTest.php b/tests/phpunit/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileServiceTest.php index cf92fe5..926973a 100644 --- a/tests/phpunit/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileServiceTest.php +++ b/tests/phpunit/Civi/Paymentprocessingcore/Service/PaymentAttemptReconcileServiceTest.php @@ -874,6 +874,113 @@ public function testCancelledResultLeavesPayLaterContributionAlone(): void { } } + // ------------------------------------------------------------------------- + // Reported-counts tests (custom-query pattern, e.g. GoCardless) + // ------------------------------------------------------------------------- + + /** + * Tests that counts reported via reportCounts() appear in the run summary, + * even with zero PaymentAttempt records (the GoCardless pattern). + */ + public function testReportedCountsFromCustomQueryHandlerAppearInSummary(): void { + // No PaymentAttempt records are created — GoCardless does not use them. + $subscriber = function (ReconcilePaymentAttemptBatchEvent $event): void { + if ($event->getProcessorType() === 'GoCardless') { + $event->reportCounts(99, 1, 2, 3); + } + }; + \Civi::dispatcher()->addListener(ReconcilePaymentAttemptBatchEvent::NAME, $subscriber, -10); + + try { + $result = $this->service->reconcileStuckAttempts(['GoCardless' => 7], 100); + + $this->assertEquals(99, $result['reconciled']); + $this->assertEquals(1, $result['unchanged']); + $this->assertEquals(2, $result['errored']); + $this->assertEquals(3, $result['unhandled']); + } + finally { + \Civi::dispatcher()->removeListener(ReconcilePaymentAttemptBatchEvent::NAME, $subscriber); + } + } + + /** + * Regression guard for the PaymentAttempt/setAttemptResult() path (e.g. + * Stripe): a handler that reconciles via setAttemptResult() and never calls + * reportCounts() must produce exactly the same summary as before, and the + * reported-counts channel must stay empty. + */ + public function testSetAttemptResultPathUnaffectedWhenNoCountsReported(): void { + $this->createStuckPaymentAttempt([ + 'processor_type' => 'dummy', + 'days_ago' => 5, + ]); + + $reportedDuringRun = NULL; + $subscriber = function (ReconcilePaymentAttemptBatchEvent $event) use (&$reportedDuringRun): void { + foreach ($event->getAttempts() as $attemptId => $attempt) { + $event->setAttemptResult($attemptId, new ReconcileAttemptResult('completed', 'PI succeeded')); + } + // A PaymentAttempt-based handler never touches the reported-counts channel. + $reportedDuringRun = $event->getReportedCounts(); + }; + \Civi::dispatcher()->addListener(ReconcilePaymentAttemptBatchEvent::NAME, $subscriber, -10); + + try { + $result = $this->service->reconcileStuckAttempts([self::PROCESSOR_TYPE => 3], 100); + + $this->assertEquals(1, $result['reconciled']); + $this->assertEquals(0, $result['unchanged']); + $this->assertEquals(0, $result['errored']); + $this->assertEquals(0, $result['unhandled']); + $this->assertEquals( + ['reconciled' => 0, 'unchanged' => 0, 'errored' => 0, 'unhandled' => 0], + $reportedDuringRun + ); + } + finally { + \Civi::dispatcher()->removeListener(ReconcilePaymentAttemptBatchEvent::NAME, $subscriber); + } + } + + /** + * Tests that a PaymentAttempt-based processor (setAttemptResult) and a + * custom-query processor (reportCounts) in the same run sum correctly, + * without double-counting. + */ + public function testSetAttemptResultAndReportedCountsCombineWithoutDoubleCounting(): void { + // One Dummy attempt reconciled via setAttemptResult(). + $this->createStuckPaymentAttempt([ + 'processor_type' => 'dummy', + 'days_ago' => 5, + ]); + + $subscriber = function (ReconcilePaymentAttemptBatchEvent $event): void { + if ($event->getProcessorType() === self::PROCESSOR_TYPE) { + foreach ($event->getAttempts() as $attemptId => $attempt) { + $event->setAttemptResult($attemptId, new ReconcileAttemptResult('completed', 'PI succeeded')); + } + } + if ($event->getProcessorType() === 'GoCardless') { + $event->reportCounts(50, 0, 0, 0); + } + }; + \Civi::dispatcher()->addListener(ReconcilePaymentAttemptBatchEvent::NAME, $subscriber, -10); + + try { + $result = $this->service->reconcileStuckAttempts([ + self::PROCESSOR_TYPE => 3, + 'GoCardless' => 7, + ], 100); + + // 1 from the Dummy attempt path + 50 reported by the custom-query path. + $this->assertEquals(51, $result['reconciled']); + } + finally { + \Civi::dispatcher()->removeListener(ReconcilePaymentAttemptBatchEvent::NAME, $subscriber); + } + } + // ------------------------------------------------------------------------- // Helper methods // -------------------------------------------------------------------------