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-20855: Disabling "Search Primary Details Only" causes partial CiviMail delivery failure #10746

Closed
wants to merge 1 commit into from

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Jul 24, 2017

@mfb
Copy link
Contributor

mfb commented Jul 27, 2017

Confirmed that this resolves the bug 👍

@monishdeb
Copy link
Member Author

Thanks @mfb and @lcdservices for your feedback

@colemanw / @eileenmcnaughton can you merge this PR?

// Search by Primary location type ONLY if searchPrimaryDetailsOnly setting is on OR
// search query is trigerred via api OR
// special parameter search_by_primary_details_only = TRUE
self::$_alwaysSearchByPrimaryOnly = (Civi::settings()->get('searchPrimaryDetailsOnly') || $apiEntity || !empty($params['search_by_primary_details_only']));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the api should set $params['search_by_primary_details_only'] rather than $apiEntity being code for that.

Copy link
Member Author

@monishdeb monishdeb Aug 4, 2017

Choose a reason for hiding this comment

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

@eileenmcnaughton didn't found the exact spot set search_by_primary_details_only = 1 via all API params that uses location type. Reason why the earlier code used $apiEntity. Or can you suggest me where could we set this parameter for API?

@eileenmcnaughton
Copy link
Contributor

@monishdeb test is good & underlying logic is good. I'm less keen on what variables are being set where & what they are being relied on to do.

I put my thoughts in a separate PR https://github.com/civicrm/civicrm-core/pull/10915/files (wanted to see what will happen with tests on it) - but basically I think we should consolidate on existing parameters & be explicit about passing in searchPrimary rather than overloading the 'apiEntity' which already has an ambiguous relationship to 'mode'

@monishdeb
Copy link
Member Author

Closing in favor of #10915

@monishdeb monishdeb closed this Aug 29, 2017
@monishdeb monishdeb deleted the CRM-20855 branch August 29, 2017 15:07
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