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.

Sign up for GitHub

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 #10915

Merged
merged 1 commit into from
Sep 4, 2017
Merged

CRM-20855: Disabling "Search Primary Details Only" causes partial CiviMail delivery failure #10915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions CRM/Contact/BAO/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,17 +420,21 @@ class CRM_Contact_BAO_Query {
* @param null $displayRelationshipType
* @param string $operator
* @param string $apiEntity
*
* @return \CRM_Contact_BAO_Query
* @param bool|NULL $primaryLocationOnly
*/
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's pass in a var that says what we want to do, rather than overload one we don't really understand

) {
if ($primaryLocationOnly === NULL) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

$primaryLocationOnly = Civi::settings()->get('searchPrimaryDetailsOnly');
}
$this->_primaryLocation = $primaryLocationOnly;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out there is already a property for primary only so let's use that.

$this->_params = &$params;
if ($this->_params == NULL) {
$this->_params = array();
Expand Down Expand Up @@ -2633,29 +2637,25 @@ public static function fromClause(&$tables, $inner = NULL, $right = NULL, $prima
}
continue;
}
$searchPrimary = '';
if (Civi::settings()->get('searchPrimaryDetailsOnly') || $apiEntity) {
$searchPrimary = "AND {$name}.is_primary = 1";
}

$limitToPrimaryClause = $primaryLocation ? "AND {$name}.is_primary = 1" : '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this name for the var is closer. Let's always pass in $primaryLocation correctly & not try to derive in this function


