Sutty/trabajo-afectivo
5
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Refactoring: Drop Ticket.access_condition in favor of using Pundit scopes.

Browse source
This commit is contained in:
Ryan Lue 2021-07-20 17:35:02 +00:00 committed by Martin Gruner
parent 9f7bec8942
commit 276c45b2e9
21 changed files with 428 additions and 160 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
app/controllers/concerns/ticket_stats.rb
View file

@ -16,7 +16,7 @@ module TicketStats
assets_of_tickets(tickets, assets)
end
def ticket_stats_last_year(condition, access_condition)
def ticket_stats_last_year(condition)
volume_by_year = []
now = Time.zone.now
@ -26,16 +26,16 @@ module TicketStats
date_end = "#{date_to_check.year}-#{date_to_check.month}-#{date_to_check.end_of_month.day} 00:00:00"
# created
created = Ticket.where('created_at > ? AND created_at < ?', date_start, date_end)
.where(access_condition)
.where(condition)
.count
created = TicketPolicy::ReadScope.new(current_user).resolve
.where('created_at > ? AND created_at < ?', date_start, date_end)
.where(condition)
.count
# closed
closed = Ticket.where('close_at > ? AND close_at < ?', date_start, date_end)
.where(access_condition)
.where(condition)
.count
closed = TicketPolicy::ReadScope.new(current_user).resolve
.where('close_at > ? AND close_at < ?', date_start, date_end)
.where(condition)
.count
data = {
month: date_to_check.month,

54
app/controllers/tickets_controller.rb
View file

@ -23,8 +23,10 @@ class TicketsController < ApplicationController
per_page = 100
end
access_condition = Ticket.access_condition(current_user, 'read')
tickets = Ticket.where(access_condition).order(id: :asc).offset(offset).limit(per_page)
tickets = TicketPolicy::ReadScope.new(current_user).resolve
.order(id: :asc)
.offset(offset)
.limit(per_page)
if response_expand?
list = []
@ -317,36 +319,29 @@ class TicketsController < ApplicationController
ticket = Ticket.find(params[:ticket_id])
assets = ticket.assets({})
# open tickets by customer
access_condition = Ticket.access_condition(current_user, 'read')
ticket_lists = Ticket
.where(
customer_id: ticket.customer_id,
state_id: Ticket::State.by_category(:open).pluck(:id), # rubocop:disable Rails/PluckInWhere
)
.where(access_condition)
.where.not(id: [ ticket.id ])
.order(created_at: :desc)
.limit(6)
tickets = TicketPolicy::ReadScope.new(current_user).resolve
.where(
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)
# if we do not have open related tickets, search for any tickets
if ticket_lists.blank?
ticket_lists = Ticket
.where(
customer_id: ticket.customer_id,
).where.not(
state_id: Ticket::State.by_category(:merged).pluck(:id),
)
.where(access_condition)
.where.not(id: [ ticket.id ])
.order(created_at: :desc)
.limit(6)
end
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)
# get related assets
ticket_ids_by_customer = []
ticket_lists.each do |ticket_list|
tickets.each do |ticket_list|
ticket_ids_by_customer.push ticket_list.id
assets = ticket_list.assets(assets)
end
@ -534,7 +529,6 @@ class TicketsController < ApplicationController
# lookup open user tickets
limit = 100
assets = {}
access_condition = Ticket.access_condition(current_user, 'read')
user_tickets = {}
if params[:user_id]
@ -573,7 +567,7 @@ class TicketsController < ApplicationController
condition = {
'tickets.customer_id' => user.id,
}
user_tickets[:volume_by_year] = ticket_stats_last_year(condition, access_condition)
user_tickets[:volume_by_year] = ticket_stats_last_year(condition)
end
@ -615,7 +609,7 @@ class TicketsController < ApplicationController
condition = {
'tickets.organization_id' => organization.id,
}
org_tickets[:volume_by_year] = ticket_stats_last_year(condition, access_condition)
org_tickets[:volume_by_year] = ticket_stats_last_year(condition)
end
# return result

52
app/models/ticket.rb
View file

