Merge pull request #11431 from eileenmcnaughton/merge

Merge

CRM/Contribute/BAO/Contribution.php

@@ -129,6 +129,10 @@ public static function add(&$params, $ids = array()) {
     if (!empty($params['skipCleanMoney'])) {
       unset($moneyFields[0]);
     }
+    else {
+      // @todo put a deprecated here - this should be done in the form layer.
+      $params['skipCleanMoney'] = FALSE;
+    }
 
     foreach ($moneyFields as $field) {
       if (isset($params[$field])) {
@@ -4207,6 +4211,11 @@ public static function getPaymentInfo($id, $component, $getTrxnInfo = FALSE, $us
   public static function checkTaxAmount($params, $isLineItem = FALSE) {
     $taxRates = CRM_Core_PseudoConstant::getTaxRates();
 
+    // This function should be only called after standardisation (removal of
+    // thousand separator & using a decimal point for cents separator.
+    // However, we don't know if that is always true :-(
+    // There is a deprecation notice tho :-)
+    $unknownIfMoneyIsClean = empty($params['skipCleanMoney']) && !$isLineItem;
     // Update contribution.
     if (!empty($params['id'])) {
       // CRM-19126 and CRM-19152 If neither total or financial_type_id are set on an update
@@ -4253,7 +4262,7 @@ public static function checkTaxAmount($params, $isLineItem = FALSE) {
       empty($params['skipLineItem']) && !$isLineItem
     ) {
       $taxRateParams = $taxRates[$params['financial_type_id']];
-      $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount(CRM_Utils_Array::value('total_amount', $params), $taxRateParams);
+      $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount(CRM_Utils_Array::value('total_amount', $params), $taxRateParams, $unknownIfMoneyIsClean);
       $params['tax_amount'] = round($taxAmount['tax_amount'], 2);
 
       // Get Line Item on update of contribution
@@ -4285,9 +4294,9 @@ public static function checkTaxAmount($params, $isLineItem = FALSE) {
     }
     else {
       // update line item of contrbution
-      if (isset($params['financial_type_id']) && array_key_exists($params['financial_type_id'], $taxRates) && $isLineItem) {
+      if (isset($params['financial_type_id']) && array_key_exists($params['financial_type_id'], $taxRates)  && $isLineItem) {
         $taxRate = $taxRates[$params['financial_type_id']];
-        $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($params['line_total'], $taxRate);
+        $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($params['line_total'], $taxRate, $unknownIfMoneyIsClean);
         $params['tax_amount'] = round($taxAmount['tax_amount'], 2);
       }
     }

CRM/Contribute/BAO/Contribution/Utils.php

@@ -130,6 +130,9 @@ public static function processConfirm(
         $contributionParams['payment_instrument_id'] = $paymentParams['payment_instrument_id'] = $form->_paymentProcessor['payment_instrument_id'];
       }
 
+      // @todo this is the wrong place for this - it should be done as close to form submission
+      // as possible
+      $paymentParams['amount'] = CRM_Utils_Rule::cleanMoney($paymentParams['amount']);
       $contribution = CRM_Contribute_Form_Contribution_Confirm::processFormContribution(
         $form,
         $paymentParams,
@@ -484,15 +487,24 @@ public static function getFirstLastDetails($contactID) {
    *   Amount of field.
    * @param float $taxRate
    *   Tax rate of selected financial account for field.
+   * @param bool $ugWeDoNotKnowIfItNeedsCleaning_Help
+   *   This should ALWAYS BE FALSE and then be removed. A 'clean' money string uses a standardised format
+   *   such as '1000.99' for one thousand $/Euro/CUR and ninety nine cents/units.
+   *   However, we are in the habit of not necessarily doing that so need to grandfather in
+   *   the new expectation.
    *
    * @return array
    *   array of tax amount
    *
    */
-  public static function calculateTaxAmount($amount, $taxRate) {
+  public static function calculateTaxAmount($amount, $taxRate, $ugWeDoNotKnowIfItNeedsCleaning_Help = FALSE) {
     $taxAmount = array();
+    if ($ugWeDoNotKnowIfItNeedsCleaning_Help) {
+      Civi::log()->warning('Deprecated function, make sure money is in usable format before calling this.', array('civi.tag' => 'deprecated'));
+      $amount = CRM_Utils_Rule::cleanMoney($amount);
+    }
     // There can not be any rounding at this stage - as this is prior to quantity multiplication
-    $taxAmount['tax_amount'] = ($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount);
+    $taxAmount['tax_amount'] = ($taxRate / 100) * $amount;
 
     return $taxAmount;
   }

CRM/Contribute/Form/Contribution/Confirm.php

@@ -158,7 +158,6 @@ public static function handlePledge(&$form, $params, $contributionParams, $pledg
    *
    * @param array $params
    * @param int $financialTypeID
-   * @param float $nonDeductibleAmount
    * @param bool $pending
    * @param array $paymentProcessorOutcome
    * @param string $receiptDate
@@ -167,13 +166,11 @@ public static function handlePledge(&$form, $params, $contributionParams, $pledg
    * @return array
    */
   public static function getContributionParams(
-    $params, $financialTypeID, $nonDeductibleAmount, $pending,
+    $params, $financialTypeID, $pending,
     $paymentProcessorOutcome, $receiptDate, $recurringContributionID) {
     $contributionParams = array(
       'financial_type_id' => $financialTypeID,
       'receive_date' => (CRM_Utils_Array::value('receive_date', $params)) ? CRM_Utils_Date::processDate($params['receive_date']) : date('YmdHis'),
-      'non_deductible_amount' => $nonDeductibleAmount,
-      'total_amount' => $params['amount'],
       'tax_amount' => CRM_Utils_Array::value('tax_amount', $params),
       'amount_level' => CRM_Utils_Array::value('amount_level', $params),
       'invoice_id' => $params['invoiceID'],
@@ -207,10 +204,6 @@ public static function getContributionParams(
       );
     }
 
-    // CRM-4038: for non-en_US locales, CRM_Contribute_BAO_Contribution::add() expects localised amounts
-    $contributionParams['non_deductible_amount'] = trim(CRM_Utils_Money::format($contributionParams['non_deductible_amount'], ' '));
-    $contributionParams['total_amount'] = trim(CRM_Utils_Money::format($contributionParams['total_amount'], ' '));
-
     if ($recurringContributionID) {
       $contributionParams['contribution_recur_id'] = $recurringContributionID;
     }
@@ -955,7 +948,6 @@ public static function processFormContribution(
     $params['is_recur'] = $isRecur;
     $params['payment_instrument_id'] = CRM_Utils_Array::value('payment_instrument_id', $contributionParams);
     $recurringContributionID = self::processRecurringContribution($form, $params, $contactID, $financialType);
-    $nonDeductibleAmount = self::getNonDeductibleAmount($params, $financialType, $online, $form);
 
     $now = date('YmdHis');
     $receiptDate = CRM_Utils_Array::value('receipt_date', $params);
@@ -965,10 +957,15 @@ public static function processFormContribution(
 
     if (isset($params['amount'])) {
       $contributionParams = array_merge(self::getContributionParams(
-        $params, $financialType->id, $nonDeductibleAmount, TRUE,
+        $params, $financialType->id, TRUE,
         $result, $receiptDate,
         $recurringContributionID), $contributionParams
       );
+      $contributionParams['non_deductible_amount'] = self::getNonDeductibleAmount($params, $financialType, $online, $form);
+      $contributionParams['skipCleanMoney'] = TRUE;
+      // @todo this is the wrong place for this - it should be done as close to form submission
+      // as possible
+      $contributionParams['total_amount'] = $params['amount'];
       $contribution = CRM_Contribute_BAO_Contribution::add($contributionParams);
 
       $invoiceSettings = Civi::settings()->get('contribution_invoice_settings');
@@ -1993,6 +1990,7 @@ public static function submit($params) {
     }
 
     $priceFields = $priceFields[$priceSetID]['fields'];
+    $lineItems = array();
     CRM_Price_BAO_PriceSet::processAmount($priceFields, $paramsProcessedForForm, $lineItems, 'civicrm_contribution');
     $form->_lineItem = array($priceSetID => $lineItems);
     $membershipPriceFieldIDs = array();

CRM/Price/BAO/LineItem.php

@@ -333,6 +333,11 @@ public static function getLineItems($entityId, $entity = 'participant', $isQuick
    *   this is
    *                          lineItem array)
    * @param string $amount_override
+   *   Amount override must be in format 1000.00 - ie no thousand separator & if
+   *   a decimal point is used it should be a decimal
+   *
+   * @todo - this parameter is only used for partial payments. It's unclear why a partial
+   *  payment would change the line item price.
    */
   public static function format($fid, $params, $fields, &$values, $amount_override = NULL) {
     if (empty($params["price_{$fid}"])) {
@@ -356,10 +361,6 @@ public static function format($fid, $params, $fields, &$values, $amount_override
     foreach ($params["price_{$fid}"] as $oid => $qty) {
       $price = $amount_override === NULL ? $options[$oid]['amount'] : $amount_override;
 
-      // lets clean the price in case it is not yet cleant
-      // CRM-10974
-      $price = CRM_Utils_Rule::cleanMoney($price);
-
       $participantsPerField = CRM_Utils_Array::value('count', $options[$oid], 0);
 
       $values[$oid] = array(

CRM/Price/Page/Field.php

@@ -149,7 +149,7 @@ public function browse() {
           $getTaxDetails = TRUE;
         }
         if (isset($priceField[$priceFieldBAO->id]['tax_rate'])) {
-          $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($priceField[$priceFieldBAO->id]['price'], $priceField[$priceFieldBAO->id]['tax_rate']);
+          $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($priceField[$priceFieldBAO->id]['price'], $priceField[$priceFieldBAO->id]['tax_rate'], TRUE);
           $priceField[$priceFieldBAO->id]['tax_amount'] = $taxAmount['tax_amount'];
         }
       }

CRM/Price/Page/Option.php

@@ -158,7 +158,7 @@ public function browse() {
         if ($invoicing && isset($customOption[$id]['tax_rate'])) {
           $getTaxDetails = TRUE;
         }
-        $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($customOption[$id]['amount'], $customOption[$id]['tax_rate']);
+        $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($customOption[$id]['amount'], $customOption[$id]['tax_rate'], TRUE);
         $customOption[$id]['tax_amount'] = $taxAmount['tax_amount'];
       }
       if (!empty($values['financial_type_id'])) {

CRM/Utils/Rule.php

@@ -516,9 +516,15 @@ public static function numberOfDigit($value, $noOfDigit) {
   }
 
   /**
-   * @param $value
+   * Strip thousand separator from a money string.
+   *
+   * Note that this should be done at the form layer. Once we are processing
+   * money at the BAO or processor layer we should be working with something that
+   * is already in a normalised format.
+   *
+   * @param string $value
    *
-   * @return mixed
+   * @return string
    */
   public static function cleanMoney($value) {
     // first remove all white space

api/v3/Contribution.php

@@ -45,6 +45,9 @@ function civicrm_api3_contribution_create(&$params) {
   $values = array();
   _civicrm_api3_custom_format_params($params, $values, 'Contribution');
   $params = array_merge($params, $values);
+  // The BAO should not clean money - it should be done in the form layer & api wrapper
+  // (although arguably the api should expect pre-cleaned it seems to do some cleaning.)
+  $params['skipCleanMoney'] = TRUE;
 
   if (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) {
     if (empty($params['id'])) {

tests/phpunit/CRM/Contribute/BAO/ContributionTest.php

@@ -1084,8 +1084,14 @@ public function testgetSalesTaxFinancialAccounts() {
 
   /**
    * Test for function createProportionalEntry().
+   *
+   * @param string $thousandSeparator
+   *   punctuation used to refer to thousands.
+   *
+   * @dataProvider getThousandSeparators
    */
-  public function testcreateProportionalEntry() {
+  public function testCreateProportionalEntry($thousandSeparator) {
+    $this->setCurrencySeparators($thousandSeparator);
     list($contribution, $financialAccount) = $this->createContributionWithTax();
     $params = array(
       'total_amount' => 55,

tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php

@@ -311,8 +311,14 @@ public function testGetPreviousFinancialItem() {
 
   /**
    * Check method getPreviousFinancialItem() with tax entry.
+   *
+   * @param string $thousandSeparator
+   *   punctuation used to refer to thousands.
+   *
+   * @dataProvider getThousandSeparators
    */
-  public function testGetPreviousFinancialItemHavingTax() {
+  public function testGetPreviousFinancialItemHavingTax($thousandSeparator) {
+    $this->setCurrencySeparators($thousandSeparator);
     $contactId = $this->individualCreate();
     $this->enableTaxAndInvoicing();
     $this->relationForFinancialTypeWithFinancialAccount(1);

tests/phpunit/CRM/Member/Form/MembershipTest.php

@@ -610,8 +610,14 @@ public function testContributionUpdateOnMembershipTypeChange() {
 
   /**
    * Test the submit function of the membership form for partial payment.
+   *
+   * @param string $thousandSeparator
+   *   punctuation used to refer to thousands.
+   *
+   * @dataProvider getThousandSeparators
    */
-  public function testSubmitPartialPayment() {
+  public function testSubmitPartialPayment($thousandSeparator) {
+    $this->setCurrencySeparators($thousandSeparator);
     // Step 1: submit a partial payment for a membership via backoffice
     $form = $this->getForm();
     $form->preProcess();
@@ -629,7 +635,7 @@ public function testSubmitPartialPayment() {
       // This format reflects the 23 being the organisation & the 25 being the type.
       'membership_type_id' => array(23, $this->membershipTypeAnnualFixedID),
       'record_contribution' => 1,
-      'total_amount' => $partiallyPaidAmount,
+      'total_amount' => $this->formatMoneyInput($partiallyPaidAmount),
       'receive_date' => date('m/d/Y', time()),
       'receive_date_time' => '08:36PM',
       'payment_instrument_id' => array_search('Check', $this->paymentInstruments),
@@ -655,7 +661,7 @@ public function testSubmitPartialPayment() {
     $submitParams = array(
       'contribution_id' => $contribution['contribution_id'],
       'contact_id' => $this->_individualId,
-      'total_amount' => $partiallyPaidAmount,
+      'total_amount' => $this->formatMoneyInput($partiallyPaidAmount),
       'currency' => 'USD',
       'financial_type_id' => 2,
       'receive_date' => '04/21/2015',
@@ -676,7 +682,6 @@ public function testSubmitPartialPayment() {
       'contact_id' => $this->_individualId,
     ));
     $this->assertEquals('Completed', $contribution['contribution_status']);
-    // $this->assertEquals(50.00, $contribution['total_amount']);
     // $this->assertEquals(50.00, $contribution['net_amount']);
   }
 

tests/phpunit/CiviTest/CiviUnitTestCase.php

@@ -2587,6 +2587,8 @@ public function quickCleanUpFinancialEntities() {
     $var = TRUE;
     CRM_Member_BAO_Membership::createRelatedMemberships($var, $var, TRUE);
     $this->disableTaxAndInvoicing();
+    $this->setCurrencySeparators(',');
+    CRM_Core_PseudoConstant::flush('taxRates');
     System::singleton()->flushProcessors();
   }
 
@@ -3943,4 +3945,35 @@ public function getFormObject($class) {
     return $form;
   }
 
+  /**
+   * Get possible thousand separators.
+   *
+   * @return array
+   */
+  public function getThousandSeparators() {
+    return array(array('.'), array(','));
+  }
+
+  /**
+   * Set the separators for thousands and decimal points.
+   *
+   * @param string $thousandSeparator
+   */
+  protected function setCurrencySeparators($thousandSeparator) {
+    Civi::settings()->set('monetaryThousandSeparator', $thousandSeparator);
+    Civi::settings()
+      ->set('monetaryDecimalPoint', ($thousandSeparator === ',' ? '.' : ','));
+  }
+
+  /**
+   * Format money as it would be input.
+   *
+   * @param string $amount
+   *
+   * @return string
+   */
+  protected function formatMoneyInput($amount) {
+    return CRM_Utils_Money::format($amount, NULL, '%a');
+  }
+
 }