FIX: use active record `update_attribute` instead of mini sql. (#14367)

FIX: use active record update_attribute instead of mini sql. (#14367)

  • DEV: use active record save! instead of mini sql.

The “save” method will trigger the before_save callback “match_primary_group_changes” for User model. Else flair_group_id won’t be removed from the user.

  • check whether the method match_primary_group_changes called or not.
diff --git a/app/models/group.rb b/app/models/group.rb
index 4742e4b..374058f 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -692,7 +692,6 @@ class Group < ActiveRecord::Base
     has_webhooks = WebHook.active_web_hooks(:group_user)
     payload = WebHook.generate_payload(:group_user, group_user, WebHookGroupUserSerializer) if has_webhooks
     group_user.destroy
-    user.update_attribute(:primary_group_id, nil) if user.primary_group_id == self.id
     DiscourseEvent.trigger(:user_removed_from_group, user, self)
     WebHook.enqueue_hooks(:group_user, :user_removed_from_group,
       id: group_user.id,
diff --git a/app/models/group_user.rb b/app/models/group_user.rb
index 521709f..c7277e5 100644
--- a/app/models/group_user.rb
+++ b/app/models/group_user.rb
@@ -83,12 +83,9 @@ class GroupUser < ActiveRecord::Base
   end
 
   def remove_primary_group
-    DB.exec("
-      UPDATE users
-      SET primary_group_id = NULL
-      WHERE id = :user_id AND primary_group_id = :id",
-      id: group.id, user_id: user_id
-    )
+    return if user.primary_group_id != group_id
+    return if self.destroyed_by_association&.active_record == User # User is being destroyed, so don't try to update
+    user.update_attribute(:primary_group_id, nil)
   end
 
   def grant_other_available_title
diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb
index e6b3e55..cee19b1 100644
--- a/spec/models/group_user_spec.rb
+++ b/spec/models/group_user_spec.rb
@@ -224,4 +224,19 @@ describe GroupUser do
       expect(group.group_users.find_by(user_id: user_2.id).first_unread_pm_at).to eq_time(10.minutes.ago)
     end
   end
+
+  describe '#destroy!' do
+    fab!(:group) { Fabricate(:group) }
+
+    it "removes `primary_group_id` and exec `match_primary_group_changes` method on user model" do
+      user = Fabricate(:user, primary_group: group)
+      group_user = Fabricate(:group_user, group: group, user: user)
+
+      user.expects(:match_primary_group_changes).once
+      group_user.destroy!
+
+      user.reload
+      expect(user.primary_group_id).to be_nil
+    end
+  end
 end

GitHub sha: 28be284b27b8addd0d73f4f8efce438ae4a45a0d

This commit appears in #14367 which was approved by eviltrout. It was merged by tgxworld.