diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 308a3a10d1a..88ed0c3ef4c 100644 --- a/app/services/merge_requests/build_service.rb +++ b/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? diff --git a/app/views/projects/merge_requests/creations/_new_compare.html.haml b/app/views/projects/merge_requests/creations/_new_compare.html.haml index be01905dd35..c6615b26bc0 100644 --- a/app/views/projects/merge_requests/creations/_new_compare.html.haml +++ b/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")) diff --git a/changelogs/unreleased/fj-62807-not-prefill-target-branch.yml b/changelogs/unreleased/fj-62807-not-prefill-target-branch.yml new file mode 100644 index 00000000000..f19634d80b2 --- /dev/null +++ b/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 diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index f18239f6d39..d546a092680 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/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