dev/core#542 dev/core#1947 - Show inactive active case role for closed cases

[sunilpawar]

Overview

Currently when any case is closed , the all the roles on case contact marked to inactive but putting current date on their relationship record. there is no other way to find out who were these inactive role at-least through case manage screen.

Before

For closed case there was no way to find out Inactive role on case with different role type.

After

Closed case will show all roles (active and inactive) by default. if case manager want to see only active roles then checkbox will be available to see only

If any open case does not contain any inactive relationship then checkbox is invisible

Comments

I have reworked on #13510

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

[eileenmcnaughton]

@ray-wright @jitendrapurohit you were involved in the original PR any thoughts. Also pinging @demeritcowboy & @kainuk

[demeritcowboy]

I had started looking at it and it seems like a reasonable approach - just also wanted to see how it reconciles with https://lab.civicrm.org/dev/core/-/issues/1401 and https://lab.civicrm.org/dev/core/-/issues/1947#note_43363. Ping also @lcdservices .

[demeritcowboy]

Nice job overall. There are a couple issues below.

• General standards
  • [r-explain] PASS
  • [r-user] Issue
    • I agree with having different defaults depending on if you're viewing open or closed since 99% of the time case managers will be viewing open cases and the default should be to only show active roles when the case is open. There seems to be some mismatch though since the checkbox is unchecked suggesting it's showing all. There are several ways this screen could be redesigned but if going with a checkbox then the checkbox should match what it's showing. So I think it's almost right, but then see also r-run below since I think it's related.
    • There is some inconsistency with the Other Relationships section, but it's better than now.
  • [r-doc] PASS
  • [r-run] Issue
    • When you reassign a role, the table refreshes but appears inconsistent. And visiting any manage case page which has roles that have been reassigned has a mismatch between what it says it's initially showing and what it's showing.
