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

@@ -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

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)

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

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: "<a class='autodevops-link' href='#{popover_content_url}' target='_blank' rel='noopener noreferrer nofollow'>#{popover_content_text}</a>",
       } }
         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

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

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 ""
 

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

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 <a class=\"mr-iid\" href=\"#{project_merge_request_path(merge_request.project, merge_request)}\">#{merge_request.to_reference}</a> " \
+                          "with <a class=\"ref-name\" href=\"#{project_commits_path(merge_request.source_project, merge_request.source_branch)}\">#{merge_request.source_branch}</a>")
+      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 <a class=\"mr-iid\" href=\"#{project_merge_request_path(merge_request.project, merge_request)}\">#{merge_request.to_reference}</a> " \
+                          "with <a class=\"ref-name\" href=\"#{project_commits_path(merge_request.source_project, merge_request.source_branch)}\">#{merge_request.source_branch}</a> " \
+                          "into <a class=\"ref-name\" href=\"#{project_commits_path(merge_request.target_project, merge_request.target_branch)}\">#{merge_request.target_branch}</a>")
+      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 <a class=\"ref-name\" href=\"#{project_commits_path(pipeline.project, pipeline.ref)}\">#{pipeline.ref}</a>")
+        end
+
+        context 'when ref contains malicious script' do
+          let(:pipeline) { create(:ci_pipeline, ref: "<script>alter('1')</script>", project: project) }
+
+          it 'does not include the malicious script' do
+            is_expected.not_to include("<script>alter('1')</script>")
+          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 <span class=\"ref-name\">#{pipeline.ref}</span>")
+        end
+
+        context 'when ref contains malicious script' do
+          let(:pipeline) { create(:ci_pipeline, ref: "<script>alter('1')</script>", project: project) }
+
+          it 'does not include the malicious script' do
+            is_expected.not_to include("<script>alter('1')</script>")
+          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

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)

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