Merge branch 'simplify-boards-list-mutation' into 'master'

Remove authorize_list_type_resource method

See merge request gitlab-org/gitlab!49350

app/controllers/boards/lists_controller.rb

@@ -19,12 +19,12 @@ module Boards
     end
 
     def create
-      list = Boards::Lists::CreateService.new(board.resource_parent, current_user, create_list_params).execute(board)
+      response = Boards::Lists::CreateService.new(board.resource_parent, current_user, create_list_params).execute(board)
 
-      if list.valid?
-        render json: serialize_as_json(list)
+      if response.success?
+        render json: serialize_as_json(response.payload[:list])
       else
-        render json: list.errors, status: :unprocessable_entity
+        render json: { errors: response.errors }, status: :unprocessable_entity
       end
     end
 

app/graphql/mutations/boards/lists/create.rb

@@ -27,30 +27,16 @@ module Mutations
           board  = authorized_find!(id: args[:board_id])
           params = create_list_params(args)
 
-          authorize_list_type_resource!(board, params)
-
-          list = create_list(board, params)
+          response = create_list(board, params)
 
           {
-            list: list.valid? ? list : nil,
-            errors: errors_on_object(list)
+            list: response.success? ? response.payload[:list] : nil,
+            errors: response.errors
           }
         end
 
         private
 
-        # Overridden in EE
-        def authorize_list_type_resource!(board, params)
-          return unless params[:label_id]
-
-          labels = ::Labels::AvailableLabelsService.new(current_user, board.resource_parent, params)
-            .filter_labels_ids_in_param(:label_id)
-
-          unless labels.present?
-            raise Gitlab::Graphql::Errors::ArgumentError, 'Label not found!'
-          end
-        end
-
         def create_list(board, params)
           create_list_service =
             ::Boards::Lists::CreateService.new(board.resource_parent, current_user, params)

app/services/boards/lists/create_service.rb

@@ -6,17 +6,21 @@ module Boards
       include Gitlab::Utils::StrongMemoize
 
       def execute(board)
-        List.transaction do
-          case type
-          when :backlog
-            create_backlog(board)
-          else
-            target = target(board)
-            position = next_position(board)
-
-            create_list(board, type, target, position)
-          end
-        end
+        list = case type
+               when :backlog
+                 create_backlog(board)
+               else
+                 target = target(board)
+                 position = next_position(board)
+
+                 return ServiceResponse.error(message: _('%{board_target} not found') % { board_target: type.to_s.capitalize }) if target.blank?
+
+                 create_list(board, type, target, position)
+               end
+
+        return ServiceResponse.error(message: list.errors.full_messages) unless list.persisted?
+
+        ServiceResponse.success(payload: { list: list })
       end
 
       private
@@ -33,7 +37,7 @@ module Boards
 
       def target(board)
         strong_memoize(:target) do
-          available_labels.find(params[:label_id])
+          available_labels.find_by(id: params[:label_id]) # rubocop: disable CodeReuse/ActiveRecord
         end
       end
 

app/services/boards/lists/generate_service.rb

@@ -7,7 +7,11 @@ module Boards
         return false unless board.lists.movable.empty?
 
         List.transaction do
-          label_params.each { |params| create_list(board, params) }
+          label_params.each do |params|
+            response = create_list(board, params)
+
+            raise ActiveRecord::Rollback unless response.success?
+          end
         end
 
         true

ee/app/graphql/ee/mutations/boards/lists/create.rb

@@ -19,27 +19,6 @@ module EE
 
           private
 
-          override :authorize_list_type_resource!
-          def authorize_list_type_resource!(board, params)
-            super
-
-            if params[:milestone_id]
-              milestones = ::Boards::MilestonesFinder.new(board, current_user).execute
-
-              unless milestones.id_in(params[:milestone_id]).exists?
-                raise ::Gitlab::Graphql::Errors::ArgumentError, 'Milestone not found!'
-              end
-            end
-
-            if params[:assignee_id]
-              users = ::Boards::UsersFinder.new(board, current_user).execute
-
-              unless users.with_user(params[:assignee_id]).exists?
-                raise ::Gitlab::Graphql::Errors::ArgumentError, 'User not found!'
-              end
-            end
-          end
-
           override :create_list_params
           def create_list_params(args)
             params = super

ee/app/services/ee/boards/lists/create_service.rb

@@ -50,13 +50,13 @@ module EE
 
         def find_milestone(board)
           milestones = milestone_finder(board).execute
-          milestones.find(params['milestone_id'])
+          milestones.find_by(id: params['milestone_id']) # rubocop: disable CodeReuse/ActiveRecord
         end
 
         # rubocop: disable CodeReuse/ActiveRecord
         def find_user(board)
           user_ids = user_finder(board).execute.select(:user_id)
