From e0f596c99debe880dfcaed66220a9483d0c81e7b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 19 Sep 2016 23:51:27 +0800 Subject: [PATCH 1/8] Show all pipelines from all merge_request_diffs: This way we could also show pipelines from commits which were discarded due to a force push. --- app/models/merge_request.rb | 14 +++++++++++--- app/models/merge_request_diff.rb | 8 ++++++++ spec/models/merge_request_spec.rb | 31 +++++++++++++++++++++++++------ 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 75f48fd4ba5..4532e7df7b5 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -745,10 +745,18 @@ class MergeRequest < ActiveRecord::Base end def all_pipelines - @all_pipelines ||= - if diff_head_sha && source_project - source_project.pipelines.order(id: :desc).where(sha: commits_sha, ref: source_branch) + return unless source_project + + @all_pipelines ||= begin + if persisted? + sha = merge_request_diffs.flat_map(&:commits_sha).uniq + else + sha = diff_head_sha end + + source_project.pipelines.order(id: :desc). + where(sha: sha, ref: source_branch) + end end def merge_commit diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 18c583add88..7362886e9f5 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -117,6 +117,14 @@ class MergeRequestDiff < ActiveRecord::Base project.commit(head_commit_sha) end + def commits_sha + if @commits + commits.map(&:sha) + else + st_commits.map { |commit| commit[:id] } + end + end + def diff_refs return unless start_commit_sha || base_commit_sha diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 06feeb1bbba..fb599e1207d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -495,15 +495,34 @@ describe MergeRequest, models: true do end describe '#all_pipelines' do - let!(:pipelines) do - subject.merge_request_diff.commits.map do |commit| - create(:ci_empty_pipeline, project: subject.source_project, sha: commit.id, ref: subject.source_branch) + shared_examples 'returning pipelines with proper ordering' do + let!(:pipelines) do + subject.merge_request_diffs.flat_map do |diff| + diff.commits.map do |commit| + create(:ci_empty_pipeline, + project: subject.source_project, + sha: commit.id, + ref: subject.source_branch) + end + end + end + + it 'returns all pipelines' do + expect(subject.all_pipelines).not_to be_empty + expect(subject.all_pipelines).to eq(pipelines.reverse) end end - it 'returns a pipelines from source projects with proper ordering' do - expect(subject.all_pipelines).not_to be_empty - expect(subject.all_pipelines).to eq(pipelines.reverse) + context 'with single merge_request_diffs' do + it_behaves_like 'returning pipelines with proper ordering' + end + + context 'with multiple irrelevant merge_request_diffs' do + before do + subject.update(target_branch: 'markdown') + end + + it_behaves_like 'returning pipelines with proper ordering' end end From 54483462018a27506d826a96b3f7ee3166fb6c7e Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Sep 2016 01:30:13 +0800 Subject: [PATCH 2/8] Add an entry to CHANGELOG [ci skip] --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index a1217673c12..4cc663d9070 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -33,6 +33,7 @@ v 8.12.0 (unreleased) - Fix file permissions change when updating a file on the Gitlab UI !5979 - Change merge_error column from string to text type - Reduce contributions calendar data payload (ClemMakesApps) + - Show all pipelines for merge requests even from discarded commits !6414 - Replace contributions calendar timezone payload with dates (ClemMakesApps) - Add `web_url` field to issue, merge request, and snippet API objects (Ben Boeckel) - Enable pipeline events by default !6278 From 307be4b52145d271cfddfbbd91b8bd2280ce31ae Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Sep 2016 17:36:21 +0800 Subject: [PATCH 3/8] Introduce MergeRequest#all_commits_sha, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6414#note_15746083 --- app/models/merge_request.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4532e7df7b5..5af01846699 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -747,15 +747,16 @@ class MergeRequest < ActiveRecord::Base def all_pipelines return unless source_project - @all_pipelines ||= begin - if persisted? - sha = merge_request_diffs.flat_map(&:commits_sha).uniq - else - sha = diff_head_sha - end + @all_pipelines ||= source_project.pipelines.order(id: :desc). + where(sha: all_commits_sha, ref: source_branch) + end - source_project.pipelines.order(id: :desc). - where(sha: sha, ref: source_branch) + # Note that this would also return SHA from dangling commits + def all_commits_sha + if persisted? + merge_request_diffs.flat_map(&:commits_sha).uniq + else + [diff_head_sha] end end From f2eb10c9bc6428c532f79914b737082518fae7e8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Sep 2016 19:18:48 +0800 Subject: [PATCH 4/8] slightly tweak about the comment, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6414#note_15750250 --- app/models/merge_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5af01846699..4673f895a1a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -751,7 +751,7 @@ class MergeRequest < ActiveRecord::Base where(sha: all_commits_sha, ref: source_branch) end - # Note that this would also return SHA from dangling commits + # Note that this could also return SHA from now dangling commits def all_commits_sha if persisted? merge_request_diffs.flat_map(&:commits_sha).uniq From 55de6e15d3b29c89a137e3e8824f02ddc4b7c1e7 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Sep 2016 20:07:56 +0800 Subject: [PATCH 5/8] Test against MergeRequest#all_commits_sha, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6414#note_15750344 So that we could just use it in testing for MergeRequest#all_pipelines --- spec/models/merge_request_spec.rb | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fb599e1207d..5baed2946e5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -496,20 +496,18 @@ describe MergeRequest, models: true do describe '#all_pipelines' do shared_examples 'returning pipelines with proper ordering' do - let!(:pipelines) do - subject.merge_request_diffs.flat_map do |diff| - diff.commits.map do |commit| - create(:ci_empty_pipeline, - project: subject.source_project, - sha: commit.id, - ref: subject.source_branch) - end + let!(:all_pipelines) do + subject.all_commits_sha.map do |sha| + create(:ci_empty_pipeline, + project: subject.source_project, + sha: sha, + ref: subject.source_branch) end end it 'returns all pipelines' do expect(subject.all_pipelines).not_to be_empty - expect(subject.all_pipelines).to eq(pipelines.reverse) + expect(subject.all_pipelines).to eq(all_pipelines.reverse) end end @@ -526,6 +524,21 @@ describe MergeRequest, models: true do end end + describe '#all_commits_sha' do + let(:all_commits_sha) do + subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq + end + + before do + subject.update(target_branch: 'markdown') + end + + it 'returns all SHA from all merge_request_diffs' do + expect(subject.merge_request_diffs.size).to eq(2) + expect(subject.all_commits_sha).to eq(all_commits_sha) + end + end + describe '#participants' do let(:project) { create(:project, :public) } From 3ae99b2c9fd82e7be0f8b404167e656102aa6c8b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Sep 2016 20:29:07 +0800 Subject: [PATCH 6/8] If merge request wasn't persisted yet, we show only 1 pipeline: However, if MergeRequest#all_commits_sha would want to handle non-persisted merge request, by judging its name, it should not just give 1 SHA, but all of them. But we don't really care all_commits_sha for non-persisted merge request anyway. So I think we should just ignore that case. Better to not implementing something than implementing it in a wrong and confusing way. --- app/models/merge_request.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4673f895a1a..4cd402b1354 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -747,17 +747,21 @@ class MergeRequest < ActiveRecord::Base def all_pipelines return unless source_project - @all_pipelines ||= source_project.pipelines.order(id: :desc). - where(sha: all_commits_sha, ref: source_branch) + @all_pipelines ||= begin + sha = if persisted? + all_commits_sha + else + diff_head_sha + end + + source_project.pipelines.order(id: :desc). + where(sha: sha, ref: source_branch) + end end # Note that this could also return SHA from now dangling commits def all_commits_sha - if persisted? - merge_request_diffs.flat_map(&:commits_sha).uniq - else - [diff_head_sha] - end + merge_request_diffs.flat_map(&:commits_sha).uniq end def merge_commit From a14d6a9e4ba755cd35f911d58e5c64ad56a81093 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 20 Sep 2016 20:51:14 +0800 Subject: [PATCH 7/8] Add a test for non-persisted merge request --- spec/models/merge_request_spec.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 5baed2946e5..fce236bcb5d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -522,6 +522,28 @@ describe MergeRequest, models: true do it_behaves_like 'returning pipelines with proper ordering' end + + context 'with unsaved merge request' do + let(:project) { create(:project) } + + subject do + MergeRequest.new(source_project: project, + target_project: project, + source_branch: 'master', + target_branch: 'feature') + end + + let!(:pipeline) do + create(:ci_empty_pipeline, + project: project, + sha: subject.diff_head_sha, + ref: subject.source_branch) + end + + it 'returns pipelines from diff_head_sha' do + expect(subject.all_pipelines).to contain_exactly(pipeline) + end + end end describe '#all_commits_sha' do From 93ab5856e4de9a13e730cf9d200e4f0934f4bece Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 21 Sep 2016 01:03:47 +0800 Subject: [PATCH 8/8] Use factory instead of using new directly. Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6414#note_15765156 --- spec/models/merge_request_spec.rb | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index fce236bcb5d..38c2a28d3e1 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -524,18 +524,11 @@ describe MergeRequest, models: true do end context 'with unsaved merge request' do - let(:project) { create(:project) } - - subject do - MergeRequest.new(source_project: project, - target_project: project, - source_branch: 'master', - target_branch: 'feature') - end + subject { build(:merge_request) } let!(:pipeline) do create(:ci_empty_pipeline, - project: project, + project: subject.project, sha: subject.diff_head_sha, ref: subject.source_branch) end