diff --git a/app/assets/javascripts/create_merge_request_dropdown.js b/app/assets/javascripts/create_merge_request_dropdown.js index fb1fc9cd32e..a88b6971f90 100644 --- a/app/assets/javascripts/create_merge_request_dropdown.js +++ b/app/assets/javascripts/create_merge_request_dropdown.js @@ -84,20 +84,21 @@ export default class CreateMergeRequestDropdown { if (data.can_create_branch) { this.available(); this.enable(); + this.updateBranchName(data.suggested_branch_name); if (!this.droplabInitialized) { this.droplabInitialized = true; this.initDroplab(); this.bindEvents(); } - } else if (data.has_related_branch) { + } else { this.hide(); } }) .catch(() => { this.unavailable(); this.disable(); - Flash('Failed to check if a new branch can be created.'); + Flash(__('Failed to check related branches.')); }); } @@ -409,13 +410,16 @@ export default class CreateMergeRequestDropdown { this.unavailableButton.classList.remove('hide'); } + updateBranchName(suggestedBranchName) { + this.branchInput.value = suggestedBranchName; + this.updateCreatePaths('branch', suggestedBranchName); + } + updateInputState(target, ref, result) { // target - 'branch' or 'ref' - which the input field we are searching a ref for. // ref - string - what a user typed. // result - string - what has been found on backend. - const pathReplacement = `$1${ref}`; - // If a found branch equals exact the same text a user typed, // that means a new branch cannot be created as it already exists. if (ref === result) { @@ -426,18 +430,12 @@ export default class CreateMergeRequestDropdown { this.refIsValid = true; this.refInput.dataset.value = ref; this.showAvailableMessage('ref'); - this.createBranchPath = this.createBranchPath.replace(this.regexps.ref.createBranchPath, - pathReplacement); - this.createMrPath = this.createMrPath.replace(this.regexps.ref.createMrPath, - pathReplacement); + this.updateCreatePaths(target, ref); } } else if (target === 'branch') { this.branchIsValid = true; this.showAvailableMessage('branch'); - this.createBranchPath = this.createBranchPath.replace(this.regexps.branch.createBranchPath, - pathReplacement); - this.createMrPath = this.createMrPath.replace(this.regexps.branch.createMrPath, - pathReplacement); + this.updateCreatePaths(target, ref); } else { this.refIsValid = false; this.refInput.dataset.value = ref; @@ -457,4 +455,15 @@ export default class CreateMergeRequestDropdown { this.disableCreateAction(); } } + + // target - 'branch' or 'ref' + // ref - string - the new value to use as branch or ref + updateCreatePaths(target, ref) { + const pathReplacement = `$1${ref}`; + + this.createBranchPath = this.createBranchPath.replace(this.regexps[target].createBranchPath, + pathReplacement); + this.createMrPath = this.createMrPath.replace(this.regexps[target].createMrPath, + pathReplacement); + } } diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 767e492f566..d69015c8665 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -134,11 +134,11 @@ class Projects::IssuesController < Projects::ApplicationController def can_create_branch can_create = current_user && can?(current_user, :push_code, @project) && - @issue.can_be_worked_on?(current_user) + @issue.can_be_worked_on? respond_to do |format| format.json do - render json: { can_create_branch: can_create, has_related_branch: @issue.has_related_branch? } + render json: { can_create_branch: can_create, suggested_branch_name: @issue.suggested_branch_name } end end end @@ -177,7 +177,7 @@ class Projects::IssuesController < Projects::ApplicationController end def authorize_create_merge_request! - render_404 unless can?(current_user, :push_code, @project) && @issue.can_be_worked_on?(current_user) + render_404 unless can?(current_user, :push_code, @project) && @issue.can_be_worked_on? end def render_issue_json diff --git a/app/models/concerns/uniquify.rb b/app/models/concerns/uniquify.rb index a7fe5951b6e..549a76da20e 100644 --- a/app/models/concerns/uniquify.rb +++ b/app/models/concerns/uniquify.rb @@ -1,13 +1,21 @@ +# Uniquify +# +# Return a version of the given 'base' string that is unique +# by appending a counter to it. Uniqueness is determined by +# repeated calls to the passed block. +# +# You can pass an initial value for the counter, if not given +# counting starts from 1. +# +# If `base` is a function/proc, we expect that calling it with a +# candidate counter returns a string to test/return. class Uniquify - # Return a version of the given 'base' string that is unique - # by appending a counter to it. Uniqueness is determined by - # repeated calls to the passed block. - # - # If `base` is a function/proc, we expect that calling it with a - # candidate counter returns a string to test/return. + def initialize(counter = nil) + @counter = counter + end + def string(base) @base = base - @counter = nil increment_counter! while yield(base_string) base_string diff --git a/app/models/issue.rb b/app/models/issue.rb index 7611e83647c..0332bfa9371 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -194,6 +194,15 @@ class Issue < ActiveRecord::Base branches_with_iid - branches_with_merge_request end + def suggested_branch_name + return to_branch_name unless project.repository.branch_exists?(to_branch_name) + + start_counting_from = 2 + Uniquify.new(start_counting_from).string(-> (counter) { "#{to_branch_name}-#{counter}" }) do |suggested_branch_name| + project.repository.branch_exists?(suggested_branch_name) + end + end + # Returns boolean if a related branch exists for the current issue # ignores merge requests branchs def has_related_branch? @@ -248,11 +257,8 @@ class Issue < ActiveRecord::Base end end - def can_be_worked_on?(current_user) - !self.closed? && - !self.project.forked? && - self.related_branches(current_user).empty? && - self.closed_by_merge_requests(current_user).empty? + def can_be_worked_on? + !self.closed? && !self.project.forked? end # Returns `true` if the current issue can be viewed by either a logged in User diff --git a/changelogs/unreleased/42803-show-new-branch-mr-button.yml b/changelogs/unreleased/42803-show-new-branch-mr-button.yml new file mode 100644 index 00000000000..d689ff7f001 --- /dev/null +++ b/changelogs/unreleased/42803-show-new-branch-mr-button.yml @@ -0,0 +1,5 @@ +--- +title: Show new branch/mr button even when branch exists +merge_request: 17712 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0eec9793391..cd30783c274 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1776,6 +1776,9 @@ msgstr "" msgid "Failed to change the owner" msgstr "" +msgid "Failed to check related branches." +msgstr "" + msgid "Failed to remove issue from board, please try again." msgstr "" diff --git a/spec/javascripts/issue_spec.js b/spec/javascripts/issue_spec.js index f37426a72d4..047ecab27db 100644 --- a/spec/javascripts/issue_spec.js +++ b/spec/javascripts/issue_spec.js @@ -92,6 +92,7 @@ describe('Issue', function() { function mockCanCreateBranch(canCreateBranch) { mock.onGet(/(.*)\/can_create_branch$/).reply(200, { can_create_branch: canCreateBranch, + suggested_branch_name: 'foo-99', }); } diff --git a/spec/models/concerns/uniquify_spec.rb b/spec/models/concerns/uniquify_spec.rb index 914730718e7..6cd2de6dcce 100644 --- a/spec/models/concerns/uniquify_spec.rb +++ b/spec/models/concerns/uniquify_spec.rb @@ -22,6 +22,15 @@ describe Uniquify do expect(result).to eq('test_string2') end + it 'allows to pass an initial value for the counter' do + start_counting_from = 2 + uniquify = described_class.new(start_counting_from) + + result = uniquify.string('test_string') { |s| s == 'test_string' } + + expect(result).to eq('test_string2') + end + it 'allows passing in a base function that defines the location of the counter' do result = uniquify.string(-> (counter) { "test_#{counter}_string" }) do |s| s == 'test__string' diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 11154291368..128acf83686 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -376,6 +376,48 @@ describe Issue do end end + describe '#suggested_branch_name' do + let(:repository) { double } + + subject { build(:issue) } + + before do + allow(subject.project).to receive(:repository).and_return(repository) + end + + context '#to_branch_name does not exists' do + before do + allow(repository).to receive(:branch_exists?).and_return(false) + end + + it 'returns #to_branch_name' do + expect(subject.suggested_branch_name).to eq(subject.to_branch_name) + end + end + + context '#to_branch_name exists not ending with -index' do + before do + allow(repository).to receive(:branch_exists?).and_return(true) + allow(repository).to receive(:branch_exists?).with(/#{subject.to_branch_name}-\d/).and_return(false) + end + + it 'returns #to_branch_name ending with -2' do + expect(subject.suggested_branch_name).to eq("#{subject.to_branch_name}-2") + end + end + + context '#to_branch_name exists ending with -index' do + before do + allow(repository).to receive(:branch_exists?).and_return(true) + allow(repository).to receive(:branch_exists?).with("#{subject.to_branch_name}-3").and_return(false) + end + + it 'returns #to_branch_name ending with max index + 1' do + expect(subject.suggested_branch_name).to eq("#{subject.to_branch_name}-3") + end + end + end + describe '#has_related_branch?' do let(:issue) { create(:issue, title: "Blue Bell Knoll") } subject { issue.has_related_branch? } @@ -425,6 +467,27 @@ describe Issue do end end + describe '#can_be_worked_on?' do + let(:project) { build(:project) } + subject { build(:issue, :opened, project: project) } + + context 'is closed' do + subject { build(:issue, :closed) } + + it { is_expected.not_to be_can_be_worked_on } + end + + context 'project is forked' do + before do + allow(project).to receive(:forked?).and_return(true) + end + + it { is_expected.not_to be_can_be_worked_on } + end + + it { is_expected.to be_can_be_worked_on } + end + describe '#participants' do context 'using a public project' do let(:project) { create(:project, :public) }