From a4fab22e281ca83ede839ee864b1ddf7dd6ea8ca Mon Sep 17 00:00:00 2001 From: Tanmay Sharma Date: Fri, 29 Aug 2025 16:26:19 +0700 Subject: [PATCH] fix review changes --- .../auto_assignment/assignment_service.rb | 10 ++-- .../auto_assignment/assignment_service.rb | 7 ++- .../auto_assignment/capacity_service_spec.rb | 12 +++-- .../assignment_service_basic_spec.rb | 23 +++++----- ...signment_service_fair_distribution_spec.rb | 46 +++++++++++-------- .../assignment_service_spec.rb | 46 ++++++++++++------- 6 files changed, 89 insertions(+), 55 deletions(-) diff --git a/app/services/auto_assignment/assignment_service.rb b/app/services/auto_assignment/assignment_service.rb index 48c8b1fba..02e42b554 100644 --- a/app/services/auto_assignment/assignment_service.rb +++ b/app/services/auto_assignment/assignment_service.rb @@ -32,11 +32,11 @@ class AutoAssignment::AssignmentService def unassigned_conversations(limit) scope = inbox.conversations.unassigned.open - if assignment_config['conversation_priority'] == 'longest_waiting' - scope = scope.order(last_activity_at: :asc, created_at: :asc) - else - scope.order(created_at: :asc) - end + scope = if assignment_config['conversation_priority'] == 'longest_waiting' + scope.order(last_activity_at: :asc, created_at: :asc) + else + scope.order(created_at: :asc) + end scope.limit(limit) end diff --git a/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb b/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb index 8a135f484..1b0bbc079 100644 --- a/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb +++ b/enterprise/app/services/enterprise/auto_assignment/assignment_service.rb @@ -66,7 +66,12 @@ module Enterprise::AutoAssignment::AssignmentService scope = apply_exclusion_rules(scope) # Apply conversation priority from config - scope = apply_conversation_priority(scope) + scope = if assignment_config['conversation_priority'] == 'longest_waiting' + scope.order(last_activity_at: :asc, created_at: :asc) + else + scope.order(created_at: :asc) + end + scope.limit(limit) end diff --git a/spec/enterprise/auto_assignment/capacity_service_spec.rb b/spec/enterprise/auto_assignment/capacity_service_spec.rb index b945f5140..98d7c2172 100644 --- a/spec/enterprise/auto_assignment/capacity_service_spec.rb +++ b/spec/enterprise/auto_assignment/capacity_service_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' RSpec.describe Enterprise::AutoAssignment::CapacityService, type: :service do let(:account) { create(:account) } - let(:inbox) { create(:inbox, account: account) } + let(:inbox) { create(:inbox, account: account, enable_auto_assignment: true) } # Assignment policy with rate limiting let(:assignment_policy) do @@ -87,9 +87,11 @@ RSpec.describe Enterprise::AutoAssignment::CapacityService, type: :service do describe 'assignment with capacity' do let(:service) { AutoAssignment::AssignmentService.new(inbox: inbox) } - let(:conversation) { create(:conversation, inbox: inbox, assignee: nil, status: :open) } it 'assigns to agents with available capacity' do + # Create conversation before assignment + conversation = create(:conversation, inbox: inbox, assignee: nil, status: :open) + # Mock the selector to prefer agent_at_capacity (but should skip due to capacity) selector = instance_double(AutoAssignment::RoundRobinSelector) allow(AutoAssignment::RoundRobinSelector).to receive(:new).and_return(selector) @@ -97,7 +99,8 @@ RSpec.describe Enterprise::AutoAssignment::CapacityService, type: :service do agents.map(&:user).find { |u| [agent_with_capacity, agent_without_capacity].include?(u) } end - expect(service.perform_for_conversation(conversation)).to be true + assigned_count = service.perform_bulk_assignment(limit: 1) + expect(assigned_count).to eq(1) expect(conversation.reload.assignee).to be_in([agent_with_capacity, agent_without_capacity]) expect(conversation.reload.assignee).not_to eq(agent_at_capacity) end @@ -108,7 +111,8 @@ RSpec.describe Enterprise::AutoAssignment::CapacityService, type: :service do # agent_without_capacity has no limit, so should still be available conversation2 = create(:conversation, inbox: inbox, assignee: nil, status: :open) - expect(service.perform_for_conversation(conversation2)).to be true + assigned_count = service.perform_bulk_assignment(limit: 1) + expect(assigned_count).to eq(1) expect(conversation2.reload.assignee).to eq(agent_without_capacity) end end diff --git a/spec/services/auto_assignment/assignment_service_basic_spec.rb b/spec/services/auto_assignment/assignment_service_basic_spec.rb index 9106d8381..b85381a71 100644 --- a/spec/services/auto_assignment/assignment_service_basic_spec.rb +++ b/spec/services/auto_assignment/assignment_service_basic_spec.rb @@ -26,19 +26,19 @@ RSpec.describe AutoAssignment::AssignmentService do it 'assigns an available agent to conversation' do conversation = create_test_conversation - result = service.perform_for_conversation(conversation) + assigned_count = service.perform_bulk_assignment(limit: 1) - expect(result).to be true + expect(assigned_count).to eq(1) expect(conversation.reload.assignee).to eq(agent) end - it 'returns false when no agents are online' do + it 'returns 0 when no agents are online' do allow(OnlineStatusTracker).to receive(:get_available_users).and_return({}) conversation = create_test_conversation - result = service.perform_for_conversation(conversation) + assigned_count = service.perform_bulk_assignment(limit: 1) - expect(result).to be false + expect(assigned_count).to eq(0) expect(conversation.reload.assignee).to be_nil end end @@ -49,9 +49,9 @@ RSpec.describe AutoAssignment::AssignmentService do it 'does not assign any agent' do conversation = create_test_conversation - result = service.perform_for_conversation(conversation) + assigned_count = service.perform_bulk_assignment(limit: 1) - expect(result).to be false + expect(assigned_count).to eq(0) expect(conversation.reload.assignee).to be_nil end end @@ -63,18 +63,19 @@ RSpec.describe AutoAssignment::AssignmentService do it 'only assigns to open conversations' do resolved_conversation = create_test_conversation(status: 'resolved') - result = service.perform_for_conversation(resolved_conversation) + assigned_count = service.perform_bulk_assignment(limit: 1) - expect(result).to be false + expect(assigned_count).to eq(0) + expect(resolved_conversation.reload.assignee).to be_nil end it 'does not reassign already assigned conversations' do other_agent = create(:user, account: account, role: :agent) assigned_conversation = create_test_conversation(assignee: other_agent) - result = service.perform_for_conversation(assigned_conversation) + assigned_count = service.perform_bulk_assignment(limit: 1) - expect(result).to be false + expect(assigned_count).to eq(0) expect(assigned_conversation.reload.assignee).to eq(other_agent) end end diff --git a/spec/services/auto_assignment/assignment_service_fair_distribution_spec.rb b/spec/services/auto_assignment/assignment_service_fair_distribution_spec.rb index a5d2b0018..b3a86e8b8 100644 --- a/spec/services/auto_assignment/assignment_service_fair_distribution_spec.rb +++ b/spec/services/auto_assignment/assignment_service_fair_distribution_spec.rb @@ -52,10 +52,9 @@ RSpec.describe AutoAssignment::AssignmentService do # Each agent can handle 2 conversations conversations = Array.new(4) { create_test_conversation } - # First 4 conversations should be assigned (2 per agent) - conversations.each do |conv| - expect(service.perform_for_conversation(conv)).to be true - end + # Assign first 4 conversations (2 per agent) + assigned_count = service.perform_bulk_assignment(limit: 4) + expect(assigned_count).to eq(4) # Verify distribution assigned_to_agent1 = conversations.count { |c| c.reload.assignee == agent1 } @@ -64,15 +63,16 @@ RSpec.describe AutoAssignment::AssignmentService do expect(assigned_to_agent1).to eq(2) expect(assigned_to_agent2).to eq(2) - # Fifth conversation should fail (both agents at limit) + # Fifth conversation should not be assigned (both agents at limit) fifth_conversation = create_test_conversation - expect(service.perform_for_conversation(fifth_conversation)).to be false + additional_assigned = service.perform_bulk_assignment(limit: 1) + expect(additional_assigned).to eq(0) expect(fifth_conversation.reload.assignee).to be_nil end it 'tracks assignments using individual Redis keys' do conversation = create_test_conversation - service.perform_for_conversation(conversation) + service.perform_bulk_assignment(limit: 1) # Check that assignment key exists pattern = "assignment:#{inbox.id}:agent:#{conversation.reload.assignee.id}:*" @@ -81,13 +81,18 @@ RSpec.describe AutoAssignment::AssignmentService do end it 'allows new assignments after window expires' do - # Assign 2 conversations to agent1 - 2.times do - conversation = create_test_conversation - allow(service).to receive(:round_robin_selector).and_return( - instance_double(AutoAssignment::RoundRobinSelector, select_agent: agent1) - ) - service.perform_for_conversation(conversation) + # Create 2 conversations and force assignment to agent1 + conversations = Array.new(2) { create_test_conversation } + allow(service).to receive(:round_robin_selector).and_return( + instance_double(AutoAssignment::RoundRobinSelector, select_agent: agent1) + ) + + # Assign both to agent1 + assigned_count = service.perform_bulk_assignment(limit: 2) + expect(assigned_count).to eq(2) + + conversations.each do |c| + expect(c.reload.assignee).to eq(agent1) end # Agent1 is now at limit @@ -105,7 +110,9 @@ RSpec.describe AutoAssignment::AssignmentService do allow(service).to receive(:round_robin_selector).and_return( instance_double(AutoAssignment::RoundRobinSelector, select_agent: agent1) ) - expect(service.perform_for_conversation(new_conversation)).to be true + assigned_count = service.perform_bulk_assignment(limit: 1) + expect(assigned_count).to eq(1) + expect(new_conversation.reload.assignee).to eq(agent1) end end @@ -116,9 +123,12 @@ RSpec.describe AutoAssignment::AssignmentService do it 'assigns without limits' do # Create more conversations than would be allowed with limits - 5.times do - conversation = create_test_conversation - expect(service.perform_for_conversation(conversation)).to be true + conversations = Array.new(5) { create_test_conversation } + + assigned_count = service.perform_bulk_assignment(limit: 5) + expect(assigned_count).to eq(5) + + conversations.each do |conversation| expect(conversation.reload.assignee).not_to be_nil end end diff --git a/spec/services/auto_assignment/assignment_service_spec.rb b/spec/services/auto_assignment/assignment_service_spec.rb index 62dfdb2b9..686b98be7 100644 --- a/spec/services/auto_assignment/assignment_service_spec.rb +++ b/spec/services/auto_assignment/assignment_service_spec.rb @@ -19,21 +19,23 @@ RSpec.describe AutoAssignment::AssignmentService do conversation end - describe '#perform_for_conversation' do - let(:conversation) { create_test_conversation } - + describe 'assignment behavior' do before do allow(OnlineStatusTracker).to receive(:get_available_users).and_return({ agent.id.to_s => 'online' }) end context 'when auto assignment is enabled' do it 'assigns conversation to available agent' do - expect(service.perform_for_conversation(conversation)).to be true + conversation = create_test_conversation + + assigned_count = service.perform_bulk_assignment(limit: 1) + + expect(assigned_count).to eq(1) expect(conversation.reload.assignee).to eq(agent) end it 'dispatches assignee changed event' do - # The conversation update triggers its own event through callbacks + conversation = create_test_conversation allow(Rails.configuration.dispatcher).to receive(:dispatch).and_call_original expect(Rails.configuration.dispatcher).to receive(:dispatch).with( @@ -42,28 +44,38 @@ RSpec.describe AutoAssignment::AssignmentService do hash_including(conversation: conversation, user: agent) ) - service.perform_for_conversation(conversation) + service.perform_bulk_assignment(limit: 1) end - it 'returns false when no agents available' do + it 'returns 0 when no agents available' do + create_test_conversation allow(OnlineStatusTracker).to receive(:get_available_users).and_return({}) - expect(service.perform_for_conversation(conversation)).to be false + + assigned_count = service.perform_bulk_assignment(limit: 1) + + expect(assigned_count).to eq(0) end end context 'when conversation already assigned' do - let(:conversation) { create_test_conversation(assignee: agent) } - it 'does not reassign' do - expect(service.perform_for_conversation(conversation)).to be false + conversation = create_test_conversation(assignee: agent) + + assigned_count = service.perform_bulk_assignment(limit: 1) + + expect(assigned_count).to eq(0) + expect(conversation.reload.assignee).to eq(agent) end end context 'when conversation is not open' do - let(:conversation) { create_test_conversation(status: 'resolved') } - it 'does not assign' do - expect(service.perform_for_conversation(conversation)).to be false + conversation = create_test_conversation(status: 'resolved') + + assigned_count = service.perform_bulk_assignment(limit: 1) + + expect(assigned_count).to eq(0) + expect(conversation.reload.assignee).to be_nil end end end @@ -124,7 +136,9 @@ RSpec.describe AutoAssignment::AssignmentService do conversation = create_test_conversation - expect(service.perform_for_conversation(conversation)).to be true + assigned_count = service.perform_bulk_assignment(limit: 1) + + expect(assigned_count).to eq(1) expect(conversation.reload.assignee).to eq(agent2) end @@ -136,7 +150,7 @@ RSpec.describe AutoAssignment::AssignmentService do expect(rate_limiter).to receive(:track_assignment).with(conversation) - service.perform_for_conversation(conversation) + service.perform_bulk_assignment(limit: 1) end end