FIX: properly clean out post and user actions on destroy user

FIX: properly clean out post and user actions on destroy user

This corrects 2 issues:

First is a regression with d7c08e21 for some reason dependent :delete_all respects default scopes where-as dependent :destroy bypasses it.

Secondly, we were keeping orphan user actions around on user destroy, this ensures we remove all the user actions not only ones that originated by the user.

So for example: if I like a post of user A we create a user action saying I did that, but once user A is deleted we were not removing the action leading to an orphan action in the database.

diff --git a/app/models/user.rb b/app/models/user.rb
index e10ebba..3659d67 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -34,12 +34,9 @@ class User < ActiveRecord::Base
   has_many :topics
   has_many :user_open_ids, dependent: :destroy
 
-  # Do Not Change to user_actions/post_actions to dependent: destroy
-  # users can have lots of actions, bypass tracking of all destroyed objects
-  # this means there is a much higher likelihood that users with lots of state
-  # can be destroyed.
-  has_many :user_actions, dependent: :delete_all
-  has_many :post_actions, dependent: :delete_all
+  # dependent deleting handled via before_destroy
+  has_many :user_actions
+  has_many :post_actions
 
   has_many :user_badges, -> { where('user_badges.badge_id IN (SELECT id FROM badges WHERE enabled)') }, dependent: :destroy
   has_many :badges, through: :user_badges
@@ -136,6 +133,11 @@ class User < ActiveRecord::Base
     # These tables don't have primary keys, so destroying them with activerecord is tricky:
     PostTiming.where(user_id: self.id).delete_all
     TopicViewItem.where(user_id: self.id).delete_all
+    UserAction.where('user_id = :user_id OR target_user_id = :user_id OR acting_user_id = :user_id', user_id: self.id).delete_all
+
+    # we need to bypass the default scope here, which appears not bypassed for :delete_all
+    # however :destroy it is bypassed
+    PostAction.with_deleted.where(user_id: self.id).delete_all
   end
 
   # Skip validating email, for example from a particular auth provider plugin
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 93e9798..1775351 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1966,4 +1966,27 @@ describe User do
       end
     end
   end
+
+  context "#destroy!" do
+    it 'clears up associated data on destroy!' do
+      user = Fabricate(:user)
+      post = Fabricate(:post)
+
+      PostAction.act(user, post, PostActionType.types[:like])
+      PostAction.remove_act(user, post, PostActionType.types[:like])
+
+      UserAction.create!(user_id: user.id, action_type: UserAction::LIKE)
+      UserAction.create!(user_id: -1, action_type: UserAction::LIKE, target_user_id: user.id)
+      UserAction.create!(user_id: -1, action_type: UserAction::LIKE, acting_user_id: user.id)
+
+      user.reload
+
+      user.destroy!
+
+      expect(UserAction.where(user_id: user.id).length).to eq(0)
+      expect(UserAction.where(target_user_id: user.id).length).to eq(0)
+      expect(UserAction.where(acting_user_id: user.id).length).to eq(0)
+      expect(PostAction.with_deleted.where(user_id: user.id).length).to eq(0)
+    end
+  end
 end

GitHub sha: d5ecf8e8

1 Like