From a2842241d4f583625053aba4d2c37a2f15b21913 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 3 Jan 2019 10:59:14 +0000 Subject: Add committers and authors methods on MergeRequest These are used by the EE-only approvers feature --- app/models/commit_collection.rb | 6 +++++ app/models/merge_request.rb | 24 +++++++++++++------ .../merge_requests/creations_controller_spec.rb | 2 +- spec/models/commit_collection_spec.rb | 25 ++++++++++++++++++- spec/models/merge_request_spec.rb | 28 ++++++++++++++++++++++ .../merge_requests/refresh_service_spec.rb | 2 +- 6 files changed, 77 insertions(+), 10 deletions(-) diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb index e349f0fe971..885f61beb05 100644 --- a/app/models/commit_collection.rb +++ b/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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 613860ec31a..5310f2ee765 100644 --- a/app/models/merge_request.rb +++ b/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 - compare_commits.reverse - else - [] - end + 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 diff --git a/spec/controllers/projects/merge_requests/creations_controller_spec.rb b/spec/controllers/projects/merge_requests/creations_controller_spec.rb index ac93393ac3a..f031a74c5bd 100644 --- a/spec/controllers/projects/merge_requests/creations_controller_spec.rb +++ b/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) diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb index 066fe7d154e..005005b236b 100644 --- a/spec/models/commit_collection_spec.rb +++ b/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( diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 96d49e86dab..e18b29df321 100644 --- a/spec/models/merge_request_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 1d9c75dedce..1169ed5f9f2 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/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 -- cgit v1.2.1