From a64f2b3564bce2a8660396c97062472ed4fbf4f2 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Fri, 3 Mar 2023 18:41:32 +1300 Subject: [PATCH] dev/core#4158 Fix incorrect option_group update in paypal recurring This line was pointed out as being problematic in the issue https://lab.civicrm.org/dev/core/-/issues/4158 and in a test I was able to verify it causes issues if the value for Completed in the contribution status option group is not the same as the contribution recur option group. However, it appears to be more of a theoretical / test creatable regression than a real world one & is not a credible fix for the bug. I think an rc merge without a stable back port is OK --- CRM/Core/Payment/PayPalProIPN.php | 2 +- .../CRM/Core/Payment/PayPalProIPNTest.php | 40 +++++++------ tests/phpunit/CiviTest/CiviUnitTestCase.php | 57 +++++++++++++++---- tests/phpunit/api/v3/ContributionTest.php | 2 +- tests/phpunit/api/v3/JobTest.php | 6 +- 5 files changed, 76 insertions(+), 31 deletions(-) diff --git a/CRM/Core/Payment/PayPalProIPN.php b/CRM/Core/Payment/PayPalProIPN.php index 4dc98b5c31..6f9fdeb59b 100644 --- a/CRM/Core/Payment/PayPalProIPN.php +++ b/CRM/Core/Payment/PayPalProIPN.php @@ -291,7 +291,7 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN { // In future moving to create pending & then complete, but this OK for now. // Also consider accepting 'Failed' like other processors. - $input['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_ContributionRecur', 'contribution_status_id', 'Completed'); + $input['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); $input['invoice_id'] = md5(uniqid(rand(), TRUE)); $input['original_contribution_id'] = $this->getContributionID(); $input['contribution_recur_id'] = $this->getContributionRecurID(); diff --git a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php index fd763db77c..af7ee37ced 100644 --- a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php @@ -9,6 +9,8 @@ +--------------------------------------------------------------------+ */ +use Civi\Api4\Contribution; + /** * Class CRM_Core_Payment_PayPalProIPNTest * @group headless @@ -36,7 +38,7 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { $this->_paymentProcessorID = $this->paymentProcessorCreate(['is_test' => 0]); $this->_contactID = $this->individualCreate(); $contributionPage = $this->callAPISuccess('contribution_page', 'create', [ - 'title' => "Test Contribution Page", + 'title' => 'Test Contribution Page', 'financial_type_id' => $this->_financialTypeID, 'currency' => 'USD', 'payment_processor' => $this->_paymentProcessorID, @@ -53,32 +55,38 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { } /** - * Test IPN response updates contribution_recur & contribution for first & second contribution. + * Test IPN response updates contribution_recur & contribution for first & + * second contribution. * - * The scenario is that a pending contribution exists and the first call will update it to completed. - * The second will create a new contribution. + * The scenario is that a pending contribution exists and the first call will + * update it to completed. The second will create a new contribution. + * + * @throws \CRM_Core_Exception */ - public function testIPNPaymentRecurSuccess() { + public function testIPNPaymentRecurSuccess(): void { + $this->disorganizeOptionValues(); $this->setupRecurringPaymentProcessorTransaction(); global $_GET; $_GET = $this->getPaypalProRecurTransaction(); $paypalIPN = new CRM_Core_Payment_PayPalProIPN($this->getPaypalProRecurTransaction()); $paypalIPN->main(); - $contribution = $this->callAPISuccess('contribution', 'getsingle', ['id' => $this->_contributionID]); - $this->assertEquals(1, $contribution['contribution_status_id']); + $contribution = Contribution::get()->addWhere('id', '=', $this->_contributionID) + ->addSelect('contribution_status_id:name', 'trxn_id', 'source') + ->execute()->first(); + $this->assertEquals('Completed', $contribution['contribution_status_id:name']); $this->assertEquals('8XA571746W2698126', $contribution['trxn_id']); // source gets set by processor - $this->assertTrue(substr($contribution['contribution_source'], 0, 20) == "Online Contribution:"); + $this->assertEquals('Online Contribution:', substr($contribution['source'], 0, 20)); $contributionRecur = $this->callAPISuccess('contribution_recur', 'getsingle', ['id' => $this->_contributionRecurID]); $this->assertEquals(5, $contributionRecur['contribution_status_id']); $paypalIPN = new CRM_Core_Payment_PayPalProIPN($this->getPaypalProRecurSubsequentTransaction()); $paypalIPN->main(); - $contribution = $this->callAPISuccess('contribution', 'get', [ + $contribution = $this->callAPISuccess('Contribution', 'get', [ 'contribution_recur_id' => $this->_contributionRecurID, 'sequential' => 1, ]); $this->assertEquals(2, $contribution['count']); - $this->assertEquals('secondone', $contribution['values'][1]['trxn_id']); + $this->assertEquals('second-one', $contribution['values'][1]['trxn_id']); $this->assertEquals('Debit Card', $contribution['values'][1]['payment_instrument']); } @@ -87,7 +95,7 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { * * @throws \CRM_Core_Exception */ - public function testIPNPaymentMembershipRecurSuccess() { + public function testIPNPaymentMembershipRecurSuccess(): void { $durationUnit = 'year'; $this->setupMembershipRecurringPaymentProcessorTransaction(['duration_unit' => $durationUnit, 'frequency_unit' => $durationUnit]); $this->callAPISuccessGetSingle('membership_payment', []); @@ -98,7 +106,7 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { $this->assertEquals(1, $contribution['contribution_status_id']); $this->assertEquals('8XA571746W2698126', $contribution['trxn_id']); // source gets set by processor - $this->assertTrue(substr($contribution['contribution_source'], 0, 20) == "Online Contribution:"); + $this->assertEquals('Online Contribution:', substr($contribution['contribution_source'], 0, 20)); $contributionRecur = $this->callAPISuccess('contribution_recur', 'getsingle', ['id' => $this->_contributionRecurID]); $this->assertEquals(5, $contributionRecur['contribution_status_id']); $paypalIPN = new CRM_Core_Payment_PaypalProIPN($this->getPaypalProRecurSubsequentTransaction()); @@ -111,7 +119,7 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { 'sequential' => 1, ]); $this->assertEquals(2, $contribution['count']); - $this->assertEquals('secondone', $contribution['values'][1]['trxn_id']); + $this->assertEquals('second-one', $contribution['values'][1]['trxn_id']); $this->callAPISuccessGetCount('line_item', [ 'entity_id' => $this->ids['membership'], 'entity_table' => 'civicrm_membership', @@ -155,7 +163,7 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { 'sequential' => 1, ]); $this->assertEquals(1, $contribution['count']); - $this->assertEquals('secondone', $contribution['values'][0]['trxn_id']); + $this->assertEquals('second-one', $contribution['values'][0]['trxn_id']); $this->assertEquals(strtotime('03:59:05 Jul 14, 2013 PDT'), strtotime($contribution['values'][0]['receive_date'])); } @@ -348,8 +356,8 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase { * * @return array */ - public function getPaypalProRecurSubsequentTransaction() { - return array_merge($this->getPaypalProRecurTransaction(), ['txn_id' => 'secondone']); + public function getPaypalProRecurSubsequentTransaction(): array { + return array_merge($this->getPaypalProRecurTransaction(), ['txn_id' => 'second-one']); } /** diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index b9a4a5ffa6..a6986d28f2 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -285,8 +285,7 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { protected function setUp(): void { CRM_Core_I18n::clearLocale(); parent::setUp(); - $session = CRM_Core_Session::singleton(); - $session->set('userID', NULL); + CRM_Core_Session::singleton()->set('userID'); $this->_apiversion = 3; @@ -311,7 +310,6 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { // disable any left-over test extensions CRM_Core_DAO::executeQuery('DELETE FROM civicrm_extension WHERE full_name LIKE "test.%"'); - // reset all the caches CRM_Utils_System::flushCache(); @@ -457,10 +455,7 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { $this->resetLabels(); error_reporting(E_ALL & ~E_NOTICE); - CRM_Utils_Hook::singleton()->reset(); - if ($this->hookClass) { - $this->hookClass->reset(); - } + $this->resetHooks(); CRM_Core_Session::singleton()->reset(1); if ($this->tx) { @@ -988,11 +983,10 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { 'payment_instrument_id' => 1, 'non_deductible_amount' => 10.00, 'source' => 'SSF', - 'contribution_status_id' => 1, + 'contribution_status_id' => 'Completed', ], $params); - $result = $this->callAPISuccess('contribution', 'create', $params); - return $result['id']; + return $this->callAPISuccess('Contribution', 'create', $params)['id']; } /** @@ -1798,6 +1792,8 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { /** * Clean up financial entities after financial tests (so we remember to get all the tables :-)) + * + * @noinspection PhpUnhandledExceptionInspection */ public function quickCleanUpFinancialEntities(): void { $tablesToTruncate = [ @@ -1852,6 +1848,7 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { catch (CRM_Core_Exception $e) { $this->fail('failed to cleanup financial types ' . $e->getMessage()); } + $this->organizeOptionValues(); CRM_Core_PseudoConstant::flush('taxRates'); System::singleton()->flushProcessors(); // @fixme this parameter is leaking - it should not be defined as a class static @@ -3926,4 +3923,44 @@ WHERE table_schema = DATABASE()"); } } + /** + * Disorganize our option values to ensure we are not relying on luck. + * + * For example our contributions and recurring contributions use different + * option groups for contribution_status_id but by happy co-incidence (ahem) + * both have 1 for completed by default. This upsets that co-incidence for more + * robust testing. Ideally we would do this for all tests & for a range of values + * but there is too much hard-coding to roll that out right now. + * + * @noinspection PhpUnhandledExceptionInspection + */ + protected function disorganizeOptionValues(): void { + OptionValue::update(FALSE)->setValues(['value' => 20]) + ->addWhere('name', '=', 'Completed') + ->addWhere('option_group_id:name', '=', 'contribution_status') + ->execute(); + } + + /** + * This undoes the `disorganizeOptionValues` function. + * + * @noinspection PhpUnhandledExceptionInspection + */ + protected function organizeOptionValues(): void { + OptionValue::update(FALSE)->setValues(['value' => 1]) + ->addWhere('name', '=', 'Completed') + ->addWhere('option_group_id:name', '=', 'contribution_status') + ->execute(); + } + + /** + * Reset any registered hooks. + */ + protected function resetHooks(): void { + CRM_Utils_Hook::singleton()->reset(); + if ($this->hookClass) { + $this->hookClass->reset(); + } + } + } diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index f41051ef5f..ce043a42c8 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -90,7 +90,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { 'fee_amount' => 5.00, 'net_amount' => 95.00, 'source' => 'SSF', - 'contribution_status_id' => 1, + 'contribution_status_id' => 'Completed', ]; $this->_processorParams = [ 'domain_id' => 1, diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php index 237a2bad76..72292b6a6e 100644 --- a/tests/phpunit/api/v3/JobTest.php +++ b/tests/phpunit/api/v3/JobTest.php @@ -57,6 +57,7 @@ class api_v3_JobTest extends CiviUnitTestCase { * Cleanup after test. */ public function tearDown(): void { + $this->resetHooks(); $this->quickCleanUpFinancialEntities(); $this->quickCleanup(['civicrm_contact', 'civicrm_address', 'civicrm_email', 'civicrm_website', 'civicrm_phone', 'civicrm_job', 'civicrm_action_log', 'civicrm_action_schedule', 'civicrm_group', 'civicrm_group_contact'], TRUE); $this->quickCleanup(['civicrm_contact', 'civicrm_address', 'civicrm_email', 'civicrm_website', 'civicrm_phone'], TRUE); @@ -124,6 +125,7 @@ class api_v3_JobTest extends CiviUnitTestCase { * e.g {if {contact.first_name|boolean} * * @dataProvider dataProviderNamesAndGreetings + * @throws \CRM_Core_Exception */ public function testUpdateGreetingBooleanToken($params, $expectedEmailGreeting): void { $this->setEmailGreetingTemplateToConditional(); @@ -287,7 +289,7 @@ class api_v3_JobTest extends CiviUnitTestCase { 'return' => 'id', 'name_a_b' => 'Employee of', ]); - $result = $this->callAPISuccess('relationship', 'create', [ + $result = $this->callAPISuccess('Relationship', 'create', [ 'relationship_type_id' => $relationshipTypeID, 'contact_id_a' => $individualID, 'contact_id_b' => $orgID, @@ -451,8 +453,6 @@ class api_v3_JobTest extends CiviUnitTestCase { * Check that the merge carries across various related entities. * * Note the group combinations & expected results: - * - * @throws \CRM_Core_Exception */ public function testBatchMergeWithAssets(): void { $contactID = $this->individualCreate(); -- 2.25.1