From c16c6ad8329246c1375e2ec99630a07504a2f224 Mon Sep 17 00:00:00 2001 From: Camilo Rodriguez Date: Mon, 18 Jun 2018 18:48:44 +0000 Subject: [PATCH] CRM-188: Fix Floating Point Precision Comparison Exception Under certain circumstances, floating point comparison between set and calculated values may fail, even if they should be equal. This is due to the way floating point values are represented internally in binary form. To avoid this problem, comparison needs to be made using an expected precision below which compared values may be treated as equal. Implemented precision of 0.001 and used it to check the difference between both values is less. --- CRM/Contribute/BAO/Contribution.php | 17 ++++- .../Exception/CheckLineItemsException.php | 13 ++++ CRM/Utils/Money.php | 32 +++++++++ .../CRM/Contribute/BAO/ContributionTest.php | 70 ++++++++++++++++++- tests/phpunit/CRM/Utils/MoneyTest.php | 11 +++ 5 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 CRM/Contribute/Exception/CheckLineItemsException.php diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 6d8f3de6dd..5b5b4768bf 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -4997,6 +4997,7 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) public static function checkLineItems(&$params) { $totalAmount = CRM_Utils_Array::value('total_amount', $params); $lineItemAmount = 0; + foreach ($params['line_items'] as &$lineItems) { foreach ($lineItems['line_item'] as &$item) { if (empty($item['financial_type_id'])) { @@ -5005,11 +5006,20 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) $lineItemAmount += $item['line_total']; } } + if (!isset($totalAmount)) { $params['total_amount'] = $lineItemAmount; } - elseif ($totalAmount != $lineItemAmount) { - throw new API_Exception("Line item total doesn't match with total amount."); + else { + $currency = CRM_Utils_Array::value('currency', $params, ''); + + if (empty($currency)) { + $currency = CRM_Core_Config::singleton()->defaultCurrency; + } + + if (!CRM_Utils_Money::equals($totalAmount, $lineItemAmount, $currency)) { + throw new CRM_Contribute_Exception_CheckLineItemsException(); + } } } @@ -5026,11 +5036,13 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) if (!empty($params['financial_account_id'])) { return $params['financial_account_id']; } + $contributionStatus = CRM_Contribute_PseudoConstant::contributionStatus($params['contribution_status_id'], 'name'); $preferredAccountsRelationships = array( 'Refunded' => 'Credit/Contra Revenue Account is', 'Chargeback' => 'Chargeback Account is', ); + if (in_array($contributionStatus, array_keys($preferredAccountsRelationships))) { $financialTypeID = !empty($params['financial_type_id']) ? $params['financial_type_id'] : $params['prevContribution']->financial_type_id; return CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship( @@ -5038,6 +5050,7 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) $preferredAccountsRelationships[$contributionStatus] ); } + return $default; } diff --git a/CRM/Contribute/Exception/CheckLineItemsException.php b/CRM/Contribute/Exception/CheckLineItemsException.php new file mode 100644 index 0000000000..d14a3aec98 --- /dev/null +++ b/CRM/Contribute/Exception/CheckLineItemsException.php @@ -0,0 +1,13 @@ + $precision) { + return FALSE; + } + + return TRUE; + } + } diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php index ae9a427c94..97ffc6103a 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -793,18 +793,84 @@ WHERE eft.entity_id = %1 AND ft.to_financial_account_id <> %2"; ), ), ); + try { CRM_Contribute_BAO_Contribution::checkLineItems($params); $this->fail("Missed expected exception"); } - catch (Exception $e) { - $this->assertEquals("Line item total doesn't match with total amount.", $e->getMessage()); + catch (CRM_Contribute_Exception_CheckLineItemsException $e) { + $this->assertEquals( + CRM_Contribute_Exception_CheckLineItemsException::LINE_ITEM_DIFFERRING_TOTAL_EXCEPTON_MSG, + $e->getMessage() + ); } + $this->assertEquals(3, $params['line_items'][0]['line_item'][0]['financial_type_id']); $params['total_amount'] = 300; + CRM_Contribute_BAO_Contribution::checkLineItems($params); } + /** + * Tests CRM_Contribute_BAO_Contribution::checkLineItems() method works with + * floating point values. + */ + public function testCheckLineItemsWithFloatingPointValues() { + $params = array( + 'contact_id' => 202, + 'receive_date' => date('Y-m-d'), + 'total_amount' => 16.67, + 'financial_type_id' => 3, + 'line_items' => array( + array( + 'line_item' => array( + array( + 'entity_table' => 'civicrm_contribution', + 'price_field_id' => 8, + 'price_field_value_id' => 16, + 'label' => 'test 1', + 'qty' => 1, + 'unit_price' => 14.85, + 'line_total' => 14.85, + ), + array( + 'entity_table' => 'civicrm_contribution', + 'price_field_id' => 8, + 'price_field_value_id' => 17, + 'label' => 'Test 2', + 'qty' => 1, + 'unit_price' => 1.66, + 'line_total' => 1.66, + 'financial_type_id' => 1, + ), + array( + 'entity_table' => 'civicrm_contribution', + 'price_field_id' => 8, + 'price_field_value_id' => 17, + 'label' => 'Test 2', + 'qty' => 1, + 'unit_price' => 0.16, + 'line_total' => 0.16, + 'financial_type_id' => 1, + ), + ), + 'params' => array(), + ), + ), + ); + + $foundException = FALSE; + + try { + CRM_Contribute_BAO_Contribution::checkLineItems($params); + } + catch (CRM_Contribute_Exception_CheckLineItemsException $e) { + $foundException = TRUE; + } + + $this->assertFalse($foundException); + } + /** * Test activity amount updation. */ diff --git a/tests/phpunit/CRM/Utils/MoneyTest.php b/tests/phpunit/CRM/Utils/MoneyTest.php index c3b0b063f3..921818cad5 100644 --- a/tests/phpunit/CRM/Utils/MoneyTest.php +++ b/tests/phpunit/CRM/Utils/MoneyTest.php @@ -19,6 +19,17 @@ class CRM_Utils_MoneyTest extends CiviUnitTestCase { $this->assertEquals($expectedResult, CRM_Utils_Money::subtractCurrencies($leftOp, $rightOp, $currency)); } + public function testEquals() { + $testValue = 0.01; + + for ($i = 0; $i <= 10; $i++) { + $equalValues = CRM_Utils_Money::equals($testValue, $testValue + ($i * 0.0001), 'USD'); + $this->assertTrue($equalValues); + } + + $this->assertFalse(CRM_Utils_Money::equals($testValue, $testValue + 0.001000000001, 'USD')); + } + /** * @return array */ -- 2.25.1