Fixes #2421 - API creates empty tickets without articles if data is missing

app/controllers/concerns/creates_ticket_articles.rb

@@ -13,7 +13,7 @@ module CreatesTicketArticles
     subtype = params.delete(:subtype)
 
     # check min. params
-    raise Exceptions::UnprocessableEntity, __('Need at least article: { body: "some text" }') if !params[:body]
+    raise Exceptions::UnprocessableEntity, __('Need at least article: { body: "some text" }') if params[:body].blank?
 
     # fill default values
     if params[:type_id].blank? && params[:type].blank?

app/controllers/tickets_controller.rb

@@ -72,83 +72,79 @@ class TicketsController < ApplicationController
 
   # POST /api/v1/tickets
   def create
-    customer = {}
-    if params[:customer].instance_of?(ActionController::Parameters)
-      customer = params[:customer]
-      params.delete(:customer)
-    end
-
-    clean_params = Ticket.association_name_to_id_convert(params)
+    ticket = nil
 
-    # overwrite params
-    if !current_user.permissions?('ticket.agent')
-      %i[owner owner_id customer customer_id organization organization_id preferences].each do |key|
-        clean_params.delete(key)
+    Transaction.execute do # rubocop:disable Metrics/BlockLength
+      customer = {}
+      if params[:customer].instance_of?(ActionController::Parameters)
+        customer = params[:customer]
+        params.delete(:customer)
       end
-      clean_params[:customer_id] = current_user.id
-    end
 
-    # The parameter :customer_id is 'abused' in cases where it is not an integer, but a string like
-    #   'guess:customers.email@domain.com' which implies that the customer should be looked up.
-    if clean_params[:customer_id].is_a?(String) && clean_params[:customer_id] =~ %r{^guess:(.+?)$}
-      email_address = $1
-      email_address_validation = EmailAddressValidation.new(email_address)
-      if !email_address_validation.valid_format?
-        render json: { error: "Invalid email '#{email_address}' of customer" }, status: :unprocessable_entity
-        return
-      end
-      local_customer = User.find_by(email: email_address.downcase)
-      if !local_customer
-        role_ids = Role.signup_role_ids
-        local_customer = User.create(
-          firstname: '',
-          lastname:  '',
-          email:     email_address,
-          password:  '',
-          active:    true,
-          role_ids:  role_ids,
-        )
-      end
-      clean_params[:customer_id] = local_customer.id
-    end
+      clean_params = Ticket.association_name_to_id_convert(params)
 
-    # try to create customer if needed
-    if clean_params[:customer_id].blank? && customer.present?
-      check_attributes_by_current_user_permission(customer)
-      clean_customer = User.association_name_to_id_convert(customer)
-      local_customer = nil
-      if !local_customer && clean_customer[:id].present?
-        local_customer = User.find_by(id: clean_customer[:id])
-      end
-      if !local_customer && clean_customer[:email].present?
-        local_customer = User.find_by(email: clean_customer[:email].downcase)
-      end
-      if !local_customer && clean_customer[:login].present?
-        local_customer = User.find_by(login: clean_customer[:login].downcase)
+      # overwrite params
+      if !current_user.permissions?('ticket.agent')
+        %i[owner owner_id customer customer_id organization organization_id preferences].each do |key|
+          clean_params.delete(key)
+        end
+        clean_params[:customer_id] = current_user.id
       end
-      if !local_customer
-        role_ids = Role.signup_role_ids
-        local_customer = User.new(clean_customer)
-        local_customer.role_ids = role_ids
-        local_customer.save!
+
+      # The parameter :customer_id is 'abused' in cases where it is not an integer, but a string like
+      #   'guess:customers.email@domain.com' which implies that the customer should be looked up.
+      if clean_params[:customer_id].is_a?(String) && clean_params[:customer_id] =~ %r{^guess:(.+?)$}
+        email_address = $1
+        email_address_validation = EmailAddressValidation.new(email_address)
+        if !email_address_validation.valid_format?
+          render json: { error: "Invalid email '#{email_address}' of customer" }, status: :unprocessable_entity
+          return
+        end
+        local_customer = User.find_by(email: email_address.downcase)
+        if !local_customer
+          role_ids = Role.signup_role_ids
+          local_customer = User.create(
+            firstname: '',
+            lastname:  '',
+            email:     email_address,
+            password:  '',
+            active:    true,
+            role_ids:  role_ids,
+          )
+        end
+        clean_params[:customer_id] = local_customer.id
       end
