From 63dc1f23a2aa81608ab3e9830b41402802be2d66 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 1 May 2018 18:20:14 +1200 Subject: [PATCH] Enable tests on the logging summary report. Despite seeming like a lot of change this is mostly just extraction to run via the tests. I can break out into a couple of refactor commits for reviewablility. I have tested this on mysql 5.6 with full group by mode enabled. --- CRM/Core/DAO.php | 4 +- CRM/Logging/ReportSummary.php | 198 +++++++++++--------- CRM/Report/Form/Contact/LoggingSummary.php | 6 + CRM/Utils/SQL.php | 3 - tests/phpunit/api/v3/ReportTemplateTest.php | 12 +- 5 files changed, 131 insertions(+), 92 deletions(-) diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index d7e148051e..5d86562d41 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -169,7 +169,7 @@ class CRM_Core_DAO extends DB_DataObject { */ public static function disableFullGroupByMode() { $currentModes = CRM_Utils_SQL::getSqlModes(); - if (CRM_Utils_SQL::supportsFullGroupBy() && in_array('ONLY_FULL_GROUP_BY', $currentModes) && CRM_Utils_SQL::isGroupByModeInDefault()) { + if (in_array('ONLY_FULL_GROUP_BY', $currentModes) && CRM_Utils_SQL::isGroupByModeInDefault()) { $key = array_search('ONLY_FULL_GROUP_BY', $currentModes); unset($currentModes[$key]); CRM_Core_DAO::executeQuery("SET SESSION sql_mode = %1", array(1 => array(implode(',', $currentModes), 'String'))); @@ -181,7 +181,7 @@ class CRM_Core_DAO extends DB_DataObject { */ public static function reenableFullGroupByMode() { $currentModes = CRM_Utils_SQL::getSqlModes(); - if (CRM_Utils_SQL::supportsFullGroupBy() && !in_array('ONLY_FULL_GROUP_BY', $currentModes) && CRM_Utils_SQL::isGroupByModeInDefault()) { + if (!in_array('ONLY_FULL_GROUP_BY', $currentModes) && CRM_Utils_SQL::isGroupByModeInDefault()) { $currentModes[] = 'ONLY_FULL_GROUP_BY'; CRM_Core_DAO::executeQuery("SET SESSION sql_mode = %1", array(1 => array(implode(',', $currentModes), 'String'))); } diff --git a/CRM/Logging/ReportSummary.php b/CRM/Logging/ReportSummary.php index 9cb352354d..5d3f98d853 100644 --- a/CRM/Logging/ReportSummary.php +++ b/CRM/Logging/ReportSummary.php @@ -39,20 +39,20 @@ class CRM_Logging_ReportSummary extends CRM_Report_Form { protected $loggingDB; /** - * The log table currently being processed. + * Clause used in the final run of buildQuery but not when doing preliminary work. + * + * (We do this to all the api to run this report since it doesn't call postProcess). * * @var string */ - protected $currentLogTable; + protected $logTypeTableClause; /** - * Clause used in the final run of buildQuery but not when doing preliminary work. - * - * (We do this to all the api to run this report since it doesn't call postProcess). + * The log table currently being processed. * * @var string */ - protected $logTypeTableClause; + protected $currentLogTable; /** * Class constructor. @@ -236,84 +236,7 @@ class CRM_Logging_ReportSummary extends CRM_Report_Form { $this->beginPostProcess(); $rows = array(); - $tempColumns = "id int(10), log_civicrm_entity_log_grouping varchar(32)"; - if (!empty($this->_params['fields']['log_action'])) { - $tempColumns .= ", log_action varchar(64)"; - } - $tempColumns .= ", log_type varchar(64), log_user_id int(10), log_date timestamp"; - if (!empty($this->_params['fields']['altered_contact'])) { - $tempColumns .= ", altered_contact varchar(128)"; - } - $tempColumns .= ", altered_contact_id int(10), log_conn_id varchar(17), is_deleted tinyint(4)"; - if (!empty($this->_params['fields']['display_name'])) { - $tempColumns .= ", display_name varchar(128)"; - } - - // temp table to hold all altered contact-ids - $sql = "CREATE TEMPORARY TABLE civicrm_temp_civireport_logsummary ( {$tempColumns} ) ENGINE=HEAP"; - CRM_Core_DAO::executeQuery($sql); - $this->addToDeveloperTab($sql); - - $logTypes = CRM_Utils_Array::value('log_type_value', $this->_params); - unset($this->_params['log_type_value']); - if (empty($logTypes)) { - foreach (array_keys($this->_logTables) as $table) { - $type = $this->getLogType($table); - $logTypes[$type] = $type; - } - } - - $this->logTypeTableClause = '(1)'; - if ($logTypeTableValue = CRM_Utils_Array::value("log_type_table_value", $this->_params)) { - $this->logTypeTableClause = $this->whereClause($this->_columns['log_civicrm_entity']['filters']['log_type_table'], - $this->_params['log_type_table_op'], $logTypeTableValue, NULL, NULL); - unset($this->_params['log_type_table_value']); - } - - foreach ($this->_logTables as $entity => $detail) { - if ((in_array($this->getLogType($entity), $logTypes) && - CRM_Utils_Array::value('log_type_op', $this->_params) == 'in') || - (!in_array($this->getLogType($entity), $logTypes) && - CRM_Utils_Array::value('log_type_op', $this->_params) == 'notin') - ) { - $this->currentLogTable = $entity; - $sql = $this->buildQuery(FALSE); - $sql = str_replace("entity_log_civireport.log_type as", "'{$entity}' as", $sql); - $sql = "INSERT IGNORE INTO civicrm_temp_civireport_logsummary {$sql}"; - CRM_Core_DAO::executeQuery($sql); - $this->addToDeveloperTab($sql); - } - } - - $this->currentLogTable = ''; - - // add computed log_type column so that we can do a group by after that, which will help - // alterDisplay() counts sync with pager counts - $sql = "SELECT DISTINCT log_type FROM civicrm_temp_civireport_logsummary"; - $dao = CRM_Core_DAO::executeQuery($sql); - $this->addToDeveloperTab($sql); - $replaceWith = array(); - while ($dao->fetch()) { - $type = $this->getLogType($dao->log_type); - if (!array_key_exists($type, $replaceWith)) { - $replaceWith[$type] = array(); - } - $replaceWith[$type][] = $dao->log_type; - } - foreach ($replaceWith as $type => $tables) { - if (!empty($tables)) { - $replaceWith[$type] = implode("','", $tables); - } - } - - $sql = "ALTER TABLE civicrm_temp_civireport_logsummary ADD COLUMN log_civicrm_entity_log_type_label varchar(64)"; - CRM_Core_DAO::executeQuery($sql); - $this->addToDeveloperTab($sql); - foreach ($replaceWith as $type => $in) { - $sql = "UPDATE civicrm_temp_civireport_logsummary SET log_civicrm_entity_log_type_label='{$type}', log_date=log_date WHERE log_type IN('$in')"; - CRM_Core_DAO::executeQuery($sql); - $this->addToDeveloperTab($sql); - } + $this->buildTemporaryTables(); $sql = $this->buildQuery(); $this->buildRows($sql, $rows); @@ -433,6 +356,101 @@ WHERE log_date <= %1 AND id = %2 ORDER BY log_date DESC LIMIT 1"; return NULL; } + /** + * Build the temporary tables for the query. + */ + protected function buildTemporaryTables() { + $tempColumns = "id int(10), log_civicrm_entity_log_grouping varchar(32)"; + if (!empty($this->_params['fields']['log_action'])) { + $tempColumns .= ", log_action varchar(64)"; + } + $tempColumns .= ", log_type varchar(64), log_user_id int(10), log_date timestamp"; + if (!empty($this->_params['fields']['altered_contact'])) { + $tempColumns .= ", altered_contact varchar(128)"; + } + $tempColumns .= ", altered_contact_id int(10), log_conn_id varchar(17), is_deleted tinyint(4)"; + if (!empty($this->_params['fields']['display_name'])) { + $tempColumns .= ", display_name varchar(128)"; + } + + // temp table to hold all altered contact-ids + $sql = "CREATE TEMPORARY TABLE civicrm_temp_civireport_logsummary ( {$tempColumns} ) ENGINE=HEAP"; + CRM_Core_DAO::executeQuery($sql); + $this->addToDeveloperTab($sql); + + $logTypes = CRM_Utils_Array::value('log_type_value', $this->_params); + unset($this->_params['log_type_value']); + if (empty($logTypes)) { + foreach (array_keys($this->_logTables) as $table) { + $type = $this->getLogType($table); + $logTypes[$type] = $type; + } + } + + $logTypeTableClause = '(1)'; + if ($logTypeTableValue = CRM_Utils_Array::value("log_type_table_value", $this->_params)) { + $logTypeTableClause = $this->whereClause($this->_columns['log_civicrm_entity']['filters']['log_type_table'], + $this->_params['log_type_table_op'], $logTypeTableValue, NULL, NULL); + unset($this->_params['log_type_table_value']); + } + + foreach ($this->_logTables as $entity => $detail) { + if ((in_array($this->getLogType($entity), $logTypes) && + CRM_Utils_Array::value('log_type_op', $this->_params) == 'in') || + (!in_array($this->getLogType($entity), $logTypes) && + CRM_Utils_Array::value('log_type_op', $this->_params) == 'notin') + ) { + $this->currentLogTable = $entity; + $sql = $this->buildQuery(FALSE); + $sql = str_replace("entity_log_civireport.log_type as", "'{$entity}' as", $sql); + $sql = "INSERT IGNORE INTO civicrm_temp_civireport_logsummary {$sql}"; + CRM_Core_DAO::disableFullGroupByMode(); + CRM_Core_DAO::executeQuery($sql); + CRM_Core_DAO::reenableFullGroupByMode(); + $this->addToDeveloperTab($sql); + } + } + + $this->currentLogTable = ''; + + // add computed log_type column so that we can do a group by after that, which will help + // alterDisplay() counts sync with pager counts + $sql = "SELECT DISTINCT log_type FROM civicrm_temp_civireport_logsummary"; + $dao = CRM_Core_DAO::executeQuery($sql); + $this->addToDeveloperTab($sql); + $replaceWith = array(); + while ($dao->fetch()) { + $type = $this->getLogType($dao->log_type); + if (!array_key_exists($type, $replaceWith)) { + $replaceWith[$type] = array(); + } + $replaceWith[$type][] = $dao->log_type; + } + foreach ($replaceWith as $type => $tables) { + if (!empty($tables)) { + $replaceWith[$type] = implode("','", $tables); + } + } + + $sql = "ALTER TABLE civicrm_temp_civireport_logsummary ADD COLUMN log_civicrm_entity_log_type_label varchar(64)"; + CRM_Core_DAO::executeQuery($sql); + $this->addToDeveloperTab($sql); + foreach ($replaceWith as $type => $in) { + $sql = "UPDATE civicrm_temp_civireport_logsummary SET log_civicrm_entity_log_type_label='{$type}', log_date=log_date WHERE log_type IN('$in')"; + CRM_Core_DAO::executeQuery($sql); + $this->addToDeveloperTab($sql); + } + $this->logTypeTableClause = $logTypeTableClause; + } + + /** + * Common processing, also via api/unit tests. + */ + public function beginPostProcessCommon() { + parent::beginPostProcessCommon(); + $this->buildTemporaryTables(); + } + /** * Build the report query. * @@ -464,4 +482,16 @@ GROUP BY log_civicrm_entity_log_date, log_civicrm_entity_log_type_label, log_civ return $sql; } + /** + * Build output rows. + * + * @param string $sql + * @param array $rows + */ + public function buildRows($sql, &$rows) { + parent::buildRows($sql, $rows); + // Clean up the temp table - mostly for the unit test. + CRM_Core_DAO::executeQuery('DROP TEMPORARY TABLE IF EXISTS civicrm_temp_civireport_logsummary'); + } + } diff --git a/CRM/Report/Form/Contact/LoggingSummary.php b/CRM/Report/Form/Contact/LoggingSummary.php index 857c767bb2..6aaddeb3b3 100644 --- a/CRM/Report/Form/Contact/LoggingSummary.php +++ b/CRM/Report/Form/Contact/LoggingSummary.php @@ -31,6 +31,8 @@ * @copyright CiviCRM LLC (c) 2004-2018 */ class CRM_Report_Form_Contact_LoggingSummary extends CRM_Logging_ReportSummary { + + public $optimisedForOnlyFullGroupBy = FALSE; /** * Class constructor. */ @@ -299,6 +301,10 @@ class CRM_Report_Form_Contact_LoggingSummary extends CRM_Logging_ReportSummary { * Generate From Clause. */ public function from() { + if (!$this->currentLogTable) { + // From has already been built in this case. + return; + } $entity = $this->currentLogTable; $detail = $this->_logTables[$entity]; diff --git a/CRM/Utils/SQL.php b/CRM/Utils/SQL.php index 3a90ee05d1..f8bc37a16d 100644 --- a/CRM/Utils/SQL.php +++ b/CRM/Utils/SQL.php @@ -112,9 +112,6 @@ class CRM_Utils_SQL { * @return bool */ public static function isGroupByModeInDefault() { - if (!self::supportsFullGroupBy()) { - return FALSE; - } $sqlModes = explode(',', CRM_Core_DAO::singleValueQuery('SELECT @@global.sql_mode')); if (!in_array('ONLY_FULL_GROUP_BY', $sqlModes)) { return FALSE; diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php index 16ad8868bb..96718a185f 100644 --- a/tests/phpunit/api/v3/ReportTemplateTest.php +++ b/tests/phpunit/api/v3/ReportTemplateTest.php @@ -140,7 +140,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { } /** - * Tet api to get rows from reports. + * Test api to get rows from reports. * * @dataProvider getReportTemplates * @@ -149,12 +149,20 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { * @throws \PHPUnit_Framework_IncompleteTestError */ public function testReportTemplateGetRowsAllReports($reportID) { + //$reportID = 'logging/contact/summary'; if (stristr($reportID, 'has existing issues')) { $this->markTestIncomplete($reportID); } + if (substr($reportID, 0, '7') === 'logging') { + Civi::settings()->set('logging', 1); + } + $this->callAPISuccess('report_template', 'getrows', array( 'report_id' => $reportID, )); + if (substr($reportID, 0, '7') === 'logging') { + Civi::settings()->set('logging', 0); + } } /** @@ -189,8 +197,6 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { $reportsToSkip = array( 'activity' => 'does not respect function signature on from clause', 'event/income' => 'I do no understand why but error is Call to undefined method CRM_Report_Form_Event_Income::from() in CRM/Report/Form.php on line 2120', - 'logging/contact/summary' => '(likely to be test related) probably logging off Undefined index: Form/Contact/LoggingSummary.php(231): PHP', - 'logging/contribute/summary' => '(likely to be test related) probably logging off DB Error: no such table', 'contribute/history' => 'Declaration of CRM_Report_Form_Contribute_History::buildRows() should be compatible with CRM_Report_Form::buildRows($sql, &$rows)', 'activitySummary' => 'We use temp tables for the main query generation and name are dynamic. These names are not available in stats() when called directly.', ); -- 2.25.1