From 9d28d6831e83d86d300c880b993499957335a357 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Tue, 22 Nov 2022 17:34:46 +1300 Subject: [PATCH] dev/core#4000 Fix (old) regression - ensure only template contribution updates update recur Copy edits on message Update CRM/Upgrade/Incremental/php/FiveSixtyOne.php Co-authored-by: Matthew Wire --- CRM/Contribute/BAO/Contribution.php | 6 +- CRM/Contribute/BAO/ContributionRecur.php | 19 ++++--- CRM/Upgrade/Incremental/php/FiveSixtyOne.php | 17 ++++++ .../Contribute/BAO/ContributionRecurTest.php | 56 +++++++++++++++---- 4 files changed, 79 insertions(+), 19 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 87784ecc73..b1971d2add 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -17,6 +17,7 @@ use Civi\Api4\ContributionRecur; use Civi\Api4\LineItem; use Civi\Api4\ContributionSoft; use Civi\Api4\PaymentProcessor; +use Civi\Core\Event\PostEvent; /** * @@ -604,9 +605,12 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution im /** * Event fired after modifying a contribution. + * * @param \Civi\Core\Event\PostEvent $event + * + * @throws \CRM_Core_Exception */ - public static function self_hook_civicrm_post(\Civi\Core\Event\PostEvent $event) { + public static function self_hook_civicrm_post(PostEvent $event): void { if ($event->action === 'edit') { CRM_Contribute_BAO_ContributionRecur::updateOnTemplateUpdated($event->object); } diff --git a/CRM/Contribute/BAO/ContributionRecur.php b/CRM/Contribute/BAO/ContributionRecur.php index 732ce892f5..3f7731ef28 100644 --- a/CRM/Contribute/BAO/ContributionRecur.php +++ b/CRM/Contribute/BAO/ContributionRecur.php @@ -996,21 +996,24 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) * @param \CRM_Contribute_DAO_Contribution $contribution * * @throws \CRM_Core_Exception - * @throws \Civi\API\Exception\UnauthorizedException */ - public static function updateOnTemplateUpdated(CRM_Contribute_DAO_Contribution $contribution) { - if (empty($contribution->contribution_recur_id)) { + public static function updateOnTemplateUpdated(CRM_Contribute_DAO_Contribution $contribution): void { + if ($contribution->is_template === '0' || empty($contribution->contribution_recur_id)) { return; } - $contributionRecur = ContributionRecur::get(FALSE) - ->addWhere('id', '=', $contribution->contribution_recur_id) - ->execute() - ->first(); - if ($contribution->total_amount === NULL || $contribution->currency === NULL) { + if ($contribution->total_amount === NULL || $contribution->currency === NULL || $contribution->is_template === NULL) { // The contribution has not been fully loaded, so fetch a full copy now. $contribution->find(TRUE); } + if (!$contribution->is_template) { + return; + } + + $contributionRecur = ContributionRecur::get(FALSE) + ->addWhere('id', '=', $contribution->contribution_recur_id) + ->execute() + ->first(); if ($contribution->currency !== $contributionRecur['currency'] || !CRM_Utils_Money::equals($contributionRecur['amount'], $contribution->total_amount, $contribution->currency)) { ContributionRecur::update(FALSE) diff --git a/CRM/Upgrade/Incremental/php/FiveSixtyOne.php b/CRM/Upgrade/Incremental/php/FiveSixtyOne.php index 00576aeb04..cd79cbd7bc 100644 --- a/CRM/Upgrade/Incremental/php/FiveSixtyOne.php +++ b/CRM/Upgrade/Incremental/php/FiveSixtyOne.php @@ -21,6 +21,23 @@ */ class CRM_Upgrade_Incremental_php_FiveSixtyOne extends CRM_Upgrade_Incremental_Base { + public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NULL): void { + if ($rev === '5.61.alpha1' && CRM_Core_DAO::singleValueQuery('SELECT id FROM civicrm_contribution_recur LIMIT 1')) { + $documentationUrl = 'https://docs.civicrm.org/dev/en/latest/financial/recurring-contributions/'; + $documentationAnchor = 'target="_blank" href="' . htmlentities($documentationUrl) . '"'; + $extensionUrl = 'https://docs.civicrm.org/dev/en/latest/financial/recurring-contributions/'; + $extensionAnchor = 'target="_blank" href="' . htmlentities($extensionUrl) . '"'; + + $preUpgradeMessage .= '

