By default userId should overwrite the visitorId (#16124)

* By default userId should overwrite the visitorId

* handle new visits

* Update Request.php

* fix some tests

* fix tests

* fix more tests

* add more tests, fix tests

* Update global.ini.php

* add better tests

* update screenshot

* update test file

Co-authored-by: diosmosis <diosmosis@users.noreply.github.com>

10 files changed, 298 insertions, 18 deletions

diff --git a/plugins/UserId/tests/Fixtures/OverwriteUserIdFixture.php b/plugins/UserId/tests/Fixtures/OverwriteUserIdFixture.php
new file mode 100644
index 0000000000..5c1a16650d
--- /dev/null
+++ b/plugins/UserId/tests/Fixtures/OverwriteUserIdFixture.php
@@ -0,0 +1,66 @@
+<?php
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link    https://matomo.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+namespace Piwik\Plugins\UserId\tests\Fixtures;
+
+use Piwik\Config;
+use Piwik\Date;
+use Piwik\Plugins\UserId\API;
+use Piwik\Tests\Framework\Fixture;
+
+/**
+ * Generates visits with user IDs and creates the user IDs index for testing
+ */
+class OverwriteUserIdFixture extends Fixture
+{
+    public $dateTime = '2010-02-01 11:22:33';
+    public $idSite = 1;
+
+    public function setUp(): void
+    {
+        if (!self::siteCreated($idSite = 1)) {
+            self::createWebsite($this->dateTime);
+        }
+
+        $this->trackVisits();
+    }
+
+    private function trackVisits()
+    {
+        $testEnv = $this->getTestEnvironment();
+        $testEnv->overrideConfig('Tracker', 'enable_userid_overwrites_visitorid', 0);
+        $testEnv->save();
+
+        $t = self::getTracker($this->idSite, $this->dateTime, $defaultInit = true);
+        $t->setTokenAuth(self::getTokenAuth());
+        $t->enableBulkTracking();
+
+        foreach (array('user1', 'user2', 'user3') as $key => $userId) {
+            for ($numVisits = 0; $numVisits < 2; $numVisits++) {
+                $t->setUserId($userId);
+                $t->setIp('10.10.10.' . ($key+1) . $numVisits);
+                // each time we have a different visitorId and it should create many unique visitors and not just a unique
+                // visitor per user. We don't force a new visit so this should create multiple visits each time even though
+                // visitorId is always the same
+                // If userId was to overwrite the visitorId then we would only see 1 visit for each visitor, but here we see
+                // multiple visits since the visitorId always changes even though userId stays the same
+                $t->setVisitorId(substr(md5($numVisits.$key.$userId), 0, 16));
+                $t->setUrl('http://example.org/my/dir/page' . $numVisits);
+
+                $visitDateTime = Date::factory($this->dateTime)->addPeriod($numVisits, 'minute')->getDatetime();
+                $t->setForceVisitDateTime($visitDateTime);
+
+                self::assertTrue($t->doTrackPageView('incredible title ' . ($numVisits % 3)));
+            }
+        }
+
+        self::checkBulkTrackingResponse($t->doBulkTrack());
+
+        $testEnv->overrideConfig('Tracker', 'enable_userid_overwrites_visitorid', 1);
+        $testEnv->save();
+    }
+}
\ No newline at end of file
diff --git a/plugins/UserId/tests/System/ApiTest.php b/plugins/UserId/tests/System/ApiTest.php
index 82f1079d71..3d626944f1 100644
--- a/plugins/UserId/tests/System/ApiTest.php
+++ b/plugins/UserId/tests/System/ApiTest.php
@@ -8,6 +8,7 @@
 
 namespace Piwik\Plugins\UserId\tests\System;
 
+use Piwik\Config;
 use Piwik\Plugins\UserId\tests\Fixtures\TrackFewVisitsAndCreateUsers;
 use Piwik\Tests\Framework\TestCase\SystemTestCase;
 
diff --git a/plugins/UserId/tests/System/OverwriteUserIdTest.php b/plugins/UserId/tests/System/OverwriteUserIdTest.php
new file mode 100644
index 0000000000..6b018d3749
--- /dev/null
+++ b/plugins/UserId/tests/System/OverwriteUserIdTest.php
@@ -0,0 +1,90 @@
+<?php
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link https://matomo.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Plugins\UserId\tests\System;
+
+use Piwik\Config;
+use Piwik\Plugins\UserId\tests\Fixtures\OverwriteUserIdFixture;
+use Piwik\Plugins\UserId\tests\Fixtures\TrackFewVisitsAndCreateUsers;
+use Piwik\Tests\Framework\TestCase\SystemTestCase;
+
+/**
+ * @group UserId
+ * @group OverwriteUserIdTest
+ * @group Plugins
+ */
+class OverwriteUserIdTest extends SystemTestCase
+{
+    /**
+     * @var TrackFewVisitsAndCreateUsers
+     */
+    public static $fixture;
+
+    /**
+     * @dataProvider getApiForTesting
+     */
+    public function testApi($api, $params)
+    {
+        $this->runApiTests($api, $params);
+    }
+
+    public function getApiForTesting()
+    {
+        $api = 'UserId.getUsers';
+        $startDate = substr(self::$fixture->dateTime, 0, 10);
+        $endDate = date('Y-m-d', strtotime($startDate) + 3600*24*365);
+
+        $apiToTest   = array();
+        $apiToTest[] = array(
+            $api,
+            array(
+                'date' => $startDate,
+                'periods' => array('day'),
+                'idSite'     => 1,
+                'testSuffix' => ''
+            )
+        );
+        $apiToTest[] = array(
+            $api,
+            array(
+                'date' => "$startDate,$endDate",
+                'periods' => array('range'),
+                'idSite'     => 1,
+                'testSuffix' => ''
+            )
+        );
+
+        // we expext to always see 1 action only per visitor as visitorId changes every time ...
+        // we also expect a new visit to be created even though userId stays the same
+        $apiToTest[] = array(
+            'Live.getLastVisitsDetails',
+            array(
+                'date' => "$startDate,$endDate",
+                'periods' => array('range'),
+                'idSite'     => 1,
+                'testSuffix' => '',
+                'otherRequestParameters' => array('doNotFetchActions' => '1', 'showColumns' => 'idVisit,userId,visitIp,actions'),
+            )
+        );
+
+        return $apiToTest;
+    }
+
+    public static function getOutputPrefix()
+    {
+        return 'overwriteUserId';
+    }
+
+    public static function getPathToTestDirectory()
+    {
+        return dirname(__FILE__);
+    }
+
+}
+
+OverwriteUserIdTest::$fixture = new OverwriteUserIdFixture();
\ No newline at end of file
diff --git a/plugins/UserId/tests/System/expected/test___UserId.getUsers_range.xml b/plugins/UserId/tests/System/expected/test___UserId.getUsers_range.xml
index fc8e450e2e..9ee518b4c3 100644
--- a/plugins/UserId/tests/System/expected/test___UserId.getUsers_range.xml
+++ b/plugins/UserId/tests/System/expected/test___UserId.getUsers_range.xml
@@ -4,9 +4,9 @@
 		<label>user3</label>
 		<nb_visits>31</nb_visits>
 		<nb_actions>44</nb_actions>
-		<max_actions>3</max_actions>
-		<sum_visit_length>12</sum_visit_length>
-		<bounce_count>19</bounce_count>
+		<max_actions>4</max_actions>
+		<sum_visit_length>11</sum_visit_length>
+		<bounce_count>20</bounce_count>
 		<nb_visits_converted>0</nb_visits_converted>
 		<sum_daily_nb_uniq_visitors>30</sum_daily_nb_uniq_visitors>
 		<sum_daily_nb_users>30</sum_daily_nb_users>
@@ -17,9 +17,9 @@
 		<label>user2</label>
 		<nb_visits>21</nb_visits>
 		<nb_actions>29</nb_actions>
-		<max_actions>2</max_actions>
-		<sum_visit_length>8</sum_visit_length>
-		<bounce_count>13</bounce_count>
+		<max_actions>3</max_actions>
+		<sum_visit_length>7</sum_visit_length>
+		<bounce_count>14</bounce_count>
 		<nb_visits_converted>0</nb_visits_converted>
 		<sum_daily_nb_uniq_visitors>20</sum_daily_nb_uniq_visitors>
 		<sum_daily_nb_users>20</sum_daily_nb_users>
diff --git a/plugins/UserId/tests/System/expected/test_ascSortOrder__UserId.getUsers_range.xml b/plugins/UserId/tests/System/expected/test_ascSortOrder__UserId.getUsers_range.xml
index 4c4facb17c..07a49c9db5 100644
--- a/plugins/UserId/tests/System/expected/test_ascSortOrder__UserId.getUsers_range.xml
+++ b/plugins/UserId/tests/System/expected/test_ascSortOrder__UserId.getUsers_range.xml
@@ -17,9 +17,9 @@
 		<label>user2</label>
 		<nb_visits>21</nb_visits>
 		<nb_actions>29</nb_actions>
-		<max_actions>2</max_actions>
-		<sum_visit_length>8</sum_visit_length>
-		<bounce_count>13</bounce_count>
+		<max_actions>3</max_actions>
+		<sum_visit_length>7</sum_visit_length>
+		<bounce_count>14</bounce_count>
 		<nb_visits_converted>0</nb_visits_converted>
 		<sum_daily_nb_uniq_visitors>20</sum_daily_nb_uniq_visitors>
 		<sum_daily_nb_users>20</sum_daily_nb_users>
@@ -30,9 +30,9 @@
 		<label>user3</label>
 		<nb_visits>31</nb_visits>
 		<nb_actions>44</nb_actions>
-		<max_actions>3</max_actions>
-		<sum_visit_length>12</sum_visit_length>
-		<bounce_count>19</bounce_count>
+		<max_actions>4</max_actions>
+		<sum_visit_length>11</sum_visit_length>
+		<bounce_count>20</bounce_count>
 		<nb_visits_converted>0</nb_visits_converted>
 		<sum_daily_nb_uniq_visitors>30</sum_daily_nb_uniq_visitors>
 		<sum_daily_nb_users>30</sum_daily_nb_users>
diff --git a/plugins/UserId/tests/System/expected/test_limit__UserId.getUsers_range.xml b/plugins/UserId/tests/System/expected/test_limit__UserId.getUsers_range.xml
index 0ebc46bfd4..425e57694a 100644
--- a/plugins/UserId/tests/System/expected/test_limit__UserId.getUsers_range.xml
+++ b/plugins/UserId/tests/System/expected/test_limit__UserId.getUsers_range.xml
@@ -4,9 +4,9 @@
 		<label>user2</label>
 		<nb_visits>21</nb_visits>
 		<nb_actions>29</nb_actions>
-		<max_actions>2</max_actions>
-		<sum_visit_length>8</sum_visit_length>
-		<bounce_count>13</bounce_count>
+		<max_actions>3</max_actions>
+		<sum_visit_length>7</sum_visit_length>
+		<bounce_count>14</bounce_count>
 		<nb_visits_converted>0</nb_visits_converted>
 		<sum_daily_nb_uniq_visitors>20</sum_daily_nb_uniq_visitors>
 		<sum_daily_nb_users>20</sum_daily_nb_users>
diff --git a/plugins/UserId/tests/System/expected/test_overwriteUserId__Live.getLastVisitsDetails_range.xml b/plugins/UserId/tests/System/expected/test_overwriteUserId__Live.getLastVisitsDetails_range.xml
new file mode 100644
index 0000000000..dbec744212
--- /dev/null
+++ b/plugins/UserId/tests/System/expected/test_overwriteUserId__Live.getLastVisitsDetails_range.xml
@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<idVisit>2</idVisit>
+		<visitIp>10.10.10.11</visitIp>
+		<userId>user1</userId>
+		<actions>1</actions>
+	</row>
+	<row>
+		<idVisit>4</idVisit>
+		<visitIp>10.10.10.21</visitIp>
+		<userId>user2</userId>
+		<actions>1</actions>
+	</row>
+	<row>
+		<idVisit>6</idVisit>
+		<visitIp>10.10.10.31</visitIp>
+		<userId>user3</userId>
+		<actions>1</actions>
+	</row>
+	<row>
+		<idVisit>1</idVisit>
+		<visitIp>10.10.10.10</visitIp>
+		<userId>user1</userId>
+		<actions>1</actions>
+	</row>
+	<row>
+		<idVisit>3</idVisit>
+		<visitIp>10.10.10.20</visitIp>
+		<userId>user2</userId>
+		<actions>1</actions>
+	</row>
+	<row>
+		<idVisit>5</idVisit>
+		<visitIp>10.10.10.30</visitIp>
+		<userId>user3</userId>
+		<actions>1</actions>
+	</row>
+</result>
\ No newline at end of file
diff --git a/plugins/UserId/tests/System/expected/test_overwriteUserId__UserId.getUsers_day.xml b/plugins/UserId/tests/System/expected/test_overwriteUserId__UserId.getUsers_day.xml
new file mode 100644
index 0000000000..2d752ccc15
--- /dev/null
+++ b/plugins/UserId/tests/System/expected/test_overwriteUserId__UserId.getUsers_day.xml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<label>user1</label>
+		<nb_uniq_visitors>2</nb_uniq_visitors>
+		<nb_visits>2</nb_visits>
+		<nb_actions>2</nb_actions>
+		<nb_users>1</nb_users>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>2</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		
+		<segment>userId==user1</segment>
+	</row>
+	<row>
+		<label>user2</label>
+		<nb_uniq_visitors>2</nb_uniq_visitors>
+		<nb_visits>2</nb_visits>
+		<nb_actions>2</nb_actions>
+		<nb_users>1</nb_users>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>2</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		
+		<segment>userId==user2</segment>
+	</row>
+	<row>
+		<label>user3</label>
+		<nb_uniq_visitors>2</nb_uniq_visitors>
+		<nb_visits>2</nb_visits>
+		<nb_actions>2</nb_actions>
+		<nb_users>1</nb_users>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>2</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		
+		<segment>userId==user3</segment>
+	</row>
+</result>
\ No newline at end of file
diff --git a/plugins/UserId/tests/System/expected/test_overwriteUserId__UserId.getUsers_range.xml b/plugins/UserId/tests/System/expected/test_overwriteUserId__UserId.getUsers_range.xml
new file mode 100644
index 0000000000..2839b1ff17
--- /dev/null
+++ b/plugins/UserId/tests/System/expected/test_overwriteUserId__UserId.getUsers_range.xml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<label>user1</label>
+		<nb_visits>2</nb_visits>
+		<nb_actions>2</nb_actions>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>2</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<sum_daily_nb_uniq_visitors>2</sum_daily_nb_uniq_visitors>
+		<sum_daily_nb_users>1</sum_daily_nb_users>
+		
+		<segment>userId==user1</segment>
+	</row>
+	<row>
+		<label>user2</label>
+		<nb_visits>2</nb_visits>
+		<nb_actions>2</nb_actions>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>2</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<sum_daily_nb_uniq_visitors>2</sum_daily_nb_uniq_visitors>
+		<sum_daily_nb_users>1</sum_daily_nb_users>
+		
+		<segment>userId==user2</segment>
+	</row>
+	<row>
+		<label>user3</label>
+		<nb_visits>2</nb_visits>
+		<nb_actions>2</nb_actions>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>2</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<sum_daily_nb_uniq_visitors>2</sum_daily_nb_uniq_visitors>
+		<sum_daily_nb_users>1</sum_daily_nb_users>
+		
+		<segment>userId==user3</segment>
+	</row>
+</result>
\ No newline at end of file
diff --git a/plugins/UserId/tests/System/expected/test_searchByUserId__UserId.getUsers_range.xml b/plugins/UserId/tests/System/expected/test_searchByUserId__UserId.getUsers_range.xml
index 14cd163444..2f4630730a 100644
--- a/plugins/UserId/tests/System/expected/test_searchByUserId__UserId.getUsers_range.xml
+++ b/plugins/UserId/tests/System/expected/test_searchByUserId__UserId.getUsers_range.xml
@@ -4,9 +4,9 @@
 		<label>user2</label>
 		<nb_visits>21</nb_visits>
 		<nb_actions>29</nb_actions>
-		<max_actions>2</max_actions>
-		<sum_visit_length>8</sum_visit_length>
-		<bounce_count>13</bounce_count>
+		<max_actions>3</max_actions>
+		<sum_visit_length>7</sum_visit_length>
+		<bounce_count>14</bounce_count>
 		<nb_visits_converted>0</nb_visits_converted>
 		<sum_daily_nb_uniq_visitors>20</sum_daily_nb_uniq_visitors>
 		<sum_daily_nb_users>20</sum_daily_nb_users>