refs #534:

* implementing matts suggestions * fixing truncation bug * new label encoding * extended label filter tests
matomo-org · Mar 7, 2012 · d36477c · d36477c
1 parent 4439bd3
commit d36477c
Show file tree

Hide file tree

Showing 11 changed files with 217 additions and 46 deletions.
diff --git a/core/API/DataTableLabelFilter.php b/core/API/DataTableLabelFilter.php
@@ -15,7 +15,8 @@
  * added to every API call. If the parameter is set, only the row with the matching
  * label is returned.
  * 
- * Some reports use recursive labels (e.g. action reports). Use ->>- to join them.
+ * The labels passed to this class should be urlencoded.
+ * Some reports use recursive labels (e.g. action reports). Use > to join them.
  * 
  * This filter does not work when expanded=1 is set because it is designed to load
  * only the subtables on the path, not all existing subtables (which would happen with
@@ -27,9 +28,6 @@
  */
 class Piwik_API_DataTableLabelFilter
 {
-
-    /** The separator to be used for specifying recursive labels */
-    const RECURSIVE_LABEL_SEPARATOR = '-&gt;&gt;-';
 
 	private $apiModule;
 	private $apiMethod;
@@ -67,7 +65,9 @@ public function filter($label, $dataTable, $apiModule=false, $apiMethod=false,
 			$this->apiMethod = $apiMethod;
 			$this->request = $request;
 
-			$label = explode(self::RECURSIVE_LABEL_SEPARATOR, $label);
+			$label = explode('>', $label);
+			$label = array_map('urldecode', $label);
+
 			if (count($label) > 1)
 			{
 				// do a recursive search
@@ -77,7 +77,34 @@ public function filter($label, $dataTable, $apiModule=false, $apiMethod=false,
 		}
 
 		// do a non-recursive search
-		return $dataTable->getFilteredTableFromLabel($label);
+		foreach ($this->getLabelVariations($label) as $label)
+		{
+			$result = $dataTable->getFilteredTableFromLabel($label);
+			if ($result->getFirstRow() !== false)
+			{
+				return $result;
+			}
+		}
+
+		return $result;
+	}
+
+	/** Use variations of the label to make it easier to specify the desired label */
+	private function getLabelVariations($label) {
+		$variations = array(
+			$label,
+			htmlentities($label)
+		);
+
+		if ($this->apiModule == 'Actions' && $this->apiMethod == 'getPageTitles')
+		{
+			// special case: the Actions.getPageTitles report prefixes some labels with a blank.
+			// the blank might be passed by the user but is removed in Piwik_API_Request::getRequestArrayFromString.
+			$variations[] = ' '.$label;
+			$variations[] = ' '.htmlentities($label);
+		}
+
+		return $variations;
 	}
 
 	/**
@@ -137,14 +164,18 @@ protected function doFilterRecursiveDescend($labelParts, $dataTable, $date=false
 		{
 			throw new Exception("Using the label filter is not supported for DataTable ".get_class($dataTable));
 		}
+
 		// search for the first part of the tree search
         $labelPart = array_shift($labelParts);
-        $row = $dataTable->getRowFromLabel($labelPart);
-		if ($row === false)
+		foreach ($this->getLabelVariations($labelPart) as $labelPart)
 		{
-			$labelPart = htmlentities($labelPart);
 			$row = $dataTable->getRowFromLabel($labelPart);
+			if ($row !== false)
+			{
+				break;
+			}
 		}
+
 		if ($row === false)
 		{
 			// not found

diff --git a/core/API/ResponseBuilder.php b/core/API/ResponseBuilder.php
@@ -297,8 +297,9 @@ protected function handleDataTable($datatable)
         $label = Piwik_Common::getRequestVar('label', '', 'string', $this->request);
         if ($label !== '')
         {
-            $label = Piwik_Common::unsanitizeInputValue($label);
-            $label = Piwik_DataTable_Filter_SafeDecodeLabel::filterValue($label);
+			// the label can be passed with html entities.
+			// we remove them here and try with and without them in the label filter.
+			$label = html_entity_decode($label);
             $filter = new Piwik_API_DataTableLabelFilter;
             $datatable = $filter->filter($label, $datatable, $this->apiModule, $this->apiMethod, $this->request);
         }

diff --git a/core/DataTable/Array.php b/core/DataTable/Array.php
@@ -135,6 +135,25 @@ public function getArray()
 	public function getTable($label)
 	{
 		return $this->array[$label];
+    }
+
+    /**
+	 * Returns the first row
+	 * This method can be used to treat DataTable and DataTable_Array in the same way
+	 *
+	 * @return Piwik_DataTable_Row
+	 */
+	public function getFirstRow()
+	{
+		foreach ($this->array as $table)
+		{
+			$row = $table->getFirstRow();
+			if ($row !== false)
+			{
+				return $row;
+			}
+		}
+		return false;
 	}
 
 	/**

diff --git a/core/Visualization/Sparkline.php b/core/Visualization/Sparkline.php
@@ -60,7 +60,7 @@ function main()
 		foreach($this->values as $value)
 		{
 			// 50% and 50s should be plotted as 50
-			$toRemove = array('%', 's');
+			$toRemove = array('%', str_replace('%s', '', Piwik_Translate('General_Seconds')));
 			$value = str_replace($toRemove, '', $value);
 			// replace localized decimal separator
 			$value = str_replace(',', '.', $value);

diff --git a/plugins/CoreHome/DataTableAction/MultiRowEvolution.php b/plugins/CoreHome/DataTableAction/MultiRowEvolution.php
@@ -41,11 +41,16 @@ public function __construct($idSite, $date)
 		parent::__construct($idSite, $date);
 
 		// extract multiple labels from the label parameter
-		$this->labels = explode('<+MultiRow+>', html_entity_decode($this->label));
-		if (count($this->labels) == 1) throw new Exception("Expecting at least two labels.");
+		$this->labels = explode(',', $this->label);
+		$this->labels = array_map('urldecode', $this->labels);
+
+		if (count($this->labels) == 1)
+		{
+			throw new Exception("Expecting at least two labels.");
+		}
 
 		// get selected metric
-		$this->metric = Piwik_Common::getRequestVar('metric', '', 'string');
+		$this->metric = Piwik_Common::getRequestVar('column', '', 'string');
 		if (empty($this->metric))
 		{
 			$this->metric = reset(array_keys($this->availableMetrics));
@@ -66,6 +71,7 @@ protected function loadDataTable()
 		{
 			// TODO this used to be: $_GET['label'] = $this->label = $rowLabel;
 			// is the $_GET assignment obsolete after label filter refactorings?
+			// maybe it is needed for the export icon?
 			$this->label = $rowLabel;
 			$table = $this->doLoadDataTable();
 			$dataTableArrays[$rowLabelIndex] = $table->getArray();
@@ -107,10 +113,10 @@ protected function loadDataTable()
 				}
 			}
 
-			if (!$urlFound && strpos($rowLabel, Piwik_API_DataTableLabelFilter::RECURSIVE_LABEL_SEPARATOR) !== false)
+			if (!$urlFound && strpos($rowLabel, '>') !== false)
 			{
 				// if we have a recursive label and no url, use the path
-				$this->labels[$rowLabelIndex] = str_replace(Piwik_API_DataTableLabelFilter::RECURSIVE_LABEL_SEPARATOR, ' - ', $rowLabel);
+				$this->labels[$rowLabelIndex] = str_replace('>', ' - ', $rowLabel);
 			}
 		}
 

diff --git a/plugins/CoreHome/templates/datatable.js b/plugins/CoreHome/templates/datatable.js
@@ -761,25 +761,36 @@ dataTable.prototype =
 	{
 		var self = this;
 
-		if(typeof truncationOffset == 'undefined') {
+		domElemToTruncate = $(domElemToTruncate);
+
+		if (typeof domElemToTruncate.data('originalText') != 'undefined')
+		{
+			// truncate only once. otherwise, the tooltip will show the truncated text as well.
+			return;
+		}
+
+		// make the original text (before truncation) available for others. 
+		// the .truncate plugins adds a title to the dom element but the .tooltip 
+		// plugin removes that again. 
+		domElemToTruncate.data('originalText', domElemToTruncate.text());
+
+		if (typeof truncationOffset == 'undefined')
+		{
 			truncationOffset = 0;
 		}
 		var truncationLimit = 50;
-		// Different truncation limit for different datatable types?
- 		// in a subtable
-		if(typeof self.param.idSubtable != 'undefined') {}
-		// when showing all columns
-		if(typeof self.param.idSubtable == 'undefined'
-			&& self.param.viewDataTable == 'tableAllColumns') { truncationLimit = 25; }
-		// when showing all columns in a subtable, space is restricted
-		else if(self.param.viewDataTable == 'tableAllColumns') {}		
 
-		truncationLimit += truncationOffset;
+		if (typeof self.param.idSubtable == 'undefined'
+			&& self.param.viewDataTable == 'tableAllColumns')
+		{
+			// when showing all columns in a subtable, space is restricted
+			truncationLimit = 25; 
+		}
 
-		domElemToTruncate = $(domElemToTruncate);
+		truncationLimit += truncationOffset;
 		domElemToTruncate.truncate(truncationLimit);
-		$('.truncated', domElemToTruncate)
-			.tooltip();
+
+		$('.truncated', domElemToTruncate).tooltip();
 	},
 
 	//Apply some miscelleaneous style to the DataTable

diff --git a/tests/integration/LabelFilter.test.php b/tests/integration/LabelFilter.test.php
@@ -17,26 +17,28 @@ class Test_Piwik_Integration_LabelFilter extends Test_Integration_Facade
 
 	public function getApiToTest()
 	{
-        $labelsToTest = array(
+		$labelsToTest = array(
             // first level
             'nonExistent', 
-            'dir', 
-            '/0',
-            '/ééé"\'... <this is cool>!',
+            'dir' => '  dir   ', 
+            '0' => urlencode('/0'),
+
+			// TODO the label in the API output is ...&amp;#039;... why does it only work this way?
+            'thisiscool' => urlencode('/ééé&quot;&#039;... &lt;this is cool&gt;!'),
 
             // second level
-            'dir->>-nonExistent', 
-            'dir->>-/file.php?foo=bar&foo2=bar',
+            'dirnonExistent' => 'dir>nonExistent', 
+            'dirfilephpfoobarfoo2bar' => 'dir>'.urlencode('/file.php?foo=bar&foo2=bar'),
 
             // 4 levels
-            'dir2->>-sub->>-0->>-/file.php'
+            'dir2sub0filephp' => 'dir2>sub>0>'.urlencode('/file.php')
         );
 
         $return = array();
-        foreach ($labelsToTest as $label) {
-//        	var_dump(urlencode($label));
+        foreach ($labelsToTest as $suffix => $label)
+		{
             $return[] = array('Actions.getPageUrls', array(
-                'testSuffix' => '_'.preg_replace('/[^a-z0-9]*/mi', '', $label),
+                'testSuffix' => '_'.$suffix,
                 'idSite' => $this->idSite,
                 'date' => $this->dateTime,
                 'otherRequestParameters' => array(
@@ -45,7 +47,7 @@ public function getApiToTest()
                 )
             ));
         }
