From 55a8519f84e0719c58094ff4e8dbd830085e39cb Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 23:35:13 +0200 Subject: [PATCH] fix: a group that cannot be deleted says why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a user group that something still holds came back as "The record is in use" — MySQL error 1451 translated by the database layer — which names nothing. The group's own "Used by" panel was no help either: it lists the group's *users*, so a group with no members but a hundred accounts scoped to it looked entirely safe to delete right up until it was not. The delete now asks first, and the refusal says what is holding it: "Group in use", with "Accounts: 2 - Accounts in history: 1". Getting that right meant reading the schema rather than the existing helper. Six foreign keys point at UserGroup and only three of them refuse a delete: fk_User_userGroupId RESTRICT a user whose main group this is fk_Account_userGroupId RESTRICT an account owned by the group fk_AccountHistory_userGroupId RESTRICT a history row, which outlives its account fk_AccountToUserGroup_userGroupId CASCADE fk_ItemPreset_userGroupId CASCADE fk_UserToUserGroup_userGroupId CASCADE `getUsage()` had it wrong in both directions: it queried UserToUserGroup and AccountToUserGroup, which cascade, and not AccountHistory, which does not. So wiring it up as it stood would have refused deletes the database would have allowed — a group whose only "usage" is the members it is supposed to have — while still letting through the one case that actually fails. It asks for exactly the three RESTRICT relations now. That method had never run. It has no caller anywhere in src, and the coverage report shows count="0" on both it and the service method in front of it — which is presumably how it came to disagree with the schema without anyone noticing. The foreign keys still stand behind all this: a row created between the check and the delete is refused by the database exactly as before. This is about telling an administrator what to go and fix. This is the same defect the codebase already fixed for individual users — `User::getUsageForUser()` carries a comment about the panel disagreeing with the delete — and it was never carried across to groups. Checked by making the refusal unreachable: the new service test fails. --- src/Application/User/Services/UserGroup.php | 46 +++++++++++++++++++ .../Out/User/Repositories/UserGroup.php | 27 +++++++---- .../User/Services/UserGroupTest.php | 45 ++++++++++++++++++ .../Out/User/Repositories/UserGroupTest.php | 30 ++++++++++-- 4 files changed, 134 insertions(+), 14 deletions(-) diff --git a/src/Application/User/Services/UserGroup.php b/src/Application/User/Services/UserGroup.php index ed410579d..a26d0237b 100644 --- a/src/Application/User/Services/UserGroup.php +++ b/src/Application/User/Services/UserGroup.php @@ -39,6 +39,7 @@ use SP\Domain\Core\Exceptions\NoSuchItemException; use SP\Domain\Common\Dtos\QueryResult; +use function SP\__; use function SP\__u; /** @@ -93,11 +94,56 @@ public function getById(int $id): UserGroupModel */ public function delete(int $id): void { + // Said before the database says it, and in terms an administrator can act on. Three + // foreign keys refuse this delete — a user whose main group it is, an account owned by it, + // and a history row, which outlives the account it describes. Without this the refusal + // came back from MySQL as error 1451 and was rendered as "The record is in use", which + // names nothing; and the group's own "Used by" panel lists only its users, so a group with + // no members but a hundred accounts looked perfectly safe to delete right up until it was + // not. + // + // The foreign keys still stand behind this — a row created between the two statements is + // refused by the database as before. This is about telling somebody what to go and fix. + $usedBy = $this->getUsage($id); + + if ($usedBy !== []) { + throw ServiceException::warning( + __u('Group in use'), + self::describeUsage($usedBy) + ); + } + if ($this->userGroupRepository->delete($id)->getAffectedNumRows() === 0) { throw NoSuchItemException::info(__u('Group not found')); } } + /** + * What is holding the group, counted by kind, for the hint on the refusal. + * + * @param array $usedBy + */ + private static function describeUsage(array $usedBy): string + { + $counts = array_count_values(array_map(static fn(Simple $row): string => (string)$row['ref'], $usedBy)); + + $described = [ + 'User' => __('Users'), + 'Account' => __('Accounts'), + 'AccountHistory' => __('Accounts in history'), + ]; + + $parts = []; + + foreach ($described as $ref => $label) { + if (isset($counts[$ref])) { + $parts[] = sprintf('%s: %d', $label, $counts[$ref]); + } + } + + return implode(' - ', $parts); + } + /** * @param int[] $ids * diff --git a/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php b/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php index 8715f1adf..cd3ca2d9e 100644 --- a/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php +++ b/src/Infrastructure/Adapter/Out/User/Repositories/UserGroup.php @@ -27,6 +27,7 @@ use Exception; use SP\Domain\Account\Models\Account as AccountModel; +use SP\Domain\Account\Models\AccountHistory as AccountHistoryModel; use SP\Domain\Account\Models\AccountToUserGroup as AccountToUserGroupModel; use SP\Domain\Common\Models\Simple; use SP\Domain\Core\Dtos\ItemSearchDto; @@ -83,28 +84,36 @@ public function delete(int $id): QueryResult */ public function getUsage(int $userGroupId): QueryResult { + // Exactly the references that refuse the delete, which is not the same as the references + // that exist. Six foreign keys point at UserGroup; three carry ON DELETE CASCADE and + // therefore never block — AccountToUserGroup, ItemPreset and UserToUserGroup. The other + // three have no ON DELETE clause, so they are RESTRICT, and they are what MySQL refuses on: + // + // fk_User_userGroupId a user whose *main* group this is + // fk_Account_userGroupId an account owned by this group + // fk_AccountHistory_userGroupId a history row, which outlives the account it describes + // + // This listed UserToUserGroup and AccountToUserGroup — two that cascade — and omitted + // AccountHistory, which does not. Answering with the cascading ones would refuse a delete + // the database would have allowed; omitting the history one lets a delete through that it + // then refuses for a reason nothing has mentioned. $query = $this->queryFactory ->newSelect() ->from(UserModel::TABLE) ->cols(['userGroupId AS id', '"User" AS ref']) ->where('userGroupId = :userGroupId1') ->unionAll() - ->from(UserToUserGroupModel::TABLE) - ->cols(['userGroupId AS id', '"UserGroup" AS ref']) + ->from(AccountModel::TABLE) + ->cols(['userGroupId AS id', '"Account" AS ref']) ->where('userGroupId = :userGroupId2') ->unionAll() - ->from(AccountToUserGroupModel::TABLE) - ->cols(['userGroupId AS id', '"AccountToUserGroup" AS ref']) + ->from(AccountHistoryModel::TABLE) + ->cols(['userGroupId AS id', '"AccountHistory" AS ref']) ->where('userGroupId = :userGroupId3') - ->unionAll() - ->from(AccountModel::TABLE) - ->cols(['userGroupId AS id', '"Account" AS ref']) - ->where('userGroupId = :userGroupId4') ->bindValues([ 'userGroupId1' => $userGroupId, 'userGroupId2' => $userGroupId, 'userGroupId3' => $userGroupId, - 'userGroupId4' => $userGroupId, ]); return $this->db->runQuery(QueryData::build($query)); diff --git a/tests/Unit/Application/User/Services/UserGroupTest.php b/tests/Unit/Application/User/Services/UserGroupTest.php index 6d38f9582..69039a9b8 100644 --- a/tests/Unit/Application/User/Services/UserGroupTest.php +++ b/tests/Unit/Application/User/Services/UserGroupTest.php @@ -38,6 +38,7 @@ use SP\Application\User\Services\UserGroup; use SP\Domain\Core\Exceptions\NoSuchItemException; use SP\Domain\Common\Dtos\QueryResult; +use SP\Domain\Common\Models\Simple; use SP\Tests\Support\Generators\UserGroupGenerator; use SP\Tests\Support\Stubs\UserGroupRepositoryStub; use SP\Tests\Support\UnitaryTestCase; @@ -330,6 +331,12 @@ public function testDeleteByIdBatchWithException() */ public function testDelete() { + $this->userGroupRepository + ->expects($this->once()) + ->method('getUsage') + ->with(100) + ->willReturn(new QueryResult([])); + $this->userGroupRepository ->expects($this->once()) ->method('delete') @@ -339,6 +346,44 @@ public function testDelete() $this->userGroup->delete(100); } + /** + * A group something still holds is refused, and the refusal says what holds it. + * + * Three foreign keys refuse this delete: a user whose main group it is, an account owned by + * it, and a history row, which outlives the account it describes. The refusal used to come + * back from MySQL as error 1451 and be rendered as "The record is in use", which names + * nothing — and the group's own "Used by" panel lists only its users, so a group with no + * members but a hundred accounts looked entirely safe to delete right up until it was not. + * + * @throws ConstraintException + * @throws NoSuchItemException + * @throws QueryException + */ + public function testDeleteRefusesAGroupInUseAndSaysWhatHoldsIt() + { + $this->userGroupRepository + ->expects($this->once()) + ->method('getUsage') + ->with(100) + ->willReturn( + new QueryResult([ + new Simple(['id' => 100, 'ref' => 'Account']), + new Simple(['id' => 100, 'ref' => 'Account']), + new Simple(['id' => 100, 'ref' => 'AccountHistory']), + ]) + ); + + $this->userGroupRepository->expects($this->never())->method('delete'); + + try { + $this->userGroup->delete(100); + self::fail('Expected a ServiceException'); + } catch (ServiceException $e) { + self::assertSame('Group in use', $e->getMessage()); + self::assertSame('Accounts: 2 - Accounts in history: 1', $e->getHint()); + } + } + /** * @throws ConstraintException * @throws NoSuchItemException diff --git a/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserGroupTest.php b/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserGroupTest.php index 33228e00a..516a58520 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserGroupTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/User/Repositories/UserGroupTest.php @@ -344,24 +344,44 @@ static function (QueryData $arg) { $this->userGroup->getAll(); } - public function testGetUsage() + /** + * getUsage() answers with the references that actually refuse the delete. + * + * Six foreign keys point at UserGroup. Three cascade — AccountToUserGroup, ItemPreset and + * UserToUserGroup — so they never block. The other three have no ON DELETE clause, so they are + * RESTRICT and they are exactly what MySQL refuses on: a user whose main group this is, an + * account owned by it, and a history row, which outlives the account it describes. + * + * This used to query UserToUserGroup and AccountToUserGroup, which cascade, and not + * AccountHistory, which does not — wrong in both directions. Asserted on the statement, since + * which tables it reads is the whole point. + */ + public function testGetUsageAsksOnlyWhatCanRefuseTheDelete() { + $statement = null; + $this->database ->expects($this->once()) ->method('runQuery') ->with( - self::callback(static function (QueryData $queryData) { + self::callback(static function (QueryData $queryData) use (&$statement) { $params = $queryData->getQuery()->getBindValues(); + $statement = $queryData->getQuery()->getStatement(); - return count($params) === 4 + return count($params) === 3 && $params['userGroupId1'] === 100 && $params['userGroupId2'] === 100 - && $params['userGroupId3'] === 100 - && $params['userGroupId4'] === 100; + && $params['userGroupId3'] === 100; }) ); $this->userGroup->getUsage(100); + + self::assertIsString($statement); + self::assertStringContainsString('User', $statement); + self::assertStringContainsString('AccountHistory', $statement); + self::assertStringNotContainsString('UserToUserGroup', $statement, 'that one cascades'); + self::assertStringNotContainsString('AccountToUserGroup', $statement, 'so does that one'); } public function testGetByName()