From d47468d0da5a531e833d1ea201ccb94ba42a0dc9 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 11 Jul 2018 15:58:27 +1200 Subject: [PATCH] Fix activity report to bring it under standardised report testing --- CRM/Report/Form/Activity.php | 199 ++++++++++++-------- tests/phpunit/api/v3/ReportTemplateTest.php | 1 - 2 files changed, 116 insertions(+), 84 deletions(-) diff --git a/CRM/Report/Form/Activity.php b/CRM/Report/Form/Activity.php index 9f2680e06f..5cb371d84d 100644 --- a/CRM/Report/Form/Activity.php +++ b/CRM/Report/Form/Activity.php @@ -91,6 +91,9 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { $condition = " AND ( v.component_id IS NULL {$include} )"; $this->activityTypes = CRM_Core_OptionGroup::values('activity_type', FALSE, FALSE, FALSE, $condition); asort($this->activityTypes); + + // @todo split the 3 different contact tables into their own array items. + // this will massively simplify the needs of this report. $this->_columns = array( 'civicrm_contact' => array( 'dao' => 'CRM_Contact_DAO_Contact', @@ -394,7 +397,12 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { /** * Build select clause. * - * @param null $recordType + * @todo get rid of $recordType param. It's only because 3 separate contact tables + * are mis-declared as one that we need it. + * + * @param null $recordType deprecated + * Parameter to hack around the bad decision made in construct to misrepresent + * different tables as the same table. */ public function select($recordType = 'target') { if (!array_key_exists("contact_{$recordType}", $this->_params['fields']) && @@ -416,6 +424,7 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { $removeKeys = array(); if ($recordType == 'target') { + // @todo - fix up the way the tables are declared in construct & remove this. foreach ($this->_selectClauses as $key => $clause) { if (strstr($clause, 'civicrm_contact_assignee.') || strstr($clause, 'civicrm_contact_source.') || @@ -430,6 +439,7 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { } } elseif ($recordType == 'assignee') { + // @todo - fix up the way the tables are declared in construct & remove this. foreach ($this->_selectClauses as $key => $clause) { if (strstr($clause, 'civicrm_contact_target.') || strstr($clause, 'civicrm_contact_source.') || @@ -444,6 +454,7 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { } } elseif ($recordType == 'source') { + // @todo - fix up the way the tables are declared in construct & remove this. foreach ($this->_selectClauses as $key => $clause) { if (strstr($clause, 'civicrm_contact_target.') || strstr($clause, 'civicrm_contact_assignee.') || @@ -460,6 +471,7 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { elseif ($recordType == 'final') { $this->_selectClauses = $this->_selectAliasesTotal; foreach ($this->_selectClauses as $key => $clause) { + // @todo - fix up the way the tables are declared in construct & remove this. if (strstr($clause, 'civicrm_contact_contact_target') || strstr($clause, 'civicrm_contact_contact_assignee') || strstr($clause, 'civicrm_contact_contact_source') || @@ -496,91 +508,35 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { /** * Build from clause. - * - * @param string $recordType + * @todo remove this function & declare the 3 contact tables separately */ - public function from($recordType = 'target') { + public function from() { $activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate'); - $activityTypeId = CRM_Core_DAO::getFieldValue("CRM_Core_DAO_OptionGroup", 'activity_type', 'id', 'name'); - $assigneeID = CRM_Utils_Array::key('Activity Assignees', $activityContacts); $targetID = CRM_Utils_Array::key('Activity Targets', $activityContacts); - $sourceID = CRM_Utils_Array::key('Activity Source', $activityContacts); - - if ($recordType == 'target') { - $this->_from = " - FROM civicrm_activity {$this->_aliases['civicrm_activity']} - INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} - ON {$this->_aliases['civicrm_activity']}.id = {$this->_aliases['civicrm_activity_contact']}.activity_id AND - {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$targetID} - INNER JOIN civicrm_contact civicrm_contact_target - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_contact_target.id - {$this->_aclFrom}"; - - if ($this->isTableSelected('civicrm_email')) { - $this->_from .= " - LEFT JOIN civicrm_email civicrm_email_target - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_email_target.contact_id AND - civicrm_email_target.is_primary = 1"; - } - if ($this->isTableSelected('civicrm_phone')) { - $this->_from .= " - LEFT JOIN civicrm_phone civicrm_phone_target - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_phone_target.contact_id AND - civicrm_phone_target.is_primary = 1 "; - } - $this->_aliases['civicrm_contact'] = 'civicrm_contact_target'; + $this->_from = " + FROM civicrm_activity {$this->_aliases['civicrm_activity']} + INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} + ON {$this->_aliases['civicrm_activity']}.id = {$this->_aliases['civicrm_activity_contact']}.activity_id AND + {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$targetID} + INNER JOIN civicrm_contact civicrm_contact_target + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_contact_target.id + {$this->_aclFrom}"; + + if ($this->isTableSelected('civicrm_email')) { + $this->_from .= " + LEFT JOIN civicrm_email civicrm_email_target + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_email_target.contact_id AND + civicrm_email_target.is_primary = 1"; } - if ($recordType == 'assignee') { - $this->_from = " - FROM civicrm_activity {$this->_aliases['civicrm_activity']} - INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} - ON {$this->_aliases['civicrm_activity']}.id = {$this->_aliases['civicrm_activity_contact']}.activity_id AND - {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$assigneeID} - INNER JOIN civicrm_contact civicrm_contact_assignee - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_contact_assignee.id - {$this->_aclFrom}"; - - if ($this->isTableSelected('civicrm_email')) { - $this->_from .= " - LEFT JOIN civicrm_email civicrm_email_assignee - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_email_assignee.contact_id AND - civicrm_email_assignee.is_primary = 1"; - } - if ($this->isTableSelected('civicrm_phone')) { - $this->_from .= " - LEFT JOIN civicrm_phone civicrm_phone_assignee - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_phone_assignee.contact_id AND - civicrm_phone_assignee.is_primary = 1 "; - } - $this->_aliases['civicrm_contact'] = 'civicrm_contact_assignee'; - } - - if ($recordType == 'source') { - $this->_from = " - FROM civicrm_activity {$this->_aliases['civicrm_activity']} - INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} - ON {$this->_aliases['civicrm_activity']}.id = {$this->_aliases['civicrm_activity_contact']}.activity_id AND - {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$sourceID} - INNER JOIN civicrm_contact civicrm_contact_source - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_contact_source.id - {$this->_aclFrom}"; - - if ($this->isTableSelected('civicrm_email')) { - $this->_from .= " - LEFT JOIN civicrm_email civicrm_email_source - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_email_source.contact_id AND - civicrm_email_source.is_primary = 1"; - } - if ($this->isTableSelected('civicrm_phone')) { - $this->_from .= " - LEFT JOIN civicrm_phone civicrm_phone_source - ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_phone_source.contact_id AND - civicrm_phone_source.is_primary = 1 "; - } - $this->_aliases['civicrm_contact'] = 'civicrm_contact_source'; + if ($this->isTableSelected('civicrm_phone')) { + $this->_from .= " + LEFT JOIN civicrm_phone civicrm_phone_target + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_phone_target.contact_id AND + civicrm_phone_target.is_primary = 1 "; } + $this->_aliases['civicrm_contact'] = 'civicrm_contact_target'; $this->joinAddressFromContact(); } @@ -588,6 +544,9 @@ class CRM_Report_Form_Activity extends CRM_Report_Form { /** * Build where clause. * + * @todo get rid of $recordType param. It's only because 3 separate contact tables + * are mis-declared as one that we need it. + * * @param string $recordType */ public function where($recordType = NULL) { @@ -805,10 +764,13 @@ GROUP BY civicrm_activity_id $having {$this->_orderBy}"; } } + // @todo - all this temp table stuff is here because pre 4.4 the activity contact + // form did not exist. + // Fixing the way the construct method declares them will make all this redundant. // 1. fill temp table with target results $this->buildACLClause(array('civicrm_contact_target')); $this->select('target'); - $this->from('target'); + $this->from(); $this->customDataFrom(); $this->where('target'); $insertCols = implode(',', $this->_selectAliases); @@ -834,7 +796,8 @@ GROUP BY civicrm_activity_id $having {$this->_orderBy}"; // 3. fill temp table with assignee results $this->buildACLClause(array('civicrm_contact_assignee')); $this->select('assignee'); - $this->from('assignee'); + $this->buildAssigneeFrom(); + $this->customDataFrom(); $this->where('assignee'); $insertCols = implode(',', $this->_selectAliases); @@ -846,7 +809,7 @@ GROUP BY civicrm_activity_id $having {$this->_orderBy}"; // 4. fill temp table with source results $this->buildACLClause(array('civicrm_contact_source')); $this->select('source'); - $this->from('source'); + $this->buildSourceFrom(); $this->customDataFrom(); $this->where('source'); $insertCols = implode(',', $this->_selectAliases); @@ -930,7 +893,9 @@ GROUP BY civicrm_activity_id $having {$this->_orderBy}"; $activityStatus = CRM_Core_PseudoConstant::activityStatus(); $priority = CRM_Core_PseudoConstant::get('CRM_Activity_DAO_Activity', 'priority_id'); $viewLinks = FALSE; - $context = CRM_Utils_Request::retrieve('context', 'String', $this, FALSE, 'report'); + // Would we ever want to retrieve from the form controller?? + $form = $this->noController ? NULL : $this; + $context = CRM_Utils_Request::retrieve('context', 'Alphanumeric', $form, FALSE, 'report'); $actUrl = ''; if (CRM_Core_Permission::check('access CiviCRM')) { @@ -1147,4 +1112,72 @@ GROUP BY civicrm_activity_id $having {$this->_orderBy}"; } } + /** + * @todo remove this function & declare the 3 contact tables separately + * + * (Currently the construct method incorrectly melds them - this is an interim + * refactor in order to get this under ReportTemplateTests) + */ + protected function buildAssigneeFrom() { + $activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate'); + $assigneeID = CRM_Utils_Array::key('Activity Assignees', $activityContacts); + $this->_from = " + FROM civicrm_activity {$this->_aliases['civicrm_activity']} + INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} + ON {$this->_aliases['civicrm_activity']}.id = {$this->_aliases['civicrm_activity_contact']}.activity_id AND + {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$assigneeID} + INNER JOIN civicrm_contact civicrm_contact_assignee + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_contact_assignee.id + {$this->_aclFrom}"; + + if ($this->isTableSelected('civicrm_email')) { + $this->_from .= " + LEFT JOIN civicrm_email civicrm_email_assignee + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_email_assignee.contact_id AND + civicrm_email_assignee.is_primary = 1"; + } + if ($this->isTableSelected('civicrm_phone')) { + $this->_from .= " + LEFT JOIN civicrm_phone civicrm_phone_assignee + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_phone_assignee.contact_id AND + civicrm_phone_assignee.is_primary = 1 "; + } + $this->_aliases['civicrm_contact'] = 'civicrm_contact_assignee'; + $this->joinAddressFromContact(); + } + + /** + * @todo remove this function & declare the 3 contact tables separately + * + * (Currently the construct method incorrectly melds them - this is an interim + * refactor in order to get this under ReportTemplateTests) + */ + protected function buildSourceFrom() { + $activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate'); + $sourceID = CRM_Utils_Array::key('Activity Source', $activityContacts); + $this->_from = " + FROM civicrm_activity {$this->_aliases['civicrm_activity']} + INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} + ON {$this->_aliases['civicrm_activity']}.id = {$this->_aliases['civicrm_activity_contact']}.activity_id AND + {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$sourceID} + INNER JOIN civicrm_contact civicrm_contact_source + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_contact_source.id + {$this->_aclFrom}"; + + if ($this->isTableSelected('civicrm_email')) { + $this->_from .= " + LEFT JOIN civicrm_email civicrm_email_source + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_email_source.contact_id AND + civicrm_email_source.is_primary = 1"; + } + if ($this->isTableSelected('civicrm_phone')) { + $this->_from .= " + LEFT JOIN civicrm_phone civicrm_phone_source + ON {$this->_aliases['civicrm_activity_contact']}.contact_id = civicrm_phone_source.contact_id AND + civicrm_phone_source.is_primary = 1 "; + } + $this->_aliases['civicrm_contact'] = 'civicrm_contact_source'; + $this->joinAddressFromContact(); + } + } diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php index 5d73da06c4..6f3d8c95fc 100644 --- a/tests/phpunit/api/v3/ReportTemplateTest.php +++ b/tests/phpunit/api/v3/ReportTemplateTest.php @@ -246,7 +246,6 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { */ public static function getReportTemplates() { $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', '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