refactor(captain): extract audience validation into Captain::AudienceValidator
Moves the audience condition-tree validation out of Captain::Assistant into a dedicated ActiveModel validator, and expands spec coverage for audience and response_window validation.
This commit is contained in:
@@ -113,7 +113,7 @@ class Api::V1::Accounts::Captain::AssistantsController < Api::V1::Accounts::Base
|
||||
permitted[:guardrails] = params[:assistant][:guardrails] if params[:assistant].key?(:guardrails)
|
||||
|
||||
# The audience is a recursive condition tree that strong params can't whitelist by shape;
|
||||
# route it through separately. Validity is enforced by Captain::Assistant#validate_audience_structure.
|
||||
# route it through separately. Validity is enforced by Captain::AudienceValidator.
|
||||
permit_audience_config(permitted)
|
||||
|
||||
permitted
|
||||
|
||||
@@ -47,7 +47,7 @@ class Captain::Assistant < ApplicationRecord
|
||||
validates :name, presence: true
|
||||
validates :description, presence: true, length: { maximum: DESCRIPTION_LENGTH_LIMIT }
|
||||
validates :account_id, presence: true
|
||||
validate :validate_audience_structure
|
||||
validates_with Captain::AudienceValidator
|
||||
validate :validate_response_window
|
||||
|
||||
scope :ordered, -> { order(created_at: :desc) }
|
||||
@@ -115,37 +115,11 @@ class Captain::Assistant < ApplicationRecord
|
||||
|
||||
private
|
||||
|
||||
def validate_audience_structure
|
||||
audience = config['audience']
|
||||
return if audience.blank?
|
||||
|
||||
errors.add(:config, 'audience must be a valid condition tree') unless valid_audience_node?(audience, 1)
|
||||
end
|
||||
|
||||
def validate_response_window
|
||||
window = config['response_window']
|
||||
return if window.blank?
|
||||
response_window = config['response_window']
|
||||
return if response_window.blank?
|
||||
|
||||
errors.add(:config, 'invalid response_window') unless RESPONSE_WINDOWS.include?(window)
|
||||
end
|
||||
|
||||
def valid_audience_node?(node, depth)
|
||||
return false unless node.is_a?(Hash) && depth <= Captain::AudienceMatcher::MAX_DEPTH
|
||||
|
||||
node = node.with_indifferent_access
|
||||
return valid_audience_group?(node, depth) if node.key?(:conditions)
|
||||
|
||||
valid_audience_leaf?(node)
|
||||
end
|
||||
|
||||
def valid_audience_group?(node, depth)
|
||||
node[:conditions].is_a?(Array) &&
|
||||
node[:conditions].present? &&
|
||||
node[:conditions].all? { |child| valid_audience_node?(child, depth + 1) }
|
||||
end
|
||||
|
||||
def valid_audience_leaf?(node)
|
||||
node[:attribute_key].present? && Captain::AudienceMatcher::OPERATORS.include?(node[:filter_operator])
|
||||
errors.add(:config, 'invalid response_window') unless RESPONSE_WINDOWS.include?(response_window)
|
||||
end
|
||||
|
||||
def agent_name
|
||||
|
||||
@@ -0,0 +1,29 @@
|
||||
class Captain::AudienceValidator < ActiveModel::Validator
|
||||
def validate(record)
|
||||
audience = record.config['audience']
|
||||
return if audience.blank?
|
||||
|
||||
record.errors.add(:config, 'audience must be a valid condition tree') unless valid_node?(audience, 1)
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def valid_node?(node, depth)
|
||||
return false unless node.is_a?(Hash) && depth <= Captain::AudienceMatcher::MAX_DEPTH
|
||||
|
||||
node = node.with_indifferent_access
|
||||
return valid_group?(node, depth) if node.key?(:conditions)
|
||||
|
||||
valid_leaf?(node)
|
||||
end
|
||||
|
||||
def valid_group?(node, depth)
|
||||
node[:conditions].is_a?(Array) &&
|
||||
node[:conditions].present? &&
|
||||
node[:conditions].all? { |child| valid_node?(child, depth + 1) }
|
||||
end
|
||||
|
||||
def valid_leaf?(node)
|
||||
node[:attribute_key].present? && Captain::AudienceMatcher::OPERATORS.include?(node[:filter_operator])
|
||||
end
|
||||
end
|
||||
@@ -64,8 +64,13 @@ RSpec.describe Captain::Assistant, type: :model do
|
||||
end
|
||||
|
||||
describe 'response_window validation' do
|
||||
it 'accepts a blank response_window' do
|
||||
assistant.config['response_window'] = nil
|
||||
expect(assistant).to be_valid
|
||||
end
|
||||
|
||||
it 'accepts the known windows' do
|
||||
%w[always business_hours outside_business_hours].each do |window|
|
||||
described_class::RESPONSE_WINDOWS.each do |window|
|
||||
assistant.config['response_window'] = window
|
||||
expect(assistant).to be_valid
|
||||
end
|
||||
@@ -74,22 +79,52 @@ RSpec.describe Captain::Assistant, type: :model do
|
||||
it 'rejects an unknown window' do
|
||||
assistant.config['response_window'] = 'weekends'
|
||||
expect(assistant).not_to be_valid
|
||||
expect(assistant.errors[:config]).to include('invalid response_window')
|
||||
end
|
||||
end
|
||||
|
||||
describe 'audience validation' do
|
||||
let(:leaf) { { 'attribute_key' => 'country_code', 'filter_operator' => 'equal_to', 'values' => ['US'] } }
|
||||
|
||||
it 'accepts a blank audience' do
|
||||
assistant.config['audience'] = nil
|
||||
expect(assistant).to be_valid
|
||||
end
|
||||
|
||||
it 'accepts a single leaf' do
|
||||
assistant.config['audience'] = leaf
|
||||
expect(assistant).to be_valid
|
||||
end
|
||||
|
||||
it 'accepts symbol keys' do
|
||||
assistant.config['audience'] = { attribute_key: 'country_code', filter_operator: 'equal_to', values: ['US'] }
|
||||
expect(assistant).to be_valid
|
||||
end
|
||||
|
||||
it 'accepts a well-formed nested tree' do
|
||||
assistant.config['audience'] = {
|
||||
'operator' => 'and',
|
||||
'conditions' => [
|
||||
{ 'attribute_key' => 'country_code', 'filter_operator' => 'equal_to', 'values' => ['US'] }
|
||||
{ 'operator' => 'or', 'conditions' => [leaf] },
|
||||
leaf
|
||||
]
|
||||
}
|
||||
expect(assistant).to be_valid
|
||||
end
|
||||
|
||||
it 'rejects a node that is not a hash' do
|
||||
assistant.config['audience'] = ['not-a-node']
|
||||
expect(assistant).not_to be_valid
|
||||
expect(assistant.errors[:config]).to include('audience must be a valid condition tree')
|
||||
end
|
||||
|
||||
it 'rejects an unknown operator' do
|
||||
assistant.config['audience'] = { 'attribute_key' => 'country_code', 'filter_operator' => 'bogus', 'values' => ['US'] }
|
||||
assistant.config['audience'] = leaf.merge('filter_operator' => 'bogus')
|
||||
expect(assistant).not_to be_valid
|
||||
end
|
||||
|
||||
it 'rejects a leaf missing attribute_key' do
|
||||
assistant.config['audience'] = { 'filter_operator' => 'equal_to', 'values' => ['US'] }
|
||||
expect(assistant).not_to be_valid
|
||||
end
|
||||
|
||||
@@ -98,14 +133,22 @@ RSpec.describe Captain::Assistant, type: :model do
|
||||
expect(assistant).not_to be_valid
|
||||
end
|
||||
|
||||
it 'rejects conditions that is not an array' do
|
||||
assistant.config['audience'] = { 'operator' => 'and', 'conditions' => leaf }
|
||||
expect(assistant).not_to be_valid
|
||||
end
|
||||
|
||||
it 'rejects a group containing an invalid child' do
|
||||
assistant.config['audience'] = { 'operator' => 'and', 'conditions' => [leaf, { 'attribute_key' => '' }] }
|
||||
expect(assistant).not_to be_valid
|
||||
end
|
||||
|
||||
it 'rejects nesting deeper than one level' do
|
||||
assistant.config['audience'] = {
|
||||
'operator' => 'and',
|
||||
'conditions' => [
|
||||
{ 'operator' => 'or', 'conditions' => [
|
||||
{ 'operator' => 'and', 'conditions' => [
|
||||
{ 'attribute_key' => 'country_code', 'filter_operator' => 'equal_to', 'values' => ['US'] }
|
||||
] }
|
||||
{ 'operator' => 'and', 'conditions' => [leaf] }
|
||||
] }
|
||||
]
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user