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

Re-instate Dedupe limit functionality & fix select toggle functionality #12305

Merged
merged 2 commits into from
Jul 1, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 12, 2018

Overview

Follow up fixes to #12193 for obscure dedupe scenarios

Before

  1. if the parameter 'limit=2' is passed in the url it is not heeded
  2. if the url is being used with 'criteria' the selection of rows does not work on the screen reached after a safe merge has been attempted - eg. this for matches of contact id 506191
    civicrm/contact/dedupefind?reset=1&action=update&rgid=4&criteria=%7B%22contact%22%3A%7B%22id%22%3A%7B%22IN%22%3A%5B%22506191%22%5D%7D%7D%7D&limit=1

After

  1. limit parameter re-instated (note the results are cached so you need to truncate civicrm_prevnext_cache when changing the value). Also note that 'limit' is the number of contacts to try to find matches for - so if a group has 10000 contacts in it the dedupe finder will only seek to find matches for the first contact if limit=1. (for test purposes I'm using a group of contacts who all have the same email address
    merge_limit2

  2. toggle works again
    merge_limit3

Technical Details

I've put this against the rc because it is an addendum to a patch already in the rc. The scenarios described are pretty obscure!

Comments

@monishdeb are you able to look.

It turned out the toggleDuplicates was not working when criteria was set as the validation rule didn't work.

Passing around cacheKey
is easier to validate and we know the cache willbe created at the point of toggle. Use cacheKey instead
in url
@civibot
Copy link

civibot bot commented Jun 12, 2018

(Standard links)

@@ -324,8 +324,7 @@
var is_selected = CRM.$('.crm-dedupe-select-all').prop('checked') ? 1 : 0;
}

var criteria = {/literal}'{$criteria|escape}'{literal};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This escape wasn't working - hence the switch to cachekey string in this function

@@ -2035,7 +2036,7 @@ public static function getDuplicatePairs($rule_group_id, $group_id, $reloadCache
*/
public static function getMergeCacheKeyString($rule_group_id, $group_id, $criteria = array(), $checkPermissions = TRUE) {
$contactType = CRM_Dedupe_BAO_RuleGroup::getContactTypeForRuleGroup($rule_group_id);
$cacheKeyString = "merge {$contactType}";
$cacheKeyString = "merge_{$contactType}";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add _ since we are now passing in the url & want it to be Alphanumeric

@@ -181,7 +181,7 @@ public static function dupesByParams(
* array of (cid1, cid2, weight) dupe triples
*/
public static function dupesInGroup($rgid, $gid, $searchLimit = 0) {
$cids = array_keys(CRM_Contact_BAO_Group::getMember($gid, $searchLimit));
$cids = array_keys(CRM_Contact_BAO_Group::getMember($gid, TRUE, $searchLimit));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just an OOPS - was passing $searchLimit as param 2 not param 3!

@@ -183,7 +182,7 @@ public function run() {
CRM_Dedupe_Merger::resetMergeStats($cacheKeyString);
}

$this->_mainContacts = CRM_Dedupe_Merger::getDuplicatePairs($rgid, $gid, !$isConflictMode, 0, $this->isSelected(), '', $isConflictMode, $criteria, TRUE);
$this->_mainContacts = CRM_Dedupe_Merger::getDuplicatePairs($rgid, $gid, !$isConflictMode, 0, $this->isSelected(), '', $isConflictMode, $criteria, TRUE, $limit);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pass limit through

$pnid = $_REQUEST['pnid'];
$isSelected = CRM_Utils_Type::escape($_REQUEST['is_selected'], 'Boolean');
$criteria = CRM_Utils_Request::retrieve('criteria', 'Json', $null, FALSE, '{}');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this function wasn't working when criteria was passed because the smarty layer escaping was leaving it empty & the cachekey couldn't be calculated. Turns out the criteria is really only needed to calculate the cacheKey so instead just pass that & simplify. Note the very last line on this PR's changes is where we change what is passed into this PR

@eileenmcnaughton eileenmcnaughton changed the title Dedupe limit Re-instate Dedupe limit functionality & fix select toggle functionality Jun 12, 2018
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb can you review this? I think it should go in 5.3 as related work is in 5.3 but that means it should be soon

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb ping

@monishdeb
Copy link
Member

Ok will review it by today or tomorrow.

@eileenmcnaughton
Copy link
Contributor Author

thx

@monishdeb
Copy link
Member

(CiviCRM Review Template WORD-1.1)

  • (r-explain): Happy with desription and screencast which helped me to quickly replicate the issue where selection action doesn't worked earlier.
  • (r-test) N/A
  • (r-code) PASS
  • (r-doc) PASS
  • (r-maint) PASS
  • (r-run) PASS: Tested on local and it worked fine as described in screencast.
  • (r-user) PASS
  • (r-tech) PASS

@monishdeb monishdeb merged commit ba7084e into civicrm:5.3 Jul 1, 2018
@eileenmcnaughton eileenmcnaughton deleted the dedupe_limit branch July 1, 2018 22:04
@eileenmcnaughton
Copy link
Contributor Author

thanks @monishdeb

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.

3 participants