Merge pull request #5013 from LeoMcA/alternate-emails-phase-1.5

FIX: add additional email to tests and clean up resulting mess

app/controllers/admin/groups_controller.rb

@@ -37,6 +37,7 @@ class Admin::GroupsController < Admin::AdminController
         valid_emails[email] = valid_usernames[username_lower] = id
         id
       end
+      valid_users.uniq!
       invalid_users = users.reject! { |u| valid_emails[u] || valid_usernames[u] }
       group.bulk_add(valid_users) if valid_users.present?
       users_added = valid_users.count

app/jobs/scheduled/pending_users_reminder.rb

@@ -12,7 +12,7 @@ module Jobs
           query = query.where('users.created_at < ?', SiteSetting.pending_users_reminder_delay.hours.ago)
         end
 
-        newest_username = query.limit(1).pluck(:username).first
+        newest_username = query.limit(1).select(:username).first&.username
 
         return true if newest_username == previous_newest_username # already notified
 

lib/admin_user_index_query.rb

@@ -4,7 +4,8 @@ class AdminUserIndexQuery
 
   def initialize(params = {}, klass = User, trust_levels = TrustLevel.levels)
     @params = params
-    @query = initialize_query_with_order(klass).joins(:user_emails)
+    @outer_query = initialize_query_with_order(klass)
+    @query = klass.joins(:user_emails).distinct
     @trust_levels = trust_levels
   end
 
@@ -134,7 +135,7 @@ class AdminUserIndexQuery
     append filter_by_ip
     append filter_exclude
     append filter_by_search
-    @query
+    @outer_query.from(@query, 'users')
   end
 
 end

spec/fabricators/user_email_fabricator.rb

@@ -2,3 +2,8 @@ Fabricator(:user_email) do
   email { sequence(:email) { |i| "bruce#{i}@wayne.com" } }
   primary true
 end
+
+Fabricator(:alternate_email, from: :user_email) do
+  email { sequence(:email) { |i| "bwayne#{i}@wayne.com" } }
+  primary false
+end

spec/fabricators/user_fabricator.rb

@@ -1,7 +1,7 @@
 Fabricator(:user_stat) do
 end
 
-Fabricator(:user) do
+Fabricator(:user_single_email, class_name: :user) do
   name 'Bruce Wayne'
   username { sequence(:username) { |i| "bruce#{i}" } }
   email { sequence(:email) { |i| "bruce#{i}@wayne.com" } }
@@ -11,6 +11,10 @@ Fabricator(:user) do
   active true
 end
 
+Fabricator(:user, from: :user_single_email) do
+  after_create { |user| Fabricate(:alternate_email, user: user)  }
+end
+
 Fabricator(:coding_horror, from: :user) do
   name 'Coding Horror'
   username 'CodingHorror'

spec/models/user_spec.rb

@@ -615,7 +615,7 @@ describe User do
 
     it 'email whitelist should be used when email is being changed' do
       SiteSetting.email_domains_whitelist = 'vaynermedia.com'
-      u = Fabricate(:user, email: 'good@vaynermedia.com')
+      u = Fabricate(:user_single_email, email: 'good@vaynermedia.com')
       u.email = 'nope@mailinator.com'
       expect(u).not_to be_valid
     end

spec/services/user_destroyer_spec.rb

@@ -53,10 +53,10 @@ describe UserDestroyer do
         destroy
       end
 
-      it "adds email to block list if block_email is true" do
+      it "adds emails to block list if block_email is true" do
         expect {
           UserDestroyer.new(@admin).destroy(@user, destroy_opts.merge(block_email: true))
-        }.to change { ScreenedEmail.count }.by(1)
+        }.to change { ScreenedEmail.count }.by(2)
       end
     end