Move pipeline logic to service

The RelatedBranchesService is only used in the
Projects::IssuesController, so we can freely change its output to
isolate the logic from the controller.

app/controllers/projects/issues_controller.rb

@@ -157,7 +157,7 @@ class Projects::IssuesController < Projects::ApplicationController
     @related_branches = Issues::RelatedBranchesService
       .new(project, current_user)
       .execute(issue)
-      .map { |name| { name: name, link: branch_link(name), pipeline_status: pipeline_status(name) } }
+      .map { |branch| branch.merge(link: branch_link(branch)) }
 
     respond_to do |format|
       format.json do
@@ -309,16 +309,8 @@ class Projects::IssuesController < Projects::ApplicationController
 
   private
 
-  def branch_link(branch_name)
-    project_compare_path(project, from: project.default_branch, to: branch_name)
-  end
-
-  def pipeline_status(branch_name)
-    branch = @project.repository.find_branch(branch_name)
-    target = branch&.dereferenced_target
-    pipeline = @project.pipeline_for(branch_name, target.sha) if target
-
-    pipeline.detailed_status(current_user) if can?(current_user, :read_pipeline, pipeline)
+  def branch_link(branch)
+    project_compare_path(project, from: project.default_branch, to: branch[:name])
   end
 
   def create_rate_limit

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)

spec/controllers/projects/issues_controller_spec.rb

@@ -284,53 +284,33 @@ describe Projects::IssuesController do
 
     context 'there are related branches' do
       let(:missing_branch) { "#{issue.to_branch_name}-missing" }
-      let(:unreadable_branch_name) { "#{issue.to_branch_name}-unreadable" }
-      let(:repo) { double('Repo', root_ref: 'refs/heads/master', branch_names: branch_names) }
-      let(:sha) { 'abcdef' }
+      let(:unreadable_branch) { "#{issue.to_branch_name}-unreadable" }
       let(:pipeline) { build(:ci_pipeline, :success, project: project) }
-      let(:unreadable_pipeline) { build(:ci_pipeline, :running) }
-      let(:branch) { make_branch }
-      let(:unreadable_branch) { make_branch }
-      let(:branch_names) do
+      let(:master_branch) { 'master' }
+
+      let(:related_branches) do
         [
-          generate(:branch),
-          "#{issue.iid}doesnt-match",
-          issue.to_branch_name,
-          missing_branch,
-          unreadable_branch_name
+          branch_info(issue.to_branch_name, pipeline.detailed_status(user)),
+          branch_info(missing_branch, nil),
+          branch_info(unreadable_branch, nil)
         ]
       end
 
-      def make_branch
-        double('Branch', dereferenced_target: double('Target', sha: sha))
-      end
-
       def branch_info(name, status)
         {
           name: name,
-          link: controller.project_compare_path(project, from: project.default_branch, to: name),
+          link: controller.project_compare_path(project, from: master_branch, to: name),
           pipeline_status: status
         }
       end
 
       before do
-        allow(project).to receive(:repository).and_return(repo)
         allow(controller).to receive(:find_routable!)
           .with(Project, project.full_path, any_args).and_return(project)
-
-        allow(repo).to receive(:find_branch)
-          .with(issue.to_branch_name).and_return(branch)
-        allow(repo).to receive(:find_branch)
-          .with(missing_branch).and_return(nil)
-        allow(repo).to receive(:find_branch)
-          .with(unreadable_branch_name).and_return(unreadable_branch)
-
-        expect(project).to receive(:pipeline_for)
-          .with(issue.to_branch_name, sha)
-          .and_return(pipeline)
-        expect(project).to receive(:pipeline_for)
-          .with(unreadable_branch_name, sha)
-          .and_return(unreadable_pipeline)
+        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
@@ -340,7 +320,7 @@ describe Projects::IssuesController do
         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_name, be_nil)
+          branch_info(unreadable_branch, be_nil)
         )
         expect(response).to render_template('projects/issues/_related_branches')
         expect(json_response).to match('html' => String)

spec/services/issues/related_branches_service_spec.rb

@@ -3,24 +3,86 @@
 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(repo).to receive(:branch_names).and_return(branch_names)
+    end
+
+    context 'no branches are available' do
+      let(:branch_names) { [] }
+
+      it 'returns an empty array' do
+        expect(branch_info).to be_empty
+      end
+    end
+
+    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
-      allow(issue.project.repository).to receive(:branch_names).and_return(["mpempe", "#{issue.iid}mepmep", issue.to_branch_name, "#{issue.iid}-branch"])
+        {
+          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
 
-    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"])
+        allow(repo).to receive(:find_branch).with(missing_branch).and_return(nil)
       end
 
-    it "selects the right branches when there is a referenced merge request" do
-      merge_request = create(:merge_request, { description: "Closes ##{issue.iid}",
+      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.iid}-branch" })
+                                                 source_branch: issue.to_branch_name })
         merge_request.create_cross_references!(user)
 
         referenced_merge_requests = Issues::ReferencedMergeRequestsService
@@ -28,14 +90,16 @@ describe Issues::RelatedBranchesService do
                                       .referenced_merge_requests(issue)
 
         expect(referenced_merge_requests).not_to be_empty
-      expect(subject.execute(issue)).to eq([issue.to_branch_name])
+        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