From ad97bebfed2e65951c7dc39ee80b32089a032804 Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <mail@zjvandeweg.nl>
Date: Wed, 17 Feb 2016 07:11:48 +0100
Subject: [PATCH] Enhance new branch button on an issue
---
.../projects/branches_controller.rb | 6 ++
app/models/issue.rb | 11 ++-
app/services/merge_requests/build_service.rb | 12 ++-
app/services/system_note_service.rb | 9 ++
.../projects/issues/_new_branch.html.haml | 4 +-
.../projects/branches_controller_spec.rb | 91 ++++++++++++-------
spec/models/issue_spec.rb | 4 -
spec/services/system_note_service_spec.rb | 12 +++
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) }
--
GitLab