Update visit only when needed (#15869)

* update idvisitor only when needed

* better implementation

* fix tests

6 files changed, 148 insertions, 18 deletions

diff --git a/plugins/CoreHome/Columns/VisitLastActionTime.php b/plugins/CoreHome/Columns/VisitLastActionTime.php
index ed341ec673..802f9a1755 100644
--- a/plugins/CoreHome/Columns/VisitLastActionTime.php
+++ b/plugins/CoreHome/Columns/VisitLastActionTime.php
@@ -14,6 +14,7 @@ use Piwik\Tracker\Action;
 use Piwik\Tracker\Request;
 use Piwik\Tracker\Visitor;
 use Piwik\Metrics\Formatter;
+use Piwik\Tracker\VisitorRecognizer;
 
 require_once PIWIK_INCLUDE_PATH . '/plugins/VisitTime/functions.php';
 
@@ -67,7 +68,15 @@ class VisitLastActionTime extends VisitDimension
         if ($request->getParam('ping') == 1) {
             return false;
         }
-        
+
+        $originalVisit = $visitor->getVisitorColumn(VisitorRecognizer::KEY_ORIGINAL_VISIT_ROW);
+
+        if (!empty($originalVisit['visit_last_action_time'])
+            && Date::factory($originalVisit['visit_last_action_time'])->getTimestamp() > $request->getCurrentTimestamp()) {
+            // make sure to not set visit_last_action_time to an earlier time eg if tracking requests aren't sent in order
+            return $originalVisit['visit_last_action_time'];
+        }
+
         return $this->onNewVisit($request, $visitor, $action);
     }
 }
