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

Towards CRM-20155 clean up form code in order to consolidate function… #10890

Merged
merged 1 commit into from
Aug 31, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

… use.

Overview

This is code clean up on the merge form, preliminary to extensionising the code & implementing improvements in an iterative way according to the LeXIM approach

Before

civicrm/contact/dedupefind is accessible from Find & merge duplicate contacts & works

After

No change

Technical Details

The code in the Find and Merge Duplicate Contacts form has mystified me for along time. As a step towards extracting dedupe code into separate extension/s I have gone through & tidied up the finding of duplicates to re-use a function used elsewhere rather than duplicate it on the form.

The main changes in here are

  1. $foundDupes = $this->get("dedupe_dupes_$gid"); routine is removed - I really could not see what purpose this serves @JKingsnorth @jmcclelland @deepak-srivastava maybe one of you can see a value in re-caching the dupes on the form (as well as in the prev_nextcache table) that I'm missing but my money is it is historical & preceeds the prevnext_cache mechanicsm
  2. addition of defined $criteria as a placeholder. This is mostly because I plan to make this url-passable later & feel making it explicit is helpful here. In the url passable version it is possible to dedupe from search results.
  3. I barely understand why we are setting $this->_cid, $this->_rgid, $this->_gid but to the extent it is useful setting at the start seems clearer than repeating it in if clauses
  4. (this is the main one) calling
CRM_Dedupe_Merger::getDuplicatePairs($rgid, $gid, !$isConflictMode, 0, $isConflictMode, '', $isConflictMode, $criteria, TRUE);

has the same effect as calling the removed lines 145-150, 160-162, 167-169 and 186


Comments

@JKingsnorth @jmcclelland both of you have looked at this code fairly recently - would appreciate your insights.

… use.

The code in the Find and Merge Duplicate Contacts form has mystified me for along time. As a step towards extracting
dedupe code into separate extension/s I have gone through & tidied up the finding of duplicates to re-use a function
used elsewhere rather than duplicate it on the form. In the process I tried, and failed, to come up with
a rationale for the duplicate form catching represented in setting form properties.

Note that I made the  parameter explicit. I am expecting this to be a URL paramter in the future
(currently deploying that to our site as such in the context of being able to find matches for specfic contacts.
I felt making it explicit now would aid in not missing instances of it when patching later
@jmcclelland
Copy link
Contributor

Thanks @eileenmcnaughton for doing the refactoring - I can't see any reason for the code you are getting rid of.

@totten totten added the master label Aug 30, 2017
@colemanw colemanw merged commit be23378 into civicrm:master Aug 31, 2017
@eileenmcnaughton eileenmcnaughton deleted the dedupe branch August 31, 2017 21:11
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Feb 1, 2018
… use.

PR civicrm#10890

The code in the Find and Merge Duplicate Contacts form has mystified me for along time. As a step towards extracting
dedupe code into separate extension/s I have gone through & tidied up the finding of duplicates to re-use a function
used elsewhere rather than duplicate it on the form. In the process I tried, and failed, to come up with
a rationale for the duplicate form catching represented in setting form properties.

Note that I made the  parameter explicit. I am expecting this to be a URL paramter in the future
(currently deploying that to our site as such in the context of being able to find matches for specfic contacts.
I felt making it explicit now would aid in not missing instances of it when patching later

Change-Id: I694d71570db9f70406c30769b5ee9457305c25c8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants