Tweak behaviour of orphaned segment archive purge (#14857)

4 files changed, 274 insertions, 153 deletions

diff --git a/plugins/CoreAdminHome/Tasks.php b/plugins/CoreAdminHome/Tasks.php
index ef9b0bd215..d94979c9c3 100644
--- a/plugins/CoreAdminHome/Tasks.php
+++ b/plugins/CoreAdminHome/Tasks.php
@@ -23,9 +23,9 @@ use Piwik\Piwik;
 use Piwik\Plugins\CoreAdminHome\Emails\JsTrackingCodeMissingEmail;
 use Piwik\Plugins\CoreAdminHome\Emails\TrackingFailuresEmail;
 use Piwik\Plugins\CoreAdminHome\Tasks\ArchivesToPurgeDistributedList;
+use Piwik\Plugins\SegmentEditor\Model;
 use Piwik\Plugins\SitesManager\SitesManager;
 use Piwik\Scheduler\Schedule\SpecificTime;
-use Piwik\Segment;
 use Piwik\Settings\Storage\Backend\MeasurableSettingsTable;
 use Piwik\Tracker\Failures;
 use Piwik\Site;
@@ -275,7 +275,10 @@ class Tasks extends \Piwik\Plugin\Tasks
      */
     public function purgeOrphanedArchives()
     {
-        $segmentHashesByIdSite = $this->getSegmentHashesByIdSite();
+        $eightDaysAgo = Date::factory('now')->subDay(8);
+        $model = new Model();
+        $deletedSegments = $model->getSegmentsDeletedSince($eightDaysAgo);
+
         $archiveTables = ArchiveTableCreator::getTablesArchivesInstalled('numeric');
 
         $datesPurged = array();
@@ -286,35 +289,15 @@ class Tasks extends \Piwik\Plugin\Tasks
             $dateObj = Date::factory("$year-$month-15");
 
             $this->archivePurger->purgeDeletedSiteArchives($dateObj);
-            $this->archivePurger->purgeDeletedSegmentArchives($dateObj, $segmentHashesByIdSite);
+            if (count($deletedSegments)) {
+                $this->archivePurger->purgeDeletedSegmentArchives($dateObj, $deletedSegments);
+            }
 
             $datesPurged[$date] = true;
         }
     }
 
     /**
-     * Get a list of all segment hashes that currently exist, indexed by idSite.
-     * @return array
-     */
-    public function getSegmentHashesByIdSite()
-    {
-        //Get a list of hashes of all segments that exist now
-        $sql = "SELECT DISTINCT definition, enable_only_idsite FROM " . Common::prefixTable('segment')
-            . " WHERE deleted = 0";
-        $rows = Db::fetchAll($sql);
-        $segmentHashes = array();
-        foreach ($rows as $row) {
-            $idSite = (int)$row['enable_only_idsite'];
-            if (! isset($segmentHashes[$idSite])) {
-                $segmentHashes[$idSite] = array();
-            }
-            $segmentHashes[$idSite][] = Segment::getSegmentHash($row['definition']);
-        }
-
-        return $segmentHashes;
-    }
-
-    /**
      * we should only purge outdated & custom range archives if we know cron archiving has just run,
      * or if browser triggered archiving is enabled. if cron archiving has run, then we know the latest
      * archives are in the database, and we can remove temporary ones. if browser triggered archiving is
diff --git a/plugins/CoreAdminHome/tests/Integration/TasksTest.php b/plugins/CoreAdminHome/tests/Integration/TasksTest.php
index 830da7ef97..a23d368de4 100644
--- a/plugins/CoreAdminHome/tests/Integration/TasksTest.php
+++ b/plugins/CoreAdminHome/tests/Integration/TasksTest.php
@@ -219,133 +219,6 @@ class TasksTest extends IntegrationTestCase
         $this->assertEquals(2, $mail->getNumFailures());
     }
 
-    public function test_getSegmentHashesByIdSite_emptyWhenNoSegments()
-    {
-        $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite();
-        $this->assertEquals(array(), $segmentsByIdSite);
-    }
-
-    public function test_getSegmentHashesByIdSite_allWebsiteAndSiteSpecificSegments()
-    {
-        $model = new Model();
-        $model->createSegment(array(
-            'name' => 'Test Segment 1',
-            'definition' => 'continentCode==eur',
-            'enable_only_idsite' => 0,
-            'deleted' => 0
-        ));
-        $model->createSegment(array(
-            'name' => 'Test Segment 2',
-            'definition' => 'countryCode==nz',
-            'enable_only_idsite' => 0,
-            'deleted' => 0
-        ));
-        $model->createSegment(array(
-            'name' => 'Test Segment 3',
-            'definition' => 'countryCode==au',
-            'enable_only_idsite' => 2,
-            'deleted' => 0
-        ));
-
-        $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite();
-        $expected = array(
-            0 => array('be90051048558489e1d62f4245a6dc65', 'b92fbb3009b32cf632965802de2fb760'),
-            2 => array('cffd4336c22c6782211f853495076b1a')
-        );
-        $this->assertEquals($expected, $segmentsByIdSite);
-    }
-
-    public function test_getSegmentHashesByIdSite_deletedSegment()
-    {
-        $model = new Model();
-        $model->createSegment(array(
-            'name' => 'Test Segment 1',
-            'definition' => 'continentCode==eur',
-            'enable_only_idsite' => 0,
-            'deleted' => 0
-        ));
-        $model->createSegment(array(
-            'name' => 'Test Segment 2',
-            'definition' => 'countryCode==nz',
-            'enable_only_idsite' => 0,
-            'deleted' => 1
-        ));
-        $model->createSegment(array(
-            'name' => 'Test Segment 3',
-            'definition' => 'countryCode==au',
-            'enable_only_idsite' => 2,
-            'deleted' => 0
-        ));
-
-        $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite();
-        $expected = array(
-            0 => array('be90051048558489e1d62f4245a6dc65'),
-            2 => array('cffd4336c22c6782211f853495076b1a')
-        );
-        $this->assertEquals($expected, $segmentsByIdSite);
-    }
-
-    public function test_getSegmentHashesByIdSite_invalidSegment()
-    {
-        $model = new Model();
-        $model->createSegment(array(
-            'name' => 'Test Segment 4',
-            'definition' => 'countryCode=nz',   //The single "=" is invalid - we should generate a hash anyway
-            'enable_only_idsite' => 0,
-            'deleted' => 0
-        ));
-        $model->createSegment(array(
-            'name' => 'Test Segment 5',
-            'definition' => 'countryCode==au',
-            'enable_only_idsite' => 0,
-            'deleted' => 0
-        ));
-
-        $expected = array(
-            0 => array('5ffe7e116fae7576c047b1fb811584a5', 'cffd4336c22c6782211f853495076b1a'),
-        );
-
-        $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite();
-        $this->assertEquals($expected, $segmentsByIdSite);
-    }
-
-    public function test_getSegmentHashesByIdSite_siteSpecificCustomDimension()
-    {
-        // Insert a custom dimension for idsite = 1
-        $configuration = new Configuration();
-        $configuration->configureNewDimension(
-            1, 
-            'mydimension', 
-            CustomDimensions::SCOPE_VISIT, 
-            1, 
-            1, 
-            array(), 
-            true
-        );
-
-        $model = new Model();
-        $model->createSegment(array(
-            'name' => 'Test Segment 6',
-            'definition' => 'mydimension==red',
-            'enable_only_idsite' => 1,
-            'deleted' => 0
-        ));
-        $model->createSegment(array(
-            'name' => 'Test Segment 7',
-            'definition' => 'countryCode==au',
-            'enable_only_idsite' => 2,
-            'deleted' => 0
-        ));
-
-        $expected = array(
-            1 => array('240d2a84a309debd26bdbaa8eb3d363c'),
-            2 => array('cffd4336c22c6782211f853495076b1a')
-        );
-
-        $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite();
-        $this->assertEquals($expected, $segmentsByIdSite);
-    }
-
     /**
      * @param Date[] $dates
      */
