From 57e7b9e98768d9ab3262fb2f8a61e820b13d342e Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Mon, 27 Mar 2017 16:06:18 +0200 Subject: [PATCH] Refactoring: - Changed behavior to use symbol as parameter to reduce memory coast. - Removed code duplication. - Added tests. --- app/controllers/tickets_controller.rb | 12 ++-- app/models/channel/filter/monitoring_base.rb | 2 +- .../user_ticket_counter/background_job.rb | 4 +- app/models/ticket/escalation.rb | 2 +- app/models/ticket/screen_options.rb | 4 +- app/models/ticket/state.rb | 63 ++++++++----------- db/seeds.rb | 14 ++--- lib/stats/ticket_escalation.rb | 2 +- lib/stats/ticket_in_process.rb | 14 ++--- lib/stats/ticket_load_measure.rb | 2 +- spec/models/ticket/state_spec.rb | 18 ++++++ 11 files changed, 73 insertions(+), 64 deletions(-) create mode 100644 spec/models/ticket/state_spec.rb diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 740136ea2..a4d99af88 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -274,7 +274,7 @@ class TicketsController < ApplicationController ticket_lists = Ticket .where( customer_id: ticket.customer_id, - state_id: Ticket::State.by_category('open') + state_id: Ticket::State.by_category(:open) ) .where(access_condition) .where('id != ?', [ ticket.id ]) @@ -287,7 +287,7 @@ class TicketsController < ApplicationController .where( customer_id: ticket.customer_id, ).where.not( - state_id: Ticket::State.by_category('merged') + state_id: Ticket::State.by_category(:merged) ) .where(access_condition) .where('id != ?', [ ticket.id ]) @@ -503,7 +503,7 @@ class TicketsController < ApplicationController condition = { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('open').map(&:id), + value: Ticket::State.by_category(:open).pluck(:id), }, 'ticket.customer_id' => { operator: 'is', @@ -521,7 +521,7 @@ class TicketsController < ApplicationController condition = { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('closed').map(&:id), + value: Ticket::State.by_category(:closed).pluck(:id), }, 'ticket.customer_id' => { operator: 'is', @@ -577,7 +577,7 @@ class TicketsController < ApplicationController condition = { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('open').map(&:id), + value: Ticket::State.by_category(:open).pluck(:id), }, 'ticket.organization_id' => { operator: 'is', @@ -595,7 +595,7 @@ class TicketsController < ApplicationController condition = { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('closed').map(&:id), + value: Ticket::State.by_category(:closed).pluck(:id), }, 'ticket.organization_id' => { operator: 'is', diff --git a/app/models/channel/filter/monitoring_base.rb b/app/models/channel/filter/monitoring_base.rb index 80e155fc8..08eecf563 100644 --- a/app/models/channel/filter/monitoring_base.rb +++ b/app/models/channel/filter/monitoring_base.rb @@ -46,7 +46,7 @@ class Channel::Filter::MonitoringBase customer = User.lookup(id: session_user_id) # follow up detection by meta data - open_states = Ticket::State.by_category('open') + open_states = Ticket::State.by_category(:open) Ticket.where(state: open_states).each { |ticket| next if !ticket.preferences next if !ticket.preferences['integration'] diff --git a/app/models/observer/ticket/user_ticket_counter/background_job.rb b/app/models/observer/ticket/user_ticket_counter/background_job.rb index d9e8eec09..0dc6521e0 100644 --- a/app/models/observer/ticket/user_ticket_counter/background_job.rb +++ b/app/models/observer/ticket/user_ticket_counter/background_job.rb @@ -7,14 +7,14 @@ class Observer::Ticket::UserTicketCounter::BackgroundJob def perform # open ticket count - state_open = Ticket::State.by_category('open') + state_open = Ticket::State.by_category(:open) tickets_open = Ticket.where( customer_id: @customer_id, state_id: state_open, ).count() # closed ticket count - state_closed = Ticket::State.by_category('closed') + state_closed = Ticket::State.by_category(:closed) tickets_closed = Ticket.where( customer_id: @customer_id, state_id: state_closed, diff --git a/app/models/ticket/escalation.rb b/app/models/ticket/escalation.rb index 8e21c786b..68b645469 100644 --- a/app/models/ticket/escalation.rb +++ b/app/models/ticket/escalation.rb @@ -14,7 +14,7 @@ returns =end def self.rebuild_all - state_list_open = Ticket::State.by_category('open') + state_list_open = Ticket::State.by_category(:open) ticket_ids = Ticket.where(state_id: state_list_open).pluck(:id) ticket_ids.each { |ticket_id| diff --git a/app/models/ticket/screen_options.rb b/app/models/ticket/screen_options.rb index 9d45784b7..1b9104d03 100644 --- a/app/models/ticket/screen_options.rb +++ b/app/models/ticket/screen_options.rb @@ -126,8 +126,8 @@ returns def self.list_by_customer(data) # get closed/open states - state_list_open = Ticket::State.by_category( 'open' ) - state_list_closed = Ticket::State.by_category( 'closed' ) + state_list_open = Ticket::State.by_category(:open) + state_list_closed = Ticket::State.by_category(:closed) # get tickets tickets_open = Ticket.where( diff --git a/app/models/ticket/state.rb b/app/models/ticket/state.rb index 2e3fc5c6d..e4f4efd76 100644 --- a/app/models/ticket/state.rb +++ b/app/models/ticket/state.rb @@ -14,51 +14,42 @@ class Ticket::State < ApplicationModel =begin -list tickets by customer +looks up states for a given category - states = Ticket::State.by_category('open') # open|closed|work_on|work_on_all|viewable|pending_reminder|pending_action|merged + states = Ticket::State.by_category(:open) # :open|:closed|:work_on|:work_on_all|:viewable|:pending_reminder|:pending_action|:merged returns: - state objects + state object list =end def self.by_category(category) - if category == 'open' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: ['new', 'open', 'pending reminder', 'pending action']) - ) - elsif category == 'pending_reminder' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: ['pending reminder']) - ) - elsif category == 'pending_action' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: ['pending action']) - ) - elsif category == 'work_on' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: %w(new open)) - ) - elsif category == 'work_on_all' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: ['new', 'open', 'pending reminder']) - ) - elsif category == 'viewable' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: ['new', 'open', 'pending reminder', 'pending action', 'closed', 'removed']) - ) - elsif category == 'closed' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: %w(closed)) - ) - elsif category == 'merged' - return Ticket::State.where( - state_type_id: Ticket::StateType.where(name: %w(merged)) - ) + + case category.to_sym + when :open + state_types = ['new', 'open', 'pending reminder', 'pending action'] + when :pending_reminder + state_types = ['pending reminder'] + when :pending_action + state_types = ['pending action'] + when :work_on + state_types = %w(new open) + when :work_on_all + state_types = ['new', 'open', 'pending reminder'] + when :viewable + state_types = ['new', 'open', 'pending reminder', 'pending action', 'closed', 'removed'] + when :closed + state_types = %w(closed) + when :merged + state_types = %w(merged) end - raise "Unknown category '#{category}'" + + raise "Unknown category '#{category}'" if state_types.blank? + + Ticket::State.where( + state_type_id: Ticket::StateType.where(name: state_types) + ) end =begin diff --git a/db/seeds.rb b/db/seeds.rb index b2403019f..f58beccce 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -3311,7 +3311,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('open').pluck(:id), + value: Ticket::State.by_category(:open).pluck(:id), }, 'ticket.owner_id' => { operator: 'is', @@ -3338,7 +3338,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('work_on_all').pluck(:id), + value: Ticket::State.by_category(:work_on_all).pluck(:id), }, 'ticket.owner_id' => { operator: 'is', @@ -3365,7 +3365,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('pending_reminder').pluck(:id), + value: Ticket::State.by_category(:pending_reminder).pluck(:id), }, 'ticket.owner_id' => { operator: 'is', @@ -3397,7 +3397,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('work_on_all').pluck(:id), + value: Ticket::State.by_category(:work_on_all).pluck(:id), }, }, order: { @@ -3420,7 +3420,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('pending_reminder').pluck(:id), + value: Ticket::State.by_category(:pending_reminder).pluck(:id), }, 'ticket.pending_time' => { operator: 'within next (relative)', @@ -3473,7 +3473,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('viewable').pluck(:id), + value: Ticket::State.by_category(:viewable).pluck(:id), }, 'ticket.customer_id' => { operator: 'is', @@ -3500,7 +3500,7 @@ Overview.create_if_not_exists( condition: { 'ticket.state_id' => { operator: 'is', - value: Ticket::State.by_category('viewable').pluck(:id), + value: Ticket::State.by_category(:viewable).pluck(:id), }, 'ticket.organization_id' => { operator: 'is', diff --git a/lib/stats/ticket_escalation.rb b/lib/stats/ticket_escalation.rb index 95a58275a..6c0662f33 100644 --- a/lib/stats/ticket_escalation.rb +++ b/lib/stats/ticket_escalation.rb @@ -4,7 +4,7 @@ class Stats::TicketEscalation def self.generate(user) - open_state_ids = Ticket::State.by_category('open').map(&:id) + open_state_ids = Ticket::State.by_category(:open).pluck(:id) # get users groups group_ids = user.groups.map(&:id) diff --git a/lib/stats/ticket_in_process.rb b/lib/stats/ticket_in_process.rb index c8efc14f2..a617b36e1 100644 --- a/lib/stats/ticket_in_process.rb +++ b/lib/stats/ticket_in_process.rb @@ -5,26 +5,26 @@ class Stats::TicketInProcess def self.generate(user) # get own tickets which are "workable" - open_state_ids = Ticket::State.by_category('work_on').map(&:id) - pending_state_ids = Ticket::State.by_category('pending_reminder').map(&:id) + open_state_ids = Ticket::State.by_category(:work_on).pluck(:id) + pending_state_ids = Ticket::State.by_category(:pending_reminder).pluck(:id) own_ticket_ids = Ticket.select('id').where( 'owner_id = ? AND (state_id IN (?) OR (state_id IN (?) AND pending_time < ?))', user.id, open_state_ids, pending_state_ids, Time.zone.now - ).limit(1000).map(&:id) + ).limit(1000).pluck(:id) # get all tickets where I worked on today (owner & closed today) - closed_state_ids = Ticket::State.by_category('closed').map(&:id) + closed_state_ids = Ticket::State.by_category(:closed).pluck(:id) closed_ticket_ids = Ticket.select('id').where( 'owner_id = ? AND state_id IN (?) AND close_at > ?', user.id, closed_state_ids, Time.zone.now - 1.day - ).limit(100).map(&:id) + ).limit(100).pluck(:id) # get all tickets which I changed to pending action - pending_action_state_ids = Ticket::State.by_category('pending_action').map(&:id) + pending_action_state_ids = Ticket::State.by_category(:pending_action).pluck(:id) pending_action_ticket_ids = Ticket.select('id').where( 'owner_id = ? AND state_id IN (?) AND updated_at > ?', user.id, pending_action_state_ids, Time.zone.now - 1.day - ).limit(100).map(&:id) + ).limit(100).pluck(:id) all_ticket_ids = own_ticket_ids.concat(closed_ticket_ids).concat(pending_action_ticket_ids).uniq diff --git a/lib/stats/ticket_load_measure.rb b/lib/stats/ticket_load_measure.rb index 43647c069..67c33836d 100644 --- a/lib/stats/ticket_load_measure.rb +++ b/lib/stats/ticket_load_measure.rb @@ -4,7 +4,7 @@ class Stats::TicketLoadMeasure def self.generate(user) - open_state_ids = Ticket::State.by_category('open').map(&:id) + open_state_ids = Ticket::State.by_category(:open).pluck(:id) # owned tickets count = Ticket.where(owner_id: user.id, state_id: open_state_ids).count diff --git a/spec/models/ticket/state_spec.rb b/spec/models/ticket/state_spec.rb new file mode 100644 index 000000000..6e802e8ff --- /dev/null +++ b/spec/models/ticket/state_spec.rb @@ -0,0 +1,18 @@ +require 'rails_helper' + +RSpec.describe Ticket::State do + + context '.by_category' do + + it 'looks up states by category' do + result = described_class.by_category(:open) + expect(result).to be_an(ActiveRecord::Relation) + expect(result).to_not be_empty + expect(result.first).to be_a(Ticket::State) + end + + it 'raises RuntimeError for invalid category' do + expect { described_class.by_category(:invalidcategoryname) }.to raise_error(RuntimeError) + end + end +end