For Outlinks and Download URLs, do not exclude URL parameters and sto…

…re URLs untouched Also adds a missing URL Encoding for action URLs. (when outlink or download urls contained `&` and such characters breaking the URL this possibly corrupted the tracking request). Fixes #6244
matomo-org · May 25, 2015 · 5e3f188 · 5e3f188
1 parent fe3e5ec
commit 5e3f188
Show file tree

Hide file tree

Showing 14 changed files with 74 additions and 40 deletions.
diff --git a/core/Tracker/Action.php b/core/Tracker/Action.php
@@ -206,6 +206,12 @@ protected function setActionUrl($url)
         }
     }
 
+    protected function setActionUrlWithoutExcludingParameters($url)
+    {
+        $this->rawActionUrl = PageUrl::getUrlIfLookValid($url);
+        $this->actionUrl = PageUrl::getUrlIfLookValid($url);
+    }
+
     abstract protected function getActionsToLookup();
 
     protected function getUrlAndType()

diff --git a/plugins/Actions/Actions/ActionClickUrl.php b/plugins/Actions/Actions/ActionClickUrl.php
@@ -24,7 +24,7 @@ class ActionClickUrl extends Action
     public function __construct(Request $request)
     {
         parent::__construct(self::TYPE_OUTLINK, $request);
-        $this->setActionUrl($request->getParam('link'));
+        $this->setActionUrlWithoutExcludingParameters($request->getParam('link'));
     }
 
     public static function shouldHandle(Request $request)

diff --git a/plugins/Actions/Actions/ActionDownloadUrl.php b/plugins/Actions/Actions/ActionDownloadUrl.php
@@ -20,7 +20,7 @@ class ActionDownloadUrl extends Action
     public function __construct(Request $request)
     {
         parent::__construct(self::TYPE_DOWNLOAD, $request);
-        $this->setActionUrl($request->getParam('download'));
+        $this->setActionUrlWithoutExcludingParameters($request->getParam('download'));
     }
 
     public static function shouldHandle(Request $request)

diff --git a/plugins/Actions/ArchivingHelper.php b/plugins/Actions/ArchivingHelper.php
@@ -603,7 +603,12 @@ private static function parseNameFromPageUrl($name, $type, $urlPrefix)
         $urlFragment = $matches[3];
 
         if (in_array($type, array(Action::TYPE_DOWNLOAD, Action::TYPE_OUTLINK))) {
-            return array(trim($urlHost), '/' . trim($urlPath));
+            $path = '/' . trim($urlPath);
+            if (!empty($urlFragment)) {
+                $path .= '#' . $urlFragment;
+            }
+
+            return array(trim($urlHost), $path);
         }
 
         $name = $urlPath;

diff --git a/tests/PHPUnit/Fixtures/OneVisitorTwoVisits.php b/tests/PHPUnit/Fixtures/OneVisitorTwoVisits.php
@@ -106,15 +106,17 @@ private function trackVisits()
 
         // Click on external link after 6 minutes (3rd action)
         $t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.1)->getDatetime());
-        self::checkResponse($t->doTrackAction('http://dev.piwik.org/svn', 'link'));
+
+        // Testing Outlink that contains a URL Fragment
+        self::checkResponse($t->doTrackAction('https://outlinks.org/#!outlink-with-fragment-<script>', 'link'));
 
         // Click on file download after 12 minutes (4th action)
         $t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.2)->getDatetime());
         self::checkResponse($t->doTrackAction('http://piwik.org/path/again/latest.zip', 'download'));
 
         // Click on two more external links, one the same as before (5th & 6th actions)
         $t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.22)->getDateTime());
-        self::checkResponse($t->doTrackAction('http://outlinks.org/other_outlink', 'link'));
+        self::checkResponse($t->doTrackAction('http://outlinks.org/other_outlink#fragment&pk_campaign=Open%20partnership', 'link'));
         $t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.25)->getDateTime());
         self::checkResponse($t->doTrackAction('http://dev.piwik.org/svn', 'link'));
 

diff --git a/tests/PHPUnit/System/BackwardsCompatibility1XTest.php b/tests/PHPUnit/System/BackwardsCompatibility1XTest.php
@@ -124,6 +124,11 @@ public function getApiForTesting()
              // the Action.getPageTitles test fails for unknown reason, so skipping it
              // eg. https://travis-ci.org/piwik/piwik/jobs/24449365
             'Action.getPageTitles',
+
+            // Outlinks now tracked with URL Fragment which was not the case in 1.X
+            'Actions.get',
+            'Actions.getOutlink',
+            'Actions.getOutlinks',
         );
 
         $apiNotToCall = array_merge($apiNotToCall, $reportsToCompareSeparately);

