delete old log_* table data also without idvisit (#17964)

4 files changed, 129 insertions, 36 deletions

diff --git a/core/DataAccess/RawLogDao.php b/core/DataAccess/RawLogDao.php
index a3bd3ed8b2..196e4ed04f 100644
--- a/core/DataAccess/RawLogDao.php
+++ b/core/DataAccess/RawLogDao.php
@@ -145,22 +145,6 @@ class RawLogDao
     }
 
     /**
-     * Deletes conversions for the supplied visit IDs from log_conversion. This method does not cascade, so
-     * conversion items will not be deleted.
-     *
-     * @param int[] $visitIds
-     * @return int The number of deleted rows.
-     */
-    public function deleteFromLogTable($tableName, $visitIds)
-    {
-        $sql = "DELETE FROM `" . Common::prefixTable($tableName) . "` WHERE idvisit IN "
-             . $this->getInFieldExpressionWithInts($visitIds);
-
-        $statement = Db::query($sql);
-        return $statement->rowCount();
-    }
-
-    /**
      * Deletes conversion items for the supplied visit IDs from log_conversion_item.
      *
      * @param int[] $visitIds
diff --git a/core/LogDeleter.php b/core/LogDeleter.php
index 64e9c37746..84ba4b5160 100644
--- a/core/LogDeleter.php
+++ b/core/LogDeleter.php
@@ -8,8 +8,10 @@
 
 namespace Piwik;
 
+use Piwik\Container\StaticContainer;
 use Piwik\DataAccess\RawLogDao;
 use Piwik\Plugin\LogTablesProvider;
+use Piwik\Plugins\PrivacyManager\Model\DataSubjects;
 use Piwik\Plugins\SitesManager\Model;
 
 /**
@@ -43,18 +45,12 @@ class LogDeleter
      */
     public function deleteVisits($visitIds)
     {
-        $numDeletedVisits = 0;
-
-        foreach ($this->logTablesProvider->getAllLogTables() as $logTable) {
-            if ($logTable->getColumnToJoinOnIdVisit()) {
-                $numVisits = $this->rawLogDao->deleteFromLogTable($logTable->getName(), $visitIds);
-                if ($logTable->getName() === 'log_visit') {
-                    $numDeletedVisits = $numVisits;
-                }
-            }
-        }
-
-        return $numDeletedVisits;
+        $visitIds = array_map(function($visitid) {
+            return ['idvisit' => $visitid];
+        }, $visitIds);
+        $dataSubjects = StaticContainer::get(DataSubjects::class);
+        $deleteCounts = $dataSubjects->deleteDataSubjectsWithoutInvalidatingArchives($visitIds);
+        return $deleteCounts['log_visit'];
     }
 
     /**
@@ -99,7 +95,6 @@ class LogDeleter
             $ids = array_map(function ($row) { return (int) (reset($row)); }, $logs);
             sort($ids);
             $logsDeleted += $logPurger->deleteVisits($ids);
-
             if (!empty($afterChunkDeleted)) {
                 $afterChunkDeleted($logsDeleted);
             }
diff --git a/plugins/PrivacyManager/Model/DataSubjects.php b/plugins/PrivacyManager/Model/DataSubjects.php
index 1819b20bfb..0e9acd71a5 100644
--- a/plugins/PrivacyManager/Model/DataSubjects.php
+++ b/plugins/PrivacyManager/Model/DataSubjects.php
@@ -109,10 +109,7 @@ class DataSubjects
 
         $datesToInvalidateByIdSite = $this->getDatesToInvalidate($visits);
 
-        $logTables = $this->getLogTablesToDeleteFrom();
-        $deleteCounts = $this->deleteLogDataFrom($logTables, function ($tableToSelectFrom) use ($visits) {
-            return $this->visitsToWhereAndBind($tableToSelectFrom, $visits);
-        });
+        $deleteCounts = $this->deleteDataSubjectsWithoutInvalidatingArchives($visits);
 
         $this->invalidateArchives($datesToInvalidateByIdSite);
 
@@ -459,12 +456,22 @@ class DataSubjects
     {
         $where = array();
         $bind = array();
+        $in = array();
         foreach ($visits as $visit) {
-            $where[] = '(' . $tableToSelect . '.idsite = ? AND ' . $tableToSelect . '.idvisit = ?)';
-            $bind[] = $visit['idsite'];
-            $bind[] = $visit['idvisit'];
+            if (empty($visit['idsite'])) {
+                $in[] = (int) $visit['idvisit'];
+            } else {
+                $where[] = sprintf('(%s.idsite = %d AND %s.idvisit = %d)',
+                    $tableToSelect, (int) $visit['idsite'], $tableToSelect, (int) $visit['idvisit']);
+            }
         }
         $where = implode(' OR ', $where);
+        if (!empty($in)) {
+            if (!empty($where)) {
+                $where .= ' OR ';
+            }
+            $where .= $tableToSelect . '.idvisit in (' . implode(',',$in) . ')';
+        }
 
         return array($where, $bind);
     }
@@ -512,4 +519,18 @@ class DataSubjects
 
     }
 
+    /**
+     * @param $visits
+     * @return array
+     * @throws \Zend_Db_Statement_Exception
+     */
+    public function deleteDataSubjectsWithoutInvalidatingArchives($visits): array
+    {
+        $logTables = $this->getLogTablesToDeleteFrom();
+        $deleteCounts = $this->deleteLogDataFrom($logTables, function ($tableToSelectFrom) use ($visits) {
+            return $this->visitsToWhereAndBind($tableToSelectFrom, $visits);
+        });
+        return $deleteCounts;
+    }
+
 }
