Proper encoding of segment values for Actions reports. (#13481)

6 files changed, 102 insertions, 20 deletions

diff --git a/plugins/Actions/DataTable/Filter/Actions.php b/plugins/Actions/DataTable/Filter/Actions.php
index 1295cae57e..29bfe1bf49 100644
--- a/plugins/Actions/DataTable/Filter/Actions.php
+++ b/plugins/Actions/DataTable/Filter/Actions.php
@@ -35,7 +35,8 @@ class Actions extends BaseFilter
      */
     public function filter($table)
     {
-        $table->filter(function (DataTable $dataTable) {
+        $isFlattening = Common::getRequestVar('flat', 0);
+        $table->filter(function (DataTable $dataTable) use ($isFlattening) {
             $site = $dataTable->getMetadata('site');
             $urlPrefix = $site ? $site->getMainUrl() : null;
 
@@ -60,21 +61,21 @@ class Actions extends BaseFilter
                     $folderUrlStart = $row->getMetadata('folder_url_start');
                     $label = $row->getColumn('label');
                     if ($url) {
-                        $row->setMetadata('segmentValue', urldecode($url));
+                        $row->setMetadata('segmentValue', urlencode($url));
                     } else if ($folderUrlStart) {
                         $row->setMetadata('segment', 'pageUrl=^' . urlencode(urlencode($folderUrlStart)));
                     } else if ($pageTitlePath) {
                         if ($row->getIdSubDataTable()) {
-                            $row->setMetadata('segment', 'pageTitle=^' . urlencode(urlencode(trim(urldecode($pageTitlePath)))));
+                            $row->setMetadata('segment', 'pageTitle=^' . urlencode(urlencode(trim($pageTitlePath))));
                         } else {
-                            $row->setMetadata('segmentValue', trim(urldecode($pageTitlePath)));
+                            $row->setMetadata('segmentValue', urlencode(trim($pageTitlePath)));
                         }
                     } else if ($isPageTitleType && !in_array($label, [DataTable::LABEL_SUMMARY_ROW])) {
                         // for older data w/o page_title_path metadata
                         if ($row->getIdSubDataTable()) {
-                            $row->setMetadata('segment', 'pageTitle=^' . urlencode(urlencode(trim(urldecode($label)))));
+                            $row->setMetadata('segment', 'pageTitle=^' . urlencode(urlencode(trim($label))));
                         } else {
-                            $row->setMetadata('segmentValue', trim(urldecode($label)));
+                            $row->setMetadata('segmentValue', urlencode(trim($label)));
                         }
                     } else if ($this->actionType == Action::TYPE_PAGE_URL && $urlPrefix) { // folder for older data w/ no folder URL metadata
                         $row->setMetadata('segment', 'pageUrl=^' . urlencode(urlencode($urlPrefix . '/' . $label)));
@@ -82,7 +83,7 @@ class Actions extends BaseFilter
                 }
 
                 // remove the default action name 'index' in the end of flattened urls and prepend $actionDelimiter
-                if (Common::getRequestVar('flat', 0)) {
+                if ($isFlattening) {
                     $label = $row->getColumn('label');
                     $stringToSearch = $actionDelimiter.$defaultActionName;
                     if (substr($label, -strlen($stringToSearch)) == $stringToSearch) {
@@ -98,11 +99,6 @@ class Actions extends BaseFilter
             }
         });
 
-        // TODO can we remove this one again?
-        $table->queueFilter('GroupBy', array('label', function ($label) {
-            return urldecode($label);
-        }));
-
         foreach ($table->getRowsWithoutSummaryRow() as $row) {
             $subtable = $row->getSubtable();
             if ($subtable) {
diff --git a/plugins/Actions/tests/System/ApiTest.php b/plugins/Actions/tests/System/ApiTest.php
new file mode 100644
index 0000000000..40e110dada
--- /dev/null
+++ b/plugins/Actions/tests/System/ApiTest.php
@@ -0,0 +1,86 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Plugins\Actions\tests\System;
+
+use Piwik\API\Request;
+use Piwik\DataTable;
+use Piwik\Tests\Framework\Fixture;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
+
+class ApiTest extends IntegrationTestCase
+{
+    public function test_actionUrlSegmentValueIsProperlyEncoded_inActionsReports()
+    {
+        $url = 'http://example+site.org/a+b/index.html';
+
+        $idSite = Fixture::createWebsite('2012-03-04 00:00:00');
+        $t = Fixture::getTracker($idSite, '2015-03-04 03:24:00');
+        $t->setUrl($url);
+        Fixture::checkResponse($t->doTrackPageView('a page+view'));
+
+        /** @var DataTable $urls */
+        $urls = Request::processRequest('Actions.getPageUrls', [
+            'idSite' => $idSite,
+            'period' => 'day',
+            'date' => '2015-03-04',
+            'flat' => '1',
+        ]);
+
+        $this->assertEquals(1, $urls->getRowsCount());
+
+        $urlSegment = $urls->getFirstRow()->getMetadata('segment');
+
+        /** @var DataTable $urlsWithSegment */
+        $urlsWithSegment = Request::processRequest('Actions.getPageUrls', [
+            'idSite' => $idSite,
+            'period' => 'day',
+            'date' => '2015-03-04',
+            'segment' => $urlSegment,
+            'flat' => '1',
+        ]);
+
+        $this->assertEquals(1, $urlsWithSegment->getRowsCount());
+
+        // NOTE: the label here is incorrect due to SafeDecodeLabel. this is a known issue, but changing it would
+        // break BC elsewhere
+        $this->assertEquals('/a b/index.html', $urlsWithSegment->getFirstRow()->getColumn('label'));
+
+        $pages = Request::processRequest('Actions.getPageTitles', [
+            'idSite' => $idSite,
+            'period' => 'day',
+            'date' => '2015-03-04',
+            'flat' => '1',
+        ]);
+
+        $this->assertEquals(1, $pages->getRowsCount());
+
+        $pageSegment = $pages->getFirstRow()->getMetadata('segment');
+
+        /** @var DataTable $pagesWithSegment */
+        $pagesWithSegment = Request::processRequest('Actions.getPageTitles', [
+            'idSite' => $idSite,
+            'period' => 'day',
+            'date' => '2015-03-04',
+            'segment' => $pageSegment,
+            'flat' => '1',
+        ]);
+
+        $this->assertEquals(1, $pagesWithSegment->getRowsCount());
+
+        // NOTE: the label here is incorrect due to SafeDecodeLabel. this is a known issue, but changing it would
+        // break BC elsewhere
+        $this->assertEquals('a page view', $pagesWithSegment->getFirstRow()->getColumn('label'));
+    }
+
+    protected static function configureFixture($fixture)
+    {
+        parent::configureFixture($fixture);
+        $fixture->createSuperUser = true;
+    }
+}
\ No newline at end of file
diff --git a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_auto_expand.png b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_auto_expand.png
index 3ff4a2ccd8..357a8a70a8 100644
--- a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_auto_expand.png
+++ b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_auto_expand.png
@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:377222cc01db62a192d1d01524aaa651a25c73fff29a5a051b878a8dc56f3cf4
-size 376343
+oid sha256:465cb75127af83e432460cdc96833cd24b0ef552449da338a3e2c8461201255b
+size 381867
diff --git a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_column_sorted.png b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_column_sorted.png
index ae4a0ad6d1..4e32cb3937 100644
--- a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_column_sorted.png
+++ b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_column_sorted.png
@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:e4f9b43bd75105de4023baf9345cbe47f138e69df30ff4c745c7d058eba1b229
-size 352520
+oid sha256:684944d2901be9a9b9c2fff980e6586dd1f65f69cce4f7e6c759874a7a50ccb0
+size 357980
diff --git a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_initial.png b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_initial.png
index dfd04fc0e8..e576e5d2af 100644
--- a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_initial.png
+++ b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_initial.png
@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:9d529f2e6ef47420eff3159924c5de969b93ef4447ad45f27114f0c0203596b5
-size 351793
+oid sha256:7470a5b62a30f1baede58f5424ba723bcd9c7bf2553c266d9f3114f8c5d92050
+size 357256
diff --git a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_subtables_loaded.png b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_subtables_loaded.png
index d0414cc612..1b73f0a03e 100644
--- a/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_subtables_loaded.png
+++ b/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_subtables_loaded.png
@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:b006bd8903a40cda94780f77e59632871ad9c6264e77ad3291e31560bc829a0c
-size 419585
+oid sha256:3002fa86fbcd1af5bd3665f56f352efe567e89ad98e85dea4e7ef669903322af
+size 425752