CRM-6341 Only the first 50 groups can appear in "Manage Groups" if ACLs are used

Patch that fixes the issue. Looks like a trivial fix, and influence should be very localized.

Here we are building 'Limit' clause dynamically.
So could you please try to replicate on http://drupal.demo.civicrm.org/

thanks.

> Here we are building 'Limit' clause dynamically.

Yes, and THAT'S the bug. You should not be. That query should give you the universe of all groups available on that domain IIRC. LIMITing that query means that anything at the end of the query past the LIMIT will never appear in Manage Groups for ANY user, whatever their ACL configuration is.

> So could you please try to replicate on http://drupal.demo.civicrm.org/

Hard to demo. If the lmit is N (it's 50 on my system), that means that I need to create N + x groups on the system so you can see that the last x groups alphabetically never appear in Manage Groups. Unless I can mass add groups, that's a lot of work to validate a one-line patch.

this is a bug. we need to get the total groups that the user can see before we make the call. currently the permissioning is happening AFTER the query which is wrong

I've revised the patch to take into account the pager. This works on my target system.

My one question here is what we need to do if a system (1) implements ACLs, but (2) also uses the "edit all groups" priv for some users. I'm not sure how this code needs to look to handle both cases correctly. I suspect that in (2), we need to "short circuit" the ACL check, but I'm not sure. I'm assuming Lobo can answer this question, however.

Revised patch as per previous comment.

New version of the patch. I've added a check for the 'edit all contacts' privilege, so that we short-circuit the ACL logic in this case.

I'm not sure when it makes sense to use 'edit all contacts' vs. 'view all contacts', but hopefully the Civi-Folks will have a better idea of this.

Rob - you are my hero - I could absolutely not figure out why our NSW users saw no groups at all until I started trolling IRC & found this.

The patch works on our site which is 3.2.5

These 448 issues have not been worked on for the past 18 months.

Doing a bulk close of old issues to make the issue queue more manageable. We should do this on a periodic basis.

Rob - thanks for the patch. I tested on a 4.0.7 site and it works correctly both for ACL'd users and users with "edit all contacts" perm. My guess would have been that both "edit all contacts" and "view all contacts" perms are relevant, as IIRC either will circumvent contact & group ACLs, however I tested as a user with "view all contacts" but not "edit all contacts" and this worked correctly too.

Eileen - have you tested on other versions in the meantime? All OK?

Core team - any reason not to reopen this issue & review/commit the patch?

Cheers, Dave J

eileen, dave:

can u'll agree on this patch and if so commit for 4.3 please

thanx

lobo

This is obviously pretty old now & I can't remember if it's running on our site still - but it looks fundamentally sensible to me - I think that the CRM_ACL_API::group function might return early / have sensible handling if the edit all contacts permission exists so the if-then handling may be unnecessary.

I think we should commit this fix

OK - so I tried to look at applying this & the code has been heavily rewritten - it appears to me this is probably fixed in trunk as a result of one of these. (It's not possible to apply the patch as the surrounding code has been completely changed)

http://issues.civicrm.org/jira/browse/CRM-9497
http://issues.civicrm.org/jira/browse/CRM-9155

Per discussions with Dave J - the code has 'moved' on since this was logged & the patch (& hopefully the problem) no longer seem to apply