Add CiviCase option for showing case activities in normal views #16360

artfulrobot · 2020-01-23T11:15:24Z

Overview

Provides a setting controlling whether activities that belong to cases are visible outside of cases.

Before

If I have a case and add an activity, assigning a contact or two. Those contacts cannot see the activity on their contact record. Therefore the activity listing is incomplete and unclear.

After

Administrators can choose to show the activities on contact records or not from Administer » CiviCase » CiviCase Settings. Look for the setting called "Include case activities in general activity views."
It defaults to the legacy config: hide the activities.
When enabled:
- Case Activities are shown in counts and listings in dashboards and on the Activities tab of contact records.
- On a contact record Activity Tab, case action links are used in place of normal activity action links for case activities (i.e. you get case versions of View / Edit, and Manage Case instead of File On Case etc.).

Technical Details

Please see discussion below.

Comments

The following come from Pradeep and Demerit (thanks!) from https://civicrm.stackexchange.com/q/34407/35

https://issues.civicrm.org/jira/browse/CRM-3829 and also a reference that it is "in the spec" https://forum.civicrm.org/index.php%3Ftopic=6895.0.html

civibot · 2020-01-23T11:15:27Z

(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 · 2020-01-23T15:30:51Z

A couple notes:

You get Notice: Array to string conversion in CRM_Core_Action::replace() (line 364 of ...\CRM\Core\Action.php) on every manage screen because it's trying to format links but case_id is an array.
If you choose File On Case for a case activity from the activity tab it not only removes it from the original case but creates a revision even if you have revisions turned off.
I can see at least one followup ticket/PR being requested: "It should show the case id for activities when viewing from the activity tab and show a link to manage case".
- Maybe also "it should group activities that belong to the same case together".
- Basically a series of requests to duplicate case functionality on the activities tab. Maybe that's neither negative nor positive, I'm just adding thoughts from an r-tech view.

artfulrobot · 2020-01-23T16:12:44Z

THanks for your comments @demeritcowboy

array to string - yes you're right. I assume that you found that with your PR too?
weird.
link to manage case: Agree that's useful. Other bullets, I'm not so worried about those. The main problem as I see it is that currently there's no way for a person who has been assigned an activity to see it from their record. Likewise on 'Client' contact records: you could look at their activities and miss the fact that there's meetings taking place inside cases; each case only shows it's own activities, so without the feature added by this PR there's nowhere that you can see a full log of comms etc.

CRM/Upgrade/Incremental/php/FiveTwentyThree.php

settings/Case.setting.php

CRM/Case/Info.php

templates/CRM/Admin/Form/Setting/Case.tpl

CRM/Activity/Selector/Activity.php

artfulrobot · 2020-01-23T17:23:30Z

In case it wasn't clear from my zillions of commits just now:

array to string - fixed, I think.
I removed the "File on case" for activities already on cases.
I added a "Manage Case" link for activities already on a case.

eileenmcnaughton · 2020-01-23T21:18:52Z

@artfulrobot I think we'd better get unit tests here - this is exactly the sort of thing we have trouble maintaining because someone makes a change & it gets tested with the setting on but doesn't notice it breaks something with the setting off (which is why I push back a lot on settings).

I kind of feel like 'show all' should be the default & 'only show in cases' should be added by an extension - which is why I'm open to adding a setting in core but to make it robust we need test cover - existing tests in CRM_Activity_BAO_ActivityTest should be a good starting point

mattwire · 2020-01-23T21:34:25Z

@artfulrobot +1 for this. I already implemented it quite a long time ago in the fastactivity extension that I co-wrote: https://github.com/systopia/de.systopia.fastactivity - worth a look to see how I did it :-) One of the things I added was an optional column that showed the case ID - this made it much easier to see which activities were on a case (and to access the case directly).

demeritcowboy · 2020-01-23T21:38:34Z

To clarify about making it the default - you're talking new installs? On an existing install the typical case manager will now suddenly have thousands of activities showing on their activity tab that mostly aren't assigned to them.

Also a technote if you're hiding the File On Case link for existing case activities you'll also need to hide it on the view form.

eileenmcnaughton · 2020-01-23T22:08:14Z

@demeritcowboy no - I'm not saying to change the default - it can default to no change -

artfulrobot · 2020-01-24T09:15:06Z

Thanks everyone for the feedback, really helpful. I'll take a look at fast activities, Matt, although I'd probably avoid adding a column since people's screens stopped getting wider when FHD arrived!

demeritcowboy · 2020-01-24T20:16:32Z

Hi @artfulrobot can you add WIP to the title if you're still working on it? The array to string message is back - maybe got lost in a rebase?

artfulrobot · 2020-01-24T21:29:50Z

@demeritcowboy hmmm I've added WIP for now, but I thought it was ready.

has tests now, and they pass locally. The jenkins error seems to come up intermittantly and I'm not sure that it's actually related - it was present in Wrong parameter passed to executeQuery function #16353 which was recently merged too.
I don't see the array to string errors, can you say what you do to get those to appear?

Thanks :-)

