Restrict case roles by group

[colemanw]

Overview

Allows an admin to limit selection of contacts to certain groups when adding case roles.

Before

Any contact can be selected for a case role.

After

Each case role selector can be limited to one or more group.

Doing so will affect the contact selectors for add/edit role on the case summary screen. Those selectors will also have the "New Contact" buttons disabled.

Technical Details

Stores group by name in order to be xml-friendly. This also changes the activityAsgmtGrps setting to store group name instead of id (although it will still work with previously stored ids for backward-compat).

[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).

[demeritcowboy]

Hi @colemanw I can review this week. I might ask about the groups name/id part, but I don't have it in sensible english-y words yet. At the moment it goes something like "managed entities blah blah name xml managed entities".

[colemanw]

@demeritcowboy you make an eloquent point there 😁 but I'm not sure how this would affect managed entities. I just picked "name" as the key in the xml because everything else in the xml is keyed by name so I figure groups should be as well.

[demeritcowboy]

My thinking is like so:

Unless there was a request to have groups specifiable by name in external xml files, it's not necessary/useful because:

• The existing groups-related feature for the case type was introduced long after the case type editing UI became available, and for people using the UI it's irrelevant to them how that setting gets stored.
• The use of name in the case xml context was intended to be about portability between installs, where you could take the xml file and plop it onto another site and have it work. Not necessarily dev->staging->production, but between two separate unrelated sites where ids might not match up.
• It was maybe never fully utilized, and maybe no longer fully works, but in theory managed entities combined with case type forking mostly facilitated it. There's a separate discussion about import/export, I'm just getting at why managed entities is relevant to the use of name.
• For that to work with groups the same way, it would require managed entities to manage groups, which I don't think they do?

Hence the managed entities reference in my pseudoglish (TM) earlier, which would now be more like "managed entities name xml relationship types currently yes, groups currently no have managed entities".

[colemanw]

Not sure it has much benefit either, but in general I think storing names is better than storing ids for portability. Sometimes you create groups on a staging site and they end up with a different id than the ones on the live site. Also @lcdservices requested that groups be by name in the xml and he's the one who commissioned this feature :)

[demeritcowboy]

Ok so there was a request - got it.

[lcdservices]

@colemanw tested and this is working as expected. Can you detail how these params would be handled in the XML file (vs. via the UI)? I tried adding <activityAsgmtGrps>Office_Staff</activityAsgmtGrps> to the top level of the XML file (same level as name) but it's not picking it up. I'm assuming <groups>ABC</groups> should be within the RelationshipType branch? And would multiple values be comma separated, or handled in sub-elements (e.g. <group></group>)?

[lcdservices]

@colemanw based on how the xml is constructed when the case type is configured in the UI, it should look like this:

<ActivityAsgmtGrps>
  <Group>Office_Staff</Group>
</ActivityAsgmtGrps>

However, when I add that to our XML file and load the case config via the UI, it does not populate with the group. It does filter in the case activity form. So it's parsing the XML and applying the filter correctly. But I think we should ensure the UI accurately reflects the XML params.

The case role groups are a single tag <groups>Office_Staff</groups> within the RelationshipType tag. So I'm assuming multiple values would be comma-separated. Am in the process of testing whether that loads via XML properly.

[lcdservices]

I've confirmed that the role filter is working properly when configured via the XML file. However, as with the activity field, the values do not preload when viewing an XML-based config in the UI.

[demeritcowboy]

@lcdservices Is this a case type you've already "forked"? When you make changes in the xml after forking they are disconnected. I've found that confusing but it's always been that way I think. It displays correctly for me in the UI when I add a <groups> tag to the xml for an unforked case type.

I have a couple minor things but I'll wait until you guys finish first.

[lcdservices]

@demeritcowboy no, it's not forked. We're running off the XML with no modification. But if someone did want to fork it, I would expect the full XML config to load in the UI as their starting point.

Other alterations to the XML are reflected in the UI.

[colemanw]

@lcdservices can you show me your xml file?

[demeritcowboy]

If it helps I was noticing that if the group has is_hidden=1 or is_active=0 in civicrm_group it behaves the way you're seeing, where it DOES get picked up for filtering on manage case popups, and other xml file changes get reflected in the UI on the case type edit page, but you don't see the group on the case type edit page.

[colemanw]

@demeritcowboy have you had a chance to r-run this yet? I'm interested to know if you hit any problems with it like @lcdservices did.

[demeritcowboy]

Did that get resolved? Otherwise the main thing I saw is a visual weirdness. See below.

• General standards
  • [r-explain] PASS
    • A clarification that this is "restrict" as in "as a convenience to the user the UI only offers relevant choices", not restrict as in "a setting that prevents assigning / an ACL-like setting". I don't see it as a security issue here but you can easily assign any contact to the role from the manage case screen even with this setting.
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] Mostly PASS
    • On both firefox and chrome there's a visual weirdness on the case type edit screen for the new dropdown when you add a role to the table. The box is a different size than the boxes already in the table.
    • Also tested adding a role from the manage case screen using the add role button (so not from the roles in the table already). It correctly restricts if you choose a relationship type that was in the restricted list in the case type definition.
      • It DOES let you pick any contact if you add the same relationship type from the Other Relationships section on manage case, but I don't think that's a concern here.
    • I can see the groups feature was like this before so just mentioning for reference:
      • If you had an existing group set for the role, and then disable the group, then later when you go to edit the case type the row has a blank. Then saving removes the group from the config. So it depends on what's expected if you were to later re-enable the group. I know there's different opinions on what disabled means.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] Undecided
  • [r-test] PASS
    • I have some tests pending in another PR that test the xml although don't really test the groups, but for the record this passes those tests.

[mlutfy]

I have to admit that I do not fully understand the discussion.

• Is there anything blocking the merge?
• Since it's a new feature, shouldn't it require a test?

[eileenmcnaughton]

@colemanw @demeritcowboy ping - what is the status?

[demeritcowboy]

Is there anything blocking the merge?

• There was an unresolved (as far as I know) issue for @lcdservices that may or may not be a local issue. I was only able to reproduce it in the situation described at Restrict case roles by group #15570 (comment). But I don't know if that was the reason for it for him.
• There are some possible issues noted in the review that I can't say if they matter or not. It depends on what the requirements were, so it's for @colemanw and @lcdservices to say if those issues matter.

Since it's a new feature, shouldn't it require a test?

I would agree.

[lcdservices]

The issue with the group filter loading in the UI is specific to our site. I implemented the PR on a clean install and it loads in the UI as expected.

I reviewed the discussion and don't see any other items to discuss. There was discussion about the use of the group name in the xml file, but that was affirmed. It was a specific request because it aids in portability.

@colemanw can you do a test for this, and then we can get it merged?

[colemanw]

Thanks everyone for the review. Based on testing by @lcdservices I think the only thing missing from this was a unit test, which I've added. So I believe this can be merged once that passes.

[demeritcowboy]

@colemanw Am curious why the last minute change in line 170 in BAO/CaseType.php? c4bfbde#diff-f9628269feadc01ddab8f94b9be7adb9

[colemanw]

@demeritcowboy I added the (array) casting so that field will always be stored as an array even if a single value is passed.

[demeritcowboy]

I see. I was thinking more about the original is_array() instead of $key == 'groups' part, but I'll go off and think about it separately.

[colemanw]

Well currently only groups is allowed to be an array, so this way we force it to be an array rather than relying on how it's input.