fix: an account is not saved with only part of its sharing

src/Application/Account/Services/AccountItems.php

@@ -39,8 +39,6 @@
 use SP\Domain\Core\Exceptions\QueryException;
 use SP\Domain\Core\Exceptions\SPException;
 
-use function SP\processException;
-
 /**
  * Class AccountItems
  */
@@ -161,43 +159,57 @@ function () use ($accountId, $userIds, $isEdit) {
         );
     }
 
+    /**
+     * The sharing an account is created with — all of it, or none of the account.
+     *
+     * This used to wrap the five writes below in a `catch (SPException)` that called
+     * `processException()` and returned, so a failure part-way left the account saved with whatever
+     * subset had been inserted first — or with nothing shared at all, if the first call was the one
+     * that threw — and the create still reported plain success. `AccountToUserGroup`,
+     * `AccountToUser` and `AccountToTag` all carry a foreign key to the row they name, so a group,
+     * user or tag deleted between the form being drawn and submitted throws a `ConstraintException`
+     * from inside one of them.
+     *
+     * `Account::create()` calls this inside its own `transactionAware()`, so letting the exception
+     * out rolls the account back with it — which is the whole point of that transaction, and what
+     * the edit path has always done: `replaceUserGroups()` and `replaceUsers()` run their
+     * delete-and-insert inside a transaction and let failures propagate.
+     *
+     * @throws SPException
+     */
     public function addItems(bool $userCanChangePermissions, int $accountId, AccountCreateDto $accountCreateDto): void
     {
-        try {
-            if ($userCanChangePermissions) {
-                if (null !== $accountCreateDto->userGroupsView
-                    && !empty($accountCreateDto->userGroupsView)
-                ) {
-                    $this->accountToUserGroupRepository->addByType(
-                        $accountId,
-                        $accountCreateDto->userGroupsView
-                    );
-                }
-
-                if (null !== $accountCreateDto->userGroupsEdit
-                    && !empty($accountCreateDto->userGroupsEdit)
-                ) {
-                    $this->accountToUserGroupRepository->addByType(
-                        $accountId,
-                        $accountCreateDto->userGroupsEdit,
-                        true
-                    );
-                }
+        if ($userCanChangePermissions) {
+            if (null !== $accountCreateDto->userGroupsView
+                && !empty($accountCreateDto->userGroupsView)
+            ) {
+                $this->accountToUserGroupRepository->addByType(
+                    $accountId,
+                    $accountCreateDto->userGroupsView
+                );
+            }
 
-                if (null !== $accountCreateDto->usersView && !empty($accountCreateDto->usersView)) {
-                    $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersView);
-                }
+            if (null !== $accountCreateDto->userGroupsEdit
+                && !empty($accountCreateDto->userGroupsEdit)
+            ) {
+                $this->accountToUserGroupRepository->addByType(
+                    $accountId,
+                    $accountCreateDto->userGroupsEdit,
+                    true
+                );
+            }
 
-                if (null !== $accountCreateDto->usersEdit && !empty($accountCreateDto->usersEdit)) {
-                    $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersEdit, true);
-                }
+            if (null !== $accountCreateDto->usersView && !empty($accountCreateDto->usersView)) {
+                $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersView);
             }
 
-            if (null !== $accountCreateDto->tags && !empty($accountCreateDto->tags)) {
-                $this->accountToTagRepository->add($accountId, $accountCreateDto->tags);
+            if (null !== $accountCreateDto->usersEdit && !empty($accountCreateDto->usersEdit)) {
+                $this->accountToUserRepository->addByType($accountId, $accountCreateDto->usersEdit, true);
             }
-        } catch (SPException $e) {
-            processException($e);
+        }
+
+        if (null !== $accountCreateDto->tags && !empty($accountCreateDto->tags)) {
+            $this->accountToTagRepository->add($accountId, $accountCreateDto->tags);
         }
     }
 }

tests/Unit/Application/Account/Services/AccountItemsTest.php

@@ -302,7 +302,23 @@ public function testAddItemsWithNoPermission()
         $this->accountItems->addItems(false, 100, $accountCreateDto);
     }
 
-    public function testAddItemsWithException()
+    /**
+     * A failure adding an account's sharing is reported, not swallowed.
+     *
+     * This used to assert the opposite — it called `addItems()` with a throwing repository and no
+     * `expectException`, so it passed only because nothing propagated. What that cost: the five
+     * writes were wrapped in a `catch (SPException)` that logged and returned, so an account was
+     * saved with whatever subset had been inserted before the failure (or with nothing shared at
+     * all, if the first call threw) and the create reported plain success.
+     *
+     * `AccountToUserGroup`, `AccountToUser` and `AccountToTag` each carry a foreign key to the row
+     * they name, so a group, user or tag deleted between the form being drawn and submitted throws
+     * a `ConstraintException` from inside one of them.
+     *
+     * `Account::create()` calls this inside its own `transactionAware()`, so the exception now
+     * takes the account with it — which is what the edit path has always done.
+     */
+    public function testAddItemsReportsAFailureRatherThanSavingPartOfTheSharing()
     {
         $accountCreateDto = AccountDataGenerator::factory()->buildAccountCreateDto();
 
@@ -319,6 +335,9 @@ public function testAddItemsWithException()
             ->method('add')
             ->willThrowException(SPException::error('test'));
 
+        $this->expectException(SPException::class);
+        $this->expectExceptionMessage('test');
+
         $this->accountItems->addItems(false, 100, $accountCreateDto);
     }
 

tests/Unit/Application/Account/Services/AccountTest.php

@@ -1879,6 +1879,50 @@ public function testEditPasswordThatMatchedNoRowIsNotReportedAsSaved(): void
         $this->account->editPassword($id, $accountUpdateDto);
     }
 
+    /**
+     * An account whose sharing could not be applied is not saved at all.
+     *
+     * `addItems()` used to catch and log, so a create whose group/user/tag insert failed part-way
+     * left the account stored with whatever subset had landed first — or with nothing shared, if
+     * the first call threw — and answered with plain success. Those three tables each carry a
+     * foreign key to the row they name, so a group, user or tag deleted between the form being
+     * drawn and submitted reaches exactly that.
+     *
+     * The account create already runs inside `transactionAware()`; what was missing was letting the
+     * failure reach it. Asserted through the service rather than on `addItems()` directly, because
+     * the thing that matters is that the *account* goes with it.
+     *
+     * @throws Exception
+     */
+    public function testCreateIsAbandonedWhenItsSharingCannotBeApplied()
+    {
+        $accountCreateDto = AccountDataGenerator::factory()->buildAccountCreateDto();
+
+        $this->accountCryptService
+            ->method('getPasswordEncrypted')
+            ->willReturn(new EncryptedPassword(self::$faker->password(), self::$faker->password()));
+
+        $this->itemPresetService->method('getForCurrentUser')->willReturn(null);
+
+        $this->accountRepository
+            ->method('create')
+            ->willReturn(new QueryResult(null, 0, self::$faker->numberBetween(1, 1000)));
+
+        $this->accountItemsService
+            ->expects(self::once())
+            ->method('addItems')
+            ->willThrowException(SPException::error('a shared group no longer exists'));
+
+        // Nothing after the sharing runs, and the transaction the create sits in takes the account
+        // with it.
+        $this->accountPresetService->expects(self::never())->method('addPresetPermissions');
+
+        $this->expectException(SPException::class);
+        $this->expectExceptionMessage('a shared group no longer exists');
+
+        $this->account->create($accountCreateDto);
+    }
+
     /**
      * The account the service is expected to write, give or take the second it was stamped in.
      *