-          ::User.where(id: user_ids).find(params['assignee_id'])
+          ::User.where(id: user_ids).find_by(id: params['assignee_id'])
         end
         # rubocop: enable CodeReuse/ActiveRecord
 

ee/lib/ee/api/boards_responses.rb

@@ -12,31 +12,6 @@ module EE
             params.slice(:label_id, :milestone_id, :assignee_id)
           end
 
-          # Overrides API::BoardsResponses authorize_list_type_resource!
-          def authorize_list_type_resource!
-            # rubocop: disable CodeReuse/ActiveRecord
-            if params[:label_id] && !available_labels_for(board_parent).exists?(params[:label_id])
-              render_api_error!({ error: 'Label not found!' }, 400)
-            end
-            # rubocop: enable CodeReuse/ActiveRecord
-
-            if params[:milestone_id]
-              milestones = ::Boards::MilestonesFinder.new(board, current_user).execute
-
-              unless milestones.id_in(params[:milestone_id]).exists?
-                render_api_error!({ error: 'Milestone not found!' }, 400)
-              end
-            end
-
-            if params[:assignee_id]
-              users = ::Boards::UsersFinder.new(board, current_user).execute
-
-              unless users.with_user(params[:assignee_id]).exists?
-                render_api_error!({ error: 'User not found!' }, 400)
-              end
-            end
-          end
-
           # Overrides API::BoardsResponses list_creation_params
           params :list_creation_params do
             optional :label_id, type: Integer, desc: 'The ID of an existing label'

ee/spec/controllers/boards/lists_controller_spec.rb

@@ -167,20 +167,22 @@ RSpec.describe Boards::ListsController do
 
     context 'with invalid params' do
       context 'when label is nil' do
-        it 'returns a not found 404 response' do
+        it 'returns an unprocessable entity 422 response' do
           create_board_list user: user, board: board, label_id: nil
 
-          expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response['errors']).to eq(['Label not found'])
         end
       end
 
       context 'when label that does not belongs to group' do
-        it 'returns a not found 404 response' do
+        it 'returns an unprocessable entity 422 response' do
           label = create(:label, name: 'Development')
 
           create_board_list user: user, board: board, label_id: label.id
 
-          expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response['errors']).to eq(['Label not found'])
         end
       end
     end

ee/spec/graphql/ee/mutations/boards/lists/create_spec.rb

@@ -66,9 +66,8 @@ RSpec.describe Mutations::Boards::Lists::Create do
         context 'when milestone not found' do
           let(:list_create_params) { { milestone_id: "gid://gitlab/Milestone/#{non_existing_record_id}" } }
 
-          it 'raises an error' do
-            expect { subject }
-              .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Milestone not found!')
+          it 'returns an error' do
+            expect(subject[:errors]).to include 'Milestone not found'
           end
         end
       end
@@ -97,9 +96,8 @@ RSpec.describe Mutations::Boards::Lists::Create do
         context 'when user not found' do
           let(:list_create_params) { { assignee_id: "gid://gitlab/User/#{non_existing_record_id}" } }
 
-          it 'raises an error' do
-            expect { subject }
-              .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'User not found!')
+          it 'returns an error' do
+            expect(subject[:errors]).to include 'Assignee not found'
           end
         end
       end

ee/spec/services/ee/boards/lists/create_service_spec.rb

@@ -4,47 +4,51 @@ require 'spec_helper'
 
 RSpec.describe Boards::Lists::CreateService do
   describe '#execute' do
-    let(:project) { create(:project) }
-    let(:board) { create(:board, project: project) }
-    let(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:board) { create(:board, project: project) }
+    let_it_be(:user) { create(:user) }
 
     context 'when assignee_id param is sent' do
-      let(:other_user) { create(:user) }
+      let_it_be(:other_user) { create(:user) }
 
-      subject(:service) { described_class.new(project, user, 'assignee_id' => other_user.id) }
-
-      before do
+      before_all do
         project.add_developer(user)
         project.add_developer(other_user)
+      end
+
+      subject(:service) { described_class.new(project, user, 'assignee_id' => other_user.id) }
 
+      before do
         stub_licensed_features(board_assignee_lists: true)
       end
 
       it 'creates a new assignee list' do
-        list = service.execute(board)
+        response = service.execute(board)
 
-        expect(list.list_type).to eq('assignee')
-        expect(list).to be_valid
+        expect(response.success?).to eq(true)
+        expect(response.payload[:list].list_type).to eq('assignee')
       end
     end
 
     context 'when milestone_id param is sent' do
-      let(:user) { create(:user) }
-      let(:milestone) { create(:milestone, project: project) }
+      let_it_be(:user) { create(:user) }
+      let_it_be(:milestone) { create(:milestone, project: project) }
+
+      before_all do
+        project.add_developer(user)
+      end
 
       subject(:service) { described_class.new(project, user, 'milestone_id' => milestone.id) }
 
       before do
