From 65e978281bfd3fce49e5a04f440c8992cd516852 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Tue, 25 May 2021 10:59:56 +1200 Subject: [PATCH] [REF] Paypal ipn - cleanup references to completion This extracts a function to check if the contribution is completed. I also rationalised the validation - it was using a combo of recur and first to validate but on thinking it through I realised all it was saying was 'if we are finalising a pending contribution the amount must match' I think that's fine even for recur with a change in amount - that seems to me to be something that happens down the track but we still expect the very first one to come in with the value it originally had - if that is NOT true then we probably should just remove the check --- CRM/Core/Payment/PayPalProIPN.php | 34 ++++++++++--------- .../CRM/Core/Payment/PayPalProIPNTest.php | 16 +++++---- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/CRM/Core/Payment/PayPalProIPN.php b/CRM/Core/Payment/PayPalProIPN.php index b21134f8dc..6a4771be7d 100644 --- a/CRM/Core/Payment/PayPalProIPN.php +++ b/CRM/Core/Payment/PayPalProIPN.php @@ -363,14 +363,11 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN { public function single($input, $contribution, $recur = FALSE, $first = FALSE) { // make sure the invoice is valid and matches what we have in the contribution record - if ((!$recur) || ($recur && $first)) { + if (!$this->isContributionCompleted()) { if ($this->getContributionObject()->invoice_id !== $input['invoice']) { throw new CRM_Core_Exception('PayPalProIPN: Invoice values dont match between database and IPN request.'); } - } - - if (!$recur) { - if ($this->getContributionObject()->total_amount != $input['amount']) { + if (!$this->getContributionRecurID() && $this->getContributionObject()->total_amount != $input['amount']) { throw new CRM_Core_Exception('PayPalProIPN: Amount values dont match between database and IPN request.'); } } @@ -380,8 +377,8 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN { Contribution::update(FALSE)->setValues([ 'cancel_date' => 'now', 'contribution_status_id:name' => 'Failed', - ])->addWhere('id', '=', $contribution->id)->execute(); - Civi::log()->debug("Setting contribution status to Failed"); + ])->addWhere('id', '=', $this->getContributionID())->execute(); + Civi::log()->debug('Setting contribution status to Failed'); return; } if ($status === 'Pending') { @@ -396,14 +393,12 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN { Civi::log()->debug("Setting contribution status to Cancelled"); return; } - elseif ($status !== 'Completed') { + if ($status !== 'Completed') { Civi::log()->debug('Returning since contribution status is not handled'); return; } - // check if contribution is already completed, if so we ignore this ipn - $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); - if ($contribution->contribution_status_id == $completedStatusId) { + if ($this->isContributionCompleted()) { Civi::log()->debug('PayPalProIPN: Returning since contribution has already been handled.'); echo 'Success: Contribution has already been handled

'; return; @@ -527,11 +522,7 @@ INNER JOIN civicrm_membership_payment mp ON m.id = mp.membership_id AND mp.contr if ($this->getContributionRecurID()) { $contributionRecur = $this->getContributionRecurObject(); // check if first contribution is completed, else complete first contribution - $first = TRUE; - $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); - if ($contribution->contribution_status_id == $completedStatusId) { - $first = FALSE; - } + $first = !$this->isContributionCompleted(); $this->recur($input, $contributionRecur, $contribution, $first); return; } @@ -731,4 +722,15 @@ INNER JOIN civicrm_membership_payment mp ON m.id = mp.membership_id AND mp.contr return $this->getValue('c', TRUE); } + /** + * Is the original contribution completed. + * + * @return bool + * @throws \CRM_Core_Exception + */ + private function isContributionCompleted(): bool { + $status = CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $this->getContributionObject()->contribution_status_id); + return $status === 'Completed'; + } + } diff --git a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php index 57d9e4d209..ace9f34a04 100644 --- a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php @@ -124,14 +124,18 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { } /** - * CRM-13743 test IPN edge case where the first transaction fails and the second succeeds. + * CRM-13743 test IPN edge case where the first transaction fails and the + * second succeeds. * - * We are checking that the created contribution has the same date as IPN says it should - * Note that only one contribution will be created (no evidence of the failed contribution is left) - * It seems likely that may change in future & this test will start failing (I point this out in the hope it - * will help future debuggers) + * We are checking that the created contribution has the same date as IPN + * says it should Note that only one contribution will be created (no + * evidence of the failed contribution is left) It seems likely that may + * change in future & this test will start failing (I point this out in the + * hope it will help future debuggers) + * + * @throws \CRM_Core_Exception */ - public function testIPNPaymentCRM13743() { + public function testIPNPaymentCRM13743(): void { $this->setupRecurringPaymentProcessorTransaction(); $firstPaymentParams = $this->getPaypalProRecurTransaction(); $firstPaymentParams['txn_type'] = 'recurring_payment_failed'; -- 2.25.1