' . + ts('This release contains a change to the behaviour of recurring contributions under some edge-case circumstances.') + . ' ' . ts('Since 5.49 the amount and currency on the recurring contribution record changed when the amount of any contribution against it was changed, indicating a change in future intent.') + . ' ' . ts('It is generally not possible to edit the amount for contributions linked to recurring contributions so for most sites this would never occur anyway.') + . ' ' . ts('If you still want this behaviour you should install the Recur future amounts extension', [1 => $extensionAnchor]) + . ' ' . ts('Please read about recurring contribution templates for more information', [1 => $documentationAnchor]) + . '

'; + } + } + /** * Upgrade step; adds tasks including 'runSql'. * diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php index a4d9ba52ba..1206088082 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php @@ -117,23 +117,40 @@ class CRM_Contribute_BAO_ContributionRecurTest extends CiviUnitTestCase { } /** - * Test we don't change unintended fields on API edit + * Test we don't change unintended fields on the recurring contribution. + * + * This tests two scenarios + * - editing a contribution_recur and changing unrelated fields should leave the + * currency unchanged + * - Adding (or editing) contributions on the recurring should only alter + * it if the contribution is a template contribution. * * @throws \CRM_Core_Exception */ public function testUpdateRecur(): void { $createParams = $this->_params; $createParams['currency'] = 'XAU'; - $contributionRecur = $this->callAPISuccess('contribution_recur', 'create', $createParams); - $editParams = [ - 'id' => $contributionRecur['id'], + $contributionRecurID = $this->callAPISuccess('ContributionRecur', 'create', $createParams)['id']; + $contributionID = Contribution::create()->setValues([ + 'contribution_recur_id' => $contributionRecurID, + 'total_amount' => 5, + 'currency' => 'USD', + 'contact_id' => $this->_params['contact_id'], + 'financial_type_id:name' => 'Donation', + ])->execute()->first()['id']; + $this->assertContributionRecurValues($contributionRecurID, 3, 'XAU', 'a non template contribution should not change the recurring amount details'); + + Contribution::update()->setValues(['receive_date' => 'yesterday'])->addWhere('id', '=', $contributionID)->execute(); + $this->assertContributionRecurValues($contributionRecurID, 3, 'XAU', 'a non template contribution should not change the recurring amount details'); + + $contributionRecurID = $this->callAPISuccess('ContributionRecur', 'create', [ + 'id' => $contributionRecurID, 'end_date' => '+ 4 weeks', - ]; - $contributionRecur = $this->callAPISuccess('contribution_recur', 'create', $editParams); - $dao = new CRM_Contribute_BAO_ContributionRecur(); - $dao->id = $contributionRecur['id']; - $dao->find(TRUE); - $this->assertEquals('XAU', $dao->currency, 'Edit clobbered recur currency'); + ])['id']; + $this->assertContributionRecurValues($contributionRecurID, 3, 'XAU', 'an unrelated contribution recur update should not change the amount details'); + + Contribution::update()->setValues(['is_template' => TRUE])->addWhere('id', '=', $contributionID)->execute(); + $this->assertContributionRecurValues($contributionRecurID, 5, 'USD', 'a change to a template contribution should change the recurring amount details'); } /** @@ -739,4 +756,23 @@ class CRM_Contribute_BAO_ContributionRecurTest extends CiviUnitTestCase { $this->assertEquals('0', $recurring3Get['is_email_receipt']); } + /** + * Assert the contribution recur values match. + * + * @param int $contributionRecurID + * @param int $amount + * @param string $currency + * @param string $message + * + * @throws \CRM_Core_Exception + */ + protected function assertContributionRecurValues(int $contributionRecurID, int $amount, string $currency, string $message = ''): void { + $contributionRecur = ContributionRecur::get()->setSelect([ + 'amount', + 'currency', + ])->addWhere('id', '=', $contributionRecurID)->execute()->first(); + $this->assertEquals($currency, $contributionRecur['currency'], $message); + $this->assertEquals($amount, $contributionRecur['amount'], $message); + } + } -- 2.25.1