dev/core#1438 Enable matching on contact phone when importing contributions

[jmcclelland]

Overview

https://lab.civicrm.org/dev/core/issues/1438

When importing contributions, you can't match an existing contact using a phone number, even when the default unsupervised rule specified the phone field. It is because the dedupe rule calls the phone field "phone_numeric" yet the contact list of importable fields lists the field as "phone" - so no match is every made.

Before

When the field mapper screen appears and you have an supervised dedupe rule with a phone field, you get a missing index error, a phantom "match to contact" option in the field list, and the phone field is not listed.

After

The contribution import field lists the Phone number field and it properly matches.

Technical Details

I've implemented the change in the Dedupe matcher code so it adjusts to return phone instead of phone_numeric. I'm not sure what impact this might have on other code - so suggestions appreciated!

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

This is perfectly fine as-is but could be replaced with just $ruleFields[] = ($ruleBao->rule_field == 'phone_numeric') ? 'phone' : $ruleBao->rule_field;. Science studies have shown using ternary operators makes other people think you are taller.

[jmcclelland]

I know you are joking about being taller :) but I am curious about any studies of ternary operators. I find them hard to parse, whereas the code above clearly states the default (what we expect) and indicates the exception. This format also leaves room for additional exceptions to be easily added. I also know that this stuff is very subjective and I'm not sure if I'm the outlier or not!

[demeritcowboy]

@jmcclelland I can reproduce the issue but then with the patch when I go to import a file it won't import them and gives me a download errors file with "Multiple matching contact records detected for this row. The contribution was not imported", even when there is only one (or no match). Does it work for you? This was my test file:

Phone,Amount,Date,FinancialType
(555) 128-2233,100.00,2019-01-22,Donation
543-494-3848,12.33,2019-03-04,Donation

[seamuslee001]

@jmcclelland @demeritcowboy my feeling is that the better solution might be to try and clean up the import data to match against phone numeric perhaps?

[jmcclelland]

@jmcclelland @demeritcowboy my feeling is that the better solution might be to try and clean up the import data to match against phone numeric perhaps?

Unfortunately, that doesn't work (at least as is with the code). The problem is that no phone fields will ever appear in the field list on a contribution import (and if a phone number is in the unsupervised dedupe rule you get strange behavior because the field list is populated with an empty value).

The reason for the behavior is: we pull in a list of importable contact fields (which gives us a field called "phone") and we pull in a list of fields from the unsupervised rule (which gives us a field called phone_numeric). If they match, the field is properly included. But, of course, they don't match.

We could change the contact function's list of importable fields to offer phone_numeric, but then we are probably really going to be in trouble, since it's phone that we really want to import.

[jmcclelland]

@jmcclelland I can reproduce the issue but then with the patch when I go to import a file it won't import them and gives me a download errors file with "Multiple matching contact records detected for this row. The contribution was not imported", even when there is only one (or no match). Does it work for you? This was my test file:

Phone,Amount,Date,FinancialType
(555) 128-2233,100.00,2019-01-22,Donation
543-494-3848,12.33,2019-03-04,Donation
``'

Thanks for testing!

Strange - I first tried with a file containing the data you provided. And I got two errors (neither mathching contact was found). Then, I edited two existing records to add those phone numbers. I imported again and this time it was successful. I'm not sure why it is not working for you.

Here's my dedupe rule (is it possible your dedupe rule has a threshhold lower then the phone weight?):

[demeritcowboy]

I can't comment if the change is the right approach but for the record I did do some more tests including contact imports and nothing interesting went wrong. Also noted that while it imports phone as written in the file, it DOES do the dedupe comparison itself against phone_numeric, which does seem like it would be the desired way of matching.

By the way my problem before was that I had the length field set in the rule.

[jmcclelland]

Thanks @demeritcowboy for the additional testing - much appreciated. What do you think @seamuslee001 ?

[jaapjansma]

Betty and I are reviewing PR's and we came across this PR below our findings.

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR and in the issue.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: We have tested this PR with importing contributions whereby we match on the phone number of the contact. It all seemed to work.
• Developer standards
  • Technical impact (r-tech)
    • ISSUE: I see that the code in Rule.php and RuleGroup.php changes the phone_numeric field to phone. I am wondering why not give the user/site administrator the choice of using Phone or Phone Numeric as a field for dedupe rules. Any ideas on that? This seems to me a bit hackish...
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage
  • Test results (r-test)
    • PASS: The test results are all-clear.

We are not sure whether my comment on the technical impact is blocking for merging this as we do think it is separate issue from this one. So we would advice to merge this and probably create a separate PR/issue for the fact that one cannot chose between phone or phone numeric in the dedupe rules and in the import.
I have created an issue for the comment above: https://lab.civicrm.org/dev/core/issues/1600

@mattwire or @eileenmcnaughton are you able to merge this PR?