Ensure that the is_deleted filter is added correctly if we are not se… #19088
Conversation
|
(Standard links)
|
|
@kcristiano you might be interested in this as well |
CRM/Contact/BAO/Query.php
Outdated
| @@ -5054,7 +5054,12 @@ 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); | |||
| if ($this->_permissionWhereClause === '( 1 )') { | |||
| $this->_permissionWhereClause = str_replace('( 1 )', '(contact_a.is_deleted = 0)', $this->_permissionWhereClause); | |||
There was a problem hiding this comment.
This looks redundant to me. There's no need to perform a str_replace when you know exactly what the string is.
I can't see from the logic above why this line shouldn't read
$this->_permissionWhereClause = '(contact_a.is_deleted = 0)';
CRM/Contact/BAO/Query.php
Outdated
| $this->_permissionWhereClause = str_replace('( 1 )', '(contact_a.is_deleted = 0)', $this->_permissionWhereClause); | ||
| } | ||
| else { | ||
| $this->_permissionWhereClause .= ' AND (contact_a.is_deleted = 0)'; |
There was a problem hiding this comment.
um... in fact why wouldn't this produce exactly the same result if $this->_permissionWhereClause DID === '( 1 )' ? I mean, doesn't the SQL become:
WHERE ( 1 ) AND (contact_a.is_deleted = 0) ?
There was a problem hiding this comment.
I seem to recall that being confusing I think and also this is consistent with the code lower down and I would argue that such a change should probably be done in a broader cleanup rather than in a regression fix
…arching for deleted contacts but have access to view deleted contacts Simplify IF down as per feed back from ACD and also comment from Eileen in chat and extend unit test as well
seamuslee001
force-pushed
the
dev_core_2227
branch
from
025a3ef to
9982726
Compare
|
@eileenmcnaughton @andrew-cormick-dockery I have now updated the if statements to be much more simplified as per the feedback and also added a new test case / unit test in the CRM_Contact_SelectorTest to cover this and I confirmed that with the original code it wasn't being picked up where as it is now |
| @@ -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); | |||
There was a problem hiding this comment.
Are we sure that permissionWhereClause would never be empty at this point - in which case there is a small risk the 'AND' is wrong
There was a problem hiding this comment.
@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
|
Discussed on IRC - I'm Ok with the testing @seamuslee001 has done on their staging |
…arching for deleted contacts but have access to view deleted contacts
Overview
This fixes a slight regression from the ACL work that Coleman and I did which is that an ACLed user that has the permission to view contacts in the trash, sees contacts within the trash when doing an ordinary advanced search.
Before
Contacts that are found in the trash appear in an advanced search for ACLed users even when not searching in the trash
After
Contacts that are in the trash only show on an advanced search when doing a search in the trash
Technical Details
The issue here is that the permissionClause may not be
( 1 )here as it may also apply to an ACLed user (i.e. someone without view / edit all contacts) so we need to do similar work to lines 5065 - 5070ping @eileenmcnaughton @andrew-cormick-dockery @johntwmyan