diff --git a/src/Application/Account/Services/AccountHistory.php b/src/Application/Account/Services/AccountHistory.php index 4ac2bc85a..e8a52a9c4 100644 --- a/src/Application/Account/Services/AccountHistory.php +++ b/src/Application/Account/Services/AccountHistory.php @@ -141,14 +141,31 @@ public function delete(int $id): void */ public function deleteByIdBatch(array $ids): int { - return $this->accountHistoryRepository->transactionAware(function () use ($ids) { + $count = $this->accountHistoryRepository->transactionAware(function () use ($ids) { return $this->accountHistoryRepository->deleteByIdBatch($ids); }, $this); + + // These are history row ids, so every one of them should have matched. Returning the count + // and leaving it at that meant a purge which removed fewer rows than it was given — a + // stale id, a row another session had already deleted — was reported as done: the + // controller discards this value and answers success either way. An old password the + // operator believes they destroyed is exactly what history is kept in. + // + // The single delete beside this one has always thrown; this is that check, for the batch. + if ($count !== count($ids)) { + throw new ServiceException(__u('Error while deleting the accounts'), SPException::WARNING); + } + + return $count; } /** * Deletes all the items for given accounts id * + * Deliberately unchecked, unlike deleteByIdBatch() above: these are *account* ids, and an + * account may have no history at all — a purge that removes nothing from it has done exactly + * what it was asked. There is no count here to compare against. + * * @param int[] $ids * * @return int diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php index 04d56d2af..da8b020a1 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AccountHistoryManager/AccountHistoryManagerTest.php @@ -84,6 +84,19 @@ public function deleteMultiple() new QueryResult([AccountDataGenerator::factory()->buildAccountHistoryData()]) ); + // The delete has to report as many rows as it was given ids, because the service now + // checks: a purge that removed fewer rows than it was asked to is not a purge that + // happened, and history is where an account's previous passwords live. The harness's + // default answer is one affected row whatever the statement, which for three ids reads as + // exactly that failure. Not a static closure — the harness binds it with Closure::call(). + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + if (str_contains($queryData->getQuery()->getStatement(), 'DELETE')) { + return new QueryResult([], 3); + } + + return new QueryResult([], 1, 100); + }; + $container = $this->buildContainer( IntegrationTestCase::buildRequest( 'post', diff --git a/tests/Unit/Application/Account/Services/AccountHistoryTest.php b/tests/Unit/Application/Account/Services/AccountHistoryTest.php index 34710e020..22a955377 100644 --- a/tests/Unit/Application/Account/Services/AccountHistoryTest.php +++ b/tests/Unit/Application/Account/Services/AccountHistoryTest.php @@ -216,6 +216,57 @@ public function testDeleteByIdBatch() $this->accountHistory->deleteByIdBatch($ids); } + /** + * A purge that removed fewer rows than it was given says so. + * + * The ids here are history rows, so every one of them should have matched. The count was + * returned and thrown away — the controller discards it and answers "Accounts updated" either + * way — so a stale id, or a row another session had already deleted, was reported as a purge + * that had happened. An old password the operator believes they destroyed is precisely what + * history holds. + * + * @throws ServiceException + */ + public function testDeleteByIdBatchThatRemovedFewerRowsThanItWasGiven() + { + $ids = [1, 2, 3]; + + $this->accountHistoryRepository + ->expects($this->once()) + ->method('transactionAware') + ->with(self::withResolveCallableCallback()) + ->willReturn(2); + + $this->accountHistoryRepository + ->expects(self::once()) + ->method('deleteByIdBatch') + ->with($ids) + ->willReturn(2); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Error while deleting the accounts'); + + $this->accountHistory->deleteByIdBatch($ids); + } + + /** + * Deleting by *account* id is deliberately not held to that: an account may have no history at + * all, so removing nothing from it is the right answer rather than a failure. There is no count + * to compare against. + */ + public function testDeleteByAccountIdBatchAcceptsAccountsWithNoHistory() + { + $ids = [1, 2, 3]; + + $this->accountHistoryRepository + ->expects(self::once()) + ->method('deleteByAccountIdBatch') + ->with($ids) + ->willReturn(0); + + self::assertSame(0, $this->accountHistory->deleteByAccountIdBatch($ids)); + } + public function testSearch() { $itemSearchData =