summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorFrancisco Javier López <fjlopez@gitlab.com>2019-09-05 13:40:45 +0200
committerFrancisco Javier López <fjlopez@gitlab.com>2019-09-10 12:56:32 +0200
commitd1ba86645164c7b06eb558bd8a0c78d6cf6a8de6 (patch)
tree26ec9b7e500fb378ad5797db224a501253ab81e8
parent86a3d82298ea9137c467129d1c828b92d7392ecd (diff)
downloadgitlab-ce-d1ba86645164c7b06eb558bd8a0c78d6cf6a8de6.tar.gz
Avoid setting merge request target branch when source if default branchfj-62807-not-prefill-target-branch
In case the source and the target project are the same, the source branch is the default branch, and the target branch is not present, we will avoid prefilling the target branch with the repository default branch. Letting the user decide.
-rw-r--r--app/services/merge_requests/build_service.rb21
-rw-r--r--app/views/projects/merge_requests/creations/_new_compare.html.haml2
-rw-r--r--changelogs/unreleased/fj-62807-not-prefill-target-branch.yml5
-rw-r--r--spec/services/merge_requests/build_service_spec.rb108
4 files changed, 83 insertions, 53 deletions
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index b28f80939ae..a1937d24294 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -21,7 +21,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
@@ -81,8 +82,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?
@@ -137,7 +142,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_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
- it 'allows the merge request to be created' do
- expect(merge_request.can_be_created).to eq(true)
+ 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