create config setting to control if roles are disabled when a case is closed

added comp:CiviCase ~88 needs:concept approval labels

@colemanw @jamienovick1 can you both weigh in on the above? thx.

I agree I'd like to be able to see the roles on closed cases. Just see also #1401 (closed)

I remember there were a couple tangled things, where just not ending roles then causes something else to not work. There was a proposal mentioned in that ticket which is probably still valid.

mentioned in issue #1401 (closed)

@lcdweb @DaveD

We've been looking at something similar for one of our clients, but in the context of the new cases extension: https://github.com/compucorp/uk.co.compucorp.civicase

So my view is that the problem is not so much that we end the relationship, but more that we don't display the disabled / ended relationships on the case summary screen in the same way we do for relationships on a contact. After all, it might be relevant to know who was involved previously in a case, as well as who is currently involved.

Our plan is to expose both current and expired memberships on this screen in the new cases extension, making it clear which have ended, so that you can see people who were previously involved in the case also and to basically give a similar amount of edit functionality to the roles as you do to cases (including editing the start and end dates).

I think that might be preferable than an additional setting? But if not then I don't see a problem with the setting from a user experience perspective as long as the default behavior is unchanged.

I guess the question then arises about how the setting would work? Would the business logic for the setting be in the form layer or baked into the API or somewhere else?

The reason I ask is that we're really like to make sure that these sorts of settings are respected, no matter where in the application the user interacts and we're facing a few issues at the moment where webform has it's own "business logic" for how it deals with some case interactions as it has different API calls to some of the Civi forms. I guess you could have similar issues with Civirules changing a case status to closed etc.

Best

Our plan is to expose both current and expired memberships on this screen in the new cases extension, making it clear which have ended

I think I personally like that, and it would untangle at least the manage case part from the other issues. (I assume memberships is just a typo - civi has too many things!)

I'm thinking for ACTIVE CASES though somebody will want to be able to hide the past roles, or make it the default that they're hidden. You do need to be able to quickly see who the CURRENT role is on open cases without the clutter of past roles. As you've mentioned the style of the regular contact relationships tab isn't bad. Or a collapsed section might save screen real estate.

Out-of-scope but on-topic, I'd like to some day combine the Roles and Other Relationships on manage case, and so similarly just visually indicate which are which but all visible at the same time.

some thoughts/feedback:

