Merge branch 'add-label-push-opts' into 'master'

Support adding and removing labels w/ push opts Closes #5942 See merge request gitlab-org/gitlab-ce!31831

Merge branch 'add-label-push-opts' into 'master'
Support adding and removing labels w/ push opts Closes #5942 See merge request gitlab-org/gitlab-ce!31831
42850018 · Ash McKenzie · 71ff2c13 · 582e075e · 42850018 · 42850018
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -76,13 +76,16 @@ def filter_milestone
  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]
+    label_ids_to_filter(:add_label_ids, :add_labels, false)
+    label_ids_to_filter(:remove_label_ids, :remove_labels, true)
+    label_ids_to_filter(:label_ids, :labels, false)
+  end

-    if params[:label_ids]
-      params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids)
-    elsif params[:labels]
-      params[:label_ids] = labels_service.find_or_create_by_titles.map(&:id)
+  def label_ids_to_filter(label_id_key, label_key, find_only)
+    if params[label_id_key]
+      params[label_id_key] = labels_service.filter_labels_ids_in_param(label_id_key)
+    elsif params[label_key]
+      params[label_id_key] = labels_service.find_or_create_by_titles(label_key, find_only: find_only).map(&:id)
    end
  end


--- a/app/services/labels/available_labels_service.rb
+++ b/app/services/labels/available_labels_service.rb
@@ -9,8 +9,8 @@ def initialize(current_user, parent, params)
      @params = params
    end

-    def find_or_create_by_titles
-      labels = params.delete(:labels)
+    def find_or_create_by_titles(key = :labels, find_only: false)
+      labels = params.delete(key)

      return [] unless labels

@@ -23,7 +23,7 @@ def find_or_create_by_titles
          include_ancestor_groups: true,
          title: label_name.strip,
          available_labels: available_labels
-        ).execute
+        ).execute(find_only: find_only)

        label
      end.compact

--- a/app/services/labels/find_or_create_service.rb
+++ b/app/services/labels/find_or_create_service.rb
@@ -9,9 +9,9 @@ def initialize(current_user, parent, params = {})
      @params = params.dup.with_indifferent_access
    end

-    def execute(skip_authorization: false)
+    def execute(skip_authorization: false, find_only: false)
      @skip_authorization = skip_authorization
-      find_or_create_label
+      find_or_create_label(find_only: find_only)
    end

    private
@@ -30,9 +30,11 @@ def available_labels
    # Only creates the label if current_user can do so, if the label does not exist
    # and the user can not create the label, nil is returned
    # rubocop: disable CodeReuse/ActiveRecord
-    def find_or_create_label
+    def find_or_create_label(find_only: false)
      new_label = available_labels.find_by(title: title)

+      return new_label if find_only
+
      if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, parent))
        create_params = params.except(:include_ancestor_groups)
        new_label = Labels::CreateService.new(create_params).execute(parent_type.to_sym => parent)

--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -18,6 +18,18 @@ def execute
      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 = []

--- a/app/services/merge_requests/push_options_handler_service.rb
+++ b/app/services/merge_requests/push_options_handler_service.rb
@@ -100,7 +100,8 @@ def create!(branch)
        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 @@ def base_params
        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 @@ def base_params
        )
      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


--- a/changelogs/unreleased/add-label-push-opts.yml
+++ b/changelogs/unreleased/add-label-push-opts.yml
+---
+title: Support adding and removing labels w/ push opts
+merge_request: 31831
+author:
+type: added
--- 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.

--- a/lib/gitlab/push_options.rb
+++ b/lib/gitlab/push_options.rb
@@ -7,10 +7,12 @@ class PushOptions
        keys: [
          :create,
          :description,
+          :label,
          :merge_when_pipeline_succeeds,
          :remove_source_branch,
          :target,
-          :title
+          :title,
+          :unlabel
        ]
      },
      ci: {
@@ -18,6 +20,11 @@ class PushOptions
      }
    }).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 @@ def parse_options(raw_options)
        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

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