Match users better by their private commit email

Private commit emails were introduced in !22560, but some parts of
GitLab were not updated to take account of them. This commit adds
support in places that were missed.

6 files changed, 119 insertions, 44 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 9dd0cbacd9e..546fcc54a15 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -230,24 +230,13 @@ class Commit
 
   def lazy_author
     BatchLoader.for(author_email.downcase).batch do |emails, loader|
-      # A Hash that maps user Emails to the corresponding User objects. The
-      # Emails at this point are the _primary_ Emails of the Users.
-      users_for_emails = User
-        .by_any_email(emails)
-        .each_with_object({}) { |user, hash| hash[user.email] = user }
-
-      users_for_ids = users_for_emails
-        .values
-        .each_with_object({}) { |user, hash| hash[user.id] = user }
-
-      # Some commits may have used an alternative Email address. In this case we
-      # need to query the "emails" table to map those addresses to User objects.
-      Email
-        .where(email: emails - users_for_emails.keys)
-        .pluck(:email, :user_id)
-        .each { |(email, id)| users_for_emails[email] = users_for_ids[id] }
-
-      users_for_emails.each { |email, user| loader.call(email, user) }
+      users = User.by_any_email(emails).includes(:emails)
+
+      emails.each do |email|
+        user = users.find { |u| u.any_email?(email) }
+
+        loader.call(email, user)
+      end
     end
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index a400058e87e..01eba7e0426 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -349,20 +349,28 @@ class User < ActiveRecord::Base
     def find_by_any_email(email, confirmed: false)
       return unless email
 
-      downcased = email.downcase
-
-      find_by_private_commit_email(downcased) || by_any_email(downcased, confirmed: confirmed).take
+      by_any_email(email, confirmed: confirmed).take
     end
 
-    # Returns a relation containing all the users for the given Email address
-    def by_any_email(email, confirmed: false)
-      users = where(email: email)
-      users = users.confirmed if confirmed
+    # Returns a relation containing all the users for the given email addresses
+    #
+    # @param emails [String, Array<String>] email addresses to check
+    # @param confirmed [Boolean] Only return users where the email is confirmed
+    def by_any_email(emails, confirmed: false)
+      emails = Array(emails).map(&:downcase)
+
+      from_users = where(email: emails)
+      from_users = from_users.confirmed if confirmed
 
-      emails = joins(:emails).where(emails: { email: email })
-      emails = emails.confirmed if confirmed
+      from_emails = joins(:emails).where(emails: { email: emails })
+      from_emails = from_emails.confirmed if confirmed
 
-      from_union([users, emails])
+      items = [from_users, from_emails]
+
+      user_ids = Gitlab::PrivateCommitEmail.user_ids_for_emails(emails)
+      items << where(id: user_ids) if user_ids.present?
+
+      from_union(items)
     end
 
     def find_by_private_commit_email(email)
@@ -1031,6 +1039,7 @@ class User < ActiveRecord::Base
   def all_emails
     all_emails = []
     all_emails << email unless temp_oauth_email?
+    all_emails << private_commit_email
     all_emails.concat(emails.map(&:email))
     all_emails
   end
@@ -1043,16 +1052,24 @@ class User < ActiveRecord::Base
     verified_emails
   end
 
+  def any_email?(check_email)
+    downcased = check_email.downcase
+
+    # handle the outdated private commit email case
+    return true if persisted? &&
+        id == Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
+
+    all_emails.include?(check_email.downcase)
+  end
+
   def verified_email?(check_email)
     downcased = check_email.downcase
 
-    if email == downcased
-      primary_email_verified?
-    else
-      user_id = Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
+    # handle the outdated private commit email case
+    return true if persisted? &&
+        id == Gitlab::PrivateCommitEmail.user_id_for_email(downcased)
 
-      user_id == id || emails.confirmed.where(email: downcased).exists?
-    end
+    verified_emails.include?(check_email.downcase)
   end
 
   def hook_attrs
diff --git a/lib/gitlab/private_commit_email.rb b/lib/gitlab/private_commit_email.rb
index bade2248ccd..536fc9dae3a 100644
--- a/lib/gitlab/private_commit_email.rb
+++ b/lib/gitlab/private_commit_email.rb
@@ -18,6 +18,10 @@ module Gitlab
         match[:id].to_i
       end
 
+      def user_ids_for_emails(emails)
+        emails.map { |email| user_id_for_email(email) }.compact.uniq
+      end
+
       def for_user(user)
         hostname = Gitlab::CurrentSettings.current_application_settings.commit_email_hostname
 
diff --git a/spec/lib/gitlab/private_commit_email_spec.rb b/spec/lib/gitlab/private_commit_email_spec.rb
index bc86cd3842a..10bf624bbdd 100644
--- a/spec/lib/gitlab/private_commit_email_spec.rb
+++ b/spec/lib/gitlab/private_commit_email_spec.rb
@@ -4,6 +4,9 @@ require 'spec_helper'
 
 describe Gitlab::PrivateCommitEmail do
   let(:hostname) { Gitlab::CurrentSettings.current_application_settings.commit_email_hostname }