I think there are really two questions here: first, what should we do with relationships (if anything) when a case is closed. this is a data questions. and secondly, what should we display given the different data scenarios (active, inactive, post end date, etc)
My proposal deals with the first issue. in so doing, it largely solves the second issue -- for those who would enable the setting that prevents relationships from being disabled on close.
personally, I think the data issue (should relationships have an end date stored) is bigger than just the display. I think there are many reasons why an org would not want that behavior to kick in -- apart from the display problems.
re: the code -- the current behavior is handled in the form layer, not the API. see: https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/Form/Activity/ChangeCaseStatus.php#L158 -- I would just wrap that in a conditional based on the setting. pretty simple modification.
I think the suggestions to optionally display disabled relationships, or display them in a separate panel, makes sense. I kind of like the UI suggestion in this PR (https://github.com/civicrm/civicrm-core/pull/13510) to display/hide via a checkbox. but a separate panel would be fine.
the one piece of functionality that I wouldn't want to disrupt is how if you edit an existing assigned role and change the assignee, it disables the previous relationship and creates the new one. roles are kind of a special version of relationships, and I think this concept that a case may have a single assigned role of a certain type that is switched out at times, is important to preserve. (I know you can add new roles, but the current model presents it in the most commonly used way, IMHO.)

re: the code -- the current behavior is handled in the form layer, not the API. see: https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/Form/Activity/ChangeCaseStatus.php#L158 -- I would just wrap that in a conditional based on the setting. pretty simple modification.

This is exactly the problem. We have business logic in the form layer, and we are going to add more business logic there; when of course the ultimate aim is to remove the existing form layer. It's really not sustainable to keep adding more business logic to forms when we know we want to a) replace the forms b) also use webform / caldera forms for integration.

I'm not a developer, but I do think any business logic should be in the backend and perhaps we should attempt to move the rest of the logic there also? @eileen might have a view on this point.

the one piece of functionality that I wouldn't want to disrupt is how if you edit an existing assigned role and change the assignee, it disables the previous relationship and creates the new one. roles are kind of a special version of relationships, and I think this concept that a case may have a single assigned role of a certain type that is switched out at times, is important to preserve. (I know you can add new roles, but the current model presents it in the most commonly used way, IMHO.)

Agree that this should not be changed. Note that currently you can also add a contact with the same case role to have 2, which is somewhat problematic. In new cases we are adding a setting so that you can select whether this is allowed or not. The business logic for that will be in the backend so that Webform respects the

So I haven't gone into the detail too much on this but in general I do agree with

getting the business logic out of the form layer
ensuring that any new behaviour controlled by a setting is covered by a unit test (since they are particularly different to prevent breakage on).

re: business logic -- so where do we think this should live? a "changeStatus()" method in the Case BAO? or are we suggesting this should be baked right into the case API (i.e. Case_BAO::create)? I'm not crazy about that -- I feel like the API should remain simple, and largely oriented around CRUD operations. The form (to some degree) and BAO layers are where business logic should reside.

@DaveD can you comment? in general I would say BAO::create makes sense but I can't comment to this specific case

This has kind of become a couple different topics so I wanted to read over the comments here again and review what the issues in #1401 (closed) were again.

I haven't forgotten about this just would like a few more days.

I've done a pretty deep dive on this and the short version is leaving end_date null when a case closes does solve several items, even though it isn't the root issue, and this was also the proposal elsewhere. Most places in CiviCase seem to have already effectively changed the definition of case role so that it doesn't involve relationship.end_date. So mostly I'm saying just remove those lines in the form, with no setting. I've made a fancy table farther below, and here's the longer version:

Facts

Even with the proposed setting, a separate fix would still be needed for the manage case screen for closed cases for the people who don't turn it on.
That separate fix would also need to work with the setting checked.
By making it a UI setting, Civi is declaring that it will support both definitions of a case role, and people will expect screens, api, reports, etc to have appropriate output for both definitions.
Because of the implementation history for the feature to track multiple people in a role over time, including "tweaks" for the parts of CiviCase that no longer worked, CiviCase is currently inconsistent with the rest of Civi with regards to the civicrm_relationship.is_active field. It uses it in a different way, and except for the manage case roles section, CiviCase is currently dependent on is_active=1 for case roles for everywhere else to work. This also means CiviCase is currently incompatible with the scheduled job that syncs this field to end_date.
Nobody has stated anywhere either on SE or github or lab that they want the manage case screen for closed cases to continue not showing the roles.

Analysis

One option would be for the feature that tracks multiple people in a role over time to get a rewrite, which means changing most places in CiviCase and revisiting the use of is_active. I don't think anyone has the appetite for that right now.
- It would involve using effective-dating queries and something new other than is_active to differentiate a previous role assignee from when you have multiple concurrent people in the same role. (Example: if you're using cases to model events and are tracking participants as roles, several people will be in the attendee role at the same time.)
Another, and the one which matches what you're proposing but without a setting, is to just change the definition of case role to be more like regular relationships and leave end_date null when the case closes. This was the proposal elsewhere, which scared me at first but I'm over it now if I just accept the existence of is_active and how it has ended up being entrenched.
- This requires a small change in api case.create (_civicrm_api3_case_create_xmlProcessor) as well as removing those lines in the form.
- Also can remove the lines that reopen the relationship when a case reopens.
- Also an upgrade script, something like (untested) UPDATE civicrm_relationship r INNER JOIN civicrm_case c ON c.id = r.case_id SET r.end_date = NULL WHERE c.end_date IS NOT NULL AND r.is_active = 1 AND r.end_date IS NOT NULL;
- The only place I can see it affecting is something like a relationship CiviReport that uses end_date, except you can't filter cases easily in that civireport and it doesn't have any case columns, so it's not really a case-related report. Possibly a fancy Advanced Search might be affected. If that does cause troubles, a simple join on civicrm_case to look at case.end_date to see if the case is closed would allow you to tell if the relationship was "truly ended" - not too different from how you exclude activities when the corresponding case is in the trash.
- @jamienovick1 I think this satisfies your use-cases too, where there's no new decision tree being added, and you can still visually display all the relationships similar to the contact relationship tab.

Table

I don't really expect anyone to read this and it's just for reference supplementing my train of thought. The last column mostly just shows how you can't use civicase and the scheduled job at the same time.

Place	With end date when case closes	With no end date when case closes	With no end date when case closes - Multiple people in role over time	With end date when case closes - Using scheduled job
Manage Case	Doesn't display	Displays	Displays properly	Doesn't display
Reporting on Relationships	Effective dating	Need to additionally look up case end date to get effective dating	Need to additionally look up case end date to get effective dating	Depends what you're using
Case Dashboard for My Cases
- case counts	ok	ok	ok	wrong
- case manager	ok	ok	ok	missing (for all cases - case not even listed for my cases)
- listed	ok	ok	ok	not listed
Dashlet for My Cases	ditto	ditto	ditto	ditto
Cases tab on a contact	Case manager displays	Case manager displays	Case manager displays	Case manager is missing
Find Cases for closed cases	Case manager displays	Case manager displays	Case manager displays	Case manager is missing
Advanced Search - relationship	maybe a bit broken already	since definition has changed it depends what you're searching on	since definition has changed it depends what you're searching on	maybe a bit broken already
Print Report	kind of broken already	kind of broken already	kind of broken already	missing
api - case - get	Ignores end date	Ignores end date	Ignores end date	missing
api/BAO to close case	ok	The api will automatically set the relationship end date if you create a case with status closed (see _civicrm_api3_case_create_xmlProcessor)		ok
api - relationship - create	ok	ok	ok
Reopening a case	ok-ish	ok but can remove these lines too	ok but can remove these lines too

Impressively thorough analysis! Can we give this a concept approval tag and I'll move forward with the simple approach to the patch, as outlined?

So I'm mostly OK to concept approve based on @DaveD being subject matter expert in CiviCase - but maybe leave a couple of days for @jamienovick1 to weigh in

mentioned in issue #2378 (closed)

mentioned in commit f124dfae

changed milestone to %5.40.0

Going to close as an alternative PR https://github.com/civicrm/civicrm-core/pull/19737 has been merged. Might be worth revisiting someday but can reopen then.

closed

I'd like to see this revisited (seeking concept approval). The PR that toggles the display of inactive roles is useful regardless, but not the same as what is proposed here. I still think forcibly disabling roles upon closing should not be assumed as the universally preferred workflow. We can pretty easily create a setting to control this and condition that behavior accordingly.