-        project.add_developer(user)
-
         stub_licensed_features(board_milestone_lists: true)
       end
 
       it 'creates a milestone list when param is valid' do
-        list = service.execute(board)
+        response = service.execute(board)
 
-        expect(list.list_type).to eq('milestone')
-        expect(list).to be_valid
+        expect(response.success?).to eq(true)
+        expect(response.payload[:list].list_type).to eq('milestone')
       end
     end
 

ee/spec/support/shared_examples/requests/api/assignee_board_list_shared_examples.rb

@@ -7,7 +7,7 @@ RSpec.shared_examples 'assignee board list' do
       post api(url, user), params: { assignee_id: other_user.id }
 
       expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response.dig('message', 'error')).to eq('User not found!')
+      expect(json_response.dig('message', 'error')).to eq('Assignee not found')
     end
 
     it 'returns 400 if assignee list feature is not available' do
@@ -16,8 +16,8 @@ RSpec.shared_examples 'assignee board list' do
       post api(url, user), params: { assignee_id: user.id }
 
       expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response.dig('message', 'list_type'))
-          .to contain_exactly('Assignee lists not available with your current license')
+      expect(json_response.dig('message', 'error'))
+          .to eq('List type Assignee lists not available with your current license')
     end
 
     it 'creates an assignee list if user is found' do

ee/spec/support/shared_examples/requests/api/milestone_board_list_shared_examples.rb

@@ -7,7 +7,7 @@ RSpec.shared_examples 'milestone board list' do
       post api(url, user), params: { milestone_id: other_milestone.id }
 
       expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response.dig('message', 'error')).to eq('Milestone not found!')
+      expect(json_response.dig('message', 'error')).to eq('Milestone not found')
     end
 
     it 'returns 400 if milestone list feature is not available' do
@@ -16,8 +16,8 @@ RSpec.shared_examples 'milestone board list' do
       post api(url, user), params: { milestone_id: milestone.id }
 
       expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response.dig('message', 'list_type'))
-        .to contain_exactly('Milestone lists not available with your current license')
+      expect(json_response.dig('message', 'error'))
+        .to eq('List type Milestone lists not available with your current license')
     end
 
     it 'creates a milestone list if milestone is found' do

lib/api/boards.rb

@@ -117,8 +117,6 @@ module API
           use :list_creation_params
         end
         post '/lists' do
-          authorize_list_type_resource!
-
           authorize!(:admin_list, user_project)
 
           create_list

lib/api/boards_responses.rb

@@ -47,12 +47,12 @@ module API
           create_list_service =
             ::Boards::Lists::CreateService.new(board_parent, current_user, create_list_params)
 
-          list = create_list_service.execute(board)
+          response = create_list_service.execute(board)
 
-          if list.valid?
-            present list, with: Entities::List
+          if response.success?
+            present response.payload[:list], with: Entities::List
           else
-            render_validation_error!(list)
+            render_api_error!({ error: response.errors.first }, 400)
           end
         end
 
@@ -80,14 +80,6 @@ module API
           end
         end
 
-        # rubocop: disable CodeReuse/ActiveRecord
-        def authorize_list_type_resource!
-          unless available_labels_for(board_parent).exists?(params[:label_id])
-            render_api_error!({ error: 'Label not found!' }, 400)
-          end
-        end
-        # rubocop: enable CodeReuse/ActiveRecord
-
         params :list_creation_params do
           requires :label_id, type: Integer, desc: 'The ID of an existing label'
         end

lib/api/group_boards.rb

@@ -83,8 +83,6 @@ module API
           use :list_creation_params
         end
         post '/lists' do
-          authorize_list_type_resource!
-
           authorize!(:admin_list, user_group)
 
           create_list

locale/gitlab.pot

@@ -371,6 +371,9 @@ msgstr ""
 msgid "%{authorsName}'s thread"
 msgstr ""
 
+msgid "%{board_target} not found"
+msgstr ""
+
 msgid "%{code_open}Masked%{code_close} variables are hidden in job logs (though they must match certain regexp requirements to do so)."
 msgstr ""
 

spec/controllers/boards/lists_controller_spec.rb

@@ -85,20 +85,22 @@ RSpec.describe Boards::ListsController do
 
     context 'with invalid params' do
       context 'when label is nil' do
-        it 'returns a not found 404 response' do
+        it 'returns an unprocessable entity 422 response' do
           create_board_list user: user, board: board, label_id: nil
 
-          expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response['errors']).to eq(['Label not found'])
         end
       end
 
       context 'when label that does not belongs to project' do