-      clean_params[:customer_id] = local_customer.id
-    end
 
-    clean_params = Ticket.param_cleanup(clean_params, true)
-    clean_params[:screen] = 'create_middle'
-    ticket = Ticket.new(clean_params)
-    authorize!(ticket, :create?)
+      # try to create customer if needed
+      if clean_params[:customer_id].blank? && customer.present?
+        check_attributes_by_current_user_permission(customer)
+        clean_customer = User.association_name_to_id_convert(customer)
+        local_customer = nil
+        if !local_customer && clean_customer[:id].present?
+          local_customer = User.find_by(id: clean_customer[:id])
+        end
+        if !local_customer && clean_customer[:email].present?
+          local_customer = User.find_by(email: clean_customer[:email].downcase)
+        end
+        if !local_customer && clean_customer[:login].present?
+          local_customer = User.find_by(login: clean_customer[:login].downcase)
+        end
+        if !local_customer
+          role_ids = Role.signup_role_ids
+          local_customer = User.new(clean_customer)
+          local_customer.role_ids = role_ids
+          local_customer.save!
+        end
+        clean_params[:customer_id] = local_customer.id
+      end
 
-    # check if article is given
-    if !params[:article]
-      render json: { error: 'article hash is missing' }, status: :unprocessable_entity
-      return
-    end
+      clean_params = Ticket.param_cleanup(clean_params, true)
+      clean_params[:screen] = 'create_middle'
+      ticket = Ticket.new(clean_params)
+      authorize!(ticket, :create?)
 
-    # create ticket
-    ticket.save!
-    ticket.with_lock do
+      # create ticket
+      ticket.save!
 
       # create tags if given
       if params[:tags].present?
@@ -170,33 +166,34 @@ class TicketsController < ApplicationController
       if params[:article]
         article_create(ticket, params[:article])
       end
-    end
-    # create links (e. g. in case of ticket split)
-    # links: {
-    #   Ticket: {
-    #     parent: [ticket_id1, ticket_id2, ...]
-    #     normal: [ticket_id1, ticket_id2, ...]
-    #     child: [ticket_id1, ticket_id2, ...]
-    #   },
-    # }
-    if params[:links].present?
-      link = params[:links].permit!.to_h
-      raise Exceptions::UnprocessableEntity, __('Invalid link structure') if !link.is_a? Hash
-
-      link.each do |target_object, link_types_with_object_ids|
-        raise Exceptions::UnprocessableEntity, __('Invalid link structure (Object)') if !link_types_with_object_ids.is_a? Hash
-
-        link_types_with_object_ids.each do |link_type, object_ids|
-          raise Exceptions::UnprocessableEntity, __('Invalid link structure (Object->LinkType)') if !object_ids.is_a? Array
-
-          object_ids.each do |local_object_id|
-            link = Link.add(
-              link_type:                link_type,
-              link_object_target:       target_object,
-              link_object_target_value: local_object_id,
-              link_object_source:       'Ticket',
-              link_object_source_value: ticket.id,
-            )
+
+      # create links (e. g. in case of ticket split)
+      # links: {
+      #   Ticket: {
+      #     parent: [ticket_id1, ticket_id2, ...]
+      #     normal: [ticket_id1, ticket_id2, ...]
+      #     child: [ticket_id1, ticket_id2, ...]
+      #   },
+      # }
+      if params[:links].present?
+        link = params[:links].permit!.to_h
+        raise Exceptions::UnprocessableEntity, __('Invalid link structure') if !link.is_a? Hash
+
+        link.each do |target_object, link_types_with_object_ids|
+          raise Exceptions::UnprocessableEntity, __('Invalid link structure (Object)') if !link_types_with_object_ids.is_a? Hash
+
+          link_types_with_object_ids.each do |link_type, object_ids|
+            raise Exceptions::UnprocessableEntity, __('Invalid link structure (Object->LinkType)') if !object_ids.is_a? Array
+
+            object_ids.each do |local_object_id|
+              link = Link.add(
+                link_type:                link_type,
+                link_object_target:       target_object,
+                link_object_target_value: local_object_id,
+                link_object_source:       'Ticket',
+                link_object_source_value: ticket.id,
+              )
+            end
           end
         end
       end