-        
+
         $label = 'dir';
         $return[] = array('Actions.getPageUrls', array(
             'testSuffix' => '_'.$label.'_range',
@@ -58,6 +60,47 @@ public function getApiToTest()
             )
         ));
 
+		$return[] = array('Actions.getPageTitles', array(
+            'testSuffix' => '_titles',
+            'idSite' => $this->idSite,
+            'date' => $this->dateTime,
+            'otherRequestParameters' => array(
+				// encode once for test framework and once for the label filter.
+				// note: title has no blank prefixed here. in the report it has.
+                'label' => urlencode(urlencode('incredible title! <>,;')),
+                'expanded' => 0
+            )
+        ));
+
+		$return[] = array('Actions.getPageTitles', array(
+            'testSuffix' => '_titlesRecursive',
+            'idSite' => $this->idSite,
+            'date' => $this->dateTime,
+            'otherRequestParameters' => array(
+                'label' => 
+					'   '. // test trimming
+					urlencode(urlencode('incredible parent title! <>,;').
+					'>'.
+					urlencode('subtitle <>,;')),
+                'expanded' => 0
+            )
+        ));
+
+		$keyword = '&lt;&gt;&amp;\&quot;the pdo extension is required for this adapter but the extension is not loaded';
+		$searchEngineTest = array(
+            'testSuffix' => '_keywords_html',
+            'idSite' => $this->idSite,
+            'date' => $this->dateTime,
+            'otherRequestParameters' => array(
+               	'label' => urlencode('Google>'.urlencode($keyword)),
+                'expanded' => 0
+            )
+        );
+		$return[] = array('Referers.getSearchEngines', $searchEngineTest);
+
+		$searchEngineTest['otherRequestParameters']['label'] = urlencode('Google>'.urlencode(html_entity_decode($keyword)));
+		$return[] = array('Referers.getSearchEngines', $searchEngineTest);
+
 		return $return;
 	}
 
