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.

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 6d8f3de6dd9cc174e69a846c99e9dc2d817f759d..5b5b4768bfb5eefd0890df7952ea2471f3b86bfe 100644 (file)
--- 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 (file)
index 0000000..d14a3ae
--- /dev/null
+++ b/CRM/Contribute/Exception/CheckLineItemsException.php
@@ -0,0 +1,13 @@
+<?php\r
+\r
+/**\r
+ * Class CRM_Contribute_Exception_CheckLineItemsException\r
+ */\r
+class CRM_Contribute_Exception_CheckLineItemsException extends API_Exception {\r
+  const LINE_ITEM_DIFFERRING_TOTAL_EXCEPTON_MSG = "Line item total doesn't match with total amount.";\r
+\r
+  public function __construct($message = self::LINE_ITEM_DIFFERRING_TOTAL_EXCEPTON_MSG, $error_code = 0, array $extraParams = [], $previous = NULL) {\r
+    parent::__construct($message, $error_code, $extraParams, $previous);\r
+  }\r
+\r
+}\r

diff --git a/CRM/Utils/Money.php b/CRM/Utils/Money.php
index 28ecf6aafe9f662e09461120983c72f3422f77fe..59168cb606a7dc6c890c16118db69f0f5f3fe6f3 100644 (file)
--- a/CRM/Utils/Money.php
+++ b/CRM/Utils/Money.php
@@ -154,4 +154,36 @@ class CRM_Utils_Money {
     }
   }
 
+  /**
+   * Tests if two currency values are equal, taking into account the currency's
+   * precision, so that if the difference between the two values is less than
+   * one more order of magnitude for the precision, then the values are
+   * considered as equal. So, if the currency has  precision of 2 decimal
+   * points, a difference of more than 0.001 will cause the values to be
+   * considered as different. Anything less than 0.001 will be considered as
+   * equal.
+   *
+   * Eg.
+   *
+   * 1.2312 == 1.2319 with a currency precision of 2 decimal points
+   * 1.2310 != 1.2320 with a currency precision of 2 decimal points
+   * 1.3000 != 1.2000 with a currency precision of 2 decimal points
+   *
+   * @param $value1
+   * @param $value2
+   * @param $currency
+   *
+   * @return bool
+   */
+  public static function equals($value1, $value2, $currency) {
+    $precision = 1 / pow(10, self::getCurrencyPrecision($currency) + 1);
+    $difference = self::subtractCurrencies($value1, $value2, $currency);
+
+    if (abs($difference) > $precision) {
+      return FALSE;
+    }
+
+    return TRUE;
+  }
+
 }

diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
index ae9a427c9487f1b7aecfcc8711f47232fd047bc3..97ffc6103af6b4ed5539d97543c86cebbbae5f7a 100644 (file)
--- 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 c3b0b063f3303ed341efa9dae4c4d87fa0e8d765..921818cad58c9f5bb95fd77a008f3fe270bea102 100644 (file)
--- 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
    */