From: eileen Date: Sun, 26 May 2019 22:12:28 +0000 (+1200) Subject: Remove reference to 'changePaymentInstrument' from updateFinancialAccounts as never... X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=efb16c63b9c960ce9d419c24e5010f6ffc598df4;p=civicrm-core.git Remove reference to 'changePaymentInstrument' from updateFinancialAccounts as never passed in This mostly removes a bit IF statement that deals with a 'never true' scenario --- diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 99c901e30d..65d78c4919 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -3760,13 +3760,6 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $currentContributionStatus = CRM_Core_PseudoConstant::getLabel('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $params['contribution']->contribution_status_id); $previousContributionStatus = CRM_Contribute_PseudoConstant::contributionStatus($params['prevContribution']->contribution_status_id, 'name'); - if (($previousContributionStatus == 'Pending' - || $previousContributionStatus == 'In Progress') - && $currentContributionStatus == 'Completed' - && $context == 'changePaymentInstrument' - ) { - return; - } if ($context == 'changedStatus') { list($continue, $isARefund) = self::updateFinancialAccountsOnContributionStatusChange($params, $context, $previousContributionStatus, $currentContributionStatus); // @todo - it may be that this is always false & the parent function is just a confusing wrapper for the child fn. @@ -3783,68 +3776,68 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $trxn = CRM_Core_BAO_FinancialTrxn::create($params['trxnParams']); // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['entity_id'] = $trxn->id; - if ($context != 'changePaymentInstrument') { - $itemParams['entity_table'] = 'civicrm_line_item'; - $trxnIds['id'] = $params['entity_id']; - $previousLineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['contribution']->id); - foreach ($params['line_item'] as $fieldId => $fields) { - foreach ($fields as $fieldValueId => $lineItemDetails) { - $prevFinancialItem = CRM_Financial_BAO_FinancialItem::getPreviousFinancialItem($lineItemDetails['id']); - $receiveDate = CRM_Utils_Date::isoToMysql($params['prevContribution']->receive_date); - if ($params['contribution']->receive_date) { - $receiveDate = CRM_Utils_Date::isoToMysql($params['contribution']->receive_date); - } - $financialAccount = self::getFinancialAccountForStatusChangeTrxn($params, CRM_Utils_Array::value('financial_account_id', $prevFinancialItem)); + $itemParams['entity_table'] = 'civicrm_line_item'; + $trxnIds['id'] = $params['entity_id']; + $previousLineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['contribution']->id); + foreach ($params['line_item'] as $fieldId => $fields) { + foreach ($fields as $fieldValueId => $lineItemDetails) { + $prevFinancialItem = CRM_Financial_BAO_FinancialItem::getPreviousFinancialItem($lineItemDetails['id']); + $receiveDate = CRM_Utils_Date::isoToMysql($params['prevContribution']->receive_date); + if ($params['contribution']->receive_date) { + $receiveDate = CRM_Utils_Date::isoToMysql($params['contribution']->receive_date); + } + + $financialAccount = self::getFinancialAccountForStatusChangeTrxn($params, CRM_Utils_Array::value('financial_account_id', $prevFinancialItem)); - $currency = $params['prevContribution']->currency; - // @todo we should stop passing $params by reference - splitting this out would be a step towards that. - if ($params['contribution']->currency) { - $currency = $params['contribution']->currency; + $currency = $params['prevContribution']->currency; + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. + if ($params['contribution']->currency) { + $currency = $params['contribution']->currency; + } + $previousLineItemTotal = CRM_Utils_Array::value('line_total', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0); + $itemParams = [ + 'transaction_date' => $receiveDate, + 'contact_id' => $params['prevContribution']->contact_id, + 'currency' => $currency, + 'amount' => self::getFinancialItemAmountFromParams($inputParams, $context, $lineItemDetails, $isARefund, $previousLineItemTotal), + 'description' => CRM_Utils_Array::value('description', $prevFinancialItem), + 'status_id' => $prevFinancialItem['status_id'], + 'financial_account_id' => $financialAccount, + 'entity_table' => 'civicrm_line_item', + 'entity_id' => $lineItemDetails['id'], + ]; + $financialItem = CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. + $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $itemParams['amount']; + $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id; + + if (($lineItemDetails['tax_amount'] && $lineItemDetails['tax_amount'] !== 'null') || ($context == 'changeFinancialType')) { + $invoiceSettings = Civi::settings()->get('contribution_invoice_settings'); + $taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings); + $taxAmount = (float) $lineItemDetails['tax_amount']; + if ($context == 'changeFinancialType' && $lineItemDetails['tax_amount'] === 'null') { + // reverse the Sale Tax amount if there is no tax rate associated with new Financial Type + $taxAmount = CRM_Utils_Array::value('tax_amount', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0); } - $previousLineItemTotal = CRM_Utils_Array::value('line_total', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0); - $itemParams = [ - 'transaction_date' => $receiveDate, - 'contact_id' => $params['prevContribution']->contact_id, - 'currency' => $currency, - 'amount' => self::getFinancialItemAmountFromParams($inputParams, $context, $lineItemDetails, $isARefund, $previousLineItemTotal), - 'description' => CRM_Utils_Array::value('description', $prevFinancialItem), - 'status_id' => $prevFinancialItem['status_id'], - 'financial_account_id' => $financialAccount, - 'entity_table' => 'civicrm_line_item', - 'entity_id' => $lineItemDetails['id'], - ]; - $financialItem = CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); - // @todo we should stop passing $params by reference - splitting this out would be a step towards that. - $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $itemParams['amount']; - $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id; - - if (($lineItemDetails['tax_amount'] && $lineItemDetails['tax_amount'] !== 'null') || ($context == 'changeFinancialType')) { - $invoiceSettings = Civi::settings()->get('contribution_invoice_settings'); - $taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings); - $taxAmount = (float) $lineItemDetails['tax_amount']; - if ($context == 'changeFinancialType' && $lineItemDetails['tax_amount'] === 'null') { - // reverse the Sale Tax amount if there is no tax rate associated with new Financial Type - $taxAmount = CRM_Utils_Array::value('tax_amount', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0); - } - elseif ($previousLineItemTotal != $lineItemDetails['line_total']) { - $taxAmount -= CRM_Utils_Array::value('tax_amount', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0); - } - if ($taxAmount != 0) { - $itemParams['amount'] = self::getMultiplier($params['contribution']->contribution_status_id, $context) * $taxAmount; - $itemParams['description'] = $taxTerm; - if ($lineItemDetails['financial_type_id']) { - $itemParams['financial_account_id'] = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount( - $lineItemDetails['financial_type_id'], - 'Sales Tax Account is' - ); - } - CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); + elseif ($previousLineItemTotal != $lineItemDetails['line_total']) { + $taxAmount -= CRM_Utils_Array::value('tax_amount', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0); + } + if ($taxAmount != 0) { + $itemParams['amount'] = self::getMultiplier($params['contribution']->contribution_status_id, $context) * $taxAmount; + $itemParams['description'] = $taxTerm; + if ($lineItemDetails['financial_type_id']) { + $itemParams['financial_account_id'] = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount( + $lineItemDetails['financial_type_id'], + 'Sales Tax Account is' + ); } + CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); } } } } + if ($context == 'changeFinancialType') { // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['skipLineItem'] = FALSE; diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php index 03dbdb2369..e99cc71c05 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -1115,7 +1115,8 @@ WHERE eft.entity_id = %1 AND ft.to_financial_account_id <> %2"; 'previous_line_total' => 100, 'diff' => -1, ), - 'context' => 'changePaymentInstrument', + // Most contexts are ignored. Removing refs to change payment instrument so placeholder. + 'context' => 'not null', 'expectedItemAmount' => -100, ), array(