Merge pull request #11660 from JMAConsulting/CRM-21754
authorcolemanw <coleman@civicrm.org>
Mon, 16 Jul 2018 22:12:21 +0000 (18:12 -0400)
committerGitHub <noreply@github.com>
Mon, 16 Jul 2018 22:12:21 +0000 (18:12 -0400)
CRM-21754: Duplicate rows in Activity Details report when address fields are displayed

1  2 
CRM/Report/Form.php
CRM/Report/Form/Activity.php
tests/phpunit/CRM/Report/Form/ActivityTest.php

diff --combined CRM/Report/Form.php
index 89f20ab751c133115bc4d93ab143ec01d5bb9275,7615c3dfbdb3dba67fccf56cf77e64f16e1dec96..f3dbf456e87b073fbf00e46df2d00e96dd9dc172
@@@ -1,9 -1,9 +1,9 @@@
  <?php
  /*
    +--------------------------------------------------------------------+
 -  | CiviCRM version 4.7                                                |
 +  | CiviCRM version 5                                                  |
    +--------------------------------------------------------------------+
 -  | Copyright CiviCRM LLC (c) 2004-2017                                |
 +  | Copyright CiviCRM LLC (c) 2004-2018                                |
    +--------------------------------------------------------------------+
    | This file is a part of CiviCRM.                                    |
    |                                                                    |
@@@ -203,10 -203,6 +203,10 @@@ class CRM_Report_Form extends CRM_Core_
     */
    protected $addPaging = TRUE;
  
 +  protected $isForceGroupBy = FALSE;
 +
 +  protected $groupConcatTested = FALSE;
 +
    /**
     * An attribute for checkbox/radio form field layout
     *
  
    protected $sql;
  
 +  /**
 +   * An instruction not to add a Group By.
 +   *
 +   * This is relevant where the group by might be otherwise added after the code that determines the group by array.
 +   *
 +   * e.g. where stat fields are being added but other settings cause it to not be desirable to add a group by
 +   * such as in pivot charts when no row header is set
 +   *
 +   * @var bool
 +   */
 +  protected $noGroupBy = FALSE;
 +
    /**
     * SQL being run in this report as an array.
     *
     */
  
    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
 +   */
 +  public $optimisedForOnlyFullGroupBy = TRUE;
    /**
     * Class constructor.
     */
                $this->_columns[$tableName][$fieldGrp][$fieldName]['name'] = $name;
              }
  
 +            if (!isset($this->_columns[$tableName][$fieldGrp][$fieldName]['table_name'])) {
 +              $this->_columns[$tableName][$fieldGrp][$fieldName]['table_name'] = $tableName;
 +            }
 +
              // set dbAlias = alias.name, unless already set
              if (!isset($this->_columns[$tableName][$fieldGrp][$fieldName]['dbAlias'])) {
                $this->_columns[$tableName][$fieldGrp][$fieldName]['dbAlias']
              if (!isset($this->_columns[$tableName]['metadata'][$fieldName])) {
                $this->_columns[$tableName]['metadata'][$fieldName] = $this->_columns[$tableName][$fieldGrp][$fieldName];
              }
 +            else {
 +              $this->_columns[$tableName]['metadata'][$fieldName] = array_merge($this->_columns[$tableName][$fieldGrp][$fieldName], $this->_columns[$tableName]['metadata'][$fieldName]);
 +            }
            }
          }
        }
        if (array_key_exists('fields', $table)) {
          foreach ($table['fields'] as $fieldName => $field) {
            if (empty($field['no_display'])) {
 -            if (isset($field['required'])) {
 +            if (!empty($field['required'])) {
                // set default
                $this->_defaults['fields'][$fieldName] = 1;
  
      $this->_params = $params;
    }
  
 +  /**
 +   * Getter for $_params.
 +   *
 +   * @return void|array $params
 +   */
 +  public function getParams() {
 +    return $this->_params;
 +  }
 +
    /**
     * Setter for $_id.
     *
      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']);
 +    }
 +  }
 +
 +  /**
 +   * Create a temporary table.
 +   *
 +   * This function creates a table AND adds the details to the developer tab & $this->>temporary tables.
 +   *
 +   * @todo improve presentation on the developer tab since CREATE TEMPORARY is removed.
 +   *
 +   * @param string $identifier
 +   * @param $sql
 +   * @param bool $isTrueTemporary
 +   *   Is this a mysql temporary table or temporary in a less technical sense.
 +   *
 +   * @return string
 +   */
 +  public function createTemporaryTable($identifier, $sql, $isTrueTemporary = TRUE) {
 +    $this->addToDeveloperTab($sql);
 +    $name = CRM_Utils_SQL_TempTable::build()->setUtf8(TRUE)->setDurable($isTrueTemporary)->createWithQuery($sql)->getName();
 +    $this->temporaryTables[$identifier] = ['temporary' => $isTrueTemporary, 'name' => $name];
 +    return $name;
 +  }
 +
    /**
     * Add columns to report.
     */
      foreach ($this->_columns as $tableName => $table) {
        if (array_key_exists('group_bys', $table)) {
          foreach ($table['group_bys'] as $fieldName => $field) {
 -          if (!empty($field)) {
 +          if (!empty($field) && empty($field['no_display'])) {
              $options[$field['title']] = $fieldName;
              if (!empty($field['frequency'])) {
                $freqElements[$field['title']] = $fieldName;
    /**
     * Get values for from and to for date ranges.
     *
 +   * @deprecated
 +   *
     * @param bool $relative
     * @param string $from
     * @param string $to
     */
    public function getFromTo($relative, $from, $to, $fromTime = NULL, $toTime = NULL) {
      if (empty($toTime)) {
 +      // odd legacy behaviour to treat NULL as 'end of the day'
 +      // recommend updating reports to call CRM_Utils_Date::getFromTo
 +      //directly (default on the function is the actual default there).
        $toTime = '235959';
      }
 -    //FIX ME not working for relative
 -    if ($relative) {
 -      list($term, $unit) = CRM_Utils_System::explode('.', $relative, 2);
 -      $dateRange = CRM_Utils_Date::relativeToAbsolute($term, $unit);
 -      $from = substr($dateRange['from'], 0, 8);
 -      //Take only Date Part, Sometime Time part is also present in 'to'
 -      $to = substr($dateRange['to'], 0, 8);
 -    }
 -    $from = CRM_Utils_Date::processDate($from, $fromTime);
 -    $to = CRM_Utils_Date::processDate($to, $toTime);
 -    return array($from, $to);
 +    return CRM_Utils_Date::getFromTo($relative, $from, $to, $fromTime, $toTime);
    }
  
    /**
@@@ -2329,42 -2258,6 +2329,42 @@@ WHERE cg.extends IN ('" . implode("','"
      // process grand-total row
      $this->grandTotal($rows);
  
 +    // Find alter display functions.
 +    $firstRow = reset($rows);
 +    if ($firstRow) {
 +      $selectedFields = array_keys($firstRow);
 +      $alterFunctions = $alterMap = $alterSpecs = array();
 +      foreach ($this->_columns as $tableName => $table) {
 +        if (array_key_exists('metadata', $table)) {
 +          foreach ($table['metadata'] as $field => $specs) {
 +            if (in_array($tableName . '_' . $field, $selectedFields)) {
 +              if (array_key_exists('alter_display', $specs)) {
 +                $alterFunctions[$tableName . '_' . $field] = $specs['alter_display'];
 +                $alterMap[$tableName . '_' . $field] = $field;
 +                $alterSpecs[$tableName . '_' . $field] = NULL;
 +              }
 +              // Add any alters that can be intuited from the field specs.
 +              // So far only boolean but a lot more could be.
 +              if (empty($alterSpecs[$tableName . '_' . $field]) && isset($specs['type']) && $specs['type'] == CRM_Utils_Type::T_BOOLEAN) {
 +                $alterFunctions[$tableName . '_' . $field] = 'alterBoolean';
 +                $alterMap[$tableName . '_' . $field] = $field;
 +                $alterSpecs[$tableName . '_' . $field] = NULL;
 +              }
 +            }
 +          }
 +        }
 +      }
 +
 +      // Run the alter display functions
 +      foreach ($rows as $index => & $row) {
 +        foreach ($row as $selectedField => $value) {
 +          if (array_key_exists($selectedField, $alterFunctions)) {
 +            $rows[$index][$selectedField] = $this->{$alterFunctions[$selectedField]}($value, $row, $selectedField, $alterMap[$selectedField], $alterSpecs[$selectedField]);
 +          }
 +        }
 +      }
 +    }
 +
      // use this method for formatting rows for display purpose.
      $this->alterDisplay($rows);
      CRM_Utils_Hook::alterReportVar('rows', $rows, $this);
      $this->alterCustomDataDisplay($rows);
    }
  
 +  /**
 +   * @param $value
 +   * @param $row
 +   * @param $selectedfield
 +   * @param $criteriaFieldName
 +   *
 +   * @return array
 +   */
 +  protected function alterStateProvinceID($value, &$row, $selectedfield, $criteriaFieldName) {
 +    $url = CRM_Utils_System::url(CRM_Utils_System::currentPath(), "reset=1&force=1&{$criteriaFieldName}_op=in&{$criteriaFieldName}_value={$value}", $this->_absoluteUrl);
 +    $row[$selectedfield . '_link'] = $url;
 +    $row[$selectedfield . '_hover'] = ts("%1 for this state.", array(
 +      1 => $value,
 +    ));
 +
 +    $states = CRM_Core_PseudoConstant::stateProvince($value, FALSE);
 +    if (!is_array($states)) {
 +      return $states;
 +    }
 +  }
 +
 +  /**
 +   * @param $value
 +   * @param $row
 +   * @param $selectedField
 +   * @param $criteriaFieldName
 +   *
 +   * @return array
 +   */
 +  protected function alterCountryID($value, &$row, $selectedField, $criteriaFieldName) {
 +    $url = CRM_Utils_System::url(CRM_Utils_System::currentPath(), "reset=1&force=1&{$criteriaFieldName}_op=in&{$criteriaFieldName}_value={$value}", $this->_absoluteUrl);
 +    $row[$selectedField . '_link'] = $url;
 +    $row[$selectedField . '_hover'] = ts("%1 for this country.", array(
 +      1 => $value,
 +    ));
 +    $countries = CRM_Core_PseudoConstant::country($value, FALSE);
 +    if (!is_array($countries)) {
 +      return $countries;
 +    }
 +  }
 +
 +  /**
 +   * @param $value
 +   * @param $row
 +   * @param $selectedfield
 +   * @param $criteriaFieldName
 +   *
 +   * @return array
 +   */
 +  protected function alterCountyID($value, &$row, $selectedfield, $criteriaFieldName) {
 +    $url = CRM_Utils_System::url(CRM_Utils_System::currentPath(), "reset=1&force=1&{$criteriaFieldName}_op=in&{$criteriaFieldName}_value={$value}", $this->_absoluteUrl);
 +    $row[$selectedfield . '_link'] = $url;
 +    $row[$selectedfield . '_hover'] = ts("%1 for this county.", array(
 +      1 => $value,
 +    ));
 +    $counties = CRM_Core_PseudoConstant::county($value, FALSE);
 +    if (!is_array($counties)) {
 +      return $counties;
 +    }
 +  }
 +
 +  /**
 +   * @param $value
 +   * @param $row
 +   * @param $selectedfield
 +   * @param $criteriaFieldName
 +   *
 +   * @return mixed
 +   */
 +  protected function alterLocationTypeID($value, &$row, $selectedfield, $criteriaFieldName) {
 +    $values = $this->getLocationTypeOptions();
 +    return CRM_Utils_Array::value($value, $values);
 +  }
 +
 +  /**
 +   * @param $value
 +   * @param $row
 +   * @param $fieldname
 +   *
 +   * @return mixed
 +   */
 +  protected function alterContactID($value, &$row, $fieldname) {
 +    $nameField = substr($fieldname, 0, -2) . 'name';
 +    static $first = TRUE;
 +    static $viewContactList = FALSE;
 +    if ($first) {
 +      $viewContactList = CRM_Core_Permission::check('access CiviCRM');
 +      $first = FALSE;
 +    }
 +    if (!$viewContactList) {
 +      return $value;
 +    }
 +    if (array_key_exists($nameField, $row)) {
 +      $row[$nameField . '_link'] = CRM_Utils_System::url("civicrm/contact/view", 'reset=1&cid=' . $value, $this->_absoluteUrl);
 +    }
 +    else {
 +      $row[$fieldname . '_link'] = CRM_Utils_System::url("civicrm/contact/view", 'reset=1&cid=' . $value, $this->_absoluteUrl);
 +    }
 +    return $value;
 +  }
 +
 +  /**
 +   * @param $value
 +   *
 +   * @return mixed
 +   */
 +  protected function alterBoolean($value) {
 +    $options = array(0 => '', 1 => ts('Yes'));
 +    if (isset($options[$value])) {
 +      return $options[$value];
 +    }
 +    return $value;
 +  }
 +
    /**
     * Build chart.
     *
                $select = $this->addStatisticsToSelect($field, $tableName, $fieldName, $select);
              }
              else {
 -              $select = $this->addBasicFieldToSelect($tableName, $fieldName, $field, $select);
 +
 +              $selectClause = $this->getSelectClauseWithGroupConcatIfNotGroupedBy($tableName, $fieldName, $field);
 +              if ($selectClause) {
 +                $select[] = $selectClause;
 +              }
 +              else {
 +                $select = $this->addBasicFieldToSelect($tableName, $fieldName, $field, $select);
 +              }
              }
            }
          }
     * @return bool
     */
    public function selectClause(&$tableName, $tableKey, &$fieldName, &$field) {
 +    if (!empty($field['pseudofield'])) {
 +      $alias = "{$tableName}_{$fieldName}";
 +      $this->_columnHeaders["{$tableName}_{$fieldName}"]['title'] = CRM_Utils_Array::value('title', $field);
 +      $this->_columnHeaders["{$tableName}_{$fieldName}"]['type'] = CRM_Utils_Array::value('type', $field);
 +      $this->_columnHeaders["{$tableName}_{$fieldName}"]['dbAlias'] = CRM_Utils_Array::value('dbAlias', $field);
 +      $this->_selectAliases[] = $alias;
 +      return ' 1 as  ' . $alias;
 +    }
      return FALSE;
    }
  
      $this->groupBy();
      $this->orderBy();
  
 -    // order_by columns not selected for display need to be included in SELECT
 -    $unselectedSectionColumns = $this->unselectedSectionColumns();
 -    foreach ($unselectedSectionColumns as $alias => $section) {
 -      $this->_select .= ", {$section['dbAlias']} as {$alias}";
 +    foreach ($this->unselectedOrderByColumns() as $alias => $field) {
 +      $clause = $this->getSelectClauseWithGroupConcatIfNotGroupedBy($field['table_name'], $field['name'], $field);
 +      if (!$clause) {
 +        $clause = "{$field['dbAlias']} as {$alias}";
 +      }
 +      $this->_select .= ", $clause ";
      }
  
      if ($applyLimit && empty($this->_params['charts'])) {
  
          if (!empty($orderByField)) {
            $this->_orderByFields[$orderByField['tplField']] = $orderByField;
 -          $orderBys[] = "{$orderByField['dbAlias']} {$orderBy['order']}";
 +          if ($this->groupConcatTested) {
 +            $orderBys[$orderByField['tplField']] = "{$orderByField['tplField']} {$orderBy['order']}";
 +          }
 +          else {
 +            // Not sure when this is preferable to using tplField (which has
 +            // definitely been tested to work in cases then this does not.
 +            // in caution not switching unless report has been tested for
 +            // group concat functionality.
 +            $orderBys[$orderByField['tplField']] = "{$orderByField['dbAlias']} {$orderBy['order']}";
 +          }
  
            // Record any section headers for assignment to the template
            if (!empty($orderBy['section'])) {
      $this->assign('sections', $this->_sections);
    }
  
 +  /**
 +   * Determine unselected columns.
 +   *
 +   * @return array
 +   */
 +  public function unselectedOrderByColumns() {
 +    return array_diff_key($this->_orderByFields, $this->getSelectColumns());
 +  }
 +
    /**
     * Determine unselected columns.
     *
     * @param array $rows
     */
    public function buildRows($sql, &$rows) {
 +    if (!$this->optimisedForOnlyFullGroupBy) {
 +      CRM_Core_DAO::disableFullGroupByMode();
 +    }
      $dao = CRM_Core_DAO::executeQuery($sql);
 +    if (stristr($this->_select, 'SQL_CALC_FOUND_ROWS')) {
 +      $this->_rowsFound = CRM_Core_DAO::singleValueQuery('SELECT FOUND_ROWS()');
 +    }
 +    CRM_Core_DAO::reenableFullGroupByMode();
      if (!is_array($rows)) {
        $rows = array();
      }
          WHERE smartgroup_contact.group_id IN ({$smartGroups}) ";
      }
  
 -    $this->groupTempTable = 'civicrm_report_temp_group_' . date('Ymd_') . uniqid();
 +    $this->groupTempTable = CRM_Utils_SQL_TempTable::build()->setCategory('rptgrp')->setId(date('Ymd_') . uniqid())->getName();
      $this->executeReportQuery("
        CREATE TEMPORARY TABLE $this->groupTempTable $this->_databaseAttributes
        $query
@@@ -4259,11 -3996,10 +4259,11 @@@ LEFT JOIN civicrm_contact {$field['alia
          }
          if (array_key_exists('filters', $table)) {
            foreach ($table['filters'] as $filterName => $filter) {
 -            if (!empty($this->_params["{$filterName}_value"]) ||
 -              CRM_Utils_Array::value("{$filterName}_op", $this->_params) ==
 -              'nll' ||
 -              CRM_Utils_Array::value("{$filterName}_op", $this->_params) ==
 +            if (!empty($this->_params["{$filterName}_value"])
 +              || !empty($this->_params["{$filterName}_relative"])
 +              || CRM_Utils_Array::value("{$filterName}_op", $this->_params) ==
 +              'nll'
 +              || CRM_Utils_Array::value("{$filterName}_op", $this->_params) ==
                'nnll'
              ) {
                $this->_selectedTables[] = $tableName;
  
    /**
     * Do AlterDisplay processing on Address Fields.
+    *  If there are multiple address field values then
+    *  on basis of provided separator the code values are translated into respective labels
     *
     * @param array $row
     * @param array $rows
     * @param int $rowNum
     * @param string $baseUrl
     * @param string $linkText
+    * @param string $separator
     *
     * @return bool
     */
-   public function alterDisplayAddressFields(&$row, &$rows, &$rowNum, $baseUrl, $linkText) {
+   public function alterDisplayAddressFields(&$row, &$rows, &$rowNum, $baseUrl, $linkText, $separator = ',') {
      $criteriaQueryParams = CRM_Report_Utils_Report::getPreviewCriteriaQueryParams($this->_defaults, $this->_params);
      $entryFound = FALSE;
-     // handle country
-     if (array_key_exists('civicrm_address_country_id', $row)) {
-       if ($value = $row['civicrm_address_country_id']) {
-         $rows[$rowNum]['civicrm_address_country_id'] = CRM_Core_PseudoConstant::country($value, FALSE);
-         if ($baseUrl) {
-           $url = CRM_Report_Utils_Report::getNextUrl($baseUrl,
-             "reset=1&force=1&{$criteriaQueryParams}&" .
-             "country_id_op=in&country_id_value={$value}",
-             $this->_absoluteUrl, $this->_id
-           );
-           $rows[$rowNum]['civicrm_address_country_id_link'] = $url;
-           $rows[$rowNum]['civicrm_address_country_id_hover'] = ts("%1 for this country.",
-             array(1 => $linkText)
-           );
-         }
-       }
-       $entryFound = TRUE;
-     }
-     if (array_key_exists('civicrm_address_county_id', $row)) {
-       if ($value = $row['civicrm_address_county_id']) {
-         $rows[$rowNum]['civicrm_address_county_id'] = CRM_Core_PseudoConstant::county($value, FALSE);
-         if ($baseUrl) {
-           $url = CRM_Report_Utils_Report::getNextUrl($baseUrl,
-             "reset=1&force=1&{$criteriaQueryParams}&" .
-             "county_id_op=in&county_id_value={$value}",
-             $this->_absoluteUrl, $this->_id
-           );
-           $rows[$rowNum]['civicrm_address_county_id_link'] = $url;
-           $rows[$rowNum]['civicrm_address_county_id_hover'] = ts("%1 for this county.",
-             array(1 => $linkText)
-           );
-         }
-       }
-       $entryFound = TRUE;
-     }
-     // handle state province
-     if (array_key_exists('civicrm_address_state_province_id', $row)) {
-       if ($value = $row['civicrm_address_state_province_id']) {
-         $rows[$rowNum]['civicrm_address_state_province_id'] = CRM_Core_PseudoConstant::stateProvince($value, FALSE);
-         if ($baseUrl) {
-           $url = CRM_Report_Utils_Report::getNextUrl($baseUrl,
-             "reset=1&force=1&{$criteriaQueryParams}&state_province_id_op=in&state_province_id_value={$value}",
-             $this->_absoluteUrl, $this->_id
-           );
-           $rows[$rowNum]['civicrm_address_state_province_id_link'] = $url;
-           $rows[$rowNum]['civicrm_address_state_province_id_hover'] = ts("%1 for this state.",
-             array(1 => $linkText)
-           );
+     $columnMap = array(
+       'civicrm_address_country_id' => 'country',
+       'civicrm_address_county_id' => 'county',
+       'civicrm_address_state_province_id' => 'stateProvince',
+     );
+     foreach ($columnMap as $fieldName => $fnName) {
+       if (array_key_exists($fieldName, $row)) {
+         if ($values = $row[$fieldName]) {
+           $values = (array) explode($separator, $values);
+           $rows[$rowNum][$fieldName] = [];
+           $addressField = $fnName == 'stateProvince' ? 'state' : $fnName;
+           foreach ($values as $value) {
+             $rows[$rowNum][$fieldName][] = CRM_Core_PseudoConstant::$fnName($value);
+           }
+           $rows[$rowNum][$fieldName] = implode($separator, $rows[$rowNum][$fieldName]);
+           if ($baseUrl) {
+             $url = CRM_Report_Utils_Report::getNextUrl($baseUrl,
+               sprintf("reset=1&force=1&%s&%s_op=in&%s_value=%s",
+                 $criteriaQueryParams,
+                 str_replace('civicrm_address_', '', $fieldName),
+                 str_replace('civicrm_address_', '', $fieldName),
+                 implode(',', $values)
+               ), $this->_absoluteUrl, $this->_id
+             );
+             $rows[$rowNum]["{$fieldName}_link"] = $url;
+             $rows[$rowNum]["{$fieldName}_hover"] = ts("%1 for this %2.", array(1 => $linkText, 2 => $addressField));
+           }
+           $entryFound = TRUE;
          }
        }
-       $entryFound = TRUE;
      }
  
      return $entryFound;
  
    /**
     * Add Address into From Table if required.
 +   *
 +   * @deprecated use joinAddressFromContact
 +   * (left here in case extensions use it).
     */
    public function addAddressFromClause() {
 +    CRM_Core_Error::deprecatedFunctionWarning('CRM_Report_Form::joinAddressFromContact');
      // include address field if address column is to be included
      if ((isset($this->_addressField) &&
          $this->_addressField
  
    /**
     * Add Phone into From Table if required.
 +   *
 +   * @deprecated use joinPhoneFromContact
 +   *  (left here in case extensions use it).
     */
    public function addPhoneFromClause() {
 +    CRM_Core_Error::deprecatedFunctionWarning('CRM_Report_Form::joinPhoneFromContact');
      // include address field if address column is to be included
      if ($this->isTableSelected('civicrm_phone')) {
        $this->_from .= "
      }
    }
  
 +  /**
 +   * Add Address into From Table if required.
 +   *
 +   * Prefix will be added to both tables as
 +   * it is assumed you are using it to get address of a secondary contact.
 +   *
 +   * @param string $prefix
 +   * @param array $extra Additional options.
 +   *      Not currently used in core but may be used in override extensions.
 +   */
 +  protected function joinAddressFromContact($prefix = '', $extra = array()) {
 +    $addressTables = ['civicrm_address', 'civicrm_country', 'civicrm_worldregion', 'civicrm_state_province'];
 +    $isJoinRequired = $this->_addressField;
 +    foreach ($addressTables as $addressTable) {
 +      if ($this->isTableSelected($prefix . $addressTable)) {
 +        $isJoinRequired = TRUE;
 +      }
 +    }
 +    if ($isJoinRequired) {
 +      $this->_from .= "
 +                 LEFT JOIN civicrm_address {$this->_aliases[$prefix . 'civicrm_address']}
 +                           ON ({$this->_aliases[$prefix . 'civicrm_contact']}.id =
 +                               {$this->_aliases[$prefix . 'civicrm_address']}.contact_id) AND
 +                               {$this->_aliases[$prefix . 'civicrm_address']}.is_primary = 1\n";
 +    }
 +  }
 +
 +  /**
 +   * Add Country into From Table if required.
 +   *
 +   * Prefix will be added to both tables as
 +   * it is assumed you are using it to get address of a secondary contact.
 +   *
 +   * @param string $prefix
 +   * @param array $extra Additional options.
 +   *      Not currently used in core but may be used in override extensions.
 +   */
 +  protected function joinCountryFromAddress($prefix = '', $extra = array()) {
 +    // include country field if country column is to be included
 +    if ($this->isTableSelected($prefix . 'civicrm_country') || $this->isTableSelected($prefix . 'civicrm_worldregion')) {
 +      if (empty($this->_aliases[$prefix . 'civicrm_country'])) {
 +        $this->_aliases[$prefix . 'civicrm_country'] = $prefix . '_report_country';
 +      }
 +      $this->_from .= "
 +            LEFT JOIN civicrm_country {$this->_aliases[$prefix . 'civicrm_country']}
 +                   ON {$this->_aliases[$prefix . 'civicrm_address']}.country_id = {$this->_aliases[$prefix . 'civicrm_country']}.id AND
 +                      {$this->_aliases[$prefix . 'civicrm_address']}.is_primary = 1 ";
 +    }
 +  }
 +
 +  /**
 +   * Add Phone into From Table if required.
 +   *
 +   * Prefix will be added to both tables as
 +   * it is assumed you are using it to get address of a secondary contact.
 +   *
 +   * @param string $prefix
 +   * @param array $extra Additional options.
 +   *      Not currently used in core but may be used in override extensions.
 +   */
 +  protected function joinPhoneFromContact($prefix = '', $extra = array()) {
 +    // include phone field if phone column is to be included
 +    if ($this->isTableSelected($prefix . 'civicrm_phone')) {
 +      $this->_from .= "
 +      LEFT JOIN civicrm_phone {$this->_aliases[$prefix . 'civicrm_phone']}
 +             ON {$this->_aliases[$prefix . 'civicrm_contact']}.id = {$this->_aliases[$prefix . 'civicrm_phone']}.contact_id AND
 +                {$this->_aliases[$prefix . 'civicrm_phone']}.is_primary = 1\n";
 +    }
 +  }
 +
 +  /**
 +   * Add Email into From Table if required.
 +   *
 +   * Prefix will be added to both tables as
 +   * it is assumed you are using it to get address of a secondary contact.
 +   *
 +   * @param string $prefix
 +   * @param array $extra Additional options.
 +   *      Not currently used in core but may be used in override extensions.
 +   */
 +  protected function joinEmailFromContact($prefix = '', $extra = array()) {
 +    // include email field if email column is to be included
 +    if ($this->isTableSelected($prefix . 'civicrm_email')) {
 +      $this->_from .= "
 +            LEFT JOIN  civicrm_email {$this->_aliases[$prefix . 'civicrm_email']}
 +                   ON ({$this->_aliases[$prefix . 'civicrm_contact']}.id = {$this->_aliases[$prefix . 'civicrm_email']}.contact_id AND
 +                       {$this->_aliases[$prefix . 'civicrm_email']}.is_primary = 1) ";
 +    }
 +  }
 +
    /**
     * Add Financial Transaction into From Table if required.
     */
      return $fields;
    }
  
 -  /**
 -   * Get address columns to add to array.
 -   *
 -   * @param array $options
 -   *   - prefix Prefix to add to table (in case of more than one instance of the table)
 -   *   - prefix_label Label to give columns from this address table instance
 -   *
 -   * @return array
 -   *   address columns definition
 -   */
 -  public function getAddressColumns($options = array()) {
 -    $options += array(
 -      'prefix' => '',
 -      'prefix_label' => '',
 -      'group_by' => TRUE,
 -      'order_by' => TRUE,
 -      'filters' => TRUE,
 -      'defaults' => array(),
 -    );
 -    return $this->addAddressFields(
 -      $options['group_by'],
 -      $options['order_by'],
 -      $options['filters'],
 -      $options['defaults']
 -    );
 -  }
 -
    /**
     * Get a standard set of contact fields.
     *
          'is_order_bys' => TRUE,
          'is_group_bys' => TRUE,
          'is_fields' => TRUE,
 +        'is_filters' => TRUE,
        ),
        $options['prefix'] . 'external_identifier' => array(
          'name' => 'external_identifier',
          'title' => $options['prefix_label'] . ts('Nick Name'),
          'is_fields' => TRUE,
        ),
 +      $options['prefix'] . 'prefix_id' => array(
 +        'name' => 'prefix_id',
 +        'title' => $options['prefix_label'] . ts('Prefix'),
 +        'options' => CRM_Contact_BAO_Contact::buildOptions('prefix_id'),
 +        'operatorType' => CRM_Report_Form::OP_MULTISELECT,
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +      ),
 +      $options['prefix'] . 'suffix_id' => array(
 +        'name' => 'suffix_id',
 +        'title' => $options['prefix_label'] . ts('Suffix'),
 +        'options' => CRM_Contact_BAO_Contact::buildOptions('suffix_id'),
 +        'operatorType' => CRM_Report_Form::OP_MULTISELECT,
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +      ),
        $options['prefix'] . 'gender_id' => array(
          'name' => 'gender_id',
          'title' => $options['prefix_label'] . ts('Gender'),
      return $this->buildColumns($spec, $options['prefix'] . 'civicrm_contact', 'CRM_Contact_DAO_Contact', $tableAlias, $this->getDefaultsFromOptions($options), $options);
    }
  
 +  /**
 +   * Get address columns to add to array.
 +   *
 +   * @param array $options
 +   *  - prefix Prefix to add to table (in case of more than one instance of the table)
 +   *  - prefix_label Label to give columns from this address table instance
 +   *  - group_bys enable these fields for group by - default false
 +   *  - order_bys enable these fields for order by
 +   *  - filters enable these fields for filtering
 +   *
 +   * @return array address columns definition
 +   */
 +  protected function getAddressColumns($options = array()) {
 +    $defaultOptions = array(
 +      'prefix' => '',
 +      'prefix_label' => '',
 +      'fields' => TRUE,
 +      'group_bys' => FALSE,
 +      'order_bys' => TRUE,
 +      'filters' => TRUE,
 +      'join_filters' => FALSE,
 +      'fields_defaults' => array(),
 +      'filters_defaults' => array(),
 +      'group_bys_defaults' => array(),
 +      'order_bys_defaults' => array(),
 +    );
 +
 +    $options = array_merge($defaultOptions, $options);
 +    $defaults = $this->getDefaultsFromOptions($options);
 +    $tableAlias = $options['prefix'] . 'address';
 +
 +    $spec = array(
 +      $options['prefix'] . 'name' => array(
 +        'title' => ts($options['prefix_label'] . 'Address Name'),
 +        'name' => 'name',
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'street_number' => array(
 +        'name' => 'street_number',
 +        'title' => ts($options['prefix_label'] . 'Street Number'),
 +        'type' => 1,
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'odd_street_number' => array(
 +        'title' => ts('Odd / Even Street Number'),
 +        'name' => 'odd_street_number',
 +        'type' => CRM_Utils_Type::T_INT,
 +        'no_display' => TRUE,
 +        'required' => TRUE,
 +        'dbAlias' => '(address_civireport.street_number % 2)',
 +        'is_fields' => TRUE,
 +        'is_order_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'street_name' => array(
 +        'name' => 'street_name',
 +        'title' => ts($options['prefix_label'] . 'Street Name'),
 +        'type' => 1,
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'operator' => 'like',
 +        'is_order_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'street_address' => array(
 +        'title' => ts($options['prefix_label'] . 'Street Address'),
 +        'name' => 'street_address',
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'supplemental_address_1' => array(
 +        'title' => ts($options['prefix_label'] . 'Supplementary Address Field 1'),
 +        'name' => 'supplemental_address_1',
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'supplemental_address_2' => array(
 +        'title' => ts($options['prefix_label'] . 'Supplementary Address Field 2'),
 +        'name' => 'supplemental_address_2',
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'supplemental_address_3' => array(
 +        'title' => ts($options['prefix_label'] . 'Supplementary Address Field 3'),
 +        'name' => 'supplemental_address_3',
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'street_number' => array(
 +        'name' => 'street_number',
 +        'title' => ts($options['prefix_label'] . 'Street Number'),
 +        'type' => 1,
 +        'is_order_bys' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'street_name' => array(
 +        'name' => 'street_name',
 +        'title' => ts($options['prefix_label'] . 'Street Name'),
 +        'type' => 1,
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'street_unit' => array(
 +        'name' => 'street_unit',
 +        'title' => ts($options['prefix_label'] . 'Street Unit'),
 +        'type' => 1,
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'city' => array(
 +        'title' => ts($options['prefix_label'] . 'City'),
 +        'name' => 'city',
 +        'operator' => 'like',
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +        'is_order_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'postal_code' => array(
 +        'title' => ts($options['prefix_label'] . 'Postal Code'),
 +        'name' => 'postal_code',
 +        'type' => 1,
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +        'is_order_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'postal_code_suffix' => array(
 +        'title' => ts($options['prefix_label'] . 'Postal Code Suffix'),
 +        'name' => 'postal_code',
 +        'type' => 1,
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +        'is_order_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'county_id' => array(
 +        'title' => ts($options['prefix_label'] . 'County'),
 +        'alter_display' => 'alterCountyID',
 +        'name' => 'county_id',
 +        'type' => CRM_Utils_Type::T_INT,
 +        'operatorType' => CRM_Report_Form::OP_MULTISELECT,
 +        'options' => CRM_Core_PseudoConstant::county(),
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'state_province_id' => array(
 +        'title' => ts($options['prefix_label'] . 'State/Province'),
 +        'alter_display' => 'alterStateProvinceID',
 +        'name' => 'state_province_id',
 +        'type' => CRM_Utils_Type::T_INT,
 +        'operatorType' => CRM_Report_Form::OP_MULTISELECT,
 +        'options' => CRM_Core_PseudoConstant::stateProvince(),
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +      ),
 +      $options['prefix'] . 'country_id' => array(
 +        'title' => ts($options['prefix_label'] . 'Country'),
 +        'alter_display' => 'alterCountryID',
 +        'name' => 'country_id',
 +        'is_fields' => TRUE,
 +        'is_filters' => TRUE,
 +        'is_group_bys' => TRUE,
 +        'type' => CRM_Utils_Type::T_INT,
 +        'operatorType' => CRM_Report_Form::OP_MULTISELECT,
 +        'options' => CRM_Core_PseudoConstant::country(),
 +      ),
 +      $options['prefix'] . 'location_type_id' => array(
 +        'name' => 'is_primary',
 +        'title' => ts($options['prefix_label'] . 'Location Type'),
 +        'type' => CRM_Utils_Type::T_INT,
 +        'is_fields' => TRUE,
 +        'alter_display' => 'alterLocationTypeID',
 +      ),
 +      $options['prefix'] . 'id' => array(
 +        'title' => ts($options['prefix_label'] . 'ID'),
 +        'name' => 'id',
 +        'is_fields' => TRUE,
 +      ),
 +      $options['prefix'] . 'is_primary' => array(
 +        'name' => 'is_primary',
 +        'title' => ts($options['prefix_label'] . 'Primary Address?'),
 +        'type' => CRM_Utils_Type::T_BOOLEAN,
 +        'is_fields' => TRUE,
 +      ),
 +    );
 +    return $this->buildColumns($spec, $options['prefix'] . 'civicrm_address', 'CRM_Core_DAO_Address', $tableAlias, $defaults, $options);
 +  }
 +
    /**
     * Build the columns.
     *
     */
    protected function storeGroupByArray() {
  
 -    if (CRM_Utils_Array::value('group_bys', $this->_params) &&
 -      is_array($this->_params['group_bys']) &&
 -      !empty($this->_params['group_bys'])
 -    ) {
 -      foreach ($this->_columns as $tableName => $table) {
 -        $table = $this->_columns[$tableName];
 -        if (array_key_exists('group_bys', $table)) {
 -          foreach ($table['group_bys'] as $fieldName => $fieldData) {
 -            $field = $this->_columns[$tableName]['metadata'][$fieldName];
 -            if (!empty($this->_params['group_bys'][$fieldName])) {
 -              if (!empty($field['chart'])) {
 -                $this->assign('chartSupported', TRUE);
 -              }
 +    if (!CRM_Utils_Array::value('group_bys', $this->_params)
 +      || !is_array($this->_params['group_bys'])) {
 +      $this->_params['group_bys'] = [];
 +    }
  
 -              if (!empty($table['group_bys'][$fieldName]['frequency']) &&
 -                !empty($this->_params['group_bys_freq'][$fieldName])
 -              ) {
 +    foreach ($this->_columns as $tableName => $table) {
 +      $table = $this->_columns[$tableName];
 +      if (array_key_exists('group_bys', $table)) {
 +        foreach ($table['group_bys'] as $fieldName => $fieldData) {
 +          $field = $this->_columns[$tableName]['metadata'][$fieldName];
 +          if (!empty($this->_params['group_bys'][$fieldName]) || !empty($fieldData['required'])) {
 +            if (!empty($field['chart'])) {
 +              $this->assign('chartSupported', TRUE);
 +            }
  
 -                switch ($this->_params['group_bys_freq'][$fieldName]) {
 -                  case 'FISCALYEAR':
 -                    $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = self::fiscalYearOffset($field['dbAlias']);
 +            if (!empty($table['group_bys'][$fieldName]['frequency']) &&
 +              !empty($this->_params['group_bys_freq'][$fieldName])
 +            ) {
  
 -                  case 'YEAR':
 -                    $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = " {$this->_params['group_bys_freq'][$fieldName]}({$field['dbAlias']})";
 +              switch ($this->_params['group_bys_freq'][$fieldName]) {
 +                case 'FISCALYEAR':
 +                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = self::fiscalYearOffset($field['dbAlias']);
  
 -                  default:
 -                    $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "EXTRACT(YEAR_{$this->_params['group_bys_freq'][$fieldName]} FROM {$field['dbAlias']})";
 +                case 'YEAR':
 +                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = " {$this->_params['group_bys_freq'][$fieldName]}({$field['dbAlias']})";
 +
 +                default:
 +                  $this->_groupByArray[$tableName . '_' . $fieldName . '_start'] = "EXTRACT(YEAR_{$this->_params['group_bys_freq'][$fieldName]} FROM {$field['dbAlias']})";
  
 -                }
                }
 -              else {
 -                if (!in_array($field['dbAlias'], $this->_groupByArray)) {
 -                  $this->_groupByArray[$tableName . '_' . $fieldName] = $field['dbAlias'];
 -                }
 +            }
 +            else {
 +              if (!in_array($field['dbAlias'], $this->_groupByArray)) {
 +                $this->_groupByArray[$tableName . '_' . $fieldName] = $field['dbAlias'];
                }
              }
            }
 -
          }
 +
        }
      }
    }
      return $defaults;
    }
  
 +  /**
 +   * Get the select clause for a field, wrapping in GROUP_CONCAT if appropriate.
 +   *
 +   * Full group by mode dictates that a field must either be in the group by function or
 +   * wrapped in a aggregate function. Here we wrap the field in GROUP_CONCAT if it is not in the
 +   * group concat.
 +   *
 +   * @param string $tableName
 +   * @param string $fieldName
 +   * @param string $field
 +   * @return string
 +   */
 +  protected function getSelectClauseWithGroupConcatIfNotGroupedBy($tableName, &$fieldName, &$field) {
 +    if ($this->groupConcatTested && (!empty($this->_groupByArray) || $this->isForceGroupBy)) {
 +      if ((empty($field['statistics']) || in_array('GROUP_CONCAT', $field['statistics']))) {
 +        $label = CRM_Utils_Array::value('title', $field);
 +        $alias = "{$tableName}_{$fieldName}";
 +        $this->_columnHeaders["{$tableName}_{$fieldName}"]['title'] = $label;
 +        $this->_selectAliases[] = $alias;
 +        if (empty($this->_groupByArray[$tableName . '_' . $fieldName])) {
 +          return "GROUP_CONCAT(DISTINCT {$field['dbAlias']}) as $alias";
 +        }
 +        return "({$field['dbAlias']}) as $alias";
 +      }
 +    }
 +  }
 +
  }
index 1e7be33769bed2417af87c0c01430f3713c07601,fef30e881107809757b18c9d7e2b22ab688ae80a..60f18d2ad2d87e68cc78ba1314b430403b8f4701
@@@ -1,9 -1,9 +1,9 @@@
  <?php
  /*
   +--------------------------------------------------------------------+
 - | CiviCRM version 4.7                                                |
 + | CiviCRM version 5                                                  |
   +--------------------------------------------------------------------+
 - | Copyright CiviCRM LLC (c) 2004-2017                                |
 + | Copyright CiviCRM LLC (c) 2004-2018                                |
   +--------------------------------------------------------------------+
   | This file is a part of CiviCRM.                                    |
   |                                                                    |
@@@ -28,7 -28,7 +28,7 @@@
  /**
   *
   * @package CRM
 - * @copyright CiviCRM LLC (c) 2004-2017
 + * @copyright CiviCRM LLC (c) 2004-2018
   */
  class CRM_Report_Form_Activity extends CRM_Report_Form {
    protected $_selectAliasesTotal = array();
@@@ -91,9 -91,6 +91,9 @@@
      $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',
    /**
     * 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 = NULL) {
 +  public function select($recordType = 'target') {
      if (!array_key_exists("contact_{$recordType}", $this->_params['fields']) &&
        $recordType != 'final'
      ) {
  
      $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.') ||
        }
      }
      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.') ||
            strstr($clause, 'civicrm_email_target.') ||
            strstr($clause, 'civicrm_email_source.') ||
            strstr($clause, 'civicrm_phone_target.') ||
-           strstr($clause, 'civicrm_phone_source.')
+           strstr($clause, 'civicrm_phone_source.') ||
+           strstr($clause, 'civicrm_address_')
          ) {
            $removeKeys[] = $key;
            unset($this->_selectClauses[$key]);
        }
      }
      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.') ||
            strstr($clause, 'civicrm_email_target.') ||
            strstr($clause, 'civicrm_email_assignee.') ||
            strstr($clause, 'civicrm_phone_target.') ||
-           strstr($clause, 'civicrm_phone_assignee.')
+           strstr($clause, 'civicrm_phone_assignee.') ||
+           strstr($clause, 'civicrm_address_')
          ) {
            $removeKeys[] = $key;
            unset($this->_selectClauses[$key]);
      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') ||
            strstr($clause, 'civicrm_email_contact_source_email') ||
            strstr($clause, 'civicrm_email_contact_assignee_email') ||
            strstr($clause, 'civicrm_email_contact_target_email') ||
-           strstr($clause, 'civicrm_phone_contact_target_phone')
+           strstr($clause, 'civicrm_phone_contact_target_phone') ||
+           strstr($clause, 'civicrm_address_')
          ) {
-           $this->_selectClauses[$key] = "GROUP_CONCAT($clause SEPARATOR ';') as $clause";
+           $this->_selectClauses[$key] = "GROUP_CONCAT(DISTINCT $clause SEPARATOR ';') as $clause";
          }
        }
      }
          unset($this->_selectAliases[$key]);
        }
  
-       if ($recordType != 'final') {
+       if ($recordType == 'target') {
          foreach ($this->_columns['civicrm_address']['order_bys'] as $fieldName => $field) {
            $orderByFld = $this->_columns['civicrm_address']['order_bys'][$fieldName];
            $fldInfo = $this->_columns['civicrm_address']['fields'][$fieldName];
  
    /**
     * Build from clause.
 -   *
 -   * @param string $recordType
 +   * @todo remove this function & declare the 3 contact tables separately
     */
 -  public function from($recordType) {
 +  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->addAddressFromClause();
 +    $this->joinAddressFromContact();
    }
  
    /**
     * 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) {
      $new_having = ' addtogroup_contact_id';
      $having = str_ireplace(' civicrm_contact_contact_target_id', $new_having, $this->_having);
      $query = "$select
 -FROM civireport_activity_temp_target tar
 +FROM {$this->temporaryTables['activity_temp_table']} tar
  GROUP BY civicrm_activity_id $having {$this->_orderBy}";
      $select = 'AS addtogroup_contact_id';
      $query = str_ireplace('AS civicrm_contact_contact_target_id', $select, $query);
        }
      }
  
 +    // @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);
 -    $tempQuery = "CREATE TEMPORARY TABLE civireport_activity_temp_target {$this->_databaseAttributes} AS
 -{$this->_select} {$this->_from} {$this->_where} ";
 -    $this->executeReportQuery($tempQuery);
 +    $tempTableName = $this->createTemporaryTable('activity_temp_table', "{$this->_select} {$this->_from} {$this->_where}");
  
      // 2. add new columns to hold assignee and source results
      // fixme: add when required
      $tempQuery = "
 -  ALTER TABLE  civireport_activity_temp_target
 +  ALTER TABLE  $tempTableName
    MODIFY COLUMN civicrm_contact_contact_target_id VARCHAR(128),
    ADD COLUMN civicrm_contact_contact_assignee VARCHAR(128),
    ADD COLUMN civicrm_contact_contact_source VARCHAR(128),
      // 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);
 -    $tempQuery = "INSERT INTO civireport_activity_temp_target ({$insertCols})
 +    $tempQuery = "INSERT INTO $tempTableName ({$insertCols})
  {$this->_select}
  {$this->_from} {$this->_where}";
      $this->executeReportQuery($tempQuery);
      // 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);
 -    $tempQuery = "INSERT INTO civireport_activity_temp_target ({$insertCols})
 +    $tempQuery = "INSERT INTO $tempTableName ({$insertCols})
  {$this->_select}
  {$this->_from} {$this->_where}";
      $this->executeReportQuery($tempQuery);
      $this->orderBy();
      foreach ($this->_sections as $alias => $section) {
        if (!empty($section) && $section['name'] == 'activity_date_time') {
 -        $this->alterSectionHeaderForDateTime('civireport_activity_temp_target', $section['tplField']);
 +        $this->alterSectionHeaderForDateTime($tempTableName, $section['tplField']);
        }
      }
  
      }
  
      $sql = "{$this->_select}
 -      FROM civireport_activity_temp_target tar
 +      FROM $tempTableName tar
        INNER JOIN civicrm_activity {$this->_aliases['civicrm_activity']} ON {$this->_aliases['civicrm_activity']}.id = tar.civicrm_activity_id
        INNER JOIN civicrm_activity_contact {$this->_aliases['civicrm_activity_contact']} ON {$this->_aliases['civicrm_activity_contact']}.activity_id = {$this->_aliases['civicrm_activity']}.id
        AND {$this->_aliases['civicrm_activity_contact']}.record_type_id = {$sourceID}
      $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')) {
          }
        }
  
-       $entryFound = $this->alterDisplayAddressFields($row, $rows, $rowNum, 'activity', 'List all activities for this ') ? TRUE : $entryFound;
+       $entryFound = $this->alterDisplayAddressFields($row, $rows, $rowNum, 'activity', 'List all activities for this', ';') ? TRUE : $entryFound;
  
        if (!$entryFound) {
          break;
        $this->_select = CRM_Contact_BAO_Query::appendAnyValueToSelect($ifnulls, $sectionAliases);
  
        $query = $this->_select .
 -        ", count(DISTINCT civicrm_activity_id) as ct from civireport_activity_temp_target group by " .
 +        ", count(DISTINCT civicrm_activity_id) as ct from {$this->temporaryTables['activity_temp_table']} group by " .
          implode(", ", $sectionAliases);
  
        // initialize array of total counts
      }
    }
  
 +  /**
 +   * @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();
 +  }
 +
  }
index 54dbc999b656accf8a0c52e8316e7c77be2d9ae1,40701db881fc850298c93a6ab836ea486b8a31b6..d33751958b10230110f8222431f8b5582cce4796
@@@ -1,9 -1,9 +1,9 @@@
  <?php
  /*
   +--------------------------------------------------------------------+
 - | CiviCRM version 4.7                                                |
 + | CiviCRM version 5                                                  |
   +--------------------------------------------------------------------+
 - | Copyright CiviCRM LLC (c) 2004-2017                                |
 + | Copyright CiviCRM LLC (c) 2004-2018                                |
   +--------------------------------------------------------------------+
   | This file is a part of CiviCRM.                                    |
   |                                                                    |
@@@ -49,6 -49,7 +49,7 @@@ class CRM_Report_Form_ActivityTest exte
      CRM_Core_DAO::executeQuery('DROP TEMPORARY TABLE IF EXISTS civireport_contribution_detail_temp1');
      CRM_Core_DAO::executeQuery('DROP TEMPORARY TABLE IF EXISTS civireport_contribution_detail_temp2');
      CRM_Core_DAO::executeQuery('DROP TEMPORARY TABLE IF EXISTS civireport_contribution_detail_temp3');
+     CRM_Core_DAO::executeQuery('DROP TEMPORARY TABLE IF EXISTS civireport_activity_temp_target');
    }
  
    /**
      $this->assertTrue(TRUE, "Testo");
    }
  
+   /**
+    * Ensure that activity detail report only shows addres fields of target contact
+    */
+   public function testTargetAddressFields() {
+     $countryNames = array_flip(CRM_Core_PseudoConstant::country());
+     // Create contact 1 and 2 with address fields, later considered as target contacts for activity
+     $contactID1 = $this->individualCreate(array(
+       'api.Address.create' => array(
+         'contact_id' => '$value.id',
+         'location_type_id' => 'Home',
+         'city' => 'ABC',
+         'country_id' => $countryNames['India'],
+       ),
+     ));
+     $contactID2 = $this->individualCreate(array(
+       'api.Address.create' => array(
+         'contact_id' => '$value.id',
+         'location_type_id' => 'Home',
+         'city' => 'DEF',
+         'country_id' => $countryNames['United States'],
+       ),
+     ));
+     // Create Contact 3 later considered as assignee contact of activity
+     $contactID3 = $this->individualCreate(array(
+       'api.Address.create' => array(
+         'contact_id' => '$value.id',
+         'location_type_id' => 'Home',
+         'city' => 'GHI',
+         'country_id' => $countryNames['China'],
+       ),
+     ));
+     // create dummy activity type
+     $activityTypeID = CRM_Utils_Array::value('id', $this->callAPISuccess('option_value', 'create', array(
+       'option_group_id' => 'activity_type',
+       'name' => 'Test activity type',
+       'label' => 'Test activity type',
+     )));
+     // create activity
+     $result = $this->callAPISuccess('activity', 'create', array(
+       'subject' => 'Make-it-Happen Meeting',
+       'activity_date_time' => date('Ymd'),
+       'duration' => 120,
+       'location' => 'Pennsylvania',
+       'details' => 'a test activity',
+       'status_id' => 1,
+       'activity_type_id' => 'Test activity type',
+       'source_contact_id' => $this->individualCreate(),
+       'target_contact_id' => array($contactID1, $contactID2),
+       'assignee_contact_id' => $contactID3,
+     ));
+     // display city and country field so that we can check its value
+     $input = array(
+       'fields' => array(
+         'city',
+         'country_id',
+       ),
+       'order_bys' => array(
+         'city' => array(),
+         'country_id' => array('default' => TRUE),
+       ),
+     );
+     // generate result
+     $obj = $this->getReportObject('CRM_Report_Form_Activity', $input);
+     $rows = $obj->getResultSet();
+     // ensure that only 1 activity is created
+     $this->assertEquals(1, count($rows));
+     // ensure that country values of respective target contacts are only shown
+     $this->assertEquals('India;United States', $rows[0]['civicrm_address_country_id']);
+     // ensure that city values of respective target contacts are only shown
+     $this->assertEquals('ABC;DEF', $rows[0]['civicrm_address_city']);
+   }
  }