@@ -83,24 +126,25 @@ protected function trackVisits()
     	$idSite = $this->idSite;
         $t = $this->getTracker($idSite, $dateTime, $defaultInit = true, $useThirdPartyCookie = 1);
 
+		$t->setUrlReferrer('http://www.google.com.vn/url?sa=t&rct=j&q=%3C%3E%26%5C%22the%20pdo%20extension%20is%20required%20for%20this%20adapter%20but%20the%20extension%20is%20not%20loaded&source=web&cd=4&ved=0FjAD&url=http%3A%2F%2Fforum.piwik.org%2Fread.php%3F2%2C1011&ei=y-HHAQ&usg=AFQjCN2-nt5_GgDeg&cad=rja');
         $t->setUrl('http://example.org/%C3%A9%C3%A9%C3%A9%22%27...%20%3Cthis%20is%20cool%3E!');
-        $this->checkResponse($t->doTrackPageView('incredible title!'));
+        $this->checkResponse($t->doTrackPageView('incredible title! <>,;'));
 
         $t->setUrl('http://example.org/dir/file.php?foo=bar&foo2=bar');
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.2)->getDatetime());
-        $this->checkResponse($t->doTrackPageView('incredible title!'));
+        $this->checkResponse($t->doTrackPageView('incredible title! <>,;'));
 
         $t->setUrl('http://example.org/dir/file.php?foo=bar&foo2=bar2');
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.3)->getDatetime());
-        $this->checkResponse($t->doTrackPageView('incredible title!'));
+        $this->checkResponse($t->doTrackPageView('incredible parent title! <>,; / subtitle <>,;'));
 
         $t->setUrl('http://example.org/dir2/file.php?foo=bar&foo2=bar');
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.4)->getDatetime());
-        $this->checkResponse($t->doTrackPageView('incredible title!'));
+        $this->checkResponse($t->doTrackPageView('incredible title! <>,;'));
 
         $t->setUrl('http://example.org/dir2/sub/0/file.php');
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.4)->getDatetime());
-        $this->checkResponse($t->doTrackPageView('incredible title!'));
+        $this->checkResponse($t->doTrackPageView('incredible title! <>,;'));
 
         $t->setUrl('http://example.org/0');
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.4)->getDatetime());

