From cf28d075c3942d5c8b5a1af1c00283ab98314105 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 15 Mar 2017 23:08:01 +1300 Subject: [PATCH] CRM-20260 fix data errors creation in financial_line_item when editing contribution, with sales tax enabled --- CRM/Contribute/BAO/Contribution.php | 34 ++--- CRM/Financial/BAO/FinancialItem.php | 28 ++-- .../CRM/Contribute/BAO/ContributionTest.php | 4 +- .../CRM/Contribute/Form/ContributionTest.php | 124 +++++++++++++++--- .../CRM/Financial/BAO/FinancialItemTest.php | 4 +- 5 files changed, 142 insertions(+), 52 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 515570213d..5e7dd3f2ff 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -3540,14 +3540,14 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac 'entity_table' => 'civicrm_financial_item', ); foreach ($params['line_item'] as $fieldId => $fields) { - foreach ($fields as $fieldValueId => $fieldValues) { + foreach ($fields as $fieldValueId => $lineItemDetails) { $fparams = array( 1 => array(CRM_Core_PseudoConstant::getKey('CRM_Financial_BAO_FinancialItem', 'status_id', 'Paid'), 'Integer'), - 2 => array($fieldValues['id'], 'Integer'), + 2 => array($lineItemDetails['id'], 'Integer'), ); CRM_Core_DAO::executeQuery($query, $fparams); $fparams = array( - 1 => array($fieldValues['id'], 'Integer'), + 1 => array($lineItemDetails['id'], 'Integer'), ); $financialItem = CRM_Core_DAO::executeQuery($sql, $fparams); while ($financialItem->fetch()) { @@ -3570,14 +3570,14 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $itemParams['entity_table'] = 'civicrm_line_item'; $trxnIds['id'] = $params['entity_id']; foreach ($params['line_item'] as $fieldId => $fields) { - foreach ($fields as $fieldValueId => $fieldValues) { - $prevFinancialItem = CRM_Financial_BAO_FinancialItem::getPreviousFinancialItem($fieldValues['id']); + 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, $prevFinancialItem); + $financialAccount = self::getFinancialAccountForStatusChangeTrxn($params, $prevFinancialItem['financial_account_id']); $currency = $params['prevContribution']->currency; if ($params['contribution']->currency) { @@ -3594,7 +3594,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac } } else { - $amount = $diff * $fieldValues['line_total']; + $amount = $diff * $lineItemDetails['line_total']; } $itemParams = array( @@ -3602,23 +3602,23 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac 'contact_id' => $params['prevContribution']->contact_id, 'currency' => $currency, 'amount' => $amount, - 'description' => $prevFinancialItem->description, - 'status_id' => $prevFinancialItem->status_id, + 'description' => $prevFinancialItem['description'], + 'status_id' => $prevFinancialItem['status_id'], 'financial_account_id' => $financialAccount, 'entity_table' => 'civicrm_line_item', - 'entity_id' => $fieldValues['id'], + 'entity_id' => $lineItemDetails['id'], ); $financialItem = CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $amount; $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id; - if ($fieldValues['tax_amount']) { + if ($lineItemDetails['tax_amount']) { $invoiceSettings = Civi::settings()->get('contribution_invoice_settings'); $taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings); - $itemParams['amount'] = $diff * $fieldValues['tax_amount']; + $itemParams['amount'] = $diff * $lineItemDetails['tax_amount']; $itemParams['description'] = $taxTerm; - if ($fieldValues['financial_type_id']) { - $itemParams['financial_account_id'] = self::getFinancialAccountId($fieldValues['financial_type_id']); + if ($lineItemDetails['financial_type_id']) { + $itemParams['financial_account_id'] = self::getFinancialAccountId($lineItemDetails['financial_type_id']); } CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); } @@ -5020,11 +5020,11 @@ LIMIT 1;"; * Get the financial account for the item associated with the new transaction. * * @param array $params - * @param CRM_Financial_BAO_FinancialItem $prevFinancialItem + * @param int $default * * @return int */ - public static function getFinancialAccountForStatusChangeTrxn($params, $prevFinancialItem) { + public static function getFinancialAccountForStatusChangeTrxn($params, $default) { if (!empty($params['financial_account_id'])) { return $params['financial_account_id']; @@ -5041,7 +5041,7 @@ LIMIT 1;"; $preferredAccountsRelationships[$contributionStatus] ); } - return $prevFinancialItem->financial_account_id; + return $default; } /** diff --git a/CRM/Financial/BAO/FinancialItem.php b/CRM/Financial/BAO/FinancialItem.php index 3eb0782fba..2425628cc1 100644 --- a/CRM/Financial/BAO/FinancialItem.php +++ b/CRM/Financial/BAO/FinancialItem.php @@ -96,7 +96,7 @@ class CRM_Financial_BAO_FinancialItem extends CRM_Financial_DAO_FinancialItem { 'currency' => $contribution->currency, 'entity_table' => 'civicrm_line_item', 'entity_id' => $lineItem->id, - 'description' => ($lineItem->qty != 1 ? $lineItem->qty . ' of ' : '') . ' ' . $lineItem->label, + 'description' => ($lineItem->qty != 1 ? $lineItem->qty . ' of ' : '') . $lineItem->label, 'status_id' => $itemStatus, ); @@ -286,25 +286,25 @@ WHERE cc.id IN (' . implode(',', $contactIds) . ') AND con.is_test = 0'; } /** - * Get last financial item data. + * Get most relevant previous financial item relating to the line item. * - * @param int $entityId + * This function specifically excludes sales tax. * - * @param string $entityTable + * @param int $entityId * * @return object CRM_Core_DAO */ - public static function getPreviousFinancialItem($entityId, $entityTable = 'civicrm_line_item') { - $queryParams = array( - 1 => array($entityId, 'Integer'), - 2 => array($entityTable, 'String'), + public static function getPreviousFinancialItem($entityId) { + $params = array( + 'entity_id' => $entityId, + 'entity_table' => 'civicrm_line_item', + 'options' => array('limit' => 1, 'sort' => 'id DESC'), ); - $query = 'SELECT id, description, status_id, financial_account_id - FROM civicrm_financial_item - WHERE entity_id = %1 AND entity_table = %2 ORDER BY id DESC LIMIT 1'; - $prevFinancialItem = CRM_Core_DAO::executeQuery($query, $queryParams); - $prevFinancialItem->fetch(); - return $prevFinancialItem; + $salesTaxFinancialAccounts = civicrm_api3('FinancialAccount', 'get', array('is_tax' => 1)); + if ($salesTaxFinancialAccounts['count']) { + $params['financial_account_id'] = array('NOT IN' => array_keys($salesTaxFinancialAccounts['values'])); + } + return civicrm_api3('FinancialItem', 'getsingle', $params); } } diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php index 77acd3d2cd..005a14d8c8 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -1075,14 +1075,14 @@ WHERE eft.entity_id = %1 AND ft.to_financial_account_id <> %2"; $previousLineItem = CRM_Financial_BAO_FinancialItem::getPreviousFinancialItem($contribution['id']); $eftParams = array( 'entity_table' => 'civicrm_financial_item', - 'entity_id' => $previousLineItem->id, + 'entity_id' => $previousLineItem['id'], 'financial_trxn_id' => $financialTrxn['id'], ); CRM_Contribute_BAO_Contribution::createProportionalEntry($entityParams, $eftParams); $trxnTestArray = array_merge($eftParams, array( 'amount' => 50, )); - $entityFinancialTrxn = $this->callAPISuccessGetSingle('EntityFinancialTrxn', $eftParams, $trxnTestArray); + $this->callAPISuccessGetSingle('EntityFinancialTrxn', $eftParams, $trxnTestArray); } /** diff --git a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php index c921470c3a..6f36045db5 100644 --- a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php @@ -859,19 +859,26 @@ Price Field - Price Field 1 1 $ 100.00 $ 100.00 public function testReSubmitSaleTax() { $this->enableTaxAndInvoicing(); $this->relationForFinancialTypeWithFinancialAccount($this->_financialTypeId); - $form = new CRM_Contribute_Form_Contribution(); + list($form, $contribution) = $this->doInitialSubmit(); + $this->assertEquals(110, $contribution['total_amount']); + $this->assertEquals(10, $contribution['tax_amount']); + $this->assertEquals(110, $contribution['net_amount']); + $mut = new CiviMailUtils($this, TRUE); + // Testing here if when we edit something trivial like adding a check_number tax, net, total amount stay the same: $form->testSubmit(array( - 'total_amount' => 100, - 'financial_type_id' => $this->_financialTypeId, - 'receive_date' => '04/21/2015', - 'receive_date_time' => '11:27PM', - 'contact_id' => $this->_individualId, - 'payment_instrument_id' => array_search('Check', $this->paymentInstruments), - 'contribution_status_id' => 1, + 'id' => $contribution['id'], + 'tax_amount' => $contribution['tax_amount'], + 'financial_type_id' => $contribution['financial_type_id'], + 'receive_date' => $contribution['receive_date'], + 'payment_instrument_id' => $contribution['payment_instrument_id'], 'price_set_id' => 0, + 'check_number' => 12345, + 'contribution_status_id' => 1, + 'is_email_receipt' => 1, + 'from_email_address' => 'demo@example.com', ), - CRM_Core_Action::ADD + CRM_Core_Action::UPDATE ); $contribution = $this->callAPISuccessGetSingle('Contribution', array( @@ -883,11 +890,41 @@ Price Field - Price Field 1 1 $ 100.00 $ 100.00 $this->assertEquals(10, $contribution['tax_amount']); $this->assertEquals(110, $contribution['net_amount']); + $strings = array( + 'Total Tax Amount : $ 10.00', + 'Total Amount : $ 110.00', + 'Date Received: April 21st, 2015', + 'Paid By: Check', + 'Check Number: 12345', + ); + + $mut->checkMailLog($strings); + $this->callAPISuccessGetCount('FinancialTrxn', array(), 3); + $items = $this->callAPISuccess('FinancialItem', 'get', array('sequential' => 1)); + $this->assertEquals(2, $items['count']); + $this->assertEquals('Contribution Amount', $items['values'][0]['description']); + $this->assertEquals('Sales Tax', $items['values'][1]['description']); + + $this->assertEquals(100, $items['values'][0]['amount']); + $this->assertEquals(10, $items['values'][1]['amount']); + } + + /** + * Create a contribution & then edit it via backoffice form, checking tax with: default price_set + * + * @throws \Exception + */ + public function testReSubmitSaleTaxAlteredAmount() { + $this->enableTaxAndInvoicing(); + $this->relationForFinancialTypeWithFinancialAccount($this->_financialTypeId); + list($form, $contribution) = $this->doInitialSubmit(); + $mut = new CiviMailUtils($this, TRUE); // Testing here if when we edit something trivial like adding a check_number tax, net, total amount stay the same: $form->testSubmit(array( 'id' => $contribution['id'], - 'tax_amount' => $contribution['tax_amount'], + 'total_amount' => 200, + 'tax_amount' => 20, 'financial_type_id' => $contribution['financial_type_id'], 'receive_date' => $contribution['receive_date'], 'payment_instrument_id' => $contribution['payment_instrument_id'], @@ -905,21 +942,74 @@ Price Field - Price Field 1 1 $ 100.00 $ 100.00 'return' => array('tax_amount', 'total_amount', 'net_amount', 'financial_type_id', 'receive_date', 'payment_instrument_id'), ) ); - $this->assertEquals(110, $contribution['total_amount']); - $this->assertEquals(10, $contribution['tax_amount']); - $this->assertEquals(110, $contribution['net_amount']); + $this->assertEquals(220, $contribution['total_amount']); + $this->assertEquals(20, $contribution['tax_amount']); + $this->assertEquals(220, $contribution['net_amount']); $strings = array( - 'Total Tax Amount : $ 10.00', - 'Total Amount : $ 110.00', + 'Total Tax Amount : $ 20.00', + 'Total Amount : $ 220.00', 'Date Received: April 21st, 2015', 'Paid By: Check', 'Check Number: 12345', ); $mut->checkMailLog($strings); - $this->callAPISuccessGetCount('FinancialTrxn', array(), 3); - $this->callAPISuccessGetCount('FinancialItem', array(), 2); + $this->callAPISuccessGetCount('FinancialTrxn', array(), 4); + $items = $this->callAPISuccess('FinancialItem', 'get', array('sequential' => 1)); + $this->assertEquals(4, $items['count']); + $this->assertEquals('Contribution Amount', $items['values'][0]['description']); + $this->assertEquals('Sales Tax', $items['values'][1]['description']); + $this->assertEquals('Contribution Amount', $items['values'][0]['description']); + $this->assertEquals('Sales Tax', $items['values'][1]['description']); + + $this->assertEquals(100, $items['values'][0]['amount']); + $this->assertEquals(10, $items['values'][1]['amount']); + // @todo what should the amount BE? I believe this is incorrect elsewhere too. + // currently it is $120 - ie the first one not incremented. This is consistent + // with my testing on CRM-19723 + $this->assertEquals(20, $items['values'][3]['amount']); + } + + /** + * Do the first contributions, in preparation for an edit-submit. + * + * @return array + * + * @throws \Exception + */ + protected function doInitialSubmit() { + $form = new CRM_Contribute_Form_Contribution(); + + $form->testSubmit(array( + 'total_amount' => 100, + 'financial_type_id' => $this->_financialTypeId, + 'receive_date' => '04/21/2015', + 'receive_date_time' => '11:27PM', + 'contact_id' => $this->_individualId, + 'payment_instrument_id' => array_search('Check', $this->paymentInstruments), + 'contribution_status_id' => 1, + 'price_set_id' => 0, + ), + CRM_Core_Action::ADD + ); + $contribution = $this->callAPISuccessGetSingle('Contribution', + array( + 'contribution_id' => 1, + 'return' => array( + 'tax_amount', + 'total_amount', + 'net_amount', + 'financial_type_id', + 'receive_date', + 'payment_instrument_id', + ), + ) + ); + $this->assertEquals(110, $contribution['total_amount']); + $this->assertEquals(10, $contribution['tax_amount']); + $this->assertEquals(110, $contribution['net_amount']); + return array($form, $contribution); } /** diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php index 93542d9328..907b698060 100644 --- a/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php +++ b/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php @@ -296,9 +296,9 @@ class CRM_Financial_BAO_FinancialItemTest extends CiviUnitTestCase { $contribution = CRM_Contribute_BAO_Contribution::create($params); $financialItem = CRM_Financial_BAO_FinancialItem::getPreviousFinancialItem($contribution->id); - $params = array('id' => $financialItem->id); + $params = array('id' => $financialItem['id']); $financialItem = $this->callAPISuccess('FinancialItem', 'get', $params); - $this->assertEquals($financialItem['values'][$financialItem['id']]['amount'], 200.00, "The amounts do not match."); + $this->assertEquals(200.00, $financialItem['values'][$financialItem['id']]['amount'], "The amounts do not match."); } } -- 2.25.1