spec/requests/ticket_spec.rb

@@ -101,12 +101,45 @@ RSpec.describe 'Ticket', type: :request do
         article:     {},
       }
       authenticated_as(agent)
-      post '/api/v1/tickets', params: params, as: :json
+      expect { post '/api/v1/tickets', params: params, as: :json }.not_to change(Ticket, :count)
       expect(response).to have_http_status(:unprocessable_entity)
       expect(json_response).to be_a_kind_of(Hash)
       expect(json_response['error']).to eq('Need at least article: { body: "some text" }')
     end
 
+    it 'does ticket create with agent - article.body set to empty string (01.03)' do
+      params = {
+        title:       'a new ticket #3',
+        group:       ticket_group.name,
+        priority:    '2 normal',
+        state:       'new',
+        customer_id: customer.id,
+        article:     { body: " \n " },
+      }
+      authenticated_as(agent)
+      expect { post '/api/v1/tickets', params: params, as: :json }.not_to change(Ticket, :count)
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json_response).to be_a_kind_of(Hash)
+      expect(json_response['error']).to eq('Need at least article: { body: "some text" }')
+    end
+
+    it 'does ticket create with agent - missing article (01.03)' do
+      params = {
+        title:       'a new ticket #3',
+        group:       ticket_group.name,
+        priority:    '2 normal',
+        state:       'new',
+        customer_id: customer.id
+      }
+      authenticated_as(agent)
+      expect { post '/api/v1/tickets', params: params, as: :json }.to change(Ticket, :count).by(1)
+      expect(response).to have_http_status(:created)
+      expect(json_response).to be_a_kind_of(Hash)
+
+      ticket = Ticket.find(json_response['id'])
+      expect(ticket.articles).to be_empty
+    end
+
     it 'does ticket create with agent - minimal article (01.03)' do
       params = {
         title:       'a new ticket #3',
@@ -151,30 +184,6 @@ RSpec.describe 'Ticket', type: :request do
       expect(json_response['created_by_id']).to eq(agent.id)
     end
 
-    it 'does ticket create with empty article body' do
-      params = {
-        title:    'a new ticket with empty article body',
-        group:    ticket_group.name,
-        priority: '2 normal',
-        state:    'new',
-        customer: customer.email,
-        article:  { body: '' }
-      }
-      authenticated_as(agent)
-      post '/api/v1/tickets', params: params, as: :json
-      expect(response).to have_http_status(:created)
-      expect(json_response).to be_a_kind_of(Hash)
-      expect(json_response['state_id']).to eq(Ticket::State.lookup(name: 'new').id)
-      expect(json_response['title']).to eq('a new ticket with empty article body')
-      expect(json_response['customer_id']).to eq(customer.id)
-      expect(json_response['updated_by_id']).to eq(agent.id)
-      expect(json_response['created_by_id']).to eq(agent.id)
-      ticket = Ticket.find(json_response['id'])
-      expect(ticket.articles.count).to eq(1)
-      article = ticket.articles.first
-      expect(article.body).to eq('')
-    end
-
     it 'does ticket create with agent - wrong owner_id - 0 (01.05)' do
       params = {
         title:       'a new ticket #4',