CRM: Ensuring users assigned to contacts linked to transactions, invoices, quotes can view those objects #36239
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Significance: patch | ||
| Type: fixed | ||
|
|
||
| Permissions: Allowing users assigned to contacts to view linked objects even if assign ownership is unchecked |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -170,35 +170,57 @@ public function preChecks(){ | |||||
| $is_malformed_obj = false; | ||||||
|
|
||||||
| if (is_array($this->obj) && isset($this->obj['owner'])){ | ||||||
| $obj_owner = (int) $this->obj['owner']; | ||||||
|
|
||||||
| // Transactions can have a contact or company assigned, and quotes just a contact. This covers checking owners for both. | ||||||
| if ( isset( $this->obj['contact'][0]['owner'] ) ) { | ||||||
| $obj_owner = (int) $this->obj['contact'][0]['owner']; | ||||||
|
|
||||||
| } elseif ( isset( $this->obj['company'][0]['owner'] ) ) { | ||||||
| $obj_owner = (int) $this->obj['company'][0]['owner']; | ||||||
| // phpcs:disable Generic.WhiteSpace.ScopeIndent.IncorrectExact,Generic.WhiteSpace.ScopeIndent.Incorrect -- this sniff is incorrectly reporting spacing issues. | ||||||
| } | ||||||
|
|
||||||
| // This covers checking owners for assigned contacts or companies in invoices. | ||||||
| if ( $this->objTypeID === 4 ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase | ||||||
coder-karen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| $data = zeroBSCRM_invoicing_getInvoiceData( $this->objID ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase | ||||||
| if ( ! empty( $data['invoiceObj']['contact'] ) ) { | ||||||
| $obj_owner = (int) $data['invoiceObj']['contact'][0]['owner']; | ||||||
| } elseif ( ! empty( $data['invoiceObj']['contact'] ) ) { | ||||||
| $obj_owner = (int) $data['invoiceObj']['company'][0]['owner']; | ||||||
| } | ||||||
| } | ||||||
| } else { | ||||||
| // phpcs:enable Generic.WhiteSpace.ScopeIndent.IncorrectExact,Generic.WhiteSpace.ScopeIndent.Incorrect | ||||||
| // if $this->obj is not an array, somehow it's not been loaded properly (probably perms) | ||||||
| // get owner info anyway | ||||||
| $is_malformed_obj = true; | ||||||
| $obj_owner = $zbs->DAL->getObjectOwner( // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase | ||||||
| array( | ||||||
| 'objID' => $this->objID, // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase | ||||||
| 'objTypeID' => $this->objTypeID, // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase | ||||||
| ) | ||||||
| ); | ||||||
| } | ||||||
| // get current user | ||||||
| $current_user_id = get_current_user_id(); | ||||||
|
|
||||||
| if ( $obj_owner > 0 && $obj_owner !== $current_user_id ) { | ||||||
|
||||||
| if ( $obj_owner > 0 && $obj_owner !== $current_user_id ) { | |
| if ( ( $obj_owner > 0 && $obj_owner !== $current_user_id ) || $obj_owner === -1 ) { |
There was a problem hiding this comment.
This change would prevent unassigned contacts to be viewed by all contact managers.
I'm suggesting this because this is our current behavior, and in my humble opinion, we should not change it.
I think that comparing to -1 is enough to make the assertion that the contact is unassigned.
There was a problem hiding this comment.
This behaviour hasn't been changed - prior to this PR CRM Contact Managers could view all unassigned contacts as well, so that hasn't been changed here.
That is with 'Contact Assignment' checked and 'Assign Ownership' unchecked. It's the same if both are unchecked, and checked, both before and after this PR, for me, too. It should work as is currently anyway because if $obj_owner is -1 (unassigned), then it will return true for $obj_owner !== $current_user_id. I do see however that I changed this to be a strict comparison, and that should have been changed back with the other comparison reverts (as there is the chance some results will be strings not ints). I'll change that now - 0f0a197. I'm not sure if that makes any difference in your test case though, let me know if so.
There was a problem hiding this comment.
I apologize for the confusion in my earlier message. Rectifying, only transactions, quotes, and invoices linked to unassigned contacts were made accessible to contact managers in this branch.
The interaction with transactions/invoices/quotes linked to assigned contacts was preserved.
This is what happens:
TRUNK (as a contact manager)
Unassigned Contact (I can see the contact):
The same contact's transaction:
fix/crm-assign-ownership-viewobjects (as that same contact manager)
Unassigned Contact (I can see the contact):
The same contact's transaction:
There was a problem hiding this comment.
I do actually see what you mean now, thanks!
The behaviour in trunk is slightly more nuanced, here's what I found:
Trunk: Very inconsistent behaviour - some older transactions, invoices, quotes, I can't view
regardless of being unassigned, assigned to a contact with no owner, and newer ones I can view in the same circumstances plus being linked to a contact with owner (regardless of owner).
Branch: Behaviour is consistent. Except - transactions and invoices created by the contact manager and unassigned or assigned to a contact with no owner are viewable (previously I thought it was just when unassigned), or the same objects created by admin and assigned to a contact with no owner.
The one thing there that looks like it needs fixing is the situation you mentioned - the object linked to a contact who doesn't have an owner, though there isn't a clear description of what this should have been previously. It just makes sense to fix. And your fix of course does work! I'll remove the strict equality check as $obj_owner can apparently be a string in some situations. Commit added here: 0e8fe5b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find an obvious fix for this sniff issue. This is what I was seeing:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes this old code makes the linter go haywire, I see no problem in disabling the error for this line.