\ No newline at end of file
diff --git a/plugins/CoreHome/tests/Integration/Column/VisitLastActionTimeTest.php b/plugins/CoreHome/tests/Integration/Column/VisitLastActionTimeTest.php
new file mode 100644
index 0000000000..d58e429960
--- /dev/null
+++ b/plugins/CoreHome/tests/Integration/Column/VisitLastActionTimeTest.php
@@ -0,0 +1,121 @@
+<?php
+/**
+ * Piwik - 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\CoreHome\tests\Integration\Column;
+
+use Piwik\Cache;
+use Piwik\Common;
+use Piwik\DataAccess\ArchiveTableCreator;
+use Piwik\Date;
+use Piwik\Db;
+use Piwik\Metrics;
+use Piwik\Plugins\CoreHome\Columns\UserId;
+use Piwik\Plugins\CoreHome\Columns\VisitLastActionTime;
+use Piwik\Tests\Framework\Fixture;
+use Piwik\Tests\Framework\Mock\FakeAccess;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
+use Piwik\DataTable;
+use Piwik\Tracker\Request;
+use Piwik\Tracker\Visit\VisitProperties;
+use Piwik\Tracker\Visitor;
+use Piwik\Tracker\VisitorRecognizer;
+
+/**
+ * @group CoreHome
+ * @group VisitLastActionTimeTest
+ * @group Plugins
+ * @group Column
+ */
+class VisitLastActionTimeTest extends IntegrationTestCase
+{
+    /**
+     * @var VisitLastActionTime
+     */
+    private $lastAction;
+
+    public function setUp()
+    {
+        parent::setUp();
+        $this->lastAction = new VisitLastActionTime();
+    }
+
+    public function tearDown()
+    {
+        parent::tearDown();
+    }
+
+
+    private function makeRequest($request)
+    {
+        $request['idsite'] = 1;
+
+        return new Request($request);
+    }
+
+    private function getVisitor()
+    {
+        $visit = new VisitProperties();
+        $visit->setProperty('idvisit', '321');
+        $visit->setProperty('idvisitor', Common::hex2bin('1234567890234567'));
+        $visitor = new Visitor($visit, $isKnown = false);
+
+        return $visitor;
+    }
+
+    public function test_onExistingVisit_whenPing()
+    {
+        $request = $this->makeRequest(array('ping' => 1));
+        $visitor = $this->getVisitor();
+        $this->assertFalse($this->lastAction->onExistingVisit($request, $visitor, $action = null));
+    }
+
+    public function test_onExistingVisit_whenNewVisitReturnsTimeFromRequest()
+    {
+        $now = time() - 5; // -5 so we make sure this time is used and not actually now
+        $request = $this->makeRequest(array('cdt' => $now));
+        $this->assertEquals($now, $request->getCurrentTimestamp());
+
+        $visitor = $this->getVisitor();
+
+        $expected = Date::factory($now)->getDatetime();
+        $this->assertSame($expected, $this->lastAction->onExistingVisit($request, $visitor, $action = null));
+    }
+
+    public function test_onExistingVisit_whenKnownVisitRequestTimeIsNewer()
+    {
+        $now = time() - 5; // -5 so we make sure this time is used and not actually now
+        $previousTime = $now - 10; // is older
+        $request = $this->makeRequest(array('cdt' => $now));
+        $this->assertEquals($now, $request->getCurrentTimestamp());
+
+        $visitor = $this->getVisitor();
+        $visitor->setVisitorColumn(VisitorRecognizer::KEY_ORIGINAL_VISIT_ROW,
+            array('visit_last_action_time' => Date::factory($previousTime)->getDatetime())
+        );
+
+        $expected = Date::factory($now)->getDatetime();
+        $this->assertSame($expected, $this->lastAction->onExistingVisit($request, $visitor, $action = null));
+    }
+
+    public function test_onExistingVisit_whenKnownVisitAndPreviousVisitTimeIsNewer()
+    {
+        $now = time() - 5; // -5 so we make sure this time is used and not actually now
+        $previousTime = $now + 10; // is newer
+        $request = $this->makeRequest(array('cdt' => $now));
+        $this->assertEquals($now, $request->getCurrentTimestamp());
+
+        $visitor = $this->getVisitor();
+        $visitor->setVisitorColumn(VisitorRecognizer::KEY_ORIGINAL_VISIT_ROW,
+            array('visit_last_action_time' => Date::factory($previousTime)->getDatetime())
+        );
+
+        $expected = Date::factory($previousTime)->getDatetime();
+        // should keep existing visit last action time
+        $this->assertSame($expected, $this->lastAction->onExistingVisit($request, $visitor, $action = null));
+    }
+}
diff --git a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__Goals.get_day.xml b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__Goals.get_day.xml
index 27200051aa..3a8d1b2d0c 100644
--- a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__Goals.get_day.xml
+++ b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__Goals.get_day.xml
@@ -1,14 +1,14 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
-	<nb_conversions>8</nb_conversions>
+	<nb_conversions>7</nb_conversions>
 	<nb_visits_converted>2</nb_visits_converted>
 	<revenue>1332</revenue>
 	<conversion_rate>100%</conversion_rate>
-	<nb_conversions_new_visit>6</nb_conversions_new_visit>
+	<nb_conversions_new_visit>4</nb_conversions_new_visit>
 	<nb_visits_converted_new_visit>1</nb_visits_converted_new_visit>
 	<revenue_new_visit>1332</revenue_new_visit>
 	<conversion_rate_new_visit>100%</conversion_rate_new_visit>
-	<nb_conversions_returning_visit>2</nb_conversions_returning_visit>
+	<nb_conversions_returning_visit>3</nb_conversions_returning_visit>
 	<nb_visits_converted_returning_visit>1</nb_visits_converted_returning_visit>
 	<revenue_returning_visit>0</revenue_returning_visit>
 	<conversion_rate_returning_visit>100%</conversion_rate_returning_visit>
diff --git a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitTime.getVisitInformationPerServerTime_day.xml b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitTime.getVisitInformationPerServerTime_day.xml
index 4a3f7b7062..c5be750480 100644
--- a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitTime.getVisitInformationPerServerTime_day.xml
+++ b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitTime.getVisitInformationPerServerTime_day.xml
@@ -6,9 +6,9 @@
 		<nb_visits>2</nb_visits>
 		<nb_actions>5</nb_actions>
 		<nb_users>0</nb_users>