diff --git a/plugins/SegmentEditor/Model.php b/plugins/SegmentEditor/Model.php
index 0025c7c15c..c505f7a4f2 100644
--- a/plugins/SegmentEditor/Model.php
+++ b/plugins/SegmentEditor/Model.php
@@ -129,6 +129,87 @@ class Model
         return $segment;
     }
 
+    /**
+     * Gets a list of segments that have been deleted in the last week and therefore may have orphaned archives.
+     * @param Date $date Segments deleted on or after this date will be returned.
+     * @return array of segments. The segments are only populated with the fields needed for archive invalidation
+     * (e.g. definition, enable_only_idsite).
+     * @throws \Exception
+     */
+    public function getSegmentsDeletedSince(Date $date)
+    {
+        $dateStr = $date->getDatetime();
+        $sql = "SELECT DISTINCT definition, enable_only_idsite FROM " . Common::prefixTable('segment')
+            . " WHERE deleted = 1 AND ts_last_edit >= ?";
+        $deletedSegments = Db::fetchAll($sql, array($dateStr));
+
+        if (empty($deletedSegments)) {
+            return array();
+        }
+
+        $existingSegments = $this->getExistingSegmentsLike($deletedSegments);
+
+        foreach ($deletedSegments as $i => $deleted) {
+            $deletedSegments[$i]['idsites_to_preserve'] = array();
+            foreach ($existingSegments as $existing) {
+                if ($existing['definition'] != $deleted['definition'] &&
+                    $existing['definition'] != urlencode($deleted['definition']) &&
+                    $existing['definition'] != urldecode($deleted['definition'])
+                ) {
+                    continue;
+                }
+
+                if (
+                    $existing['enable_only_idsite'] == $deleted['enable_only_idsite']
+                    || $existing['enable_only_idsite'] == 0
+                ) {
+                    // There is an identical segment (for either the specific site or for all sites) that is active
+                    // The archives for this segment will therefore still be needed
+                    unset($deletedSegments[$i]);
+                    break;
+                } elseif ($deleted['enable_only_idsite'] == 0) {
+                    // It is an all-sites segment that got deleted, but there is a single-site segment that is active
+                    // Need to make sure we don't erase the segment's archives for that particular site
+                    $deletedSegments[$i]['idsites_to_preserve'][] = $existing['enable_only_idsite'];
+                }
+            }
+        }
+
+        return $deletedSegments;
+    }
+
+    private function getExistingSegmentsLike(array $segments)
+    {
+        if (empty($segments)) {
+            return array();
+        }
+
+        $whereClauses = array();
+        $bind = array();
+        $definitionWhereClauseTemplate = '(definition = ? OR definition = ? OR definition = ?)';
+        foreach ($segments as $segment) {
+            // Sometimes they are stored encoded and sometimes they aren't
+            $bind[] = $segment['definition'];
+            $bind[] = urlencode($segment['definition']);
+            $bind[] = urldecode($segment['definition']);
+
+            if ($segment['enable_only_idsite'] == 0) {
+                // They deleted an all-sites segment, but there is a single-site segment with same definition?
+                // Need to handle this carefully so that the archives for the single-site segment are preserved
+                $whereClauses[] = "$definitionWhereClauseTemplate";
+            } else {
+                $whereClauses[] = "($definitionWhereClauseTemplate AND (enable_only_idsite = ? OR enable_only_idsite = 0))";
+                $bind[] = $segment['enable_only_idsite'];
+            }
+        }
+        $whereClauses = implode(' OR ', $whereClauses);
+
+        // Check for any non-deleted segments with the same definition
+        $sql = "SELECT DISTINCT definition, enable_only_idsite FROM " . Common::prefixTable('segment')
+            . " WHERE deleted = 0 AND (" . $whereClauses . ")";
+        return Db::fetchAll($sql, $bind);
+    }
+
     public function deleteSegment($idSegment)
     {
         $fieldsToSet = array(
diff --git a/plugins/SegmentEditor/tests/Integration/ModelTest.php b/plugins/SegmentEditor/tests/Integration/ModelTest.php
index 92034f3efc..f309221c8f 100644
--- a/plugins/SegmentEditor/tests/Integration/ModelTest.php
+++ b/plugins/SegmentEditor/tests/Integration/ModelTest.php
@@ -25,6 +25,8 @@ class ModelTest extends IntegrationTestCase
 
     private $idSegment3;
 
+    private $idSegment4;
+
     public function setUp()
     {
         parent::setUp();
@@ -55,7 +57,11 @@ class ModelTest extends IntegrationTestCase
     {
         parent::tearDown();
         // Force a hard delete of segment
-        $idsToDelete = $this->idSegment1 . ', ' . $this->idSegment2 . ', ' . $this->idSegment3;
+        $idsToDelete = array($this->idSegment1, $this->idSegment2, $this->idSegment3);
+        if ($this->idSegment4) {
+            $idsToDelete[] = $this->idSegment4;
+        }
+        $idsToDelete = implode(',', $idsToDelete);
         Db::query(
             "DELETE FROM " . Common::prefixTable('segment') . " WHERE idsegment IN ($idsToDelete)"
         );
@@ -146,6 +152,184 @@ class ModelTest extends IntegrationTestCase
         $this->assertEmpty($segment);
     }
 
+    public function test_getSegmentsDeletedSince_noDeletedSegments()
+    {
+        $date = Date::factory('now');
+        $segments = $this->model->getSegmentsDeletedSince($date);
+        $this->assertEmpty($segments);
+    }
+
+    public function test_getSegmentsDeletedSince_oneDeletedSegment()
+    {
+        $this->model->deleteSegment($this->idSegment3);
+
+        $date = Date::factory('now')->subDay(1);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        $this->assertCount(1, $segments);
+        $this->assertEquals('country==Hobbiton', $segments[0]['definition']);
+    }
+
+    public function test_getSegmentsDeletedSince_segmentDeletedTooLongAgo()
+    {
+        // Manually delete it to set timestamp 9 days in past
+        $deletedAt = Date::factory('now')->subDay(9)->toString('Y-m-d H:i:s');
+        $this->model->updateSegment($this->idSegment1, array(
+            'deleted' => 1,
+            'ts_last_edit' => $deletedAt
+        ));
+
+        // The segment deleted above should not be included as it was more than 8 days ago
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        $this->assertEmpty($segments);
+    }
+
+    public function test_getSegmentsDeletedSince_duplicateSegment()
+    {
+        // Turn segment1 into a duplicate of segment2, except it's also deleted
+        $this->model->updateSegment($this->idSegment1, array(
+            'definition' => 'country==Genovia',
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        $this->assertEmpty($segments);
+    }
+
+    public function test_getSegmentsDeletedSince_duplicateSegmentDifferentIdSite()
+    {
+        // Turn segment2 into a duplicate of segment3, except for a different idsite and also deleted
+        $this->model->updateSegment($this->idSegment2, array(
+            'definition' => 'country==Hobbiton',
+            'enable_only_idsite' => 2,
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        $this->assertCount(1, $segments);
+        $this->assertEquals('country==Hobbiton', $segments[0]['definition']);
+        $this->assertEquals(2, $segments[0]['enable_only_idsite']);
+    }
+
+    public function test_getSegmentsDeletedSince_duplicateSegmentAllSitesAndSingleSite()
+    {
+        // Turn segment2 into a duplicate of segment3, except for all sites and also deleted
+        $this->model->updateSegment($this->idSegment2, array(
+            'definition' => 'country==Hobbiton',
+            'enable_only_idsite' => 0,
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        $this->assertCount(1, $segments);
+        $this->assertEquals('country==Hobbiton', $segments[0]['definition']);
+        $this->assertEquals(0, $segments[0]['enable_only_idsite']);
+        $this->assertEquals(array(1), $segments[0]['idsites_to_preserve']);
+    }
+
+    public function test_getSegmentsDeletedSince_duplicateSegmentSingleSiteAndAllSites()
+    {
+        // Turn segment3 into a duplicate of segment1, except for a single site and deleted
+        $this->model->updateSegment($this->idSegment3, array(
+            'definition' => 'country==Narnia',
+            'enable_only_idsite' => 1,
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        // There is still a live segment for all sites, so the deleted site-specific one is ignored
+        $this->assertEmpty($segments);
+    }
+
+    public function test_getSegmentsDeletedSince_ExistingSiteSpecificAndAllSitesMatch()
+    {
+        // A deleted all-sites segment, with both an all-sites and a site-specific segment still present
+        $this->model->updateSegment($this->idSegment1, array(
+            'definition' => 'actions >= 1',
+            'enable_only_idsite' => 0,
+            'deleted' => 0,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+        $this->model->updateSegment($this->idSegment2, array(
+            'definition' => 'actions >= 1',
+            'enable_only_idsite' => 0,
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+        $this->model->updateSegment($this->idSegment3, array(
+            'definition' => 'actions >= 1',
+            'enable_only_idsite' => 3,
+            'deleted' => 0,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+        $this->idSegment4 = $this->model->createSegment(array(
+            'definition' => 'actions >= 1',
+            'enable_only_idsite' => 1,
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        $this->assertEmpty($segments);
+    }
+
+    public function test_getSegmentsDeletedSince_urlDecodedVersionOfSegment()
+    {
+        // Turn segment2 into a duplicate of segment3, except a urlencoded version
+        $this->model->updateSegment($this->idSegment2, array(
+            'definition' => 'country%3D%3DHobbiton',
+            'enable_only_idsite' => 1,
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        // The two encoded and decoded version of the segments should be treated as duplicates
+        // This means there segment has a non-deleted version so it's not returned
+        $this->assertEmpty($segments);
+    }
+
+    public function test_getSegmentsDeletedSince_urlEncodedVersionOfSegment()
+    {
+        // segment1 => url decoded version, deleted
+        $this->model->updateSegment($this->idSegment1, array(
+            'definition' => 'country==Narnia',
+            'deleted' => 1,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+        // segment2 => url encoded version, not deleted
+        $this->model->updateSegment($this->idSegment2, array(
+            'definition' => 'country%3D%3DNarnia',
+            'deleted' => 0,
+            'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s')
+        ));
+
+        $date = Date::factory('now')->subDay(8);
+        $segments = $this->model->getSegmentsDeletedSince($date);
+
+        // The two encoded and decoded version of the segments should be treated as duplicates
+        // This means there segment has a non-deleted version so it's not returned
+        $this->assertEmpty($segments);
+    }
+
     private function assertReturnedIdsMatch(array $expectedIds, array $resultSet)
     {
         $this->assertEquals(count($expectedIds), count($resultSet));