Merge pull request #7162 from piwik/6809

Some performance improvements for the all websites dashboard

4 files changed, 106 insertions, 56 deletions

diff --git a/plugins/API/ProcessedReport.php b/plugins/API/ProcessedReport.php
index bd1a885f4d..7c4bd46c9e 100644
--- a/plugins/API/ProcessedReport.php
+++ b/plugins/API/ProcessedReport.php
@@ -674,8 +674,9 @@ class ProcessedReport
                     // if we handle MultiSites.getAll we do not always have the same idSite but different ones for
                     // each site, see https://github.com/piwik/piwik/issues/5006
                     $idSiteForRow = $idSite;
-                    if ($row->getMetadata('idsite') && is_numeric($row->getMetadata('idsite'))) {
-                        $idSiteForRow = (int) $row->getMetadata('idsite');
+                    $idSiteMetadata = $row->getMetadata('idsite');
+                    if ($idSiteMetadata && is_numeric($idSiteMetadata)) {
+                        $idSiteForRow = (int) $idSiteMetadata;
                     }
 
                     $prettyValue = self::getPrettyValue($formatter, $idSiteForRow, $columnName, $columnValue, $htmlAllowed = false);
diff --git a/plugins/MultiSites/API.php b/plugins/MultiSites/API.php
index fbc3068d2b..c85dc94024 100755
--- a/plugins/MultiSites/API.php
+++ b/plugins/MultiSites/API.php
@@ -87,13 +87,14 @@ class API extends \Piwik\Plugin\API
     {
         Piwik::checkUserHasSomeViewAccess();
 
-        $idSites = $this->getSitesIdFromPattern($pattern);
+        $sites = $this->getSitesIdFromPattern($pattern, $_restrictSitesToLogin);
 
-        if (empty($idSites)) {
+        if (empty($sites)) {
             return new DataTable();
         }
+
         return $this->buildDataTable(
-            $idSites,
+            $sites,
             $period,
             $date,
             $segment,
@@ -106,27 +107,56 @@ class API extends \Piwik\Plugin\API
     /**
      * Fetches the list of sites which names match the string pattern
      *
-     * @param $pattern
+     * @param string $pattern
+     * @param bool   $_restrictSitesToLogin
      * @return array|string
      */
-    private function getSitesIdFromPattern($pattern)
+    private function getSitesIdFromPattern($pattern, $_restrictSitesToLogin)
     {
-        $idSites = 'all';
+        // First clear cache
+        Site::clearCache();
+
         if (empty($pattern)) {
-            return $idSites;
-        }
-        $idSites = array();
-        $sites = Request::processRequest('SitesManager.getPatternMatchSites',
-            array('pattern'   => $pattern,
-                  // added because caller could overwrite these
-                  'serialize' => 0,
-                  'format'    => 'original'));
-        if (!empty($sites)) {
-            foreach ($sites as $site) {
-                $idSites[] = $site['idsite'];
+
+            /** @var Scheduler $scheduler */
+            $scheduler = StaticContainer::getContainer()->get('Piwik\Scheduler\Scheduler');
+            // Then, warm the cache with only the data we should have access to
+            if (Piwik::hasUserSuperUserAccess()
+                // Hack: when this API function is called as a Scheduled Task, Super User status is enforced.
+                // This means this function would return ALL websites in all cases.
+                // Instead, we make sure that only the right set of data is returned
+                && !$scheduler->isRunningTask()
+            ) {
+                APISitesManager::getInstance()->getAllSites();
+            } else {
+                APISitesManager::getInstance()->getSitesWithAtLeastViewAccess($limit = false, $_restrictSitesToLogin);
+            }
+
+        } else {
+            $sites = Request::processRequest('SitesManager.getPatternMatchSites',
+                array('pattern'   => $pattern,
+                      // added because caller could overwrite these
+                      'showColumns' => '',
+                      'hideColumns' => '',
+                      'serialize'   => 0,
+                      'format'      => 'original'));
+
+            if (!empty($sites)) {
+                $idSites = array();
+                foreach ($sites as $site) {
+                    $idSites[] = $site['idsite'];
+                }
+
+                $model = new ModelSitesManager();
+                $sites = $model->getSitesFromIds($idSites); // getPatternMatchSites does not return all sites information...
+                Site::setSitesFromArray($sites);
             }
         }
-        return $idSites;
+
+        // Both calls above have called Site::setSitesFromArray. We now get these sites:
+        $sitesToProblablyAdd = Site::getSites();
+
+        return $sitesToProblablyAdd;
     }
 
     /**
@@ -145,8 +175,11 @@ class API extends \Piwik\Plugin\API
     public function getOne($idSite, $period, $date, $segment = false, $_restrictSitesToLogin = false, $enhanced = false)
     {
         Piwik::checkUserHasViewAccess($idSite);
+
+        $sites = $this->getSiteFromId($idSite);
+
         return $this->buildDataTable(
-            $idSite,
+            $sites,
             $period,
             $date,
             $segment,
@@ -156,37 +189,26 @@ class API extends \Piwik\Plugin\API
         );
     }
 
-    private function buildDataTable($idSitesOrIdSite, $period, $date, $segment, $_restrictSitesToLogin, $enhanced, $multipleWebsitesRequested)
+    private function getSiteFromId($idSite)
     {
-        $allWebsitesRequested = ($idSitesOrIdSite == 'all');
-        if ($allWebsitesRequested) {
-            // First clear cache
-            Site::clearCache();
-            /** @var Scheduler $scheduler */
-            $scheduler = StaticContainer::getContainer()->get('Piwik\Scheduler\Scheduler');
-            // Then, warm the cache with only the data we should have access to
-            if (Piwik::hasUserSuperUserAccess()
-                // Hack: when this API function is called as a Scheduled Task, Super User status is enforced.
-                // This means this function would return ALL websites in all cases.
-                // Instead, we make sure that only the right set of data is returned
-                && !$scheduler->isRunningTask()
-            ) {
-                APISitesManager::getInstance()->getAllSites();
-            } else {
-                APISitesManager::getInstance()->getSitesWithAtLeastViewAccess($limit = false, $_restrictSitesToLogin);
+        $idSite = (int) $idSite;
+        $sites = array(APISitesManager::getInstance()->getSiteFromId($idSite));
+
+        return $sites;
+    }
+
+    private function buildDataTable($sitesToProblablyAdd, $period, $date, $segment, $_restrictSitesToLogin, $enhanced, $multipleWebsitesRequested)
+    {
+        $idSites = array();
+        if (!empty($sitesToProblablyAdd)) {
+            foreach ($sitesToProblablyAdd as $site) {
+                $idSites[] = $site['idsite'];
             }
-            // Both calls above have called Site::setSitesFromArray. We now get these sites:
-            $sitesToProblablyAdd = Site::getSites();
-        } else if (is_array($idSitesOrIdSite)) {
-            $model = new ModelSitesManager();
-            $sitesToProblablyAdd = $model->getSitesFromIds($idSitesOrIdSite);
-        } else {
-            $sitesToProblablyAdd = array(APISitesManager::getInstance()->getSiteFromId($idSitesOrIdSite));
         }
 
         // build the archive type used to query archive data
         $archive = Archive::build(
-            $idSitesOrIdSite,
+            $idSites,
             $period,
             $date,
             $segment,
@@ -211,7 +233,10 @@ class API extends \Piwik\Plugin\API
         // $dataTable instanceOf Set
         $dataTable = $archive->getDataTableFromNumeric($fieldsToGet);
 
-        $dataTable = $this->mergeDataTableMapAndPopulateLabel($idSitesOrIdSite, $multipleWebsitesRequested, $dataTable);
+        if ($multipleWebsitesRequested && count($idSites) === 1 && Range::isMultiplePeriod($date, $period)) {
+        } else {
+            $dataTable = $this->mergeDataTableMapAndPopulateLabel($idSites, $multipleWebsitesRequested, $dataTable);
+        }
 
         if ($dataTable instanceof DataTable\Map) {
             foreach ($dataTable->getDataTables() as $table) {
@@ -237,11 +262,15 @@ class API extends \Piwik\Plugin\API
                 $dataTable->setMetadata(self::getLastPeriodMetadataName('date'), $lastPeriod);
             }
 
-            $pastArchive = Archive::build($idSitesOrIdSite, $period, $strLastDate, $segment, $_restrictSitesToLogin);
+            $pastArchive = Archive::build($idSites, $period, $strLastDate, $segment, $_restrictSitesToLogin);
 
             $pastData = $pastArchive->getDataTableFromNumeric($fieldsToGet);
 
-            $pastData = $this->mergeDataTableMapAndPopulateLabel($idSitesOrIdSite, $multipleWebsitesRequested, $pastData);
+            if ($multipleWebsitesRequested && count($idSites) === 1 && Range::isMultiplePeriod($date, $period)) {
+
+            } else {
+                $pastData = $this->mergeDataTableMapAndPopulateLabel($idSites, $multipleWebsitesRequested, $pastData);
+            }
 
             // use past data to calculate evolution percentages
             $this->calculateEvolutionPercentages($dataTable, $pastData, $apiMetrics);
@@ -259,14 +288,9 @@ class API extends \Piwik\Plugin\API
             $dataTable->queueFilter('ColumnDelete', array('label'));
         }
 
-        Site::clearCache();
-
         // replace record names with user friendly metric names
         $dataTable->queueFilter('ReplaceColumnNames', array($columnNameRewrites));
 
-        // Ensures data set sorted, for Metadata output
-        $dataTable->filter('Sort', array(self::NB_VISITS_METRIC, 'desc', $naturalSort = false));
-
         // filter rows without visits
         // note: if only one website is queried and there are no visits, we can not remove the row otherwise
         // ResponseBuilder throws 'Call to a member function getColumns() on a non-object'
diff --git a/tests/PHPUnit/System/BackwardsCompatibility1XTest.php b/tests/PHPUnit/System/BackwardsCompatibility1XTest.php
index fa8de721a3..de34f10739 100644
--- a/tests/PHPUnit/System/BackwardsCompatibility1XTest.php
+++ b/tests/PHPUnit/System/BackwardsCompatibility1XTest.php
@@ -104,7 +104,10 @@ class BackwardsCompatibility1XTest extends SystemTestCase
 
              // the Action.getPageTitles test fails for unknown reason, so skipping it
              // eg. https://travis-ci.org/piwik/piwik/jobs/24449365
-            'Action.getPageTitles'
+            'Action.getPageTitles',
+
+             // the label column is not the first column here
+            'MultiSites.getAll'
         );
 
         return array(
@@ -127,7 +130,12 @@ class BackwardsCompatibility1XTest extends SystemTestCase
                                               'periods' => array('range'), 'disableArchiving' => true)),
 
             array('VisitFrequency.get', array('idSite' => $idSite, 'date' => '2012-03-03,2012-12-12', 'periods' => array('month'),
-                                              'testSuffix' => '_multipleOldNew', 'disableArchiving' => true))
+                                              'testSuffix' => '_multipleOldNew', 'disableArchiving' => true)),
+            array('MultiSites.getAll', array('idSite' => $idSite, 'date' => $dateTime,
+                                             'disableArchiving' => true,
+                                             'otherRequestParameters' => array(
+                                                 'hideColumns' => 'nb_users',
+                                             ))),
         );
     }
 }
diff --git a/tests/PHPUnit/System/expected/test_BackwardsCompatibility1XTest__MultiSites.getAll_day.xml b/tests/PHPUnit/System/expected/test_BackwardsCompatibility1XTest__MultiSites.getAll_day.xml
new file mode 100644
index 0000000000..01a5e25002
--- /dev/null
+++ b/tests/PHPUnit/System/expected/test_BackwardsCompatibility1XTest__MultiSites.getAll_day.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<nb_visits>2</nb_visits>
+		<nb_actions>8</nb_actions>
+		<nb_pageviews>4</nb_pageviews>
+		<revenue>43</revenue>
+		<label>new name</label>
+		<visits_evolution>100%</visits_evolution>
+		<actions_evolution>100%</actions_evolution>
+		<pageviews_evolution>100%</pageviews_evolution>
+		<revenue_evolution>100%</revenue_evolution>
+		<group />
+		<main_url>http://site.com</main_url>
+		<idsite>1</idsite>
+	</row>
+</result>
\ No newline at end of file