Move all logic out of views

This moves even the path generation and permission checking logic out of
the views, making the controller responsible for packing up all the data
needed for display. This has the advantage of allowing view tests to use
the :build strategy, since no persistence is needed.

The @related_branches attribute is combined with @branch_info, so we
have just a single assigment.

app/controllers/projects/issues_controller.rb

@@ -154,13 +154,10 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def related_branches
-    @related_branches = Issues::RelatedBranchesService.new(project, current_user).execute(issue)
-    @branch_info = @related_branches.map do |branch_name|
-      branch = @project.repository.find_branch(branch_name)
-      target = branch&.dereferenced_target
-      pipeline = @project.pipeline_for(branch_name, target.sha) if target
-      { name: branch_name, pipeline: pipeline }
-    end
+    @related_branches = Issues::RelatedBranchesService
+      .new(project, current_user)
+      .execute(issue)
+      .map { |name| { name: name, link: branch_link(name), pipeline_status: pipeline_status(name) } }
 
     respond_to do |format|
       format.json do
@@ -312,6 +309,18 @@ 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)
+  end
+
   def create_rate_limit
     key = :issues_create
 

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

@@ -2,11 +2,11 @@
   %h2.related-branches-title
     = pluralize(@related_branches.size, 'Related Branch')
   %ul.unstyled-list.related-merge-requests
-    - @branch_info.each do |branch|
+    - @related_branches.each do |branch|
       %li
-        - if can?(current_user, :read_pipeline, branch[:pipeline])
+        - if branch[:pipeline_status].present?
           %span.related-branch-ci-status
-            = render 'ci/status/icon', status: branch[:pipeline].detailed_status(current_user)
+            = render 'ci/status/icon', status: branch[:pipeline_status]
         %span.related-branch-info
           %strong
-            = link_to branch[:name], project_compare_path(@project, from: @project.default_branch, to: branch[:name]), class: "ref-name"
+            = link_to branch[:name], branch[:link], class: "ref-name"

spec/controllers/projects/issues_controller_spec.rb

@@ -243,7 +243,7 @@ describe Projects::IssuesController do
     end
   end
 
-  describe '#related_branches', :aggregate_failures do
+  describe '#related_branches' do
     subject { get :related_branches, params: params, format: :json }
 
     before do
@@ -272,12 +272,11 @@ describe Projects::IssuesController do
     end
 
     context 'there are no related branches' do
-      it 'assigns empty arrays' 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(assigns(:branch_info)).to be_empty
         expect(response).to render_template('projects/issues/_related_branches')
         expect(json_response).to eq('html' => '')
       end
@@ -285,12 +284,33 @@ describe Projects::IssuesController do
 
     context 'there are related branches' do
       let(:missing_branch) { "#{issue.to_branch_name}-missing" }
-      let(:repo) { double('Repo', branch_names: branch_names) }
+      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(:pipeline) { build(:ci_pipeline, :success) }
-      let(:branch) { double('Branch', dereferenced_target: double('Target', sha: sha)) }
+      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
-        [ generate(:branch), "#{issue.iid}doesnt-match", issue.to_branch_name, missing_branch ]
+        [
+          generate(:branch),
+          "#{issue.iid}doesnt-match",
+          issue.to_branch_name,
+          missing_branch,
+          unreadable_branch_name
+        ]
+      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),
+          pipeline_status: status
+        }
       end
 
       before do
@@ -302,20 +322,25 @@ describe Projects::IssuesController do
           .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)
 
-        allow(project).to receive(:pipeline_for)
+        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)
       end
 
-      it 'finds and assigns the appropriate branch information' do
+      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(issue.to_branch_name, missing_branch)
-        expect(assigns(:branch_info)).to contain_exactly(
-          a_hash_including(name: issue.to_branch_name, pipeline: pipeline),
-          a_hash_including(name: missing_branch, pipeline: nil)
+        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)
         )
         expect(response).to render_template('projects/issues/_related_branches')
         expect(json_response).to match('html' => String)

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

@@ -5,25 +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, %w(other feature))
-    assign(:branch_info, [{ name: 'other', pipeline: nil }, { name: 'feature', pipeline: pipeline }])
-
-    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')
-    expect(rendered).to match('other')
+  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