CRM-20441 further fixes & tests for api + acl

[eileenmcnaughton]

• CRM-20441: Fatal error on contact summary for ACL'd user (from activity tab count)

[eileenmcnaughton]

This is not quite there yet - but I think the trick is here
https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:4.7.19-rc2?expand=1#diff-7673c7d055caeab2a3ec5d05a1553293R328

We just need to get the ACL adder to add a permissioned join onto ACL_Contact - pretty much whenever

[eileenmcnaughton]

@seamuslee001 @colemanw @monishdeb @davejenx

I think this is the right fix for now.....

If moves all ACL checking to the api & it checks all rows if permissions are set. This is an expansion of our api application. I think we could do better next round by doing a join check rather than row-by-row & I have commented where I see us adding it.

There is a performance cost in some cases of doing this check - we should keep improving on that, however, it might not have much impact since previously we were blocking if id was not set or contact_id was set, now we are checking those. We were already checking in the other scenarios.

Also, the api no longer throws exceptions when permissions checks fail on a per-activity basis - that is correct from an api POV but we should do a click-around.

@colemanw I couldn't figure out how to force an ACL'd join on the contact_id table, so fell back to the row-by-row check. I would like to know how, although probably we should change all places that call the check function to call the api first, then optimise it within the api

[eileenmcnaughton]

Per discussion in JIRA I have changed the behaviour on 2 tests - turns out one was written by Seamus & one by me, in order to set a baseline for changing behaviour

$params['id'] now supports NOT IN etc & test altered to reflect. 'view all activities' is not required when $params['id'] is empty. Instead there is a post-filter

[eileenmcnaughton]

@seamuslee001 what do you think? Merge this now? I'm inclined to on the basis it

1. extends test coverage
2. gets existing tests passing
3. has a negative score DESPITE adding tests
4. will mean the rc has our latest code merged into the rc & master for review by @davejenx & those other hoards!

[seamuslee001]

Agreed

[davejenx]

Reverted my cherry-picking for the previous PRs, rebased to a clean 4.7.19-rc and checked that #10251 was in git log. Then re-tested. Success: this fix also worked...

Fixes the fatal error. Contact summary now displays, with Activities tab showing count = 0.

As a further test, I tried adding an activity to one of the allowed contacts that didn't have any activities. I set source = one of the allowed contacts, target = another of the allowed contacts, no assignee. So that should be visible to the restricted user - and it is: count = 1 on Activities tab, can view activity.

Hooray! Have commented on CRM-20441.