From bd2913d320fd7170f4899cede3642b96b50aa683 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 17 Feb 2020 12:48:23 +1300 Subject: [PATCH] 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 --- CRM/Report/Form.php | 20 +++- CRM/Report/Form/Contribute/Summary.php | 64 ++++-------- api/v3/ReportTemplate.php | 4 + tests/phpunit/api/v3/ReportTemplateTest.php | 104 ++++++++++++++++++++ 4 files changed, 143 insertions(+), 49 deletions(-) diff --git a/CRM/Report/Form.php b/CRM/Report/Form.php index 669386a5b3..47adf78eed 100644 --- 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 c1ae2db628..5a4de70434 100644 --- 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 8f0aba9fa1..5d2e98fe61 100644 --- 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 b87981643c..038b076741 100644 --- 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(); -- 2.25.1