Fix archiving too many segments that aren't needed and showing 0 conversions for new visits/returning visitors (#18255)

7 files changed, 207 insertions, 21 deletions

diff --git a/core/ArchiveProcessor.php b/core/ArchiveProcessor.php
index d695e2bdb1..09bfeb2428 100644
--- a/core/ArchiveProcessor.php
+++ b/core/ArchiveProcessor.php
@@ -673,7 +673,13 @@ class ArchiveProcessor
 
         $idSites = [$params->getSite()->getId()];
 
-        $newSegment = Segment::combine($params->getSegment()->getString(), SegmentExpression::AND_DELIMITER, $segment);
+        // important to use the original segment string when combining. As the API itself would combine the original string.
+        // this prevents a bug where the API would use the segment
+        // userId!@%2540matomo.org;userId!=hello%2540matomo.org;visitorType==new
+        // vs here we would use
+        // userId!@%40matomo.org;userId!=hello%40matomo.org;visitorType==new
+        // thus these would result in different segment hashes and therefore the reports would either show 0 or archive the data twice
+        $newSegment = Segment::combine($params->getSegment()->getOriginalString(), SegmentExpression::AND_DELIMITER, $segment);
         if ($newSegment === $segment && $params->getRequestedPlugin() === $plugin) { // being processed now
             return;
         }
diff --git a/core/Updates/4.6.0-b4.php b/core/Updates/4.6.0-b4.php
new file mode 100644
index 0000000000..72b2ce0c6a
--- /dev/null
+++ b/core/Updates/4.6.0-b4.php
@@ -0,0 +1,115 @@
+<?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\Updates;
+
+use Piwik\ArchiveProcessor\Rules;
+use Piwik\Common;
+use Piwik\DataAccess\ArchiveTableCreator;
+use Piwik\Db;
+use Piwik\Plugins\SitesManager\Model;
+use Piwik\Plugins\VisitFrequency\API as VisitFrequencyAPI;
+use Piwik\Segment;
+use Piwik\Segment\SegmentExpression;
+use Piwik\Updater;
+use Piwik\Updates as PiwikUpdates;
+use Piwik\Updater\Migration;
+use Piwik\Updater\Migration\Factory as MigrationFactory;
+
+/**
+ * Update for version 4.6.0-b2.
+ */
+class Updates_4_6_0_b4 extends PiwikUpdates
+{
+    /**
+     * @var MigrationFactory
+     */
+    private $migration;
+
+    public function __construct(MigrationFactory $factory)
+    {
+        $this->migration = $factory;
+    }
+
+    /**
+     * @param Updater $updater
+     * @return Migration\Db[]
+     */
+    public function getMigrations(Updater $updater)
+    {
+        $migrations = [];
+        $sites = new Model();
+        $idSites = $sites->getSitesId();
+
+        $doneFlagsToMigrate = [];
+        foreach ($idSites as $idSite) {
+            $segmentStrings = Rules::getSegmentsToProcess([$idSite]);
+
+            foreach ($segmentStrings as $segmentString) {
+                $segment = new Segment($segmentString, [$idSite]);
+                if ($segment->getOriginalString() === $segment->getString()) {
+                    continue;
+                }
+
+                $segmentsToAppend = [VisitFrequencyAPI::NEW_VISITOR_SEGMENT, VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT];
+                foreach ($segmentsToAppend as $segmentToAppend) {
+                    // we need to migrate the existing archive
+                    $oldSegmentString = Segment::combine($segment->getString(), SegmentExpression::AND_DELIMITER, $segmentToAppend);
+                    $newSegmentString = Segment::combine($segment->getOriginalString(), SegmentExpression::AND_DELIMITER, $segmentToAppend);
+                    $oldSegmentHash = Segment::getSegmentHash($oldSegmentString);
+                    $newSegmentHash = Segment::getSegmentHash($newSegmentString);
+
+                    if ($oldSegmentHash === $newSegmentHash) {
+                        continue;
+                    }
+
+                    $doneFlagsToMigrate['done' . $oldSegmentHash . '.Goals'] = 'done' . $newSegmentHash . '.Goals';
+                    $doneFlagsToMigrate['done' . $oldSegmentHash . '.VisitsSummary'] = 'done' . $newSegmentHash . '.VisitsSummary';
+                    $doneFlagsToMigrate['done' . $oldSegmentHash . '.UserCountry'] =  'done' . $newSegmentHash . '.UserCountry';
+                }
+            }
+        }
+
+        if (!empty($doneFlagsToMigrate)) {
+            foreach (ArchiveTableCreator::getTablesArchivesInstalled() as $table) {
+                if (strpos($table, 'numeric') === false) {
+                    continue;
+                }
+
+                $sqlPlaceholders = Common::getSqlStringFieldsArray($doneFlagsToMigrate);
+                $bind = array_keys($doneFlagsToMigrate);
+
+                $selectSql = sprintf('SELECT 1 FROM %s where `name` in (%s) LIMIT 1', $table, $sqlPlaceholders);
+                $archiveTableHasDoneFlags = Db::fetchOne($selectSql, $bind);
+                if (!$archiveTableHasDoneFlags) {
+                    continue;
+                }
+
+                $sql = 'update ' . $table . ' set `name` = (case';
+                foreach ($doneFlagsToMigrate as $oldDoneFlag => $newDoneFlag) {
+                    $sql .= " when `name`   = '$oldDoneFlag' then '$newDoneFlag' ";
+                }
+                $sql .= ' else `name` end) where `name` in (' . $sqlPlaceholders . ')';
+
+                Db::query($sql, $bind);
+                $migrations[] = $this->migration->db->boundSql($sql, $bind, [Migration\Db\Sql::ERROR_CODE_DUPLICATE_ENTRY]);
+            }
+        }
+
+        return $migrations;
+    }
+
+    /**
+     * @param Updater $updater
+     */
+    public function doUpdate(Updater $updater)
+    {
+        $updater->executeMigrations(__FILE__, $this->getMigrations($updater));
+    }
+}
diff --git a/core/Version.php b/core/Version.php
index 5d385c55d7..4fe41a32a5 100644
--- a/core/Version.php
+++ b/core/Version.php
@@ -20,7 +20,7 @@ final class Version
      * The current Matomo version.
      * @var string
      */
