From b7ac1c28fb2264acc0a8d46241d1513119c0aca5 Mon Sep 17 00:00:00 2001 From: Pranav Date: Tue, 14 Jul 2026 17:06:51 -0700 Subject: [PATCH] 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. --- .../accounts/captain/assistants_controller.rb | 2 +- enterprise/app/models/captain/assistant.rb | 34 ++---------- .../validators/captain/audience_validator.rb | 29 ++++++++++ .../models/captain/assistant_spec.rb | 55 +++++++++++++++++-- 4 files changed, 83 insertions(+), 37 deletions(-) create mode 100644 enterprise/app/validators/captain/audience_validator.rb diff --git a/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb b/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb index 94a031973..9602b073d 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb @@ -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 diff --git a/enterprise/app/models/captain/assistant.rb b/enterprise/app/models/captain/assistant.rb index 462ad8134..007d8bf02 100644 --- a/enterprise/app/models/captain/assistant.rb +++ b/enterprise/app/models/captain/assistant.rb @@ -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 diff --git a/enterprise/app/validators/captain/audience_validator.rb b/enterprise/app/validators/captain/audience_validator.rb new file mode 100644 index 000000000..aa259a915 --- /dev/null +++ b/enterprise/app/validators/captain/audience_validator.rb @@ -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 diff --git a/spec/enterprise/models/captain/assistant_spec.rb b/spec/enterprise/models/captain/assistant_spec.rb index 6724abe53..bb5adb7cb 100644 --- a/spec/enterprise/models/captain/assistant_spec.rb +++ b/spec/enterprise/models/captain/assistant_spec.rb @@ -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] } ] } ] }