+  let(:id) { 1 }
+  let(:valid_email) { "#{id}-foo@#{hostname}" }
+  let(:invalid_email) { "#{id}-foo@users.noreply.bar.com" }
 
   context '.regex' do
     subject { described_class.regex }
@@ -16,18 +19,25 @@ describe Gitlab::PrivateCommitEmail do
   end
 
   context '.user_id_for_email' do
-    let(:id) { 1 }
-
     it 'parses user id from email' do
-      email = "#{id}-foo@#{hostname}"
-
-      expect(described_class.user_id_for_email(email)).to eq(id)
+      expect(described_class.user_id_for_email(valid_email)).to eq(id)
     end
 
     it 'returns nil on invalid commit email' do
-      email = "#{id}-foo@users.noreply.bar.com"
+      expect(described_class.user_id_for_email(invalid_email)).to be_nil
+    end
+  end
+
+  context '.user_ids_for_email' do
+    it 'returns deduplicated user IDs for each valid email' do
+      result = described_class.user_ids_for_emails([valid_email, valid_email, invalid_email])
+
+      expect(result).to eq([id])
+    end
 
-      expect(described_class.user_id_for_email(email)).to be_nil
+    it 'returns an empty array with no valid emails' do
+      result = described_class.user_ids_for_emails([invalid_email])
+      expect(result).to eq([])
     end
   end
 
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index ed41ff7a0fa..2a0039a0635 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -72,6 +72,7 @@ describe Commit do
     context 'using eager loading' do
       let!(:alice) { create(:user, email: 'alice@example.com') }
       let!(:bob) { create(:user, email: 'hunter2@example.com') }
+      let!(:jeff) { create(:user) }
 
       let(:alice_commit) do
         described_class.new(RepoHelpers.sample_commit, project).tap do |c|
@@ -93,7 +94,14 @@ describe Commit do
         end
       end
 
-      let!(:commits) { [alice_commit, bob_commit, eve_commit] }
+      let(:jeff_commit) do
+        # The commit for Jeff uses his private commit email
+        described_class.new(RepoHelpers.sample_commit, project).tap do |c|
+          c.author_email = jeff.private_commit_email
+        end
+      end
+
+      let!(:commits) { [alice_commit, bob_commit, eve_commit, jeff_commit] }
 
       before do
         create(:email, user: bob, email: 'bob@example.com')
@@ -125,6 +133,20 @@ describe Commit do
         expect(bob_commit.author).to eq(bob)
       end
 
+      it "preloads the authors for Commits using a User's private commit Email" do
+        commits.each(&:lazy_author)
+
+        expect(jeff_commit.author).to eq(jeff)
+      end
+
+      it "preloads the authors for Commits using a User's outdated private commit Email" do
+        jeff.update!(username: 'new-username')
+
+        commits.each(&:lazy_author)
+
+        expect(jeff_commit.author).to eq(jeff)
+      end
+
       it 'sets the author to Nil if an author could not be found for a Commit' do
         commits.each(&:lazy_author)
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 0ac5bd666ae..733c1c49f08 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1174,6 +1174,22 @@ describe User do
 
       expect(described_class.by_any_email(user.email, confirmed: true)).to eq([user])
     end
+
+    it 'finds user through a private commit email' do
+      user = create(:user)
+      private_email = user.private_commit_email
+
+      expect(described_class.by_any_email(private_email)).to eq([user])
+      expect(described_class.by_any_email(private_email, confirmed: true)).to eq([user])
+    end
+
+    it 'finds user through a private commit email in an array' do
+      user = create(:user)
+      private_email = user.private_commit_email
+
+      expect(described_class.by_any_email([private_email])).to eq([user])
+      expect(described_class.by_any_email([private_email], confirmed: true)).to eq([user])
+    end
   end
 
   describe '.search' do
@@ -1501,7 +1517,12 @@ describe User do
       email_unconfirmed = create :email, user: user
       user.reload
 
-      expect(user.all_emails).to match_array([user.email, email_unconfirmed.email, email_confirmed.email])
+      expect(user.all_emails).to contain_exactly(
+        user.email,
+        user.private_commit_email,
+        email_unconfirmed.email,
+        email_confirmed.email
+      )
     end
   end
 
@@ -1512,7 +1533,11 @@ describe User do
       email_confirmed = create :email, user: user, confirmed_at: Time.now
       create :email, user: user
 
-      expect(user.verified_emails).to match_array([user.email, user.private_commit_email, email_confirmed.email])
+      expect(user.verified_emails).to contain_exactly(
+        user.email,
+        user.private_commit_email,
+        email_confirmed.email
+      )
     end
   end
 
@@ -1532,6 +1557,14 @@ describe User do
       expect(user.verified_email?(user.private_commit_email)).to be_truthy
     end
 
+    it 'returns true for an outdated private commit email' do
+      old_email = user.private_commit_email
+
+      user.update!(username: 'changed-username')
+
+      expect(user.verified_email?(old_email)).to be_truthy
+    end
+
     it 'returns false when the email is not verified/confirmed' do
       email_unconfirmed = create :email, user: user
       user.reload