From d6c63cc5b2de7dc3e5116a5737015409ec236d8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 14 Jun 2017 19:20:18 +0200 Subject: [PATCH] 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 :shrug: --- .../regular/automatic_group_membership.rb | 6 ++-- app/models/discourse_single_sign_on.rb | 1 + app/models/email_token.rb | 2 +- app/models/user.rb | 34 +++++++++---------- spec/jobs/automatic_group_membership_spec.rb | 14 ++++---- spec/models/user_spec.rb | 20 ++--------- 6 files changed, 32 insertions(+), 45 deletions(-) diff --git a/app/jobs/regular/automatic_group_membership.rb b/app/jobs/regular/automatic_group_membership.rb index dbe679889b0..91a417927a9 100644 --- a/app/jobs/regular/automatic_group_membership.rb +++ b/app/jobs/regular/automatic_group_membership.rb @@ -18,9 +18,9 @@ module Jobs .activated .where(staged: false) .find_each do |user| - - group.add(user) - GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user) + next unless user.email_confirmed? + group.add(user) + GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user) end Group.reset_counters(group.id, :group_users) diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index 93c879b5dff..267ef981410 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/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| diff --git a/app/models/email_token.rb b/app/models/email_token.rb index 820ea078f9b..225e083b54d 100644 --- a/app/models/email_token.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 33b3d0b6eb9..659fd7aa09a 100644 --- a/app/models/user.rb +++ b/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 diff --git a/spec/jobs/automatic_group_membership_spec.rb b/spec/jobs/automatic_group_membership_spec.rb index 5989deb4898..55776b99e42 100644 --- a/spec/jobs/automatic_group_membership_spec.rb +++ b/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") - user2 = Fabricate(:user, email: "foo@bar.com") - user3 = Fabricate(:user, email: "bar@wat.com", staged: true) - user4 = Fabricate(:user, email: "abc@wat.com", active: false) + user1 = Fabricate(:user, email: "no@bar.com") + user2 = Fabricate(:user, email: "no@wat.com") + user3 = Fabricate(:user, email: "noo@wat.com", staged: true) + 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cf0bd01d015..7e2545e0e1d 100644 --- a/spec/models/user_spec.rb +++ b/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(email_token) + EmailToken.confirm(user.email_tokens.last.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(email_token) + EmailToken.confirm(user.email_tokens.last.token) user.reload expect(user.title).to eq("bars and wats")