diff --git a/plugins/PrivacyManager/tests/Integration/Model/DataSubjectsTest.php b/plugins/PrivacyManager/tests/Integration/Model/DataSubjectsTest.php
index 176aee1ba0..eadc9263ac 100644
--- a/plugins/PrivacyManager/tests/Integration/Model/DataSubjectsTest.php
+++ b/plugins/PrivacyManager/tests/Integration/Model/DataSubjectsTest.php
@@ -66,6 +66,99 @@ class DataSubjectsTest extends IntegrationTestCase
         $this->theFixture->trackingTime = $this->originalTrackingTime;
     }
 
+    public function test_deleteDataSubjectsWithoutInvalidatingArchives_deleteVisitsWithoutIdsite()
+    {
+        $this->theFixture->setUpWebsites();
+        $this->theFixture->trackVisits($idSite = 1, 1);
+
+        $this->assertNotEmpty($this->getVisit(1, 1));
+        $this->assertNotEmpty($this->getLinkAction(1, 1));
+        $this->assertNotEmpty($this->getConversion(1, 1));
+        $this->assertNotEmpty($this->getOneVisit(1, 1, TestLogFoo::TABLE));
+
+        $this->assertNotEmpty($this->getVisit(1, 3));
+        $this->assertNotEmpty($this->getLinkAction(1, 3));
+        $this->assertNotEmpty($this->getConversion(1, 3));
+
+        $this->assertNotEmpty($this->getVisit(1, 2));
+        $this->assertNotEmpty($this->getLinkAction(1, 2));
+        $this->assertNotEmpty($this->getOneVisit(1, 2, TestLogFoo::TABLE));
+
+        $visits = array(array('idvisit' => 1),array('idvisit' => 3),array('idvisit' => 999));
+        $result = $this->dataSubjects->deleteDataSubjectsWithoutInvalidatingArchives($visits);
+
+        $this->assertEquals(array(
+            'log_conversion' => 2,
+            'log_conversion_item' => 0,
+            'log_link_visit_action' => 12,
+            'log_visit' => 2,
+            'log_foo_bar_baz' => 2,
+            'log_foo_bar' => 2,
+            'log_foo' => 2
+        ), $result);
+
+        $this->assertEmpty($this->getVisit(1, 1));
+        $this->assertEmpty($this->getLinkAction(1, 1));
+        $this->assertEmpty($this->getConversion(1, 1));
+        $this->assertEmpty($this->getOneVisit(1, 1, TestLogFoo::TABLE));
+
+        $this->assertEmpty($this->getVisit(1, 3));
+        $this->assertEmpty($this->getLinkAction(1, 3));
+        $this->assertEmpty($this->getConversion(1, 3));
+        $this->assertEmpty($this->getOneVisit(1, 3, TestLogFoo::TABLE));
+
+        // idvisit 2 still exists
+        $this->assertNotEmpty($this->getVisit(1, 2));
+        $this->assertNotEmpty($this->getLinkAction(1, 2));
+        $this->assertNotEmpty($this->getOneVisit(1, 2, TestLogFoo::TABLE));
+    }
+
+    public function test_deleteDataSubjectsWithoutInvalidatingArchives_deleteVisitWithAndWithoutIdSite()
+    {
+        $this->theFixture->setUpWebsites();
+        $this->theFixture->trackVisits($idSite = 1, 1);
+
+        $this->assertNotEmpty($this->getVisit(1, 1));
+        $this->assertNotEmpty($this->getLinkAction(1, 1));
+        $this->assertNotEmpty($this->getConversion(1, 1));
+        $this->assertNotEmpty($this->getOneVisit(1, 1, TestLogFoo::TABLE));
+
+        $this->assertNotEmpty($this->getVisit(1, 3));
+        $this->assertNotEmpty($this->getLinkAction(1, 3));
+        $this->assertNotEmpty($this->getConversion(1, 3));
+
+        $this->assertNotEmpty($this->getVisit(1, 2));
+        $this->assertNotEmpty($this->getLinkAction(1, 2));
+        $this->assertNotEmpty($this->getOneVisit(1, 2, TestLogFoo::TABLE));
+
+        $visits = array(array('idvisit' => 1),array('idvisit' => 3, 'idsite' => 1),array('idvisit' => 999),array('idvisit' => 999, 'idsite' => 999));
+        $result = $this->dataSubjects->deleteDataSubjectsWithoutInvalidatingArchives($visits);
+
+        $this->assertEquals(array(
+            'log_conversion' => 2,
+            'log_conversion_item' => 0,
+            'log_link_visit_action' => 12,
+            'log_visit' => 2,
+            'log_foo_bar_baz' => 2,
+            'log_foo_bar' => 2,
+            'log_foo' => 2
+        ), $result);
+
+        $this->assertEmpty($this->getVisit(1, 1));
+        $this->assertEmpty($this->getLinkAction(1, 1));
+        $this->assertEmpty($this->getConversion(1, 1));
+        $this->assertEmpty($this->getOneVisit(1, 1, TestLogFoo::TABLE));
+
+        $this->assertEmpty($this->getVisit(1, 3));
+        $this->assertEmpty($this->getLinkAction(1, 3));
+        $this->assertEmpty($this->getConversion(1, 3));
+
+        // idvisit 2 still exists
+        $this->assertNotEmpty($this->getVisit(1, 2));
+        $this->assertNotEmpty($this->getLinkAction(1, 2));
+        $this->assertNotEmpty($this->getOneVisit(1, 2, TestLogFoo::TABLE));
+    }
+
     public function test_deleteExport_deleteOneVisit()
     {
         $this->theFixture->setUpWebsites();