Use discard for soft-deleting users

Gemfile

@@ -54,6 +54,7 @@ gem "bcrypt", "~> 3.1"
 gem "maintenance_tasks", "~> 2.4"
 gem "strong_migrations", "~> 1.7"
 gem "phlex-rails", "~> 1.1"
+gem "discard", "~> 1.3"
 
 # Admin dashboard
 gem "avo", "~> 2.47"

Gemfile.lock

@@ -209,6 +209,8 @@ GEM
       rake (> 10, < 14)
       ruby-statistics (>= 2.1)
       thor (>= 0.19, < 2)
+    discard (1.3.0)
+      activerecord (>= 4.2, < 8)
     docile (1.4.0)
     dogstatsd-ruby (5.6.1)
     domain_name (0.6.20231109)
@@ -749,6 +751,7 @@ DEPENDENCIES
   dartsass-sprockets (~> 3.1)
   ddtrace (~> 1.19)
   derailed_benchmarks (~> 2.1)
+  discard (~> 1.3)
   dogstatsd-ruby (~> 5.5)
   dotenv-rails (~> 2.8)
   factory_bot_rails (~> 6.4)
@@ -898,6 +901,7 @@ CHECKSUMS
   dead_end (4.0.0) sha256=695c8438993bb4c5415b1618a1b6e0afcae849ef2812fb8cb3846723904307eb
   debase-ruby_core_source (3.3.1) sha256=ed904cae290edf0cf274ad707f8981bf1cefad8081e78d4bb71be2a483bc2c08
   derailed_benchmarks (2.1.2) sha256=eaadc6206ceeb5538ff8f5e04a0023d54ebdd95d04f33e8960fb95a5f189a14f
+  discard (1.3.0) sha256=55218997ca4dc11f0594f1bb2d1196c4a959ceb562f1ab6490130233598dda67
   docile (1.4.0) sha256=5f1734bde23721245c20c3d723e76c104208e1aa01277a69901ce770f0ebb8d3
   dogstatsd-ruby (5.6.1) sha256=615dd1328c57ab66fb48cbf83b38ce2060d8353707be0bc3deb0ae960a659982
   domain_name (0.6.20231109) sha256=5127a1521ecca79d54accefc393f6d19db8600c6224416004414f7eaa28aecbe

app/jobs/delete_user_job.rb

@@ -2,74 +2,13 @@ class DeleteUserJob < ApplicationJob
   queue_as :default
   queue_with_priority PRIORITIES.fetch(:profile_deletion)
 
-  ASSOCIATIONS = {
-    approved_ownership_requests: :destroy,
-    closed_ownership_calls: :destroy,
-    closed_ownership_requests: :destroy,
-    oidc_pending_trusted_publishers: :destroy,
-    ownership_calls: :destroy,
-    ownership_requests: :destroy,
-    ownerships: :destroy,
-    subscriptions: :destroy,
-    unconfirmed_ownerships: :destroy,
-    web_hooks: :destroy,
-    webauthn_credentials: :destroy,
-    webauthn_verification: :destroy,
-
-    api_keys: nil,
-    audits: nil,
-    deletions: nil,
-    oidc_api_key_roles: nil,
-    pushed_versions: nil
-  }.freeze
-
   def perform(user:)
-    email = user.email
-    return if user.reload.deleted_at?
-
-    begin
-      user.transaction do
-        user.yank_gems
-        user.api_keys.expire_all!
-        handle_associations(user)
-
-        user.update!(
-          deleted_at: Time.current, email: "deleted+#{user.id}@rubygems.org",
-          handle: nil, email_confirmed: false,
-          unconfirmed_email: nil, blocked_email: nil,
-          api_key: nil, confirmation_token: nil, remember_token: nil,
-          twitter_username: nil, webauthn_id: nil, full_name: nil,
-          totp_seed: nil, mfa_hashed_recovery_codes: nil,
-          mfa_level: :disabled,
-          password: SecureRandom.hex(20).encode("UTF-8")
-        )
-        Mailer.deletion_complete(email).deliver_later
-      end
-    rescue ActiveRecord::ActiveRecordError => e
-      # Catch the exception so we can log it, otherwise using `destroy` would give
-      # us no hint as to why the deletion failed.
-      Rails.error.report(e, context: { user:, email: }, handled: true)
-      Mailer.deletion_failed(email).deliver_later
-    end
-  end
-
-  def handle_associations(user)
-    user.class.reflect_on_all_associations.each do |reflection|
-      next if reflection.through_reflection?
-
-      action = ASSOCIATIONS.fetch(reflection.name)
-      next unless action
-      send(:"#{action}_#{reflection.class.name.tr(':', '_')}", user.association(reflection.name), reflection)
-    end
-  end
-
-  def destroy_ActiveRecord__Reflection__HasManyReflection(association, reflection) # rubocop:disable Naming/MethodName
-    # No point in executing the counter update since we're going to destroy the parent anyway
-    association.load_target.each { |t| t.destroyed_by_association = reflection }
-    association.destroy_all
-  end
-
-  def destroy_ActiveRecord__Reflection__HasOneReflection(association, _reflection) # rubocop:disable Naming/MethodName
-    association.delete
+    return if user.discarded?
+    user.discard!
+  rescue ActiveRecord::ActiveRecordError, Discard::RecordNotDiscarded => e
+    # Catch the exception so we can log it, otherwise using `destroy` would give
+    # us no hint as to why the deletion failed.
+    Rails.error.report(e, context: { user: user.as_json, email: }, handled: true)
+    Mailer.deletion_failed(email).deliver_later
   end
 end

