From 2078a079cd05c52bb3b9bd541e44067682fa32ab Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 24 Aug 2016 13:42:22 +0200 Subject: [PATCH] Improved validation messages of controllers. --- app/controllers/application_controller.rb | 81 +++++++ app/controllers/ticket_articles_controller.rb | 82 ++++--- app/controllers/tickets_controller.rb | 79 ------- config/routes/ticket.rb | 1 + test/controllers/tickets_controller_test.rb | 222 ++++++++++++++++++ 5 files changed, 350 insertions(+), 115 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 288b4971b..2c4bad688 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -356,6 +356,87 @@ class ApplicationController < ActionController::Base raise Exceptions::NotAuthorized end + def article_create(ticket, params) + + # create article if given + form_id = params[:form_id] + params.delete(:form_id) + + # check min. params + raise 'Need at least article: { body: "some text" }' if !params[:body] + + # fill default values + if params[:type_id].empty? && params[:type].empty? + params[:type_id] = Ticket::Article::Type.lookup(name: 'note').id + end + if params[:sender_id].empty? && params[:sender].empty? + sender = 'Customer' + if current_user.permissions?('ticket.agent') + sender = 'Agent' + end + params[:sender_id] = Ticket::Article::Sender.lookup(name: sender).id + end + + clean_params = Ticket::Article.param_association_lookup(params) + clean_params = Ticket::Article.param_cleanup(clean_params, true) + + # overwrite params + if !current_user.permissions?('ticket.agent') + clean_params[:sender_id] = Ticket::Article::Sender.lookup(name: 'Customer').id + clean_params.delete(:sender) + type = Ticket::Article::Type.lookup(id: clean_params[:type_id]) + if type.name !~ /^(note|web)$/ + clean_params[:type_id] = Ticket::Article::Type.lookup(name: 'note').id + end + clean_params.delete(:type) + clean_params[:internal] = false + end + + article = Ticket::Article.new(clean_params) + article.ticket_id = ticket.id + + # store dataurl images to store + if form_id && article.body && article.content_type =~ %r{text/html}i + article.body.gsub!( %r{(}i ) { |_item| + file_attributes = StaticAssets.data_url_attributes($2) + cid = "#{ticket.id}.#{form_id}.#{rand(999_999)}@#{Setting.get('fqdn')}" + headers_store = { + 'Content-Type' => file_attributes[:mime_type], + 'Mime-Type' => file_attributes[:mime_type], + 'Content-ID' => cid, + 'Content-Disposition' => 'inline', + } + store = Store.add( + object: 'UploadCache', + o_id: form_id, + data: file_attributes[:content], + filename: cid, + preferences: headers_store + ) + "#{$1}cid:#{cid}\">" + } + end + + # find attachments in upload cache + if form_id + article.attachments = Store.list( + object: 'UploadCache', + o_id: form_id, + ) + end + article.save! + + # remove attachments from upload cache + return article if !form_id + + Store.remove( + object: 'UploadCache', + o_id: form_id, + ) + + article + end + def permission_check(key) if @_token_auth user = Token.check( diff --git a/app/controllers/ticket_articles_controller.rb b/app/controllers/ticket_articles_controller.rb index 2bbc204ae..59ce08262 100644 --- a/app/controllers/ticket_articles_controller.rb +++ b/app/controllers/ticket_articles_controller.rb @@ -18,10 +18,7 @@ class TicketArticlesController < ApplicationController if params[:expand] result = article.attributes_with_relation_names - - # add attachments result[:attachments] = article.attachments - render json: result, status: :ok return end @@ -49,7 +46,6 @@ class TicketArticlesController < ApplicationController # ignore internal article if customer is requesting next if article.internal == true && current_user.permissions?('ticket.customer') - result = article.attributes_with_relation_names # add attachments @@ -83,7 +79,6 @@ class TicketArticlesController < ApplicationController # ignore internal article if customer is requesting next if article.internal == true && current_user.permissions?('ticket.customer') - articles.push article.attributes_with_relation_names } render json: articles @@ -91,35 +86,24 @@ class TicketArticlesController < ApplicationController # POST /articles def create - form_id = params[:form_id] + ticket = Ticket.find(params[:ticket_id]) + ticket_permission(ticket) + article = article_create(ticket, params) - clean_params = Ticket::Article.param_association_lookup(params) - clean_params = Ticket::Article.param_cleanup(clean_params, true) - article = Ticket::Article.new(clean_params) - - # permission check - article_permission(article) - - # find attachments in upload cache - if form_id - article.attachments = Store.list( - object: 'UploadCache', - o_id: form_id, - ) + if params[:expand] + result = article.attributes_with_relation_names + result[:attachments] = article.attachments + render json: result, status: :created + return end - if article.save - - # remove attachments from upload cache - Store.remove( - object: 'UploadCache', - o_id: form_id, - ) - - render json: article, status: :created - else - render json: article.errors, status: :unprocessable_entity + if params[:full] + full = Ticket::Article.full(params[:id]) + render json: full, status: :created + return end + + render json: article.attributes_with_relation_names, status: :created end # PUT /articles/1 @@ -129,23 +113,49 @@ class TicketArticlesController < ApplicationController article = Ticket::Article.find(params[:id]) article_permission(article) + if !current_user.permissions?('ticket.agent') && !current_user.permissions?('admin') + raise Exceptions::NotAuthorized, 'Not authorized (ticket.agent or admin permission required)!' + end + clean_params = Ticket::Article.param_association_lookup(params) clean_params = Ticket::Article.param_cleanup(clean_params, true) - if article.update_attributes(clean_params) - render json: article, status: :ok - else - render json: article.errors, status: :unprocessable_entity + article.update_attributes!(clean_params) + + if params[:expand] + result = article.attributes_with_relation_names + result[:attachments] = article.attachments + render json: result, status: :ok + return end + + if params[:full] + full = Ticket::Article.full(params[:id]) + render json: full, status: :ok + return + end + + render json: article.attributes_with_relation_names, status: :ok end # DELETE /articles/1 def destroy article = Ticket::Article.find(params[:id]) article_permission(article) - article.destroy - head :ok + if current_user.permissions?('admin') + article.destroy! + head :ok + return + end + + if current_user.permissions?('ticket.agent') && article.created_by_id == current_user.id && article.type.name == 'note' + article.destroy! + head :ok + return + end + + raise Exceptions::NotAuthorized, 'Not authorized (admin permission required)!' end # DELETE /ticket_attachment_upload diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 62d7b4bf6..829052d0b 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -617,85 +617,6 @@ class TicketsController < ApplicationController ticket_ids end - def article_create(ticket, params) - - # create article if given - form_id = params[:form_id] - params.delete(:form_id) - - # check min. params - raise 'Need at least article: { body: "some text" }' if !params[:body] - - # fill default values - if params[:type_id].empty? - params[:type_id] = Ticket::Article::Type.lookup(name: 'note').id - end - if params[:sender_id].empty? - sender = 'Customer' - if current_user.permissions?('ticket.agent') - sender = 'Agent' - end - params[:sender_id] = Ticket::Article::Sender.lookup(name: sender).id - end - - clean_params = Ticket::Article.param_association_lookup(params) - clean_params = Ticket::Article.param_cleanup(clean_params, true) - - # overwrite params - if !current_user.permissions?('ticket.agent') - clean_params[:sender_id] = Ticket::Article::Sender.lookup(name: 'Customer').id - clean_params.delete(:sender) - type = Ticket::Article::Type.lookup(id: clean_params[:type_id]) - if type !~ /^(note|web)$/ - clean_params[:type_id] = Ticket::Article::Type.lookup(name: 'note').id - end - clean_params.delete(:type) - clean_params[:internal] = false - end - - article = Ticket::Article.new(clean_params) - article.ticket_id = ticket.id - - # store dataurl images to store - if form_id && article.body && article.content_type =~ %r{text/html}i - article.body.gsub!( %r{(}i ) { |_item| - file_attributes = StaticAssets.data_url_attributes($2) - cid = "#{ticket.id}.#{form_id}.#{rand(999_999)}@#{Setting.get('fqdn')}" - headers_store = { - 'Content-Type' => file_attributes[:mime_type], - 'Mime-Type' => file_attributes[:mime_type], - 'Content-ID' => cid, - 'Content-Disposition' => 'inline', - } - store = Store.add( - object: 'UploadCache', - o_id: form_id, - data: file_attributes[:content], - filename: cid, - preferences: headers_store - ) - "#{$1}cid:#{cid}\">" - } - end - - # find attachments in upload cache - if form_id - article.attachments = Store.list( - object: 'UploadCache', - o_id: form_id, - ) - end - article.save! - - # remove attachments from upload cache - return if !form_id - - Store.remove( - object: 'UploadCache', - o_id: form_id, - ) - end - def ticket_all(ticket) # get attributes to update diff --git a/config/routes/ticket.rb b/config/routes/ticket.rb index 882538d37..6dc60d9b3 100644 --- a/config/routes/ticket.rb +++ b/config/routes/ticket.rb @@ -40,6 +40,7 @@ Zammad::Application.routes.draw do match api_path + '/ticket_articles/by_ticket/:id', to: 'ticket_articles#index_by_ticket', via: :get match api_path + '/ticket_articles', to: 'ticket_articles#create', via: :post match api_path + '/ticket_articles/:id', to: 'ticket_articles#update', via: :put + match api_path + '/ticket_articles/:id', to: 'ticket_articles#destroy', via: :delete match api_path + '/ticket_attachment/:ticket_id/:article_id/:id', to: 'ticket_articles#attachment', via: :get match api_path + '/ticket_attachment_upload', to: 'ticket_articles#ticket_attachment_upload_add', via: :post match api_path + '/ticket_attachment_upload', to: 'ticket_articles#ticket_attachment_upload_delete', via: :delete diff --git a/test/controllers/tickets_controller_test.rb b/test/controllers/tickets_controller_test.rb index 094b10164..314620696 100644 --- a/test/controllers/tickets_controller_test.rb +++ b/test/controllers/tickets_controller_test.rb @@ -123,6 +123,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(Ticket::State.lookup(name: 'new').id, result['state_id']) assert_equal('a new ticket #3', result['title']) assert_equal(@customer_without_org.id, result['customer_id']) + assert_equal(@agent.id, result['updated_by_id']) + assert_equal(@agent.id, result['created_by_id']) end test '02.02 ticket create with agent' do @@ -149,6 +151,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(Hash, result.class) assert_equal(Ticket::State.lookup(name: 'new').id, result['state_id']) assert_equal('a new ticket #1', result['title']) + assert_equal(@agent.id, result['updated_by_id']) + assert_equal(@agent.id, result['created_by_id']) links = Link.list( link_object: 'Ticket', link_object_value: result['id'], @@ -215,6 +219,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(ticket.id, result['id']) assert_equal('ticket with corret ticket id', result['title']) assert_equal(ticket.customer_id, result['customer_id']) + assert_equal(1, result['updated_by_id']) + assert_equal(1, result['created_by_id']) params = { title: 'ticket with corret ticket id - 2', @@ -227,6 +233,71 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(ticket.id, result['id']) assert_equal('ticket with corret ticket id - 2', result['title']) assert_equal(@agent.id, result['customer_id']) + assert_equal(@agent.id, result['updated_by_id']) + assert_equal(1, result['created_by_id']) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + } + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(false, result['internal']) + assert_equal(@agent.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Agent').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'note').id, result['type_id']) + + delete "/api/v1/ticket_articles/#{result['id']}", {}.to_json, @headers.merge('Authorization' => credentials) + assert_response(200) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + type: 'email', + internal: true, + } + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(true, result['internal']) + assert_equal(@agent.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Agent').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'email').id, result['type_id']) + + params = { + subject: 'new subject', + } + put "/api/v1/ticket_articles/#{result['id']}", params.to_json, @headers.merge('Authorization' => credentials) + assert_response(200) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('new subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(true, result['internal']) + assert_equal(@agent.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Agent').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'email').id, result['type_id']) + + delete "/api/v1/ticket_articles/#{result['id']}", {}.to_json, @headers.merge('Authorization' => credentials) + assert_response(401) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('Not authorized (admin permission required)!', result['error']) delete "/api/v1/tickets/#{ticket.id}", {}.to_json, @headers.merge('Authorization' => credentials) assert_response(401) @@ -253,6 +324,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(ticket.id, result['id']) assert_equal('ticket with corret ticket id', result['title']) assert_equal(ticket.customer_id, result['customer_id']) + assert_equal(1, result['updated_by_id']) + assert_equal(1, result['created_by_id']) params = { title: 'ticket with corret ticket id - 2', @@ -265,6 +338,68 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(ticket.id, result['id']) assert_equal('ticket with corret ticket id - 2', result['title']) assert_equal(@agent.id, result['customer_id']) + assert_equal(@admin.id, result['updated_by_id']) + assert_equal(1, result['created_by_id']) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + } + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(false, result['internal']) + assert_equal(@admin.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Agent').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'note').id, result['type_id']) + + params = { + subject: 'new subject', + internal: true, + } + put "/api/v1/ticket_articles/#{result['id']}", params.to_json, @headers.merge('Authorization' => credentials) + assert_response(200) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('new subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(true, result['internal']) + assert_equal(@admin.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Agent').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'note').id, result['type_id']) + + delete "/api/v1/ticket_articles/#{result['id']}", {}.to_json, @headers.merge('Authorization' => credentials) + assert_response(200) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + type: 'email', + } + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(false, result['internal']) + assert_equal(@admin.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Agent').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'email').id, result['type_id']) + + delete "/api/v1/ticket_articles/#{result['id']}", {}.to_json, @headers.merge('Authorization' => credentials) + assert_response(200) delete "/api/v1/tickets/#{ticket.id}", {}.to_json, @headers.merge('Authorization' => credentials) assert_response(200) @@ -288,6 +423,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(Ticket::State.lookup(name: 'new').id, result['state_id']) assert_equal('a new ticket #c1', result['title']) assert_equal(@customer_without_org.id, result['customer_id']) + assert_equal(@customer_without_org.id, result['updated_by_id']) + assert_equal(@customer_without_org.id, result['created_by_id']) end test '03.02 ticket create with customer with wrong customer' do @@ -311,6 +448,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(Ticket::State.lookup(name: 'new').id, result['state_id']) assert_equal('a new ticket #c2', result['title']) assert_equal(@customer_without_org.id, result['customer_id']) + assert_equal(@customer_without_org.id, result['updated_by_id']) + assert_equal(@customer_without_org.id, result['created_by_id']) end test '03.03 ticket with wrong ticket id' do @@ -364,6 +503,8 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(ticket.id, result['id']) assert_equal('ticket with corret ticket id', result['title']) assert_equal(ticket.customer_id, result['customer_id']) + assert_equal(1, result['updated_by_id']) + assert_equal(1, result['created_by_id']) params = { title: 'ticket with corret ticket id - 2', @@ -376,6 +517,87 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(ticket.id, result['id']) assert_equal('ticket with corret ticket id - 2', result['title']) assert_equal(ticket.customer_id, result['customer_id']) + assert_equal(@customer_without_org.id, result['updated_by_id']) + assert_equal(1, result['created_by_id']) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + } + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(@customer_without_org.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Customer').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'note').id, result['type_id']) + + delete "/api/v1/ticket_articles/#{result['id']}", {}.to_json, @headers.merge('Authorization' => credentials) + assert_response(401) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('Not authorized (admin permission required)!', result['error']) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + type: 'email', + sender: 'Agent', + } + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(@customer_without_org.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Customer').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'note').id, result['type_id']) + + delete "/api/v1/ticket_articles/#{result['id']}", {}.to_json, @headers.merge('Authorization' => credentials) + assert_response(401) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('Not authorized (admin permission required)!', result['error']) + + params = { + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + type: 'web', + sender: 'Agent', + internal: true, + } + + post '/api/v1/ticket_articles', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(ticket.id, result['ticket_id']) + assert_equal('some subject', result['subject']) + assert_equal('some body', result['body']) + assert_equal('text/plain', result['content_type']) + assert_equal(false, result['internal']) + assert_equal(@customer_without_org.id, result['created_by_id']) + assert_equal(Ticket::Article::Sender.lookup(name: 'Customer').id, result['sender_id']) + assert_equal(Ticket::Article::Type.lookup(name: 'web').id, result['type_id']) + + params = { + subject: 'new subject', + } + put "/api/v1/ticket_articles/#{result['id']}", params.to_json, @headers.merge('Authorization' => credentials) + assert_response(401) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('Not authorized (ticket.agent or admin permission required)!', result['error']) delete "/api/v1/tickets/#{ticket.id}", {}.to_json, @headers.merge('Authorization' => credentials) assert_response(401)