Skip to content
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

Expose sort_name as a dedupe-matchable field #13864

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 20, 2019

Overview

Allow sort_name to be used in dedupe rules

Before

sort_name not available

After

sort_name available, test

Technical Details

This opens up the option of us using a hook to do some standardisation - e.g getting rid of 'The'
in the sort name or perhaps trailing suffixes like 'Ltd' & it helping with both sorting and with deduping

Note that most of the fields available come from 'importable fields' - I thought about making this field 'importable' but it felt like there might be unknown consequences so I opted for just adding it in & securing it with a unit test (IMHO the test qualifies as the 'additional code improvement' required to justify it)

Comments

@civibot
Copy link

civibot bot commented Mar 20, 2019

(Standard links)

@civibot civibot bot added the master label Mar 20, 2019
@eileenmcnaughton eileenmcnaughton force-pushed the rule_group branch 2 times, most recently from 25a8bfc to 5238367 Compare March 20, 2019 05:39
@eileenmcnaughton
Copy link
Contributor Author

test this please

@@ -126,7 +127,7 @@ public static function &supportedFields($requestedType) {
}
}
CRM_Utils_Hook::dupeQuery(CRM_Core_DAO::$_nullObject, 'supportedFields', $fields);
return $fields[$requestedType];
return !empty($fields[$requestedType]) ? $fields[$requestedType] : [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enotice fix as we are adding a test which fails on enotice

@mattwire
Copy link
Contributor

@eileenmcnaughton I've reviewed and tested this. Tests are passing. But the hardcoded sort_name field is a bit odd - I can see why you've done it and the explanation justifies it - but can you put that justification in as a code comment before merging?

@eileenmcnaughton
Copy link
Contributor Author

@mattwire good point - you'll get me trained on code comments yet :-)

This opens up the option of us using a hook to do some standardisation - e.g getting rid of 'The'
in the sort name & it helping with both sorting and with deduping
@eileenmcnaughton
Copy link
Contributor Author

@mattwire all good?

@mattwire
Copy link
Contributor

Good to merge

@eileenmcnaughton eileenmcnaughton merged commit 3d2f7bc into civicrm:master Mar 27, 2019
@eileenmcnaughton eileenmcnaughton deleted the rule_group branch March 27, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants