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);