fix: a group that cannot be deleted says why

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<int, Simple> $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
      *

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

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

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