From 44a4184eb8ee368f2b618b22429d71abedbc8dab Mon Sep 17 00:00:00 2001 From: Marcin Kurczewski Date: Wed, 19 Nov 2014 19:07:29 +0100 Subject: [PATCH] Changed snapshot merging to work for deletions Creating and deleting stuff will remove history snapshots if it occurs within five minute gap. This is to prevent spamming history with tag names that are introduced by an accident and removed shortly after. --- src/Dao/SnapshotDao.php | 2 +- src/Services/HistoryService.php | 57 +++++++++++--------- tests/Services/HistoryServiceTest.php | 78 +++++++++++++++++++++++---- 3 files changed, 103 insertions(+), 34 deletions(-) diff --git a/src/Dao/SnapshotDao.php b/src/Dao/SnapshotDao.php index 57e5fafb..7f41293a 100644 --- a/src/Dao/SnapshotDao.php +++ b/src/Dao/SnapshotDao.php @@ -48,7 +48,7 @@ class SnapshotDao extends AbstractDao private function getUser(Snapshot $snapshot) { - $userId = $snapshot->getUserId(); + $userId = $snapshot->getUserId(); return $this->userDao->findById($userId); } } diff --git a/src/Services/HistoryService.php b/src/Services/HistoryService.php index 52b92f98..cd784f9d 100644 --- a/src/Services/HistoryService.php +++ b/src/Services/HistoryService.php @@ -42,28 +42,43 @@ class HistoryService $snapshot->setTime($this->timeService->getCurrentTime()); $snapshot->setUser($this->authService->getLoggedInUser()); - $lastSnapshot = $this->getLastSnapshot($snapshot); + $earlierSnapshots = array_values($this->snapshotDao->findEarlierSnapshots($snapshot)); + $snapshotsLeft = count($earlierSnapshots); - $dataDifference = $this->getSnapshotDataDifference($snapshot, $lastSnapshot); - $snapshot->setDataDifference($dataDifference); - - if ($snapshot->getOperation() !== Snapshot::OPERATION_DELETE && $lastSnapshot !== null) + while (true) { - //don't save if nothing changed - if (empty($dataDifference['+']) && empty($dataDifference['-'])) - { - if ($snapshot->getId()) - $this->snapshotDao->deleteById($snapshot->getId()); - return $lastSnapshot; - } + $lastSnapshot = array_shift($earlierSnapshots); + $dataDifference = $this->getSnapshotDataDifference($snapshot, $lastSnapshot); + $snapshot->setDataDifference($dataDifference); - //merge recent edits + if ($lastSnapshot === null) + break; + + //recent snapshots should be merged $isFresh = ((strtotime($snapshot->getTime()) - strtotime($lastSnapshot->getTime())) <= 5 * 60); - if ($isFresh && $lastSnapshot->getUserId() === $snapshot->getUserId()) - { - $lastSnapshot->setData($snapshot->getData()); - return $this->saveSnapshot($lastSnapshot); - } + if (!$isFresh || $lastSnapshot->getUserId() !== $snapshot->getUserId()) + break; + + //delete the current snapshot + if ($snapshot->getId()) + $this->snapshotDao->deleteById($snapshot->getId()); + + //become previous snapshot, but keep the current data + $snapshot->setId($lastSnapshot->getId()); + if ($snapshot->getOperation() !== Snapshot::OPERATION_DELETE) + $snapshot->setOperation($lastSnapshot->getOperation()); + + -- $snapshotsLeft; + } + + $onlyRemovalLeft = (!$snapshotsLeft && $snapshot->getOperation() === Snapshot::OPERATION_DELETE); + + $emptyDiff = (empty($dataDifference['+']) && empty($dataDifference['-'])); + if ($onlyRemovalLeft || $emptyDiff) + { + if ($snapshot->getId()) + $this->snapshotDao->deleteById($snapshot->getId()); + return array_shift($earlierSnapshots); } return $this->snapshotDao->save($snapshot); @@ -110,10 +125,4 @@ class HistoryService '-' => $diffFunction($oldData, $newData), ]; } - - private function getLastSnapshot(Snapshot $reference) - { - $earlierSnapshots = $this->snapshotDao->findEarlierSnapshots($reference); - return empty($earlierSnapshots) ? null : array_shift($earlierSnapshots); - } } diff --git a/tests/Services/HistoryServiceTest.php b/tests/Services/HistoryServiceTest.php index 59770654..bc76d588 100644 --- a/tests/Services/HistoryServiceTest.php +++ b/tests/Services/HistoryServiceTest.php @@ -123,6 +123,7 @@ final class HistoryServiceTest extends AbstractTestCase public static function mergingProvider() { { + //basic merging $oldSnapshot = new Snapshot(1); $oldSnapshot->setTime(date('c', 1)); $oldSnapshot->setOperation(Snapshot::OPERATION_CREATION); @@ -139,10 +140,11 @@ final class HistoryServiceTest extends AbstractTestCase $expectedSnapshot->setData(['new' => '2']); $expectedSnapshot->setDataDifference(['+' => ['new' => '2'], '-' => []]); - yield [$oldSnapshot, $newSnapshot, $expectedSnapshot, date('c', 3)]; + yield [[$oldSnapshot], $newSnapshot, $expectedSnapshot, date('c', 3), [2]]; } { + //too big time gap for merge $oldSnapshot = new Snapshot(1); $oldSnapshot->setOperation(Snapshot::OPERATION_CREATION); $oldSnapshot->setData(['old' => '1']); @@ -157,10 +159,11 @@ final class HistoryServiceTest extends AbstractTestCase $expectedSnapshot->setData(['new' => '2']); $expectedSnapshot->setDataDifference(['+' => ['new' => '2'], '-' => ['old' => '1']]); - yield [$oldSnapshot, $newSnapshot, $expectedSnapshot, date('c', 3000)]; + yield [[$oldSnapshot], $newSnapshot, $expectedSnapshot, date('c', 3000), []]; } { + //operations done by different user shouldn't be merged $oldSnapshot = new Snapshot(1); $oldSnapshot->setOperation(Snapshot::OPERATION_CREATION); $oldSnapshot->setData(['old' => '1']); @@ -177,10 +180,11 @@ final class HistoryServiceTest extends AbstractTestCase $expectedSnapshot->setDataDifference(['+' => ['new' => '2'], '-' => ['old' => '1']]); $expectedSnapshot->setUserId(null); - yield [$oldSnapshot, $newSnapshot, $expectedSnapshot, null]; + yield [[$oldSnapshot], $newSnapshot, $expectedSnapshot, null, []]; } { + //merge that leaves only delete snapshot should be removed altogether $oldSnapshot = new Snapshot(1); $oldSnapshot->setOperation(Snapshot::OPERATION_CREATION); $oldSnapshot->setData(['old' => '1']); @@ -189,35 +193,91 @@ final class HistoryServiceTest extends AbstractTestCase $newSnapshot->setOperation(Snapshot::OPERATION_DELETE); $newSnapshot->setData(['new' => '2']); + yield [[$oldSnapshot], $newSnapshot, null, null, [2, 1]]; + } + + { + //chaining to creation snapshot should preserve operation type + $oldestSnapshot = new Snapshot(1); + $oldestSnapshot->setOperation(Snapshot::OPERATION_CREATION); + $oldestSnapshot->setData(['oldest' => '0']); + + $oldSnapshot = new Snapshot(2); + $oldSnapshot->setOperation(Snapshot::OPERATION_CHANGE); + $oldSnapshot->setData(['old' => '1']); + + $newSnapshot = new Snapshot(3); + $newSnapshot->setOperation(Snapshot::OPERATION_CHANGE); + $newSnapshot->setData(['oldest' => '0', 'new' => '2']); + + $expectedSnapshot = new Snapshot(1); + $expectedSnapshot->setOperation(Snapshot::OPERATION_CREATION); + $expectedSnapshot->setData(['oldest' => '0', 'new' => '2']); + $expectedSnapshot->setDataDifference(['+' => ['oldest' => '0', 'new' => '2'], '-' => []]); + + yield [[$oldSnapshot, $oldestSnapshot], $newSnapshot, $expectedSnapshot, null, [3, 2]]; + + $newSnapshot = clone($newSnapshot); + $newSnapshot->setId(null); + yield [[$oldSnapshot, $oldestSnapshot], $newSnapshot, $expectedSnapshot, null, [2]]; + } + + { + //chaining to edit snapshot should update operation type + $oldestSnapshot = new Snapshot(1); + $oldestSnapshot->setOperation(Snapshot::OPERATION_CREATION); + $oldestSnapshot->setData(['oldest' => '0']); + $oldestSnapshot->setTime(date('c', 1)); + + $oldSnapshot = new Snapshot(2); + $oldSnapshot->setOperation(Snapshot::OPERATION_CHANGE); + $oldSnapshot->setData(['old' => '1']); + $oldSnapshot->setTime(date('c', 400)); + + $newSnapshot = new Snapshot(3); + $newSnapshot->setOperation(Snapshot::OPERATION_DELETE); + $newSnapshot->setData(['new' => '2']); + $newSnapshot->setTime(date('c', 401)); + $expectedSnapshot = new Snapshot(2); $expectedSnapshot->setOperation(Snapshot::OPERATION_DELETE); $expectedSnapshot->setData(['new' => '2']); - $expectedSnapshot->setDataDifference(['+' => ['new' => '2'], '-' => ['old' => '1']]); + $expectedSnapshot->setDataDifference(['+' => ['new' => '2'], '-' => ['oldest' => '0']]); + $expectedSnapshot->setTime(date('c', 402)); - yield [$oldSnapshot, $newSnapshot, $expectedSnapshot, null]; + yield [[$oldSnapshot, $oldestSnapshot], $newSnapshot, $expectedSnapshot, date('c', 402), [3]]; + + $newSnapshot = clone($newSnapshot); + $newSnapshot->setId(null); + yield [[$oldSnapshot, $oldestSnapshot], $newSnapshot, $expectedSnapshot, date('c', 402), []]; } } /** * @dataProvider mergingProvider */ - public function testMerging($oldSnapshot, $newSnapshot, $expectedSnapshot, $currentTime) + public function testMerging($earlierSnapshots, $newSnapshot, $expectedSnapshot, $currentTime, $expectedDeletions = []) { $this->timeServiceMock->method('getCurrentTime')->willReturn($currentTime); $this->snapshotDaoMock + ->expects($this->once()) ->method('findEarlierSnapshots') - ->will( - $this->onConsecutiveCalls([$oldSnapshot], null)); + ->willReturn($earlierSnapshots); $this->snapshotDaoMock - ->expects($this->once()) + ->expects($this->exactly($expectedSnapshot === null ? 0 : 1)) ->method('save') ->will($this->returnCallback(function($param) use (&$actualSnapshot) { $actualSnapshot = $param; })); + $this->snapshotDaoMock + ->expects($this->exactly(count($expectedDeletions))) + ->method('deleteById') + ->withConsecutive(...array_map(function($del) { return [$del]; }, $expectedDeletions)); + $historyService = $this->getHistoryService(); $historyService->saveSnapshot($newSnapshot); $this->assertEntitiesEqual($expectedSnapshot, $actualSnapshot);