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

dev/core#961 - Contribution page including 2 email fields does not re… #14252

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

jitendrapurohit
Copy link
Contributor

…spect dedupe rule.

Overview

Unsupervised rule fails if we have 2 email fields to match on with latter set as empty.

Before

To replicate -

  • Add a profile with first name, last name, billing email and work email.
  • Create a contact with first name = "Test", Last name = "Dedupe" and email = "test@example.com"
  • Add the profile to a contribution page.
  • Submit the contribution page anonymously and enter First name = Test, Last name = "Dedupe" and billing email = "test@example.com". Keep work email as empty.

image

  • Complete the payment.
  • The payment should be recorded against the existing contact based on dedupe matching.
  • As we do not have any value filled for second email field, dedupe fails and a new contact is created with same details.

After

Payment gets created on the existing contact. Dedupe match works correctly.

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/961

@civibot civibot bot added the master label May 15, 2019
@civibot
Copy link

civibot bot commented May 15, 2019

(Standard links)

@@ -281,6 +281,8 @@ public static function formatParams($fields, $ctype) {
if (preg_match('/(.*)-(Primary-[\d+])$|(.*)-(\d+|Primary)$/', $key, $matches)) {
$return = array_values(array_filter($matches));
$flat[$return[1]] = $value;
// make sure the first occurrence is kept, not the last
$flat[$return[1]] = !isset($flat[$return[1]]) ? $value : $flat[$return[1]];
Copy link
Contributor

Choose a reason for hiding this comment

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

@jitendrapurohit what happens if the first is empty & the second isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @eileenmcnaughton. On testing the above scenario, I noticed it was still breaking if only the second field has the value in it. Fixed it and also extended the unit test to assert this additional behavior.

@eileenmcnaughton
Copy link
Contributor

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

OK I stepped through this & it makes sense - there is STILL a logic flaw IMHO in that it will only check one of the passed in emails - without this patch it will check the last, regardless of whether it is empty or not. With this patch it will match the first non-empty one. I think the new approach is better but not 'quite right'. The unit test sells me on merging this as we will lock in fixing the tested scenario.

The patch feels low risk to me based on digging into the code. Having multiple of the same type of location in a form seems relatively uncommon & I think it's unlikely people would have built flows around the assumption that one is more 'important' than the other if they have

@eileenmcnaughton eileenmcnaughton merged commit 1316177 into civicrm:master Aug 2, 2019
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