From fa839a6825d6e6bea4005b5a7d1420260c6ecd31 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 +++- .../Event/Form/Task/ParticipantStatusTest.php | 30 ++++++++++++ tests/phpunit/CiviTest/CiviUnitTestCase.php | 11 ++--- tests/phpunit/api/v3/ContributionTest.php | 49 ++++++++++++++++--- 5 files changed, 90 insertions(+), 24 deletions(-) create mode 100644 tests/phpunit/CRM/Event/Form/Task/ParticipantStatusTest.php diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 4a9bd727df..e16eba1d01 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -2610,10 +2610,11 @@ LEFT JOIN civicrm_contribution contribution ON ( componentPayment.contribution_ ]) ); $input['line_item'] = $contributionParams['line_item'] = $templateContribution['line_item']; - $contributionParams['status_id'] = 'Pending'; - if (isset($contributionParams['financial_type_id'])) { - // Give precedence to passed in type. + + if (isset($contributionParams['financial_type_id']) && count($input['line_item']) === 1) { + // We permit the financial type to be overridden for single line items. + // More comments on this are in getTemplateTransaction. $contribution->financial_type_id = $contributionParams['financial_type_id']; } else { @@ -4439,10 +4440,8 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac '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; @@ -4486,7 +4485,6 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $changeDate = CRM_Utils_Array::value('trxn_date', $input, date('YmdHis')); $contributionResult = self::repeatTransaction($contribution, $input, $contributionParams); - $contributionParams['financial_type_id'] = $contribution->financial_type_id; $values = []; @@ -4530,9 +4528,6 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $contributionParams['id'] = $contribution->id; $contributionParams['is_post_payment_create'] = $isPostPaymentCreate; - // 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! - unset($contributionParams['financial_type_id']); if (!$contributionResult) { $contributionResult = civicrm_api3('Contribution', 'create', $contributionParams); } diff --git a/CRM/Contribute/BAO/ContributionRecur.php b/CRM/Contribute/BAO/ContributionRecur.php index 72d5d4eaf9..63d7799768 100644 --- a/CRM/Contribute/BAO/ContributionRecur.php +++ b/CRM/Contribute/BAO/ContributionRecur.php @@ -442,8 +442,15 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) } 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/CRM/Event/Form/Task/ParticipantStatusTest.php b/tests/phpunit/CRM/Event/Form/Task/ParticipantStatusTest.php new file mode 100644 index 0000000000..ac809b175a --- /dev/null +++ b/tests/phpunit/CRM/Event/Form/Task/ParticipantStatusTest.php @@ -0,0 +1,30 @@ +customGroupCreate(['extends' => 'Participant', 'title' => 'Participant']); + $field = $this->customFieldCreate(['custom_group_id' => $group['id'], 'html_type' => 'CheckBox', 'option_values' => ['two' => 'A couple', 'three' => 'A few', 'four' => 'Too Many']]); + $participantID = $this->participantCreate(); + $participant = $this->callAPISuccessGetSingle('Participant', ['id' => $participantID]); + $this->assertEquals(2, $participant['participant_status_id']); + + $form = $this->getFormObject('CRM_Event_Form_Task_Batch'); + $form->submit(['field' => [$participantID => ['participant_status_id' => 1, 'custom_' . $field['id'] => ['two' => 1, 'four' => 1]]]]); + + $participant = $this->callAPISuccessGetSingle('Participant', ['id' => $participantID]); + $this->assertEquals(1, $participant['participant_status_id']); + } + +} diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index d9d795b84d..34bba898b1 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -2780,13 +2780,10 @@ VALUES protected function swapMessageTemplateForTestTemplate($templateName = 'contribution_online_receipt', $type = 'html') { $testTemplate = file_get_contents(__DIR__ . '/../../templates/message_templates/' . $templateName . '_' . $type . '.tpl'); CRM_Core_DAO::executeQuery( - "UPDATE civicrm_option_group og - LEFT JOIN civicrm_option_value ov ON ov.option_group_id = og.id - LEFT JOIN civicrm_msg_template m ON m.workflow_id = ov.id - SET m.msg_{$type} = %1 - WHERE og.name LIKE 'msg_tpl_workflow_%' - AND ov.name = '{$templateName}' - AND m.is_default = 1", [1 => [$testTemplate, 'String']] + "UPDATE civicrm_msg_template + SET msg_{$type} = %1 + WHERE workflow_name = '{$templateName}' + AND is_default = 1", [1 => [$testTemplate, 'String']] ); } diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index d5c9659909..166c9fde55 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -2714,7 +2714,10 @@ class api_v3_ContributionTest extends CiviUnitTestCase { } /** - * 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(); @@ -2741,7 +2744,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ], ]; - $this->callAPISuccessGetSingle('contribution', [ + $this->callAPISuccessGetSingle('Contribution', [ 'total_amount' => 100, 'financial_type_id' => 2, ]); @@ -2764,6 +2767,37 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $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. */ @@ -3014,7 +3048,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { * Test that $is_recur is assigned to the receipt. */ public function testCompleteTransactionForRecurring() { - + $this->mut = new CiviMailUtils($this, TRUE); $this->swapMessageTemplateForTestTemplate(); $recurring = $this->setUpRecurringContribution(); $contributionPage = $this->createReceiptableContributionPage(['is_recur' => TRUE, 'recur_frequency_unit' => 'month', 'recur_interval' => 1]); @@ -4094,6 +4128,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { * 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([ @@ -4107,12 +4142,14 @@ class api_v3_ContributionTest extends CiviUnitTestCase { '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; } -- 2.25.1