dev/core#1230 [Dedupe] Do not geocode while merging, rely on existing values

[eileenmcnaughton]

Overview

This is a performance fix when merging - don't do geocoding when merging - we already have
geocode data for the contacts. We also move the 'whole address' so shouldn't re-geocode for more info

https://issues.civicrm.org/jira/browse/CRM-21786

Before

Geocoding calls happen during merging

After

No calls (existing geocode info in use)

Technical Details

Comments

@pfigel @lcdservices

[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).

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[demeritcowboy]

I can do an r-run just an initial comment: I'm not familiar with where else the skip parameter can be set or if this code can be run outside the UI but even if it's not possible right now for the skip parameter to have been previously set and even if CRM_Utils_Address::format() and anything it might call doesn't use it currently and even though there is no other code in that block currently, it might be more future proof to save the old value and replace after?

$old_skip_geocode = NULL;
if (isset($value['skip_geocode'])) {
  $old_skip_geocode = $value['skip_geocode'];
}
$value['skip_geocode'] = TRUE;
CRM_Core_BAO_Address::fixAddress($value);
if (isset($old_skip_geocode)) {
  $value['skip_geocode'] = $old_skip_geocode;
} else {
  unset($value['skip_geocode']);
}

Also to address the other comment clearly fixAddress() fixes the address (grin). There's even a comment in there explaining that because of "network latency" sometimes non-numeric string values manage to get in there so they need to be ignored... Ahh good old 2007.

[demeritcowboy]

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • I didn't do a before and after speed test on a lot of contacts but as to whether it seems to all work it looks good.
    • There's a situation I did test where if the winner contact has an ungeocoded address and the loser contact has the same address but it is geocoded and you choose not to overwrite (because they seem the same on the merge screen and the default is don't overwrite) then the winning result isn't geocoded either. But that seems picky and is easily manually or batch-update fixable.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] N/A
  • [r-test] PASS

[eileenmcnaughton]

Merging per review.

In response to comment one $value is really a throw-away parameter so resetting it is really just for clarity

On the edge case - I think if some addresses are not geocoding them that is a system issue & we shouldn't slow down deduping for it

[demeritcowboy]

It's currently a throwaway parameter, but in the future there might be more code in that block, or format() might end up using it or passing it on to something that does. But I don't see it as a blocker.

[eileenmcnaughton]

Please please we want less not more code there....