Couple assorted changes (#13935)

* Allow annotations API to accept multiple periods, so evolution graphs that use multiple periods work. * Remove warning when rows_to_display viewdatatable config property is left at its default value. * Allow individual cells in an html visualization to be styled (if the visualization is extended). * Remove unneeded TODO. * In series picker encode picked rows in case the labels have commas. * Must decode the rows value as well (as it is not handled by API, must be done in plugin). * Allow joins to specified through LogAggregator::queryConversionsByDimension(). * Add safety check to _idts processing: if visitor is unknown, ignore _idts value, since it is their first visit. * In the tracker when searching by visitor ID, search through entire log_visit table instead of just in the last 30 mins. * When tracking visitor days since first, do not round since this can result in inaccurate data when rounding up. Which can cause trouble when finding the start visit for a log. * Allow HtmlTable descendants to add any html attributes to cells. * Allow derived Visualizations to add custom parameters to API requests via a new RequestConfig method. * Tweak to TODO. * Add test for annotations API change & get to pass. * Apply more review feedback * Update INI config docs for window_look_back_for_visitor. * Only copy visitor properties if action is part of an existing visit. * Some more properties that should be copied over from known visitor even if new visit. * Fixing some tests. * update test * Fix CustomEventsTest test failures. * Fixing more tests. * update rest of tests * Fixing tests. * Update some test files. * Fix log statements. * To better handle out of order actions, add part of last_action_time check to visitor ID search. * Update tests. * Updating expected screenshots. * Fix ArchiveCronTest. * Throw exception if idorder not unique. * Only throw exception if idorder specified. * Fixing a couple tests. * Fix another test.
matomo-org · Mar 13, 2019 · def7436 · def7436
1 parent e26f5e7
commit def7436
Show file tree

Hide file tree

Showing 240 changed files with 3,859 additions and 727 deletions.
diff --git a/config/global.ini.php b/config/global.ini.php
@@ -744,10 +744,12 @@
 ; `_paq.push(['setSessionCookieTimeout', timeoutInSeconds=1800])`
 visit_standard_length = 1800
 
-; The window to look back for a previous visit by this current visitor. Defaults to visit_standard_length.
+; The amount of time in the past to match the current visitor to a known visitor via fingerprint. Defaults to visit_standard_length.
 ; If you are looking for higher accuracy of "returning visitors" metrics, you may set this value to 86400 or more.
 ; This is especially useful when you use the Tracking API where tracking Returning Visitors often depends on this setting.
-; The value window_look_back_for_visitor is used only if it is set to greater than visit_standard_length
+; The value window_look_back_for_visitor is used only if it is set to greater than visit_standard_length.
+; Note: visitors with visitor IDs will be matched by visitor ID from any point in time, this is only for recognizing visitors
+; by device fingerprint.
 window_look_back_for_visitor = 0
 
 ; visitors that stay on the website and view only one page will be considered as time on site of 0 second

diff --git a/core/DataAccess/LogAggregator.php b/core/DataAccess/LogAggregator.php
@@ -805,15 +805,15 @@ protected function getActionsMetricFields()
      *                                 clause. These can be aggregate expressions, eg, `SUM(somecol)`.
      * @return \Zend_Db_Statement
      */
-    public function queryConversionsByDimension($dimensions = array(), $where = false, $additionalSelects = array())
+    public function queryConversionsByDimension($dimensions = array(), $where = false, $additionalSelects = array(), $extraFrom = [])
     {
         $dimensions = array_merge(array(self::IDGOAL_FIELD), $dimensions);
         $tableName  = self::LOG_CONVERSION_TABLE;
         $availableMetrics = $this->getConversionsMetricFields();
 
         $select = $this->getSelectStatement($dimensions, $tableName, $additionalSelects, $availableMetrics);
 
-        $from    = array($tableName);
+        $from    = array_merge([$tableName], $extraFrom);
         $where   = $this->getWhereStatement($tableName, self::CONVERSION_DATETIME_FIELD, $where);
         $groupBy = $this->getGroupByStatement($dimensions, $tableName);
         $orderBy = false;

diff --git a/core/Tracker/GoalManager.php b/core/Tracker/GoalManager.php
@@ -12,6 +12,7 @@
 use Piwik\Common;
 use Piwik\Container\StaticContainer;
 use Piwik\Date;
+use Piwik\Exception\InvalidRequestParameterException;
 use Piwik\Piwik;
 use Piwik\Plugin\Dimension\ConversionDimension;
 use Piwik\Plugin\Dimension\VisitDimension;
@@ -741,7 +742,15 @@ protected function insertNewConversion($conversion, $visitInformation, Request $
         $newGoalDebug['idvisitor'] = bin2hex($newGoalDebug['idvisitor']);
         Common::printDebug($newGoalDebug);
 
+        $idorder = $request->getParam('ec_id');
+
         $wasInserted = $this->getModel()->createConversion($conversion);
+        if (!$wasInserted
+            && !empty($idorder)
+        ) {
+            $idSite = $request->getIdSite();
+            throw new InvalidRequestParameterException("Invalid non-unique idsite/idorder combination ($idSite, $idorder), conversion was not inserted.");
+        }
 
         return $wasInserted;
     }

diff --git a/core/Tracker/Model.php b/core/Tracker/Model.php
@@ -357,23 +357,26 @@ public function findVisitor($idSite, $configId, $idVisitor, $fieldsToRead, $shou
         // 		this page view to the wrong visitor, but this is better than creating artificial visits.
         // 2) there is a visitor ID and we trust it (config setting trust_visitors_cookies, OR it was set using &cid= in tracking API),
         //      and in these cases, we force to look up this visitor id
-        $whereCommon = "visit_last_action_time >= ? AND visit_last_action_time <= ? AND idsite = ?";
-        $bindSql = array(
+        $configIdWhere = "visit_last_action_time >= ? AND visit_last_action_time <= ? AND idsite = ?";
+        $configIdbindSql = array(
             $timeLookBack,
             $timeLookAhead,
             $idSite
         );
 
+        $visitorIdWhere = 'idsite = ? AND visit_last_action_time <= ?';
+        $visitorIdbindSql = [$idSite, $timeLookAhead];
+
         if ($shouldMatchOneFieldOnly && $isVisitorIdToLookup) {
-            $visitRow = $this->findVisitorByVisitorId($idVisitor, $select, $from, $whereCommon, $bindSql);
+            $visitRow = $this->findVisitorByVisitorId($idVisitor, $select, $from, $visitorIdWhere, $visitorIdbindSql);
         } elseif ($shouldMatchOneFieldOnly) {
-            $visitRow = $this->findVisitorByConfigId($configId, $select, $from, $whereCommon, $bindSql);
+            $visitRow = $this->findVisitorByConfigId($configId, $select, $from, $configIdWhere, $configIdbindSql);
         } else {
-            $visitRow = $this->findVisitorByVisitorId($idVisitor, $select, $from, $whereCommon, $bindSql);
+            $visitRow = $this->findVisitorByVisitorId($idVisitor, $select, $from, $visitorIdWhere, $visitorIdbindSql);
 
             if (empty($visitRow)) {
-                $whereCommon .= ' AND user_id IS NULL ';
-                $visitRow = $this->findVisitorByConfigId($configId, $select, $from, $whereCommon, $bindSql);
+                $configIdWhere .= ' AND user_id IS NULL ';
+                $visitRow = $this->findVisitorByConfigId($configId, $select, $from, $configIdWhere, $configIdbindSql);
             }
         }
 

diff --git a/core/Tracker/Request.php b/core/Tracker/Request.php
@@ -237,7 +237,7 @@ public function getDaysSinceFirstVisit()
             $cookieFirstVisitTimestamp = $this->getCurrentTimestamp();
         }
 
-        $daysSinceFirstVisit = round(($this->getCurrentTimestamp() - $cookieFirstVisitTimestamp) / 86400, $precision = 0);
+        $daysSinceFirstVisit = floor(($this->getCurrentTimestamp() - $cookieFirstVisitTimestamp) / 86400);
 
         if ($daysSinceFirstVisit < 0) {
             $daysSinceFirstVisit = 0;

diff --git a/core/Tracker/VisitorRecognizer.php b/core/Tracker/VisitorRecognizer.php
@@ -65,6 +65,11 @@ class VisitorRecognizer
      */
     private $eventDispatcher;
 
+    /**
+     * @var array
+     */
+    private $visitRow;
+
     public function __construct($trustCookiesOnly, $visitStandardLength, $lookbackNSecondsCustom, $trackerAlwaysNewVisitor,
                                 Model $model, EventDispatcher $eventDispatcher)
     {
@@ -96,12 +101,13 @@ public function findKnownVisitor($configId, VisitProperties $visitProperties, Re
             Common::printDebug("Visitor doesn't have the piwik cookie...");
         }
 
-        $persistedVisitAttributes = $this->getVisitFieldsPersist();
+        $persistedVisitAttributes = $this->getVisitorFieldsPersist();
 
         $shouldMatchOneFieldOnly  = $this->shouldLookupOneVisitorFieldOnly($isVisitorIdToLookup, $request);
         list($timeLookBack, $timeLookAhead) = $this->getWindowLookupThisVisit($request);
 
         $visitRow = $this->model->findVisitor($idSite, $configId, $idVisitor, $persistedVisitAttributes, $shouldMatchOneFieldOnly, $isVisitorIdToLookup, $timeLookBack, $timeLookAhead);
+        $this->visitRow = $visitRow;
 
         $isNewVisitForced = $request->getParam('new_visit');
         $isNewVisitForced = !empty($isNewVisitForced);
@@ -117,37 +123,15 @@ public function findKnownVisitor($configId, VisitProperties $visitProperties, Re
             && $visitRow
             && count($visitRow) > 0
         ) {
-
-            // These values will be used throughout the request
-            foreach ($persistedVisitAttributes as $field) {
-                $visitProperties->setProperty($field, $visitRow[$field]);
-            }
-
             $visitProperties->setProperty('visit_last_action_time', strtotime($visitRow['visit_last_action_time']));
             $visitProperties->setProperty('visit_first_action_time', strtotime($visitRow['visit_first_action_time']));
-
-            // Custom Variables copied from Visit in potential later conversion
-            if (!empty($numCustomVarsToRead)) {
-                for ($i = 1; $i <= $numCustomVarsToRead; $i++) {
-                    if (isset($visitRow['custom_var_k' . $i])
-                        && strlen($visitRow['custom_var_k' . $i])
-                    ) {
-                        $visitProperties->setProperty('custom_var_k' . $i, $visitRow['custom_var_k' . $i]);
-                    }
-                    if (isset($visitRow['custom_var_v' . $i])
-                        && strlen($visitRow['custom_var_v' . $i])
-                    ) {
-                        $visitProperties->setProperty('custom_var_v' . $i, $visitRow['custom_var_v' . $i]);
-                    }
-                }
-            }
+            $visitProperties->setProperty('idvisitor', $visitRow['idvisitor']);
+            $visitProperties->setProperty('user_id', $visitRow['user_id']);
 
             Common::printDebug("The visitor is known (idvisitor = " . bin2hex($visitProperties->getProperty('idvisitor')) . ",
                     config_id = " . bin2hex($configId) . ",
-                    idvisit = {$visitProperties->getProperty('idvisit')},
                     last action = " . date("r", $visitProperties->getProperty('visit_last_action_time')) . ",
-                    first action = " . date("r", $visitProperties->getProperty('visit_first_action_time')) . ",
-                    visit_goal_buyer' = " . $visitProperties->getProperty('visit_goal_buyer') . ")");
+                    first action = " . date("r", $visitProperties->getProperty('visit_first_action_time')) . ")");
 
             return true;
         } else {
@@ -157,6 +141,22 @@ public function findKnownVisitor($configId, VisitProperties $visitProperties, Re
         }
     }
 
+    public function updateVisitPropertiesFromLastVisitRow(VisitProperties $visitProperties)
+    {
+        // These values will be used throughout the request
+        foreach ($this->getVisitorFieldsPersist() as $field) {
+            if ($field == 'visit_last_action_time' || $field == 'visit_first_action_time') {
+                continue;
+            }
+
+            $visitProperties->setProperty($field, $this->visitRow[$field]);
+        }
+
+        Common::printDebug("The visit is part of an existing visit (
+            idvisit = {$visitProperties->getProperty('idvisit')},
+            visit_goal_buyer' = " . $visitProperties->getProperty('visit_goal_buyer') . ")");
+    }
+
     protected function shouldLookupOneVisitorFieldOnly($isVisitorIdToLookup, Request $request)
     {
         $isForcedUserIdMustMatch = (false !== $request->getForcedUserId());
@@ -212,7 +212,7 @@ protected function getWindowLookupThisVisit(Request $request)
     /**
      * @return array
      */
-    private function getVisitFieldsPersist()
+    private function getVisitorFieldsPersist()
     {
         if (is_null($this->visitFieldsToSelect)) {
             $fields = array(

diff --git a/core/ViewDataTable/Request.php b/core/ViewDataTable/Request.php
@@ -60,7 +60,7 @@ public function getRequestArray()
             'format' => 'original'
         );
 
-        $toSetEventually = array(
+        $toSetEventually = array_merge(array(
             'filter_limit',
             'keep_totals_row',
             'keep_summary_row',
@@ -76,7 +76,7 @@ public function getRequestArray()
             'pivotBy',
             'pivotByColumn',
             'pivotByColumnLimit'
-        );
+        ), $this->requestConfig->getExtraParametersToSet());
 
         foreach ($toSetEventually as $varToSet) {
             $value = $this->getDefaultOrCurrent($varToSet);

diff --git a/core/ViewDataTable/RequestConfig.php b/core/ViewDataTable/RequestConfig.php
@@ -345,4 +345,15 @@ public function getApiMethodToRequest()
 
         return $method;
     }
+
+    /**
+     * Override this method if you want to add custom request parameters to the API request based on ViewDataTable
+     * parameters. Return in the result the list of extra parameters.
+     *
+     * @return array eg, `['mycustomparam']`
+     */
+    public function getExtraParametersToSet()
+    {
+        return [];
+    }
 }
diff --git a/...ns/API/tests/System/expected/test_AutoSuggestAPITest__Live.getLastVisitsDetails_range.xml b/...ns/API/tests/System/expected/test_AutoSuggestAPITest__Live.getLastVisitsDetails_range.xml
@@ -436,7 +436,7 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>0</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
 		<visitDuration>0</visitDuration>
 		<visitDurationPretty>0s</visitDurationPretty>
@@ -798,10 +798,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -1184,7 +1184,7 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>0</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
 		<visitDuration>0</visitDuration>
 		<visitDurationPretty>0s</visitDurationPretty>
@@ -1546,10 +1546,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -1932,7 +1932,7 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>0</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
 		<visitDuration>0</visitDuration>
 		<visitDurationPretty>0s</visitDurationPretty>
@@ -2497,10 +2497,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -2648,10 +2648,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -3261,7 +3261,7 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>0</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
 		<visitDuration>0</visitDuration>
 		<visitDurationPretty>0s</visitDurationPretty>
@@ -3412,7 +3412,7 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>0</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
 		<visitDuration>0</visitDuration>
 		<visitDurationPretty>0s</visitDurationPretty>
@@ -4399,10 +4399,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -4550,10 +4550,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -4701,10 +4701,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>
@@ -4852,10 +4852,10 @@
 		<visitCount>5</visitCount>
 		<visitEcommerceStatus>none</visitEcommerceStatus>
 		<visitEcommerceStatusIcon />
-		<daysSinceFirstVisit>101</daysSinceFirstVisit>
+		<daysSinceFirstVisit>100</daysSinceFirstVisit>
 		<daysSinceLastEcommerceOrder>51</daysSinceLastEcommerceOrder>
-		<visitDuration>0</visitDuration>
-		<visitDurationPretty>0s</visitDurationPretty>
+		<visitDuration>1</visitDuration>
+		<visitDurationPretty>1s</visitDurationPretty>
 		<searches>0</searches>
 		<actions>1</actions>
 		<interactions>1</interactions>

diff --git a/...sts/System/expected/test_AutoSuggestAPITest_actionServerHour__VisitsSummary.get_range.xml b/...sts/System/expected/test_AutoSuggestAPITest_actionServerHour__VisitsSummary.get_range.xml
@@ -4,7 +4,7 @@
 	<nb_actions>18</nb_actions>
 	<nb_visits_converted>18</nb_visits_converted>
 	<bounce_count>18</bounce_count>
-	<sum_visit_length>0</sum_visit_length>
+	<sum_visit_length>8</sum_visit_length>
 	<max_actions>1</max_actions>
 	<bounce_rate>100%</bounce_rate>
 	<nb_actions_per_visit>1</nb_actions_per_visit>