-		<max_actions>3</max_actions>
-		<sum_visit_length>363</sum_visit_length>
-		<bounce_count>0</bounce_count>
+		<max_actions>4</max_actions>
+		<sum_visit_length>1121</sum_visit_length>
+		<bounce_count>1</bounce_count>
 		<nb_visits_converted>2</nb_visits_converted>
 		<goals>
 			<row idgoal='1'>
@@ -22,8 +22,8 @@
 				<revenue>666</revenue>
 			</row>
 			<row idgoal='3'>
-				<nb_conversions>2</nb_conversions>
-				<nb_visits_converted>2</nb_visits_converted>
+				<nb_conversions>1</nb_conversions>
+				<nb_visits_converted>1</nb_visits_converted>
 				<revenue>0</revenue>
 			</row>
 			<row idgoal='4'>
@@ -37,7 +37,7 @@
 				<revenue>0</revenue>
 			</row>
 		</goals>
-		<nb_conversions>8</nb_conversions>
+		<nb_conversions>7</nb_conversions>
 		<revenue>1332</revenue>
 		<segment>visitStartServerHour==0</segment>
 	</row>
diff --git a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitsSummary.get_day.xml b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitsSummary.get_day.xml
index 7168091d51..bfbd730f69 100644
--- a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitsSummary.get_day.xml
+++ b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit__VisitsSummary.get_day.xml
@@ -5,10 +5,10 @@
 	<nb_visits>2</nb_visits>
 	<nb_actions>5</nb_actions>
 	<nb_visits_converted>2</nb_visits_converted>
-	<bounce_count>0</bounce_count>
-	<sum_visit_length>363</sum_visit_length>
-	<max_actions>3</max_actions>
-	<bounce_rate>0%</bounce_rate>
+	<bounce_count>1</bounce_count>
+	<sum_visit_length>1121</sum_visit_length>
+	<max_actions>4</max_actions>
+	<bounce_rate>50%</bounce_rate>
 	<nb_actions_per_visit>2.5</nb_actions_per_visit>
-	<avg_time_on_site>182</avg_time_on_site>
+	<avg_time_on_site>561</avg_time_on_site>
 </result>
\ No newline at end of file
diff --git a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit_withLogLinkVisitActionSegment__Goals.get_day.xml b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit_withLogLinkVisitActionSegment__Goals.get_day.xml
index 27200051aa..3a8d1b2d0c 100644
--- a/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit_withLogLinkVisitActionSegment__Goals.get_day.xml
+++ b/plugins/Goals/tests/System/expected/test_trackGoals_allowMultipleConversionsPerVisit_withLogLinkVisitActionSegment__Goals.get_day.xml
@@ -1,14 +1,14 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
-	<nb_conversions>8</nb_conversions>
+	<nb_conversions>7</nb_conversions>
 	<nb_visits_converted>2</nb_visits_converted>
 	<revenue>1332</revenue>
 	<conversion_rate>100%</conversion_rate>
-	<nb_conversions_new_visit>6</nb_conversions_new_visit>
+	<nb_conversions_new_visit>4</nb_conversions_new_visit>
 	<nb_visits_converted_new_visit>1</nb_visits_converted_new_visit>
 	<revenue_new_visit>1332</revenue_new_visit>
 	<conversion_rate_new_visit>100%</conversion_rate_new_visit>
-	<nb_conversions_returning_visit>2</nb_conversions_returning_visit>
+	<nb_conversions_returning_visit>3</nb_conversions_returning_visit>
 	<nb_visits_converted_returning_visit>1</nb_visits_converted_returning_visit>
 	<revenue_returning_visit>0</revenue_returning_visit>
 	<conversion_rate_returning_visit>100%</conversion_rate_returning_visit>