Dedupe fixes CRM-18442, CRM-18539, CRM-18480 #8251

eileenmcnaughton · 2016-04-27T05:38:19Z

This PR contains fixes for several merge issues & includes the changes in #8385 (still open in case John prefers to QA a smaller PR) and #8352 ( which I'm closing in favour of this).

Includes

CRM-18480 fix batch merge job to cope with new inability to re-merge (regression this QA cycle)
CRM-18539 CRM-18539: Permit criteria for batch merging (other than group) (new api feature)
CRM-18546 Duplicate email addresses created when merging in 4.7 (4.7 regression)
CRM-18442 Dedupe query: remove OR join in favour of more performant union (improvement in 4.7, note there are other code areas to expand this to -ie Org, Household, unsupervised queries).

eileenmcnaughton · 2016-05-11T03:40:35Z

test this please

eileenmcnaughton · 2016-05-18T00:31:32Z

test this please

eileenmcnaughton · 2016-05-18T05:45:33Z

test this please

JKingsnorth · 2016-05-18T10:52:57Z

Throwing an exception on trying to merge a deleted contact makes better sense. I was just recreating the logic here, which was silly of me:

civicrm-core/CRM/Dedupe/Finder.php

Line 182 in b6a6d15

CRM_Core_Error::fatal("contact id of $cid does not exist");

JKingsnorth · 2016-05-18T10:57:32Z

Jenkins retest this please

eileenmcnaughton · 2016-05-18T11:00:58Z

The tests have been pretty flakey today - but we can add the 'merge on pass' flag when you reach the point that you are happy with your QA & a passing test outcome is the remaining blocker

JKingsnorth · 2016-05-18T12:37:26Z

Warning: Missing argument 3 for CRM_Dedupe_Merger::getMergeCacheKeyString(), called in C:\xampp\htdocs\civi\sites\all\modules\civicrm\CRM\Contact\Page\DedupeFind.php on line 148

and I'm running through the manual tests now.

JKingsnorth · 2016-05-18T13:38:37Z

In fact this is being a bit hit and miss. I'm now hitting a huge number of e-notices, spotted in the drupal watchdog logs, about the AJAX dedupe finder page, and no duplicates are being detected:
Notice: Undefined variable: whereClause in CRM_Contact_Page_AJAX::getDedupes() (line 724 of C:\xampp\htdocs\civi\sites\all\modules\civicrm\CRM\Contact\Page\AJAX.php).

All similar to the above.

This is caused by the CRM-18539 commit.

I'm also running into an intermittent issue:
Use a dedupe rule no filtered by group - 90 results
Go back to 'Find and merge'
Use the dedupe rule filtered by a group - 0 results (expected 40)

But I've not been able to reliably recreate it against any particular commit yet =[

Grief. There's another issue at play here too regarding the prevNextCache not being updated and the duplicate finder not actually reflecting the most up to date information from peoples' records.

JKingsnorth · 2016-05-18T14:34:39Z

I've tested commit 664d553 (CRM-18546 Duplicate email addresses created when merging in 4.7) in isolation and it appears to work OK.

Tested safe merging contacts with duplicate email addresses - no duplicate email created (other fields migrated as expected)
Tested safe merging contacts with duplicate email address and conflicting addresses - caught as a conflict
Tested force merging those contacts - no duplicate email address added
Tested safe merging contacts with duplicate phones - no duplicate phone created either - yay
Tested a 'manual' merge and the add/overwrite options for location fields still work as expected

So I think that commit, in isolation, is good.

I wondered whether we should extend the test to check for duplicate phone numbers too - but I don't think it's necessary.

JKingsnorth · 2016-05-18T15:54:02Z

Also identified https://issues.civicrm.org/jira/browse/CRM-18575 as part of QA on this.

eileenmcnaughton · 2016-05-18T20:07:26Z

@JKingsnorth the commit you tried suucessfully isolated in this PR #8385 so if that is ok we can merge that PR.

JKingsnorth · 2016-05-18T20:30:52Z

Yup. If that PR is the commit then go for it.
On 18 May 2016 21:07, "Eileen McNaughton" notifications@github.com wrote:

@JKingsnorth https://github.com/Jkingsnorth the commit you tried
suucessfully isolated in this PR #8385
#8385 so if that is ok we
can merge that PR.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8251 (comment)

