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