[REF] Be able to remove clients whom are not the primary client of th… #24841

seamuslee001 · 2022-10-28T03:32:39Z

…e case from manage case view

Overview

This adds in the ability to remove a non primary case client from being linked to a case from the manage case screen

Before

No way to remove a case client

After

Remove a case client possible if not primary. If Primary can use the re-assign case

ping @lcdservices

civibot · 2022-10-28T03:32:42Z

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

lcdservices · 2022-10-28T12:18:24Z

@seamuslee001 looks good. The additional constituent(s) now have an icon next to their name that allows you to remove them from the case.

demeritcowboy · 2022-10-28T13:00:51Z

CRM/Case/Form/CaseView.php

+      'activity_type_id' => 'Open Case',
+      'return' => ['assignee_contact_id', 'source_contact_id', 'target_contact_id'],
+      'sequential' => 1,
+    ])['values'][0]['target_contact_id'][0]);


This gives a warning Warning: Undefined array key 0 in CRM_Case_Form_CaseView->preProcess() (line 63 when there is no open case in the config. It's not a common configuration and not recommended but I've seen it out in the wild and we know from chat threads it exists. There is CRM_Case_BAO_Case::getCaseClients?

@demeritcowboy that wouldn't tell me which of the n case clients has the activities of the case set to them right?

Maybe we need a clearer description of what the user use-cases are and why the need for the concept of Primary. In the most general case I don't see why someone can't remove ANY of the clients as long as you can't remove the "last" one (when there's only one left/existing). If there's a finer requirement for "don't let me remove one where there's an activity where they are the target", then we should spell that out (and unit test 😈).

@demeritcowboy don't mention that u word :-P, so basically Brian came to me with a problem that you couldn't remove the Additional Clients when you have multiple clients.

I found that you could remove the 'primary' one i.e. the one with the activities attached to it by using the EditClient form (which should really be call re-assign case but anywho) What that does do is re-builds the case with essentially a new 'primary' case contact i.e. the one that is the target on the activities.

So I was thinking we would need some difference between the other cases clients i.e. the ones that don't have any activities linked to them and the one that has the activities linked to them you know.

The EditClient/Reassign isn't tied to activities, and there is no real concept of "primary". It depends on what url you followed to get to Manage Case. The cid in the url can be any of the client ids. If you come from one of their contact Case tabs then the reassign button will reassign that person, completely separate from what is on the activities.

So it sounds like the biz requirement is just to be able to remove any of the clients. So the only restriction/lookup would be that you shouldn't end up with a case with 0 clients, i.e. the "x" shouldn't be shown if there is only one client. And ideally the preprocess() for the delete form would bounce if you visit a crafted url directly and there is only one client.

@demeritcowboy so there will be one client that is attached to various activities based on this https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/Form/Activity/OpenCase.php#L214 / https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/Form/Activity/OpenCase.php#L294 so what happens if we decide to view one of the other clients and then go to remove the one that was originally linked to the activities should this form just move those activities to the currently viewed contact?

I suppose that could be an optional checkbox option, but it would depend on the given install and what cases represent in that install and what multiple clients means in that install, and why they've chosen to remove that client, and whether it makes sense in that situation to "change history". In general it's perfectly fine to have activities on a case with any target.

Since I'm the one who usually overthinks things, it feels weird for me to type that maybe this PR doesn't need that much brainwave activity, although I appreciate it.

Everything that's been said so far suggests it really just needs a way to remove clients, but don't end up with 0 clients.

I agree. I don't think we should automatically relink activities. This should be kept simple and straightforward.

demeritcowboy · 2022-10-28T13:01:52Z

CRM/Case/Form/DeleteClient.php