app/models/user.rb

@@ -1,18 +1,27 @@
 class User < ApplicationRecord
   include UserMultifactorMethods
   include Clearance::User
+
   include Gravtastic
   is_gravtastic default: "retro"
 
+  include Discard::Model
+  self.discard_column = :deleted_at
+
   default_scope { not_deleted }
 
   before_save :_generate_confirmation_token_no_reset_unconfirmed_email, if: :will_save_change_to_unconfirmed_email?
   before_create :_generate_confirmation_token_no_reset_unconfirmed_email
+  before_discard :yank_gems
+  before_discard :expire_all_api_keys
+  before_discard :destroy_associations_for_discard
+  before_discard :clear_personal_attributes
   before_destroy :yank_gems
+  after_discard :send_deletion_complete_email
 
-  scope :not_deleted, -> { where(deleted_at: nil) }
-  scope :deleted, -> { with_deleted.where.not(deleted_at: nil) }
-  scope :with_deleted, -> { unscope(where: :deleted_at) }
+  scope :not_deleted, -> { kept }
+  scope :deleted, -> { with_discarded.discarded }
+  scope :with_deleted, -> { with_discarded }
 
   has_many :ownerships, -> { confirmed }, dependent: :destroy, inverse_of: :user
 
@@ -260,13 +269,6 @@ def ld_context
     )
   end
 
-  def yank_gems
-    versions_to_yank = only_owner_gems.map(&:versions).flatten
-    versions_to_yank.each do |v|
-      deletions.create(version: v)
-    end
-  end
-
   private
 
   def update_email
@@ -282,11 +284,51 @@ def unconfirmed_email_exists?
     User.exists?(email: unconfirmed_email)
   end
 
+  def yank_gems
+    versions_to_yank = only_owner_gems.map(&:versions).flatten
+    versions_to_yank.each do |v|
+      deletions.create(version: v)
+    end
+  end
+
   def toxic_email_domain
     return unless (domain = email.split("@").last)
     toxic_domains_path = Pathname.new(Gemcutter::Application.config.toxic_domains_filepath)
     toxic = toxic_domains_path.exist? && toxic_domains_path.readlines.grep(/^#{Regexp.escape(domain)}$/).any?
 
     errors.add(:email, I18n.t("activerecord.errors.messages.blocked", domain: domain)) if toxic
   end
+
+  def expire_all_api_keys
+    api_keys.unexpired.expire_all!
+  end
+
+  def destroy_associations_for_discard
+    ownerships.unscope(where: :confirmed_at).destroy_all
+    ownership_requests.update_all(status: :closed)
+    ownership_calls.unscope(where: :status).destroy_all
+    oidc_pending_trusted_publishers.destroy_all
+    subscriptions.destroy_all
+    web_hooks.destroy_all
+    webauthn_credentials.destroy_all
+    webauthn_verification&.destroy!
+  end
+
+  def clear_personal_attributes
+    @email_before_discard = email
+    update!(
+      email: "deleted+#{id}@rubygems.org",
+      handle: nil, email_confirmed: false,
+      unconfirmed_email: nil, blocked_email: nil,
+      api_key: nil, confirmation_token: nil, remember_token: nil,
+      twitter_username: nil, webauthn_id: nil, full_name: nil,
+      totp_seed: nil, mfa_hashed_recovery_codes: nil,
+      mfa_level: :disabled,
+      password: SecureRandom.hex(20).encode("UTF-8")
+    )
+  end
+
+  def send_deletion_complete_email
+    Mailer.deletion_complete(@email_before_discard).deliver_later
+  end
 end

test/jobs/delete_user_job_test.rb

@@ -141,10 +141,10 @@ class DeleteUserJobTest < ActiveJob::TestCase
     assert_delete user
     assert_deleted open_call
     assert_deleted closed_call
-    assert_deleted open_request
-    assert_deleted approved_request
-    assert_deleted closed_request
     assert_deleted other_request
+    assert_predicate approved_request.reload, :approved?
+    assert_predicate open_request.reload, :closed?
+    assert_predicate closed_request.reload, :closed?
     assert_equal other_call.reload.user, other_user
   end
 
@@ -153,8 +153,9 @@ def assert_delete(user)
     Mailer.expects(:deletion_failed).never
     DeleteUserJob.new(user:).perform(user:)
 
-    refute_predicate user, :destroyed?
+    refute_predicate user.reload, :destroyed?
     assert_predicate user.reload, :deleted_at
+    assert_predicate user.reload, :discarded?
     user.validate!
   end