diff --git a/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.getOutlink_day.xml b/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.getOutlink_day.xml
@@ -4,8 +4,8 @@
 		<label>/svn</label>
 		<nb_visits>1</nb_visits>
 		<nb_uniq_visitors>1</nb_uniq_visitors>
-		<nb_hits>2</nb_hits>
-		<sum_time_spent>540</sum_time_spent>
+		<nb_hits>1</nb_hits>
+		<sum_time_spent>180</sum_time_spent>
 		<url>http://dev.piwik.org/svn</url>
 	</row>
 </result>
diff --git a/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.getOutlinks_day.xml b/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.getOutlinks_day.xml
@@ -1,34 +1,42 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
 	<row>
-		<label>dev.piwik.org</label>
-		<nb_visits>1</nb_visits>
+		<label>outlinks.org</label>
+		<nb_visits>2</nb_visits>
 		<nb_hits>2</nb_hits>
-		<sum_time_spent>540</sum_time_spent>
+		<sum_time_spent>468</sum_time_spent>
 		<subtable>
 			<row>
-				<label>/svn</label>
+				<label>/#!outlink-with-fragment-&lt;script&gt;</label>
 				<nb_visits>1</nb_visits>
 				<nb_uniq_visitors>1</nb_uniq_visitors>
-				<nb_hits>2</nb_hits>
-				<sum_time_spent>540</sum_time_spent>
-				<url>http://dev.piwik.org/svn</url>
+				<nb_hits>1</nb_hits>
+				<sum_time_spent>360</sum_time_spent>
+				<url>https://outlinks.org/#!outlink-with-fragment-&lt;script&gt;</url>
+			</row>
+			<row>
+				<label>/other_outlink#fragment&amp;pk_campaign=Open partnership</label>
+				<nb_visits>1</nb_visits>
+				<nb_uniq_visitors>1</nb_uniq_visitors>
+				<nb_hits>1</nb_hits>
+				<sum_time_spent>108</sum_time_spent>
+				<url>http://outlinks.org/other_outlink#fragment&amp;pk_campaign=Open%20partnership</url>
 			</row>
 		</subtable>
 	</row>
 	<row>
-		<label>outlinks.org</label>
+		<label>dev.piwik.org</label>
 		<nb_visits>1</nb_visits>
 		<nb_hits>1</nb_hits>
-		<sum_time_spent>108</sum_time_spent>
+		<sum_time_spent>180</sum_time_spent>
 		<subtable>
 			<row>
-				<label>/other_outlink</label>
+				<label>/svn</label>
 				<nb_visits>1</nb_visits>
 				<nb_uniq_visitors>1</nb_uniq_visitors>
 				<nb_hits>1</nb_hits>
-				<sum_time_spent>108</sum_time_spent>
-				<url>http://outlinks.org/other_outlink</url>
+				<sum_time_spent>180</sum_time_spent>
+				<url>http://dev.piwik.org/svn</url>
 			</row>
 		</subtable>
 	</row>

diff --git a/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.get_day.xml b/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.get_day.xml
@@ -5,7 +5,7 @@
 	<nb_downloads>1</nb_downloads>
 	<nb_uniq_downloads>1</nb_uniq_downloads>
 	<nb_outlinks>3</nb_outlinks>
-	<nb_uniq_outlinks>2</nb_uniq_outlinks>
+	<nb_uniq_outlinks>3</nb_uniq_outlinks>
 	<nb_searches>0</nb_searches>
 	<nb_keywords>0</nb_keywords>
 	<avg_time_generation>0.155</avg_time_generation>

diff --git a/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits_csv__API.get_month.csv b/tests/PHPUnit/System/expected/test_OneVisitorTwoVisits_csv__API.get_month.csv
diff --git a/...it/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.getOutlink_day.xml b/...it/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.getOutlink_day.xml
@@ -4,8 +4,8 @@
 		<label>/svn</label>
 		<nb_visits>1</nb_visits>
 		<nb_uniq_visitors>1</nb_uniq_visitors>
-		<nb_hits>2</nb_hits>
-		<sum_time_spent>432</sum_time_spent>
+		<nb_hits>1</nb_hits>
+		<sum_time_spent>72</sum_time_spent>
 		<url>http://dev.piwik.org/svn</url>
 	</row>
 </result>
diff --git a/...t/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.getOutlinks_day.xml b/...t/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.getOutlinks_day.xml
@@ -1,34 +1,42 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
 	<row>
-		<label>dev.piwik.org</label>
-		<nb_visits>1</nb_visits>
+		<label>outlinks.org</label>
+		<nb_visits>2</nb_visits>
 		<nb_hits>2</nb_hits>