diff --git a/tests/integration/expected/test_LabelFilter_keywords__Referers.getSearchEngines_day.xml b/tests/integration/expected/test_LabelFilter_keywords__Referers.getSearchEngines_day.xml
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<label>&lt;&gt;&amp;\&quot;the pdo extension is required for this adapter but the extension is not loaded</label>
+		<nb_uniq_visitors>1</nb_uniq_visitors>
+		<nb_visits>1</nb_visits>
+		<nb_actions>6</nb_actions>
+		<max_actions>6</max_actions>
+		<sum_visit_length>1441</sum_visit_length>
+		<bounce_count>0</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<url>http://google.com/search?q=%3C%3E%26%5C%22the+pdo+extension+is+required+for+this+adapter+but+the+extension+is+not+loaded</url>
+	</row>
+</result>
diff --git a/tests/integration/expected/test_LabelFilter_keywords_html__Referers.getSearchEngines_day.xml b/tests/integration/expected/test_LabelFilter_keywords_html__Referers.getSearchEngines_day.xml
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<label>&lt;&gt;&amp;\&quot;the pdo extension is required for this adapter but the extension is not loaded</label>
+		<nb_uniq_visitors>1</nb_uniq_visitors>
+		<nb_visits>1</nb_visits>
+		<nb_actions>6</nb_actions>
+		<max_actions>6</max_actions>
+		<sum_visit_length>1441</sum_visit_length>
+		<bounce_count>0</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<url>http://google.com/search?q=%3C%3E%26%5C%22the+pdo+extension+is+required+for+this+adapter+but+the+extension+is+not+loaded</url>
+	</row>
+</result>