Merge branch 'api-mr-put-labels' into 'master'

PUT MergeRequest API endpoint - accept labels as an array

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

app/services/issuable_base_service.rb

@@ -89,7 +89,7 @@ class IssuableBaseService < BaseService
 
     return unless labels
 
-    params[:label_ids] = labels.split(",").map do |label_name|
+    params[:label_ids] = labels.map do |label_name|
       label = Labels::FindOrCreateService.new(
         current_user,
         parent,

lib/api/issues.rb

@@ -34,7 +34,7 @@ module API
       # rubocop: enable CodeReuse/ActiveRecord
 
       params :issues_params do
-        optional :labels, type: String, desc: 'Comma-separated list of label names'
+        optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
         optional :milestone, type: String, desc: 'Milestone title'
         optional :order_by, type: String, values: %w[created_at updated_at], default: 'created_at',
                             desc: 'Return issues ordered by `created_at` or `updated_at` fields.'
@@ -65,7 +65,7 @@ module API
         optional :assignee_ids, type: Array[Integer], desc: 'The array of user IDs to assign issue'
         optional :assignee_id,  type: Integer, desc: '[Deprecated] The ID of a user to assign issue'
         optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue'
-        optional :labels, type: String, desc: 'Comma-separated list of label names'
+        optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
         optional :due_date, type: String, desc: 'Date string in the format YEAR-MONTH-DAY'
         optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential'
         optional :discussion_locked, type: Boolean, desc: " Boolean parameter indicating if the issue's discussion is locked"

lib/api/merge_requests.rb

@@ -95,7 +95,7 @@ module API
         optional :sort, type: String, values: %w[asc desc], default: 'desc',
                         desc: 'Return merge requests sorted in `asc` or `desc` order.'
         optional :milestone, type: String, desc: 'Return merge requests for a specific milestone'
-        optional :labels, type: String, desc: 'Comma-separated list of label names'
+        optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
         optional :created_after, type: DateTime, desc: 'Return merge requests created after the specified time'
         optional :created_before, type: DateTime, desc: 'Return merge requests created before the specified time'
         optional :updated_after, type: DateTime, desc: 'Return merge requests updated after the specified time'
@@ -179,8 +179,8 @@ module API
           optional :description, type: String, desc: 'The description of the merge request'
           optional :assignee_id, type: Integer, desc: 'The ID of a user to assign the merge request'
           optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request'
-          optional :labels, type: String, desc: 'Comma-separated list of label names'
-          optional :remove_source_branch, type: Boolean, desc: 'Delete source branch when merging'
+          optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
+          optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging'
           optional :allow_collaboration, type: Boolean, desc: 'Allow commits from members who can merge to the target branch'
           optional :allow_maintainer_to_push, type: Boolean, as: :allow_collaboration, desc: '[deprecated] See allow_collaboration'
           optional :squash, type: Grape::API::Boolean, desc: 'When true, the commits will be squashed into a single commit on merge'

lib/api/validations/types/labels_list.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Types
+      class LabelsList
+        def self.coerce
+          lambda do |value|
+            case value
+            when String
+              value.split(',').map(&:strip)
+            when Array
+              value.map { |v| v.to_s.split(',').map(&:strip) }.flatten
+            when LabelsList
+              value
+            else
+              []
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/requests/api/merge_requests_spec.rb

@@ -617,19 +617,24 @@ describe API::MergeRequests do
     end
   end
 
-  describe "POST /projects/:id/merge_requests" do
+  describe 'POST /projects/:id/merge_requests' do
     context 'between branches projects' do
-      it "returns merge_request" do
-        post api("/projects/#{project.id}/merge_requests", user),
-             params: {
+      context 'different labels' do
+        let(:params) do
+          {
             title: 'Test merge_request',
             source_branch: 'feature_conflict',
             target_branch: 'master',
-               author: user,
-               labels: 'label, label2',
+            author_id: user.id,
             milestone_id: milestone.id,
             squash: true
           }
+        end
+
+        shared_examples_for 'creates merge request with labels' do
+          it 'returns merge_request' do
+            params[:labels] = labels
+            post api("/projects/#{project.id}/merge_requests", user), params: params
 
             expect(response).to have_gitlab_http_status(201)
             expect(json_response['title']).to eq('Test merge_request')
@@ -638,6 +643,90 @@ describe API::MergeRequests do
             expect(json_response['squash']).to be_truthy
             expect(json_response['force_remove_source_branch']).to be_falsy
           end
+        end
+
+        it_behaves_like 'creates merge request with labels' do
+          let(:labels) { 'label, label2' }
+        end
+
+        it_behaves_like 'creates merge request with labels' do
+          let(:labels) { %w(label label2) }
+        end
+
+        it_behaves_like 'creates merge request with labels' do
+          let(:labels) { %w(label label2) }
+        end
+
+        it 'creates merge request with special label names' do
+          params[:labels] = 'label, label?, label&foo, ?, &'
+          post api("/projects/#{project.id}/merge_requests", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to include 'label'
+          expect(json_response['labels']).to include 'label?'
+          expect(json_response['labels']).to include 'label&foo'
+          expect(json_response['labels']).to include '?'
+          expect(json_response['labels']).to include '&'
+        end
+
+        it 'creates merge request with special label names as array' do
+          params[:labels] = ['label', 'label?', 'label&foo, ?, &', '1, 2', 3, 4]
+          post api("/projects/#{project.id}/merge_requests", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to include 'label'
+          expect(json_response['labels']).to include 'label?'
+          expect(json_response['labels']).to include 'label&foo'
+          expect(json_response['labels']).to include '?'
+          expect(json_response['labels']).to include '&'
+          expect(json_response['labels']).to include '1'
+          expect(json_response['labels']).to include '2'
+          expect(json_response['labels']).to include '3'
+          expect(json_response['labels']).to include '4'
+        end
+
+        it 'empty label param does not add any labels' do
+          params[:labels] = ''
+          post api("/projects/#{project.id}/merge_requests", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to eq([])
+        end
+
+        it 'empty label param as array does not add any labels, but only explicitly as json' do
+          params[:labels] = []
+          post api("/projects/#{project.id}/merge_requests", user),
+            params: params.to_json,
+            headers: { 'Content-Type': 'application/json' }
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to eq([])
+        end
+
+        xit 'empty label param as array, does not add any labels' do
+          params[:labels] = []
+          post api("/projects/#{project.id}/merge_requests", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to eq([])
+        end
+
+        it 'array with one empty string element does not add labels' do
+          params[:labels] = ['']
+          post api("/projects/#{project.id}/merge_requests", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to eq([])
+        end
+
+        it 'array with multiple empty string elements, does not add labels' do
+          params[:labels] = ['', '', '']
+          post api("/projects/#{project.id}/merge_requests", user), params: params
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(json_response['labels']).to eq([])
+        end
+      end
 
       it "returns 422 when source_branch equals target_branch" do
         post api("/projects/#{project.id}/merge_requests", user),
@@ -663,23 +752,6 @@ describe API::MergeRequests do
         expect(response).to have_gitlab_http_status(400)
       end
 
-      it 'allows special label names' do
-        post api("/projects/#{project.id}/merge_requests", user),
-             params: {
-               title: 'Test merge_request',
-               source_branch: 'markdown',
-               target_branch: 'master',
-               author: user,
-               labels: 'label, label?, label&foo, ?, &'
-             }
-        expect(response).to have_gitlab_http_status(201)
-        expect(json_response['labels']).to include 'label'
-        expect(json_response['labels']).to include 'label?'
-        expect(json_response['labels']).to include 'label&foo'
-        expect(json_response['labels']).to include '?'
-        expect(json_response['labels']).to include '&'
-      end
-
       context 'with existing MR' do
         before do
           post api("/projects/#{project.id}/merge_requests", user),
@@ -1122,6 +1194,7 @@ describe API::MergeRequests do
       expect(json_response['force_remove_source_branch']).to be_truthy
     end
 
+    context 'when updating labels' do
       it 'allows special label names' do
         put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
           params: {
@@ -1137,6 +1210,83 @@ describe API::MergeRequests do
         expect(json_response['labels']).to include '&'
       end
 
+      it 'also accepts labels as an array' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
+          params: {
+            title: 'new issue',
+            labels: ['label', 'label?', 'label&foo, ?, &', '1, 2', 3, 4]
+          }
+
+        expect(response.status).to eq(200)
+        expect(json_response['labels']).to include 'label'
+        expect(json_response['labels']).to include 'label?'
+        expect(json_response['labels']).to include 'label&foo'
+        expect(json_response['labels']).to include '?'
+        expect(json_response['labels']).to include '&'
+        expect(json_response['labels']).to include '1'
+        expect(json_response['labels']).to include '2'
+        expect(json_response['labels']).to include '3'
+        expect(json_response['labels']).to include '4'
+      end
+
+      it 'empty label param removes labels' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
+          params: {
+            title: 'new issue',
+            labels: ''
+          }
+
+        expect(response.status).to eq(200)
+        expect(json_response['labels']).to eq []
+      end
+
+      it 'label param as empty array, but only explicitly as json, removes labels' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
+          params: {
+            title: 'new issue',
+            labels: []
+          }.to_json,
+          headers: { 'Content-Type' => 'application/json' }
+
+        expect(response.status).to eq(200)
+        expect(json_response['labels']).to eq []
+      end
+
+      xit 'empty label as array, removes labels' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
+          params: {
+            title: 'new issue',
+            labels: []
+          }
+
+        expect(response.status).to eq(200)
+        # fails, as grape ommits for some reason empty array as optional param value, so nothing it passed along
+        expect(json_response['labels']).to eq []
+      end
+
+      it 'array with one empty string element removes labels' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
+          params: {
+            title: 'new issue',
+            labels: ['']
+          }
+
+        expect(response.status).to eq(200)
+        expect(json_response['labels']).to eq []
+      end
+
+      it 'array with multiple empty string elements, removes labels' do
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user),
+          params: {
+            title: 'new issue',
+            labels: ['', '', '']
+          }
+
+        expect(response.status).to eq(200)
+        expect(json_response['labels']).to eq []
+      end
+    end
+
     it 'does not update state when title is empty' do
       put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), params: { state_event: 'close', title: nil }
 

spec/support/shared_examples/requests/api/merge_requests_list.rb

@@ -186,6 +186,37 @@ shared_examples 'merge requests list' do
       expect(json_response.length).to eq(0)
     end
 
+    it 'returns an array of labeled merge requests where all labels match' do
+      path = endpoint_path + "?labels[]=#{label.title}&labels[]=#{label2.title}"
+
+      get api(path, user)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response).to be_an Array
+      expect(json_response.length).to eq(1)
+      expect(json_response.first['labels']).to eq([label2.title, label.title])
+    end
+
+    it 'returns an array of merge requests with any label when filtering by any label' do
+      get api(endpoint_path, user), params: { labels: [" #{label.title} ", " #{label2.title} "] }
+
+      expect_paginated_array_response
+      expect(json_response).to be_an Array
+      expect(json_response.length).to eq(1)
+      expect(json_response.first['labels']).to eq([label2.title, label.title])
+      expect(json_response.first['id']).to eq(merge_request.id)
+    end
+
+    it 'returns an array of merge requests with any label when filtering by any label' do
+      get api(endpoint_path, user), params: { labels: ["#{label.title} , #{label2.title}"] }
+
+      expect_paginated_array_response
+      expect(json_response).to be_an Array
+      expect(json_response.length).to eq(1)
+      expect(json_response.first['labels']).to eq([label2.title, label.title])
+      expect(json_response.first['id']).to eq(merge_request.id)
+    end
+
     it 'returns an array of merge requests with any label when filtering by any label' do
       get api(endpoint_path, user), params: { labels: IssuesFinder::FILTER_ANY }