Merge branch '346042-refactor-create-service' into 'master'

Refactor WorkItems::CreateService

See merge request gitlab-org/gitlab!80309

app/graphql/mutations/work_items/create.rb

@@ -32,13 +32,13 @@ module Mutations
         params = global_id_compatibility_params(attributes).merge(author_id: current_user.id)
 
         spam_params = ::Spam::SpamParams.new_from_request(request: context[:request])
-        work_item = ::WorkItems::CreateService.new(project: project, current_user: current_user, params: params, spam_params: spam_params).execute
+        create_result = ::WorkItems::CreateService.new(project: project, current_user: current_user, params: params, spam_params: spam_params).execute
 
-        check_spam_action_response!(work_item)
+        check_spam_action_response!(create_result[:work_item]) if create_result[:work_item]
 
         {
-          work_item: work_item.valid? ? work_item : nil,
-          errors: errors_on_object(work_item)
+          work_item: create_result.success? ? create_result[:work_item] : nil,
+          errors: create_result.errors
         }
       end
 

app/services/work_items/create_service.rb

@@ -2,6 +2,8 @@
 
 module WorkItems
   class CreateService
+    include ::Services::ReturnServiceResponses
+
     def initialize(project:, current_user: nil, params: {}, spam_params:)
       @create_service = ::Issues::CreateService.new(
         project: project,
@@ -10,10 +12,28 @@ module WorkItems
         spam_params: spam_params,
         build_service: ::WorkItems::BuildService.new(project: project, current_user: current_user, params: params)
       )
+      @current_user = current_user
+      @project = project
     end
 
     def execute
-      @create_service.execute
+      unless @current_user.can?(:create_work_item, @project)
+        return error(_('Operation not allowed'), :forbidden)
+      end
+
+      work_item = @create_service.execute
+
+      if work_item.valid?
+        success(payload(work_item))
+      else
+        error(work_item.errors.full_messages, :unprocessable_entity, pass_back: payload(work_item))
+      end
+    end
+
+    private
+
+    def payload(work_item)
+      { work_item: work_item }
     end
   end
 end

spec/requests/api/graphql/mutations/work_items/create_spec.rb

@@ -47,6 +47,18 @@ RSpec.describe 'Create a work item' do
       )
     end
 
+    context 'when input is invalid' do
+      let(:input) { { 'title' => '', 'workItemTypeId' => WorkItems::Type.default_by_type(:task).to_global_id.to_s } }
+
+      it 'does not create and returns validation errors' do
+        expect do
+          post_graphql_mutation(mutation, current_user: current_user)
+        end.to not_change(WorkItem, :count)
+
+        expect(graphql_mutation_response(:work_item_create)['errors']).to contain_exactly("Title can't be blank")
+      end
+    end
+
     it_behaves_like 'has spam protection' do
       let(:mutation_class) { ::Mutations::WorkItems::Create }
     end

spec/services/work_items/create_service_spec.rb

@@ -5,34 +5,46 @@ require 'spec_helper'
 RSpec.describe WorkItems::CreateService do
   include AfterNextHelpers
 
-  let_it_be(:group) { create(:group) }
-  let_it_be_with_reload(:project) { create(:project, group: group) }
-  let_it_be(:user) { create(:user) }
+  let_it_be_with_reload(:project) { create(:project) }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:user_with_no_access) { create(:user) }
 
   let(:spam_params) { double }
+  let(:current_user) { guest }
+  let(:opts) do
+    {
+      title: 'Awesome work_item',
+      description: 'please fix'
+    }
+  end
+
+  before_all do
+    project.add_guest(guest)
+  end
 
   describe '#execute' do
-    let(:work_item) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute }
+    subject(:service_result) { described_class.new(project: project, current_user: current_user, params: opts, spam_params: spam_params).execute }
 
     before do
       stub_spam_services
     end
 
-    context 'when params are valid' do
-      before_all do
-        project.add_guest(user)
-      end
+    context 'when user is not allowed to create a work item in the project' do
+      let(:current_user) { user_with_no_access }
+
+      it { is_expected.to be_error }
 
-      let(:opts) do
-        {
-          title: 'Awesome work_item',
-          description: 'please fix'
-        }
+      it 'returns an access error' do
+        expect(service_result.errors).to contain_exactly('Operation not allowed')
       end
+    end
 
+    context 'when params are valid' do
       it 'created instance is a WorkItem' do
         expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute)
 
+        work_item = service_result[:work_item]
+
         expect(work_item).to be_persisted
         expect(work_item).to be_a(::WorkItem)
         expect(work_item.title).to eq('Awesome work_item')
@@ -41,17 +53,17 @@ RSpec.describe WorkItems::CreateService do
       end
     end
 
-    context 'checking spam' do
-      let(:params) do
-        {
-          title: 'Spam work_item'
-        }
-      end
+    context 'when params are invalid' do
+      let(:opts) { { title: '' } }
 
-      subject do
-        described_class.new(project: project, current_user: user, params: params, spam_params: spam_params)
+      it { is_expected.to be_error }
+
+      it 'returns validation errors' do
+        expect(service_result.errors).to contain_exactly("Title can't be blank")
       end
+    end
 
+    context 'checking spam' do
       it 'executes SpamActionService' do
         expect_next_instance_of(
           Spam::SpamActionService,
@@ -65,7 +77,7 @@ RSpec.describe WorkItems::CreateService do
           expect(instance).to receive(:execute)
         end
 
-        subject.execute
+        service_result
       end
     end
   end