diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 2ab6e88599f34c12402145f735f3fbe0c82a7304..71c86106f48615fe5eb223aedb3105f2d056146f 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -76,8 +76,17 @@ class IssuableBaseService < BaseService end def filter_labels - params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids) if params[:add_label_ids] - params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids) if params[:remove_label_ids] + if params[:add_label_ids] + params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids) + elsif params[:add_labels] + params[:add_label_ids] = labels_service.find_or_create_by_titles(:add_labels).map(&:id) + end + + if params[:remove_label_ids] + params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids) + elsif params[:remove_labels] + params[:remove_label_ids] = labels_service.find_or_create_by_titles(:remove_labels).map(&:id) + end if params[:label_ids] params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids) diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb index fe477d96970edcbaf1c2bcefd16ed2ef46ee7e54..bdd5f6605768f906558f1658a772fa143e75f0d6 100644 --- a/app/services/labels/available_labels_service.rb +++ b/app/services/labels/available_labels_service.rb @@ -9,8 +9,8 @@ module Labels @params = params end - def find_or_create_by_titles - labels = params.delete(:labels) + def find_or_create_by_titles(key = :labels) + labels = params.delete(key) return [] unless labels diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index b28f80939aeb03a3038c1ad3f8c26976fca7285d..308a3a10d1a2c08779b322b008c1c332a1ea4fa0 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -18,6 +18,18 @@ module MergeRequests merge_request.target_project = find_target_project filter_params(merge_request) + + # merge_request.assign_attributes(...) below is a Rails + # method that only work if all the params it is passed have + # corresponding fields in the database. As there are no fields + # in the database for :add_label_ids and :remove_label_ids, we + # need to remove them from the params before the call to + # merge_request.assign_attributes(...) + # + # IssuableBaseService#process_label_ids takes care + # of the removal. + params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a) + merge_request.assign_attributes(params.to_h.compact) merge_request.compare_commits = [] diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index b210004e6e1c2d2128a6c477f0949bca1d50bef4..0168b31005e19053b99e1ec0bed2a401f3f9a06f 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -100,7 +100,8 @@ module MergeRequests merge_request = ::MergeRequests::CreateService.new( project, current_user, - merge_request.attributes.merge(assignees: merge_request.assignees) + merge_request.attributes.merge(assignees: merge_request.assignees, + label_ids: merge_request.label_ids) ).execute end @@ -122,7 +123,9 @@ module MergeRequests title: push_options[:title], description: push_options[:description], target_branch: push_options[:target], - force_remove_source_branch: push_options[:remove_source_branch] + force_remove_source_branch: push_options[:remove_source_branch], + label: push_options[:label], + unlabel: push_options[:unlabel] } params.compact! @@ -134,6 +137,9 @@ module MergeRequests ) end + params[:add_labels] = params.delete(:label).keys if params.has_key?(:label) + params[:remove_labels] = params.delete(:unlabel).keys if params.has_key?(:unlabel) + params end diff --git a/changelogs/unreleased/add-label-push-opts.yml b/changelogs/unreleased/add-label-push-opts.yml new file mode 100644 index 0000000000000000000000000000000000000000..1289020e4e5e9f1ed6f7fe0e02e0e403fa249475 --- /dev/null +++ b/changelogs/unreleased/add-label-push-opts.yml @@ -0,0 +1,5 @@ +--- +title: Support adding and removing labels w/ push opts +merge_request: 31831 +author: +type: added diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index d6da8cb99c7ee623a3d7d432dd5e9832ff0a59d6..9f31f38460abc736931b310f2056889f515e382d 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -289,6 +289,7 @@ as pushing changes: - Set the merge request to remove the source branch when it's merged. - Set the title of the merge request to a particular title. - Set the description of the merge request to a particular description. +- Add or remove labels from the merge request. ### Create a new merge request using git push options @@ -375,6 +376,33 @@ git push -o merge_request.description="The description I want" You can also use this push option in addition to the `merge_request.create` push option. +### Add or remove labels using git push options + +You can add or remove labels from merge requests using push options. + +For example, to add two labels to an existing merge request, use the +`merge_request.label` push option: + +```sh +git push -o merge_request.label="label1" -o merge_request.label="label2" +``` + +To remove two labels from an existing merge request, use +the `merge_request.unlabel` push option: + +```sh +git push -o merge_request.unlabel="label1" -o merge_request.unlabel="label2" +``` + +You can also use these push options in addition to the +`merge_request.create` push option. + +To create a merge request and add two labels to it, use: + +```sh +git push -o merge_request.create -o merge_request.label="label1" -o merge_request.label="label2" +``` + ## Find the merge request that introduced a change > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/2383) in GitLab 10.5. diff --git a/lib/gitlab/push_options.rb b/lib/gitlab/push_options.rb index 682edfc425993523a3e0052eb0ef1ad8ccd66d8c..a2296d265cd9893430c45b6d6ecacff36e6356ae 100644 --- a/lib/gitlab/push_options.rb +++ b/lib/gitlab/push_options.rb @@ -7,10 +7,12 @@ module Gitlab keys: [ :create, :description, + :label, :merge_when_pipeline_succeeds, :remove_source_branch, :target, - :title + :title, + :unlabel ] }, ci: { @@ -18,6 +20,11 @@ module Gitlab } }).freeze + MULTI_VALUE_OPTIONS = [ + %w[merge_request label], + %w[merge_request unlabel] + ].freeze + NAMESPACE_ALIASES = HashWithIndifferentAccess.new({ mr: :merge_request }).freeze @@ -50,12 +57,22 @@ module Gitlab next if [namespace, key].any?(&:nil?) options[namespace] ||= HashWithIndifferentAccess.new - options[namespace][key] = value + + if option_multi_value?(namespace, key) + options[namespace][key] ||= HashWithIndifferentAccess.new(0) + options[namespace][key][value] += 1 + else + options[namespace][key] = value + end end options end + def option_multi_value?(namespace, key) + MULTI_VALUE_OPTIONS.any? { |arr| arr == [namespace, key] } + end + def parse_option(option) parts = OPTION_MATCHER.match(option) return unless parts diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index a27fea0c90f68bcadf0d345699e5ff42ad266518..ff4cdd3e7e280bbb4f7443b919f3ccddad40b322 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -13,6 +13,9 @@ describe MergeRequests::PushOptionsHandlerService do let(:target_branch) { 'feature' } let(:title) { 'my title' } let(:description) { 'my description' } + let(:label1) { 'mylabel1' } + let(:label2) { 'mylabel2' } + let(:label3) { 'mylabel3' } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } @@ -122,6 +125,16 @@ describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can change labels of a merge request' do |count| + subject(:last_mr) { MergeRequest.last } + + it 'changes label count' do + service.execute + + expect(last_mr.label_ids.count).to eq(count) + end + end + shared_examples_for 'a service that does not create a merge request' do it do expect { service.execute }.not_to change { MergeRequest.count } @@ -504,6 +517,138 @@ describe MergeRequests::PushOptionsHandlerService do end end + describe '`label` push option' do + let(:push_options) { { label: { label1 => 1, label2 => 1 } } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, label: { label1 => 1, label2 => 1 } } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can change labels of a merge request', 2 + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, label: { label1 => 1, label2 => 1 } } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can change labels of a merge request', 2 + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can change labels of a merge request', 2 + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + + describe '`unlabel` push option' do + let(:push_options) { { label: { label1 => 1, label2 => 1 }, unlabel: { label1 => 1, label3 => 1 } } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, label: { label1 => 1, label2 => 1 }, unlabel: { label1 => 1, label3 => 1 } } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can change labels of a merge request', 1 + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, label: { label1 => 1, label2 => 1 }, unlabel: { label1 => 1, label3 => 1 } } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can change labels of a merge request', 1 + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can change labels of a merge request', 1 + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + describe 'multiple pushed branches' do let(:push_options) { { create: true } } let(:changes) do