Merge branch 'fj-62807-not-prefill-target-branch' into 'master'

Avoid filling target branch when unknown and source is default branch

Closes #62807

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

app/services/merge_requests/build_service.rb

@@ -33,7 +33,8 @@ module MergeRequests
       merge_request.assign_attributes(params.to_h.compact)
 
       merge_request.compare_commits = []
-      merge_request.target_branch = find_target_branch
+      set_merge_request_target_branch
+
       merge_request.can_be_created = projects_and_branches_valid?
 
       # compare branches only if branches are valid, otherwise
@@ -93,8 +94,12 @@ module MergeRequests
       project_from_params
     end
 
-    def find_target_branch
-      target_branch || target_project.default_branch
+    def set_merge_request_target_branch
+      if source_branch_default? && !target_branch_specified?
+        merge_request.target_branch = nil
+      else
+        merge_request.target_branch ||= target_project.default_branch
+      end
     end
 
     def source_branch_specified?
@@ -149,7 +154,15 @@ module MergeRequests
     end
 
     def same_source_and_target?
-      source_project == target_project && target_branch == source_branch
+      same_source_and_target_project? && target_branch == source_branch
+    end
+
+    def source_branch_default?
+      same_source_and_target_project? && source_branch == target_project.default_branch
+    end
+
+    def same_source_and_target_project?
+      source_project == target_project
     end
 
     def source_branch_exists?

app/views/projects/merge_requests/creations/_new_compare.html.haml

@@ -51,7 +51,7 @@
                   selected: f.object.target_project_id
           .merge-request-select.dropdown
             = f.hidden_field :target_branch
-            = dropdown_toggle f.object.target_branch, { toggle: "dropdown", 'field-name': "#{f.object_name}[target_branch]", 'refs-url': refs_project_path(f.object.target_project), selected: f.object.target_branch }, { toggle_class: "js-compare-dropdown js-target-branch monospace" }
+            = dropdown_toggle f.object.target_branch || _("Select target branch"), { toggle: "dropdown", 'field-name': "#{f.object_name}[target_branch]", 'refs-url': refs_project_path(f.object.target_project), selected: f.object.target_branch }, { toggle_class: "js-compare-dropdown js-target-branch monospace" }
             .dropdown-menu.dropdown-menu-selectable.js-target-branch-dropdown.git-revision-dropdown
               = dropdown_title(_("Select target branch"))
               = dropdown_filter(_("Search branches"))

changelogs/unreleased/fj-62807-not-prefill-target-branch.yml

@@ -0,0 +1,5 @@
+---
+title: Avoid prefilling target branch when source branch is the default one
+merge_request: 32701
+author:
+type: other

spec/services/merge_requests/build_service_spec.rb

@@ -49,6 +49,22 @@ describe MergeRequests::BuildService do
     allow(project).to receive(:commit).and_return(commit_2)
   end
 
+  shared_examples 'allows the merge request to be created' do
+    it do
+      expect(merge_request.can_be_created).to eq(true)
+    end
+  end
+
+  shared_examples 'forbids the merge request from being created' do
+    it 'returns that the merge request cannot be created' do
+      expect(merge_request.can_be_created).to eq(false)
+    end
+
+    it 'adds an error message to the merge request' do
+      expect(merge_request.errors).to contain_exactly(*Array(error_message))
+    end
+  end
+
   describe '#execute' do
     it 'calls the compare service with the correct arguments' do
       allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true)
@@ -79,12 +95,8 @@ describe MergeRequests::BuildService do
     context 'missing source branch' do
       let(:source_branch) { '' }
 
-      it 'forbids the merge request from being created' do
-        expect(merge_request.can_be_created).to eq(false)
-      end
-
-      it 'adds an error message to the merge request' do
-        expect(merge_request.errors).to contain_exactly('You must select source and target branch')
+      it_behaves_like 'forbids the merge request from being created' do
+        let(:error_message) { 'You must select source and target branch' }
       end
     end
 
@@ -96,25 +108,44 @@ describe MergeRequests::BuildService do
         stub_compare
       end
 
-      it 'creates compare object with target branch as default branch' do
-        expect(merge_request.compare).to be_present
-        expect(merge_request.target_branch).to eq(project.default_branch)
-      end
+      context 'when source branch' do
+        context 'is not the repository default branch' do
+          it 'creates compare object with target branch as default branch' do
+            expect(merge_request.compare).to be_present
+            expect(merge_request.target_branch).to eq(project.default_branch)
+          end
 
