From c8e9058866229db847f5f1c71aad5fdad72ec857 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Sun, 12 Feb 2017 18:21:03 +0100 Subject: [PATCH] Moved default and follow up state/priority from source code into model. Related to issue #689 - OTRS import breaks a lot of assumptions in Zammad. --- .../controllers/customer_ticket_create.coffee | 4 +- .../app/models/ticket_priority.coffee | 2 +- app/controllers/first_steps_controller.rb | 3 - app/controllers/form_controller.rb | 2 - app/models/channel/email_parser.rb | 9 +- app/models/observer/ticket/reset_new_state.rb | 6 +- app/models/ticket.rb | 22 ++++- app/models/ticket/priority.rb | 29 ++++++ app/models/ticket/state.rb | 33 +++++++ db/migrate/20120101000010_create_ticket.rb | 6 ++ ...07081400_ticket_state_priority_defaults.rb | 57 +++++++++++ db/seeds.rb | 54 +++++++++-- lib/facebook.rb | 6 +- lib/tweet_base.rb | 6 +- test/controllers/tickets_controller_test.rb | 21 ++++ test/unit/ticket_priority_test.rb | 61 ++++++++++++ test/unit/ticket_state_test.rb | 97 +++++++++++++++++++ 17 files changed, 384 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20170207081400_ticket_state_priority_defaults.rb create mode 100644 test/unit/ticket_priority_test.rb create mode 100644 test/unit/ticket_state_test.rb diff --git a/app/assets/javascripts/app/controllers/customer_ticket_create.coffee b/app/assets/javascripts/app/controllers/customer_ticket_create.coffee index ef71a4dd3..8170abf41 100644 --- a/app/assets/javascripts/app/controllers/customer_ticket_create.coffee +++ b/app/assets/javascripts/app/controllers/customer_ticket_create.coffee @@ -102,12 +102,12 @@ class Index extends App.ControllerContent # set prio if !params.priority_id - priority = App.TicketPriority.findByAttribute( 'name', '2 normal' ) + priority = App.TicketPriority.findByAttribute( 'default_create', true ) params.priority_id = priority.id # set state if !params.state_id - state = App.TicketState.findByAttribute( 'name', 'new' ) + state = App.TicketState.findByAttribute( 'default_create', true ) params.state_id = state.id # fillup params diff --git a/app/assets/javascripts/app/models/ticket_priority.coffee b/app/assets/javascripts/app/models/ticket_priority.coffee index 413cd55b9..63ba7d06f 100644 --- a/app/assets/javascripts/app/models/ticket_priority.coffee +++ b/app/assets/javascripts/app/models/ticket_priority.coffee @@ -11,4 +11,4 @@ class App.TicketPriority extends App.Model @configure_translate = true @configure_overview = [ 'name', - ] \ No newline at end of file + ] diff --git a/app/controllers/first_steps_controller.rb b/app/controllers/first_steps_controller.rb index 9b5002609..c0084c501 100644 --- a/app/controllers/first_steps_controller.rb +++ b/app/controllers/first_steps_controller.rb @@ -188,10 +188,7 @@ class FirstStepsController < ApplicationController ticket = Ticket.create( group_id: Group.find_by(active: true, name: 'Users').id, customer_id: customer.id, - owner_id: User.find_by(login: '-').id, title: result[:subject], - state_id: Ticket::State.find_by(name: 'new').id, - priority_id: Ticket::Priority.find_by(name: '2 normal').id, ) article = Ticket::Article.create( ticket_id: ticket.id, diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index b80e26341..5732ff023 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -98,8 +98,6 @@ class FormController < ApplicationController group_id: 1, customer_id: customer.id, title: params[:title], - state_id: Ticket::State.find_by(name: 'new').id, - priority_id: Ticket::Priority.find_by(name: '2 normal').id, ) article = Ticket::Article.create( ticket_id: ticket.id, diff --git a/app/models/channel/email_parser.rb b/app/models/channel/email_parser.rb index 656015707..fe215fc52 100644 --- a/app/models/channel/email_parser.rb +++ b/app/models/channel/email_parser.rb @@ -481,10 +481,11 @@ returns end - # set ticket to open again + # set ticket to open again or keep create state if !mail['x-zammad-ticket-followup-state'.to_sym] && !mail['x-zammad-ticket-followup-state_id'.to_sym] - if state_type.name != 'new' && !mail['x-zammad-out-of-office'.to_sym] - ticket.state = Ticket::State.find_by(name: 'open') + new_state = Ticket::State.find_by(default_create: true) + if ticket.state_id != new_state.id && !mail['x-zammad-out-of-office'.to_sym] + ticket.state = Ticket::State.find_by(default_follow_up: true) ticket.save! end end @@ -514,8 +515,6 @@ returns ticket = Ticket.new( group_id: group.id, title: mail[:subject] || '', - state_id: Ticket::State.find_by(name: 'new').id, - priority_id: Ticket::Priority.find_by(name: '2 normal').id, preferences: preferences, ) set_attributes_by_x_headers(ticket, 'ticket', mail) diff --git a/app/models/observer/ticket/reset_new_state.rb b/app/models/observer/ticket/reset_new_state.rb index 001223328..3c426526c 100644 --- a/app/models/observer/ticket/reset_new_state.rb +++ b/app/models/observer/ticket/reset_new_state.rb @@ -19,10 +19,10 @@ class Observer::Ticket::ResetNewState < ActiveRecord::Observer # if current ticket state is still new ticket = Ticket.lookup(id: record.ticket_id) - return true if ticket.state.state_type.name != 'new' + new_state = Ticket::State.find_by(default_create: true) + return true if ticket.state_id != new_state.id - # TODO: add config option to state managment in UI - state = Ticket::State.lookup(name: 'open') + state = Ticket::State.find_by(default_follow_up: true) return if !state # set ticket to open diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 5fd9233e9..fa1bf2721 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -18,13 +18,11 @@ class Ticket < ApplicationModel extend Ticket::Search store :preferences - before_create :check_generate, :check_defaults, :check_title, :check_escalation_update + before_create :check_generate, :check_defaults, :check_title, :check_escalation_update, :set_default_state, :set_default_priority before_update :check_defaults, :check_title, :reset_pending_time, :check_escalation_update before_destroy :destroy_dependencies validates :group_id, presence: true - validates :priority_id, presence: true - validates :state_id, presence: true activity_stream_permission 'ticket.agent' @@ -925,4 +923,22 @@ result # destroy online notifications OnlineNotification.remove(self.class.to_s, id) end + + def set_default_state + return if state_id + + default_ticket_state = Ticket::State.find_by(default_create: true) + return if !default_ticket_state + + self.state_id = default_ticket_state.id + end + + def set_default_priority + return if priority_id + + default_ticket_priority = Ticket::Priority.find_by(default_create: true) + return if !default_ticket_priority + + self.priority_id = default_ticket_priority.id + end end diff --git a/app/models/ticket/priority.rb b/app/models/ticket/priority.rb index 9c7f0448d..fc6cfd19f 100644 --- a/app/models/ticket/priority.rb +++ b/app/models/ticket/priority.rb @@ -2,4 +2,33 @@ class Ticket::Priority < ApplicationModel self.table_name = 'ticket_priorities' validates :name, presence: true + + after_create :ensure_defaults + after_update :ensure_defaults + after_destroy :ensure_defaults + + attr_accessor :callback_loop + + def ensure_defaults + return if callback_loop + priorities_with_default = Ticket::Priority.where(default_create: true) + return if priorities_with_default.count == 1 + + if priorities_with_default.count.zero? + priority = Ticket::Priority.where(active: true).order(id: :asc).first + priority.default_create = true + priority.callback_loop = true + priority.save! + return + end + + if priorities_with_default.count > 1 + Ticket::Priority.all.each { |local_priority| + next if local_priority.id == id + local_priority.default_create = false + local_priority.callback_loop = true + local_priority.save! + } + end + end end diff --git a/app/models/ticket/state.rb b/app/models/ticket/state.rb index ebdb4e749..e6a02245b 100644 --- a/app/models/ticket/state.rb +++ b/app/models/ticket/state.rb @@ -2,10 +2,16 @@ class Ticket::State < ApplicationModel include LatestChangeObserved + after_create :ensure_defaults + after_update :ensure_defaults + after_destroy :ensure_defaults + belongs_to :state_type, class_name: 'Ticket::StateType' belongs_to :next_state, class_name: 'Ticket::State' validates :name, presence: true + attr_accessor :callback_loop + =begin list tickets by customer @@ -69,4 +75,31 @@ returns: return true if ignore_escalation false end + + def ensure_defaults + return if callback_loop + + %w(default_create default_follow_up).each do |default_field| + states_with_default = Ticket::State.where(default_field => true) + next if states_with_default.count == 1 + + if states_with_default.count.zero? + state = Ticket::State.where(active: true).order(id: :asc).first + state[default_field] = true + state.callback_loop = true + state.save! + next + end + + Ticket::State.all.each { |local_state| + next if local_state.id == id + next if local_state[default_field] == false + local_state[default_field] = false + local_state.callback_loop = true + local_state.save! + next + } + end + end + end diff --git a/db/migrate/20120101000010_create_ticket.rb b/db/migrate/20120101000010_create_ticket.rb index ab923bb41..35f134fc9 100644 --- a/db/migrate/20120101000010_create_ticket.rb +++ b/db/migrate/20120101000010_create_ticket.rb @@ -14,6 +14,8 @@ class CreateTicket < ActiveRecord::Migration t.column :name, :string, limit: 250, null: false t.column :next_state_id, :integer, null: true t.column :ignore_escalation, :boolean, null: false, default: false + t.column :default_create, :boolean, null: false, default: false + t.column :default_follow_up, :boolean, null: false, default: false t.column :note, :string, limit: 250, null: true t.column :active, :boolean, null: false, default: true t.column :updated_by_id, :integer, null: false @@ -21,9 +23,12 @@ class CreateTicket < ActiveRecord::Migration t.timestamps limit: 3, null: false end add_index :ticket_states, [:name], unique: true + add_index :ticket_states, [:default_create] + add_index :ticket_states, [:default_follow_up] create_table :ticket_priorities do |t| t.column :name, :string, limit: 250, null: false + t.column :default_create, :boolean, null: false, default: false t.column :note, :string, limit: 250, null: true t.column :active, :boolean, null: false, default: true t.column :updated_by_id, :integer, null: false @@ -31,6 +36,7 @@ class CreateTicket < ActiveRecord::Migration t.timestamps limit: 3, null: false end add_index :ticket_priorities, [:name], unique: true + add_index :ticket_priorities, [:default_create] create_table :tickets do |t| t.references :group, null: false diff --git a/db/migrate/20170207081400_ticket_state_priority_defaults.rb b/db/migrate/20170207081400_ticket_state_priority_defaults.rb new file mode 100644 index 000000000..471f8f1c8 --- /dev/null +++ b/db/migrate/20170207081400_ticket_state_priority_defaults.rb @@ -0,0 +1,57 @@ +class TicketStatePriorityDefaults < ActiveRecord::Migration + def up + + # return if it's a new setup + return if !Setting.find_by(name: 'system_init_done') + + add_column :ticket_states, :default_create, :boolean, null: false, default: false + add_index :ticket_states, :default_create + add_column :ticket_states, :default_follow_up, :boolean, null: false, default: false + add_index :ticket_states, :default_follow_up + + add_column :ticket_priorities, :default_create, :boolean, null: false, default: false + add_index :ticket_priorities, :default_create + + # Set defaults + ticket_state_new = Ticket::State.find_by(name: 'new') + if !ticket_state_new + ticket_state_new = Ticket::State.first + end + if ticket_state_new + ticket_state_new.default_create = true + ticket_state_new.save! + end + + ticket_state_open = Ticket::State.find_by(name: 'open') + if !ticket_state_open + ticket_state_open = Ticket::State.first + end + if ticket_state_open + ticket_state_open.default_follow_up = true + ticket_state_open.save! + end + + ticket_priority = Ticket::Priority.find_by(name: '2 normal') + if !ticket_priority + ticket_priority = Ticket::Priority.first + end + if ticket_priority + ticket_priority.default_create = true + ticket_priority.save! + end + + Cache.clear + end + + def down + remove_index :ticket_states, :default_create + remove_column :ticket_states, :default_create, :boolean + remove_index :ticket_states, :default_follow_up + remove_column :ticket_states, :default_follow_up, :boolean + + remove_index :ticket_priorities, :default_create + remove_column :ticket_priorities, :default_create, :boolean + + Cache.clear + end +end diff --git a/db/seeds.rb b/db/seeds.rb index 1bdc6e952..435f7d8ea 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -3181,16 +3181,53 @@ Ticket::StateType.create_if_not_exists(id: 5, name: 'closed') Ticket::StateType.create_if_not_exists(id: 6, name: 'merged') Ticket::StateType.create_if_not_exists(id: 7, name: 'removed') -Ticket::State.create_if_not_exists(id: 1, name: 'new', state_type_id: Ticket::StateType.find_by(name: 'new').id) -Ticket::State.create_if_not_exists(id: 2, name: 'open', state_type_id: Ticket::StateType.find_by(name: 'open').id) -Ticket::State.create_if_not_exists(id: 3, name: 'pending reminder', state_type_id: Ticket::StateType.find_by(name: 'pending reminder').id, ignore_escalation: true) -Ticket::State.create_if_not_exists(id: 4, name: 'closed', state_type_id: Ticket::StateType.find_by(name: 'closed').id, ignore_escalation: true) -Ticket::State.create_if_not_exists(id: 5, name: 'merged', state_type_id: Ticket::StateType.find_by(name: 'merged').id, ignore_escalation: true) -Ticket::State.create_if_not_exists(id: 6, name: 'removed', state_type_id: Ticket::StateType.find_by(name: 'removed').id, active: false, ignore_escalation: true) -Ticket::State.create_if_not_exists(id: 7, name: 'pending close', state_type_id: Ticket::StateType.find_by(name: 'pending action').id, next_state_id: 4, ignore_escalation: true) +Ticket::State.create_if_not_exists( + id: 1, + name: 'new', + state_type_id: Ticket::StateType.find_by(name: 'new').id, + default_create: true, +) +Ticket::State.create_if_not_exists( + id: 2, + name: 'open', + state_type_id: Ticket::StateType.find_by(name: 'open').id, + default_follow_up: true, +) +Ticket::State.create_if_not_exists( + id: 3, + name: 'pending reminder', + state_type_id: Ticket::StateType.find_by(name: 'pending reminder').id, + ignore_escalation: true, +) +Ticket::State.create_if_not_exists( + id: 4, + name: 'closed', + state_type_id: Ticket::StateType.find_by(name: 'closed').id, + ignore_escalation: true, +) +Ticket::State.create_if_not_exists( + id: 5, + name: 'merged', + state_type_id: Ticket::StateType.find_by(name: 'merged').id, + ignore_escalation: true, +) +Ticket::State.create_if_not_exists( + id: 6, + name: 'removed', + state_type_id: Ticket::StateType.find_by(name: 'removed').id, + active: false, + ignore_escalation: true, +) +Ticket::State.create_if_not_exists( + id: 7, + name: 'pending close', + state_type_id: Ticket::StateType.find_by(name: 'pending action').id, + next_state_id: Ticket::State.find_by(name: 'closed').id, + ignore_escalation: true, +) Ticket::Priority.create_if_not_exists(id: 1, name: '1 low') -Ticket::Priority.create_if_not_exists(id: 2, name: '2 normal') +Ticket::Priority.create_if_not_exists(id: 2, name: '2 normal', default_create: true) Ticket::Priority.create_if_not_exists(id: 3, name: '3 high') Ticket::Article::Type.create_if_not_exists(id: 1, name: 'email', communication: true) @@ -3232,7 +3269,6 @@ UserInfo.current_user_id = user_community.id ticket = Ticket.create( group_id: Group.find_by(name: 'Users').id, customer_id: User.find_by(login: 'nicole.braun@zammad.org').id, - owner_id: User.find_by(login: '-').id, title: 'Welcome to Zammad!', state_id: Ticket::State.find_by(name: 'new').id, priority_id: Ticket::Priority.find_by(name: '2 normal').id, diff --git a/lib/facebook.rb b/lib/facebook.rb index bded96b63..46f03291d 100644 --- a/lib/facebook.rb +++ b/lib/facebook.rb @@ -351,11 +351,11 @@ result return ticket.state end - state = Ticket::State.find_by(name: 'new') + state = Ticket::State.find_by(default_create: true) return state if !ticket - return ticket.state if ticket.state.name == 'new' - Ticket::State.find_by(name: 'open') + return ticket.state if ticket.state_id == state.id + Ticket::State.find_by(default_follow_up: true) end def access_token_for_page(lookup) diff --git a/lib/tweet_base.rb b/lib/tweet_base.rb index d7677065b..74a257b9c 100644 --- a/lib/tweet_base.rb +++ b/lib/tweet_base.rb @@ -337,10 +337,10 @@ class TweetBase return ticket.state end - state = Ticket::State.find_by(name: 'new') + state = Ticket::State.find_by(default_create: true) return state if !ticket - return ticket.state if ticket.state.name == 'new' - Ticket::State.find_by(name: 'open') + return ticket.state if ticket.state_id == state.id + Ticket::State.find_by(default_follow_up: true) end def tweet_limit_reached(tweet) diff --git a/test/controllers/tickets_controller_test.rb b/test/controllers/tickets_controller_test.rb index 8621fc9d1..9b6cc148e 100644 --- a/test/controllers/tickets_controller_test.rb +++ b/test/controllers/tickets_controller_test.rb @@ -242,6 +242,27 @@ class TicketsControllerTest < ActionDispatch::IntegrationTest assert_equal(@agent.id, result['created_by_id']) end + test '01.09 ticket create with agent - minimal article with guess customer' do + credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-agent@example.com', 'agentpw') + params = { + title: 'a new ticket #9', + group: 'Users', + customer_id: 'guess:some_new_customer@example.com', + article: { + body: 'some test 123', + }, + } + post '/api/v1/tickets', params.to_json, @headers.merge('Authorization' => credentials) + assert_response(201) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal(Ticket::State.lookup(name: 'new').id, result['state_id']) + assert_equal('a new ticket #9', result['title']) + assert_equal(User.lookup(email: 'some_new_customer@example.com').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 credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-agent@example.com', 'agentpw') params = { diff --git a/test/unit/ticket_priority_test.rb b/test/unit/ticket_priority_test.rb new file mode 100644 index 000000000..c6cbe4dee --- /dev/null +++ b/test/unit/ticket_priority_test.rb @@ -0,0 +1,61 @@ +# encoding: utf-8 +require 'test_helper' + +class TicketPriorityTest < ActiveSupport::TestCase + + test 'base' do + + # check current state + assert_equal(1, Ticket::Priority.where(default_create: true).count) + priority_create = Ticket::Priority.find_by(default_create: true) + + # add new state + priority_new2 = Ticket::Priority.create_if_not_exists( + name: 'priority 2', + updated_by_id: 1, + created_by_id: 1, + ) + + # verify states + assert_equal(1, Ticket::Priority.where(default_create: true).count) + assert_equal(priority_create.id, Ticket::Priority.find_by(default_create: true).id) + + # cleanup + priority_new2.destroy + + # verify states + assert_equal(1, Ticket::Priority.where(default_create: true).count) + assert_equal(priority_create.id, Ticket::Priority.find_by(default_create: true).id) + + # add new state + priority_new3 = Ticket::Priority.create_if_not_exists( + name: 'priority 3', + default_create: true, + updated_by_id: 1, + created_by_id: 1, + ) + + # verify states + assert_equal(1, Ticket::Priority.where(default_create: true).count) + assert_equal(priority_new3.id, Ticket::Priority.find_by(default_create: true).id) + assert_not_equal(priority_create.id, Ticket::Priority.find_by(default_create: true).id) + + # cleanup + priority_new3.destroy + + # verify states + assert_equal(1, Ticket::Priority.where(default_create: true).count) + assert_equal(Ticket::Priority.first, Ticket::Priority.find_by(default_create: true)) + + # cleanup + priority_create.reload + priority_create.default_create = true + priority_create.save! + + # verify states + assert_equal(1, Ticket::Priority.where(default_create: true).count) + assert_equal(priority_create.id, Ticket::Priority.find_by(default_create: true).id) + + end + +end diff --git a/test/unit/ticket_state_test.rb b/test/unit/ticket_state_test.rb new file mode 100644 index 000000000..b4f9996e7 --- /dev/null +++ b/test/unit/ticket_state_test.rb @@ -0,0 +1,97 @@ +# encoding: utf-8 +require 'test_helper' + +class TicketStateTest < ActiveSupport::TestCase + + test 'base' do + + # check current state + assert_equal(1, Ticket::State.where(default_create: true).count) + assert_equal(1, Ticket::State.where(default_follow_up: true).count) + state_create = Ticket::State.find_by(default_create: true) + state_follow_up = Ticket::State.find_by(default_follow_up: true) + + # add new state + state_new2 = Ticket::State.create_if_not_exists( + name: 'new 2', + state_type_id: Ticket::StateType.find_by(name: 'new').id, + updated_by_id: 1, + created_by_id: 1, + ) + + state_follow_up2 = Ticket::State.create_if_not_exists( + name: 'open 2', + state_type_id: Ticket::StateType.find_by(name: 'open').id, + updated_by_id: 1, + created_by_id: 1, + ) + + # verify states + assert_equal(1, Ticket::State.where(default_create: true).count) + assert_equal(1, Ticket::State.where(default_follow_up: true).count) + assert_equal(state_create.id, Ticket::State.find_by(default_create: true).id) + assert_equal(state_follow_up.id, Ticket::State.find_by(default_follow_up: true).id) + + # cleanup + state_new2.destroy + state_follow_up2.destroy + + # verify states + assert_equal(1, Ticket::State.where(default_create: true).count) + assert_equal(1, Ticket::State.where(default_follow_up: true).count) + assert_equal(state_create.id, Ticket::State.find_by(default_create: true).id) + assert_equal(state_follow_up.id, Ticket::State.find_by(default_follow_up: true).id) + + # add new state + state_new3 = Ticket::State.create_if_not_exists( + name: 'new 3', + state_type_id: Ticket::StateType.find_by(name: 'new').id, + default_create: true, + updated_by_id: 1, + created_by_id: 1, + ) + + state_follow_up3 = Ticket::State.create_if_not_exists( + name: 'open 3', + state_type_id: Ticket::StateType.find_by(name: 'open').id, + default_follow_up: true, + updated_by_id: 1, + created_by_id: 1, + ) + + # verify states + assert_equal(1, Ticket::State.where(default_create: true).count) + assert_equal(1, Ticket::State.where(default_follow_up: true).count) + assert_not_equal(state_create.id, Ticket::State.find_by(default_create: true).id) + assert_equal(state_new3.id, Ticket::State.find_by(default_create: true).id) + assert_not_equal(state_follow_up.id, Ticket::State.find_by(default_follow_up: true).id) + assert_equal(state_follow_up3.id, Ticket::State.find_by(default_follow_up: true).id) + + # cleanup + state_new3.destroy + state_follow_up3.destroy + + # verify states + assert_equal(1, Ticket::State.where(default_create: true).count) + assert_equal(1, Ticket::State.where(default_follow_up: true).count) + assert_equal(state_create.id, Ticket::State.find_by(default_create: true).id) + assert_not_equal(state_follow_up.id, Ticket::State.find_by(default_follow_up: true).id) + + # cleanup + state_create.reload + state_create.default_create = true + state_create.save! + + state_follow_up.reload + state_follow_up.default_follow_up = true + state_follow_up.save! + + # verify states + assert_equal(1, Ticket::State.where(default_create: true).count) + assert_equal(1, Ticket::State.where(default_follow_up: true).count) + assert_equal(state_create.id, Ticket::State.find_by(default_create: true).id) + assert_equal(state_follow_up.id, Ticket::State.find_by(default_follow_up: true).id) + + end + +end