switch ($name) {
case 'civicrm_address':
//CRM-14263 further handling of address joins further down...
if (!$primaryLocation) {
$searchPrimary = '';
}
$from .= " $side JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id {$searchPrimary} )";
$from .= " $side JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id {$limitToPrimaryClause} )";
continue;

case 'civicrm_phone':
$from .= " $side JOIN civicrm_phone ON (contact_a.id = civicrm_phone.contact_id {$searchPrimary}) ";
$from .= " $side JOIN civicrm_phone ON (contact_a.id = civicrm_phone.contact_id {$limitToPrimaryClause}) ";
continue;

case 'civicrm_email':
$from .= " $side JOIN civicrm_email ON (contact_a.id = civicrm_email.contact_id {$searchPrimary})";
$from .= " $side JOIN civicrm_email ON (contact_a.id = civicrm_email.contact_id {$limitToPrimaryClause})";
continue;

case 'civicrm_im':
$from .= " $side JOIN civicrm_im ON (contact_a.id = civicrm_im.contact_id {$searchPrimary}) ";
$from .= " $side JOIN civicrm_im ON (contact_a.id = civicrm_im.contact_id {$limitToPrimaryClause}) ";
continue;

case 'im_provider':
Expand All @@ -2665,7 +2665,7 @@ public static function fromClause(&$tables, $inner = NULL, $right = NULL, $prima
continue;

case 'civicrm_openid':
$from .= " $side JOIN civicrm_openid ON ( civicrm_openid.contact_id = contact_a.id {$searchPrimary} )";
$from .= " $side JOIN civicrm_openid ON ( civicrm_openid.contact_id = contact_a.id {$limitToPrimaryClause} )";
continue;

case 'civicrm_worldregion':
Expand Down Expand Up @@ -4428,6 +4428,7 @@ public static function getQuery($params = NULL, $returnProperties = NULL, $count
* The api entity being called.
* This sort-of duplicates $mode in a confusing way. Probably not by design.
*
* @param bool|null $primaryLocationOnly
* @return array
*/
public static function apiQuery(
Expand All @@ -4441,7 +4442,8 @@ public static function apiQuery(
$count = FALSE,
$skipPermissions = TRUE,
$mode = CRM_Contact_BAO_Query::MODE_CONTACTS,
$apiEntity = NULL
$apiEntity = NULL,
$primaryLocationOnly = NULL
) {

$query = new CRM_Contact_BAO_Query(
Expand All @@ -4450,7 +4452,7 @@ public static function apiQuery(
$skipPermissions,
TRUE, $smartGroupCache,
NULL, 'AND',
$apiEntity
$apiEntity, $primaryLocationOnly
);

//this should add a check for view deleted if permissions are enabled
Expand Down
2 changes: 1 addition & 1 deletion CRM/Utils/Token.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,7 +1235,7 @@ public static function getTokenDetails(
}
}

$details = CRM_Contact_BAO_Query::apiQuery($params, $returnProperties, NULL, NULL, 0, count($contactIDs));
$details = CRM_Contact_BAO_Query::apiQuery($params, $returnProperties, NULL, NULL, 0, count($contactIDs), TRUE, FALSE, TRUE, CRM_Contact_BAO_Query::MODE_CONTACTS, NULL, TRUE);

$contactDetails = &$details[0];

Expand Down
6 changes: 4 additions & 2 deletions api/v3/utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,8 @@ function _civicrm_api3_get_using_query_object($entity, $params, $additional_opti
$getCount,
$skipPermissions,
$mode,
$entity
$entity,
TRUE
);

return $entities;
Expand Down Expand Up @@ -603,7 +604,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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set extras explicitly including SearchPrimary

);
list($select, $from, $where, $having) = $query->query();

Expand Down
55 changes: 36 additions & 19 deletions tests/phpunit/CRM/Utils/Token.php → tests/phpunit/CRM/Utils/TokenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,44 @@ public function testGetTokenDetails() {
}

/**
* Test for replaceGreetingTokens.
* Test getting contacts w/o primary location type
*
* Check for situation described in CRM-19876.
*/
public function testReplaceGreetingTokens() {
$tokenString = 'First Name: {contact.first_name} Last Name: {contact.last_name} Birth Date: {contact.birth_date} Prefix: {contact.prefix_id} Suffix: {contact.individual_suffix}';
$contactDetails = array(
array(
2811 => array(
'id' => '2811',
'contact_type' => 'Individual',
'first_name' => 'Morticia',
'last_name' => 'Addams',
'prefix_id' => 2,
),
),
);
$contactId = 2811;
$className = 'CRM_Contact_BAO_Contact';
$escapeSmarty = TRUE;
CRM_Utils_Token::replaceGreetingTokens($tokenString, $contactDetails, $contactId, $className, $escapeSmarty);
$this->assertEquals($tokenString, 'First Name: Morticia Last Name: Addams Birth Date: Prefix: Ms. Suffix: ');
public function testSearchByPrimaryLocation() {
// Disable searchPrimaryDetailsOnly civi settings so we could test the functionality without it.
Civi::settings()->set('searchPrimaryDetailsOnly', '0');

// create a contact with multiple email address and among which one is primary
$contactID = $this->individualCreate();
$primaryEmail = uniqid() . '@primary.com';
$this->callAPISuccess('Email', 'create', array(
'contact_id' => $contactID,
'email' => $primaryEmail,
'location_type_id' => 'Other',
'is_primary' => 1,
));
$this->callAPISuccess('Email', 'create', array(
'contact_id' => $contactID,
'email' => uniqid() . '@galaxy.com',
'location_type_id' => 'Work',
'is_primary' => 0,
));
$this->callAPISuccess('Email', 'create', array(
'contact_id' => $contactID,
'email' => uniqid() . '@galaxy.com',
'location_type_id' => 'Work',
'is_primary' => 0,
));

$contactIDs = array($contactID);

// when we are fetching contact details ON basis of primary address fields
$contactDetails = CRM_Utils_Token::getTokenDetails($contactIDs);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

$this->assertEquals($primaryEmail, $contactDetails[0][$contactID]['email']);

// restore setting
Civi::settings()->set('searchPrimaryDetailsOnly', '1');
}

/**
Expand Down