dev/core#1710 Ensure that civicrm_case_activity is properly populated…

[seamuslee001]

… for use by report query when run in force mode

Overview

The civicrm_case_activity table and columns were being added to _columns array via the preProcess, for most cases this was ok however in dashlets and when clicking the view results mode this was problematic as in force mode (force=1) the preProcess is not executed

Before

DB Syntax Error when running report with include case activities set to true and run in force mode

After

No DB Error

ping @monishdeb @eileenmcnaughton @elisseck

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

[monishdeb]

The patch fixes the issue, but I am not confident about the buildQuery() fix made. I feel the placement of $columns['civicrm_case_activity'] is the key to the solution, as it shouldn't be in preprocess but under __contruct() fn where all column headers were declared. And based on $this->_params['include_case_activities_value'] value case and case-activity JOINs should be added in the query. I think its a regression caused by #15135 as reverting the change seems to works for me.

[demeritcowboy]

I think I agree with @monishdeb and I kinda remember looking at moving it to construct() when I did a quickie-fix in #16393, but I think the problem was that the filter wasn't defined yet at that point. But maybe I was missing something, like the JOINs you're suggesting.

[elisseck]

Thanks for writing this! I tested this PR out and it does indeed solve the problem. I also agree that __construct() is probably the place to do this if we can. It looks like there is already a hack in there for a similar issue:

(

civicrm-core/CRM/Report/Form/Activity.php

Lines 300 to 304 in eaf5879

	// Hack to get $this->_alias populated for the table.
	'civicrm_activity_contact' => [
	'dao' => 'CRM_Activity_DAO_ActivityContact',
	'fields' => [],
	],

)

Any thoughts on just replicating this? My thought is it should work to just add:

'civicrm_case_activity' => [
        'dao' => 'CRM_Case_DAO_CaseActivity',
        'fields' => [],
      ],

here so the alias is always populated whether the filter is called for or not.

[eileenmcnaughton]

@seamuslee001 I think this should also target the rc?

[seamuslee001]

@demeritcowboy @elisseck @monishdeb I have move the code around a bit as suggested can you test and as per @eileenmcnaughton i have now targeted this against the RC

[demeritcowboy]

Thanks! I gave it a spin:

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS with comment
    • Exporting the results to csv seems to add an extra unnamed column which appears to be case id. That's actually useful, just odd and has no column heading. It doesn't happen before the patch.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] Undecided
  • [r-test] PASS

[seamuslee001]

thanks @demeritcowboy I have fixed the hiding of that case_id column as it doesn't seem it should be exposed

[eileenmcnaughton]

@demeritcowboy has reviewed this so MOP

[demeritcowboy]

I reviewed it without the extra added line which I'm not sure exactly what it does. I was going to just do another run.

[demeritcowboy]

Actually I think I might have missed something the first time. Can you remove merge-on-pass? What's happening is that the activity links are now the regular activity links, not case activity links, which was what the original PR was about that led to the earlier paging problem.

[eileenmcnaughton]

done

[demeritcowboy]

If I make this change to your current patch it mostly seems to work, just with a weird display glitch on the columns tab where if you've selected case activities then sometimes the columns tab has case id as a field but you can't select it and it's unchecked but then shows in results anyway. But other than the unselected checkbox it's not wrong, and if you've filtered to see case activities it's useful to see case id.

--- a/CRM/Report/Form/Activity.php
+++ b/CRM/Report/Form/Activity.php
@@ -356,7 +356,7 @@ class CRM_Report_Form_Activity extends CRM_Report_Form {
   protected static function addCaseActivityColumns($columns) {
     $columns['civicrm_case_activity']['fields'] = [
       'case_id' => [
-        'no_display' => TRUE,
+        'title' => ts('Case ID'),
         'required' => TRUE,
         'dbAlias' => $columns['civicrm_case_activity']['alias'] . '.case_id',
       ],
@@ -731,9 +731,7 @@ GROUP BY civicrm_activity_id $having {$this->_orderBy}";
     }

     if (!empty($this->_params['include_case_activities_value'])) {
-      $columns = self::addCaseActivityColumns($this->_columns);
-      $this->_nonDisplayFields[] = 'civicrm_case_activity_case_id';
-      $this->_columns = $columns;
+      $this->_columns = self::addCaseActivityColumns($this->_columns);
     }

     // @todo - all this temp table stuff is here because pre 4.4 the activity contact

[seamuslee001]

@demeritcowboy I have made that change now can you check

[demeritcowboy]

Thanks. Looks good. For the purpose of getting the main issues to work they all seem to work.

The test fail is one of those intermittent ones.