Merge branch 'ajk-refactor-related-branches' into 'master'

Refactor related branches controller action and view template

See merge request gitlab-org/gitlab!30314

app/controllers/projects/issues_controller.rb

@@ -154,7 +154,10 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def related_branches
-    @related_branches = Issues::RelatedBranchesService.new(project, current_user).execute(issue)
+    @related_branches = Issues::RelatedBranchesService
+      .new(project, current_user)
+      .execute(issue)
+      .map { |branch| branch.merge(link: branch_link(branch)) }
 
     respond_to do |format|
       format.json do
@@ -306,6 +309,10 @@ class Projects::IssuesController < Projects::ApplicationController
 
   private
 
+  def branch_link(branch)
+    project_compare_path(project, from: project.default_branch, to: branch[:name])
+  end
+
   def create_rate_limit
     key = :issues_create
 

app/services/issues/related_branches_service.rb

@@ -5,11 +5,29 @@
 module Issues
   class RelatedBranchesService < Issues::BaseService
     def execute(issue)
-      branches_with_iid_of(issue) - branches_with_merge_request_for(issue)
+      branch_names = branches_with_iid_of(issue) - branches_with_merge_request_for(issue)
+      branch_names.map { |branch_name| branch_data(branch_name) }
     end
 
     private
 
+    def branch_data(branch_name)
+      {
+        name: branch_name,
+        pipeline_status: pipeline_status(branch_name)
+      }
+    end
+
+    def pipeline_status(branch_name)
+      branch = project.repository.find_branch(branch_name)
+      target = branch&.dereferenced_target
+
+      return unless target
+
+      pipeline = project.pipeline_for(branch_name, target.sha)
+      pipeline.detailed_status(current_user) if can?(current_user, :read_pipeline, pipeline)
+    end
+
     def branches_with_merge_request_for(issue)
       Issues::ReferencedMergeRequestsService
         .new(project, current_user)

app/views/projects/issues/_related_branches.html.haml

@@ -4,11 +4,9 @@
   %ul.unstyled-list.related-merge-requests
     - @related_branches.each do |branch|
       %li
-        - target = @project.repository.find_branch(branch).dereferenced_target
-        - pipeline = @project.pipeline_for(branch, target.sha) if target
-        - if can?(current_user, :read_pipeline, pipeline)
+        - if branch[:pipeline_status].present?
           %span.related-branch-ci-status
-            = render 'ci/status/icon', status: pipeline.detailed_status(current_user)
+            = render 'ci/status/icon', status: branch[:pipeline_status]
         %span.related-branch-info
           %strong
-            = link_to branch, project_compare_path(@project, from: @project.default_branch, to: branch), class: "ref-name"
+            = link_to branch[:name], branch[:link], class: "ref-name"

spec/controllers/projects/issues_controller_spec.rb

@@ -243,6 +243,91 @@ describe Projects::IssuesController do
     end
   end
 
