CRM-19494 Refactoring of permission code #9246
Conversation
|
test this please |
Oh nooo, sorry. Seems I got sloppy on the last 5% :-/ I'm pretty much out for the weekend, but I'll fix this on Monday. Thanks @eileenmcnaughton, enjoy the rest of the sprint! |
|
BTW, the old PR is #9229, not 9242 |
eileenmcnaughton
force-pushed
the
bjorn
branch
from
8f3528a
to
5c36900
Compare
eileenmcnaughton
force-pushed
the
bjorn
branch
2 times, most recently
from
f88e0e2
to
8b183c7
Compare
|
@systopia I feel fairly good about this now. I did some further clean up on top & seeing the lack of regression from that made me feel better about it all. I did fix a few things. Back to you to re-review but I think this can probably be merged |
|
@systopia: how does this play with https://issues.civicrm.org/jira/browse/CRM-19256 ? |
|
@nganivet I think I mis-referred to that ticket at some point - there should be no connection between the 2 |
|
@systopia This PR is just waiting on you to reconfirm the changes I made to your PR.... |
obeying master jenkins
- style - extra test - documentation - FIXME remarks
Also, index rows by id so array_keys is useful
Change-Id: Ic975b736eae4edf145636b01cb6e618bbfc0ab70
eileenmcnaughton
force-pushed
the
bjorn
branch
from
9e8fff4
to
0c50c1d
Compare
…iew my contact Conflicts: CRM/Contact/BAO/Contact/Permission.php
eileenmcnaughton
force-pushed
the
bjorn
branch
from
0c50c1d
to
9aea8e1
Compare
| @@ -140,8 +140,8 @@ public static function allow($id, $type = CRM_Core_Permission::VIEW) { | |||
| $contactID = CRM_Core_Session::getLoggedInContactID(); | |||
|
|
|||
| // first: check if contact is trying to view own contact | |||
| if ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') | |||
| || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact') | |||
| if ($contactID == $id && ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') | |||
There was a problem hiding this comment.
Sorry, thanks for spotting this.
|
@eileenmcnaughton, Sorry for the delay. The changes all look sensible to me, but I couldn't test everything in detail. I ran the |
|
OK let's merge it - I did test it quite a bit locally & we can test more once merged. |
@systopia Do you want to look at this as a proposed replacement for your PR #9242 - Your PR is good except for a couple of things
:-)
Basically you are assuming an id-indexed list of contact ids is passed in & that is not the case (issue 1). After making them id-indexed it turns out the links function goes & bashes the carefully constructed link list out of the water.
Re performance - I'm feeling OK-ish about this but there is a problem that it actually gets called twice rather than once when creating a form. I think session caching is probably the only way to avoid this. I've added comments about that.