demeritcowboy · 2020-01-24T21:35:41Z

Thanks - if you visit the latest test site here and go to any manage case screen the array to string messages come up.

And yes the conformance one is coming up semi-regularly on other PRs.

artfulrobot · 2020-01-26T08:23:13Z

Think I've found the array to string issue - there was a second place in the original code that needed changing. Fingers crossed for passing tests...!

artfulrobot · 2020-01-26T11:17:33Z

@demeritcowboy

Fixed the array to string nonsense. But not looked at this yet:

Also a technote if you're hiding the File On Case link for existing case activities you'll also need to hide it on the view form.

@eileenmcnaughton

Has tests now; I added checks to half a dozen of the existing tests for the case where the new setting is set. If you're happy can you remove the needs-tests tag?

artfulrobot · 2020-01-27T12:50:24Z

OK, I think this looks good to me now, I've removed "WIP" from title.

Let me know if you'd like this rebased and / or squashed.

demeritcowboy · 2020-01-27T15:29:33Z

Some notes so far. I'm not sure if you'd prefer to get everything at once later, or comments as they come up.

Also a technote if you're hiding the File On Case link for existing case activities you'll also need to hide it on the view form.

I'm not sure editing the regular activity.tpl is the best way to do this. This topic touches on what I meant earlier by "duplication of case functions on the activity tab".

The links on the activity tab are different links than you get on the manage case screen, e.g. it should be civicrm/case/activity/view?etc, which takes you to the correct form with the correct options. The following bullets are all just different ways of saying the same thing, but some examples of the consequences:
All the edit links are the wrong links that take you to the regular activity edit form, which doesn't have for example the send a copy section or the encounter medium field. Similarly if you're on the view form, the edit button there is the wrong link.
The different edit forms do different things in postProcess.
The assignee field has some extra functionality on the case side (e.g. restrict to groups).
Besides File On Case functionality differences between case and regular activities, another example would be the Open Case activity type. On the activity tab if you click View for Open Case it takes you to a form that has a Delete button which shouldn't be there.

The tests are great just some syntax notes:

By convention api calls use a helper $this->callAPISuccess(), which eliminates the need to check if it failed, and also normally if you want to then check if the actual value is empty you'd just assert something (e.g. lines 1588-1590).
There's some commented out lines (1559) where you were debugging/looking at api4 which should be removed.
On line 1582 you can actually just pass in the string 'Scheduled' instead of '1'. It's weird but it's an actual feature.
While it should be ok to hardcode contact id '3', you can use the helper $this->createLoggedInUser() which returns a new contact ID to use for this kind of thing.

artfulrobot · 2020-01-27T17:54:57Z

Thanks again for your work, @demeritcowboy ! "WIP" is back on the title.

I had not appreciated that case activities had a different menu link TBH, but that's clear now, I'll work on that.

✔ callAPISuccess makes sense.
✔ removed API4 commented code - oops!
✔ Using 'Scheduled' in place of '1' (I think I used '1' instead by copying the convention in the code - it's peppered throughout with using 1, and even asserting 1 with an error about scheduled, so if the case ever exists that Scheduled !== 1 then there's going to be quite a few broken tests.)
~ I deliberately chose contact 3 as that's the first contact in the xml file. I don't think there was a need to create a user here, so I've left that.

artfulrobot · 2020-01-28T16:50:48Z

@demeritcowboy I think I've addressed the points you raised. The links shown in the activity selector are now the case-specific ones as follows

View
Edit (permission is checked)
Manage Case (the Case ID is in the title attribute, FTW)