eileenmcnaughton · 2016-05-18T20:32:29Z

Can you add a comment to that commit to clarify it has been QAd? Then I'll take another look here - my best guess if the rebasing of 9e537ad is this issue. Ican split the patches out

…deleted contacts Also, I changed the fatal to an exception - low level functions should throw exceptions, it's up to the form to catch & render those

…NION Unions are much faster than OR joins. This change took the length of the query to get the dedupes on a large database from 'as long as it took for the server to fall over' to less than one second on a small group of contacts This query is only affecting one path - ie Individuals - at the moment as I can only extend that as fast as I can write tests.

eileenmcnaughton · 2016-05-19T03:31:25Z

@JKingsnorth I think I tracked down the errant cacheKeyStrings that had not been updated.

Regarding your list of regressions - I believe they are all 4.7 regressions but not specifically 4.7.8 regressions? I'd like to tackle them - but not without getting some testing into those ajax functions first, so I'd see getting some tests added as a blocker to starting fixing

JKingsnorth · 2016-05-19T08:23:47Z

JIRA issue for adding tests: https://issues.civicrm.org/jira/browse/CRM-18581

JKingsnorth · 2016-05-19T12:11:17Z

tests/phpunit/CRM/Dedupe/MergerTest.php

@@ -163,7 +163,7 @@ public function testBatchMergeSelectedDuplicates() {

    // Retrieve pairs from prev next cache table
    $select = array('pn.is_selected' => 'is_selected');
-    $cacheKeyString = "merge Individual_{$dao->id}_{$this->_groupId}";
+    $cacheKeyString = "merge Individual_{$dao->id}_{$this->_groupId}_0";


Why doesn't this use getMergeCacheKeyString ? Because it's in a test?

Actually I think it should - I was in test-debug mode at that time

JKingsnorth · 2016-05-19T12:35:29Z

Confirmed that all the manual tests pass.

This PR also fixes CRM-18579.

I just made a couple of comments on the code. Once those are addressed (and QA'd), or pushed back, we can merge.

eileenmcnaughton · 2016-05-19T12:42:26Z

I think it might be better to merge & then try to get to those as improvements because of the timeframe. I might struggle to get much done tomorrow. What do you think?

JKingsnorth · 2016-05-19T12:57:25Z

Merge away, I'll note the UI improvement in the meta issue.

@deepak-srivastava If you get a chance to test this in the test week as well that would be great; I know you've done a lot of work around all this

civicrm-builder added the master label Apr 27, 2016

eileenmcnaughton force-pushed the merge_tests branch from 3c1959c to 668f6e4 Compare May 3, 2016 22:50

totten mentioned this pull request May 6, 2016

Invitation: CiviCRM Release for June 2016 civicrm/release-management#2

Closed

eileenmcnaughton force-pushed the merge_tests branch 3 times, most recently from 4132b33 to b904ea0 Compare May 13, 2016 10:17

eileenmcnaughton mentioned this pull request May 17, 2016

CRM-18546 Duplicate email addresses created when merging in 4.7 #8385

Merged

eileenmcnaughton force-pushed the merge_tests branch 2 times, most recently from fde8f27 to c0deb05 Compare May 18, 2016 00:22

eileenmcnaughton force-pushed the merge_tests branch 3 times, most recently from 3c34db9 to d99c28e Compare May 18, 2016 04:58

eileenmcnaughton changed the title ~~CRM-18842 Dedupe query: remove OR join in favour of more performant Union~~ Dedupe fixes CRM-18842, CRM-18546 , CRM-18842, CRM-18539, CRM-18480 May 18, 2016

eileenmcnaughton changed the title ~~Dedupe fixes CRM-18842, CRM-18546 , CRM-18842, CRM-18539, CRM-18480~~ Dedupe fixes CRM-18842, CRM-18546 , CRM-18539, CRM-18480 May 18, 2016

eileenmcnaughton changed the title ~~Dedupe fixes CRM-18842, CRM-18546 , CRM-18539, CRM-18480~~ Dedupe fixes CRM-18442, CRM-18546 , CRM-18539, CRM-18480 May 18, 2016

eileenmcnaughton mentioned this pull request May 18, 2016

CRM-18546 Duplicate email addresses created when merging in 4.7 #8352

Closed

eileenmcnaughton force-pushed the merge_tests branch from d99c28e to b7ffd67 Compare May 19, 2016 00:46

eileenmcnaughton force-pushed the merge_tests branch from b7ffd67 to 5834703 Compare May 19, 2016 01:30

eileenmcnaughton added 2 commits May 19, 2016 14:09

CRM-18480 fix batch merge job to cope with new inability to re-merge …

7827275

…deleted contacts Also, I changed the fatal to an exception - low level functions should throw exceptions, it's up to the form to catch & render those

eileenmcnaughton force-pushed the merge_tests branch from 5834703 to 6a44953 Compare May 19, 2016 02:09

CRM-18539: Permit criteria for batch merging (other than group)

e23e26e

eileenmcnaughton force-pushed the merge_tests branch from 6a44953 to e23e26e Compare May 19, 2016 03:26

eileenmcnaughton changed the title ~~Dedupe fixes CRM-18442, CRM-18546 , CRM-18539, CRM-18480~~ Dedupe fixes CRM-18442, CRM-18539, CRM-18480 May 19, 2016

JKingsnorth reviewed May 19, 2016
View reviewed changes

eileenmcnaughton merged commit c5856e2 into civicrm:master May 19, 2016

eileenmcnaughton deleted the merge_tests branch May 19, 2016 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedupe fixes CRM-18442, CRM-18539, CRM-18480 #8251

Dedupe fixes CRM-18442, CRM-18539, CRM-18480 #8251

eileenmcnaughton commented Apr 27, 2016 •

edited

Loading

eileenmcnaughton commented May 11, 2016

eileenmcnaughton commented May 18, 2016

eileenmcnaughton commented May 18, 2016

JKingsnorth commented May 18, 2016

JKingsnorth commented May 18, 2016

eileenmcnaughton commented May 18, 2016

JKingsnorth commented May 18, 2016 •

edited

Loading

JKingsnorth commented May 18, 2016 •

edited

Loading

JKingsnorth commented May 18, 2016 •

edited

Loading

JKingsnorth commented May 18, 2016

eileenmcnaughton commented May 18, 2016

JKingsnorth commented May 18, 2016

eileenmcnaughton commented May 18, 2016

eileenmcnaughton commented May 19, 2016

JKingsnorth commented May 19, 2016

JKingsnorth May 19, 2016

eileenmcnaughton May 19, 2016

JKingsnorth commented May 19, 2016 •

edited

Loading

eileenmcnaughton commented May 19, 2016

JKingsnorth commented May 19, 2016

Dedupe fixes CRM-18442, CRM-18539, CRM-18480 #8251

Dedupe fixes CRM-18442, CRM-18539, CRM-18480 #8251

Conversation

eileenmcnaughton commented Apr 27, 2016 • edited Loading

eileenmcnaughton commented May 11, 2016

eileenmcnaughton commented May 18, 2016

eileenmcnaughton commented May 18, 2016

JKingsnorth commented May 18, 2016

JKingsnorth commented May 18, 2016

eileenmcnaughton commented May 18, 2016

JKingsnorth commented May 18, 2016 • edited Loading

JKingsnorth commented May 18, 2016 • edited Loading

JKingsnorth commented May 18, 2016 • edited Loading

JKingsnorth commented May 18, 2016

eileenmcnaughton commented May 18, 2016

JKingsnorth commented May 18, 2016

eileenmcnaughton commented May 18, 2016

eileenmcnaughton commented May 19, 2016

JKingsnorth commented May 19, 2016

JKingsnorth May 19, 2016

Choose a reason for hiding this comment

eileenmcnaughton May 19, 2016

Choose a reason for hiding this comment

JKingsnorth commented May 19, 2016 • edited Loading

eileenmcnaughton commented May 19, 2016

JKingsnorth commented May 19, 2016

eileenmcnaughton commented Apr 27, 2016 •

edited

Loading

JKingsnorth commented May 18, 2016 •

edited

Loading

JKingsnorth commented May 18, 2016 •

edited

Loading

JKingsnorth commented May 18, 2016 •

edited

Loading

JKingsnorth commented May 19, 2016 •

edited

Loading