From e427daf2b4b196513a32f27469141eef10bb72be Mon Sep 17 00:00:00 2001 From: eileen Date: Sun, 2 Aug 2020 08:45:13 +1200 Subject: [PATCH] [REF] Clean up handling of financial_type_id override Currently we use the primaryContributionID to determine whether the templateContribution will have more than one line item & unset the parameter. It makes much more sense to do evaluations on how many line items the template contribution will have when we have loaded it's line items. The logic is that the financial_type_id can be overriden either by editing the recurring contribution (which you are permitted to do if only one line item exists) or by passing it in via input (which is ignored if there is more than one line item. This change ensures the input parameter reaches repeatTrannsaction - which is the appropriate place to determine whether to apply it. It removes one of 2 places that refers to primaryContributionID. The other is also inappropriate as it is basically used to look up the line items to see the number of terms for the membership. However, we already know that for any repeat transactions as we have already loaded the line items. In completeOrder the line items are that of the contribution being saved. We could pass the saved contribution id into this function as a quick way to eliminate the primaryContributionID or we could just determine num_terms rather than primaryContributionID earlier on --- CRM/Contribute/BAO/Contribution.php | 15 +++++--- CRM/Contribute/BAO/ContributionRecur.php | 9 ++++- tests/phpunit/api/v3/ContributionTest.php | 47 ++++++++++++++++++++--- 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 831f5e86aae6..856746e75f9c 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -2611,9 +2611,9 @@ protected static function repeatTransaction(&$contribution, &$input, $contributi ]) ); $input['line_item'] = $contributionParams['line_item'] = $templateContribution['line_item']; - $contributionParams['status_id'] = 'Pending'; - if (isset($contributionParams['financial_type_id'])) { + + if (isset($contributionParams['financial_type_id']) && count($input['line_item']) === 1) { // Give precedence to passed in type. $contribution->financial_type_id = $contributionParams['financial_type_id']; } @@ -4440,10 +4440,8 @@ public static function completeOrder($input, &$ids, $objects, $transaction = NUL 'contribution_status_id', 'card_type_id', 'pan_truncation', + 'financial_type_id', ]; - if (self::isSingleLineItem($primaryContributionID)) { - $inputContributionWhiteList[] = 'financial_type_id'; - } $participant = $objects['participant'] ?? NULL; $recurContrib = $objects['contributionRecur'] ?? NULL; @@ -4487,6 +4485,12 @@ public static function completeOrder($input, &$ids, $objects, $transaction = NUL $changeDate = CRM_Utils_Array::value('trxn_date', $input, date('YmdHis')); self::repeatTransaction($contribution, $input, $contributionParams, $paymentProcessorId); + // @todo this line only exists because after we save the contribution in repeatTransaction + // with the correct params we resave it shortly after this with ahem params. + // if we stop doing that per + // https://github.com/civicrm/civicrm-core/pull/17972 + // this line can go. Test cover in testRepeatTransactionPassedInFinancialTypeTwoLineItems + // and testRepeatTransactionPassedInFinancialType $contributionParams['financial_type_id'] = $contribution->financial_type_id; $values = []; @@ -4533,6 +4537,7 @@ public static function completeOrder($input, &$ids, $objects, $transaction = NUL // CRM-19309 - if you update the contribution here with financial_type_id it can/will mess with $lineItem // unsetting it here does NOT cause any other contribution test to fail! + // this will no longer be required once we stop setting it above/ stop double saving. unset($contributionParams['financial_type_id']); $contributionResult = civicrm_api3('Contribution', 'create', $contributionParams); diff --git a/CRM/Contribute/BAO/ContributionRecur.php b/CRM/Contribute/BAO/ContributionRecur.php index 72d5d4eaf943..63d7799768d3 100644 --- a/CRM/Contribute/BAO/ContributionRecur.php +++ b/CRM/Contribute/BAO/ContributionRecur.php @@ -442,8 +442,15 @@ public static function getTemplateContribution($id, $overrides = []) { } if ($templateContributions->count()) { $templateContribution = $templateContributions->first(); - $result = array_merge($templateContribution, $overrides); $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($templateContribution['id']); + // We only permit the financial type to be overridden for single line items. + // Otherwise we need to figure out a whole lot of extra complexity. + // It's not UI-possible to alter financial_type_id for recurring contributions + // with more than one line item. + if (count($lineItems) > 1 && isset($overrides['financial_type_id'])) { + unset($overrides['financial_type_id']); + } + $result = array_merge($templateContribution, $overrides); $result['line_item'] = self::reformatLineItemsForRepeatContribution($result['total_amount'], $result['financial_type_id'], $lineItems, (array) $templateContribution); return $result; } diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index 70e9db324823..68205a3c2e48 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -2684,7 +2684,10 @@ public function testRepeatTransactionAlteredAmount() { } /** - * CRM-17718 test appropriate action if financial type has changed for single line items. + * Test financial_type_id override behaviour with a single line item. + * + * CRM-17718 a passed in financial_type_id is allowed to override the original contribution where there + * is only one line item. */ public function testRepeatTransactionPassedInFinancialType() { $originalContribution = $this->setUpRecurringContribution(); @@ -2711,7 +2714,7 @@ public function testRepeatTransactionPassedInFinancialType() { ], ]; - $this->callAPISuccessGetSingle('contribution', [ + $this->callAPISuccessGetSingle('Contribution', [ 'total_amount' => 100, 'financial_type_id' => 2, ]); @@ -2734,6 +2737,37 @@ public function testRepeatTransactionPassedInFinancialType() { $this->assertEquals($expectedLineItem, $lineItem2['values'][0]); } + /** + * Test financial_type_id override behaviour with a single line item. + * + * CRM-17718 a passed in financial_type_id is not allowed to override the original contribution where there + * is more than one line item. + */ + public function testRepeatTransactionPassedInFinancialTypeTwoLineItems() { + $this->_params = $this->getParticipantOrderParams(); + $originalContribution = $this->setUpRecurringContribution(); + + $this->callAPISuccess('Contribution', 'repeattransaction', [ + 'original_contribution_id' => $originalContribution['id'], + 'contribution_status_id' => 'Completed', + 'trxn_id' => 'repeat', + 'financial_type_id' => 2, + ]); + + // Retrieve the new contribution and note the financial type passed in has been ignored. + $contribution = $this->callAPISuccessGetSingle('Contribution', [ + 'trxn_id' => 'repeat', + ]); + $this->assertEquals(4, $contribution['financial_type_id']); + + $lineItems = $this->callAPISuccess('line_item', 'get', [ + 'entity_id' => $contribution['id'], + ])['values']; + foreach ($lineItems as $lineItem) { + $this->assertNotEquals(2, $lineItem['financial_type_id']); + } + } + /** * CRM-17718 test appropriate action if financial type has changed for single line items. */ @@ -4064,6 +4098,7 @@ public function _deletedAddedPaymentInstrument() { * Parameters to merge into the recur only. * * @return array|int + * @throws \CRM_Core_Exception */ protected function setUpRecurringContribution($generalParams = [], $recurParams = []) { $contributionRecur = $this->callAPISuccess('contribution_recur', 'create', array_merge([ @@ -4077,12 +4112,14 @@ protected function setUpRecurringContribution($generalParams = [], $recurParams 'frequency_unit' => 'month', 'payment_processor_id' => $this->paymentProcessorID, ], $generalParams, $recurParams)); - $originalContribution = $this->callAPISuccess('contribution', 'create', array_merge( + $contributionParams = array_merge( $this->_params, [ 'contribution_recur_id' => $contributionRecur['id'], - ], $generalParams) - ); + 'contribution_status_id' => 'Pending', + ], $generalParams); + $contributionParams['api.Payment.create'] = ['total_amount' => $contributionParams['total_amount']]; + $originalContribution = $this->callAPISuccess('Order', 'create', $contributionParams); return $originalContribution; }