(the failing test is that conformance one that keeps popping up at random)

CRM/Activity/BAO/Activity.php

artfulrobot · 2020-01-29T09:05:24Z

@demeritcowboy yes, in looking through the activity code, I have a raft of cleanups to suggest (would be separate PR). There's a lot of mess, inefficiency, variables that are assigned but never referenced and plain ol' weirdness in there - I guess from years of piecemeal additions without tests.

I've set ctx to a ZLS.

artfulrobot · 2020-02-03T18:05:02Z

@demeritcowboy how's this looking to you now? Be nice to get it merged if everyone is happy, but perhaps you have more to add? Thanks for your work on this and no rush if you're busy.

demeritcowboy · 2020-02-03T18:08:41Z

Hi I looked a little more this weekend and plan to finish this week. Note the patch doesn't apply cleanly because of the upgrade script commits but I was able to fudge it. Up to you if you want to squash/rebase now or wait just in case.

demeritcowboy · 2020-02-05T21:48:48Z

Ok I have a mixed review but mostly good. There is one permissions question that is the main thing I see.

General standards
- [r-explain] PASS-ish
  - If the main goal is because people want to see activities that are assigned to them on their activities tab, then that's fine just I don't see it working for people who are also case managers. Their assigned activities will be mixed in with the hundreds/thousands of other activities from cases they've created or worked on.
  - I took a quick look and tried the fast activity extension and so it also raises the question of "why not just use that"?
- [r-user] PASS
- [r-doc] PASS
- [r-run] Possible Issue
  - When a user only has access to their own cases, the case activities from other people's cases are now appearing on contacts' activity tabs, which sort of overrides the permissions. Clicking on the View/Manage Case link gives a permission denied error, but this brings up an interesting question: To what degree should this setting start overriding various permissions? Normally I'd argue it should still respect permissions and they shouldn't even see that these activities exist, but does that satisfy the use-case for anyone who wants this setting turned on?
Developer standards
- [r-tech] PASS
  - I agree with the comments about campaign_id and the return array in line 700 in CRM/Activity/BAO/Activity.php. It looks like the overlap was introduced in a previous shuffling around of some code. But doesn't need to be dealt with here. Good to have the comment.
  - As previously discussed it's maybe adding a bit on to the existing issues in the code (e.g. if (civicase) {} else {}), but I'd say is out of scope.
- [r-code] Just some cleanup
  - Line 2717 in CRM/Activity/BAO/Activity.php english got mangled while copying from selector/search and negating.
  - Where are the additions in Case/BAO/Case.php used? There's an assign() call in Activity/Form/Activity.php which uses it, but then those assign values aren't used anywhere? I'm guessing this was left over from a previous WIP version?
  - It needs rebasing/squashing.
- [r-maint] PASS
  - In testGetActivitiesforNonAdminDashboard, it sets permission to access all. I think this is ok though because non-admin here seems to mean something different. To be honest I'm not sure what $params['admin'] = FALSE means throughout this test file.
- [r-test] PASS

artfulrobot · 2020-02-06T07:20:21Z

@demeritcowboy thanks so much for your well thought out contents, much appreciated. I'll take a good look through.

artfulrobot · 2020-02-06T13:26:21Z

Thanks again for the feedback. Here's a re-roll and some explanation.

New commits

rebase, squash
stripped out stuff that was added in WIP then not used
corrected english in comment
added warning about permissions next to the enable civi case activities settings

`r-explain` responses

If the main goal is because people want to see activities that are assigned to them on their activities tab...

Good to bring it back to the use case, but I think I'm coming from a different angle. In one of the earlier comments I've seen that people who want this are 50:50 split with those who don't. I think the main reason for me wanting this in core is because 50% of people think it should work this way. Not because they can't get the information some other way (e.g. add an extension) but because it's the way they'd expect it to work.

Accepting the 50:50 stat (that was probably not based on a huge sample...), the two options, considered from an introduction to CiviCRM to a new user are:

You can see the Activities that relate to a contact, unless they're in a case. If they are in cases you have to find those cases and go into each case and scroll through all the activities in each one to see if any of them relate to this contact. Also, there's no way to search for cases that involve a particular contact in an activity, so unless every contact involved in an activity also has a role on the case, this means you have to go through every single case that might involve them in an activity.

OR
You can see all Activities that relate to a contact.

