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

Jump to bottom

Ensure that the is_deleted filter is added correctly if we are not se… #19088

Merged
merged 1 commit into from
Dec 2, 2020
Merged

Ensure that the is_deleted filter is added correctly if we are not se… #19088

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
9 changes: 2 additions & 7 deletions CRM/Contact/BAO/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5054,15 +5054,10 @@ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) {

if (CRM_Core_Permission::check('access deleted contacts')) {
if (!$onlyDeleted) {
$this->_permissionWhereClause = str_replace('( 1 )', '(contact_a.is_deleted = 0)', $this->_permissionWhereClause);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that permissionWhereClause would never be empty at this point - in which case there is a small risk the 'AND' is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileenmcnaughton yes as per https://github.com/civicrm/civicrm-core/blob/master/CRM/Contact/BAO/Contact/Permission.php#L306 yes because a) it is not an array being passed so it won't get into the if in L326 of that file and there for it will be either ( 1 ) or a string of is_deleted = 0

The CRM_Contact_SelectorTest and CRM_Contact_BAO_QueryTests demonstrate that sufficiently I believe

$this->_permissionWhereClause .= ' AND (contact_a.is_deleted = 0)';
}
else {
if ($this->_permissionWhereClause === '( 1 )') {
$this->_permissionWhereClause = str_replace('( 1 )', '(contact_a.is_deleted)', $this->_permissionWhereClause);
}
else {
$this->_permissionWhereClause .= " AND (contact_a.is_deleted) ";
}
$this->_permissionWhereClause .= " AND (contact_a.is_deleted) ";
}
}

Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/CRM/Contact/BAO/QueryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ public function testSearchProfilePrimaryCityCRM14263($params, $selectClause, $wh
'contact_sub_type' => 1,
'sort_name' => 1,
];
$expectedSQL = 'SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, ' . $selectClause . " FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) WHERE ( ( " . $whereClause . " ) ) AND (contact_a.is_deleted = 0) ORDER BY `contact_a`.`sort_name` ASC, `contact_a`.`id` ";
$expectedSQL = 'SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, ' . $selectClause . " FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) WHERE ( ( " . $whereClause . " ) ) AND ( 1 ) AND (contact_a.is_deleted = 0) ORDER BY `contact_a`.`sort_name` ASC, `contact_a`.`id` ";
$queryObj = new CRM_Contact_BAO_Query($params, $returnProperties);
try {
$this->assertLike($expectedSQL, $queryObj->getSearchSQL());
Expand Down
38 changes: 29 additions & 9 deletions tests/phpunit/CRM/Contact/SelectorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ public function querySets() {
'expected_query' => [
0 => 'default',
1 => 'default',
2 => "WHERE ( civicrm_email.email LIKE '%mickey@mouseville.com%' ) AND (contact_a.is_deleted = 0)",
2 => "WHERE ( civicrm_email.email LIKE '%mickey@mouseville.com%' ) AND ( 1 ) AND (contact_a.is_deleted = 0)",
],
],
],
Expand All @@ -282,7 +282,7 @@ public function querySets() {
'expected_query' => [
0 => 'default',
1 => 'default',
2 => "WHERE ( civicrm_email.email LIKE '%mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE '%Mouse%' ) OR ( civicrm_email.email LIKE '%Mouse%' ) ) ) ) AND (contact_a.is_deleted = 0)",
2 => "WHERE ( civicrm_email.email LIKE '%mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE '%Mouse%' ) OR ( civicrm_email.email LIKE '%Mouse%' ) ) ) ) AND ( 1 ) AND (contact_a.is_deleted = 0)",
],
],
],
Expand All @@ -301,7 +301,7 @@ public function querySets() {
'expected_query' => [
0 => 'default',
1 => 'default',
2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND (contact_a.is_deleted = 0)",
2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND ( 1 ) AND (contact_a.is_deleted = 0)",
],
],
],
Expand All @@ -320,7 +320,7 @@ public function querySets() {
'expected_query' => [
0 => 'default',
1 => 'default',
2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND (contact_a.is_deleted)",
2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND ( 1 ) AND (contact_a.is_deleted)",
],
],
],
Expand All @@ -344,6 +344,26 @@ public function querySets() {
],
],
],
[
[
'description' => 'Ensure that the Join to the acl contact cache is correct and that if we are not searching in the trash trashed contacts are not returned',
'class' => 'CRM_Contact_Selector',
'settings' => [['name' => 'includeWildCardInName', 'value' => FALSE]],
'form_values' => ['email' => 'mickey@mouseville.com', 'sort_name' => 'Mouse'],
'params' => [],
'return_properties' => NULL,
'context' => 'advanced',
'action' => CRM_Core_Action::ADVANCED,
'includeContactIds' => NULL,
'searchDescendentGroups' => FALSE,
'limitedPermissions' => TRUE,
'expected_query' => [
0 => 'default',
1 => 'FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 ) LEFT JOIN civicrm_country ON ( civicrm_address.country_id = civicrm_country.id ) LEFT JOIN civicrm_email ON (contact_a.id = civicrm_email.contact_id AND civicrm_email.is_primary = 1) LEFT JOIN civicrm_phone ON (contact_a.id = civicrm_phone.contact_id AND civicrm_phone.is_primary = 1) LEFT JOIN civicrm_im ON (contact_a.id = civicrm_im.contact_id AND civicrm_im.is_primary = 1) LEFT JOIN civicrm_worldregion ON civicrm_country.region_id = civicrm_worldregion.id INNER JOIN civicrm_acl_contact_cache aclContactCache ON contact_a.id = aclContactCache.contact_id',
2 => "WHERE ( civicrm_email.email LIKE 'mickey@mouseville.com%' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND aclContactCache.user_id = 0 AND (contact_a.is_deleted = 0)",
],
],
],
[
[
'description' => 'Use of quotes for exact string',
Expand All @@ -360,7 +380,7 @@ public function querySets() {
'expected_query' => [
0 => 'default',
1 => 'default',
2 => "WHERE ( civicrm_email.email = 'mickey@mouseville.com' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND (contact_a.is_deleted = 0)",
2 => "WHERE ( civicrm_email.email = 'mickey@mouseville.com' AND ( ( ( contact_a.sort_name LIKE 'Mouse%' ) OR ( civicrm_email.email LIKE 'Mouse%' ) ) ) ) AND ( 1 ) AND (contact_a.is_deleted = 0)",
],
],
],
Expand All @@ -383,7 +403,7 @@ public function querySets() {
'expected_query' => [
0 => 'SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, civicrm_address.id as address_id, civicrm_address.country_id as country_id',
1 => ' FROM civicrm_contact contact_a LEFT JOIN civicrm_address ON ( contact_a.id = civicrm_address.contact_id AND civicrm_address.is_primary = 1 )',
2 => 'WHERE ( contact_a.contact_type IN ("Individual") AND civicrm_address.country_id IS NOT NULL ) AND (contact_a.is_deleted = 0)',
2 => 'WHERE ( contact_a.contact_type IN ("Individual") AND civicrm_address.country_id IS NOT NULL ) AND ( 1 ) AND (contact_a.is_deleted = 0)',
],
],
],
Expand All @@ -403,7 +423,7 @@ public function querySets() {
'searchDescendentGroups' => FALSE,
'expected_query' => [
0 => 'SELECT contact_a.id as contact_id, source_contact.id as source_contact_id',
2 => 'WHERE ( source_contact.id IS NOT NULL ) AND (contact_a.is_deleted = 0)',
2 => 'WHERE ( source_contact.id IS NOT NULL ) AND ( 1 ) AND (contact_a.is_deleted = 0)',
],
],
],
Expand All @@ -423,7 +443,7 @@ public function querySets() {
0 => 'SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, contact_a.display_name as `display_name`, contact_a.do_not_email as `do_not_email`, contact_a.do_not_phone as `do_not_phone`, contact_a.do_not_mail as `do_not_mail`, contact_a.do_not_sms as `do_not_sms`, contact_a.do_not_trade as `do_not_trade`, contact_a.is_opt_out as `is_opt_out`, contact_a.legal_identifier as `legal_identifier`, contact_a.external_identifier as `external_identifier`, contact_a.nick_name as `nick_name`, contact_a.legal_name as `legal_name`, contact_a.image_URL as `image_URL`, contact_a.preferred_communication_method as `preferred_communication_method`, contact_a.preferred_language as `preferred_language`, contact_a.preferred_mail_format as `preferred_mail_format`, contact_a.first_name as `first_name`, contact_a.middle_name as `middle_name`, contact_a.last_name as `last_name`, contact_a.prefix_id as `prefix_id`, contact_a.suffix_id as `suffix_id`, contact_a.formal_title as `formal_title`, contact_a.communication_style_id as `communication_style_id`, contact_a.job_title as `job_title`, contact_a.gender_id as `gender_id`, contact_a.birth_date as `birth_date`, contact_a.is_deceased as `is_deceased`, contact_a.deceased_date as `deceased_date`, contact_a.household_name as `household_name`, IF ( contact_a.contact_type = \'Individual\', NULL, contact_a.organization_name ) as organization_name, contact_a.sic_code as `sic_code`, contact_a.is_deleted as `contact_is_deleted`, IF ( contact_a.contact_type = \'Individual\', contact_a.organization_name, NULL ) as current_employer, civicrm_address.id as address_id, civicrm_address.street_address as `street_address`, civicrm_address.supplemental_address_1 as `supplemental_address_1`, civicrm_address.supplemental_address_2 as `supplemental_address_2`, civicrm_address.supplemental_address_3 as `supplemental_address_3`, civicrm_address.city as `city`, civicrm_address.postal_code_suffix as `postal_code_suffix`, civicrm_address.postal_code as `postal_code`, civicrm_address.geo_code_1 as `geo_code_1`, civicrm_address.geo_code_2 as `geo_code_2`, civicrm_address.state_province_id as state_province_id, civicrm_address.country_id as country_id, civicrm_phone.id as phone_id, civicrm_phone.phone_type_id as phone_type_id, civicrm_phone.phone as `phone`, civicrm_email.id as email_id, civicrm_email.email as `email`, civicrm_email.on_hold as `on_hold`, civicrm_im.id as im_id, civicrm_im.provider_id as provider_id, civicrm_im.name as `im`, civicrm_worldregion.id as worldregion_id, civicrm_worldregion.name as `world_region`',
2 => 'WHERE displayRelType.relationship_type_id = 1
AND displayRelType.is_active = 1
AND (contact_a.is_deleted = 0)',
AND ( 1 ) AND (contact_a.is_deleted = 0)',
],
],
],
Expand Down Expand Up @@ -499,7 +519,7 @@ public function testSelectorQueryOnNonASCIIlocationType() {
$expectedQuery = [
0 => "SELECT contact_a.id as contact_id, contact_a.contact_type as `contact_type`, contact_a.contact_sub_type as `contact_sub_type`, contact_a.sort_name as `sort_name`, `Non_ASCII_Location_Type-location_type`.id as `Non_ASCII_Location_Type-location_type_id`, `Non_ASCII_Location_Type-location_type`.name as `Non_ASCII_Location_Type-location_type`, `Non_ASCII_Location_Type-email`.id as `Non_ASCII_Location_Type-email_id`, `Non_ASCII_Location_Type-email`.email as `Non_ASCII_Location_Type-email`",
// @TODO these FROM clause doesn't matches due to extra spaces or special character
2 => "WHERE ( ( `Non_ASCII_Location_Type-email`.email IS NOT NULL ) ) AND (contact_a.is_deleted = 0)",
2 => "WHERE ( ( `Non_ASCII_Location_Type-email`.email IS NOT NULL ) ) AND ( 1 ) AND (contact_a.is_deleted = 0)",
];
foreach ($expectedQuery as $index => $queryString) {
$this->assertEquals($this->strWrangle($queryString), $this->strWrangle($sql[$index]));
Expand Down