Merge branch '42803-show-new-branch-mr-button' into 'master'

Resolve "Show new branch/merge request button even if a branch / merge request already exists"

Closes #42803

See merge request gitlab-org/gitlab-ce!17712

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);
+  }
 }

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

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

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

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

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 ""
 

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',
         });
       }
 

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'

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) }