+  describe '#related_branches' do
+    subject { get :related_branches, params: params, format: :json }
+
+    before do
+      sign_in(user)
+      project.add_developer(developer)
+    end
+
+    let(:developer) { user }
+    let(:params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: issue.iid
+      }
+    end
+
+    context 'the current user cannot download code' do
+      it 'prevents access' do
+        allow(controller).to receive(:can?).with(any_args).and_return(true)
+        allow(controller).to receive(:can?).with(user, :download_code, project).and_return(false)
+
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'there are no related branches' do
+      it 'assigns empty arrays', :aggregate_failures do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(assigns(:related_branches)).to be_empty
+        expect(response).to render_template('projects/issues/_related_branches')
+        expect(json_response).to eq('html' => '')
+      end
+    end
+
+    context 'there are related branches' do
+      let(:missing_branch) { "#{issue.to_branch_name}-missing" }
+      let(:unreadable_branch) { "#{issue.to_branch_name}-unreadable" }
+      let(:pipeline) { build(:ci_pipeline, :success, project: project) }
+      let(:master_branch) { 'master' }
+
+      let(:related_branches) do
+        [
+          branch_info(issue.to_branch_name, pipeline.detailed_status(user)),
+          branch_info(missing_branch, nil),
+          branch_info(unreadable_branch, nil)
+        ]
+      end
+
+      def branch_info(name, status)
+        {
+          name: name,
+          link: controller.project_compare_path(project, from: master_branch, to: name),
+          pipeline_status: status
+        }
+      end
+
+      before do
+        allow(controller).to receive(:find_routable!)
+          .with(Project, project.full_path, any_args).and_return(project)
+        allow(project).to receive(:default_branch).and_return(master_branch)
+        allow_next_instance_of(Issues::RelatedBranchesService) do |service|
+          allow(service).to receive(:execute).and_return(related_branches)
+        end
+      end
+
+      it 'finds and assigns the appropriate branch information', :aggregate_failures do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(assigns(:related_branches)).to contain_exactly(
+          branch_info(issue.to_branch_name, an_instance_of(Gitlab::Ci::Status::Success)),
+          branch_info(missing_branch, be_nil),
+          branch_info(unreadable_branch, be_nil)
+        )
+        expect(response).to render_template('projects/issues/_related_branches')
+        expect(json_response).to match('html' => String)
+      end
+    end
+  end
+
   # This spec runs as a request-style spec in order to invoke the
   # Rails router. A controller-style spec matches the wrong route, and
   # session['user_return_to'] becomes incorrect.

spec/services/issues/related_branches_service_spec.rb

@@ -3,39 +3,103 @@
 require 'spec_helper'
 
 describe Issues::RelatedBranchesService do
-  let(:user) { create(:admin) }
-  let(:issue) { create(:issue) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:issue) { create(:issue) }
+  let(:user) { developer }
 
   subject { described_class.new(issue.project, user) }
 
+  before do
+    issue.project.add_developer(developer)
+  end
+
   describe '#execute' do
+    let(:sha) { 'abcdef' }
+    let(:repo) { issue.project.repository }
+    let(:project) { issue.project }
+    let(:branch_info) { subject.execute(issue) }
+
+    def make_branch
+      double('Branch', dereferenced_target: double('Target', sha: sha))
+    end
+
     before do
-      allow(issue.project.repository).to receive(:branch_names).and_return(["mpempe", "#{issue.iid}mepmep", issue.to_branch_name, "#{issue.iid}-branch"])
+      allow(repo).to receive(:branch_names).and_return(branch_names)
     end
 
-    it "selects the right branches when there are no referenced merge requests" do
-      expect(subject.execute(issue)).to eq([issue.to_branch_name, "#{issue.iid}-branch"])
+    context 'no branches are available' do
+      let(:branch_names) { [] }
+
+      it 'returns an empty array' do
+        expect(branch_info).to be_empty
+      end
     end
 