@ -93,43 +93,6 @@ class Ticket < ApplicationModel
=begin
get user access conditions
conditions = Ticket.access_condition( User.find(1) , 'full')
returns
result = [user1, user2, ...]
=end
def self.access_condition(user, access)
sql = []
bind = []
if user.permissions?('ticket.agent')
sql.push('group_id IN (?)')
bind.push(user.group_ids_access(access))
end
if user.permissions?('ticket.customer')
if !user.organization || (!user.organization.shared || user.organization.shared == false)
sql.push('tickets.customer_id = ?')
bind.push(user.id)
else
sql.push('(tickets.customer_id = ? OR tickets.organization_id = ?)')
bind.push(user.id)
bind.push(user.organization.id)
end
end
return if sql.blank?
[ sql.join(' OR ') ].concat(bind)
end
=begin
processes tickets which have reached their pending time and sets next state_id
processed_tickets = Ticket.process_pending
@ -500,9 +463,18 @@ get count of tickets and tickets which match on selector
return [ticket_count, tickets]
end
access_condition = Ticket.access_condition(current_user, access)
ticket_count = Ticket.distinct.where(access_condition).where(query, *bind_params).joins(tables).count
tickets = Ticket.distinct.where(access_condition).where(query, *bind_params).joins(tables).limit(limit)
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]
rescue ActiveRecord::StatementInvalid => e

30
app/models/ticket/overviews.rb
View file

@ -92,10 +92,6 @@ returns
)
return [] if overviews.blank?
# get only tickets with permissions
access_condition = Ticket.access_condition(user, 'overview')
access_condition_read = Ticket.access_condition(user, 'read')
ticket_attributes = Ticket.new.attributes
list = []
overviews.each do |overview|
@ -129,18 +125,17 @@ returns
end
end
overview_access_condition = access_condition
scope = TicketPolicy::OverviewScope
if overview.condition['ticket.mention_user_ids'].present?
overview_access_condition = access_condition_read
scope = TicketPolicy::ReadScope
end
ticket_result = Ticket.distinct
.where(overview_access_condition)
.where(query_condition, *bind_condition)
.joins(tables)
.order(Arel.sql("#{order_by} #{direction}"))
.limit(2000)
.pluck(:id, :updated_at, Arel.sql(order_by))
ticket_result = scope.new(user).resolve
.distinct
.where(query_condition, *bind_condition)
.joins(tables)
.order(Arel.sql("#{order_by} #{direction}"))
.limit(2000)
.pluck(:id, :updated_at, Arel.sql(order_by))
tickets = ticket_result.map do |ticket|
{
@ -149,7 +144,12 @@ returns
}
end
count = Ticket.distinct.where(overview_access_condition).where(query_condition, *bind_condition).joins(tables).count()
count = TicketPolicy::OverviewScope.new(user).resolve
.distinct
.where(query_condition, *bind_condition)
.joins(tables)
.count()
item = {
overview: {
name: overview.name,

29
app/models/ticket/screen_options.rb
View file

@ -177,16 +177,14 @@ returns
state_id_list_open = Ticket::State.by_category(:open).pluck(:id)
state_id_list_closed = Ticket::State.by_category(:closed).pluck(:id)
# open tickets by customer
access_condition = Ticket.access_condition(data[:current_user], 'read')
# get tickets
tickets_open = Ticket.where(
customer_id: data[:customer_id],
state_id: state_id_list_open
)
.where(access_condition)
.limit(data[:limit] || 15).order(created_at: :desc)
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|
@ -194,12 +192,13 @@ returns
assets = ticket.assets(assets)
end
tickets_closed = Ticket.where(
customer_id: data[:customer_id],
state_id: state_id_list_closed
)
.where(access_condition)
.limit(data[:limit] || 15).order(created_at: :desc)
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

31
app/models/ticket/search.rb
View file

@ -190,9 +190,6 @@ returns
return tickets
end
# fallback do sql query
access_condition = Ticket.access_condition(current_user, 'read')
# do query
# - stip out * we already search for *query* -
@ -200,22 +197,22 @@ returns
order_sql = sql_helper.get_order(sort_by, order_by, 'tickets.updated_at DESC')
if query
query.delete! '*'
tickets_all = Ticket.select("DISTINCT(tickets.id), #{order_select_sql}")
.where(access_condition)
.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)
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)
else
query_condition, bind_condition, tables = selector2sql(condition)
tickets_all = Ticket.select("DISTINCT(tickets.id), #{order_select_sql}")
.joins(tables)
.where(access_condition)
.where(query_condition, *bind_condition)
.order(Arel.sql(order_sql))
.offset(offset)
.limit(limit)
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

10
app/policies/application_policy.rb
View file

@ -8,14 +8,4 @@ class ApplicationPolicy
def initialize_context(record)
@record = record
end
class Scope
include PunditPolicy
attr_reader :scope
def initialize_context(scope)
@scope = scope
end
end
end

