From 4b58c5c4208a89800bb361840f8c442378cd19a3 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Sat, 4 Jun 2022 15:32:08 +1200 Subject: [PATCH] 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 --- CRM/Activity/Import/Parser/Activity.php | 2 +- CRM/Contact/Import/Parser/Contact.php | 20 +-- CRM/Contribute/Import/Form/MapField.php | 11 +- CRM/Contribute/Import/Parser/Contribution.php | 113 +------------ CRM/Custom/Import/Parser/Api.php | 2 +- CRM/Event/Import/Parser/Participant.php | 2 +- CRM/Import/Parser.php | 23 ++- CRM/Member/Import/Parser/Membership.php | 2 +- CRM/Utils/Rule.php | 16 +- api/v3/utils.php | 22 +-- .../Import/Parser/ContributionTest.php | 154 ++++++++++++------ .../data/contributions_amount_validate.csv | 3 + .../data/contributions_date_validate.csv | 2 + tests/phpunit/CiviTest/CiviUnitTestCase.php | 12 ++ 14 files changed, 184 insertions(+), 200 deletions(-) create mode 100644 tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_amount_validate.csv create mode 100644 tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_date_validate.csv diff --git a/CRM/Activity/Import/Parser/Activity.php b/CRM/Activity/Import/Parser/Activity.php index b09c38683e..089fa18eac 100644 --- a/CRM/Activity/Import/Parser/Activity.php +++ b/CRM/Activity/Import/Parser/Activity.php @@ -156,7 +156,7 @@ class CRM_Activity_Import_Parser_Activity extends CRM_Import_Parser { 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 index 198dba14ae..e51271549b 100644 --- a/CRM/Contact/Import/Parser/Contact.php +++ b/CRM/Contact/Import/Parser/Contact.php @@ -413,7 +413,7 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { //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 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { } } - /** - * 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 index 7b3c7fdbfb..15031e5616 100644 --- a/CRM/Contribute/Import/Form/MapField.php +++ b/CRM/Contribute/Import/Form/MapField.php @@ -247,9 +247,11 @@ class CRM_Contribute_Import_Form_MapField extends CRM_Import_Form_MapField { 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 @@ class CRM_Contribute_Import_Form_MapField extends CRM_Import_Form_MapField { $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 index 96aa3cc236..7d310a57dd 100644 --- a/CRM/Contribute/Import/Parser/Contribution.php +++ b/CRM/Contribute/Import/Parser/Contribution.php @@ -110,13 +110,6 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { */ 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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { $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,9 +216,6 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { if ($returnCode == self::ERROR) { $this->_invalidRowCount++; $recordNumber = $this->_lineCount; - if ($this->_haveColumnHeader) { - $recordNumber--; - } array_unshift($values, $recordNumber); $this->_errors[] = $values; } @@ -235,9 +223,6 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { if ($returnCode == self::PLEDGE_PAYMENT_ERROR) { $this->_invalidPledgePaymentRowCount++; $recordNumber = $this->_lineCount; - if ($this->_haveColumnHeader) { - $recordNumber--; - } array_unshift($values, $recordNumber); $this->_pledgePaymentErrors[] = $values; } @@ -245,9 +230,6 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { if ($returnCode == self::SOFT_CREDIT_ERROR) { $this->_invalidSoftCreditRowCount++; $recordNumber = $this->_lineCount; - if ($this->_haveColumnHeader) { - $recordNumber--; - } array_unshift($values, $recordNumber); $this->_softCreditErrors[] = $values; } @@ -255,9 +237,6 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { 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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { 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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { 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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { } $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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { 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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { * @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 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser { } 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 index 2532614ca0..544e14e76e 100644 --- a/CRM/Custom/Import/Parser/Api.php +++ b/CRM/Custom/Import/Parser/Api.php @@ -222,7 +222,7 @@ class CRM_Custom_Import_Parser_Api extends CRM_Import_Parser { //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 index 476f0b8ccd..2f5162b43b 100644 --- a/CRM/Event/Import/Parser/Participant.php +++ b/CRM/Event/Import/Parser/Participant.php @@ -290,7 +290,7 @@ class CRM_Event_Import_Parser_Participant extends CRM_Import_Parser { 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 index 93c5260ff1..6bd51e106e 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -1278,6 +1278,9 @@ abstract class CRM_Import_Parser { 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) { @@ -1285,7 +1288,7 @@ abstract class CRM_Import_Parser { } 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 @@ abstract class CRM_Import_Parser { $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 index 915385c653..7fb151b5a9 100644 --- a/CRM/Member/Import/Parser/Membership.php +++ b/CRM/Member/Import/Parser/Membership.php @@ -668,7 +668,7 @@ class CRM_Member_Import_Parser_Membership extends CRM_Import_Parser { } 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 index 79004f5ec1..4ba6855887 100644 --- a/CRM/Utils/Rule.php +++ b/CRM/Utils/Rule.php @@ -588,10 +588,24 @@ class CRM_Utils_Rule { /** * @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, $checkSeparatorOrder = FALSE) { + // We can't rely on only one var being passed so can't type-hint to a bool. + if ($checkSeparatorOrder === TRUE) { + $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)) { diff --git a/api/v3/utils.php b/api/v3/utils.php index 4f0674e884..516f98c086 100644 --- a/api/v3/utils.php +++ b/api/v3/utils.php @@ -220,7 +220,7 @@ function civicrm_api3_create_success($values = 1, $params = [], $entity = NULL, } if ($result['count'] == 1) { - list($result['id']) = array_keys($values); + [$result['id']] = array_keys($values); } elseif (!empty($values['id']) && is_int($values['id'])) { $result['id'] = $values['id']; @@ -571,7 +571,7 @@ function _civicrm_api3_get_using_query_object($entity, $params, $additional_opti $skipPermissions = !empty($params['check_permissions']) ? 0 : 1; - list($entities) = CRM_Contact_BAO_Query::apiQuery( + [$entities] = CRM_Contact_BAO_Query::apiQuery( $newParams, $returnProperties, NULL, @@ -619,7 +619,7 @@ function _civicrm_api3_get_query_object($params, $mode, $entity) { empty($params['check_permissions']), TRUE, TRUE, NULL, 'AND', 'NULL', TRUE ); - list($select, $from, $where, $having) = $query->query(); + [$select, $from, $where, $having] = $query->query(); $sql = "$select $from $where $having"; @@ -1108,7 +1108,7 @@ function _civicrm_api3_custom_format_params($params, &$values, $extends, $entity } foreach ($params as $key => $value) { - list($customFieldID, $customValueID) = CRM_Core_BAO_CustomField::getKeyID($key, TRUE); + [$customFieldID, $customValueID] = CRM_Core_BAO_CustomField::getKeyID($key, TRUE); if ($customFieldID && (!is_null($value))) { if ($checkCheckBoxField && !empty($fields['custom_' . $customFieldID]) && $fields['custom_' . $customFieldID]['html_type'] == 'CheckBox') { formatCheckBoxField($value, 'custom_' . $customFieldID, $entity); @@ -1537,7 +1537,7 @@ function _civicrm_api3_validate_switch_cases($fieldName, $fieldInfo, $entity, $p break; case CRM_Utils_Type::T_MONEY: - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName); foreach ((array) $fieldValue as $fieldvalue) { if (!CRM_Utils_Rule::money($fieldvalue) && !empty($fieldvalue)) { @@ -1606,7 +1606,7 @@ function _civicrm_api3_validate_fields($entity, $action, &$params, $fields) { break; case CRM_Utils_Type::T_MONEY: - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName); if (strpos($op, 'NULL') !== FALSE || strpos($op, 'EMPTY') !== FALSE) { break; } @@ -1676,7 +1676,7 @@ function _civicrm_api3_validate_foreign_keys($entity, $action, &$params, $fields * @throws Exception */ function _civicrm_api3_validate_date(&$params, &$fieldName, &$fieldInfo) { - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName); if (strpos($op, 'NULL') !== FALSE || strpos($op, 'EMPTY') !== FALSE) { return; } @@ -1766,7 +1766,7 @@ function _civicrm_api3_validate_constraint($fieldValue, $fieldName, $fieldInfo, * @throws Exception */ function _civicrm_api3_validate_unique_key(&$params, &$fieldName) { - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName); if (strpos($op, 'NULL') !== FALSE || strpos($op, 'EMPTY') !== FALSE) { return; } @@ -2056,7 +2056,7 @@ function _civicrm_api3_swap_out_aliases(&$apiRequest, $fields) { * @throws API_Exception */ function _civicrm_api3_validate_integer(&$params, $fieldName, &$fieldInfo, $entity) { - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName); if ($fieldName === 'auto_renew' && $fieldValue === TRUE) { // https://lab.civicrm.org/dev/rc/-/issues/14 $fieldValue = 1; @@ -2206,7 +2206,7 @@ function _civicrm_api3_resolve_contactID($contactIdExpr) { * @throws API_Exception */ function _civicrm_api3_validate_html(&$params, &$fieldName, $fieldInfo) { - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName); if (strpos($op, 'NULL') || strpos($op, 'EMPTY')) { return; } @@ -2229,7 +2229,7 @@ function _civicrm_api3_validate_html(&$params, &$fieldName, $fieldInfo) { */ function _civicrm_api3_validate_string(&$params, &$fieldName, &$fieldInfo, $entity, $action) { $isGet = substr($action, 0, 3) === 'get'; - list($fieldValue, $op) = _civicrm_api3_field_value_check($params, $fieldName, 'String'); + [$fieldValue, $op] = _civicrm_api3_field_value_check($params, $fieldName, 'String'); if (strpos($op, 'NULL') !== FALSE || strpos($op, 'EMPTY') !== FALSE || CRM_Utils_System::isNull($fieldValue)) { return; } diff --git a/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php b/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php index e89dc64557..8142ec86c8 100644 --- a/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php @@ -25,6 +25,11 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { */ protected $entity = 'Contribution'; + /** + * @var int + */ + protected $userJobID; + /** * Cleanup function. * @@ -32,6 +37,7 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { */ public function tearDown(): void { $this->quickCleanUpFinancialEntities(); + $this->quickCleanup(['civicrm_user_job', 'civicrm_queue', 'civicrm_queue_item'], TRUE); OptionValue::delete()->addWhere('name', '=', 'random')->execute(); parent::tearDown(); } @@ -63,22 +69,19 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { ]; $contact1Id = $this->individualCreate($contact1Params); $contact2Id = $this->individualCreate($contact2Params); - $values = [ - 'total_amount' => $this->formatMoneyInput(1230.99), - 'financial_type_id' => 'Donation', - 'external_identifier' => 'ext-1', - 'soft_credit' => 'ext-2', - ]; + $mapping = [ ['name' => 'total_amount'], + ['name' => 'receive_date'], ['name' => 'financial_type_id'], ['name' => 'external_identifier'], ['name' => 'soft_credit', 'soft_credit_type_id' => 1, 'soft_credit_match_field' => 'external_identifier'], ]; - $this->runImport($values, CRM_Import_Parser::DUPLICATE_UPDATE, CRM_Contribute_Import_Parser_Contribution::SOFT_CREDIT, $mapping); + $this->importCSV('contributions_amount_validate.csv', $mapping); $contributionsOfMainContact = Contribution::get()->addWhere('contact_id', '=', $contact1Id)->execute(); - $this->assertCount(1, $contributionsOfMainContact, 'Contribution not added for primary contact'); + // Although there are 2 rows in the csv, 1 should fail each time due to conflicting money formats. + $this->assertCount(1, $contributionsOfMainContact, 'Wrong number of contributions imported'); $this->assertEquals(1230.99, $contributionsOfMainContact->first()['total_amount']); $this->assertEquals(1230.99, $contributionsOfMainContact->first()['net_amount']); $this->assertEquals(0, $contributionsOfMainContact->first()['fee_amount']); @@ -87,23 +90,6 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { $this->assertCount(1, $contributionsOfSoftContact, 'Contribution Soft not added for primary contact'); } - /** - * Test dates are parsed - */ - public function testParsedDates(): void { - $mapperKeys = []; - $form = new CRM_Contribute_Import_Parser_Contribution($mapperKeys); - $params = ['receive_date' => '20/10/2019']; - CRM_Core_Session::singleton()->set('dateTypes', 32); - $form->formatDateFields($params); - $this->assertEquals('20191020', $params['receive_date']); - - $params = ['receive_date' => '20/10/2019']; - CRM_Core_Session::singleton()->set('dateTypes', 32); - $form->formatInput($params); - $this->assertEquals('20191020', $params['receive_date']); - } - /** * Test payment types are passed. * @@ -157,22 +143,18 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { */ public function testParsedCustomDates(): void { $this->createCustomGroupWithFieldOfType([], 'date'); - $mapperKeys = []; - $form = new CRM_Contribute_Import_Parser_Contribution($mapperKeys); - $params = [$this->getCustomFieldName('date') => '20/10/2019']; - CRM_Core_Session::singleton()->set('dateTypes', 32); - $formatted = []; - $form->formatInput($params, $formatted); - // @todo I feel like we should work towards this actually parsing $params here - - // & dropping formatting but - // per https://github.com/civicrm/civicrm-core/pull/14986 for now $formatted is parsing - // The issue I hit was that when I tried to extend to checking they were correctly imported - // I was not actually sure what correct behaviour was for what dates were accepted since - // on one hand the custom fields have a date format & on the other there is an input format & - // it seems to ignore the latter in favour of the former - which seems wrong. - $this->assertEquals('20191020000000', $formatted[$this->getCustomFieldName('date')]); - $this->callAPISuccess('CustomField', 'delete', ['id' => $this->ids['CustomField']['date']]); - $this->callAPISuccess('CustomGroup', 'delete', ['id' => $this->ids['CustomGroup']['Custom Group']]); + $this->individualCreate(['external_identifier' => 'ext-1']); + $mapping = [ + ['name' => 'total_amount'], + ['name' => 'receive_date'], + ['name' => 'financial_type_id'], + ['name' => 'external_identifier'], + ['name' => $this->getCustomFieldName('date')], + ]; + $this->importCSV('contributions_date_validate.csv', $mapping, ['dateFormats' => 32]); + $contribution = $this->callAPISuccessGetSingle('Contribution', []); + $this->assertEquals('2019-10-26 00:00:00', $contribution['receive_date']); + $this->assertEquals('2019-10-20 00:00:00', $contribution[$this->getCustomFieldName('date')]); } public function testParsedCustomOption(): void { @@ -192,8 +174,47 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { $this->callAPISuccess('CustomGroup', 'delete', ['id' => $this->ids['CustomGroup']['Custom Group']]); } + /** + * Import the csv file values. + * + * This function uses a flow that mimics the UI flow. + * + * @param string $csv Name of csv file. + * @param array $fieldMappings + * @param array $submittedValues + */ + protected function importCSV(string $csv, array $fieldMappings, array $submittedValues = []): void { + $submittedValues = array_merge([ + 'uploadFile' => ['name' => __DIR__ . '/data/' . $csv], + 'skipColumnHeader' => TRUE, + 'fieldSeparator' => ',', + 'contactType' => CRM_Import_Parser::CONTACT_INDIVIDUAL, + 'mapper' => $this->getMapperFromFieldMappings($fieldMappings), + 'dataSource' => 'CRM_Import_DataSource_CSV', + 'file' => ['name' => $csv], + 'dateFormats' => CRM_Core_Form_Date::DATE_yyyy_mm_dd, + 'onDuplicate' => CRM_Import_Parser::DUPLICATE_UPDATE, + 'groups' => [], + ], $submittedValues); + $form = $this->getFormObject('CRM_Contribute_Import_Form_DataSource', $submittedValues); + $form->buildForm(); + $form->postProcess(); + $this->userJobID = $form->getUserJobID(); + $form = $this->getFormObject('CRM_Contribute_Import_Form_MapField', $submittedValues); + $form->setUserJobID($this->userJobID); + $form->buildForm(); + $form->postProcess(); + /* @var CRM_Contribute_Import_Form_MapField $form */ + $form = $this->getFormObject('CRM_Contribute_Import_Form_Preview', $submittedValues); + $form->setUserJobID($this->userJobID); + $form->buildForm(); + $form->postProcess(); + } + /** * Test phone is included if it is part of dedupe rule. + * + * @throws \API_Exception */ public function testPhoneMatchOnContact(): void { // Update existing unsupervised rule, change to general. @@ -223,6 +244,12 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { ]); $fields = CRM_Contribute_BAO_Contribution::importableFields(); $this->assertArrayHasKey('phone', $fields); + $this->callApiSuccess('RuleGroup', 'create', [ + 'id' => $unsupervisedRuleGroup['id'], + 'used' => 'Unsupervised', + ]); + Civi\Api4\DedupeRule::delete()->addWhere('dedupe_rule_group_id', '=', $ruleGroup['id'])->execute(); + Civi\Api4\DedupeRuleGroup::delete()->addWhere('id', '=', $ruleGroup['id'])->execute(); } /** @@ -249,6 +276,23 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { } + /** + * Test the full form-flow import. + */ + public function testImport() :void { + $this->importCSV('contributions.csv', [ + ['name' => 'first_name'], + ['name' => 'total_amount'], + ['name' => 'receive_date'], + ['name' => 'financial_type_id'], + ['name' => 'email'], + ]); + $dataSource = new CRM_Import_DataSource_CSV($this->userJobID); + $row = $dataSource->getRow(); + $this->assertEquals('ERROR', $row['_status']); + $this->assertEquals('No matching Contact found for (mum@example.com )', $row['_status_message']); + } + /** * Run the import parser. * @@ -264,16 +308,8 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { if (!$fields) { $fields = array_keys($originalValues); } - $mapper = []; if ($mappings) { - foreach ($mappings as $mapping) { - $fieldInput = [$mapping['name']]; - if (!empty($mapping['soft_credit_type_id'])) { - $fieldInput[1] = $mapping['soft_credit_match_field']; - $fieldInput[2] = $mapping['soft_credit_type_id']; - } - $mapper[] = $fieldInput; - } + $mapper = $this->getMapperFromFieldMappings($mappings); } else { foreach ($fields as $field) { @@ -339,4 +375,22 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase { ]); } + /** + * @param array $mappings + * + * @return array + */ + protected function getMapperFromFieldMappings(array $mappings): array { + $mapper = []; + foreach ($mappings as $mapping) { + $fieldInput = [$mapping['name']]; + if (!empty($mapping['soft_credit_type_id'])) { + $fieldInput[1] = $mapping['soft_credit_match_field']; + $fieldInput[2] = $mapping['soft_credit_type_id']; + } + $mapper[] = $fieldInput; + } + return $mapper; + } + } diff --git a/tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_amount_validate.csv b/tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_amount_validate.csv new file mode 100644 index 0000000000..a940169109 --- /dev/null +++ b/tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_amount_validate.csv @@ -0,0 +1,3 @@ +Total Amount,Receive Date,Financial Type,External identifier,Soft Credit Ext ID +"1,230.99",2008-09-20,Donation,ext-1,ext-2 +"1.230,99",2008-09-20,Donation,ext-1,ext-2 diff --git a/tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_date_validate.csv b/tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_date_validate.csv new file mode 100644 index 0000000000..1df2fd157e --- /dev/null +++ b/tests/phpunit/CRM/Contribute/Import/Parser/data/contributions_date_validate.csv @@ -0,0 +1,2 @@ +Total Amount,Receive Date,Financial Type,External identifier,Custom date +500,26/10/2019,Donation,ext-1,20/10/2019 diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 4a29682d3d..2bbbd7d67e 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -3231,6 +3231,18 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { $_SESSION['_' . $form->controller->_name . '_container']['values']['Preview'] = $formValues; return $form; + case 'CRM_Contribute_Import_Form_DataSource': + case 'CRM_Contribute_Import_Form_MapField': + case 'CRM_Contribute_Import_Form_Preview': + $form->controller = new CRM_Contribute_Import_Controller(); + $form->controller->setStateMachine(new CRM_Core_StateMachine($form->controller)); + // The submitted values should be set on one or the other of the forms in the flow. + // For test simplicity we set on all rather than figuring out which ones go where.... + $_SESSION['_' . $form->controller->_name . '_container']['values']['DataSource'] = $formValues; + $_SESSION['_' . $form->controller->_name . '_container']['values']['MapField'] = $formValues; + $_SESSION['_' . $form->controller->_name . '_container']['values']['Preview'] = $formValues; + return $form; + case strpos($class, '_Form_') !== FALSE: $form->controller = new CRM_Core_Controller_Simple($class, $pageName); break; -- 2.25.1