diff --git a/src/Application/Account/Services/AccountItems.php b/src/Application/Account/Services/AccountItems.php index f892156a4..2fa27e4d4 100644 --- a/src/Application/Account/Services/AccountItems.php +++ b/src/Application/Account/Services/AccountItems.php @@ -68,70 +68,33 @@ public function updateItems( AccountUpdateDto $accountUpdateDto ): void { if ($userCanChangePermissions) { - if (null === $accountUpdateDto->userGroupsView) { - $this->accountToUserGroupRepository->deleteTypeByAccountId($accountId, false); - } elseif (!empty($accountUpdateDto->userGroupsView)) { - $this->accountToUserGroupRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserGroupRepository - ->deleteTypeByAccountId($accountId, false); - $this->accountToUserGroupRepository - ->addByType($accountId, $accountUpdateDto->userGroupsView); - }, - $this - ); - } - - if (null === $accountUpdateDto->userGroupsEdit) { - $this->accountToUserGroupRepository->deleteTypeByAccountId($accountId, true); - } elseif (!empty($accountUpdateDto->userGroupsEdit)) { - $this->accountToUserGroupRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserGroupRepository - ->deleteTypeByAccountId($accountId, true); - $this->accountToUserGroupRepository - ->addByType($accountId, $accountUpdateDto->userGroupsEdit, true); - }, - $this - ); - } - - if (null === $accountUpdateDto->usersView) { - $this->accountToUserRepository->deleteTypeByAccountId($accountId, false); - } elseif (!empty($accountUpdateDto->usersView)) { - $this->accountToUserRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserRepository - ->deleteTypeByAccountId($accountId, false); - $this->accountToUserRepository - ->addByType($accountId, $accountUpdateDto->usersView); - }, - $this - ); - } - - if (null === $accountUpdateDto->usersEdit) { - $this->accountToUserRepository->deleteTypeByAccountId($accountId, true); - } elseif (!empty($accountUpdateDto->usersEdit)) { - $this->accountToUserRepository->transactionAware( - function () use ($accountUpdateDto, $accountId) { - $this->accountToUserRepository - ->deleteTypeByAccountId($accountId, true); - $this->accountToUserRepository - ->addByType($accountId, $accountUpdateDto->usersEdit, true); - }, - $this - ); - } + // null means the caller said nothing about this list, and an empty array means they + // said to empty it. It used to be the other way round: null deleted every row of that + // type and an empty array matched neither branch and did nothing at all. + // + // Nothing supplies these on the API — there is no parameter for them — so every REST + // account edit silently removed all four kinds of sharing from the account it was + // editing. On the web the form only sets a list when the corresponding `_update` flag + // is posted, which the theme sends only for a select the user actually changed, so an + // edit that touched the name or the URL did the same thing. And the bulk edit's + // "Delete" checkboxes, which post exactly the empty array, were the case that did + // nothing — so the one way to deliberately clear sharing was the one way that never + // worked. + $this->replaceUserGroups($accountId, $accountUpdateDto->userGroupsView, false); + $this->replaceUserGroups($accountId, $accountUpdateDto->userGroupsEdit, true); + $this->replaceUsers($accountId, $accountUpdateDto->usersView, false); + $this->replaceUsers($accountId, $accountUpdateDto->usersEdit, true); } - if (null === $accountUpdateDto->tags) { - $this->accountToTagRepository->deleteByAccountId($accountId); - } elseif (!empty($accountUpdateDto->tags)) { + // Same rule for the tags, which had the same two branches. + if ($accountUpdateDto->tags !== null) { $this->accountToTagRepository->transactionAware( function () use ($accountUpdateDto, $accountId) { $this->accountToTagRepository->deleteByAccountId($accountId); - $this->accountToTagRepository->add($accountId, $accountUpdateDto->tags); + + if (!empty($accountUpdateDto->tags)) { + $this->accountToTagRepository->add($accountId, $accountUpdateDto->tags); + } }, $this ); @@ -141,6 +104,63 @@ function () use ($accountUpdateDto, $accountId) { /** * Adds external items to the account */ + /** + * Replace an account's shared groups of one kind, or leave them alone when none were supplied. + * + * The delete and the add are one transaction, so a failure adding leaves the existing rows + * rather than an account shared with nobody. + * + * @param int[]|null $userGroupIds + * + * @throws ConstraintException + * @throws QueryException + * @throws ServiceException + */ + private function replaceUserGroups(int $accountId, ?array $userGroupIds, bool $isEdit): void + { + if ($userGroupIds === null) { + return; + } + + $this->accountToUserGroupRepository->transactionAware( + function () use ($accountId, $userGroupIds, $isEdit) { + $this->accountToUserGroupRepository->deleteTypeByAccountId($accountId, $isEdit); + + if (!empty($userGroupIds)) { + $this->accountToUserGroupRepository->addByType($accountId, $userGroupIds, $isEdit); + } + }, + $this + ); + } + + /** + * The same for an account's shared users. + * + * @param int[]|null $userIds + * + * @throws ConstraintException + * @throws QueryException + * @throws ServiceException + */ + private function replaceUsers(int $accountId, ?array $userIds, bool $isEdit): void + { + if ($userIds === null) { + return; + } + + $this->accountToUserRepository->transactionAware( + function () use ($accountId, $userIds, $isEdit) { + $this->accountToUserRepository->deleteTypeByAccountId($accountId, $isEdit); + + if (!empty($userIds)) { + $this->accountToUserRepository->addByType($accountId, $userIds, $isEdit); + } + }, + $this + ); + } + public function addItems(bool $userCanChangePermissions, int $accountId, AccountCreateDto $accountCreateDto): void { try { diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php b/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php index 988351347..6f4498486 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php @@ -101,7 +101,11 @@ private function buildAccountUpdateDto(): AccountUpdateDto notes: $this->apiService->getParamString('notes'), isPrivate: (bool)$this->apiService->getParamInt('private'), isPrivateGroup: (bool)$this->apiService->getParamInt('privateGroup'), - tags: array_map(intval(...), $this->apiService->getParamArray('tagsId', false, [])), + // null rather than [], so that an edit which says nothing about tags leaves them + // alone. An empty array now means "clear them", which is what it reads as. + tags: ($tags = $this->apiService->getParamArray('tagsId', false)) === null + ? null + : array_map(intval(...), $tags), ); } } diff --git a/tests/Unit/Application/Account/Services/AccountItemsTest.php b/tests/Unit/Application/Account/Services/AccountItemsTest.php index b3c2018a1..e71327696 100644 --- a/tests/Unit/Application/Account/Services/AccountItemsTest.php +++ b/tests/Unit/Application/Account/Services/AccountItemsTest.php @@ -136,44 +136,65 @@ public function testUpdateItemsWithNoItems() ] ); - $this->accountToUserGroupRepository - ->expects($this->never()) - ->method('transactionAware'); - - $this->accountToUserGroupRepository - ->expects($this->exactly(2)) - ->method('deleteTypeByAccountId') - ->with(...self::withConsecutive([100, false], [100, true])); - - $this->accountToUserGroupRepository - ->expects($this->never()) - ->method('addByType'); + // Nothing at all. null is "the caller said nothing about this list", and an edit that says + // nothing about sharing must leave it alone — it used to delete every row of every type, + // which is what an account edit through the REST API did on every call, there being no + // parameter that could have said otherwise. + foreach ([$this->accountToUserGroupRepository, $this->accountToUserRepository] as $repository) { + $repository->expects($this->never())->method('transactionAware'); + $repository->expects($this->never())->method('deleteTypeByAccountId'); + $repository->expects($this->never())->method('addByType'); + } + + $this->accountToTagRepository->expects($this->never())->method('transactionAware'); + $this->accountToTagRepository->expects($this->never())->method('deleteByAccountId'); + $this->accountToTagRepository->expects($this->never())->method('add'); - $this->accountToUserRepository - ->expects($this->never()) - ->method('transactionAware'); - - $this->accountToUserRepository - ->expects($this->exactly(2)) - ->method('deleteTypeByAccountId') - ->with(...self::withConsecutive([100, false], [100, true])); + $this->accountItems->updateItems(true, 100, $accountUpdateDto); + } - $this->accountToUserRepository - ->expects($this->never()) - ->method('addByType'); + /** + * An empty list clears that kind of sharing. + * + * This is the case the bulk edit's "Delete" checkboxes post, and it used to match neither + * branch and do nothing at all — so the one deliberate way to clear an account's sharing was + * the one way that never worked, while reporting success. + * + * @throws ConstraintException + * @throws QueryException + * @throws ServiceException + */ + public function testUpdateItemsWithEmptyListsClearsThem() + { + $accountUpdateDto = AccountDataGenerator::factory() + ->buildAccountUpdateDto() + ->mutate( + [ + 'usersView' => [], + 'usersEdit' => [], + 'userGroupsView' => [], + 'userGroupsEdit' => [], + 'tags' => [] + ] + ); - $this->accountToTagRepository - ->expects($this->never()) - ->method('transactionAware'); + foreach ([$this->accountToUserGroupRepository, $this->accountToUserRepository] as $repository) { + $repository->expects($this->exactly(2)) + ->method('transactionAware') + ->with(self::withResolveCallableCallback()); + $repository->expects($this->exactly(2)) + ->method('deleteTypeByAccountId') + ->with(...self::withConsecutive([100, false], [100, true])); + // Cleared, not replaced. + $repository->expects($this->never())->method('addByType'); + } $this->accountToTagRepository ->expects($this->once()) - ->method('deleteByAccountId') - ->with(100); - - $this->accountToTagRepository - ->expects($this->never()) - ->method('add'); + ->method('transactionAware') + ->with(self::withResolveCallableCallback()); + $this->accountToTagRepository->expects($this->once())->method('deleteByAccountId')->with(100); + $this->accountToTagRepository->expects($this->never())->method('add'); $this->accountItems->updateItems(true, 100, $accountUpdateDto); }