13
app/policies/application_policy/scope.rb Normal file
View file

@ -0,0 +1,13 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class ApplicationPolicy
class Scope
include PunditPolicy
attr_reader :scope
def initialize_context(scope)
@scope = scope
end
end
end

13
app/policies/organization_policy.rb
View file

@ -14,17 +14,4 @@ class OrganizationPolicy < ApplicationPolicy
false
end
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.organization'])
scope.all
elsif user.organization_id
scope.where(id: user.organization_id)
else
scope.none
end
end
end
end

16
app/policies/organization_policy/scope.rb Normal file
View file

@ -0,0 +1,16 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class OrganizationPolicy < ApplicationPolicy
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.organization'])
scope.all
elsif user.organization_id
scope.where(id: user.organization_id)
else
scope.none
end
end
end
end

48
app/policies/ticket_policy/base_scope.rb Normal file
View file

@ -0,0 +1,48 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
# Abstract base class for various "types" of ticket access.
#
# Do NOT instantiate directly; instead,
# choose the appropriate subclass from below
# (see commit message for details).
class TicketPolicy < ApplicationPolicy
class BaseScope < ApplicationPolicy::Scope
# overwrite PunditPolicy#initialize to make `context` optional and use Ticket as default
def initialize(user, context = Ticket)
super
end
def resolve # rubocop:disable Metrics/AbcSize
raise NoMethodError, <<~ERR.chomp if instance_of?(TicketPolicy::BaseScope)
specify an access type using a subclass of TicketPolicy::BaseScope
ERR
sql = []
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))
end
if user.organization&.shared
sql.push('(tickets.customer_id = ? OR tickets.organization_id = ?)')
bind.push(user.id, user.organization.id)
else
sql.push('tickets.customer_id = ?')
bind.push(user.id)
end
scope.where sql.join(' OR '), *bind
end
# #resolve is UNDEFINED BEHAVIOR for the abstract base class (but not its subclasses)
def respond_to?(*args)
return false if args.first.to_s == 'resolve' && instance_of?(TicketPolicy::BaseScope)
super(*args)
end
end
end

6
app/policies/ticket_policy/full_scope.rb Normal file
View file

@ -0,0 +1,6 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class TicketPolicy < ApplicationPolicy
class FullScope < BaseScope
end
end

6
app/policies/ticket_policy/overview_scope.rb Normal file
View file

@ -0,0 +1,6 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class TicketPolicy < ApplicationPolicy
class OverviewScope < BaseScope
end
end

6
app/policies/ticket_policy/read_scope.rb Normal file
View file

@ -0,0 +1,6 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class TicketPolicy < ApplicationPolicy
class ReadScope < BaseScope
end
end

11
app/policies/user_policy.rb
View file

@ -37,15 +37,4 @@ class UserPolicy < ApplicationPolicy
record.organization_id == user.organization_id
end
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.user'])
scope.all
else
scope.where(id: user.id)
end
end
end
end

14
app/policies/user_policy/scope.rb Normal file
View file

@ -0,0 +1,14 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
class UserPolicy < ApplicationPolicy
class Scope < ApplicationPolicy::Scope
def resolve
if user.permissions?(['ticket.agent', 'admin.user'])
scope.all
else
scope.where(id: user.id)
end
end
end
end

25
spec/policies/ticket_policy/base_scope_spec.rb Normal file
View file

@ -0,0 +1,25 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
RSpec.describe TicketPolicy::BaseScope do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
let(:user) { create(:agent) }
it 'raises NoMethodError (undefined on abstract base class)' do
expect { scope.resolve }.to raise_error(NoMethodError)
end
end
context 'when querying for customer user' do
let(:user) { create(:customer) }
it 'raises NoMethodError (undefined on abstract base class)' do
expect { scope.resolve }.to raise_error(NoMethodError)
end
end
end
end

28
spec/policies/ticket_policy/full_scope_spec.rb Normal file
View file

@ -0,0 +1,28 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
require 'policies/ticket_policy/shared_examples'
RSpec.describe TicketPolicy::FullScope do
context 'with default scope' do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
include_examples 'for agent user', 'full'
end
context 'when querying for customer user' do
include_examples 'for customer user'
end
end
end
context 'with predefined, impossible scope' do
subject(:scope) { described_class.new(user, Ticket.where(id: -1)) }
describe '#resolve' do
include_examples 'for agent user with predefined but impossible context'
end
end
end

28
spec/policies/ticket_policy/overview_scope_spec.rb Normal file
View file

