Incorporate review

CHANGELOG

@@ -59,6 +59,7 @@ v 8.5.3
   - Show commit message in JIRA mention comment
   - Makes issue page and merge request page usable on mobile browsers.
   - Improved UI for profile settings
+  - New branch button appears on issues where applicable
 
 v 8.5.2
   - Fix sidebar overlapping content when screen width was below 1200px
@@ -175,8 +176,6 @@ v 8.5.0
   - Add label description (Nuttanart Pornprasitsakul)
   - Show label row when filtering issues or merge requests by label (Nuttanart Pornprasitsakul)
   - Add Todos
-  - User project limit is reached notice is hidden if the projects limit is zero 
-  - New branch button appears on issues where applicable (Zeger-Jan van de Weg)
 
 v 8.4.5
   - No CE-specific changes

app/assets/stylesheets/pages/issues.scss

@@ -49,12 +49,7 @@ form.edit-issue {
   margin: 0;
 }
 
-.related-branches-title {
-    font-size: 16px;
-    font-weight: 600;
-}
-
-.merge-requests-title {
+.merge-requests-title, .related-branches-title {
   font-size: 16px;
   font-weight: 600;
 }

app/controllers/projects/branches_controller.rb

@@ -27,10 +27,9 @@ class Projects::BranchesController < Projects::ApplicationController
     result = CreateBranchService.new(project, current_user).
         execute(branch_name, ref)
 
-    if params[:issue_id]
-      issue = Issue.where(id: params[:issue_id], project: @project).limit(1).first
-
-      SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name)
+    if params[:issue_iid]
+      issue = @project.issues.find_by(iid: params[:issue_iid])
+      SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue
     end
 
     if result[:status] == :success

app/models/issue.rb

@@ -87,11 +87,16 @@ class Issue < ActiveRecord::Base
   end
 
   def referenced_merge_requests(current_user = nil)
-    Gitlab::ReferenceExtractor.lazily do
-      [self, *notes].flat_map do |note|
-        note.all_references(current_user).merge_requests
-      end
-    end.sort_by(&:iid)
+    if defined?(@referenced_merge_requests)
+      @referenced_merge_requests[current_user] ||=  Gitlab::ReferenceExtractor.lazily do
+                                                      [self, *notes].flat_map do |note|
+                                                        note.all_references(current_user).merge_requests
+                                                      end
+                                                    end.sort_by(&:iid).uniq
+    else
+      @referenced_merge_requests = {}
+      referenced_merge_requests(current_user)
+    end
   end
 
   def related_branches
@@ -132,7 +137,7 @@ class Issue < ActiveRecord::Base
   def can_be_worked_on?(current_user)
     !self.closed? &&
       !self.project.forked? &&
-      referenced_merge_requests(current_user).none? { |mr| mr.closes_issue?(self) } &&
-      related_branches.empty?
+      self.related_branches.empty? &&
+      self.referenced_merge_requests(current_user).empty?
   end
 end

app/services/system_note_service.rb

@@ -213,7 +213,7 @@ class SystemNoteService
   #   "Started branch `201-issue-branch-button`"
   def self.new_issue_branch(issue, project, author, branch)
     h = Gitlab::Application.routes.url_helpers
-    link = "#{Gitlab.config.gitlab.url}#{h.namespace_project_compare_path(project.namespace, project, from: project.default_branch, to: branch)}"
+    link = "#{h.namespace_project_compare_url(project.namespace, project, from: project.default_branch, to: branch)}"
 
     body = "Started branch [#{branch}](#{link})"
     create_note(noteable: issue, project: project, author: author, note: body)

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

 - if current_user && can?(current_user, :push_code, @project) && @issue.can_be_worked_on?(current_user)
   .pull-right
-    = link_to namespace_project_branches_path(@project.namespace, @project, branch_name: @issue.to_branch_name, issue_id: @issue.id), method: :post, class: 'btn', title: @issue.to_branch_name do
+    = link_to namespace_project_branches_path(@project.namespace, @project, branch_name: @issue.to_branch_name, issue_iid: @issue.iid), method: :post, class: 'btn', title: @issue.to_branch_name do
       = icon('code-fork')
       New Branch

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

@@ -11,5 +11,5 @@
             = render_ci_status(ci_commit)
         %span.related-branch-info
           %strong
-            = link_to namespace_project_compare_path(@project.namespace, @project, from: @project.default_branch, to: branch) do
+            = link_to namespace_project_compare_path(@project.namespace, @project, from: @project.default_branch, to: branch), class: "label-branch" do
               = branch

spec/controllers/projects/branches_controller_spec.rb

@@ -66,21 +66,30 @@ describe Projects::BranchesController do
 
     describe "created from the new branch button on issues" do
       let(:branch) { "1-feature-branch" }
-      let!(:issue) { create(:issue) }
+      let!(:issue) { create(:issue, project: project) }
 
-      before do
+
+      it 'redirects' do
         post :create,
           namespace_id: project.namespace.to_param,
           project_id: project.to_param,
           branch_name: branch,
-          issue_id: issue.id
-        end
+          issue_iid: issue.iid
 
-      it 'redirects' do
         expect(subject).
           to redirect_to("/#{project.path_with_namespace}/tree/1-feature-branch")
       end
 
+      it 'posts a system note' do
+        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch")
+
+        post :create,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          branch_name: branch,
+          issue_iid: issue.iid
+      end
+
     end
   end
 

spec/features/issues/new_branch_button_spec.rb

+require 'rails_helper'
+
+feature 'Start new branch from an issue', feature: true do
+  let!(:project)   { create(:project) }
+  let!(:issue)     { create(:issue, project: project) }
+  let!(:user)      { create(:user)}
+
+  context "for team members" do
+    before do
+      project.team << [user, :master]
+      login_as(user)
+    end
+
+    it 'shown the new branch button', js: false do
+      visit namespace_project_issue_path(project.namespace, project, issue)
+
+      expect(page).to have_link "New Branch"
+    end
+
+    context "when there is a referenced merge request" do
+      let(:note) do 
+        create(:note, :on_issue, :system, project: project,
+                                          note: "mentioned in !#{referenced_mr.iid}") 
+      end
+      let(:referenced_mr) do 
+        create(:merge_request, source_project: project,
+                               target_project: project,
+                               description: "Fixes ##{issue.iid}") 
+      end
+
+      before do
+        issue.notes << note
+
+        visit namespace_project_issue_path(project.namespace, project, issue)
+      end
+
+      it "hides the new branch button", js: true do
+        expect(page).not_to have_link "New Branch"
+        expect(page).to have_content /1 Related Merge Request/
+      end
+    end
+  end
+
+  context "for visiters" do
+    it 'no button is shown', js: false do
+      visit namespace_project_issue_path(project.namespace, project, issue)
+      expect(page).not_to have_link "New Branch"
+    end
+  end
+end

spec/models/issue_spec.rb

@@ -130,6 +130,15 @@ describe Issue, models: true do
     end
   end
 
+  describe '#related_branches' do
+    it "should " do
+      allow(subject.project.repository).to receive(:branch_names).
+                                    and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name])
+
+      expect(subject.related_branches).to eq [subject.to_branch_name]
+    end
+  end
+
   it_behaves_like 'an editable mentionable' do
     subject { create(:issue) }