-    const VERSION = '4.6.0-b3';
+    const VERSION = '4.6.0-b4';
 
     const MAJOR_VERSION = 4;
 
diff --git a/plugins/Goals/API.php b/plugins/Goals/API.php
index ad0bd4121f..4938b88856 100644
--- a/plugins/Goals/API.php
+++ b/plugins/Goals/API.php
@@ -476,7 +476,16 @@ class API extends \Piwik\Plugin\API
         );
 
         foreach ($segments as $appendToMetricName => $predefinedSegment) {
-            $segmentToUse = $this->appendSegment($predefinedSegment, $segment);
+            if (!empty($predefinedSegment)) {
+                // we are disabling the archiving of these segments as the archiver archives them already using
+                // archiveProcessDependend logic. Otherwise we would eg archive reports that we don't need:
+                // userid=5;visitorType%3D%3Dnew;visitorType%3D%3Dreturning%2CvisitorType%3D%3DreturningCustomer
+                // userid=5;visitorType%3D%3Dreturning%2CvisitorType%3D%3DreturningCustomer;visitorType%3D%3Dnew;
+                // it would also archive dependends for these segments that we already combined here and then combine
+                // segments again when archiving dependends
+                Archiver::$ARCHIVE_DEPENDENT = false;
+            }
+            $segmentToUse = $this->appendSegment($segment, $predefinedSegment);
 
             /** @var DataTable|DataTable\Map $tableSegmented */
             $tableSegmented = Request::processRequest('Goals.getMetrics', array(
@@ -489,7 +498,7 @@ class API extends \Piwik\Plugin\API
                 'showAllGoalSpecificMetrics' => $showAllGoalSpecificMetrics,
                 'format_metrics' => Common::getRequestVar('format_metrics', 'bc'),
             ), $default = []);
-
+            Archiver::$ARCHIVE_DEPENDENT = true;
             $tableSegmented->filter('Piwik\Plugins\Goals\DataTable\Filter\AppendNameToColumnNames',
                                     array($appendToMetricName));
 
diff --git a/plugins/Goals/Archiver.php b/plugins/Goals/Archiver.php
index dc32c68538..195e63bb66 100644
--- a/plugins/Goals/Archiver.php
+++ b/plugins/Goals/Archiver.php
@@ -17,6 +17,8 @@ use Piwik\DataArray;
 use Piwik\DataTable;
 use Piwik\Metrics;
 use Piwik\Plugin\Manager;
+use Piwik\Segment;
+use Piwik\Segment\SegmentExpression;
 use Piwik\Tracker\GoalManager;
 use Piwik\Plugins\VisitFrequency\API as VisitFrequencyAPI;
 
@@ -39,6 +41,8 @@ class Archiver extends \Piwik\Plugin\Archiver
     const VISITS_COUNT_FIELD = 'visitor_count_visits';
     const SECONDS_SINCE_FIRST_VISIT_FIELD = 'visitor_seconds_since_first';
 
+    public static $ARCHIVE_DEPENDENT = true;
+
     /**
      * This array stores the ranges to use when displaying the 'visits to conversion' report
      */
@@ -118,8 +122,10 @@ class Archiver extends \Piwik\Plugin\Archiver
             $this->aggregateEcommerceItems();
         }
 
-        $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::NEW_VISITOR_SEGMENT);
-        $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT);
+        if (self::$ARCHIVE_DEPENDENT) {
+            $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::NEW_VISITOR_SEGMENT);
+            $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT);
+        }
     }
 
     protected function aggregateGeneralGoalMetrics()
