Refactoring: Use ticket scope policies more consistently, various small optimizations.

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
 

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 ])
+                                     .where.not(id: ticket.id)
-                                    .order(created_at: :desc)
+                                     .order(created_at: :desc)
-                                    .limit(6)
+                                     .limit(6)
 
     # if we do not have open related tickets, search for any tickets
     tickets ||= TicketPolicy::ReadScope.new(current_user).resolve
-                                       .where(
+                                       .where(customer_id: ticket.customer_id)
-                                         customer_id: ticket.customer_id,
+                                       .where.not(state_id: Ticket::State.by_category(:merged).pluck(:id))
-                                       ).where.not(
+                                       .where.not(id: ticket.id)
-                                         state_id: Ticket::State.by_category(:merged).pluck(:id),
+                                       .order(created_at: :desc)
-                                       )
+                                       .limit(6)
-                                       .where.not(id: [ ticket.id ])
-                                      .order(created_at: :desc)
-                                      .limit(6)
 
     # get related assets
     ticket_ids_by_customer = []

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<Class>]
     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

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<Integer>]
     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

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

app/models/ticket/overviews.rb

@@ -93,8 +93,7 @@ returns
     return [] if overviews.blank?
 
     ticket_attributes = Ticket.new.attributes
-    list = []
+    overviews.map do |overview|
-    overviews.each 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

app/models/ticket/screen_options.rb

@@ -173,42 +173,19 @@ returns
 
   def self.list_by_customer(data)
 
-    # get closed/open states
+    base_query = TicketPolicy::ReadScope.new(data[:current_user]).resolve
-    state_id_list_open   = Ticket::State.by_category(:open).pluck(:id)
+                                        .joins(state: :state_type)
-    state_id_list_closed = Ticket::State.by_category(:closed).pluck(:id)
+                                        .where(customer_id: data[:customer_id])
+                                        .limit(data[:limit] || 15)
+                                        .order(created_at: :desc)
 
-    # get tickets
+    open_tickets   = base_query.where(ticket_state_types: { name: Ticket::State::TYPES[:open] })
-    tickets_open = TicketPolicy::ReadScope.new(data[:current_user]).resolve
+    closed_tickets = base_query.where(ticket_state_types: { name: Ticket::State::TYPES[:closed] })
-                                          .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
 
     {
-      ticket_ids_open:   ticket_ids_open,
+      ticket_ids_open:   open_tickets.map(&:id),
-      ticket_ids_closed: ticket_ids_closed,
+      ticket_ids_closed: closed_tickets.map(&:id),
-      assets:            assets,
+      assets:            (open_tickets | closed_tickets).reduce({}) { |hash, ticket| ticket.assets(hash) },
     }
   end
 end

app/models/ticket/search.rb

@@ -190,45 +190,34 @@ returns
         return tickets
       end
 
-      # do query
+      order_sql   = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC')
-      # - stip out * we already search for *query* -
+      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')
+      ticket_ids = if query
-      order_sql        = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC')
+                     tickets_all.joins(:articles)
-      if query
+                                .where(<<~SQL.squish, query: "%#{query.delete('*')}%")
-        query.delete! '*'
+                                  tickets.title              LIKE :query
-        tickets_all = TicketPolicy::ReadScope.new(current_user).resolve
+                                  OR tickets.number          LIKE :query
-                                             .select("DISTINCT(tickets.id), #{order_select_sql}")
+                                  OR ticket_articles.body    LIKE :query
-                                             .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}%")
+                                  OR ticket_articles.from    LIKE :query
-                                             .joins(:articles)
+                                  OR ticket_articles.to      LIKE :query
-                                             .order(Arel.sql(order_sql))
+                                  OR ticket_articles.subject LIKE :query
-                                             .offset(offset)
+                                SQL
-                                             .limit(limit)
+                   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)
+        ticket_ids
-        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)
       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
 

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
+    Ticket::State.joins(:state_type).where(ticket_state_types: { name: state_types })
-    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)
-    )
   end
 
 =begin

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

app/policies/ticket_policy/full_scope.rb

@@ -2,5 +2,6 @@
 
 class TicketPolicy < ApplicationPolicy
   class FullScope < BaseScope
+    ACCESS_TYPE = :full
   end
 end

app/policies/ticket_policy/overview_scope.rb

@@ -2,5 +2,6 @@
 
 class TicketPolicy < ApplicationPolicy
   class OverviewScope < BaseScope
+    ACCESS_TYPE = :overview
   end
 end

app/policies/ticket_policy/read_scope.rb

@@ -2,5 +2,6 @@
 
 class TicketPolicy < ApplicationPolicy
   class ReadScope < BaseScope
+    ACCESS_TYPE = :read
   end
 end

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

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') } }