Rewrite deleting again

app/jobs/delete_user_job.rb

@@ -2,6 +2,27 @@ 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?
@@ -10,13 +31,8 @@ def perform(user:)
       user.transaction do
         user.yank_gems
         user.api_keys.expire_all!
-        user.class.reflect_on_all_associations.each do |association|
-          next if association.name == :api_keys
-          case association.options[:dependent]
-          when :destroy, :destroy_async
-            user.association(association.name).handle_dependency
-          end
-        end
+        handle_associations(user)
+
         user.update!(
           deleted_at: Time.current, email: "deleted+#{user.id}@rubygems.org",
           handle: nil, email_confirmed: false,
@@ -36,4 +52,26 @@ def perform(user:)
       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) do
+        raise ActiveRecord::DeleteRestrictionError, reflection.name
+      end
+      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
+  end
 end

test/integration/password_reset_test.rb

@@ -216,6 +216,15 @@ def forgot_password_with(email)
     assert_equal email, @user.email
   end
 
+  test "resetting password of soft-deleted user" do
+    @user.update!(deleted_at: Time.zone.now, email: "deleted+#{@user.id}@rubygems.org")
+
+    forgot_password_with @user.email
+
+    assert_empty ActionMailer::Base.deliveries
+    page.assert_text "instructions for changing your password"
+  end
+
   teardown do
     @authenticator&.remove!
     Capybara.reset_sessions!

test/jobs/delete_user_job_test.rb

@@ -9,6 +9,9 @@ class DeleteUserJobTest < ActiveJob::TestCase
     assert_delete user
     assert_predicate version.reload, :yanked?
     refute_predicate rubygem.reload, :indexed?
+
+    rubygem.validate!
+    version.validate!
   end
 
   test "sends deletion failed on failure" do
@@ -36,11 +39,14 @@ class DeleteUserJobTest < ActiveJob::TestCase
     assert_delete user
     assert_predicate user.reload, :deleted_at
     assert_predicate api_key.reload, :expired?
+    assert_equal version.reload.pusher_api_key, api_key
+
     User.unscoped do
       assert_equal api_key.reload.owner, user
       assert_equal version.reload.pusher, user
     end
-    assert_equal version.reload.pusher_api_key, api_key
+
+    version.validate!
   end
 
   test "succeeds with version deletion" do
@@ -53,16 +59,24 @@ class DeleteUserJobTest < ActiveJob::TestCase
     assert_delete user
     assert_predicate user.reload, :deleted_at
     assert_predicate api_key.reload, :expired?
-    User.unscoped do
-      assert_equal api_key.reload.owner, user
-      assert_equal version.reload.pusher, user
-      assert_equal deletion.reload.user, user
-    end
+    assert_nil api_key.reload.owner
+    assert_nil version.reload.pusher
+    assert_nil deletion.reload.user
+
     assert_equal version.reload.pusher_api_key, api_key
     assert_empty rubygem.reload.owners
     assert_empty user.ownerships
     assert_predicate version.reload, :yanked?
     refute_predicate rubygem.reload, :indexed?
+
+    User.unscoped do
+      assert_equal api_key.reload.owner, user
+      assert_equal version.reload.pusher, user
+      assert_equal deletion.reload.user, user
+    end
+
+    version.validate!
+    deletion.validate!
   end
 
   test "succeeds with rubygem with shared ownership" do
@@ -80,6 +94,8 @@ class DeleteUserJobTest < ActiveJob::TestCase
       assert_equal api_key.reload.owner, user
       assert_equal version.reload.pusher, user
     end
+    assert_nil api_key.reload.owner
+    assert_nil version.reload.pusher
     assert_equal version.reload.pusher_api_key, api_key
     assert_equal [other_user], rubygem.reload.owners
     assert_empty user.ownerships
@@ -92,9 +108,11 @@ class DeleteUserJobTest < ActiveJob::TestCase
   test "succeeds with webauthn credentials" do
     user = create(:user)
     user_credential = create(:webauthn_credential, user: user)
+    webauthn_verification = create(:webauthn_verification, user: user)
 
     assert_delete user
     assert_deleted user_credential
+    assert_deleted webauthn_verification
   end
 
   test "succeeds with subscription" do
@@ -132,15 +150,15 @@ class DeleteUserJobTest < ActiveJob::TestCase
 
   def assert_delete(user)
     Mailer.expects(:deletion_complete).with(user.email).returns(mock(deliver_later: nil))
+    Mailer.expects(:deletion_failed).never
     DeleteUserJob.new(user:).perform(user:)
 
     refute_predicate user, :destroyed?
     assert_predicate user.reload, :deleted_at
+    user.validate!
   end
 
   def assert_deleted(record)
-    error = assert_raises(ActiveRecord::RecordNotFound) { assert_predicate record.reload, :destroyed? }
-
-    assert_equal record.class.name, error.model, "Expected #{record.class} to be class of not found error"
+    refute record.class.unscoped.exists?(record.id), "Expected #{record.class} with id #{record.id} to be deleted"
   end
 end

test/models/parallel_pusher_test.rb

@@ -13,7 +13,6 @@ class ParallelPusherTest < ActiveSupport::TestCase
 
     teardown do
       @user.destroy!
-      @api_key.destroy!
       Rubygem.find_by(name: "hola").destroy!
       GemDownload.delete_all
       RubygemFs.mock!