+      'api.case_contact.delete' => ['id' => "\$value.id"],
+    ]);
+
+    $pirmaryCaseContact = civicrm_api3('Activity', 'get', [


demeritcowboy · 2022-10-28T13:10:02Z

templates/CRM/Case/Form/DeleteClient.tpl

+{* template for assigning the current case to another client*}
+<div class="crm-block crm-form-block crm-case-editclient-form-block">
+  <div class="messages status no-popup">
+    {icon icon="fa-info-circle"}{/icon} {ts 1=$currentClientName 2=$id}Remove Client %1 from case %2{/ts}


Suggested change

{icon icon="fa-info-circle"}{/icon} {ts 1=$currentClientName 2=$id}Remove Client %1 from case %2{/ts}

{icon icon="fa-info-circle"}{/icon} {ts 1=$currentClientName|escape 2=$id}Remove Client %1 from case %2{/ts}

demeritcowboy · 2022-10-28T13:27:38Z

I think ideally it would add a "Removed client from case" activity the same as when you add. And/or the name of the activity should be "change case client" and then use the same activity type. I don't know if I'd call it a blocker though since the existing one is kind of useless since it doesn't say who was added.

Also fyi see #24843 that came up.

lcdservices · 2022-11-01T14:10:07Z

chiming in:

I'm not crazy about the idea of a "primary" client, because that's not a concept used anywhere else. I understand the need to retain one contact, but would prefer all attached contacts have the option to remove them unless there is only one contact, in which case the reassign case tool should be used. as it stands, if you first attach the case to contact A, then add contact B, and want to remove contact A, you have to remove B and then reassign A to B -- which seems cumbersome and unnecessary.
I do like the idea of tracking the change with an activity.

seamuslee001 · 2022-11-14T23:22:38Z

@lcdservices @demeritcowboy I believe I have addressed all comments now can you please review again

demeritcowboy · 2022-11-15T05:53:55Z

There's an extra "county" file in here. Rebase woes I assume.

seamuslee001 · 2022-11-15T05:55:21Z

@demeritcowboy sorted

lcdservices · 2022-11-15T14:03:22Z

@seamuslee001 I'm still only seeing the option to remove a contact on all but one contact (i.e. the idea of a "primary" contact is still incorrectly intact). See screenshot. If a case has > 1 contact, I should have an X by each contact. Once we're down to 1 contact, the X should be removed.

Two other things:

currently when you click to remove a contact, it redirects to a full screen confirmation form. any chance that can be handled in a modal popup rather than a full page redirect?
when on the confirmation form if you cancel it is redirecting you back to the contact record. it should return you to the manage case page

The activity is working correctly.

lcdservices · 2022-11-16T19:24:15Z

I tested the latest update. The X appears next to all case clients now, and the confirmation is via a modal popup. But the X also appears if there is only one client. If you click it, there is a popup that tells you that you cannot delete it as it's the only attached client (the wording/grammar needs improvement) -- but I feel like we should simply not display the X at all if count(clients) = 1. Why add an option + validation message when we can simply suppress the option to begin with?

demeritcowboy · 2022-11-16T22:16:34Z

CRM/Upgrade/Incremental/php/FiveFiftySeven.php

+      'description' => ts('Case client was removed from a case'),
+      'is_active' => TRUE,
+      'filter' => 1,
+      'component_id' => CRM_Core_Component::getComponentID('CiviCase'),


Administrivia:

This returns null if civicase is not enabled, so it would become a contact component.

Also none of the other activity types have filter=1.

…e case from manage case view Modify to allow any other than the currently viewed contact to be removed from a case and create activity when it is removed Remove restriction on link showing and convert to popup link Only show x if there is more than 2 clients Only Add in new activity type if CiviCase is enabled

lcdservices · 2022-11-18T11:57:29Z

CRM/Upgrade/Incremental/php/FiveFiftySeven.php

+  }
+
+  public static function addCaseClientRemovedActivity() {
+    if (CRM_Core_Component::isEnabled('CiviCase')) {


@seamuslee001 If I understand the previous note correctly, I think you need to do something a bit more nuanced. The above will prevent the activity type from being created if CiviCase is not enabled, which addresses the immediate concern. But if a person enables CiviCase at a later time, we need to make sure this activity is created. So perhaps we need to also add this code to the CiviCase enabling process?

In a stock install without civicase the case-y stuff is still always inserted into the db at install time, so we should always insert this one too. How about without the if and set it like:

'component_id' => CRM_Core_DAO::singleValueQuery("SELECT id FROM civicrm_component WHERE name='CiviCase'"),

demeritcowboy · 2023-03-30T04:15:57Z

I'm going to close this based on (a) conflicted files, and (b) one or two outstanding other issues. In the meantime the (admittedly awkward) workaround is visit Manage Case starting from the contact record that you want to remove (i.e. the cid= in the url is the one you want to remove).

colemanw · 2023-08-28T15:38:24Z

@seamuslee001 please rebase & reopen

seamuslee001 · 2023-08-30T00:13:30Z

I have re-opened this and rebased it against current master @lcdservices @demeritcowboy and I hopefully fixed up the upgrade step as well

seamuslee001 · 2023-08-30T00:14:31Z

actually looks like it made me create a new PR so that is here #27200

civibot bot added the master label Oct 28, 2022

demeritcowboy reviewed Oct 28, 2022

View reviewed changes

seamuslee001 force-pushed the case_remove_client branch from 30f8b98 to ded1760 Compare November 14, 2022 23:21

seamuslee001 force-pushed the case_remove_client branch from ded1760 to 19dee67 Compare November 15, 2022 05:55

seamuslee001 force-pushed the case_remove_client branch from 19dee67 to 24ae90b Compare November 15, 2022 06:02

seamuslee001 force-pushed the case_remove_client branch from 24ae90b to 48e630f Compare November 16, 2022 10:20

demeritcowboy reviewed Nov 16, 2022

View reviewed changes

seamuslee001 force-pushed the case_remove_client branch from 48e630f to 3766a9a Compare November 17, 2022 07:32

seamuslee001 force-pushed the case_remove_client branch from 3766a9a to 07a737c Compare November 18, 2022 05:39

lcdservices reviewed Nov 18, 2022

View reviewed changes

demeritcowboy closed this Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Be able to remove clients whom are not the primary client of th… #24841

[REF] Be able to remove clients whom are not the primary client of th… #24841

seamuslee001 commented Oct 28, 2022

civibot bot commented Oct 28, 2022

lcdservices commented Oct 28, 2022

demeritcowboy Oct 28, 2022

seamuslee001 Oct 28, 2022

demeritcowboy Oct 28, 2022

seamuslee001 Oct 29, 2022

demeritcowboy Oct 29, 2022

seamuslee001 Nov 7, 2022

demeritcowboy Nov 7, 2022

lcdservices Nov 7, 2022

demeritcowboy Oct 28, 2022

demeritcowboy Oct 28, 2022

demeritcowboy commented Oct 28, 2022

lcdservices commented Nov 1, 2022

seamuslee001 commented Nov 14, 2022

demeritcowboy commented Nov 15, 2022

seamuslee001 commented Nov 15, 2022

lcdservices commented Nov 15, 2022

lcdservices commented Nov 16, 2022

demeritcowboy Nov 16, 2022

lcdservices Nov 18, 2022

demeritcowboy Nov 18, 2022

demeritcowboy commented Mar 30, 2023

colemanw commented Aug 28, 2023

seamuslee001 commented Aug 30, 2023

seamuslee001 commented Aug 30, 2023

	{icon icon="fa-info-circle"}{/icon} {ts 1=$currentClientName 2=$id}Remove Client %1 from case %2{/ts}
	{icon icon="fa-info-circle"}{/icon} {ts 1=$currentClientName\|escape 2=$id}Remove Client %1 from case %2{/ts}

[REF] Be able to remove clients whom are not the primary client of th… #24841

[REF] Be able to remove clients whom are not the primary client of th… #24841

Conversation

seamuslee001 commented Oct 28, 2022

Overview

Before

After

civibot bot commented Oct 28, 2022

lcdservices commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Oct 28, 2022

lcdservices commented Nov 1, 2022

seamuslee001 commented Nov 14, 2022

demeritcowboy commented Nov 15, 2022

seamuslee001 commented Nov 15, 2022

lcdservices commented Nov 15, 2022

lcdservices commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Mar 30, 2023

colemanw commented Aug 28, 2023

seamuslee001 commented Aug 30, 2023

seamuslee001 commented Aug 30, 2023