From e6bab5eae9e7293dff9ecb9852f85e6aa01dad7f Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 8 Jun 2018 13:32:37 +1200 Subject: [PATCH] dev/core#170 fix regression on soft_credits with unit test --- CRM/Report/Form.php | 17 ++ CRM/Report/Form/Contribute/Detail.php | 163 ++++++++++++-------- api/v3/ReportTemplate.php | 2 + tests/phpunit/api/v3/ReportTemplateTest.php | 19 +++ 4 files changed, 133 insertions(+), 68 deletions(-) diff --git a/CRM/Report/Form.php b/CRM/Report/Form.php index 367408a339..dc599e03b2 100644 --- a/CRM/Report/Form.php +++ b/CRM/Report/Form.php @@ -488,6 +488,14 @@ class CRM_Report_Form extends CRM_Core_Form { protected $sqlArray; + /** + * Tables created for the report that need removal afterwards. + * + * ['civicrm_temp_report_x' => ['temporary' => TRUE, 'name' => 'civicrm_temp_report_x'] + * @var array + */ + protected $temporaryTables = []; + /** * Can this report use the sql mode ONLY_FULL_GROUP_BY. * @var bool @@ -1114,6 +1122,15 @@ class CRM_Report_Form extends CRM_Core_Form { return $this->_defaults; } + /** + * Remove any temporary tables. + */ + public function cleanUpTemporaryTables() { + foreach ($this->temporaryTables as $temporaryTable) { + CRM_Core_DAO::executeQuery('DROP ' . ($temporaryTable['temporary'] ? 'TEMPORARY' : '') . ' TABLE IF EXISTS ' . $temporaryTable['name']); + } + } + /** * Add columns to report. */ diff --git a/CRM/Report/Form/Contribute/Detail.php b/CRM/Report/Form/Contribute/Detail.php index 7910fa1453..3619a47409 100644 --- a/CRM/Report/Form/Contribute/Detail.php +++ b/CRM/Report/Form/Contribute/Detail.php @@ -46,6 +46,32 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form { protected $groupConcatTested = TRUE; + protected $isTempTableBuilt = FALSE; + + /** + * Query mode. + * + * This can be 'Main' or 'SoftCredit' to denote which query we are building. + * + * @var string + */ + protected $queryMode = 'Main'; + + /** + * Is this report being run on contributions as the base entity. + * + * The report structure is generally designed around a base entity but + * depending on input it can be run in a sort of hybrid way that causes a lot + * of complexity. + * + * If it is in isContributionsOnlyMode we can simplify. + * + * (arguably there should be 2 separate report templates, not one doing double duty.) + * + * @var bool + */ + protected $isContributionBaseMode = FALSE; + /** * This report has been optimised for group filtering. * @@ -465,34 +491,59 @@ GROUP BY {$this->_aliases['civicrm_contribution']}.currency"; } /** - * This function appears to have been overrriden for the purposes of facilitating soft credits in the report. + * Build the report query. * - * The report appears to have 2 different functions: - * 1) contribution report - * 2) soft credit report - showing a row per 'payment engagement' (payment or soft credit). There is a separate - * soft credit report as well. + * @param bool $applyLimit * - * Somewhat confusingly this report returns multiple rows per contribution when soft credits are included. It feels - * like there is a case to split it into 2 separate reports. - * - * Soft credit functionality is not currently unit tested for this report. + * @return string */ - public function postProcess() { - // @todo in order to make this report testable we need to remove this function override in favour of the - // functions called by the reportTemplate.getrows api - this requires a bit of tidy up! - // get the acl clauses built before we assemble the query - $this->buildACLClause($this->_aliases['civicrm_contact']); + public function buildQuery($applyLimit = TRUE) { + if ($this->isTempTableBuilt) { + return "SELECT * FROM civireport_contribution_detail_temp3 $this->_orderBy"; + } + return parent::buildQuery($applyLimit); + } - $this->beginPostProcess(); + /** + * Shared function for preliminary processing. + * + * This is called by the api / unit tests and the form layer and is + * the right place to do 'initial analysis of input'. + */ + public function beginPostProcessCommon() { + // CRM-18312 - display soft_credits and soft_credits_for column + // when 'Contribution or Soft Credit?' column is not selected + if (empty($this->_params['fields']['contribution_or_soft'])) { + $this->_params['fields']['contribution_or_soft'] = 1; + $this->noDisplayContributionOrSoftColumn = TRUE; + } + if (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) == 'contributions_only') { + $this->isContributionBaseMode = TRUE; + } + if ($this->isContributionBaseMode && + (!empty($this->_params['fields']['soft_credit_type_id']) + || !empty($this->_params['soft_credit_type_id_value'])) + ) { + unset($this->_params['fields']['soft_credit_type_id']); + if (!empty($this->_params['soft_credit_type_id_value'])) { + $this->_params['soft_credit_type_id_value'] = array(); + CRM_Core_Session::setStatus(ts('Is it not possible to filter on soft contribution type when not including soft credits.')); + } + } // 1. use main contribution query to build temp table 1 $sql = $this->buildQuery(); $tempQuery = "CREATE TEMPORARY TABLE civireport_contribution_detail_temp1 {$this->_databaseAttributes} AS {$sql}"; - $this->addToDeveloperTab($tempQuery); - CRM_Core_DAO::executeQuery($tempQuery); + $this->temporaryTables['civireport_contribution_detail_temp1'] = ['name' => 'civireport_contribution_detail_temp1', 'temporary' => TRUE]; + $this->executeReportQuery($tempQuery); $this->setPager(); // 2. customize main contribution query for soft credit, and build temp table 2 with soft credit contributions only + $this->queryMode = 'SoftCredit'; + // Rebuild select with no groupby. Do not let column headers change. + $headers = $this->_columnHeaders; + $this->select(); + $this->_columnHeaders = $headers; $this->softCreditFrom(); // also include custom group from if included // since this might be included in select @@ -504,11 +555,13 @@ GROUP BY {$this->_aliases['civicrm_contribution']}.currency"; if (!empty($this->_groupBy) && !$this->noDisplayContributionOrSoftColumn) { $this->_groupBy .= ', contribution_soft_civireport.amount'; } - // we inner join with temp1 to restrict soft contributions to those in temp1 table - $sql = "{$select} {$this->_from} {$this->_where} {$this->_groupBy}"; + // we inner join with temp1 to restrict soft contributions to those in temp1 table. + // no group by here as we want to display as many soft credit rows as actually exist. + $sql = "{$select} {$this->_from} {$this->_where}"; $tempQuery = "CREATE TEMPORARY TABLE civireport_contribution_detail_temp2 {$this->_databaseAttributes} AS {$sql}"; - $this->addToDeveloperTab($tempQuery); - CRM_Core_DAO::executeQuery($tempQuery); + $this->executeReportQuery($tempQuery); + $this->temporaryTables['civireport_contribution_detail_temp2'] = ['name' => 'civireport_contribution_detail_temp2', 'temporary' => TRUE]; + if (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) == 'soft_credits_only' ) { @@ -527,66 +580,41 @@ GROUP BY {$this->_aliases['civicrm_contribution']}.currency"; $this->customDataFrom(); // 3. Decide where to populate temp3 table from - if (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) == - 'contributions_only' + if ($this->isContributionBaseMode ) { - $tempQuery = "(SELECT * FROM civireport_contribution_detail_temp1)"; + $this->executeReportQuery( + "CREATE TEMPORARY TABLE civireport_contribution_detail_temp3 {$this->_databaseAttributes} AS (SELECT * FROM civireport_contribution_detail_temp1)" + ); } elseif (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) == 'soft_credits_only' ) { - $tempQuery = "(SELECT * FROM civireport_contribution_detail_temp2)"; + $this->executeReportQuery( + "CREATE TEMPORARY TABLE civireport_contribution_detail_temp3 {$this->_databaseAttributes} AS (SELECT * FROM civireport_contribution_detail_temp2)" + ); } else { - $tempQuery = " + $this->executeReportQuery("CREATE TEMPORARY TABLE civireport_contribution_detail_temp3 {$this->_databaseAttributes} (SELECT * FROM civireport_contribution_detail_temp1) UNION ALL -(SELECT * FROM civireport_contribution_detail_temp2)"; +(SELECT * FROM civireport_contribution_detail_temp2)"); } - - // 4. build temp table 3 - $sql = "CREATE TEMPORARY TABLE civireport_contribution_detail_temp3 {$this->_databaseAttributes} AS {$tempQuery}"; - $this->addToDeveloperTab($sql); - CRM_Core_DAO::executeQuery($sql); - - // 6. show result set from temp table 3 - $rows = array(); - $sql = "SELECT * FROM civireport_contribution_detail_temp3 $this->_orderBy"; - $this->buildRows($sql, $rows); - - // format result set. - $this->formatDisplay($rows, FALSE); - - // assign variables to templates - $this->doTemplateAssignment($rows); - // do print / pdf / instance stuff if needed - $this->endPostProcess($rows); + $this->temporaryTables['civireport_contribution_detail_temp3'] = ['name' => 'civireport_contribution_detail_temp3', 'temporary' => TRUE]; + $this->isTempTableBuilt = TRUE; } /** - * Shared function for preliminary processing. + * Store group bys into array - so we can check elsewhere what is grouped. * - * This is called by the api / unit tests and the form layer and is - * the right place to do 'initial analysis of input'. + * If we are generating a table of soft credits we do not want to be using + * group by. */ - public function beginPostProcessCommon() { - // CRM-18312 - display soft_credits and soft_credits_for column - // when 'Contribution or Soft Credit?' column is not selected - if (empty($this->_params['fields']['contribution_or_soft'])) { - $this->_params['fields']['contribution_or_soft'] = 1; - $this->noDisplayContributionOrSoftColumn = TRUE; + protected function storeGroupByArray() { + if ($this->queryMode === 'SoftCredit') { + $this->_groupByArray = []; } - - if (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) == - 'contributions_only' && - (!empty($this->_params['fields']['soft_credit_type_id']) - || !empty($this->_params['soft_credit_type_id_value'])) - ) { - unset($this->_params['fields']['soft_credit_type_id']); - if (!empty($this->_params['soft_credit_type_id_value'])) { - $this->_params['soft_credit_type_id_value'] = array(); - CRM_Core_Session::setStatus(ts('Is it not possible to filter on soft contribution type when not including soft credits.')); - } + else { + parent::storeGroupByArray(); } } @@ -600,7 +628,6 @@ UNION ALL * Rows generated by SQL, with an array for each row. */ public function alterDisplay(&$rows) { - $checkList = array(); $entryFound = FALSE; $display_flag = $prev_cid = $cid = 0; $contributionTypes = CRM_Contribute_PseudoConstant::financialType(); @@ -717,7 +744,7 @@ UNION ALL array_key_exists('civicrm_contribution_contribution_id', $row) ) { $query = " -SELECT civicrm_contact_id, civicrm_contact_sort_name, civicrm_contribution_total_amount_sum, civicrm_contribution_currency +SELECT civicrm_contact_id, civicrm_contact_sort_name, civicrm_contribution_total_amount, civicrm_contribution_currency FROM civireport_contribution_detail_temp2 WHERE civicrm_contribution_contribution_id={$row['civicrm_contribution_contribution_id']}"; $this->addToDeveloperTab($query); @@ -729,7 +756,7 @@ WHERE civicrm_contribution_contribution_id={$row['civicrm_contribution_contribu $dao->civicrm_contact_id); $string = $string . ($string ? $separator : '') . "{$dao->civicrm_contact_sort_name} " . - CRM_Utils_Money::format($dao->civicrm_contribution_total_amount_sum, $dao->civicrm_contribution_currency); + CRM_Utils_Money::format($dao->civicrm_contribution_total_amount, $dao->civicrm_contribution_currency); } $rows[$rowNum]['civicrm_contribution_soft_credits'] = $string; } diff --git a/api/v3/ReportTemplate.php b/api/v3/ReportTemplate.php index e121c41146..d13cd54b44 100644 --- a/api/v3/ReportTemplate.php +++ b/api/v3/ReportTemplate.php @@ -113,6 +113,7 @@ function civicrm_api3_report_template_delete($params) { function civicrm_api3_report_template_getrows($params) { civicrm_api3_verify_one_mandatory($params, NULL, array('report_id', 'instance_id')); list($rows, $instance, $metadata) = _civicrm_api3_report_template_getrows($params); + $instance->cleanUpTemporaryTables(); return civicrm_api3_create_success($rows, $params, 'ReportTemplate', 'getrows', CRM_Core_DAO::$_nullObject, $metadata); } @@ -187,6 +188,7 @@ 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); + $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 2820cdcd19..118a3e57f3 100644 --- a/tests/phpunit/api/v3/ReportTemplateTest.php +++ b/tests/phpunit/api/v3/ReportTemplateTest.php @@ -432,7 +432,26 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { 'options' => array('metadata' => array('sql')), )); $this->assertEquals(2, $rows['values'][0]['civicrm_contribution_total_amount_count']); + } + /** + * Test the group filter works on the contribution summary. + */ + public function testContributionDetailSoftCredits() { + $contactID = $this->individualCreate(); + $contactID2 = $this->individualCreate(); + $this->contributionCreate(['contact_id' => $contactID, 'api.ContributionSoft.create' => ['amount' => 5, 'contact_id' => $contactID2]]); + $template = 'contribute/detail'; + $rows = $this->callAPISuccess('report_template', 'getrows', array( + 'report_id' => $template, + 'contribution_or_soft_value' => 'contributions_only', + 'fields' => ['soft_credits' => 1, 'contribution_or_soft' => 1, 'sort_name' => 1], + 'options' => array('metadata' => array('sql')), + )); + $this->assertEquals( + "Anderson, Anthony $ 5.00", + $rows['values'][0]['civicrm_contribution_soft_credits'] + ); } /** -- 2.25.1