@@ -502,7 +508,9 @@ class Archiver extends \Piwik\Plugin\Archiver
                 $columnsToRenameAfterAggregation = null,
                 $countRowsRecursive = array());
 
-        $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::NEW_VISITOR_SEGMENT);
-        $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT);
+        if (self::$ARCHIVE_DEPENDENT) {
+            $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::NEW_VISITOR_SEGMENT);
+            $this->getProcessor()->processDependentArchive('Goals', VisitFrequencyAPI::RETURNING_VISITOR_SEGMENT);
+        }
     }
 }
diff --git a/plugins/Goals/tests/System/ProcessDependentArchiveTest.php b/plugins/Goals/tests/System/ProcessDependentArchiveTest.php
new file mode 100644
index 0000000000..f700e20b5a
--- /dev/null
+++ b/plugins/Goals/tests/System/ProcessDependentArchiveTest.php
@@ -0,0 +1,58 @@
+<?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\Goals\tests\System;
+
+use Piwik\Common;
+use Piwik\Db;
+use Piwik\Plugins\Goals\API;
+use Piwik\Tests\Fixtures\ThreeGoalsOnePageview;
+use Piwik\Tests\Framework\TestCase\SystemTestCase;
+
+/**
+ * @property ThreeGoalsOnePageview $fixture
+ */
+class ProcessDependentArchiveTest extends SystemTestCase
+{
+    /**
+     * @var ThreeGoalsOnePageview
+     */
+    public static $fixture = null; // initialized below class definition
+
+    private $archiveTable = 'archive_numeric_2009_01';
+    private $requestRange = '2009-01-02,2009-01-26';
+
+    public function tearDown(): void
+    {
+        Db::query('DELETE from ' . Common::prefixTable($this->archiveTable) . ' WHERE period = 5');
+        parent::tearDown();
+    }
+
+    public function test_numArchivesCreated()
+    {
+        API::getInstance()->get(self::$fixture->idSite, 'range', $this->requestRange);
+        $this->assertNumRangeArchives(6);
+    }
+
+    public function test_numArchivesCreatedWithSegment()
+    {
+        API::getInstance()->get(self::$fixture->idSite, 'range', $this->requestRange,'userId!@%2540matomo.org;userId!=hello%2540matomo.org');
+        $this->assertNumRangeArchives(6);
+    }
+
+    private function assertNumRangeArchives($expectedArchives)
+    {
+        $archives = Db::fetchAll('SELECT `name` from ' . Common::prefixTable($this->archiveTable) . ' WHERE period = 5 and `name` like "done%"');
+        $numArchives = count($archives);
+        $message = sprintf('Expected archives: %s, got: %s. These were the archives %s', $expectedArchives, $numArchives, json_encode($archives));
+        $this->assertEquals($expectedArchives, $numArchives, $message);
+    }
+
+}
+ProcessDependentArchiveTest::$fixture = new ThreeGoalsOnePageview();
diff --git a/plugins/VisitFrequency/API.php b/plugins/VisitFrequency/API.php
index 5af6927cb0..a8119636a2 100644
--- a/plugins/VisitFrequency/API.php
+++ b/plugins/VisitFrequency/API.php
@@ -14,6 +14,7 @@ use Piwik\Period;
 use Piwik\Piwik;
 use Piwik\Plugins\API\DataTable\MergeDataTables;
 use Piwik\Plugins\VisitsSummary\API as APIVisitsSummary;
+use Piwik\Segment;
 use Piwik\Segment\SegmentExpression;
 
 /**
@@ -60,7 +61,7 @@ class API extends \Piwik\Plugin\API
         }
 
         foreach ($visitTypes as $columnSuffix => $visitorTypeSegment) {
-            $modifiedSegment = $this->appendVisitorTypeSegment($segment, $visitorTypeSegment);
+            $modifiedSegment = Segment::combine($segment, SegmentExpression::AND_DELIMITER, $visitorTypeSegment);
 
             $columnsForVisitType = empty($columns) ? array() : $this->unprefixColumns($columns, $columnSuffix);
 
@@ -95,17 +96,6 @@ class API extends \Piwik\Plugin\API
         return $resultSet;
     }
 
-    protected function appendVisitorTypeSegment($segment, $toAppend)
-    {
-        if (empty($segment)) {
-            $segment = '';
-        } else {
-            $segment .= urlencode(SegmentExpression::AND_DELIMITER);
-        }
-        $segment .= $toAppend;
-        return $segment;
-    }
-
     protected function unprefixColumns(array $requestedColumns, $suffix)
     {
         $result = array();
@@ -125,4 +115,4 @@ class API extends \Piwik\Plugin\API
         }
         $table->filter('ReplaceColumnNames', array($rename));
     }
-}
\ No newline at end of file
+}