dev/core#641: Case Activity Assignment Restriction

[tunbola]

Overview

Currently, a user can create and assign an activity to any CiviCRM contact. This in turn sends an email notification to the assignee. However, some users have reported that they mistakenly assigned activities to contacts with similar names which meant that someone who should not have access to any details of the activity got a link in the email stating some basic details of the activity.
This PR adds the ability to restrict the assignment of case activities to a group. A user will not be able to assign activities to contacts that does not belong to the restricted groups.
Activity assignment can also be restricted to only contacts having user accounts.

Before

It is not possible to restrict the assignment of case activities to a group or to contacts having user accounts.

After

It is now possible to restrict the assignment of case activities to a group or to contacts having user accounts.

When searching for a contact to assign for an activity, it displays only people assigned to that group - if that case type has group restriction, or it displays only contacts with user accounts if that case type has restriction to only website users. Also the option to create a new contact when that activity is associated with a case type which is restricted to a group or to only website users is removed.

Technical Details

Two new parameters RestrictActivityAsgmt and ActivityAsgmtGrps added for this task were simply appended to the existing xml data of the definition column for a case type.

The buildQuickForm function of the CRM_Case_Form_Activity form class was modified to restrict the assignee related fields to only fetch contacts associated with restricted groups for case types with such restrictions or to restrict assignee related fields to only fetch contacts with user accounts when the case type is restricted to website users.

    if ($this->restrictAssignmentByUserAccount()) {
      $assigneeParameters = ['uf_user' => 1];
    }
    else {
      $activityAssignmentGroups = $this->getActivityAssignmentGroups();
      if (!empty($activityAssignmentGroups)) {
        $assigneeParameters = ['group' => ['IN' => $activityAssignmentGroups]];
      }
    }

Comments

PR to update user guide here: civicrm/civicrm-user-guide#375

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Thanks for pushing this change. I think it's useful.
As I think I mentioned in our chat, the schema update really isn't necessary, as the definition column contains an open-ended json blob that can easily be appended to.

[tunbola]

Thanks @colemanw, didn't catch that in the chat. Will take a quick look at what you suggested now.

[colemanw]

Another thing I think I mentioned in the chat is you can restrict an entityRef to only lookup contacts who have a user account. That alone could prevent a lot of mistakes.

[tunbola]

@colemanw , I updated the PR, can you take a look again?

[marcelocompucorp]

Hi @colemanw , just a kind follow up regarding the last update from @tunbola . Can you please check? Tks.

[mattwire]

I like the idea of this. But I have a couple of concerns / thoughts about the implementation that I think we need input from a few people:

• Could this be implemented by extension (using either new or existing hooks)?
• There is existing "ACL" access control in CiviCRM which can be used to restrict access by group. This PR is implementing a different way of controlling access - could we use/enhance the existing methods.
• There is a need for "improved" access control/restrictions around activities in general, ie. restricting activity types to certain groups etc. and any work in this area would be good to think about how it could help that in the future.
• How will this PR work with the Activity/Case API functions? Where possible we want to keep the "business logic" away from the form layer so that the forms can be replaced by alternative user interfaces in the future (eg. mobile app, webform_civicrm, civicase extension).

Not sure who to tag as an extensive cases user... @lcdservices @guanhuan @JoeMurray ?

[lcdservices]

I'm a strong +1 on this. I have had to implement this functionality many times via hooks, but I think it deserves to be in core. Re: @mattwire 's items:

• yes, it can be done in an extension, but I think it's useful enough and would be widely used enough to deserve to be in core
• the goal of this is to restrict how the assignment field is used -- not restrict access to contacts (which is what the ACL feature does). the use case is that an org only wants case activities to be assignable to staff -- so they construct a staff group and apply this config setting.
• I agree that activities would benefit from improved ACL. but this feature is not an ACL function at all. we're not restricting access to any records with it. we're just filtering the assignee selection field to limit it by a group. that's not access related. it is a business workflow function.
• I agree this should not impact the API. it looks to me like this is only impacting the form layer, so I don't think that's an issue.

[shitijg]

+1 on the replies by @lcdservices

I would agree to the idea of having this as an extension, but the fact that it's got the current permissions as "default" - I would say that it warrants itself to be in core.

Also agree with the ACL part ie this is just filtering on the form level who you can assign the activity to.

[colemanw]

I would suggest making this function more generic - rename it to restrictAssignment and have it return the value of restrictActivityAsgmt (0, 1, or 2)

[colemanw]

You could save some space in the xml file and prevent errors by skipping this section if not needed. E.g. have the conditional on 197 read

if (!empty($definition['activityAsgmtGrps']) && $definition['restrictActivityAsgmt'] == 2) {

[colemanw]

I've tested this and it works 👍
I've also pushed up some additional fixups to the activity form logic for caseType and caseTypeDefinition.

I'm a little hung up on the UI and workflow. It seems to me that the 2 modes: "restrict to group" and "restrict to cms users" are not mutually exclusive, and perhaps instead of a 3-option radio button we want a checkbox for "restrict to cms users" followed by a dropdown for "restrict to group" - leaving them both blank would have no restrictions, and checking the box + selecting groups would apply both restrictions.

[shitijg]

@colemanw agree that these are not mutually exclusive.

Just trying to interpret your suggestion there - if we check the box + select a group - should it then filter this list to:

1. ALL contacts with a CMS login
   AND
2. ALL contacts within the selected group.

As far as I understand, there will still be an intersection of contacts somewhere ie contacts who belong to the selected group who have a CMS login ie 1 contact can meet 2 criteria.

The CMS login wasn't necessarily in our initial requirement - so maybe adding it is adding a layer of complexity? Perhaps we can leave it at the CRM contact level?

Thanks and let me know what you think.
Shitij

[marcelocompucorp]

Hi @colemanw , did you have time to check Shitij's comments? Tks

[colemanw]

@shitijg yes that's right. If you add both params to the api call then it will filter out non cms users and contacts who don't belong to the group.
From a code pov it's not that complex. It's not like you have to calculate the intersection yourself - just add 0, 1 or 2 params as specified by the case definition and the civi api handles the rest.

[tunbola]

@colemanw , Made changes and update screenshots.

[shitijg]

thanks @tunbola

@colemanw understand your point and Tunbola has made changes accordingly. Hope this is ok to go now?

Thanks

[colemanw]

• General standards
  • (r-explain) Pass
  • (r-user) Pass: Improves UX
  • (r-doc) Pass
  • (r-run) Pass: Works
• Developer standards
  • (r-tech) Pass
  • (r-code) Pass
  • (r-maint) Pass
  • (r-test) Pass

[tunbola]

@colemanw , seems the the check is failing because of some tests unrelated with this PR. Can you help take a look? Thanks

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@tunbola if you wish to re-run tests I think you can just comment test this please. If that doesn't work any change to your PR (e.g improving the commit message & force-pushing) will re-trigger it

[eileenmcnaughton]

test this please