@ -0,0 +1,28 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
require 'policies/ticket_policy/shared_examples'
RSpec.describe TicketPolicy::OverviewScope do
context 'with default scope' do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
include_examples 'for agent user', 'overview'
end
context 'when querying for customer user' do
include_examples 'for customer user'
end
end
end
context 'with predefined, impossible scope' do
subject(:scope) { described_class.new(user, Ticket.where(id: -1)) }
describe '#resolve' do
include_examples 'for agent user with predefined but impossible context'
end
end
end

29
spec/policies/ticket_policy/read_scope_spec.rb Normal file
View file

@ -0,0 +1,29 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
require 'rails_helper'
require 'policies/ticket_policy/shared_examples'
RSpec.describe TicketPolicy::ReadScope do
context 'with default scope' do
subject(:scope) { described_class.new(user) }
describe '#resolve' do
context 'when querying for agent user' do
include_examples 'for agent user', 'read'
end
context 'when querying for customer user' do
include_examples 'for customer user'
end
end
end
context 'with predefined, impossible scope' do
subject(:scope) { described_class.new(user, Ticket.where(id: -1)) }
describe '#resolve' do
include_examples 'for agent user with predefined but impossible context'
end
end
end

121
spec/policies/ticket_policy/shared_examples.rb Normal file
View file

@ -0,0 +1,121 @@
# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
RSpec.shared_examples 'for agent user' do |access_type|
let(:member_groups) { create_list(:group, 2) }
let(:nonmember_group) { create(:group) }
before do
create(:ticket, group: member_groups.first)
create(:ticket, group: member_groups.second)
create(:ticket, group: nonmember_group)
end
shared_examples 'shown' do
it 'returns its groups’ tickets' do
expect(scope.resolve)
.to match_array(Ticket.where(group: member_groups))
end
end
shared_examples 'hidden' do
it 'does not return its groups’ tickets' do
expect(scope.resolve)
.to be_empty
end
end
context 'with direct access via User#groups' do
let(:user) { create(:agent, groups: member_groups) }
context 'when checkin 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') } }
include_examples 'shown'
end
context 'when limited to "read" access' do
before { user.user_groups.each { |ug| ug.update_columns(access: 'read') } }
include_examples access_type == 'read' ? 'shown' : 'hidden'
end
context 'when limited to "overview" access' do
before { user.user_groups.each { |ug| ug.update_columns(access: 'overview') } }
include_examples access_type == 'overview' ? 'shown' : 'hidden'
end
end
context 'with indirect access via Role#groups' do
let(:user) { create(:agent).tap { |u| u.roles << role } }
let(:role) { create(:role, groups: member_groups) }
context 'when checkin 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') } }
include_examples 'shown'
end
context 'when limited to "read" access' do
before { role.role_groups.each { |rg| rg.update_columns(access: 'read') } }
include_examples access_type == 'read' ? 'shown' : 'hidden'
end
context 'when limited to "overview" access' do
before { role.role_groups.each { |rg| rg.update_columns(access: 'overview') } }
include_examples access_type == 'overview' ? 'shown' : 'hidden'
end
end
end
RSpec.shared_examples 'for agent user with predefined but impossible context' do
let(:member_groups) { create_list(:group, 2) }
let(:nonmember_group) { create(:group) }
let(:user) { create(:agent, groups: member_groups) }
before do
create(:ticket, group: member_groups.first)
create(:ticket, group: member_groups.second)
create(:ticket, group: nonmember_group)
end
it 'does not find tickets because of restrictive predefined scope' do
expect(scope.resolve).to match_array(Ticket.where(id: -1))
end
end
RSpec.shared_examples 'for customer user' do
let(:user) { create(:customer, organization: organization) }
let!(:user_tickets) { create_list(:ticket, 2, customer: user) }
let(:teammate) { create(:customer, organization: organization) }
let!(:teammate_tickets) { create_list(:ticket, 2, customer: teammate) }
context 'with no #organization' do
let(:organization) { nil }
it 'returns only the customer’s tickets' do
expect(scope.resolve).to match_array(user_tickets)
end
end
context 'with a non-shared #organization' do
let(:organization) { create(:organization, shared: false) }
it 'returns only the customer’s tickets' do
expect(scope.resolve).to match_array(user_tickets)
end
end
context 'with a shared #organization (default)' do
let(:organization) { create(:organization, shared: true) }
it 'returns only the customer’s or organization’s tickets' do
expect(scope.resolve).to match_array(user_tickets | teammate_tickets)
end
end
end