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

Clarify dedupe rules as "Name or Email" #26673

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented Jun 27, 2023

Overview

All of the default Organizational and Household dedupe rules are named "Name and Email", but they actually match on Name or Email, so this is misleading.

Before

Organizational and Household dedupe rules named "Name and Email".

After

Organizational and Household dedupe rules named "Name or Email".

Comments

I can't see that it would make sense to try to change these on existing installs, as they may have been changed, so we'd have to check that the rule group exists with the "Name and Email" name, etc, plus that it has the specific rules. I think better to leave it.

@civibot
Copy link

civibot bot commented Jun 27, 2023

(Standard links)

@civibot civibot bot added the master label Jun 27, 2023
@larssandergreen larssandergreen changed the title Clarify dedupe rules as "Name or Email" and cleanup Clarify dedupe rules as "Name or Email" Jun 27, 2023
@agileware-justin
Copy link
Contributor

agileware-justin commented Jun 29, 2023

@larssandergreen yes, good PR 👏

Would you mind fixing this useless performance hit for the Individual rule: Name and Address (reserved)

See how middle_name and suffix_id are part of the criteria but have a weight of 1? That's completely useless and unnecessarily includes searching middle_name and suffix_id for no reason I can understand. Seems like this would be a good time to remove both these fields.

INSERT INTO civicrm_dedupe_rule_group (contact_type, threshold, used, name, title, is_reserved)
VALUES ('Individual', 15, 'General', 'IndividualGeneral', 'Name and Address (reserved)', 1);

SELECT @drgid := MAX(id) FROM civicrm_dedupe_rule_group;
INSERT INTO civicrm_dedupe_rule (dedupe_rule_group_id, rule_table, rule_field, rule_weight)
VALUES (@drgid, 'civicrm_contact', 'first_name',     '5'),
       (@drgid, 'civicrm_contact', 'last_name',      '5'),
       (@drgid, 'civicrm_address', 'street_address', '5'),
       (@drgid, 'civicrm_contact', 'middle_name',    '1'),
       (@drgid, 'civicrm_contact', 'suffix_id',      '1');

And then there is this one, Individual Name and Email (reserved).

last_name has a weighting of 7, why? Just make it 5. Only needs to add up to 20. Pretty weird weighting IMHO.

INSERT INTO civicrm_dedupe_rule_group (contact_type, threshold, used, name, title, is_reserved)
VALUES ('Individual', 20, 'Supervised', 'IndividualSupervised', 'Name and Email (reserved)', 1);

SELECT @drgid := MAX(id) FROM civicrm_dedupe_rule_group;
INSERT INTO civicrm_dedupe_rule (dedupe_rule_group_id, rule_table, rule_field, rule_weight)
VALUES (@drgid, 'civicrm_contact', 'first_name', 5),
       (@drgid, 'civicrm_contact', 'last_name',  7),
       (@drgid, 'civicrm_email'  , 'email',     10);

@larssandergreen
Copy link
Contributor Author

@agileware-justin I noticed the same thing. I actually did fix those in an earlier version of this PR. But then I dug a little deeper and found there's a bit more to it because there's also a hard-coded query for IndividualGeneral that will need to be changed, so I pulled those changes out and posted an issue to determine what exactly that rule should do. If you drop a comment over there, that would be good to get the ball rolling.

@demeritcowboy demeritcowboy added merge ready PR will be merged after a few days if there are no objections and removed merge ready PR will be merged after a few days if there are no objections labels Jun 29, 2023
@demeritcowboy demeritcowboy merged commit 455bb93 into civicrm:master Jun 29, 2023
@demeritcowboy
Copy link
Contributor

I was going to put merge-ready because for household I'm not sure such a rule even makes sense for unsupervised, but it's clearly true and this will make it clearer that maybe it doesn't make sense.

@larssandergreen
Copy link
Contributor Author

@demeritcowboy Agreed, it don't think it makes much sense for households. We don't use them so I didn't really think about it, but now that you mention it, it's probably worth fixing while we're here. I don't think it should match on name alone, but it probably should match on email alone, so maybe just email makes sense.

@agileware-justin
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants