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

3 files changed, 123 insertions, 2 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;