• Developer standards
  • [r-tech] Issue
    • I've never seen the {html_checkboxes} thing before and it doesn't appear to be used anywhere else. Could it just be a regular <input> checkbox instead? And then you also wouldn't need name*= later since you would know the name. Also the field comes out looking scrunched because there's no whitespace between the checkbox and label using this method.
    • It's still maybe an open question as to whether the better long-term fix is leave the end_date null when cases close. I have some WIP on that but this approach with the checkbox works too.
  • [r-code] Issue
    • date("F d, Y" shouldn't be hardcoded. I guess can do it the same way as the case dashboard? CRM_Utils_Date::customFormat()
    • It's not clear why there's a smarty variable statusClass which converts Opened to 1. It seems harder to follow since there's only one place it gets used later on and it could just look at caseDetails.status_class.
    • $dao->free(); seems to be phased out. It's not used anymore.
    • Using array('cid' => $activekey) is out of style. People seem to want ['cid' => $activekey] syntax.
    • Some inconsistent use of .css('display','none') and .hide(). It's ok just I'd probably pick one. hide() and show() seem more common.
    • I would probably use {* *} comments in smarty instead of <!-- -->. Currently one of them doesn't make sense in the html output without being able to see the smarty code.
    • In getCaseManagerContact line 1767, it's not always going to return the recent manager. It might accidentally most of the time, but it's not guaranteed. But this might have been wrong before too so maybe out of scope.
  • [r-maint] ?
    • Without a test this will likely get broken, but I don't know how to test this easily.
  • [r-test] PASS

[lcdservices]

+1 for this. I like the idea, and agree with @demeritcowboy's assessment. I can do some testing after you work through the first round of feedback.

[kainuk]

Hi @sunilpawar, I created a core hack for a customer that has exactly this problem. I will test and ask him of it works in his situation.

[sunilpawar]

I will revise PR as suggested by @demeritcowboy

[demeritcowboy]

Hi @sunilpawar thanks for the updates. Wondering if you're still working on it? The default for open cases seems to have changed now - it should only be current role assignments for open cases since this is where case managers spend 99% of their time.

Also for the pager for example on the second page it says "Showing 11 to 4 of 4 entries" and only shows 1 page available, and now sorting by name seems to crash. The pager isn't a bad idea but it could be a different PR too.

[ray-wright]

Two things I'm now having trouble getting working

• The 'Show active relationships' being checked by default is not working for me. (In my original PR I had that reversed, I used 'show inactive relationships' instead, and the default behavior was unchecked. I'm not sure I have a huge opinion either way but perhaps it would make more sense to title the current checkbox 'Show only active relationships'.)

• I'm no longer seeing case managers listed on case listings (either case search or the case listing on a contact)

[eileenmcnaughton]

@sunilpawar are you still working on this?

[sunilpawar]

@sunilpawar are you still working on this?

yes @eileenmcnaughton

[sunilpawar]

@demeritcowboy i have fixed issue sorting on name column.

by default it shows all roles , disabled/inactive roles gray out. on clicking on checkbox 'Show active relationships' , it show only active roles (hide disabled/inactive roles rows).

[demeritcowboy]

Thanks will take a look!

[demeritcowboy]

One thing I notice right away is now I get the below when just opening the roles section on a case, and it causes the role name column to no longer be a hyperlink to the contact:

"Undefined index: name"

#0 CRM\Activity\Page\AJAX.php(215): "Undefined index: name", "CRM\Ac...", 215, (Array:19)
#1 CRM\Core\Invoke.php(279): CRM_Activity_Page_AJAX::getCaseRoles()
#2 CRM\Core\Invoke.php(69): CRM_Core_Invoke::runItem((Array:12))
#3 CRM\Core\Invoke.php(36): CRM_Core_Invoke::_invoke((Array:3))
#4 drupal\civicrm.module(458): CRM_Core_Invoke::invoke((Array:3))
#5 includes\menu.inc(527): civicrm_invoke("ajax", "caseroles")

Also for OPEN cases, 99% of the time the case manager doesn't want/need to see the previous roles and it's clutter when it's mixed all together. On an OPEN case the default should be only current roles if the method being used is a checkbox (as opposed to e.g. separate sections like on a contact's relationships tab which keeps the clutter separate). You almost had it in the original version of the PR. And I think I agree with @ray-wright that the wording would be better reversed, i.e. "Show inactive relationships", which on an OPEN case would be unchecked by default, and when viewing a CLOSED case would be checked by default.

[sunilpawar]

@demeritcowboy i did corrected linking on name (php notices).

I will check and revert the logic as @ray-wright have it.

reason for changing logic was to show all roles for closed case when (e.g) case of have more than 10 roles. in that case pagination will show 'Showing 1 to 10 of 15 entries, but it actually showing 1 or 2 active records. then when click on 'Show inactive relationships' then it will show all rows. To just avoid confusion on these case, i reverted the logic.

But i am ok to go with original PR.

[demeritcowboy]

Thanks for your work on this @sunilpawar

[sunilpawar]

@demeritcowboy i have updated PR
Now irrespective case status , i am showing active role on page load. then clicking on 'Show Inactive relationships' checkbox, it show only inactive roles if exist.

[sunilpawar]

[ray-wright]

When a case is closed the roles are ended automatically and after the Disable expired relationships scheduled job runs they are disabled - so most closed cases should only have inactive roles. Is it possible if the case is closed to have 'show inactive relationships' checked already?

[sunilpawar]

When a case is closed the roles are ended automatically and after the Disable expired relationships scheduled job runs they are disabled - so most closed cases should only have inactive roles. Is it possible if the case is closed to have 'show inactive relationships' checked already?

working on it.

[demeritcowboy]

Thanks, I should be able to take another look over the next few days.

[demeritcowboy]

Ok one minor thing that's easy and then what to do with the pager:

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • except as listed in r-code below
• Developer standards
  • [r-tech] PASS
    • Just the same note as earlier that it's still an open question whether a different strategy of not setting end_date when cases close would be better, because there's still other places in civi affected by related issues.
  • [r-code] Issue
    • Need to wrap lines 1786 to 1798 in if (!empty($caseManagerNameArray)) { because otherwise the dashboard and Find Cases gives errors about cases that have no Manager assigned when it uses max() on the empty array, and there's no need to go thru any calculations anyway just let it return on line 1801. Previously it skipped it because of the if ($dao->fetch()).
    • The pager - it's a good idea but I'm going to suggest taking the changes out and leaving it for a future PR because:
      • It doesn't currently work. It shows all the roles even on the first page, and then on the second page it appears to show that there's no case manager assigned.
      • The numbering at the bottom is off.
      • It changes the getCaseRoles function to now default to limiting to 10 instead of the previous "all". Any code using it would need to know to pass in 'rp', and in fact there's no way to specify "all" except by making rp really high. Compucorp uses it, civimobileapi uses it, possibly others.
    • Can you squash your commits?
    • Some of the code added to CaseView.tpl could be simplified but can leave it for now.
  • [r-maint] ?
    • Just the same comment as before.
  • [r-test] PASS

[sunilpawar]

@demeritcowboy i have squashed commits.

[demeritcowboy]

Ok thanks, just checking are there more changes coming? e.g. I still see the other issues - Error on the dashboard/find cases when no manager has ever been assigned to a case: Warning: max(): Array must contain at least one element in CRM_Case_BAO_Case::getCaseManagerContact() (line 1823 of .../CRM/Case/BAO/Case.php). And the pager additions are problematic and change the default return count for getCaseRoles and no longer provide a way to get "all" results.

[sunilpawar]

@demeritcowboy i fixed PHP Notice, i did one correction on pagination total result count.

[demeritcowboy]

Ok thanks - I still get a problem with the pager-related additions though. Here's a specific example:

BEFORE:

1. Create a case with 11 or more roles assigned on it.
2. Run cv ev "echo count(CRM_Case_BAO_Case::getCaseRoles(<client_contact_id>, <case_id>));", where you replace client_contact_id and case_id with the right ids.
3. That gives 11 (or whatever number you have).

AFTER:

1. It gives 10.

Could we move the pager additions to a separate PR and work on it separately?

[sunilpawar]

@demeritcowboy ok, i will create separate PR for pagination and will remove pagination changes from this PR.

[sunilpawar]

@demeritcowboy i have reverted the pagination functionality

[demeritcowboy]

Thanks will take a look!

[demeritcowboy]

Thanks for sticking with it. Yay! I think if you can just squash the commits and then we're good. I have a small touchup but I can submit after.

[sunilpawar]

@demeritcowboy i have squashed commits.

[demeritcowboy]

Great, thanks! Have marked merge-ready.

[demeritcowboy]

Ok let's merge this. It's still not clear to me if one of the other approaches is better long-term but this is a good improvement on the form that should help a lot of people.

[ray-wright]

Thanks @demeritcowboy as the loose originator of the improvement (we contracted @sunilpawar and Skvare to make it usable for core) I'm excited for this to be in core and am totally open to other approaches should they come!