From 44da47c8452f3cdf4feca1c345dfcff329fded46 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Wed, 12 Nov 2025 17:59:52 +0530 Subject: [PATCH] review changes --- .../auto_assignment/assignment_service.rb | 8 ++++---- app/services/auto_assignment/rate_limiter.rb | 4 ++-- .../auto_assignment/assignment_service.rb | 15 ++++----------- lib/redis/redis_keys.rb | 5 +++++ .../auto_assignment/assignment_service_spec.rb | 4 ++++ .../auto_assignment/assignment_service_spec.rb | 7 ++++++- .../services/auto_assignment/rate_limiter_spec.rb | 8 ++++---- 7 files changed, 29 insertions(+), 22 deletions(-) diff --git a/app/services/auto_assignment/assignment_service.rb b/app/services/auto_assignment/assignment_service.rb index 809ece192..59e7b5a7b 100644 --- a/app/services/auto_assignment/assignment_service.rb +++ b/app/services/auto_assignment/assignment_service.rb @@ -2,7 +2,7 @@ class AutoAssignment::AssignmentService pattr_initialize [:inbox!] def perform_bulk_assignment(limit: 100) - return 0 unless inbox.enable_auto_assignment + return 0 unless inbox.auto_assignment_v2_enabled? assigned_count = 0 @@ -32,10 +32,10 @@ class AutoAssignment::AssignmentService def unassigned_conversations(limit) scope = inbox.conversations.unassigned.open - scope = if assignment_config['conversation_priority'] == 'longest_waiting' - scope.order(last_activity_at: :asc, created_at: :asc) + scope = if assignment_config['conversation_priority'].to_s == 'longest_waiting' + scope.reorder(last_activity_at: :asc, created_at: :asc) else - scope.order(created_at: :asc) + scope.reorder(created_at: :asc) end scope.limit(limit) diff --git a/app/services/auto_assignment/rate_limiter.rb b/app/services/auto_assignment/rate_limiter.rb index fb3bee837..df0591191 100644 --- a/app/services/auto_assignment/rate_limiter.rb +++ b/app/services/auto_assignment/rate_limiter.rb @@ -40,10 +40,10 @@ class AutoAssignment::RateLimiter end def assignment_key_pattern - "assignment:#{inbox.id}:agent:#{agent.id}:*" + format(Redis::RedisKeys::ASSIGNMENT_KEY_PATTERN, inbox_id: inbox.id, agent_id: agent.id) end def build_assignment_key(conversation_id) - "assignment:#{inbox.id}:agent:#{agent.id}:conversation:#{conversation_id}" + format(Redis::RedisKeys::ASSIGNMENT_KEY, inbox_id: inbox.id, agent_id: agent.id, conversation_id: conversation_id) end end diff --git a/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb b/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb index 1b0bbc079..4f3da08ee 100644 --- a/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb +++ b/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb @@ -13,13 +13,6 @@ module Enterprise::AutoAssignment::AssignmentService }.compact end - # Override to check policy first - def assignment_enabled? - return policy.enabled? if policy - - super - end - # Extend agent finding to add capacity checks def find_available_agent agents = filter_agents_by_rate_limit(inbox.available_agents) @@ -65,11 +58,11 @@ module Enterprise::AutoAssignment::AssignmentService # Apply exclusion rules from capacity policy or assignment policy scope = apply_exclusion_rules(scope) - # Apply conversation priority from config - scope = if assignment_config['conversation_priority'] == 'longest_waiting' - scope.order(last_activity_at: :asc, created_at: :asc) + # Apply conversation priority using enum methods if policy exists + scope = if policy&.longest_waiting? + scope.reorder(last_activity_at: :asc, created_at: :asc) else - scope.order(created_at: :asc) + scope.reorder(created_at: :asc) end scope.limit(limit) diff --git a/lib/redis/redis_keys.rb b/lib/redis/redis_keys.rb index fa64d1043..f5d297e5f 100644 --- a/lib/redis/redis_keys.rb +++ b/lib/redis/redis_keys.rb @@ -42,4 +42,9 @@ module Redis::RedisKeys SLACK_MESSAGE_MUTEX = 'SLACK_MESSAGE_LOCK::%s::%s'.freeze EMAIL_MESSAGE_MUTEX = 'EMAIL_CHANNEL_LOCK::%s'.freeze CRM_PROCESS_MUTEX = 'CRM_PROCESS_MUTEX::%s'.freeze + + ## Auto Assignment Keys + # Track conversation assignments to agents for rate limiting + ASSIGNMENT_KEY = 'ASSIGNMENT::%d::AGENT::%d::CONVERSATION::%d'.freeze + ASSIGNMENT_KEY_PATTERN = 'ASSIGNMENT::%d::AGENT::%d::*'.freeze end diff --git a/spec/enterprise/services/enterprise/auto_assignment/assignment_service_spec.rb b/spec/enterprise/services/enterprise/auto_assignment/assignment_service_spec.rb index 055d7673c..432f397be 100644 --- a/spec/enterprise/services/enterprise/auto_assignment/assignment_service_spec.rb +++ b/spec/enterprise/services/enterprise/auto_assignment/assignment_service_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' RSpec.describe Enterprise::AutoAssignment::AssignmentService, type: :service do let(:account) { create(:account) } + let(:assignment_policy) { create(:assignment_policy, account: account, enabled: true) } let(:inbox) { create(:inbox, account: account) } let(:agent1) { create(:user, account: account, name: 'Agent 1') } let(:agent2) { create(:user, account: account, name: 'Agent 2') } @@ -12,6 +13,9 @@ RSpec.describe Enterprise::AutoAssignment::AssignmentService, type: :service do create(:inbox_member, inbox: inbox, user: agent1) create(:inbox_member, inbox: inbox, user: agent2) + # Link inbox to assignment policy + create(:inbox_assignment_policy, inbox: inbox, assignment_policy: assignment_policy) + allow(account).to receive(:feature_enabled?).and_return(false) allow(account).to receive(:feature_enabled?).with('assignment_v2').and_return(true) diff --git a/spec/services/auto_assignment/assignment_service_spec.rb b/spec/services/auto_assignment/assignment_service_spec.rb index 36f7d029e..535277714 100644 --- a/spec/services/auto_assignment/assignment_service_spec.rb +++ b/spec/services/auto_assignment/assignment_service_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' RSpec.describe AutoAssignment::AssignmentService do let(:account) { create(:account) } + let(:assignment_policy) { create(:assignment_policy, account: account, enabled: true) } let(:inbox) { create(:inbox, account: account, enable_auto_assignment: true) } let(:service) { described_class.new(inbox: inbox) } let(:agent) { create(:user, account: account, role: :agent, availability: :online) } @@ -9,6 +10,10 @@ RSpec.describe AutoAssignment::AssignmentService do let(:conversation) { create(:conversation, inbox: inbox, assignee: nil) } before do + # Enable assignment_v2 feature for the account + allow(account).to receive(:feature_enabled?).with('assignment_v2').and_return(true) + # Link inbox to assignment policy + create(:inbox_assignment_policy, inbox: inbox, assignment_policy: assignment_policy) create(:inbox_member, inbox: inbox, user: agent) end @@ -105,7 +110,7 @@ RSpec.describe AutoAssignment::AssignmentService do end context 'when auto assignment is disabled' do - before { inbox.update!(enable_auto_assignment: false) } + before { assignment_policy.update!(enabled: false) } it 'returns 0 without processing' do assigned_count = service.perform_bulk_assignment(limit: 10) diff --git a/spec/services/auto_assignment/rate_limiter_spec.rb b/spec/services/auto_assignment/rate_limiter_spec.rb index b503f3f38..fceebf7de 100644 --- a/spec/services/auto_assignment/rate_limiter_spec.rb +++ b/spec/services/auto_assignment/rate_limiter_spec.rb @@ -59,7 +59,7 @@ RSpec.describe AutoAssignment::RateLimiter do end it 'creates a Redis key with correct expiry' do - expected_key = "assignment:#{inbox.id}:agent:#{agent.id}:conversation:#{conversation.id}" + expected_key = format(Redis::RedisKeys::ASSIGNMENT_KEY, inbox_id: inbox.id, agent_id: agent.id, conversation_id: conversation.id) expect(Redis::Alfred).to receive(:set).with( expected_key, conversation.id.to_s, @@ -90,7 +90,7 @@ RSpec.describe AutoAssignment::RateLimiter do end it 'counts matching Redis keys' do - pattern = "assignment:#{inbox.id}:agent:#{agent.id}:*" + pattern = format(Redis::RedisKeys::ASSIGNMENT_KEY_PATTERN, inbox_id: inbox.id, agent_id: agent.id) allow(Redis::Alfred).to receive(:keys_count).with(pattern).and_return(3) expect(rate_limiter.current_count).to eq(3) @@ -108,7 +108,7 @@ RSpec.describe AutoAssignment::RateLimiter do end it 'uses the custom window value' do - expected_key = "assignment:#{inbox.id}:agent:#{agent.id}:conversation:#{conversation.id}" + expected_key = format(Redis::RedisKeys::ASSIGNMENT_KEY, inbox_id: inbox.id, agent_id: agent.id, conversation_id: conversation.id) expect(Redis::Alfred).to receive(:set).with( expected_key, conversation.id.to_s, @@ -126,7 +126,7 @@ RSpec.describe AutoAssignment::RateLimiter do end it 'uses the default window value of 3600' do - expected_key = "assignment:#{inbox.id}:agent:#{agent.id}:conversation:#{conversation.id}" + expected_key = format(Redis::RedisKeys::ASSIGNMENT_KEY, inbox_id: inbox.id, agent_id: agent.id, conversation_id: conversation.id) expect(Redis::Alfred).to receive(:set).with( expected_key, conversation.id.to_s,