Contribution import - tests & fixes on dates, amount

Fixes inconsistent amount handling - these are issues that I found through writing unit tests to cover them The money handling was permitting amounts where the decimal separator was before the thousand. On the dates there was a format being handled for some fields but not others
civicrm · Jun 4, 2022 · 6990696 · 6990696
1 parent 0bd65eb
commit 6990696
Show file tree

Hide file tree

Showing 14 changed files with 183 additions and 200 deletions.
diff --git a/CRM/Activity/Import/Parser/Activity.php b/CRM/Activity/Import/Parser/Activity.php
@@ -156,7 +156,7 @@ public function import($onDuplicate, &$values) {
     foreach ($params as $key => $val) {
       if ($customFieldID = CRM_Core_BAO_CustomField::getKeyID($key)) {
         if (!empty($customFields[$customFieldID]) && $customFields[$customFieldID]['data_type'] == 'Date') {
-          CRM_Contact_Import_Parser_Contact::formatCustomDate($params, $params, $dateType, $key);
+          $this->formatCustomDate($params, $params, $dateType, $key);
         }
         elseif (!empty($customFields[$customFieldID]) && $customFields[$customFieldID]['data_type'] == 'Boolean') {
           $params[$key] = CRM_Utils_String::strtoboolstr($val);

diff --git a/CRM/Contact/Import/Parser/Contact.php b/CRM/Contact/Import/Parser/Contact.php
@@ -413,7 +413,7 @@ private function formatCommonData($params, &$formatted) {
         //we should not update Date to null, CRM-4062
         if ($val && ($customFields[$customFieldID]['data_type'] == 'Date')) {
           //CRM-21267
-          CRM_Contact_Import_Parser_Contact::formatCustomDate($params, $formatted, $dateType, $key);
+          $this->formatCustomDate($params, $formatted, $dateType, $key);
         }
         elseif ($customFields[$customFieldID]['data_type'] == 'Boolean') {
           if (empty($val) && !is_numeric($val) && $this->_onDuplicate == CRM_Import_Parser::DUPLICATE_FILL) {
@@ -1137,24 +1137,6 @@ public function formatParams(&$params, $onDuplicate, $cid) {
     }
   }
 
-  /**
-   * Convert any given date string to default date array.
-   *
-   * @param array $params
-   *   Has given date-format.
-   * @param array $formatted
-   *   Store formatted date in this array.
-   * @param int $dateType
-   *   Type of date.
-   * @param string $dateParam
-   *   Index of params.
-   */
-  public static function formatCustomDate(&$params, &$formatted, $dateType, $dateParam) {
-    //fix for CRM-2687
-    CRM_Utils_Date::convertToDefaultDate($params, $dateType, $dateParam);
-    $formatted[$dateParam] = CRM_Utils_Date::processDate($params[$dateParam]);
-  }
-
   /**
    * Get the message for a successful import.
    *

diff --git a/CRM/Contribute/Import/Form/MapField.php b/CRM/Contribute/Import/Form/MapField.php
@@ -247,9 +247,11 @@ public function buildQuickForm() {
             0,
           ];
         }
-        if (!empty($mapperKeysValues) && $mapperKeysValues[$i][0] == 'soft_credit') {
-          $js .= "cj('#mapper_" . $i . "_1').val($mapperKeysValues[$i][1]);\n";
-          $js .= "cj('#mapper_" . $i . "_2').val($mapperKeysValues[$i][2]);\n";
+        if (!empty($mapperKeysValues) && ($mapperKeysValues[$i][0] ?? NULL) === 'soft_credit') {
+          $softCreditField = $mapperKeysValues[$i][1];
+          $softCreditTypeID = $mapperKeysValues[$i][2];
+          $js .= "cj('#mapper_" . $i . "_1').val($softCreditField);\n";
+          $js .= "cj('#mapper_" . $i . "_2').val($softCreditTypeID);\n";
         }
       }
       $sel->setOptions([$sel1, $sel2, $sel3, $sel4]);
@@ -453,8 +455,7 @@ public function postProcess() {
       $this->getSubmittedValue('fieldSeparator'),
       $mapper,
       $this->getSubmittedValue('skipColumnHeader'),
-      CRM_Import_Parser::MODE_PREVIEW,
-      $this->get('contactType')
+      CRM_Import_Parser::MODE_PREVIEW
     );
 
     // add all the necessary variables to the form

diff --git a/CRM/Contribute/Import/Parser/Contribution.php b/CRM/Contribute/Import/Parser/Contribution.php
@@ -110,13 +110,6 @@ public function __construct($mapperKeys = []) {
    */
   protected $_softCreditErrorsFileName;
 
-  /**
-   * Whether the file has a column header or not
-   *
-   * @var bool
-   */
-  protected $_haveColumnHeader;
-
   /**
    * @param string $fileName
    * @param string $separator
@@ -146,8 +139,6 @@ public function run(
 
     $this->init();
 
-    $this->_haveColumnHeader = $skipColumnHeader;
-
     $this->_lineCount = $this->_validSoftCreditRowCount = $this->_validPledgePaymentRowCount = 0;
     $this->_invalidRowCount = $this->_validCount = $this->_invalidSoftCreditRowCount = $this->_invalidPledgePaymentRowCount = 0;
     $this->_totalCount = 0;
@@ -225,39 +216,27 @@ public function run(
       if ($returnCode == self::ERROR) {
         $this->_invalidRowCount++;
         $recordNumber = $this->_lineCount;
-        if ($this->_haveColumnHeader) {
-          $recordNumber--;
-        }
         array_unshift($values, $recordNumber);
         $this->_errors[] = $values;
       }
 
       if ($returnCode == self::PLEDGE_PAYMENT_ERROR) {
         $this->_invalidPledgePaymentRowCount++;
         $recordNumber = $this->_lineCount;
-        if ($this->_haveColumnHeader) {
-          $recordNumber--;
-        }
         array_unshift($values, $recordNumber);
         $this->_pledgePaymentErrors[] = $values;
       }
 
       if ($returnCode == self::SOFT_CREDIT_ERROR) {
         $this->_invalidSoftCreditRowCount++;
         $recordNumber = $this->_lineCount;
-        if ($this->_haveColumnHeader) {
-          $recordNumber--;
-        }
         array_unshift($values, $recordNumber);
         $this->_softCreditErrors[] = $values;
       }
 
       if ($returnCode == self::DUPLICATE) {
         $this->_duplicateCount++;
         $recordNumber = $this->_lineCount;
-        if ($this->_haveColumnHeader) {
-          $recordNumber--;
-        }
         array_unshift($values, $recordNumber);
         $this->_duplicates[] = $values;
         if ($onDuplicate != self::DUPLICATE_SKIP) {
@@ -376,6 +355,9 @@ protected function getFieldMappings(): array {
   public function getMappedRow(array $values): array {
     $params = [];
     foreach ($this->getFieldMappings() as $i => $mappedField) {
+      if ($mappedField['name'] === 'do_not_import' || $mappedField['name'] === NULL) {
+        continue;
+      }
       if (!empty($mappedField['soft_credit_match_field'])) {
         $params['soft_credit'][$i] = ['soft_credit_type_id' => $mappedField['soft_credit_type_id'], $mappedField['soft_credit_match_field'] => $values[$i]];
       }
@@ -640,7 +622,6 @@ public function summary(&$values) {
     if ($errorMessage) {
       $tempMsg = "Invalid value for field(s) : $errorMessage";
       array_unshift($values, $tempMsg);
-      $errorMessage = NULL;
       $this->setImportStatus($rowNumber, 'ERROR', $tempMsg);
       return CRM_Import_Parser::ERROR;
     }
@@ -675,7 +656,7 @@ public function import($onDuplicate, &$values) {
     }
 
     $params = $this->getMappedRow($values);
-    $formatted = array_merge(['version' => 3, 'skipRecentView' => TRUE, 'skipCleanMoney' => FALSE, 'contribution_id' => $params['id'] ?? NULL], $params);
+    $formatted = array_merge(['version' => 3, 'skipRecentView' => TRUE, 'skipCleanMoney' => TRUE, 'contribution_id' => $params['id'] ?? NULL], $params);
     //CRM-10994
     if (isset($params['total_amount']) && $params['total_amount'] == 0) {
       $params['total_amount'] = '0.00';
@@ -967,61 +948,6 @@ public function &getImportedContributions() {
     return $this->_newContributions;
   }
 
-  /**
-   * Format date fields from input to mysql.
-   *
-   * @param array $params
-   *
-   * @return array
-   *   Error messages, if any.
-   */
-  public function formatDateFields(&$params) {
-    $errorMessage = [];
-    $dateType = CRM_Core_Session::singleton()->get('dateTypes');
-    foreach ($params as $key => $val) {
-      if ($val) {
-        switch ($key) {
-          case 'receive_date':
-            if ($dateValue = CRM_Utils_Date::formatDate($params[$key], $dateType)) {
-              $params[$key] = $dateValue;
-            }
-            else {
-              $errorMessage[] = ts('Receive Date');
-            }
-            break;
-
-          case 'cancel_date':
-            if ($dateValue = CRM_Utils_Date::formatDate($params[$key], $dateType)) {
-              $params[$key] = $dateValue;
-            }
-            else {
-              $errorMessage[] = ts('Cancel Date');
-            }
-            break;
-
-          case 'receipt_date':
-            if ($dateValue = CRM_Utils_Date::formatDate($params[$key], $dateType)) {
-              $params[$key] = $dateValue;
-            }
-            else {
-              $errorMessage[] = ts('Receipt date');
-            }
-            break;
-
-          case 'thankyou_date':
-            if ($dateValue = CRM_Utils_Date::formatDate($params[$key], $dateType)) {
-              $params[$key] = $dateValue;
-            }
-            else {
-              $errorMessage[] = ts('Thankyou Date');
-            }
-            break;
-        }
-      }
-    }
-    return $errorMessage;
-  }
-
   /**
    * Format input params to suit api handling.
    *
@@ -1034,37 +960,16 @@ public function formatDateFields(&$params) {
    * @param array $formatted
    */
   public function formatInput(&$params, &$formatted = []) {
-    $dateType = CRM_Core_Session::singleton()->get('dateTypes');
-    $customDataType = !empty($params['contact_type']) ? $params['contact_type'] : 'Contribution';
-    $customFields = CRM_Core_BAO_CustomField::getFields($customDataType);
-    // @todo call formatDateFields & move custom data handling there.
-    // Also note error handling for dates is currently in  deprecatedFormatParams
-    // we should use the error handling in formatDateFields.
     foreach ($params as $key => $val) {
       // @todo - call formatDateFields instead.
       if ($val) {
         switch ($key) {
-          case 'receive_date':
-          case 'cancel_date':
-          case 'receipt_date':
-          case 'thankyou_date':
-            $params[$key] = CRM_Utils_Date::formatDate($params[$key], $dateType);
-            break;
 
           case 'pledge_payment':
             $params[$key] = CRM_Utils_String::strtobool($val);
             break;
 
         }
-        if ($customFieldID = CRM_Core_BAO_CustomField::getKeyID($key)) {
-          if ($customFields[$customFieldID]['data_type'] == 'Date') {
-            CRM_Contact_Import_Parser_Contact::formatCustomDate($params, $formatted, $dateType, $key);
-            unset($params[$key]);
-          }
-          elseif ($customFields[$customFieldID]['data_type'] == 'Boolean') {
-            $params[$key] = CRM_Utils_String::strtoboolstr($val);
-          }
-        }
       }
     }
   }
@@ -1183,16 +1088,6 @@ private function deprecatedFormatParams($params, &$values, $create = FALSE, $onD
           }
           break;
 
-        case 'non_deductible_amount':
-        case 'total_amount':
-        case 'fee_amount':
-        case 'net_amount':
-          // @todo add test like testPaymentTypeLabel & remove these lines as we can anticipate error will still be caught & handled.
-          if (!CRM_Utils_Rule::money($value)) {
-            return civicrm_api3_create_error("$key not a valid amount: $value");
-          }
-          break;
-
         case 'currency':
           if (!CRM_Utils_Rule::currencyCode($value)) {
             return civicrm_api3_create_error("currency not a valid code: $value");

diff --git a/CRM/Custom/Import/Parser/Api.php b/CRM/Custom/Import/Parser/Api.php
@@ -222,7 +222,7 @@ private function formatCommonData($params, &$formatted) {
         //we should not update Date to null, CRM-4062
         if ($val && ($customFields[$customFieldID]['data_type'] == 'Date')) {
           //CRM-21267
-          CRM_Contact_Import_Parser_Contact::formatCustomDate($params, $formatted, $dateType, $key);
+          $this->formatCustomDate($params, $formatted, $dateType, $key);
         }
         elseif ($customFields[$customFieldID]['data_type'] == 'Boolean') {
           if (empty($val) && !is_numeric($val)) {

diff --git a/CRM/Event/Import/Parser/Participant.php b/CRM/Event/Import/Parser/Participant.php
@@ -290,7 +290,7 @@ public function import($onDuplicate, &$values) {
       if ($val) {
         if ($customFieldID = CRM_Core_BAO_CustomField::getKeyID($key)) {
           if ($customFields[$customFieldID]['data_type'] == 'Date') {
-            CRM_Contact_Import_Parser_Contact::formatCustomDate($params, $formatted, $dateType, $key);
+            $this->formatCustomDate($params, $formatted, $dateType, $key);
             unset($params[$key]);
           }
           elseif ($customFields[$customFieldID]['data_type'] == 'Boolean') {

diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php
@@ -1278,14 +1278,17 @@ protected function getTransformedFieldValue(string $fieldName, $importedValue) {
     if ($fieldMetadata['type'] === CRM_Utils_Type::T_FLOAT) {
       return CRM_Utils_Rule::numeric($importedValue) ? $importedValue : 'invalid_import_value';
     }
+    if ($fieldMetadata['type'] === CRM_Utils_Type::T_MONEY) {
+      return CRM_Utils_Rule::money($importedValue, TRUE) ? CRM_Utils_Rule::cleanMoney($importedValue) : 'invalid_import_value';
+    }
     if ($fieldMetadata['type'] === CRM_Utils_Type::T_BOOLEAN) {
       $value = CRM_Utils_String::strtoboolstr($importedValue);
       if ($value !== FALSE) {
         return (bool) $value;
       }
       return 'invalid_import_value';
     }
-    if ($fieldMetadata['type'] === CRM_Utils_Type::T_DATE) {
+    if ($fieldMetadata['type'] === CRM_Utils_Type::T_DATE || $fieldMetadata['type'] === (CRM_Utils_Type::T_DATE + CRM_Utils_Type::T_TIME) || $fieldMetadata['type'] === CRM_Utils_Type::T_TIMESTAMP ) {
       $value = CRM_Utils_Date::formatDate($importedValue, $this->getSubmittedValue('dateFormats'));
       return ($value) ?: 'invalid_import_value';
     }
@@ -1742,4 +1745,22 @@ protected function setImportStatus(int $id, string $status, string $message, ?in
     $this->getDataSourceObject()->updateStatus($id, $status, $message, $entityID);
   }
 
+  /**
+   * Convert any given date string to default date array.
+   *
+   * @param array $params
+   *   Has given date-format.
+   * @param array $formatted
+   *   Store formatted date in this array.
+   * @param int $dateType
+   *   Type of date.
+   * @param string $dateParam
+   *   Index of params.
+   */
+  public static function formatCustomDate(&$params, &$formatted, $dateType, $dateParam) {
+    //fix for CRM-2687
+    CRM_Utils_Date::convertToDefaultDate($params, $dateType, $dateParam);
+    $formatted[$dateParam] = CRM_Utils_Date::processDate($params[$dateParam]);
+  }
+
 }
diff --git a/CRM/Member/Import/Parser/Membership.php b/CRM/Member/Import/Parser/Membership.php
@@ -668,7 +668,7 @@ public function import($onDuplicate, &$values) {
           }
           if ($customFieldID = CRM_Core_BAO_CustomField::getKeyID($key)) {
             if ($customFields[$customFieldID]['data_type'] == 'Date') {
-              CRM_Contact_Import_Parser_Contact::formatCustomDate($params, $formatted, $dateType, $key);
+              $this->formatCustomDate($params, $formatted, $dateType, $key);
               unset($params[$key]);
             }
             elseif ($customFields[$customFieldID]['data_type'] == 'Boolean') {

diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php
@@ -588,10 +588,23 @@ public static function cleanMoney($value) {
 
   /**
    * @param string $value
+   * @param bool $checkSeparatorOrder
+   *   Should the order of the separators be checked. ie if the thousand
+   *   separator is , then it should never be after the decimal separator .
+   *   so 1.300,23 would be invalid in that case. Honestly I'm amazed this
+   *   check wasn't being done but in the interest of caution adding as opt in.
+   *   Note clean money would convert this to 1.30023....
    *
    * @return bool
    */
-  public static function money($value) {
+  public static function money($value, bool $checkSeparatorOrder = FALSE) {
+    if ($checkSeparatorOrder) {
+      $thousandSeparatorPosition = strpos((string) $value, \Civi::settings()->get('monetaryThousandSeparator'));
+      $decimalSeparatorPosition = strpos((string) $value, \Civi::settings()->get('monetaryDecimalPoint'));
+      if ($thousandSeparatorPosition && $decimalSeparatorPosition && $thousandSeparatorPosition > $decimalSeparatorPosition) {
+        return FALSE;
+      }
+    }
     $value = self::cleanMoney($value);
 
     if (self::integer($value)) {