Report an update that changed nothing instead of reporting success - #719
Merged
Conversation
Six services handed the caller success for an update whose WHERE matched no rows. Editing a category, client, tag, group, preset or authorisation that had been deleted in the meantime answered "updated" — the row was gone, nothing was written, and whoever was editing it was told their change had been saved. Every one of those repositories already counts the affected rows, and the authorisation one goes as far as returning a boolean for exactly this purpose, which the service dropped. UserProfile has always made the check; this is the same check, in the five places it was missing and the one where it was computed and ignored. The group's check is inside its transaction, so the members are not written either. Five tests pinned the old answer as correct — the four API edit-nonexistent cases and the web one, which said in as many words that this was asymmetric with the profile side. They now assert the refusal, and the group service gains a unit test for it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Six services handed the caller success for an update whose
WHEREmatched no rows:CategoryCategory updatedClientClient updatedTagTag updatedUserGroupGroup updatedItemPresetPermission updatedAuthTokenAuthorization updatedEditing one of these after it had been deleted — from another session, or another administrator's window — reported the change as saved. The row was gone, nothing was written, and the person editing it had no way to know.
Every one of those repositories already counts the affected rows, and
AuthTokenRepository::update()goes as far as returninggetAffectedNumRows() === 1for exactly this purpose — which the service dropped on the floor.UserProfileService::update()has always made the check. This is the same check, in the five places it was missing and the one where it was computed and ignored.Details worth knowing
Error while updating the category, …), so nothing needs translating; the authorisation one reusesToken not found, which is what its API answers elsewhere for the same situation.Five tests pinned the old answer
The four API edit a nonexistent id cases and the web one — which said in as many words that this was "asymmetric with the profile side". They now assert the refusal. The group service also gains a unit test: an update of a group that is not there raises, and the members are never touched.
Testing
Both suites green: 3062 unit, 821 integration. PHPStan and PHPCS clean.