Improved validation messages of controllers.

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{(<img\s.+?src=")(data:image/(jpeg|png);base64,.+?)">}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(

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
 
+    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

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{(<img\s.+?src=")(data:image/(jpeg|png);base64,.+?)">}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

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

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)