CRM-21558 Add batch update via profile for cases #11411

mattwire · 2017-12-14T14:31:31Z

Overview

This adds "Update Multiple Cases" option to search tasks and allows you to batch update multiple cases in the same way as other entities.

Before

Not possible to batch update multiple cases via profile.

After

Select "Update multiple cases" from a search task and see:

The you see the profile:

CRM-21558: Add batch update via profile to cases

eileenmcnaughton · 2017-12-14T21:34:39Z

@colemanw - this would be a good one for you to evaluate.... It probably would be possible to do as an extension too - although it would require some core tidy-ups to facilitate that

colemanw · 2017-12-14T23:25:50Z

I think this is fine to go into core. @jamienovick can you ask someone on your team to review this please?

mickadoo · 2017-12-18T11:22:23Z

@mattwire can you rebase to add the ticket number to the commit message?

mickadoo

I can see how this change would be useful. I left a lot of comments on this, but so much of the code is boilerplate stuff from previous tasks. I can understand why you might not want to rewrite it. It's concerning there's so much duplicate code in the first place.

I tested this locally and it seems to work. One thing that might be worth noting is that when I created a custom fieldset for a certain case type, and then added that field to the profile used in batch editing, all cases will display the fields to edit the custom data, regardless of case type. However when you save the form only the correct case type will save the custom data.

mickadoo · 2017-12-18T11:23:19Z

CRM/Case/Form/Task/Batch.php

+ *
+ * @package CRM
+ * @copyright CiviCRM LLC (c) 2004-2017
+ * $Id$


Is $Id$ supposed to be here?

mickadoo · 2017-12-18T11:41:32Z

CRM/Case/Form/Task/Batch.php

