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:

  1. Remove the avatar when deleting an achievement: the service change, with specs that an achievement's avatar Upload record 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 when destroy returns false. It carries the Changelog: fixed trailer and describes the whole change, so it is the right message if the MR is squashed.
  2. 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 delete call and with destroy.

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.rb from master: 7 examples, 4 failures, all of them new examples. The two avatar examples (local storage and object storage) fail on expect(Upload.exists?(upload.id)).to be(false) with got true; the two error examples fail on expect(response).to be_error because the service answered success.
  • Commit 1, spec/services/achievements/destroy_service_spec.rb and spec/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.rb and spec/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.rb and the Danger one) on each commit: no problems. git diff --check: clean.
  • The throwaway request spec behind the table above sends achievementsDelete for an achievement with an avatar as a group Maintainer. On master the mutation answers with no errors, and the Upload record and the file are still there afterwards. On this branch it answers with no errors and both are gone. One visible difference: the avatarUrl the mutation echoes for the deleted achievement was http://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 only achievement { id } and errors.

How to set up and validate locally

  1. As a Maintainer of a group, open http://gdk.test:3000/groups/<group>/-/achievements, create an achievement and give it an avatar.

  2. 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
  3. Delete the achievement from the same page, or send mutation { achievementsDelete(input: { achievementId: "gid://gitlab/Achievements::Achievement/<id>" }) { errors } }.

  4. In the console, Upload.exists?(upload.id) and File.exist?(path) were both true before this MR; with it, both are false.

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.

Merge request reports

Loading
Loading