From d2efdb547d0e5a526f20fe1609680f152da3fb6f Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Wed, 21 Jul 2021 14:04:17 +0000 Subject: [PATCH] Refactoring: Use ticket scope policies more consistently, various small optimizations. --- app/controllers/concerns/ticket_stats.rb | 4 +- app/controllers/tickets_controller.rb | 19 +++--- app/models/concerns/has_groups.rb | 12 +--- app/models/concerns/has_roles.rb | 10 +-- app/models/ticket.rb | 9 +-- app/models/ticket/overviews.rb | 8 +-- app/models/ticket/screen_options.rb | 43 +++---------- app/models/ticket/search.rb | 61 ++++++++----------- app/models/ticket/state.rb | 55 ++++++----------- app/policies/ticket_policy/base_scope.rb | 3 +- app/policies/ticket_policy/full_scope.rb | 1 + app/policies/ticket_policy/overview_scope.rb | 1 + app/policies/ticket_policy/read_scope.rb | 1 + spec/models/ticket/state_spec.rb | 4 +- .../policies/ticket_policy/shared_examples.rb | 4 +- 15 files changed, 81 insertions(+), 154 deletions(-) diff --git a/app/controllers/concerns/ticket_stats.rb b/app/controllers/concerns/ticket_stats.rb index bcb85f42d..bcf5b1a15 100644 --- a/app/controllers/concerns/ticket_stats.rb +++ b/app/controllers/concerns/ticket_stats.rb @@ -27,13 +27,13 @@ module TicketStats # created created = TicketPolicy::ReadScope.new(current_user).resolve - .where('created_at > ? AND created_at < ?', date_start, date_end) + .where(created_at: (date_start..date_end)) .where(condition) .count # closed closed = TicketPolicy::ReadScope.new(current_user).resolve - .where('close_at > ? AND close_at < ?', date_start, date_end) + .where(close_at: (date_start..date_end)) .where(condition) .count diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 877495bc6..fd60c92fe 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -324,20 +324,17 @@ class TicketsController < ApplicationController customer_id: ticket.customer_id, state_id: Ticket::State.by_category(:open).select(:id), ) - .where.not(id: [ ticket.id ]) - .order(created_at: :desc) - .limit(6) + .where.not(id: ticket.id) + .order(created_at: :desc) + .limit(6) # if we do not have open related tickets, search for any tickets tickets ||= TicketPolicy::ReadScope.new(current_user).resolve - .where( - customer_id: ticket.customer_id, - ).where.not( - state_id: Ticket::State.by_category(:merged).pluck(:id), - ) - .where.not(id: [ ticket.id ]) - .order(created_at: :desc) - .limit(6) + .where(customer_id: ticket.customer_id) + .where.not(state_id: Ticket::State.by_category(:merged).pluck(:id)) + .where.not(id: ticket.id) + .order(created_at: :desc) + .limit(6) # get related assets ticket_ids_by_customer = [] diff --git a/app/models/concerns/has_groups.rb b/app/models/concerns/has_groups.rb index a6c30af7c..0a4f5c94b 100644 --- a/app/models/concerns/has_groups.rb +++ b/app/models/concerns/has_groups.rb @@ -70,7 +70,7 @@ module HasGroups return false if !groups_access_permission? group_id = self.class.ensure_group_id_parameter(group_id) - access = self.class.ensure_group_access_list_parameter(access) + access = Array(access).map(&:to_sym) | [:full] # check direct access return true if group_through.klass.eager_load(:group).exists?( @@ -104,7 +104,7 @@ module HasGroups return [] if !active? return [] if !groups_access_permission? - access = self.class.ensure_group_access_list_parameter(access) + access = Array(access).map(&:to_sym) | [:full] foreign_key = group_through.foreign_key klass = group_through.klass @@ -316,7 +316,7 @@ module HasGroups # @return [Array] def group_access(group_id, access) group_id = ensure_group_id_parameter(group_id) - access = ensure_group_access_list_parameter(access) + access = Array(access).map(&:to_sym) | [:full] # check direct access instances = joins(group_through.name) @@ -364,11 +364,5 @@ module HasGroups group_or_id.id end - - def ensure_group_access_list_parameter(access) - access = [access] if access.is_a?(String) - access.push('full') if access.exclude?('full') - access - end end end diff --git a/app/models/concerns/has_roles.rb b/app/models/concerns/has_roles.rb index baacda5c6..3589cdfa5 100644 --- a/app/models/concerns/has_roles.rb +++ b/app/models/concerns/has_roles.rb @@ -30,7 +30,7 @@ module HasRoles return false if !groups_access_permission? group_id = self.class.ensure_group_id_parameter(group_id) - access = self.class.ensure_group_access_list_parameter(access) + access = Array(access).map(&:to_sym) | [:full] RoleGroup.eager_load(:group, :role).exists?( role_id: roles.pluck(:id), @@ -74,7 +74,7 @@ module HasRoles # @return [Array] def role_access(group_id, access) group_id = ensure_group_id_parameter(group_id) - access = ensure_group_access_list_parameter(access) + access = Array(access).map(&:to_sym) | [:full] role_ids = RoleGroup.eager_load(:role).where(group_id: group_id, access: access, roles: { active: true }).pluck(:role_id) join_table = reflect_on_association(:roles).join_table @@ -105,11 +105,5 @@ module HasRoles group_or_id.id end - - def ensure_group_access_list_parameter(access) - access = [access] if access.is_a?(String) - access.push('full') if access.exclude?('full') - access - end end end diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 4018658e0..d8f4bf6a8 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -463,20 +463,13 @@ get count of tickets and tickets which match on selector return [ticket_count, tickets] end - ticket_count = "TicketPolicy::#{access.camelize}Scope".constantize - .new(current_user).resolve - .distinct - .where(query, *bind_params) - .joins(tables) - .count tickets = "TicketPolicy::#{access.camelize}Scope".constantize .new(current_user).resolve .distinct .where(query, *bind_params) .joins(tables) - .limit(limit) - return [ticket_count, tickets] + return [tickets.count, tickets.limit(limit)] rescue ActiveRecord::StatementInvalid => e Rails.logger.error e raise ActiveRecord::Rollback diff --git a/app/models/ticket/overviews.rb b/app/models/ticket/overviews.rb index 0d8dd7d18..204228d75 100644 --- a/app/models/ticket/overviews.rb +++ b/app/models/ticket/overviews.rb @@ -93,8 +93,7 @@ returns return [] if overviews.blank? ticket_attributes = Ticket.new.attributes - list = [] - overviews.each do |overview| + overviews.map do |overview| query_condition, bind_condition, tables = Ticket.selector2sql(overview.condition, current_user: user) direction = overview.order[:direction] order_by = overview.order[:by] @@ -150,7 +149,7 @@ returns .joins(tables) .count() - item = { + { overview: { name: overview.name, id: overview.id, @@ -160,10 +159,7 @@ returns tickets: tickets, count: count, } - - list.push item end - list end end diff --git a/app/models/ticket/screen_options.rb b/app/models/ticket/screen_options.rb index ec6c2ec3e..7019b407e 100644 --- a/app/models/ticket/screen_options.rb +++ b/app/models/ticket/screen_options.rb @@ -173,42 +173,19 @@ returns def self.list_by_customer(data) - # get closed/open states - state_id_list_open = Ticket::State.by_category(:open).pluck(:id) - state_id_list_closed = Ticket::State.by_category(:closed).pluck(:id) + base_query = TicketPolicy::ReadScope.new(data[:current_user]).resolve + .joins(state: :state_type) + .where(customer_id: data[:customer_id]) + .limit(data[:limit] || 15) + .order(created_at: :desc) - # get tickets - tickets_open = TicketPolicy::ReadScope.new(data[:current_user]).resolve - .where( - customer_id: data[:customer_id], - state_id: state_id_list_open - ) - .limit(data[:limit] || 15) - .order(created_at: :desc) - assets = {} - ticket_ids_open = [] - tickets_open.each do |ticket| - ticket_ids_open.push ticket.id - assets = ticket.assets(assets) - end - - tickets_closed = TicketPolicy::ReadScope.new(data[:current_user]).resolve - .where( - customer_id: data[:customer_id], - state_id: state_id_list_closed - ) - .limit(data[:limit] || 15) - .order(created_at: :desc) - ticket_ids_closed = [] - tickets_closed.each do |ticket| - ticket_ids_closed.push ticket.id - assets = ticket.assets(assets) - end + open_tickets = base_query.where(ticket_state_types: { name: Ticket::State::TYPES[:open] }) + closed_tickets = base_query.where(ticket_state_types: { name: Ticket::State::TYPES[:closed] }) { - ticket_ids_open: ticket_ids_open, - ticket_ids_closed: ticket_ids_closed, - assets: assets, + ticket_ids_open: open_tickets.map(&:id), + ticket_ids_closed: closed_tickets.map(&:id), + assets: (open_tickets | closed_tickets).reduce({}) { |hash, ticket| ticket.assets(hash) }, } end end diff --git a/app/models/ticket/search.rb b/app/models/ticket/search.rb index ec88ebabf..6b97f951a 100644 --- a/app/models/ticket/search.rb +++ b/app/models/ticket/search.rb @@ -190,45 +190,34 @@ returns return tickets end - # do query - # - stip out * we already search for *query* - + order_sql = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC') + tickets_all = TicketPolicy::ReadScope.new(current_user).resolve + .order(Arel.sql(order_sql)) + .offset(offset) + .limit(limit) - order_select_sql = sql_helper.get_order_select(sort_by, order_by, 'tickets.updated_at') - order_sql = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC') - if query - query.delete! '*' - tickets_all = TicketPolicy::ReadScope.new(current_user).resolve - .select("DISTINCT(tickets.id), #{order_select_sql}") - .where('(tickets.title LIKE ? OR tickets.number LIKE ? OR ticket_articles.body LIKE ? OR ticket_articles.from LIKE ? OR ticket_articles.to LIKE ? OR ticket_articles.subject LIKE ?)', "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%") - .joins(:articles) - .order(Arel.sql(order_sql)) - .offset(offset) - .limit(limit) + ticket_ids = if query + tickets_all.joins(:articles) + .where(<<~SQL.squish, query: "%#{query.delete('*')}%") + tickets.title LIKE :query + OR tickets.number LIKE :query + OR ticket_articles.body LIKE :query + OR ticket_articles.from LIKE :query + OR ticket_articles.to LIKE :query + OR ticket_articles.subject LIKE :query + SQL + else + query_condition, bind_condition, tables = selector2sql(condition) + + tickets_all.joins(tables) + .where(query_condition, *bind_condition) + end.pluck(:id) + + if full + ticket_ids.map { |id| Ticket.lookup(id: id) } else - query_condition, bind_condition, tables = selector2sql(condition) - tickets_all = TicketPolicy::ReadScope.new(current_user).resolve - .select("DISTINCT(tickets.id), #{order_select_sql}") - .joins(tables) - .where(query_condition, *bind_condition) - .order(Arel.sql(order_sql)) - .offset(offset) - .limit(limit) + ticket_ids end - - # build result list - if !full - ids = [] - tickets_all.each do |ticket| - ids.push ticket.id - end - return ids - end - - tickets = [] - tickets_all.each do |ticket| - tickets.push Ticket.lookup(id: ticket.id) - end - tickets end end diff --git a/app/models/ticket/state.rb b/app/models/ticket/state.rb index eae388b29..6d2412047 100644 --- a/app/models/ticket/state.rb +++ b/app/models/ticket/state.rb @@ -20,6 +20,22 @@ class Ticket::State < ApplicationModel attr_accessor :callback_loop + TYPES = { + open: ['new', 'open', 'pending reminder', 'pending action'], + pending_reminder: ['pending reminder'], + pending_action: ['pending action'], + pending: ['pending reminder', 'pending action'], + work_on: %w[new open], + work_on_all: ['new', 'open', 'pending reminder'], + viewable: ['new', 'open', 'pending reminder', 'pending action', 'closed', 'removed'], + viewable_agent_new: ['new', 'open', 'pending reminder', 'pending action', 'closed'], + viewable_agent_edit: ['open', 'pending reminder', 'pending action', 'closed'], + viewable_customer_new: %w[new closed], + viewable_customer_edit: %w[open closed], + closed: %w[closed], + merged: %w[merged], + }.freeze + =begin looks up states for a given category @@ -32,42 +48,11 @@ returns: =end - def self.by_category(category) + def self.by_category(*categories) + state_types = TYPES.slice(*categories.map(&:to_sym)).values.uniq + raise ArgumentError, "No such categories (#{categories.join(', ')})" if state_types.empty? - 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 :pending - state_types = ['pending reminder', '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 :viewable_agent_new - state_types = ['new', 'open', 'pending reminder', 'pending action', 'closed'] - when :viewable_agent_edit - state_types = ['open', 'pending reminder', 'pending action', 'closed'] - when :viewable_customer_new - state_types = %w[new closed] - when :viewable_customer_edit - state_types = %w[open closed] - when :closed - state_types = %w[closed] - when :merged - state_types = %w[merged] - end - - raise "Unknown category '#{category}'" if state_types.blank? - - Ticket::State.where( - state_type_id: Ticket::StateType.where(name: state_types) - ) + Ticket::State.joins(:state_type).where(ticket_state_types: { name: state_types }) end =begin diff --git a/app/policies/ticket_policy/base_scope.rb b/app/policies/ticket_policy/base_scope.rb index fd3fc2b32..9f4e9eaa3 100644 --- a/app/policies/ticket_policy/base_scope.rb +++ b/app/policies/ticket_policy/base_scope.rb @@ -22,9 +22,8 @@ class TicketPolicy < ApplicationPolicy bind = [] if user.permissions?('ticket.agent') - access_type = self.class.name.demodulize.slice(%r{.*(?=Scope)}).underscore sql.push('group_id IN (?)') - bind.push(user.group_ids_access(access_type)) + bind.push(user.group_ids_access(self.class::ACCESS_TYPE)) end if user.organization&.shared diff --git a/app/policies/ticket_policy/full_scope.rb b/app/policies/ticket_policy/full_scope.rb index 7446790a4..4914c14ec 100644 --- a/app/policies/ticket_policy/full_scope.rb +++ b/app/policies/ticket_policy/full_scope.rb @@ -2,5 +2,6 @@ class TicketPolicy < ApplicationPolicy class FullScope < BaseScope + ACCESS_TYPE = :full end end diff --git a/app/policies/ticket_policy/overview_scope.rb b/app/policies/ticket_policy/overview_scope.rb index 312123086..5e3f9f3f7 100644 --- a/app/policies/ticket_policy/overview_scope.rb +++ b/app/policies/ticket_policy/overview_scope.rb @@ -2,5 +2,6 @@ class TicketPolicy < ApplicationPolicy class OverviewScope < BaseScope + ACCESS_TYPE = :overview end end diff --git a/app/policies/ticket_policy/read_scope.rb b/app/policies/ticket_policy/read_scope.rb index ae20cc441..794276320 100644 --- a/app/policies/ticket_policy/read_scope.rb +++ b/app/policies/ticket_policy/read_scope.rb @@ -2,5 +2,6 @@ class TicketPolicy < ApplicationPolicy class ReadScope < BaseScope + ACCESS_TYPE = :read end end diff --git a/spec/models/ticket/state_spec.rb b/spec/models/ticket/state_spec.rb index dfc1df5a6..40bec0571 100644 --- a/spec/models/ticket/state_spec.rb +++ b/spec/models/ticket/state_spec.rb @@ -44,9 +44,9 @@ RSpec.describe Ticket::State, type: :model do end context 'with invalid category name' do - it 'raises RuntimeError' do + it 'raises ArgumentError' do expect { described_class.by_category(:invalidcategoryname) } - .to raise_error(RuntimeError) + .to raise_error(ArgumentError) end end end diff --git a/spec/policies/ticket_policy/shared_examples.rb b/spec/policies/ticket_policy/shared_examples.rb index d7c43131e..03c06d94a 100644 --- a/spec/policies/ticket_policy/shared_examples.rb +++ b/spec/policies/ticket_policy/shared_examples.rb @@ -27,7 +27,7 @@ RSpec.shared_examples 'for agent user' do |access_type| context 'with direct access via User#groups' do let(:user) { create(:agent, groups: member_groups) } - context 'when checkin for "full" access' do + context 'when checking for "full" access' do # this is already true by default, but it doesn't hurt to be explicit before { user.user_groups.each { |ug| ug.update_columns(access: 'full') } } @@ -51,7 +51,7 @@ RSpec.shared_examples 'for agent user' do |access_type| let(:user) { create(:agent).tap { |u| u.roles << role } } let(:role) { create(:role, groups: member_groups) } - context 'when checkin for "full" access' do + context 'when checking for "full" access' do # this is already true by default, but it doesn't hurt to be explicit before { role.role_groups.each { |rg| rg.update_columns(access: 'full') } }