+    parent::preProcess();
+
+    //get the contact read only fields to display.
+    $readOnlyFields = array_merge(array('sort_name' => ts('Name')),


For new PRs to master are we supposed to use the short array syntax [] since PHP 5.3 support is supposed to be dropped this month?

mickadoo · 2017-12-18T11:53:50Z

CRM/Case/Form/Task/Batch.php

+
+    //get the contact read only fields to display.
+    $readOnlyFields = array_merge(array('sort_name' => ts('Name')),
+      CRM_Core_BAO_Setting::valueOptions(CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME,


Since this is a pretty long call could you make it a separate variable? It would also help explain what it is. You probably already checked, but is this variable accessible through Civi::settings, or any shorter method?

mickadoo · 2017-12-18T11:55:22Z

CRM/Case/Form/Task/Batch.php

+    // initialize the task and row fields
+    parent::preProcess();
+
+    //get the contact read only fields to display.


Space after //

mickadoo · 2017-12-18T11:55:27Z

CRM/Case/Form/Task/Batch.php

+        TRUE, NULL, FALSE, 'name', TRUE
+      )
+    );
+    //get the read only field data.


Space after //

mickadoo · 2017-12-18T13:06:23Z

CRM/Core/BAO/UFGroup.php

@@ -2233,6 +2233,14 @@ public static function buildProfile(
      $form->add('text', $name, $title, $attributes, $required);
      $form->addRule($name, ts('Please enter the duration as number of minutes (integers only).'), 'positiveInteger');
    }
+    elseif ($fieldName == 'case_status') {


Can we use strict comparison here?

There's a whole load like that - really the whole if/elseif/else would be better as a switch but I'll leave that for someone else :-)

mickadoo · 2017-12-18T13:06:57Z

CRM/Core/BAO/UFGroup.php

@@ -2514,6 +2522,11 @@ public static function setProfileDefaults(
    if ($component == 'Activity') {
      self::setComponentDefaults($fields, $componentId, $component, $defaults);
    }
+
+    //Handling Case Part of the batch profile


Space after //

mickadoo · 2017-12-18T13:07:18Z

CRM/Core/BAO/UFGroup.php

@@ -2514,6 +2522,11 @@ public static function setProfileDefaults(
    if ($component == 'Activity') {
      self::setComponentDefaults($fields, $componentId, $component, $defaults);
    }
+
+    //Handling Case Part of the batch profile
+    if (CRM_Core_Permission::access('CiviCase') && $component == 'Case') {


Strict comparison

This is consistent with the rest of the code so I won't change for now

mickadoo · 2017-12-18T13:07:52Z

CRM/Core/BAO/UFGroup.php

@@ -3160,7 +3173,7 @@ public static function encodeGroupType($coreTypes, $subTypes, $delim = CRM_Core_
   */
  public static function setComponentDefaults(&$fields, $componentId, $component, &$defaults, $isStandalone = FALSE) {
    if (!$componentId ||
-      !in_array($component, array('Contribute', 'Membership', 'Event', 'Activity'))
+      !in_array($component, array('Contribute', 'Membership', 'Event', 'Activity', 'Case'))


This array is just getting longer, it might be better as another variable

something for the future :-)

mickadoo · 2017-12-18T14:48:29Z

CRM/Case/Form/Task/PickProfile.php

+    $profiles = CRM_Core_BAO_UFGroup::getProfiles($types, TRUE);
+
+    if (empty($profiles)) {
+      CRM_Core_Session::setStatus(ts("You will need to create a Profile containing the %1 fields you want to edit before you can use Update multiple cases. Navigate to Administer CiviCRM >> CiviCRM Profile to configure a Profile. Consult the online Administrator documentation for more information.", array(1 => $types[0])), ts('Update multiple records error'), 'error');


This information is not up to date, when I tried this PR branch I needed to go to Administer > Customize Data and Screens > Profiles

mattwire · 2018-01-12T10:48:07Z

@colemanw @mickadoo @eileenmcnaughton So most of the comments raised by @mickadoo are because this is copied from all the other class implementations. There is some common refactoring I could attempt here though I've only got another couple of hours in the budget. Please bear with me and I'll see if I can fit it in.

eileenmcnaughton · 2018-01-15T01:40:24Z

@mattwire my priority for any tidy up is that you share code where possible. I'd rather see a small amount of change to improve code sharing than a large amount of addressing over-long lines. Also any code that can be REMOVED is great.

From a merger POV I'd rather see a unit test over stylistic code changes and I'm not sure that anyone has reviewed this from a UI-test / UI-makes sense point of view.

mickadoo

Thanks for updating this @mattwire. I know you probably want to get this functionality in and don't have time to refactor the whole codebase. I wasn't aware of the level of boilerplate code. I left some more comments, but if the tests pass and someone with more experience is happy with it (I see @eileenmcnaughton looked over it) then I think it's fine

mickadoo · 2018-01-18T10:36:17Z

CRM/Case/Form/Task.php

 */
-class CRM_Case_Form_Task extends CRM_Core_Form {
+class CRM_Case_Form_Task extends CRM_Core_Form_Task {


There is a fairly big tree of form classes, but from the naming at least it makes sense that Case_Form_Task extends from Core_Form_Task

mickadoo · 2018-01-18T10:36:56Z

CRM/Case/Form/Task/Batch.php

+   *
+   * @return array $defaults
+   */
+  public function setDefaultValues() {


If it just calls the parent then it's not required

mickadoo · 2018-01-18T10:37:24Z

CRM/Case/Form/Task/Batch.php

+    $params = $this->exportValues();
+
+    if (!isset($params['field'])) {
+      CRM_Core_Session::setStatus(ts("No updates have been saved."), ts('Not Saved'), 'alert');


Single quotes

mickadoo · 2018-01-18T10:48:12Z

CRM/Case/Form/Task/Batch.php

+      unset($value['case_type']);
+
+      //Get the case status
+      $value['status_id'] = (CRM_Utils_Array::value('case_status', $value)) ? $value['case_status'] : CRM_Core_DAO::getFieldValue('CRM_Case_DAO_Case', $key, 'status_id');


I'm harping on about these long lines, with such long function arguments I know it's hard to keep the lines short. That said, here's an alternative for this line

//Get the case status $daoClass = 'CRM_Case_DAO_Case'; $caseStatus = CRM_Utils_Array::value('case_status', $value); if (!$caseStatus) { // default to existing status ID $caseStatus = CRM_Core_DAO::getFieldValue($daoClass, $key, 'status_id'); }

I think there is some personal preference involved there - depending how you edit things longer or shorter likes can be more readable

Yep. But happy to clean up code where possible, just limited as to how much time I can spend on clean up at this stage - some of this could be done as smaller commits once we've made this (big) change. @mickadoo Thanks for the code snippet, I've added it to this PR.

mickadoo · 2018-01-18T10:50:32Z

CRM/Case/Form/Task/Batch.php

+      $case = CRM_Case_BAO_Case::add($value);
+
+      // add custom field values
+      if (!empty($value['custom']) &&


One place where I actually think the line is too short 😄 both conditions fit on this line comfortably

mickadoo · 2018-01-18T10:50:47Z

CRM/Case/Form/Task/Batch.php

+      }
+    }
+
+    CRM_Core_Session::setStatus(ts("Your updates have been saved."), ts('Saved'), 'success');


Double quotes

mickadoo · 2018-01-18T10:56:40Z

CRM/Core/Form/Task.php

+   * since its used for things like send email
+   */
+  public function setContactIDs() {
+    $this->_contactIds = &CRM_Core_DAO::getContactIDsFromComponent($this->_entityIds,


_contactIds is not a property of this class

mickadoo · 2018-01-18T10:57:56Z

CRM/Core/Form/Task/Batch.php

+    CRM_Utils_System::setTitle($this->_title);
+
+    $this->addDefaultButtons(ts('Save'));
+    $this->_fields = CRM_Core_BAO_UFGroup::getFields($ufGroupId, FALSE, CRM_Core_Action::VIEW);


_fields is not a property of this class

mickadoo · 2018-01-18T11:05:15Z

CRM/Core/Form/Task/PickProfile.php

+   * @return array
+   *   list of errors to be posted back to the form
+   */
+  public static function formRule($fields) {


Still not sure of the purpose of this

By default it is pointless :-) However, it makes more sense once CRM_Core_Form_Task_PickProfile is used for other entities - eg. Contacts: https://github.com/civicrm/civicrm-core/blob/master/CRM/Contact/Form/Task/PickProfile.php#L133

We could improve this/make it consistent in a future PR when we clean up some of the other entities to use the shared classes

mickadoo · 2018-01-18T11:05:49Z

CRM/Core/Form/Task/PickProfile.php

+    $this->set('ufGroupId', $params['uf_group_id']);
+
+    // also reset the batch page so it gets new values from the db
+    $this->controller->resetPage('Batch');


My IDE tells me the resetPage method doesn't exist. Is it just because it's not type-hinted correctly?

Yes, I get a few of those (PHPStorm) but I see them elsewhere in the codebase too. If you have a way to "fix" them let me know, otherwise let's ignore it for now :-)

mickadoo · 2018-01-22T12:19:02Z

@mattwire Just a general comment, I'm not sure about the convention for commits. I notice your commit message doesn't include the ticket number in it, should it be there?

Also, everything is squashed into a single commit. That makes it a bit harder to check things later on, since the commit message for all the changes won't give a good reason for any single change. It's also a bit harder when reviewing since I don't know what specific changes were made since my last review.

I know there are different opinions on this so I'm not saying you should definitely change it, just wondering how you feel about it, or the stance from the community in general.

mickadoo

Looks good to me 💯

There are still some warnings on codestyle from Jenkins

eileenmcnaughton · 2018-01-22T19:05:21Z

Agree re the JIRA in the commit message. WRT one vs many commits - my opinion is each commit should stand alone - if adding new code we don't want to see your drafts in years to come but if doing a refactor then as long as each step stands alone it can be much easier to read smaller commits. (ie. the final commits should reflect what will read well in the future)

eileenmcnaughton · 2018-01-22T19:14:05Z

So interpretting the above to how it maps to a review of this PR

(CiviCRM Review Template DEL-1.0)

JIRA (r-jira)
- PASS : The PR has a JIRA reference. (Or: it does not need one.)
Test results (r-test)
- UNREVIEWED
Code quality (r-code)
- PASS: @mickadoo has reviewed the code quality
Documentation (r-doc)
- UNREVIEWED
Maintainability (r-maint)
- UNREVIEWED
Run it (r-run)
- UNREVIEWED
User impact (r-user)
- UNREVIEWED
Technical impact (r-tech)
- UNREVIEWED:
- COMMENTS: This is mostly new code so should not cause regressions but there are some changes to case task form (which some testing would quickly establish the validity of) and some fields are removed from UFField::getAvailableFields which I don't know the impact of from my quick look at the code

eileenmcnaughton · 2018-01-22T20:08:25Z

CRM/Core/Form/Task.php

+  /**
+   * Build all the data structures needed to build the form.
+   */
+  public function preProcess() {


this whole function could be a shared function couldn't it?

this whole function could be a shared function couldn't it?
@eileenmcnaughton Not sure quite what you mean here?

I think I must have thought it was on a child class

…e classes

mattwire · 2018-01-29T14:16:07Z

Comments from client:

Seems to work fine on the test site. No comments on the usability - matches the standard bulk update using profile approach.
Using a Profile is a good idea I think as it will provide some flexibility (I'd just been thinking of it from a status perspective).

mattwire · 2018-02-14T21:04:12Z

COMMENTS: This is mostly new code so should not cause regressions but there are some changes to case task form (which some testing would quickly establish the validity of) and some fields are removed from UFField::getAvailableFields which I don't know the impact of from my quick look at the code

The fields are case specific fields - dates and case status - and act as a filter as to what fields can be edited in a batch update. I've tested that they all update correctly. I think there were also earlier issues which have been resolved by @yashodha with her batch fields PRs that have been merged over the last couple of months.

mattwire · 2018-02-14T21:05:31Z

@colemanw Do you have time to do a quick review of this? (And merge if you are happy?)

colemanw · 2018-02-18T13:13:21Z

Code has been thoroughly reviewed and runtime testing was performed by @mickadoo so I think we're good to merge.

mattwire · 2018-02-18T14:53:36Z

@colemanw Thankyou :-)

eileenmcnaughton · 2018-04-03T21:52:35Z

@mattwire @colemanw @mickadoo this caused 2 regressions not picked up in review #11928 #11926

there was some identification of this risk in my summary of the analysis. It's probably worth thinking about how this could have been picked up
#11411 (comment)

civicrm-builder added the master label Dec 14, 2017

mattwire force-pushed the CRM-21558_case_batch_task branch from aaa4b8a to 04d0351 Compare December 14, 2017 14:41

mickadoo suggested changes Dec 18, 2017

View reviewed changes

mattwire force-pushed the CRM-21558_case_batch_task branch 4 times, most recently from 07a328d to 0654056 Compare January 17, 2018 16:10

mickadoo reviewed Jan 18, 2018

View reviewed changes

mattwire force-pushed the CRM-21558_case_batch_task branch from 0654056 to be38a21 Compare January 19, 2018 03:52

mickadoo approved these changes Jan 22, 2018

View reviewed changes

eileenmcnaughton reviewed Jan 22, 2018

View reviewed changes

Add batch update via profile for cases and refactor to use shared cor…

888da08

…e classes

mattwire force-pushed the CRM-21558_case_batch_task branch from be38a21 to 888da08 Compare January 28, 2018 16:33

colemanw merged commit 555acb0 into civicrm:master Feb 18, 2018

mlutfy added this to the 4.7.32 milestone Mar 6, 2018

mattwire mentioned this pull request Apr 4, 2018

Re Add CRM_Case_Form_Task::PreProcessCommon() #11928

Merged

mattwire mentioned this pull request Apr 5, 2018

WIP dev/core#48 Fix PDF Letter only generates a single letter when multip… #11946

Closed

monishdeb mentioned this pull request Apr 17, 2018

dev/core#48 Fix PDF Letter only generates a single letter when multiple contact IDs are specified #11985

Merged

mattwire mentioned this pull request Apr 21, 2018

dev/core#71 Add functionality to create PDF/Word docs from Activity searches #12012

Closed

mattwire deleted the CRM-21558_case_batch_task branch September 25, 2018 11:07

CRM-21558 Add batch update via profile for cases #11411

CRM-21558 Add batch update via profile for cases #11411

Conversation

mattwire commented Dec 14, 2017 • edited by civicrm-builder Loading

Overview

Before

After

eileenmcnaughton commented Dec 14, 2017

colemanw commented Dec 14, 2017

mickadoo commented Dec 18, 2017

mickadoo left a 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

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

mattwire commented Jan 12, 2018

eileenmcnaughton commented Jan 15, 2018

mickadoo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickadoo commented Jan 22, 2018

mickadoo left a comment • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 22, 2018 • edited Loading

eileenmcnaughton commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Jan 29, 2018

mattwire commented Feb 14, 2018

mattwire commented Feb 14, 2018

colemanw commented Feb 18, 2018

mattwire commented Feb 18, 2018

eileenmcnaughton commented Apr 3, 2018

mattwire commented Dec 14, 2017 •

edited by civicrm-builder

Loading

mickadoo left a comment •

edited

Loading

eileenmcnaughton commented Jan 22, 2018 •

edited

Loading