diff --git a/src/Application/Account/Services/AccountItems.php b/src/Application/Account/Services/AccountItems.php index 2fa27e4d4..d69024614 100644 --- a/src/Application/Account/Services/AccountItems.php +++ b/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); } } } diff --git a/tests/Unit/Application/Account/Services/AccountItemsTest.php b/tests/Unit/Application/Account/Services/AccountItemsTest.php index e71327696..6e64fa6e1 100644 --- a/tests/Unit/Application/Account/Services/AccountItemsTest.php +++ b/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); } diff --git a/tests/Unit/Application/Account/Services/AccountTest.php b/tests/Unit/Application/Account/Services/AccountTest.php index 22773a81b..9d385a8c9 100644 --- a/tests/Unit/Application/Account/Services/AccountTest.php +++ b/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. *