Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/Application/Account/Services/AccountHistory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
51 changes: 51 additions & 0 deletions tests/Unit/Application/Account/Services/AccountHistoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down