-    it "selects the right branches when there is a referenced merge request" do
-      merge_request = create(:merge_request, { description: "Closes ##{issue.iid}",
-                                               source_project: issue.project,
-                                               source_branch: "#{issue.iid}-branch" })
-      merge_request.create_cross_references!(user)
+    context 'branches are available' do
+      let(:missing_branch) { "#{issue.to_branch_name}-missing" }
+      let(:unreadable_branch_name) { "#{issue.to_branch_name}-unreadable" }
+      let(:pipeline) { build(:ci_pipeline, :success, project: project) }
+      let(:unreadable_pipeline) { build(:ci_pipeline, :running) }
+
+      let(:branch_names) do
+        [
+          generate(:branch),
+          "#{issue.iid}doesnt-match",
+          issue.to_branch_name,
+          missing_branch,
+          unreadable_branch_name
+        ]
+      end
+
+      before do
+        {
+          issue.to_branch_name => pipeline,
+          unreadable_branch_name => unreadable_pipeline
+        }.each do |name, pipeline|
+          allow(repo).to receive(:find_branch).with(name).and_return(make_branch)
+          allow(project).to receive(:pipeline_for).with(name, sha).and_return(pipeline)
+        end
+
+        allow(repo).to receive(:find_branch).with(missing_branch).and_return(nil)
+      end
+
+      it 'selects relevant branches, along with pipeline status where available' do
+        expect(branch_info).to contain_exactly(
+          { name: issue.to_branch_name, pipeline_status: an_instance_of(Gitlab::Ci::Status::Success) },
+          { name: missing_branch, pipeline_status: be_nil },
+          { name: unreadable_branch_name, pipeline_status: be_nil }
+        )
+      end
+
+      context 'the user has access to otherwise unreadable pipelines' do
+        let(:user) { create(:admin) }
+
+        it 'returns info a developer could not see' do
+          expect(branch_info.pluck(:pipeline_status)).to include(an_instance_of(Gitlab::Ci::Status::Running))
+        end
+      end
+
+      it 'excludes branches referenced in merge requests' do
+        merge_request = create(:merge_request, { description: "Closes #{issue.to_reference}",
+                                                 source_project: issue.project,
+                                                 source_branch: issue.to_branch_name })
+        merge_request.create_cross_references!(user)
 
-      referenced_merge_requests = Issues::ReferencedMergeRequestsService
-                                    .new(issue.project, user)
-                                    .referenced_merge_requests(issue)
+        referenced_merge_requests = Issues::ReferencedMergeRequestsService
+                                      .new(issue.project, user)
+                                      .referenced_merge_requests(issue)
 
-      expect(referenced_merge_requests).not_to be_empty
-      expect(subject.execute(issue)).to eq([issue.to_branch_name])
+        expect(referenced_merge_requests).not_to be_empty
+        expect(branch_info.pluck(:name)).not_to include(merge_request.source_branch)
+      end
     end
 
-    it 'excludes stable branches from the related branches' do
-      allow(issue.project.repository).to receive(:branch_names)
-        .and_return(["#{issue.iid}-0-stable"])
+    context 'one of the branches is stable' do
+      let(:branch_names) { ["#{issue.iid}-0-stable"] }
 
-      expect(subject.execute(issue)).to eq []
+      it 'is excluded' do
+        expect(branch_info).to be_empty
+      end
     end
   end
 end

spec/views/projects/issues/_related_branches.html.haml_spec.rb

@@ -5,23 +5,25 @@ require 'spec_helper'
 describe 'projects/issues/_related_branches' do
   include Devise::Test::ControllerHelpers
 
-  let(:user) { create(:user) }
-  let(:project) { create(:project, :repository) }
-  let(:branch) { project.repository.find_branch('feature') }
-  let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.dereferenced_target.id, ref: 'feature') }
+  let(:pipeline) { build(:ci_pipeline, :success) }
+  let(:status) { pipeline.detailed_status(build(:user)) }
 
   before do
-    assign(:project, project)
-    assign(:related_branches, ['feature'])
-
-    project.add_developer(user)
-    allow(view).to receive(:current_user).and_return(user)
+    assign(:related_branches, [
+      { name: 'other', link: 'link-to-other', pipeline_status: nil },
+      { name: 'feature', link: 'link-to-feature', pipeline_status: status }
+    ])
 
     render
   end
 
-  it 'shows the related branches with their build status' do
-    expect(rendered).to match('feature')
+  it 'shows the related branches with their build status', :aggregate_failures do
+    expect(rendered).to have_text('feature')
+    expect(rendered).to have_text('other')
+    expect(rendered).to have_link(href: 'link-to-feature')
+    expect(rendered).to have_link(href: 'link-to-other')
     expect(rendered).to have_css('.related-branch-ci-status')
+    expect(rendered).to have_css('.ci-status-icon')
+    expect(rendered).to have_css('.related-branch-info')
   end
 end