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

CRM-19353 - 'Dedupe by email' not working 2 #9059

Closed
wants to merge 3 commits into from
Closed

CRM-19353 - 'Dedupe by email' not working 2 #9059

wants to merge 3 commits into from

Conversation

hakuren
Copy link

@hakuren hakuren commented Sep 16, 2016

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@colemanw
Copy link
Member

@civicrm-builder add to whitelist

Copy link
Member

@colemanw colemanw left a comment

Choose a reason for hiding this comment

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

This needs a couple of style fixes.

@eileenmcnaughton
Copy link
Contributor

test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@@ -4335,7 +4336,10 @@ public static function apiQuery(
$sql = "$select $from $where $having";

// add group by
if ($query->_useGroupBy) {
if(!empty($groupBy)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@hakuren you need to do if ( not if(

@eileenmcnaughton
Copy link
Contributor

We discussed this at the sprint & we had some concerns

  1. this was adding a new parameter to the api & to the api architecture ('group_by') and we weren't comfortable with that being added in an undiscussed, untested way
  2. We weren't convinced the UI implied that test recipients would be deduped

I'm going to close this PR because it is against an out of date RC.

I'd encourage you to open a new ticket for the outstanding issue around test recipients being deduped, since I think #9214 resolved CRM-19353 - (unless you no longer care)

Are you on the dev email list? It would be a good place to discuss adding group_by to the api?

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.

5 participants