dev/core#1596 fix (unreleased) regression on contribution summary

https://lab.civicrm.org/dev/core/issues/1596

Makes more use of parent function to fix (in general we've done fixes/ improvements to the report
generic functions that have not filtered down to reports that override them

diff --git a/CRM/Report/Form.php b/CRM/Report/Form.php
index 669386a5b3930f95177f5266e59c964cef9a52f6..47adf78eed1b64a28ccf515bca0fe5131f50c409 100644 (file)
--- a/CRM/Report/Form.php
+++ b/CRM/Report/Form.php
@@ -5890,13 +5890,27 @@ LEFT JOIN civicrm_contact {$field['alias']} ON {$field['alias']}.id = {$this->_a
               switch ($this->_params['group_bys_freq'][$fieldName]) {
                 case 'FISCALYEAR':
                   $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = self::fiscalYearOffset($field['dbAlias']);
+                  break;
 
                 case 'YEAR':
-                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = " {$this->_params['group_bys_freq'][$fieldName]}({$field['dbAlias']})";
+                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = " YEAR({$field['dbAlias']})";
+                  break;
 
-                default:
-                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "EXTRACT(YEAR_{$this->_params['group_bys_freq'][$fieldName]} FROM {$field['dbAlias']})";
+                case 'QUARTER':
+                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "YEAR({$field['dbAlias']}), QUARTER({$field['dbAlias']})";
+                  break;
 
+                case 'YEARWEEK':
+                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "YEARWEEK({$field['dbAlias']})";
+                  break;
+
+                case 'MONTH':
+                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "EXTRACT(YEAR_MONTH FROM {$field['dbAlias']})";
+                  break;
+
+                case 'DATE':
+                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "DATE({$field['dbAlias']})";
+                  break;
               }
             }
             else {

diff --git a/CRM/Report/Form/Contribute/Summary.php b/CRM/Report/Form/Contribute/Summary.php
index c1ae2db6286ac2eded48ca564041df9350096b90..5a4de704343b437a117bdd2e9383eb1e95346126 100644 (file)
--- a/CRM/Report/Form/Contribute/Summary.php
+++ b/CRM/Report/Form/Contribute/Summary.php
@@ -50,11 +50,15 @@ class CRM_Report_Form_Contribute_Summary extends CRM_Report_Form {
   protected $groupFilterNotOptimised = FALSE;
 
   /**
-   * Indicate that report is not fully FGB compliant.
+   * Use the generic (but flawed) handling to implement full group by.
+   *
+   * Note that because we are calling the parent group by function we set this to FALSE.
+   * The parent group by function adds things to the group by in order to make the mysql pass
+   * but can create incorrect results in the process.
    *
    * @var bool
    */
-  public $optimisedForOnlyFullGroupBy;
+  public $optimisedForOnlyFullGroupBy = FALSE;
 
   /**
    * Class constructor.
@@ -517,59 +521,27 @@ class CRM_Report_Form_Contribute_Summary extends CRM_Report_Form {
    * Set group by clause.
    */
   public function groupBy() {
-    $this->_groupBy = "";
-    $groupByColumns = [];
-    $append = FALSE;
+    parent::groupBy();
+
+    $isGroupByFrequency = !empty($this->_params['group_bys_freq']);
+
     if (!empty($this->_params['group_bys']) &&
       is_array($this->_params['group_bys'])
     ) {
-      foreach ($this->_columns as $tableName => $table) {
-        if (array_key_exists('group_bys', $table)) {
-          foreach ($table['group_bys'] as $fieldName => $field) {
-            if (!empty($this->_params['group_bys'][$fieldName])) {
-              if (!empty($field['chart'])) {
-                $this->assign('chartSupported', TRUE);
-              }
-
-              if (!empty($table['group_bys'][$fieldName]['frequency']) &&
-                !empty($this->_params['group_bys_freq'][$fieldName])
-              ) {
-
-                $append = "YEAR({$field['dbAlias']});;";
-                if (in_array(strtolower($this->_params['group_bys_freq'][$fieldName]),
-                  ['year']
-                )) {
-                  $append = '';
-                }
-                if ($this->_params['group_bys_freq'][$fieldName] == 'FISCALYEAR') {
-                  $groupByColumns[] = self::fiscalYearOffset($field['dbAlias']);
-                }
-                else {
-                  $groupByColumns[] = "$append {$this->_params['group_bys_freq'][$fieldName]}({$field['dbAlias']})";
-                }
-                $append = TRUE;
-              }
-              else {
-                $groupByColumns[] = $field['dbAlias'];
-              }
-            }
-          }
-        }
-      }
 
       if (!empty($this->_statFields) &&
-        (($append && count($groupByColumns) <= 1) || (!$append)) &&
+        (($isGroupByFrequency && count($this->_groupByArray) <= 1) || (!$isGroupByFrequency)) &&
         !$this->_having
       ) {
         $this->_rollup = " WITH ROLLUP";
       }
       $groupBy = [];
-      foreach ($groupByColumns as $key => $val) {
+      foreach ($this->_groupByArray as $key => $val) {
         if (strpos($val, ';;') !== FALSE) {
           $groupBy = array_merge($groupBy, explode(';;', $val));
         }
         else {
-          $groupBy[] = $groupByColumns[$key];
+          $groupBy[] = $this->_groupByArray[$key];
         }
       }
       $this->_groupBy = "GROUP BY " . implode(', ', $groupBy);
@@ -602,18 +574,18 @@ class CRM_Report_Form_Contribute_Summary extends CRM_Report_Form {
    * @param array $rows
    *
    * @return array
+   *
+   * @throws \CRM_Core_Exception
    */
   public function statistics(&$rows) {
     $statistics = parent::statistics($rows);
 
     $softCredit = CRM_Utils_Array::value('soft_amount', $this->_params['fields']);
     $onlySoftCredit = $softCredit && !CRM_Utils_Array::value('total_amount', $this->_params['fields']);
-    if (empty($this->_groupBy)) {
-      $group = "\nGROUP BY {$this->_aliases['civicrm_contribution']}.currency";
-    }
-    else {
-      $group = "\n {$this->_groupBy}, {$this->_aliases['civicrm_contribution']}.currency";
+    if (!isset($this->_groupByArray['civicrm_contribution_currency'])) {
+      $this->_groupByArray['civicrm_contribution_currency'] = 'currency';
     }
+    $group = ' GROUP BY ' . implode(', ', $this->_groupByArray);
 
     $this->from('contribution');
     if ($softCredit) {

diff --git a/api/v3/ReportTemplate.php b/api/v3/ReportTemplate.php
index 8f0aba9fa1daf3133358c977a6ee014afde71034..5d2e98fe6162982b902fa9e30bfedba44ab84aac 100644 (file)
--- a/api/v3/ReportTemplate.php
+++ b/api/v3/ReportTemplate.php
@@ -180,6 +180,10 @@ function _civicrm_api3_report_template_getrows($params) {
 function civicrm_api3_report_template_getstatistics($params) {
   list($rows, $reportInstance, $metadata) = _civicrm_api3_report_template_getrows($params);
   $stats = $reportInstance->statistics($rows);
+  if (isset($metadata['metadata']['sql'])) {
+    // Update for stats queries.
+    $metadata['metadata']['sql'] = $reportInstance->getReportSql();
+  }
   $reportInstance->cleanUpTemporaryTables();
   return civicrm_api3_create_success($stats, $params, 'ReportTemplate', 'getstatistics', CRM_Core_DAO::$_nullObject, $metadata);
 }

diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php
index b87981643c007e21dffe080772f82809dcadcf24..038b07674162b2941a928ff7f104e5c5f0d72642 100644 (file)
--- a/tests/phpunit/api/v3/ReportTemplateTest.php
+++ b/tests/phpunit/api/v3/ReportTemplateTest.php
@@ -653,6 +653,8 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
 
   /**
    * Test the group filter works on the contribution summary when 2 groups are involved.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testContributionSummaryWithTwoGroups() {
     $groupID = $this->setUpPopulatedGroup();
@@ -666,8 +668,104 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
     $this->assertEquals(4, $rows['values'][0]['civicrm_contribution_total_amount_count']);
   }
 
+  /**
+   * Test we don't get a fatal grouping  by only contribution status id.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testContributionSummaryGroupByContributionStatus() {
+    $params = [
+      'report_id' => 'contribute/summary',
+      'fields' => ['total_amount' => 1, 'country_id' => 1],
+      'group_bys' => ['contribution_status_id' => 1],
+      'options' => ['metadata' => ['sql']],
+    ];
+    $rowsSql = $this->callAPISuccess('report_template', 'getrows', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY contribution_civireport.contribution_status_id WITH ROLLUP', $rowsSql[0]);
+    $statsSql = $this->callAPISuccess('report_template', 'getstatistics', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY contribution_civireport.contribution_status_id, currency', $statsSql[2]);
+  }
+
+  /**
+   * Test we don't get a fatal grouping  by only contribution status id.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testContributionSummaryGroupByYearFrequency() {
+    $params = [
+      'report_id' => 'contribute/summary',
+      'fields' => ['total_amount' => 1, 'country_id' => 1],
+      'group_bys' => ['receive_date' => 1],
+      'group_bys_freq' => ['receive_date' => 'YEAR'],
+      'options' => ['metadata' => ['sql']],
+    ];
+    $rowsSql = $this->callAPISuccess('report_template', 'getrows', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY  YEAR(contribution_civireport.receive_date) WITH ROLLUP', $rowsSql[0]);
+    $statsSql = $this->callAPISuccess('report_template', 'getstatistics', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY  YEAR(contribution_civireport.receive_date), currency', $statsSql[2]);
+  }
+
+  /**
+   * Test we don't get a fatal grouping with QUARTER frequency.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testContributionSummaryGroupByYearQuarterFrequency() {
+    $params = [
+      'report_id' => 'contribute/summary',
+      'fields' => ['total_amount' => 1, 'country_id' => 1],
+      'group_bys' => ['receive_date' => 1],
+      'group_bys_freq' => ['receive_date' => 'QUARTER'],
+      'options' => ['metadata' => ['sql']],
+    ];
+    $rowsSql = $this->callAPISuccess('report_template', 'getrows', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY YEAR(contribution_civireport.receive_date), QUARTER(contribution_civireport.receive_date) WITH ROLLUP', $rowsSql[0]);
+    $statsSql = $this->callAPISuccess('report_template', 'getstatistics', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY YEAR(contribution_civireport.receive_date), QUARTER(contribution_civireport.receive_date), currency', $statsSql[2]);
+  }
+
+  /**
+   * Test we don't get a fatal grouping with QUARTER frequency.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testContributionSummaryGroupByDateFrequency() {
+    $params = [
+      'report_id' => 'contribute/summary',
+      'fields' => ['total_amount' => 1, 'country_id' => 1],
+      'group_bys' => ['receive_date' => 1],
+      'group_bys_freq' => ['receive_date' => 'DATE'],
+      'options' => ['metadata' => ['sql']],
+    ];
+    $rowsSql = $this->callAPISuccess('report_template', 'getrows', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY DATE(contribution_civireport.receive_date) WITH ROLLUP', $rowsSql[0]);
+    $statsSql = $this->callAPISuccess('report_template', 'getstatistics', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY DATE(contribution_civireport.receive_date), currency', $statsSql[2]);
+  }
+
+  /**
+   * Test we don't get a fatal grouping with QUARTER frequency.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testContributionSummaryGroupByWeekFrequency() {
+    $params = [
+      'report_id' => 'contribute/summary',
+      'fields' => ['total_amount' => 1, 'country_id' => 1],
+      'group_bys' => ['receive_date' => 1],
+      'group_bys_freq' => ['receive_date' => 'YEARWEEK'],
+      'options' => ['metadata' => ['sql']],
+    ];
+    $rowsSql = $this->callAPISuccess('report_template', 'getrows', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY YEARWEEK(contribution_civireport.receive_date) WITH ROLLUP', $rowsSql[0]);
+    $statsSql = $this->callAPISuccess('report_template', 'getstatistics', $params)['metadata']['sql'];
+    $this->assertContains('GROUP BY YEARWEEK(contribution_civireport.receive_date), currency', $statsSql[2]);
+  }
+
   /**
    * CRM-20640: Test the group filter works on the contribution summary when a single contact in 2 groups.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testContributionSummaryWithSingleContactsInTwoGroups() {
     list($groupID1, $individualID) = $this->setUpPopulatedGroup(TRUE);
@@ -715,6 +813,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
    * whenever they are hard-added or in the criteria.
    *
    * @return int
+   * @throws \CRM_Core_Exception
    */
   public function setUpPopulatedSmartGroup() {
     $household1ID = $this->householdCreate();
@@ -759,6 +858,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
    * @param bool $returnAddedContact
    *
    * @return int
+   * @throws \CRM_Core_Exception
    */
   public function setUpPopulatedGroup($returnAddedContact = FALSE) {
     $individual1ID = $this->individualCreate();
@@ -793,6 +893,8 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
 
   /**
    * @return array
+   *
+   * @throws \CRM_Core_Exception
    */
   public function setUpIntersectingGroups() {
     $groupID = $this->setUpPopulatedGroup();
@@ -939,6 +1041,8 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
    *
    * @param string $template
    *   Report template unique identifier.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testReportsWithNoTInSmartGroupFilter($template) {
     $groupID = $this->setUpPopulatedGroup();