-		<sum_time_spent>432</sum_time_spent>
+		<sum_time_spent>468</sum_time_spent>
 		<subtable>
 			<row>
-				<label>/svn</label>
+				<label>/#!outlink-with-fragment-&lt;script&gt;</label>
 				<nb_visits>1</nb_visits>
 				<nb_uniq_visitors>1</nb_uniq_visitors>
-				<nb_hits>2</nb_hits>
-				<sum_time_spent>432</sum_time_spent>
-				<url>http://dev.piwik.org/svn</url>
+				<nb_hits>1</nb_hits>
+				<sum_time_spent>360</sum_time_spent>
+				<url>https://outlinks.org/#!outlink-with-fragment-&lt;script&gt;</url>
+			</row>
+			<row>
+				<label>/other_outlink#fragment&amp;pk_campaign=Open partnership</label>
+				<nb_visits>1</nb_visits>
+				<nb_uniq_visitors>1</nb_uniq_visitors>
+				<nb_hits>1</nb_hits>
+				<sum_time_spent>108</sum_time_spent>
+				<url>http://outlinks.org/other_outlink#fragment&amp;pk_campaign=Open%20partnership</url>
 			</row>
 		</subtable>
 	</row>
 	<row>
-		<label>outlinks.org</label>
+		<label>dev.piwik.org</label>
 		<nb_visits>1</nb_visits>
 		<nb_hits>1</nb_hits>
-		<sum_time_spent>108</sum_time_spent>
+		<sum_time_spent>72</sum_time_spent>
 		<subtable>
 			<row>
-				<label>/other_outlink</label>
+				<label>/svn</label>
 				<nb_visits>1</nb_visits>
 				<nb_uniq_visitors>1</nb_uniq_visitors>
 				<nb_hits>1</nb_hits>
-				<sum_time_spent>108</sum_time_spent>
-				<url>http://outlinks.org/other_outlink</url>
+				<sum_time_spent>72</sum_time_spent>
+				<url>http://dev.piwik.org/svn</url>
 			</row>
 		</subtable>
 	</row>

diff --git a/...s/PHPUnit/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.get_day.xml b/...s/PHPUnit/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.get_day.xml
@@ -5,7 +5,7 @@
 	<nb_downloads>1</nb_downloads>
 	<nb_uniq_downloads>1</nb_uniq_downloads>
 	<nb_outlinks>3</nb_outlinks>
-	<nb_uniq_outlinks>2</nb_uniq_outlinks>
+	<nb_uniq_outlinks>3</nb_uniq_outlinks>
 	<nb_searches>1</nb_searches>
 	<nb_keywords>1</nb_keywords>
 	<avg_time_generation>0.155</avg_time_generation>

diff --git a/...em/expected/test_OneVisitorTwoVisits_withCookieSupport__Live.getLastVisitsDetails_day.xml b/...em/expected/test_OneVisitorTwoVisits_withCookieSupport__Live.getLastVisitsDetails_day.xml
@@ -21,7 +21,7 @@
 				<type>action</type>
 				<url>http://example.org/store/purchase.htm</url>
 				<pageTitle>Checkout/Purchasing...</pageTitle>
-				<pageIdAction>12</pageIdAction>
+				<pageIdAction>13</pageIdAction>
 
 				<pageId>9</pageId>
 				<generationTime>0.13s</generationTime>
@@ -147,7 +147,7 @@
 			</row>
 			<row>
 				<type>outlink</type>
-				<url>http://dev.piwik.org/svn</url>
+				<url>https://outlinks.org/#!outlink-with-fragment-&lt;script&gt;</url>
 				<pageTitle />
 				<pageIdAction>5</pageIdAction>
 
@@ -171,7 +171,7 @@
 			</row>
 			<row>
 				<type>outlink</type>
-				<url>http://outlinks.org/other_outlink</url>
+				<url>http://outlinks.org/other_outlink#fragment&amp;pk_campaign=Open%20partnership</url>
 				<pageTitle />
 				<pageIdAction>7</pageIdAction>
 
@@ -185,7 +185,7 @@
 				<type>outlink</type>
 				<url>http://dev.piwik.org/svn</url>
 				<pageTitle />
-				<pageIdAction>5</pageIdAction>
+				<pageIdAction>8</pageIdAction>
 
 				<pageId>6</pageId>
 				<timeSpent>72</timeSpent>
@@ -221,7 +221,7 @@
 				<type>action</type>
 				<url>http://example.org/index.htm</url>
 				<pageTitle>Looking at homepage after site search...</pageTitle>
-				<pageIdAction>10</pageIdAction>
+				<pageIdAction>11</pageIdAction>
 
 				<pageId>8</pageId>
 				<generationTime>0.02s</generationTime>