I think you'll agree that the latter is much easier to explain. But it's not just explanation - it's the mistakes that could be made from not seeing activities for a contact. If you can't see the data exists, you can't use it to take decisions. In situations where CiviCase is really the main way people interact with CiviCRM, when everything is case-based, then this is not a problem. But outside those particular organisation structures, it's damaging (IMO) to the general-use CRM functions.

What about the case managers?

Their activities tab will include lots and lots of activities because it will include all of the case activities. This is true. It's also true of the default domain record. It's also true for anyone who's been on a mailing list for a while. Of course we're grateful for the recent(ish) additions of better filters, but there's still a lot of work to do to get it useful. But at least it's one step closer to being able to inspect everything (I know there are other activities it hides).

Basically, the activities tab is hideous and usually fairly unhelpful in its summarising of a contact's activities. This is why there are so many extensions that replicate/replace its behaviour. But that's good - everyone has different needs. So things like Activity Tabs, Data Processor and Extended Reports help provide UIs that suit these different needs.

Why not just use Fast Activity?

This PR solves the general case above about onboarding, intuitiveness, data discovery, and applies to all activity listings (e.g. dashboards). The Fast Activity extension solves a particular problem - the activity tab.
That extension was created to solve various problems in core that have since been addressed - it was slow and did not have filters.
It also has the option to include case activities, which overlaps this PR. But we should not have to install an extension to do core stuff.
It wasn't working anyway on my install but it apparently also includes 'responsive' data tables - this is not, IMO, a suitable solution since it does responsive by hiding data at smaller screen sizes which is not what responsive should mean.

Permissions

Yeah. I think this is one where people are going to be split based on their organisation's policies.

It could be, however, that organisations fall into two camps along the same lines as this option provides:

those with a very case-based workflow: these probably don't want to turn this option on, and are unaffected by this PR.
those who want to see all activities on a contact - most commonly: orgs where several staff may have cause to get in touch with a contact and they want to make sure they're not interfering with what their colleagues are doing, or pestering someone ("this is the third call I've had from you lot in a week!"). At least these orgs can now see something's happening. Remember of course that contact ACLs still apply; so if they're not allowed to see the contact then they won't see the activity.

I've added an explicit but brief explanation on the settings page.

Do these alleviate any of your PASS-ish-es?

Many thanks, Rich.

demeritcowboy · 2020-02-06T17:37:04Z

Thanks for the detailed explanation. You've put a lot of work into this. I just have some final technicalities and then I'm done:

I wouldn't split the ts() between Case.tpl and Case.setting.php. I would just put it all in Case.setting.php in the description field. This also gives the translator the full context.
At the risk of alienating people, the convention is to use US English spellings in the original strings, and then it gets translated in the localization files (e.g. https://lab.civicrm.org/dev/translation/blob/master/po/en_GB/case.po#L645). So here the words "behaviour" and "summarised". It's interesting I didn't notice before because here (Canada) we kind of have a mix and it's common to see "behaviour", but then when you added "summarised" it stood out more since we don't have that.
For the very last phrase, I'd put an "and" in front, i.e. "Such users will still be prevented from managing the case and viewing/editing the activity."
In the description the permission "View All Case Activities" should be "access all cases and activities".
Yesterday it was 5.23 but now today the "add" field in Case.setting.php would be 5.24.

Different aproach to tackling civicrm#16353 Fix array-to-string notice; Improve activity action links remove upgrader - as per Pradeep's suggestion update version added tweak way description is gotten update activity tests to check civicaseShowCaseActivities setting Fix another array-to-string conversion error Remove File On Case from activity View; add Manage Case fix case activity links when using caseShowCaseActivities remove ctx parameter for case activity links various US-Englishized Case settings texts. fix permission name

artfulrobot · 2020-02-10T20:23:50Z

Thanks again @demeritcowboy I've made those changes.

demeritcowboy · 2020-02-11T02:00:57Z

Thanks looks good.

eileenmcnaughton · 2020-02-11T02:15:00Z

@demeritcowboy do you want me to merge this now?

demeritcowboy · 2020-02-11T02:21:14Z

@eileenmcnaughton Sure.

civibot bot added the master label Jan 23, 2020

artfulrobot mentioned this pull request Jan 23, 2020

Wrong parameter passed to executeQuery function #16353

Merged