CRM-20855: Disabling "Search Primary Details Only" causes partial CiviMail delivery failure #10915
Conversation
| if ($primaryLocationOnly === NULL) { | ||
| $primaryLocationOnly = Civi::settings()->get('searchPrimaryDetailsOnly'); | ||
| } | ||
| $this->_primaryLocation = $primaryLocationOnly; |
There was a problem hiding this comment.
It turns out there is already a property for primary only so let's use that.
| */ | ||
| public function __construct( | ||
| $params = NULL, $returnProperties = NULL, $fields = NULL, | ||
| $includeContactIds = FALSE, $strict = FALSE, $mode = 1, | ||
| $skipPermission = FALSE, $searchDescendentGroups = TRUE, | ||
| $smartGroupCache = TRUE, $displayRelationshipType = NULL, | ||
| $operator = 'AND', | ||
| $apiEntity = NULL | ||
| $apiEntity = NULL, | ||
| $primaryLocationOnly = NULL |
There was a problem hiding this comment.
Let's pass in a var that says what we want to do, rather than overload one we don't really understand
| $searchPrimary = "AND {$name}.is_primary = 1"; | ||
| } | ||
|
|
||
| $limitToPrimaryClause = $primaryLocation ? "AND {$name}.is_primary = 1" : ''; |
There was a problem hiding this comment.
I think this name for the var is closer. Let's always pass in $primaryLocation correctly & not try to derive in this function
CRM/Utils/Token.php
Outdated
| @@ -1235,9 +1235,7 @@ public static function getTokenDetails( | |||
| } | |||
| } | |||
|
|
|||
| $query = new CRM_Contact_BAO_Query($params, $returnProperties); | |||
There was a problem hiding this comment.
Really - this is being substantiated & then called statically in a way that shows no benefit from the substantiation
| @@ -603,7 +603,8 @@ function _civicrm_api3_get_query_object($params, $mode, $entity) { | |||
| $newParams = CRM_Contact_BAO_Query::convertFormValues($inputParams, 0, FALSE, $entity); | |||
| $query = new CRM_Contact_BAO_Query($newParams, $returnProperties, NULL, | |||
| FALSE, FALSE, $mode, | |||
| empty($params['check_permissions']) | |||
| empty($params['check_permissions']), | |||
| TRUE, TRUE, NULL, 'AND', 'NULL', TRUE | |||
There was a problem hiding this comment.
set extras explicitly including SearchPrimary
| ) { | ||
| if ($primaryLocationOnly === NULL) { |
There was a problem hiding this comment.
Perhaps this could just be FALSE ie. if it is FALSE then we check the setting & possibly update to TRUE. For TRUE always stay TRUE. I'm on the fence here
| $contactIDs = array($contactID); | ||
|
|
||
| // when we are fetching contact details ON basis of primary address fields | ||
| $contactDetails = CRM_Utils_Token::getTokenDetails($contactIDs); |
There was a problem hiding this comment.
I removed the one that involves passing in $extraParams because that is a weird funky way of passing stuff in. It is merged into $params & passed on so you don't really know what will wind up where
811a95a to
13f9571
Compare
|
@lcdservices this is a alternate PR #10746 , as per my test and added unit test and worked for me. I think it more accurate by introducing separate parameter and manipulate the underlying query fragments on basis of that. Would you like to test it? |
13f9571 to
5716104
Compare
|
rebased |
64f3607 to
8b04115
Compare
…iMail delivery failure CRM-20855: Adjust variables for clarity I'm concerned about the use of $apiEntity to mean 'do something unconnected' so suggest a specific paramteter.
8b04115 to
0606198
Compare
|
Tested, working fine. |
Overview
Alternate to #10746, fixing a problem when some contacts are missed when the 'search primary details only' setting is disabled.
Before
When a contact has 2 emails & the setting for search primary only is off, calling the tokens query gets 2 rows but truncates by the number of contacts, resulting in one being lost.
After
Test in place demonstrating that the retrieval works.
Technical Details
Replaces #10746 - the unit tests in there replicate some or all of the problem. This is just stylistic changes on top of that.
Comments
I'm not 100% sure if this solves the whole problem or one manifestation - we should confirm when closing JIRA.