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

core#2309: Validate weight and weight threshold #19604

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

monishdeb
Copy link
Member

Overview

CiviCRM Find and Merge Duplicate Contacts, does not validate the weight and weight threshold, possible to set a weight threshold which can never be achieved. Resulting in a defunct duplicate matching rule and lots of duplicate contacts.

Before

Doesn't Validate the field weight(s) with weight threshold:

After

Validate the field weight(s) with weight threshold
Screenshot 2021-02-15 at 9 37 04 PM

Comments

ping @eileenmcnaughton @jusfreeman

@civibot civibot bot added the master label Feb 15, 2021
@civibot
Copy link

civibot bot commented Feb 15, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

The patch works - the wording on the error message is not quite right - how about 'The field weights must add up to, or exceed, the threshold weight' -

@petednz
Copy link

petednz commented Feb 15, 2021

Good to see this getting a fix

@agileware-justin
Copy link
Contributor

agileware-justin commented Feb 15, 2021

Thanks for the PR @monishdeb

Two suggestions:

  1. Change the logic for weight total and weight threshold to be >= (greater than or equal too), as it's currently equal - which is not required for a valid de-dupe rule, as shown in the screenshot below.
  2. Change wording to be "Total weight must be greater than or equal to the Weight Threshold" - use the same wording of "Weight Threshold" to reinforce which field is being referred too.

Screenshot_20210216_085925

@monishdeb
Copy link
Member Author

@agileware-justin @eileenmcnaughton thanks for your feedback, updated the PR

Isn't the logic should be

IF Total Weight < Threshold weight 
   then 'Total weight must be greater than or equal to the Weight Threshold'

as per the error message?

@eileenmcnaughton
Copy link
Contributor

@jusfreeman this looks right to me now - please confirm & I can merge-on-pass it

@agileware-justin
Copy link
Contributor

Thanks @monishdeb - yes, looks good to me now.

@eileenmcnaughton eileenmcnaughton merged commit 903f48b into civicrm:master Feb 16, 2021
@eileenmcnaughton
Copy link
Contributor

@monishdeb it doesn't look like the final merged version of this was actually right - see #19952

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.

5 participants