Enhance new branch button on an issue

8 files changed, 99 insertions, 50 deletions

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index cf68f50b1f9..bcbd3aa5d9b 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -27,6 +27,12 @@ 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)
+    end
+
     if result[:status] == :success
       @branch = result[:branch]
       redirect_to namespace_project_tree_path(@project.namespace, @project,
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 243d9a5db62..3b6bff6c577 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -95,7 +95,7 @@ class Issue < ActiveRecord::Base
   end
 
   def related_branches
-    self.project.repository.branch_names.select { |branch| /\A#{iid}-/ =~ branch }
+    self.project.repository.branch_names.select { |branch| branch.start_with? "#{iid}-" }
   end
 
   # Reset issue events cache
@@ -126,12 +126,13 @@ class Issue < ActiveRecord::Base
   end
 
   def to_branch_name
-    "#{iid}-#{title.parameterize}"[0,25]
+    "#{iid}-#{title.parameterize}"
   end
 
-  def new_branch_button?(current_user)
+  def can_be_worked_on?(current_user)
     !self.closed? &&
-    referenced_merge_requests(current_user).empty? &&
-    related_branches.empty?
+      !self.project.forked? &&
+      referenced_merge_requests(current_user).none? { |mr| mr.closes_issue?(self) } &&
+      related_branches.empty?
   end
 end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 8f1b735b8df..fa34753c4fd 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -51,11 +51,15 @@ module MergeRequests
       # be interpreted as the use wants to close that issue on this project
       # Pattern example: 112-fix-mep-mep
       # Will lead to appending `Closes #112` to the description
-      if merge_request.source_branch =~ /\A\d+-/
-        closes_issue = "Closes ##{Regexp.last_match(0)[0...-1]}"
-        closes_issue.prepend("\n") if merge_request.description.present?
+      if match = merge_request.source_branch.match(/\A(\d+)-/)
+        iid = match[1]
+        closes_issue = "Closes ##{iid}"
 
-        merge_request.description << closes_issue
+        if merge_request.description.present?
+          merge_request.description << closes_issue.prepend("\n")
+        else
+          merge_request.description = closes_issue
+        end
       end
 
       merge_request
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 58a861ee08e..751815c5b18 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -207,6 +207,15 @@ class SystemNoteService
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
+  # Called when a branch is created from the 'new branch' button on a issue
+  # Example note text:
+  #
+  #   "Started branch `201-issue-branch-button`"
+  def self.new_issue_branch(issue, project, author, branch)
+    body = "Started branch `#{branch}`"
+    create_note(noteable: issue, project: project, author: author, note: body)
+  end
+
   # Called when a Mentionable references a Noteable
   #
   # noteable  - Noteable object being referenced
diff --git a/app/views/projects/issues/_new_branch.html.haml b/app/views/projects/issues/_new_branch.html.haml
index 4d5fa61a91a..a6b97b90e64 100644
--- a/app/views/projects/issues/_new_branch.html.haml
+++ b/app/views/projects/issues/_new_branch.html.haml
@@ -1,5 +1,5 @@
-- if current_user && can?(current_user, :push_code, @project) && @issue.new_branch_button?(current_user)
+- 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), method: :post, class: 'btn' do
+    = 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
       = icon('code-fork')
       New Branch
diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb
index 8e06d4bdc77..b509714e75c 100644
--- a/spec/controllers/projects/branches_controller_spec.rb
+++ b/spec/controllers/projects/branches_controller_spec.rb
@@ -17,49 +17,70 @@ describe Projects::BranchesController do
   describe "POST create" do
     render_views
 
-    before do
-      post :create,
-        namespace_id: project.namespace.to_param,
-        project_id: project.to_param,
-        branch_name: branch,
-        ref: ref
-    end
+    context "on creation of a new branch" do
+      before do
+        post :create,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          branch_name: branch,
+          ref: ref
+      end
 
-    context "valid branch name, valid source" do
-      let(:branch) { "merge_branch" }
-      let(:ref) { "master" }
-      it 'redirects' do
-        expect(subject).
-          to redirect_to("/#{project.path_with_namespace}/tree/merge_branch")
+      context "valid branch name, valid source" do
+        let(:branch) { "merge_branch" }
+        let(:ref) { "master" }
+        it 'redirects' do
+          expect(subject).
+            to redirect_to("/#{project.path_with_namespace}/tree/merge_branch")
+        end
       end
-    end
 
-    context "invalid branch name, valid ref" do
-      let(:branch) { "<script>alert('merge');</script>" }
-      let(:ref) { "master" }
-      it 'redirects' do
-        expect(subject).
-          to redirect_to("/#{project.path_with_namespace}/tree/alert('merge');")
+      context "invalid branch name, valid ref" do
+        let(:branch) { "<script>alert('merge');</script>" }
+        let(:ref) { "master" }
+        it 'redirects' do
+          expect(subject).
+            to redirect_to("/#{project.path_with_namespace}/tree/alert('merge');")
+        end
       end
-    end
 
-    context "valid branch name, invalid ref" do
-      let(:branch) { "merge_branch" }
-      let(:ref) { "<script>alert('ref');</script>" }
-      it { is_expected.to render_template('new') }
-    end
+      context "valid branch name, invalid ref" do
+        let(:branch) { "merge_branch" }
+        let(:ref) { "<script>alert('ref');</script>" }
+        it { is_expected.to render_template('new') }
+      end
+
+      context "invalid branch name, invalid ref" do
+        let(:branch) { "<script>alert('merge');</script>" }
+        let(:ref) { "<script>alert('ref');</script>" }
+        it { is_expected.to render_template('new') }
+      end
 
-    context "invalid branch name, invalid ref" do
-      let(:branch) { "<script>alert('merge');</script>" }
-      let(:ref) { "<script>alert('ref');</script>" }
-      it { is_expected.to render_template('new') }
+      context "valid branch name with encoded slashes" do
+        let(:branch) { "feature%2Ftest" }
+        let(:ref) { "<script>alert('ref');</script>" }
+        it { is_expected.to render_template('new') }
+        it { project.repository.branch_names.include?('feature/test') }
+      end
     end
 
-    context "valid branch name with encoded slashes" do
-      let(:branch) { "feature%2Ftest" }
-      let(:ref) { "<script>alert('ref');</script>" }
-      it { is_expected.to render_template('new') }
-      it { project.repository.branch_names.include?('feature/test')}
+    describe "created from the new branch button on issues" do
+      let(:branch) { "1-feature-branch" }
+      let!(:issue) { create(:issue) }
+
+      before do
+        post :create,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          branch_name: branch,
+          issue_id: issue.id
+        end
+
+      it 'redirects' do
+        expect(subject).
+          to redirect_to("/#{project.path_with_namespace}/tree/1-feature-branch")
+      end
+
     end
   end
 
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index d572a71cf46..97b2db2fba5 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -144,10 +144,6 @@ describe Issue, models: true do
   describe "#to_branch_name" do
     let(:issue) { build(:issue, title: 'a' * 30) }
 
-    it "is expected not to exceed 25 chars" do
-      expect(issue.to_branch_name.length).to eq 25
-    end
-
     it "starts with the issue iid" do
       expect(issue.to_branch_name).to match /\A#{issue.iid}-a+\z/
     end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 5dcc39f5fdc..2730b42c612 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -280,6 +280,18 @@ describe SystemNoteService, services: true do
     end
   end
 
+  describe '.new_issue_branch' do
+    subject { described_class.new_issue_branch(noteable, project, author, "1-mepmep") }
+
+    it_behaves_like 'a system note'
+
+    context 'when a branch is created from the new branch button' do
+      it 'sets the note text' do
+        expect(subject.note).to eq 'Started branch 1-mepmep'
+      end
+    end
+  end
+
   describe '.cross_reference' do
     subject { described_class.cross_reference(noteable, mentioner, author) }