-        it 'returns a not found 404 response' do
+        it 'returns an unprocessable entity 422 response' do
           label = create(:label, name: 'Development')
 
           create_board_list user: user, board: board, label_id: label.id
 
-          expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+          expect(json_response['errors']).to eq(['Label not found'])
         end
       end
     end

spec/graphql/mutations/boards/lists/create_spec.rb

@@ -68,9 +68,8 @@ RSpec.describe Mutations::Boards::Lists::Create do
         context 'when label not found' do
           let(:list_create_params) { { label_id: "gid://gitlab/Label/#{non_existing_record_id}" } }
 
-          it 'raises an error' do
-            expect { subject }
-              .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Label not found!')
+          it 'returns an error' do
+            expect(subject[:errors]).to include 'Label not found'
           end
         end
       end

spec/services/boards/lists/create_service_spec.rb

@@ -5,27 +5,29 @@ require 'spec_helper'
 RSpec.describe Boards::Lists::CreateService do
   describe '#execute' do
     shared_examples 'creating board lists' do
-      let(:user)    { create(:user) }
+      let_it_be(:user) { create(:user) }
 
-      subject(:service) { described_class.new(parent, user, label_id: label.id) }
-
-      before do
+      before_all do
         parent.add_developer(user)
       end
 
+      subject(:service) { described_class.new(parent, user, label_id: label.id) }
+
       context 'when board lists is empty' do
         it 'creates a new list at beginning of the list' do
-          list = service.execute(board)
+          response = service.execute(board)
 
-          expect(list.position).to eq 0
+          expect(response.success?).to eq(true)
+          expect(response.payload[:list].position).to eq 0
         end
       end
 
       context 'when board lists has the done list' do
         it 'creates a new list at beginning of the list' do
-          list = service.execute(board)
+          response = service.execute(board)
 
-          expect(list.position).to eq 0
+          expect(response.success?).to eq(true)
+          expect(response.payload[:list].position).to eq 0
         end
       end
 
@@ -34,9 +36,10 @@ RSpec.describe Boards::Lists::CreateService do
           create(:list, board: board, position: 0)
           create(:list, board: board, position: 1)
 
-          list = service.execute(board)
+          response = service.execute(board)
 
-          expect(list.position).to eq 2
+          expect(response.success?).to eq(true)
+          expect(response.payload[:list].position).to eq 2
         end
       end
 
@@ -44,32 +47,35 @@ RSpec.describe Boards::Lists::CreateService do
         it 'creates a new list at end of the label lists' do
           list1 = create(:list, board: board, position: 0)
 
-          list2 = service.execute(board)
+          list2 = service.execute(board).payload[:list]
 
           expect(list1.reload.position).to eq 0
           expect(list2.reload.position).to eq 1
         end
       end
 
-      context 'when provided label does not belongs to the parent' do
-        it 'raises an error' do
+      context 'when provided label does not belong to the parent' do
+        it 'returns an error' do
           label = create(:label, name: 'in-development')
           service = described_class.new(parent, user, label_id: label.id)
 
-          expect { service.execute(board) }.to raise_error(ActiveRecord::RecordNotFound)
+          response = service.execute(board)
+
+          expect(response.success?).to eq(false)
+          expect(response.errors).to include('Label not found')
         end
       end
 
       context 'when backlog param is sent' do
         it 'creates one and only one backlog list' do
           service = described_class.new(parent, user, 'backlog' => true)
-          list = service.execute(board)
+          list = service.execute(board).payload[:list]
 
           expect(list.list_type).to eq('backlog')
           expect(list.position).to be_nil
           expect(list).to be_valid
 
-          another_backlog = service.execute(board)
+          another_backlog = service.execute(board).payload[:list]
 
           expect(another_backlog).to eq list
         end
@@ -77,17 +83,17 @@ RSpec.describe Boards::Lists::CreateService do
     end
 
     context 'when board parent is a project' do
-      let(:parent) { create(:project) }
-      let(:board) { create(:board, project: parent) }
-      let(:label) { create(:label, project: parent, name: 'in-progress') }
+      let_it_be(:parent) { create(:project) }
+      let_it_be(:board) { create(:board, project: parent) }
+      let_it_be(:label) { create(:label, project: parent, name: 'in-progress') }
 
       it_behaves_like 'creating board lists'
     end
 
     context 'when board parent is a group' do
-      let(:parent) { create(:group) }
-      let(:board) { create(:board, group: parent) }
-      let(:label) { create(:group_label, group: parent, name: 'in-progress') }
+      let_it_be(:parent) { create(:group) }
+      let_it_be(:board) { create(:board, group: parent) }
+      let_it_be(:label) { create(:group_label, group: parent, name: 'in-progress') }
 
       it_behaves_like 'creating board lists'
     end