From 7a89c65378fd7893d5c0b0fdec925b870adb4ed9 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Tue, 16 Apr 2013 08:22:33 +0000 Subject: [PATCH] Refs #3465, make sure labels are associated with correct rows in multi-row evolution after generic filters (ie, Sort) are applied. --- core/API/DataTableManipulator/LabelFilter.php | 25 +++---- core/API/ResponseBuilder.php | 4 +- plugins/API/API.php | 68 +++++++++++++------ ...essedRowLabel__API.getRowEvolution_day.xml | 1 + 4 files changed, 60 insertions(+), 38 deletions(-) diff --git a/core/API/DataTableManipulator/LabelFilter.php b/core/API/DataTableManipulator/LabelFilter.php index 550781bd18b..6d5a4aa4b3a 100644 --- a/core/API/DataTableManipulator/LabelFilter.php +++ b/core/API/DataTableManipulator/LabelFilter.php @@ -25,7 +25,7 @@ class Piwik_API_DataTableManipulator_LabelFilter extends Piwik_API_DataTableMani const SEPARATOR_RECURSIVE_LABEL = '>'; private $labels; - private $addEmptyRows; + private $addLabelIndex; /** * Filter a data table by label. @@ -37,18 +37,18 @@ class Piwik_API_DataTableManipulator_LabelFilter extends Piwik_API_DataTableMani * * @param string $labels the labels to search for * @param Piwik_DataTable $dataTable the data table to be filtered - * @param bool $addEmptyRows Whether to add empty rows when a row isn't found - * for a label, or not. + * @param bool $addLabelIndex Whether to add label_index metadata describing which + * label a row corresponds to. * @return Piwik_DataTable */ - public function filter($labels, $dataTable, $addEmptyRows = false) + public function filter($labels, $dataTable, $addLabelIndex = false) { if (!is_array($labels)) { $labels = array($labels); } $this->labels = $labels; - $this->addEmptyRows = (bool)$addEmptyRows; + $this->addLabelIndex = (bool)$addLabelIndex; return $this->manipulate($dataTable); } @@ -137,7 +137,7 @@ private function getLabelVariations($label) protected function manipulateDataTable($dataTable) { $result = $dataTable->getEmptyClone(); - foreach ($this->labels as $label) { + foreach ($this->labels as $labelIndex => $label) { $row = null; foreach ($this->getLabelVariations($label) as $labelVariation) { $labelVariation = explode(self::SEPARATOR_RECURSIVE_LABEL, $labelVariation); @@ -145,19 +145,14 @@ protected function manipulateDataTable($dataTable) $row = $this->doFilterRecursiveDescend($labelVariation, $dataTable); if ($row) { + if ($this->addLabelIndex) { + $row->setMetadata('label_index', $labelIndex); + } + $result->addRow($row); break; } } - - if (empty($row) - && $this->addEmptyRows - ) // if no row has been found, add an empty one - { - $row = new Piwik_DataTable_Row(); - $row->setColumn('label', $label); - $result->addRow($row); - } } return $result; } diff --git a/core/API/ResponseBuilder.php b/core/API/ResponseBuilder.php index 6830e34c0e2..cfd50981f1a 100644 --- a/core/API/ResponseBuilder.php +++ b/core/API/ResponseBuilder.php @@ -291,10 +291,10 @@ protected function handleDataTable($datatable) $label = $this->getLabelQueryParam(); if (!empty($label)) { $label = Piwik_Common::unsanitizeInputValues($label); - $addEmptyRows = Piwik_Common::getRequestVar('labelFilterAddEmptyRows', 0, 'int', $this->request) == 1; + $addLabelIndex = Piwik_Common::getRequestVar('labelFilterAddLabelIndex', 0, 'int', $this->request) == 1; $filter = new Piwik_API_DataTableManipulator_LabelFilter($this->apiModule, $this->apiMethod, $this->request); - $datatable = $filter->filter($label, $datatable, $addEmptyRows); + $datatable = $filter->filter($label, $datatable, $addLabelIndex); } return $this->getRenderedDataTable($datatable); } diff --git a/plugins/API/API.php b/plugins/API/API.php index ec60ad2d247..31f5d3274ce 100644 --- a/plugins/API/API.php +++ b/plugins/API/API.php @@ -1175,6 +1175,15 @@ public function getRowEvolution($idSite, $period, $date, $apiModule, $apiAction, $labels = array_merge($labels, $table->getColumn('label')); } $labels = array_values(array_unique($labels)); + + // set label index metadata + $labelsToIndex = array_flip($labels); + foreach ($dataTable->getArray() as $table) { + foreach ($table->getRows() as $row) { + $label = $row->getColumn('label'); + $row->setMetadata('label_index', $labelsToIndex[$label]); + } + } } if (count($labels) > 1) { @@ -1219,7 +1228,7 @@ private function getSingleRowEvolution($dataTable, $idSite, $period, $date, $api { $metadata = $this->getRowEvolutionMetaData($idSite, $period, $date, $apiModule, $apiAction, $language, $idGoal); $metricNames = array_keys($metadata['metrics']); - + $logo = $actualLabel = false; $urlFound = false; foreach ($dataTable->getArray() as $date => $subTable) { @@ -1306,19 +1315,20 @@ private function loadRowEvolutionDataFromAPI($idSite, $period, $date, $apiModule $label = array_map('rawurlencode', $label); $parameters = array( - 'method' => $apiModule . '.' . $apiAction, - 'label' => $label, - 'idSite' => $idSite, - 'period' => $period, - 'date' => $date, - 'format' => 'original', - 'serialize' => '0', - 'segment' => $segment, - 'idGoal' => $idGoal, - - // if more than one label is used, we add empty rows for labels we can't - // find to ensure we know the order of the rows in the returned data table - 'labelFilterAddEmptyRows' => count($label) > 1 ? 1 : 0, + 'method' => $apiModule . '.' . $apiAction, + 'label' => $label, + 'idSite' => $idSite, + 'period' => $period, + 'date' => $date, + 'format' => 'original', + 'serialize' => '0', + 'segment' => $segment, + 'idGoal' => $idGoal, + + // if more than one label is used, we add metadata to ensure we know which + // row corresponds with which label (since the labels can change, and rows + // can be sorted in a different order) + 'labelFilterAddLabelIndex' => count($label) > 1 ? 1 : 0, ); // add "processed metrics" like actions per visit or bounce rate @@ -1330,7 +1340,7 @@ private function loadRowEvolutionDataFromAPI($idSite, $period, $date, $apiModule $apiModule != 'Actions' && ($apiModule != 'Goals' || ($apiAction != 'getVisitsUntilConversion' && $apiAction != 'getDaysToConversion')) - && $label // do not request processed metrics when retrieving top n labels + && !empty($label) ) { $parameters['filter_add_columns_when_show_all_columns'] = '1'; } @@ -1466,15 +1476,12 @@ private function getMultiRowEvolution($dataTable, $idSite, $period, $date, $apiM $metrics = array_keys($metadata['metrics']); $column = reset($metrics); } - + // get the processed label and logo (if any) for every requested label $actualLabels = $logos = array(); foreach ($labels as $labelIdx => $label) { foreach ($dataTable->getArray() as $table) { - // find row for this label. LabelFilter will add empty rows and - // keep them ordered in the same way the labels array is, so we - // assume the $labelIdx is also the row Id - $labelRow = $table->getRowFromId($labelIdx); + $labelRow = $this->getRowEvolutionRowFromLabelIdx($table, $labelIdx); if ($labelRow) { $actualLabels[$labelIdx] = $this->getRowUrlForEvolutionLabel( @@ -1500,7 +1507,7 @@ private function getMultiRowEvolution($dataTable, $idSite, $period, $date, $apiM $newRow = new Piwik_DataTable_Row(); foreach ($labels as $labelIdx => $label) { - $row = $table->getRowFromId($labelIdx); + $row = $this->getRowEvolutionRowFromLabelIdx($table, $labelIdx); $value = 0; if ($row) { @@ -1547,6 +1554,25 @@ private function getMultiRowEvolution($dataTable, $idSite, $period, $date, $apiM 'metadata' => $metadata ); } + + /** + * Returns the row in a datatable by its label_index metadata. + * + * @param Piwik_DataTable $table + * @param int $labelIdx + * @return Piwik_DataTable_Row|false + */ + private function getRowEvolutionRowFromLabelIdx($table, $labelIdx) + { + foreach ($table->getRows() as $row) + { + if ($row->getMetadata('label_index') == $labelIdx) + { + return $row; + } + } + return false; + } /** * Returns a prettier, more comprehensible version of a row evolution label diff --git a/tests/PHPUnit/Integration/expected/test_RowEvolution_processedRowLabel__API.getRowEvolution_day.xml b/tests/PHPUnit/Integration/expected/test_RowEvolution_processedRowLabel__API.getRowEvolution_day.xml index a372c32668b..7b26cb04217 100644 --- a/tests/PHPUnit/Integration/expected/test_RowEvolution_processedRowLabel__API.getRowEvolution_day.xml +++ b/tests/PHPUnit/Integration/expected/test_RowEvolution_processedRowLabel__API.getRowEvolution_day.xml @@ -58,6 +58,7 @@ Opera (Visits) + plugins/UserSettings/images/browsers/OP.gif 0 1 -100%