-      it 'allows the merge request to be created' do
-        expect(merge_request.can_be_created).to eq(true)
+          it_behaves_like 'allows the merge request to be created'
+        end
+
+        context 'the repository default branch' do
+          let(:source_branch) { 'master' }
+
+          it_behaves_like 'forbids the merge request from being created' do
+            let(:error_message) { 'You must select source and target branch' }
+          end
+
+          context 'when source project is different from the target project' do
+            let(:target_project) { create(:project, :public, :repository) }
+            let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) }
+            let(:source_project) { project }
+
+            it 'creates compare object with target branch as default branch' do
+              expect(merge_request.compare).to be_present
+              expect(merge_request.target_branch).to eq(project.default_branch)
+            end
+
+            it_behaves_like 'allows the merge request to be created'
+          end
+        end
       end
     end
 
     context 'same source and target branch' do
       let(:source_branch) { 'master' }
 
-      it 'forbids the merge request from being created' do
-        expect(merge_request.can_be_created).to eq(false)
-      end
-
-      it 'adds an error message to the merge request' do
-        expect(merge_request.errors).to contain_exactly('You must select different branches')
+      it_behaves_like 'forbids the merge request from being created' do
+        let(:error_message) { 'You must select different branches' }
       end
     end
 
@@ -125,9 +156,7 @@ describe MergeRequests::BuildService do
         stub_compare
       end
 
-      it 'allows the merge request to be created' do
-        expect(merge_request.can_be_created).to eq(true)
-      end
+      it_behaves_like 'allows the merge request to be created'
 
       it 'adds a WIP prefix to the merge request title' do
         expect(merge_request.title).to eq('WIP: Feature branch')
@@ -142,9 +171,7 @@ describe MergeRequests::BuildService do
         stub_compare
       end
 
-      it 'allows the merge request to be created' do
-        expect(merge_request.can_be_created).to eq(true)
-      end
+      it_behaves_like 'allows the merge request to be created'
 
       it 'uses the title of the commit as the title of the merge request' do
         expect(merge_request.title).to eq(commit_1.safe_message.split("\n").first)
@@ -254,9 +281,7 @@ describe MergeRequests::BuildService do
         stub_compare
       end
 
-      it 'allows the merge request to be created' do
-        expect(merge_request.can_be_created).to eq(true)
-      end
+      it_behaves_like 'allows the merge request to be created'
 
       it 'uses the title of the branch as the merge request title' do
         expect(merge_request.title).to eq('Feature branch')
@@ -340,12 +365,8 @@ describe MergeRequests::BuildService do
         allow(project).to receive(:commit).with(target_branch).and_return(commit_1)
       end
 
-      it 'forbids the merge request from being created' do
-        expect(merge_request.can_be_created).to eq(false)
-      end
-
-      it 'adds an error message to the merge request' do
-        expect(merge_request.errors).to contain_exactly('Source branch "feature-branch" does not exist')
+      it_behaves_like 'forbids the merge request from being created' do
+        let(:error_message) { 'Source branch "feature-branch" does not exist' }
       end
     end
 
@@ -355,12 +376,8 @@ describe MergeRequests::BuildService do
         allow(project).to receive(:commit).with(target_branch).and_return(nil)
       end
 
-      it 'forbids the merge request from being created' do
-        expect(merge_request.can_be_created).to eq(false)
-      end
-
-      it 'adds an error message to the merge request' do
-        expect(merge_request.errors).to contain_exactly('Target branch "master" does not exist')
+      it_behaves_like 'forbids the merge request from being created' do
+        let(:error_message) { 'Target branch "master" does not exist' }
       end
     end
 
@@ -369,15 +386,10 @@ describe MergeRequests::BuildService do
         allow(project).to receive(:commit).and_return(nil)
       end
 
-      it 'forbids the merge request from being created' do
-        expect(merge_request.can_be_created).to eq(false)
-      end
-
-      it 'adds both error messages to the merge request' do
-        expect(merge_request.errors).to contain_exactly(
-          'Source branch "feature-branch" does not exist',
-          'Target branch "master" does not exist'
-        )
+      it_behaves_like 'forbids the merge request from being created' do
+        let(:error_message) do
+          ['Source branch "feature-branch" does not exist', 'Target branch "master" does not exist']
+        end
       end
     end