Dupe improve custom data handling

[eileenmcnaughton]

Overview

Reduce code complexity & locking queries when merging contacts

Before

When merging contacts and transferring custom data 3 locking queries run with 2 of them cancelling each other out

1. INSERT row - the purpose being to explicitly cause the failure of
2. UPDATE IGNORE
3. DELETE row

After

None of the above are called - the custom data transfer is already done elsewhere in the process.

Note this does mean a change in behaviour from 'custom data inconsistently deleted from deleted contact' to 'custom data not deleted' - see technical details for more

Technical Details

The current custom data handling code does the following

1. For normal single rows it first inserts a row. This has the impact of rendering the
   update that follows meaningless (this was an intentional change). It then deletes the row.
   Hence the upshot is simply that it deletes the row. A separate process transfers the custom
   data for the row. In other words we are engaging in 3 queries with a fairly high chance of
   causing deadlocks in order to just delete the row.

2. For single rows where the entity reference refers to the merged contact the row is
   updated to refer to the merged contact (without the insert this works) and a further unnecessary delete follows

3. For custom groups supporting multiple rows the rows are updated to have the new entity id. An unnecessary delete follows.

This change only affects the first of these. I would like to, in a future PR, change UPDATE IGNORE to just UPDATE &
remove the unnecessary delete - with more testing.

Note that this does include a slight change of behaviour. Currently if ANY fields in a custom group
are transferred from one contact to another during merge the row is deleted (with all the custom fields in it).
However, if no fields in a set are deleted then the row is not deleted.

This felt like it was a bit short on consistency. If has a potential advantage from a DB size point of view (any
deleting is better than none) but it also increases the number of locking queries in a process that is fairly
prone to cause DB locks. Based on these considerations I didn't think it worth re-adding code complexity to
retain inconsistent deletion.

A note on tests - I pre-added a bunch of tests into _api3_ContactTest to cover the 3 scenarios above.

Comments

@pfigel @lcdservices could you try to wrap you heads around this - it follows on from analysis that @lcdservices did in gitlab which I'll try to find & link now

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

[colemanw]

Ooh, nasty APIv3 gotcha. Good catch.

[eileenmcnaughton]

Test failure was related - it was an edge-case test wisely added by @pfigel - should pass now

[pfigel]

This looks like a nice improvement. Overall, merging is still quite convoluted, so it's hard to reason about, but based on r-run, existing test coverage and improved code quality, this should work. 👍

• General standards
  • (r-explain) PASS
  • (r-user) PASS: Practically no user-visible impact for merge functionality as such, other than the change in behaviour for deleted entities (which is very edge-casey).
  • (r-doc) PASS
  • (r-run) PASS: I focused on two things during r-run - no issues found:
    • General merge behaviour - do (custom) fields still get moved, do multi-row custom fields work, are contact references updated to point to the "surviving" contact
    • Some light integration testing with XDedupe
• Developer standards
  • (r-tech) PASS: No real contract changes. Minor change in behaviour for an edge-case that was already weird. Likely no impact on extensions.
  • (r-code) PASS
  • (r-maint) PASS
  • (r-test) PASS: Existing test coverage was fairly good & was improved in advance of this.