From 19f33b0958e24b4ba68bc07d653a11aa6ca59bb4 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Wed, 18 May 2022 14:39:49 +1200 Subject: [PATCH] Cleanup gender handling in contact import --- CRM/Contact/BAO/Contact.php | 1 - CRM/Contact/Import/Parser/Contact.php | 90 ++++++------------- CRM/Import/Parser.php | 69 ++++++++++++++ tests/phpunit/CRM/Contact/BAO/ContactTest.php | 5 +- .../Import/Form/data/individual_genders.csv | 17 ++++ .../CRM/Contact/Import/Parser/ContactTest.php | 89 +++++++++++++++--- 6 files changed, 190 insertions(+), 81 deletions(-) create mode 100644 tests/phpunit/CRM/Contact/Import/Form/data/individual_genders.csv diff --git a/CRM/Contact/BAO/Contact.php b/CRM/Contact/BAO/Contact.php index ff00b9532b..0d586e08ad 100644 --- a/CRM/Contact/BAO/Contact.php +++ b/CRM/Contact/BAO/Contact.php @@ -781,7 +781,6 @@ WHERE civicrm_contact.id = " . CRM_Utils_Type::escape($id, 'Integer'); CRM_Utils_Array::lookupValue($defaults, 'prefix', CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'prefix_id'), $reverse); CRM_Utils_Array::lookupValue($defaults, 'suffix', CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'suffix_id'), $reverse); - CRM_Utils_Array::lookupValue($defaults, 'gender', CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'gender_id'), $reverse); CRM_Utils_Array::lookupValue($defaults, 'communication_style', CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'communication_style_id'), $reverse); //lookup value of email/postal greeting, addressee, CRM-4575 diff --git a/CRM/Contact/Import/Parser/Contact.php b/CRM/Contact/Import/Parser/Contact.php index e7b8c394f7..a78ab04653 100644 --- a/CRM/Contact/Import/Parser/Contact.php +++ b/CRM/Contact/Import/Parser/Contact.php @@ -1305,41 +1305,6 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { } } - /** - * Check if value present in all genders or. - * as a substring of any gender value, if yes than return corresponding gender. - * eg value might be m/M, ma/MA, mal/MAL, male return 'Male' - * but if value is 'maleabc' than return false - * - * @param string $gender - * Check this value across gender values. - * - * retunr gender value / false - * - * @return bool - */ - public function checkGender($gender) { - $gender = trim($gender, '.'); - if (!$gender) { - return FALSE; - } - - $allGenders = CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'gender_id'); - foreach ($allGenders as $key => $value) { - if (strlen($gender) > strlen($value)) { - continue; - } - if ($gender == $value) { - return $value; - } - if (substr_compare($value, $gender, 0, strlen($gender), TRUE) === 0) { - return $value; - } - } - - return FALSE; - } - /** * Check if an error in Core( non-custom fields ) field * @@ -1350,6 +1315,9 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { public function isErrorInCoreData($params, &$errorMessage) { $errors = []; foreach ($params as $key => $value) { + if ($value === 'invalid_import_value') { + $errors[] = $this->getFieldMetadata($key)['title']; + } if ($value) { $session = CRM_Core_Session::singleton(); $dateType = $session->get("dateTypes"); @@ -1383,12 +1351,6 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { } break; - case 'gender_id': - if (!self::checkGender($value)) { - $errors[] = ts('Gender'); - } - break; - case 'preferred_communication_method': $preffComm = []; $preffComm = explode(',', $value); @@ -2323,23 +2285,14 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { $locationFields = ['location_type_id', 'phone_type_id', 'provider_id', 'website_type_id']; $locationValues = array_filter(array_intersect_key($mappedField, array_fill_keys($locationFields, 1))); - $contactArray = &$params; if ($relatedContactKey) { if (!isset($params[$relatedContactKey])) { $params[$relatedContactKey] = ['contact_type' => $this->getRelatedContactType($mappedField['relationship_type_id'], $mappedField['relationship_direction'])]; } - $contactArray = &$params[$relatedContactKey]; - } - - if (!empty($locationValues)) { - $locationValues[$fieldName] = $importedValue; - $contactArray[$fieldName] = (array) ($contactArray[$fieldName] ?? []); - $contactArray[$fieldName][] = $locationValues; + $this->addFieldToParams($params[$relatedContactKey], $locationValues, $fieldName, $importedValue); } else { - // @todo - this is really the best point to convert labels - // to values. - $contactArray[$fieldName] = $importedValue; + $this->addFieldToParams($params, $locationValues, $fieldName, $importedValue); } } @@ -2541,17 +2494,6 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { return TRUE; } - if (isset($values['gender'])) { - if (!empty($params['gender_id'])) { - $genders = CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'gender_id'); - $params['gender'] = $genders[$params['gender_id']]; - } - else { - $params['gender'] = $values['gender']; - } - return TRUE; - } - if (!empty($values['preferred_communication_method'])) { $comm = []; $pcm = array_change_key_case(array_flip(CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'preferred_communication_method')), CASE_LOWER); @@ -3055,4 +2997,26 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { return Civi::$statics[__CLASS__][$cacheKey]; } + /** + * Add the given field to the contact array. + * + * @param array $contactArray + * @param array $locationValues + * @param string $fieldName + * @param mixed $importedValue + * + * @return void + * @throws \API_Exception + */ + private function addFieldToParams(array &$contactArray, array $locationValues, string $fieldName, $importedValue): void { + if (!empty($locationValues)) { + $locationValues[$fieldName] = $importedValue; + $contactArray[$fieldName] = (array) ($contactArray[$fieldName] ?? []); + $contactArray[$fieldName][] = $locationValues; + } + else { + $contactArray[$fieldName] = $this->getTransformedFieldValue($fieldName, $importedValue); + } + } + } diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php index d22959a4fc..083a0140ed 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -1175,4 +1175,73 @@ abstract class CRM_Import_Parser { throw new CRM_Core_Exception(($prefixString ? ($prefixString . ' ') : '') . ts('Missing required fields:') . ' ' . implode(' ' . ts('OR') . ' ', $missingFields)); } + /** + * Get the field value, transformed by metadata. + * + * @param string $fieldName + * @param string|int $importedValue + * Value as it came in from the datasource. + * + * @return string|array|bool|int + * @throws \API_Exception + */ + protected function getTransformedFieldValue(string $fieldName, $importedValue) { + // For now only do gender_id as we need to work through removing duplicate handling + if ($fieldName !== 'gender_id' || empty($importedValue)) { + return $importedValue; + } + return $this->getFieldOptions($fieldName)[$importedValue] ?? 'invalid_import_value'; + } + + /** + * @param string $fieldName + * + * @return false|array + * + * @throws \API_Exception + */ + protected function getFieldOptions(string $fieldName) { + return $this->getFieldMetadata($fieldName, TRUE)['options']; + } + + /** + * Get the metadata for the field. + * + * @param string $fieldName + * @param bool $loadOptions + * + * @return array + * @throws \API_Exception + */ + protected function getFieldMetadata(string $fieldName, bool $loadOptions = FALSE): array { + $fieldMetadata = $this->getImportableFieldsMetadata()[$fieldName]; + if ($loadOptions && !isset($fieldMetadata['options'])) { + if (empty($fieldMetadata['pseudoconstant'])) { + $this->importableFieldsMetadata[$fieldName]['options'] = FALSE; + } + else { + $options = civicrm_api4($fieldMetadata['entity'], 'getFields', [ + 'loadOptions' => ['id', 'name', 'label'], + 'where' => [['name', '=', $fieldMetadata['name']]], + 'select' => ['options'], + ])->first()['options']; + // We create an array of the possible variants - notably including + // name AND label as either might be used, and capitalisation variants. + $values = []; + foreach ($options as $option) { + $values[$option['id']] = $option['id']; + $values[$option['label']] = $option['id']; + $values[$option['name']] = $option['id']; + $values[strtoupper($option['name'])] = $option['id']; + $values[strtolower($option['name'])] = $option['id']; + $values[strtoupper($option['label'])] = $option['id']; + $values[strtolower($option['label'])] = $option['id']; + } + $this->importableFieldsMetadata[$fieldName]['options'] = $values; + } + return $this->importableFieldsMetadata[$fieldName]; + } + return $fieldMetadata; + } + } diff --git a/tests/phpunit/CRM/Contact/BAO/ContactTest.php b/tests/phpunit/CRM/Contact/BAO/ContactTest.php index a95cad2018..9598a8d72e 100644 --- a/tests/phpunit/CRM/Contact/BAO/ContactTest.php +++ b/tests/phpunit/CRM/Contact/BAO/ContactTest.php @@ -495,13 +495,14 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { /** * Test case for resolveDefaults( ). * + * @todo the resolveDefaults function is on it's way out - so is this test... + * * Test all pseudoConstant, stateProvince, country. */ public function testResolveDefaults() { $params = [ 'prefix_id' => 3, 'suffix_id' => 2, - 'gender_id' => 2, 'birth_date' => '1983-12-13', ]; @@ -515,8 +516,6 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { CRM_Contact_BAO_Contact::resolveDefaults($params); //check the resolve values. - $genders = CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'gender_id'); - $this->assertEquals($genders[$params['gender_id']], $params['gender'], 'Check for gender.'); $prefix = CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'prefix_id'); $this->assertEquals($prefix[$params['prefix_id']], $params['prefix'], 'Check for prefix.'); $suffix = CRM_Core_PseudoConstant::get('CRM_Contact_DAO_Contact', 'suffix_id'); diff --git a/tests/phpunit/CRM/Contact/Import/Form/data/individual_genders.csv b/tests/phpunit/CRM/Contact/Import/Form/data/individual_genders.csv new file mode 100644 index 0000000000..a92d1015cf --- /dev/null +++ b/tests/phpunit/CRM/Contact/Import/Form/data/individual_genders.csv @@ -0,0 +1,17 @@ +first_name,Last Name,Gender,mum_first_name,mum_last_name,mum_gender,Expected +Madame,1,Female,Mum,1,,Valid +Madame,2,FEMALE,Mum,2,,Valid +Madame,3,female,Mum,3,,Valid +Madame,4,Female.,Mum,4,,Invalid +Madame,5,F,Mum,5,,Invalid +Madame,6,f,Mum,6,,Invalid +Madame,7,Cat,Mum,7,,Invalid +Madame,8,1,Mum,8,,Valid +Daughter,9,,Madame,9,Female,Valid +Daughter,10,,Madame,10,FEMALE,Valid +Daughter,11,,Madame,11,female,Valid +Daughter,12,,Madame,12,Female.,Invalid +Daughter,13,,Madame,13,F,Invalid +Daughter,14,,Madame,14,f,Invalid +Daughter,15,,Madame,15,Cat,Invalid +Daughter,16,,Madame,16,1,Valid diff --git a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php index 462a32357f..75826049d6 100644 --- a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php +++ b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php @@ -14,6 +14,7 @@ * File for the CRM_Contact_Imports_Parser_ContactTest class. */ +use Civi\Api4\Contact; use Civi\Api4\UserJob; /** @@ -882,6 +883,52 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { ]; } + /** + * Test the handling of validation when importing genders. + * + * If it's not gonna import it should fail at the validation stage... + * + * @throws \API_Exception + * @throws \CRM_Core_Exception + */ + public function testImportGenders(): void { + $mapper = [ + ['first_name'], + ['last_name'], + ['gender_id'], + ['1_a_b', 'first_name'], + ['1_a_b', 'last_name'], + ['1_a_b', 'gender_id'], + ['do_not_import'], + ]; + $csv = 'individual_genders.csv'; + /* @var CRM_Import_DataSource_CSV $dataSource */ + /* @var \CRM_Contact_Import_Parser_Contact $parser */ + [$dataSource, $parser] = $this->getDataSourceAndParser($csv, $mapper, []); + while ($values = $dataSource->getRow()) { + try { + $parser->validateValues(array_values($values)); + if ($values['expected'] !== 'Valid') { + $this->fail($values['gender'] . ' should not have been valid'); + } + } + catch (CRM_Core_Exception $e) { + if ($values['expected'] !== 'Invalid') { + $this->fail($values['gender'] . ' should have been valid'); + } + } + } + + $this->importCSV($csv, $mapper); + $contacts = Contact::get() + ->addWhere('first_name', '=', 'Madame') + ->addSelect('gender_id:name')->execute(); + foreach ($contacts as $contact) { + $this->assertEquals('Female', $contact['gender_id:name']); + } + $this->assertCount(8, $contacts); + } + /** * Test that setting duplicate action to fill doesn't blow away data * that exists, but does fill in where it's empty. @@ -1108,6 +1155,33 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { $this->assertEquals($expectedResult, $parser->import($onDuplicateAction, $values), 'Return code from parser import was not as expected'); } + /** + * @param string $csv + * @param array $mapper + * @param array $submittedValues + * + * @return array + * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException + */ + protected function getDataSourceAndParser(string $csv, array $mapper, array $submittedValues): array { + $userJobID = $this->getUserJobID(array_merge([ + 'uploadFile' => ['name' => __DIR__ . '/../Form/data/' . $csv], + 'skipColumnHeader' => TRUE, + 'fieldSeparator' => ',', + 'onDuplicate' => CRM_Import_Parser::DUPLICATE_SKIP, + 'contactType' => CRM_Import_Parser::CONTACT_INDIVIDUAL, + 'mapper' => $mapper, + 'dataSource' => 'CRM_Import_DataSource_CSV', + ], $submittedValues)); + + $dataSource = new CRM_Import_DataSource_CSV($userJobID); + $parser = new CRM_Contact_Import_Parser_Contact(); + $parser->setUserJobID($userJobID); + $parser->init(); + return [$dataSource, $parser]; + } + /** * @param array $fields Array of fields to be imported * @param array $allfields Array of all fields which can be part of import @@ -1272,20 +1346,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \CRM_Core_Exception */ protected function validateCSV(string $csv, array $mapper, $submittedValues): void { - $userJobID = $this->getUserJobID(array_merge([ - 'uploadFile' => ['name' => __DIR__ . '/../Form/data/' . $csv], - 'skipColumnHeader' => TRUE, - 'fieldSeparator' => ',', - 'onDuplicate' => CRM_Import_Parser::DUPLICATE_SKIP, - 'contactType' => CRM_Import_Parser::CONTACT_INDIVIDUAL, - 'mapper' => $mapper, - 'dataSource' => 'CRM_Import_DataSource_CSV', - ], $submittedValues)); - - $dataSource = new CRM_Import_DataSource_CSV($userJobID); - $parser = new CRM_Contact_Import_Parser_Contact(); - $parser->setUserJobID($userJobID); - $parser->init(); + [$dataSource, $parser] = $this->getDataSourceAndParser($csv, $mapper, $submittedValues); $parser->validateValues(array_values($dataSource->getRow())); } -- 2.25.1