Skip to content

fix: an edit that says nothing about sharing leaves it alone #875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blaipr merged 1 commit into from
Aug 24, 2026
Merged

fix: an edit that says nothing about sharing leaves it alone #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 79 additions & 59 deletions src/Application/Account/Services/AccountItems.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
Expand All @@ -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 {
Expand Down
6 changes: 5 additions & 1 deletion src/Infrastructure/Adapter/In/Api/Controllers/Account/EditController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
}
}
85 changes: 53 additions & 32 deletions tests/Unit/Application/Account/Services/AccountItemsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down