-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup gender handling in contact import + add test #23484
Conversation
(Standard links)
|
if ($relatedContactKey) { | ||
if (!isset($params[$relatedContactKey])) { | ||
$params[$relatedContactKey] = ['contact_type' => $this->getRelatedContactType($mappedField['relationship_type_id'], $mappedField['relationship_direction'])]; | ||
} | ||
$contactArray = &$params[$relatedContactKey]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part is an extraction
4871122
to
46a3cd3
Compare
46a3cd3
to
19f33b0
Compare
@@ -781,7 +781,6 @@ public static function resolveDefaults(&$defaults, $reverse = FALSE) { | |||
|
|||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per function comments this deprecated function is only called from import - so can fully remove
Note that imports can use 'name' but this is not shown in the Gender Options display. The result is that if the label has been changed, an import file can contain gender values that work but are not obvious that they should work. This is not limited to gender: maybe option group displays should include 'name'. The previous behaviour did not reject 'F', 'M' etc but did not correctly set the gender. Allowing initials might be convenient for some imports but would require rules around handling labels with the same initial. The balance of flexibility and correctness in this PR seems good to me and requiring import data to be unambiguous is good. |
@aydun yes good point about the docs - although the goal is ti standardise the behaviour across many fields - at which point it is more meaningful to document |
@aydun thanks! Reviewing PRs is extremely helpful to the project. |
Overview
Cleanup gender handling in contact import + add test
This lays the foundation for option values to be transformed based on metadata in
getParams
, simplifying validation & later transformationsBefore
No testing on gender_id handling and unclear expectations.
On adding the test I discovered that there were 2 types invalid values for gender
Despite generous consideration in the validation routine only values that were an exact match for the label actually got saved to the database.
I concluded that if a value was not to be saved to the database it should fail validation - and hence the test checks both the validation and the actual import
After
The initial
getParams
transformation now converts the imported value to an integer, based on metadata.In line with import handling for other fields in various import places I made it accept any of id, name, label
I ALSO made it accept name & label in all upper or all lower case. This is something that was coded into
checkGender
but ultimately didn't work. The way I did it is not truly case-insensitive - I might think more on that but there is some risk with non-latin alphabets - at least the way I did it won't mess with a matching font sitll matching.Technical Details
Comments
Note that the test includes a csv file suitale for r-run testing