Merge branch '3845-committers-cannot-approve-mr-ce' into 'master'

Add committers and authors methods on MergeRequest

See merge request gitlab-org/gitlab-ce!24204

app/models/commit_collection.rb

@@ -19,6 +19,12 @@ class CommitCollection
     commits.each(&block)
   end
 
+  def committers
+    emails = commits.reject(&:merge_commit?).map(&:committer_email).uniq
+
+    User.by_any_email(emails)
+  end
+
   # Sets the pipeline status for every commit.
   #
   # Setting this status ahead of time removes the need for running a query for

app/models/merge_request.rb

@@ -284,6 +284,14 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  def committers
+    @committers ||= commits.committers
+  end
+
+  def authors
+    User.from_union([committers, User.where(id: self.author_id)])
+  end
+
   # Verifies if title has changed not taking into account WIP prefix
   # for merge requests.
   def wipless_title_changed(old_title)
@@ -327,13 +335,15 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def commits
-    if persisted?
-      merge_request_diff.commits
-    elsif compare_commits
+    return merge_request_diff.commits if persisted?
+
+    commits_arr = if compare_commits
                     compare_commits.reverse
                   else
                     []
                   end
+
+    CommitCollection.new(source_project, commits_arr, source_branch)
   end
 
   def commits_count

spec/controllers/projects/merge_requests/creations_controller_spec.rb

@@ -71,7 +71,7 @@ describe Projects::MergeRequests::CreationsController do
         expect(response).to be_success
 
         total = assigns(:total_commit_count)
-        expect(assigns(:commits)).to be_an Array
+        expect(assigns(:commits)).to be_an CommitCollection
         expect(total).to be > 0
         expect(assigns(:hidden_commit_count)).to eq(0)
         expect(response).to have_gitlab_http_status(200)

spec/models/commit_collection_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe CommitCollection do
   let(:project) { create(:project, :repository) }
-  let(:commit) { project.commit }
+  let(:commit) { project.commit("c1c67abbaf91f624347bb3ae96eabe3a1b742478") }
 
   describe '#each' do
     it 'yields every commit' do
@@ -12,6 +12,29 @@ describe CommitCollection do
     end
   end
 
+  describe '.committers' do
+    it 'returns a relation of users when users are found' do
+      user = create(:user, email: commit.committer_email.upcase)
+      collection = described_class.new(project, [commit])
+
+      expect(collection.committers).to contain_exactly(user)
+    end
+
+    it 'returns empty array when committers cannot be found' do
+      collection = described_class.new(project, [commit])
+
+      expect(collection.committers).to be_empty
+    end
+
+    it 'excludes authors of merge commits' do
+      commit = project.commit("60ecb67744cb56576c30214ff52294f8ce2def98")
+      create(:user, email: commit.committer_email.upcase)
+      collection = described_class.new(project, [commit])
+
+      expect(collection.committers).to be_empty
+    end
+  end
+
   describe '#with_pipeline_status' do
     it 'sets the pipeline status for every commit so no additional queries are necessary' do
       create(

spec/models/merge_request_spec.rb

@@ -991,6 +991,34 @@ describe MergeRequest do
     end
   end
 
+  describe '#committers' do
+    it 'returns all the committers of every commit in the merge request' do
+      users = subject.commits.map(&:committer_email).uniq.map do |email|
+        create(:user, email: email)
+      end
+
+      expect(subject.committers).to match_array(users)
+    end
+
+    it 'returns an empty array if no committer is associated with a user' do
+      expect(subject.committers).to be_empty
+    end
+  end
+
+  describe '#authors' do
+    it 'returns a list with all the committers in the merge request and author' do
+      users = subject.commits.map(&:committer_email).uniq.map do |email|
+        create(:user, email: email)
+      end
+
+      expect(subject.authors).to match_array([subject.author, *users])
+    end
+
+    it 'returns only the author if no committer is associated with a user' do
+      expect(subject.authors).to contain_exactly(subject.author)
+    end
+  end
+
   describe '#hook_attrs' do
     it 'delegates to Gitlab::HookData::MergeRequestBuilder#build' do
       builder = double

spec/services/merge_requests/refresh_service_spec.rb

@@ -509,7 +509,7 @@ describe MergeRequests::RefreshService do
           committed_date: Time.now
         )
 
-        allow_any_instance_of(MergeRequest).to receive(:commits).and_return([commit])
+        allow_any_instance_of(MergeRequest).to receive(:commits).and_return(CommitCollection.new(@project, [commit], 'feature'))
       end
 
       context 'when the merge request is sourced from the same project' do