FIX: user's default group should only be set once

Setting a user's default groups based on their email address should only be done once, ie. when they confirm their email address.
Previously we were doing this everytime we'd save a user record 🤷

app/jobs/regular/automatic_group_membership.rb

@@ -18,9 +18,9 @@ module Jobs
           .activated
           .where(staged: false)
           .find_each do |user|
-
+        next unless user.email_confirmed?
-          group.add(user)
+        group.add(user)
-          GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
+        GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
       end
 
       Group.reset_counters(group.id, :group_users)

app/models/discourse_single_sign_on.rb

@@ -68,6 +68,7 @@ class DiscourseSingleSignOn < SingleSignOn
       user.active = true
       user.save!
       user.enqueue_welcome_message('welcome_user') unless suppress_welcome_message
+      user.set_automatic_groups
     end
 
     custom_fields.each do |k,v|

app/models/email_token.rb

@@ -64,10 +64,10 @@ class EmailToken < ActiveRecord::Base
       if result[:success]
         # If we are activating the user, send the welcome message
         user.send_welcome_message = !user.active?
-
         user.active = true
         user.email = result[:email_token].email
         user.save!
+        user.set_automatic_groups
       end
 
       if user

app/models/user.rb

@@ -98,7 +98,6 @@ class User < ActiveRecord::Base
   before_save :ensure_password_is_hashed
 
   after_save :expire_tokens_if_password_changed
-  after_save :automatic_group_membership
   after_save :clear_global_notice_if_needed
   after_save :refresh_avatar
   after_save :badge_grant
@@ -919,6 +918,22 @@ class User < ActiveRecord::Base
     end
   end
 
+  def set_automatic_groups
+    return unless active && email_confirmed? && !staged
+
+    Group.where(automatic: false)
+         .where("LENGTH(COALESCE(automatic_membership_email_domains, '')) > 0")
+         .each do |group|
+
+      domains = group.automatic_membership_email_domains.gsub('.', '\.')
+
+      if email =~ Regexp.new("@(#{domains})$", true) && !group.users.include?(self)
+        group.add(self)
+        GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(self)
+      end
+    end
+  end
+
   protected
 
   def badge_grant
@@ -948,23 +963,6 @@ class User < ActiveRecord::Base
     Group.user_trust_level_change!(id, trust_level)
   end
 
-  def automatic_group_membership
-    user = User.find(self.id)
-    return unless user && user.active && user.email_confirmed? && !user.staged
-
-    Group.where(automatic: false)
-         .where("LENGTH(COALESCE(automatic_membership_email_domains, '')) > 0")
-         .each do |group|
-
-      domains = group.automatic_membership_email_domains.gsub('.', '\.')
-
-      if user.email =~ Regexp.new("@(#{domains})$", true) && !group.users.include?(user)
-        group.add(user)
-        GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
-      end
-    end
-  end
-
   def create_user_stat
     stat = UserStat.new(new_since: Time.now)
     stat.user_id = id

spec/jobs/automatic_group_membership_spec.rb

@@ -8,19 +8,21 @@ describe Jobs::AutomaticGroupMembership do
   end
 
   it "updates the membership" do
-    user1 = Fabricate(:user, email: "foo@wat.com")
+    user1 = Fabricate(:user, email: "no@bar.com")
-    user2 = Fabricate(:user, email: "foo@bar.com")
+    user2 = Fabricate(:user, email: "no@wat.com")
-    user3 = Fabricate(:user, email: "bar@wat.com", staged: true)
+    user3 = Fabricate(:user, email: "noo@wat.com", staged: true)
-    user4 = Fabricate(:user, email: "abc@wat.com", active: false)
+    user4 = Fabricate(:user, email: "yes@wat.com")
+    EmailToken.confirm(user4.email_tokens.last.token)
+
     group = Fabricate(:group, automatic_membership_email_domains: "wat.com", automatic_membership_retroactive: true)
 
     Jobs::AutomaticGroupMembership.new.execute(group_id: group.id)
 
     group.reload
-    expect(group.users.include?(user1)).to eq(true)
+    expect(group.users.include?(user1)).to eq(false)
     expect(group.users.include?(user2)).to eq(false)
     expect(group.users.include?(user3)).to eq(false)
-    expect(group.users.include?(user4)).to eq(false)
+    expect(group.users.include?(user4)).to eq(true)
   end
 
 end

spec/models/user_spec.rb

@@ -1220,29 +1220,16 @@ describe User do
       )
     }
 
-    it "doesn't automatically add inactive users" do
-      inactive_user = Fabricate(:user, active: false, email: "wat@wat.com")
-      group.reload
-      expect(group.users.include?(inactive_user)).to eq(false)
-    end
-
-    it "doesn't automatically add users with unconfirmed email" do
-      unconfirmed_email_user = Fabricate(:user, active: true, email: "wat@wat.com")
-      unconfirmed_email_user.email_tokens.create(email: unconfirmed_email_user.email)
-      group.reload
-      expect(group.users.include?(unconfirmed_email_user)).to eq(false)
-    end
-
     it "doesn't automatically add staged users" do
       staged_user = Fabricate(:user, active: true, staged: true, email: "wat@wat.com")
+      EmailToken.confirm(staged_user.email_tokens.last.token)
       group.reload
       expect(group.users.include?(staged_user)).to eq(false)
     end
 
     it "is automatically added to a group when the email matches" do
       user = Fabricate(:user, active: true, email: "foo@bar.com")
-      email_token = user.email_tokens.create(email: user.email).token
+      EmailToken.confirm(user.email_tokens.last.token)
-      EmailToken.confirm(email_token)
       group.reload
       expect(group.users.include?(user)).to eq(true)
 
@@ -1263,8 +1250,7 @@ describe User do
 
       user.password_required!
       user.save!
-      email_token = user.email_tokens.create(email: user.email).token
+      EmailToken.confirm(user.email_tokens.last.token)
-      EmailToken.confirm(email_token)
       user.reload
 
       expect(user.title).to eq("bars and wats")