From aee119c0cd44edd008f3167258bf45cb94ee5522 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 6 Mar 2019 15:34:38 +0700 Subject: [PATCH] Update pipeline detail view to accommodate post-merge pipelines Commit changes Add spec Add changelog fix fix Fix Fix spec Finish spec ok nice ok ok ok fix --- app/presenters/ci/pipeline_presenter.rb | 53 +++++++ app/presenters/merge_request_presenter.rb | 6 + .../merge_request_for_pipeline_entity.rb | 4 +- app/views/projects/pipelines/_info.html.haml | 14 +- .../nfriend-update-pipeline-detail-view.yml | 5 + locale/gitlab.pot | 12 ++ .../projects/pipelines/pipeline_spec.rb | 123 +++++++++++++++- spec/presenters/ci/pipeline_presenter_spec.rb | 134 ++++++++++++++++++ .../merge_request_presenter_spec.rb | 24 ++++ spec/serializers/pipeline_entity_spec.rb | 4 +- 10 files changed, 363 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/nfriend-update-pipeline-detail-view.yml diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index 57daf04efc6..1c1347c5a57 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -3,6 +3,7 @@ module Ci class PipelinePresenter < Gitlab::View::Presenter::Delegated include Gitlab::Utils::StrongMemoize + include ActionView::Helpers::UrlHelper # We use a class method here instead of a constant, allowing EE to redefine # the returned `Hash` more easily. @@ -32,5 +33,57 @@ module Ci "Pipeline is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}" end end + + def ref_text + if pipeline.detached_merge_request_pipeline? + _("for %{link_to_merge_request} with %{link_to_merge_request_source_branch}").html_safe % { link_to_merge_request: link_to_merge_request, link_to_merge_request_source_branch: link_to_merge_request_source_branch } + elsif pipeline.merge_request_pipeline? + _("for %{link_to_merge_request} with %{link_to_merge_request_source_branch} into %{link_to_merge_request_target_branch}").html_safe % { link_to_merge_request: link_to_merge_request, link_to_merge_request_source_branch: link_to_merge_request_source_branch, link_to_merge_request_target_branch: link_to_merge_request_target_branch } + elsif pipeline.ref + if pipeline.ref_exists? + _("for %{link_to_pipeline_ref}").html_safe % { link_to_pipeline_ref: link_to_pipeline_ref } + else + _("for %{ref}") % { ref: content_tag(:span, pipeline.ref, class: 'ref-name') } + end + end + end + + def link_to_pipeline_ref + link_to(pipeline.ref, + project_commits_path(pipeline.project, pipeline.ref), + class: "ref-name") + end + + def link_to_merge_request + return unless merge_request_presenter + + link_to(merge_request_presenter.to_reference, + project_merge_request_path(merge_request_presenter.project, merge_request_presenter), + class: 'mr-iid') + end + + def link_to_merge_request_source_branch + return unless merge_request_presenter + + link_to(merge_request_presenter.source_branch, + merge_request_presenter.source_branch_commits_path, + class: 'ref-name') + end + + def link_to_merge_request_target_branch + return unless merge_request_presenter + + link_to(merge_request_presenter.target_branch, + merge_request_presenter.target_branch_commits_path, + class: 'ref-name') + end + + private + + def merge_request_presenter + return unless pipeline.triggered_by_merge_request? + + @merge_request_presenter ||= pipeline.merge_request.present(current_user: current_user) + end end end diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index af164858408..284b1ad9b55 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -104,6 +104,12 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end end + def source_branch_commits_path + if source_branch_exists? + project_commits_path(source_project, source_branch) + end + end + def source_branch_path if source_branch_exists? project_branch_path(source_project, source_branch) diff --git a/app/serializers/merge_request_for_pipeline_entity.rb b/app/serializers/merge_request_for_pipeline_entity.rb index 7779ddfd65a..17a5c4ebbf9 100644 --- a/app/serializers/merge_request_for_pipeline_entity.rb +++ b/app/serializers/merge_request_for_pipeline_entity.rb @@ -11,7 +11,7 @@ class MergeRequestForPipelineEntity < Grape::Entity expose :title expose :source_branch - expose :source_branch_path + expose :source_branch_commits_path, as: :source_branch_path expose :target_branch - expose :target_branch_path + expose :target_branch_commits_path, as: :target_branch_path end diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 55adeb345ab..5d307d6a70d 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -10,13 +10,7 @@ .icon-container = icon('clock-o') = pluralize @pipeline.total_size, "job" - - if @pipeline.ref - from - - if @pipeline.ref_exists? - = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" - - else - %span.ref-name - = @pipeline.ref + = @pipeline.ref_text - if @pipeline.duration in = time_interval_in_words(@pipeline.duration) @@ -48,9 +42,9 @@ content: "#{popover_content_text}", } } Auto DevOps - - if @pipeline.merge_request_event? - %span.js-pipeline-url-mergerequest.badge.badge-info.has-tooltip{ title: "This pipeline is run in a merge request context" } - merge request + - if @pipeline.detached_merge_request_pipeline? + %span.js-pipeline-url-mergerequest.badge.badge-info.has-tooltip{ title: "This pipeline is run on the source branch" } + detached - if @pipeline.stuck? %span.js-pipeline-url-stuck.badge.badge-warning stuck diff --git a/changelogs/unreleased/nfriend-update-pipeline-detail-view.yml b/changelogs/unreleased/nfriend-update-pipeline-detail-view.yml new file mode 100644 index 00000000000..a24325c4eb6 --- /dev/null +++ b/changelogs/unreleased/nfriend-update-pipeline-detail-view.yml @@ -0,0 +1,5 @@ +--- +title: Update pipeline detail view to accommodate post-merge pipelines +merge_request: 25775 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c0c7041500d..47f201526d3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -9126,6 +9126,18 @@ msgstr "" msgid "estimateCommand|%{slash_command} will update the estimated time with the latest command." msgstr "" +msgid "for %{link_to_merge_request} with %{link_to_merge_request_source_branch}" +msgstr "" + +msgid "for %{link_to_merge_request} with %{link_to_merge_request_source_branch} into %{link_to_merge_request_target_branch}" +msgstr "" + +msgid "for %{link_to_pipeline_ref}" +msgstr "" + +msgid "for %{ref}" +msgstr "" + msgid "for this project" msgstr "" diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 36b8c15b8b6..5d2a99f40b7 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -1,6 +1,9 @@ require 'spec_helper' describe 'Pipeline', :js do + include RoutesHelpers + include ProjectForksHelper + let(:project) { create(:project) } let(:user) { create(:user) } let(:role) { :developer } @@ -72,6 +75,15 @@ describe 'Pipeline', :js do expect(page).to have_link(pipeline.ref) end + it 'shows the pipeline information' do + within '.pipeline-info' do + expect(page).to have_content("#{pipeline.statuses.count} jobs " \ + "for #{pipeline.ref} ") + expect(page).to have_link(pipeline.ref, + href: project_commits_path(pipeline.project, pipeline.ref)) + end + end + it_behaves_like 'showing user status' do let(:user_with_status) { pipeline.user } @@ -254,6 +266,113 @@ describe 'Pipeline', :js do expect(page).to have_content(pipeline.ref) end end + + context 'when pipeline is detached merge request pipeline' do + let(:source_project) { project } + let(:target_project) { project } + + let(:merge_request) do + create(:merge_request, + :with_detached_merge_request_pipeline, + source_project: source_project, + target_project: target_project) + end + + let(:pipeline) do + merge_request.all_pipelines.last + end + + it 'shows the pipeline information' do + within '.pipeline-info' do + expect(page).to have_content("#{pipeline.statuses.count} jobs " \ + "for !#{merge_request.iid} " \ + "with #{merge_request.source_branch}") + expect(page).to have_link("!#{merge_request.iid}", + href: project_merge_request_path(project, merge_request)) + expect(page).to have_link(merge_request.source_branch, + href: project_commits_path(merge_request.source_project, merge_request.source_branch)) + end + end + + context 'when source project is a forked project' do + let(:source_project) { fork_project(project, user, repository: true) } + + before do + visit project_pipeline_path(source_project, pipeline) + end + + it 'shows the pipeline information' do + within '.pipeline-info' do + expect(page).to have_content("#{pipeline.statuses.count} jobs " \ + "for !#{merge_request.iid} " \ + "with #{merge_request.source_branch}") + expect(page).to have_link("!#{merge_request.iid}", + href: project_merge_request_path(project, merge_request)) + expect(page).to have_link(merge_request.source_branch, + href: project_commits_path(merge_request.source_project, merge_request.source_branch)) + end + end + end + end + + context 'when pipeline is merge request pipeline' do + let(:source_project) { project } + let(:target_project) { project } + + let(:merge_request) do + create(:merge_request, + :with_merge_request_pipeline, + source_project: source_project, + target_project: target_project, + merge_sha: project.commit.id) + end + + let(:pipeline) do + merge_request.all_pipelines.last + end + + before do + pipeline.update(user: user) + end + + it 'shows the pipeline information' do + within '.pipeline-info' do + expect(page).to have_content("#{pipeline.statuses.count} jobs " \ + "for !#{merge_request.iid} " \ + "with #{merge_request.source_branch} " \ + "into #{merge_request.target_branch}") + expect(page).to have_link("!#{merge_request.iid}", + href: project_merge_request_path(project, merge_request)) + expect(page).to have_link(merge_request.source_branch, + href: project_commits_path(merge_request.source_project, merge_request.source_branch)) + expect(page).to have_link(merge_request.target_branch, + href: project_commits_path(merge_request.target_project, merge_request.target_branch)) + end + end + + context 'when source project is a forked project' do + let(:source_project) { fork_project(project, user, repository: true) } + + before do + visit project_pipeline_path(source_project, pipeline) + end + + it 'shows the pipeline information' do + within '.pipeline-info' do + expect(page).to have_content("#{pipeline.statuses.count} jobs " \ + "for !#{merge_request.iid} " \ + "with #{merge_request.source_branch} " \ + "into #{merge_request.target_branch}") + expect(page).to have_link("!#{merge_request.iid}", + href: project_merge_request_path(project, merge_request)) + expect(page).to have_link(merge_request.source_branch, + href: project_commits_path(merge_request.source_project, merge_request.source_branch)) + expect(page).to have_link(merge_request.target_branch, + href: project_commits_path(merge_request.target_project, merge_request.target_branch)) + end + end + end + end end context 'when user does not have access to read jobs' do @@ -686,9 +805,9 @@ describe 'Pipeline', :js do visit project_pipeline_path(project, pipeline) end - it 'contains badge that indicates merge request pipeline' do + it 'contains badge that indicates detached merge request pipeline' do page.within(all('.well-segment')[1]) do - expect(page).to have_content 'merge request' + expect(page).to have_content 'detached' end end end diff --git a/spec/presenters/ci/pipeline_presenter_spec.rb b/spec/presenters/ci/pipeline_presenter_spec.rb index f7ceaf844be..cda07a0ae09 100644 --- a/spec/presenters/ci/pipeline_presenter_spec.rb +++ b/spec/presenters/ci/pipeline_presenter_spec.rb @@ -1,6 +1,9 @@ require 'spec_helper' describe Ci::PipelinePresenter do + include Gitlab::Routing + + let(:user) { create(:user) } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } @@ -8,6 +11,11 @@ describe Ci::PipelinePresenter do described_class.new(pipeline) end + before do + project.add_developer(user) + allow(presenter).to receive(:current_user) { user } + end + it 'inherits from Gitlab::View::Presenter::Delegated' do expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated) end @@ -68,4 +76,130 @@ describe Ci::PipelinePresenter do end end end + + describe '#ref_text' do + subject { presenter.ref_text } + + context 'when pipeline is detached merge request pipeline' do + let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } + let(:pipeline) { merge_request.all_pipelines.last } + + it 'returns a correct ref text' do + is_expected.to eq("for #{merge_request.to_reference} " \ + "with #{merge_request.source_branch}") + end + end + + context 'when pipeline is merge request pipeline' do + let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) } + let(:pipeline) { merge_request.all_pipelines.last } + + it 'returns a correct ref text' do + is_expected.to eq("for #{merge_request.to_reference} " \ + "with #{merge_request.source_branch} " \ + "into #{merge_request.target_branch}") + end + end + + context 'when pipeline is branch pipeline' do + let(:pipeline) { create(:ci_pipeline, project: project) } + + context 'when ref exists in the repository' do + before do + allow(pipeline).to receive(:ref_exists?) { true } + end + + it 'returns a correct ref text' do + is_expected.to eq("for #{pipeline.ref}") + end + + context 'when ref contains malicious script' do + let(:pipeline) { create(:ci_pipeline, ref: "", project: project) } + + it 'does not include the malicious script' do + is_expected.not_to include("") + end + end + end + + context 'when ref exists in the repository' do + before do + allow(pipeline).to receive(:ref_exists?) { false } + end + + it 'returns a correct ref text' do + is_expected.to eq("for #{pipeline.ref}") + end + + context 'when ref contains malicious script' do + let(:pipeline) { create(:ci_pipeline, ref: "", project: project) } + + it 'does not include the malicious script' do + is_expected.not_to include("") + end + end + end + end + end + + describe '#link_to_merge_request' do + subject { presenter.link_to_merge_request } + + let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } + let(:pipeline) { merge_request.all_pipelines.last } + + it 'returns a correct link' do + is_expected + .to include(project_merge_request_path(merge_request.project, merge_request)) + end + + context 'when pipeline is branch pipeline' do + let(:pipeline) { create(:ci_pipeline, project: project) } + + it 'returns nothing' do + is_expected.to be_nil + end + end + end + + describe '#link_to_merge_request_source_branch' do + subject { presenter.link_to_merge_request_source_branch } + + let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } + let(:pipeline) { merge_request.all_pipelines.last } + + it 'returns a correct link' do + is_expected + .to include(project_commits_path(merge_request.source_project, + merge_request.source_branch)) + end + + context 'when pipeline is branch pipeline' do + let(:pipeline) { create(:ci_pipeline, project: project) } + + it 'returns nothing' do + is_expected.to be_nil + end + end + end + + describe '#link_to_merge_request_target_branch' do + subject { presenter.link_to_merge_request_target_branch } + + let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) } + let(:pipeline) { merge_request.all_pipelines.last } + + it 'returns a correct link' do + is_expected + .to include(project_commits_path(merge_request.target_project, merge_request.target_branch)) + end + + context 'when pipeline is branch pipeline' do + let(:pipeline) { create(:ci_pipeline, project: project) } + + it 'returns nothing' do + is_expected.to be_nil + end + end + end end diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index 02cefcbc916..fd03f594c35 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -345,6 +345,30 @@ describe MergeRequestPresenter do end end + describe '#source_branch_commits_path' do + subject do + described_class.new(resource, current_user: user) + .source_branch_commits_path + end + + context 'when source branch exists' do + it 'returns path' do + allow(resource).to receive(:source_branch_exists?) { true } + + is_expected + .to eq("/#{resource.source_project.full_path}/commits/#{resource.source_branch}") + end + end + + context 'when source branch does not exist' do + it 'returns nil' do + allow(resource).to receive(:source_branch_exists?) { false } + + is_expected.to be_nil + end + end + end + describe '#target_branch_tree_path' do subject do described_class.new(resource, current_user: user) diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb index c8308a0ae85..1d992e8a483 100644 --- a/spec/serializers/pipeline_entity_spec.rb +++ b/spec/serializers/pipeline_entity_spec.rb @@ -159,13 +159,13 @@ describe PipelineEntity do expect(subject[:merge_request][:source_branch]) .to eq(merge_request.source_branch) - expect(project_branch_path(project, merge_request.source_branch)) + expect(project_commits_path(project, merge_request.source_branch)) .to include(subject[:merge_request][:source_branch_path]) expect(subject[:merge_request][:target_branch]) .to eq(merge_request.target_branch) - expect(project_branch_path(project, merge_request.target_branch)) + expect(project_commits_path(project, merge_request.target_branch)) .to include(subject[:merge_request][:target_branch_path]) end end