From e7355e6d92accfdf3f5e798add646e0cc42d49d4 Mon Sep 17 00:00:00 2001 From: Mantas Date: Wed, 6 Oct 2021 08:54:33 +0300 Subject: [PATCH] Fixes #2421 - API creates empty tickets without articles if data is missing --- .../concerns/creates_ticket_articles.rb | 2 +- app/controllers/tickets_controller.rb | 181 +++++++++--------- spec/requests/ticket_spec.rb | 59 +++--- 3 files changed, 124 insertions(+), 118 deletions(-) diff --git a/app/controllers/concerns/creates_ticket_articles.rb b/app/controllers/concerns/creates_ticket_articles.rb index cefa260d1..9cab02287 100644 --- a/app/controllers/concerns/creates_ticket_articles.rb +++ b/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? diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 460fc5f2a..3ed13ed30 100644 --- a/app/controllers/tickets_controller.rb +++ b/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 + ticket = nil - clean_params = Ticket.association_name_to_id_convert(params) + Transaction.execute do # rubocop:disable Metrics/BlockLength + customer = {} + if params[:customer].instance_of?(ActionController::Parameters) + customer = params[:customer] + params.delete(:customer) + end - # 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 + clean_params = Ticket.association_name_to_id_convert(params) - # 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 + # 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 - 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 - # 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]) + # 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 - 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 - 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 + # 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_types_with_object_ids.each do |link_type, object_ids| - raise Exceptions::UnprocessableEntity, __('Invalid link structure (Object->LinkType)') if !object_ids.is_a? Array + 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 - 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, - ) + 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 diff --git a/spec/requests/ticket_spec.rb b/spec/requests/ticket_spec.rb index bf4d3754b..eec245154 100644 --- a/spec/requests/ticket_spec.rb +++ b/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',