Remove the avatar when deleting an achievement
What does this MR do and why?
Deleting an achievement, through the achievementsDelete GraphQL mutation or the delete button on a group's achievements page (which sends that mutation), leaves its avatar behind: the Upload record stays in the database and the file stays in storage, as the issue's reproduction shows.
Achievements::DestroyService removed the achievement with achievement.delete, which runs no callbacks. Achievements::Achievement includes Avatarable, which mounts AvatarUploader, and CarrierWave removes a mounted file from an after_commit callback on destroy (remove_avatar!). RecordsUploads deletes the Upload record from that same removal (before :remove, :destroy_upload). With delete, neither ran.
This MR makes the service call achievement.destroy, and answer with an error when the achievement is not destroyed, carrying the errors the record reports or, when it reports none, Failed to delete achievement. That follows Achievements::UpdateService, which answers a failed update with the record's errors. The issue suggests destroy!; I kept the service's ServiceResponse contract instead, so that a destroy something refuses reaches the caller as the mutation's errors rather than as an internal server error. Nothing else runs on destroy: the model has no destroy callbacks and none of its associations declares dependent, and the only commit callback that fires on destroy is remove_avatar! (I listed the chains in the test environment). The achievement's user achievements are still removed by the ON DELETE CASCADE foreign key on user_achievements.achievement_id, as before. The avatar's Geo verification state row in achievement_upload_states, which the issue also lists, goes with the Upload record through the ON DELETE CASCADE foreign key on achievement_upload_id: a throwaway spec that created that row for an avatar's upload showed it staying on master and gone on this branch.
The commits:
- Remove the avatar when deleting an achievement: the service change, with specs that an achievement's avatar
Uploadrecord and file are removed on local storage and on object storage, that another achievement's avatar is kept, and that the service answers with an error and keeps the achievement whendestroyreturns false. It carries theChangelog: fixedtrailer and describes the whole change, so it is the right message if the MR is squashed. - Cover the user achievements removed with an achievement: the issue asks for coverage that awarded and revoked user achievements keep their foreign key behavior, and nothing tested that deleting an achievement removes them while keeping those of another achievement. This pins it, so a later change to how achievements are deleted cannot drop it silently. It passes with the old
deletecall and withdestroy.
I picked this up because my gitlab-mcp-server exposes this mutation to its users as the achievement.delete action.
Not in this MR: avatars already orphaned by earlier deletions stay where they are, and deleting a whole group is a separate path this MR does not change, since the group's achievements are removed by the ON DELETE CASCADE foreign key on achievements.namespace_id, which runs no callbacks. If you want a cleanup for the first or a change to the second, I am happy to open a follow-up.
References
Closes #632251 (closed)
Screenshots or screen recordings
Not a UI change. What is left after achievementsDelete for an achievement with an avatar, from a throwaway request spec (not part of this MR) that sends the mutation:
| Before | After | |
|---|---|---|
| Achievement row | gone | gone |
Upload record (AvatarUploader) |
still there | gone |
| Stored avatar file | still there | gone |
Specs I ran
All on top of master at f967fc7d877, in the test environment, with bundle exec rspec:
- The specs of commit 1 with
app/services/achievements/destroy_service.rbfrommaster: 7 examples, 4 failures, all of them new examples. The two avatar examples (local storage and object storage) fail onexpect(Upload.exists?(upload.id)).to be(false)withgot true; the two error examples fail onexpect(response).to be_errorbecause the service answered success. - Commit 1,
spec/services/achievements/destroy_service_spec.rbandspec/requests/api/graphql/mutations/achievements/delete_spec.rb: 17 examples, 0 failures. - The example of commit 2 with the service from
master: passes. - Commit 2 (the branch), the two files above plus
spec/requests/api/graphql/mutations/achievements/delete_user_achievement_spec.rbandspec/models/achievements/achievement_spec.rb: 47 examples, 0 failures. - RuboCop on the Ruby files of each commit: no offenses. Both commit message linters (
scripts/lint/commit_linter.rband the Danger one) on each commit: no problems.git diff --check: clean. - The throwaway request spec behind the table above sends
achievementsDeletefor an achievement with an avatar as a group Maintainer. Onmasterthe mutation answers with no errors, and theUploadrecord and the file are still there afterwards. On this branch it answers with no errors and both are gone. One visible difference: theavatarUrlthe mutation echoes for the deleted achievement washttp://localhost/uploads/-/system/achievements/achievement/avatar/<id>/dk.png?v=...and is now/uploads/-/system/achievements/achievement/avatar/<id>/dk.png. The issue reports the old URL answering 404 once the achievement is deleted, and on this branch the file behind either URL is gone. The frontend's delete mutation selects onlyachievement { id }anderrors.
How to set up and validate locally
-
As a Maintainer of a group, open
http://gdk.test:3000/groups/<group>/-/achievements, create an achievement and give it an avatar. -
In a Rails console, note its upload and file:
achievement = Achievements::Achievement.last upload = Upload.find_by(model: achievement, uploader: 'AvatarUploader') path = achievement.avatar.path -
Delete the achievement from the same page, or send
mutation { achievementsDelete(input: { achievementId: "gid://gitlab/Achievements::Achievement/<id>" }) { errors } }. -
In the console,
Upload.exists?(upload.id)andFile.exist?(path)were bothtruebefore this MR; with it, both arefalse.
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.