From a07f2a7c1b9dd36cb7103cf7b4d5acfee7e817cd Mon Sep 17 00:00:00 2001 From: Pranav Date: Tue, 20 May 2025 19:22:17 -0700 Subject: [PATCH] feat: Add support for search_conversations in copilot (#11520) Earlier, we were manually checking if a user was an agent and filtering their conversations based on inboxes. This logic should have been part of the conversation permissions service. This PR moves the check to the right place and updates the logic accordingly. Other updates: - Add support for search_conversations service for copilot. - Use PermissionFilterService in contacts/conversations, conversations, copilot search_conversations. --------- Co-authored-by: Sojan Co-authored-by: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Co-authored-by: Muhsin Keloth --- .../contacts/conversations_controller.rb | 4 -- app/finders/conversation_finder.rb | 5 +- app/services/conversations/filter_service.rb | 10 --- .../permission_filter_service.rb | 20 +++++- .../services/captain/tools/base_service.rb | 3 +- .../copilot/search_conversations_service.rb | 72 +++++++++++++++++++ .../permission_filter_service.rb | 27 +++---- .../search_conversations_service_spec.rb | 67 +++++++++++++++++ .../permission_filter_service_spec.rb | 22 ++++-- 9 files changed, 193 insertions(+), 37 deletions(-) create mode 100644 enterprise/app/services/captain/tools/copilot/search_conversations_service.rb create mode 100644 spec/enterprise/services/captain/tools/copilot/search_conversations_service_spec.rb diff --git a/app/controllers/api/v1/accounts/contacts/conversations_controller.rb b/app/controllers/api/v1/accounts/contacts/conversations_controller.rb index fda19b8c2..20d66fb4d 100644 --- a/app/controllers/api/v1/accounts/contacts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/contacts/conversations_controller.rb @@ -12,10 +12,6 @@ class Api::V1::Accounts::Contacts::ConversationsController < Api::V1::Accounts:: Current.account ).perform - # Only allow conversations from inboxes the user has access to - inbox_ids = Current.user.assigned_inboxes.pluck(:id) - conversations = conversations.where(inbox_id: inbox_ids) - @conversations = conversations.order(last_activity_at: :desc).limit(20) end end diff --git a/app/finders/conversation_finder.rb b/app/finders/conversation_finder.rb index 1694199ba..d43ed31e7 100644 --- a/app/finders/conversation_finder.rb +++ b/app/finders/conversation_finder.rb @@ -88,7 +88,10 @@ class ConversationFinder def find_conversation_by_inbox @conversations = current_account.conversations - @conversations = @conversations.where(inbox_id: @inbox_ids) unless params[:inbox_id].blank? && @is_admin + + return unless params[:inbox_id] + + @conversations = @conversations.where(inbox_id: @inbox_ids) end def find_all_conversations diff --git a/app/services/conversations/filter_service.rb b/app/services/conversations/filter_service.rb index 21fe3c716..db2892c31 100644 --- a/app/services/conversations/filter_service.rb +++ b/app/services/conversations/filter_service.rb @@ -28,16 +28,6 @@ class Conversations::FilterService < FilterService :taggings, :inbox, { assignee: { avatar_attachment: [:blob] } }, { contact: { avatar_attachment: [:blob] } }, :team, :messages, :contact_inbox ) - account_user = @account.account_users.find_by(user_id: @user.id) - is_administrator = account_user&.role == 'administrator' - - # Ensure we only include conversations from inboxes the user has access to - unless is_administrator - inbox_ids = @user.inboxes.where(account_id: @account.id).pluck(:id) - conversations = conversations.where(inbox_id: inbox_ids) - end - - # Apply permission-based filtering Conversations::PermissionFilterService.new( conversations, @user, diff --git a/app/services/conversations/permission_filter_service.rb b/app/services/conversations/permission_filter_service.rb index a8561dbdf..31dd011d7 100644 --- a/app/services/conversations/permission_filter_service.rb +++ b/app/services/conversations/permission_filter_service.rb @@ -8,9 +8,23 @@ class Conversations::PermissionFilterService end def perform - # The base implementation simply returns all conversations - # Enterprise edition extends this with permission-based filtering - conversations + return conversations if user_role == 'administrator' + + accessible_conversations + end + + private + + def accessible_conversations + conversations.where(inbox: user.inboxes.where(account_id: account.id)) + end + + def account_user + AccountUser.find_by(account_id: account.id, user_id: user.id) + end + + def user_role + account_user&.role end end diff --git a/enterprise/app/services/captain/tools/base_service.rb b/enterprise/app/services/captain/tools/base_service.rb index 7fadc806b..9143726b7 100644 --- a/enterprise/app/services/captain/tools/base_service.rb +++ b/enterprise/app/services/captain/tools/base_service.rb @@ -1,8 +1,9 @@ class Captain::Tools::BaseService attr_accessor :assistant - def initialize(assistant) + def initialize(assistant, user: nil) @assistant = assistant + @user = user end def name diff --git a/enterprise/app/services/captain/tools/copilot/search_conversations_service.rb b/enterprise/app/services/captain/tools/copilot/search_conversations_service.rb new file mode 100644 index 000000000..6b6b063ea --- /dev/null +++ b/enterprise/app/services/captain/tools/copilot/search_conversations_service.rb @@ -0,0 +1,72 @@ +class Captain::Tools::Copilot::SearchConversationsService < Captain::Tools::BaseService + def name + 'search_conversations' + end + + def description + 'Search conversations based on parameters' + end + + def parameters + { + type: 'object', + properties: properties, + required: [] + } + end + + def execute(arguments) + status = arguments['status'] + contact_id = arguments['contact_id'] + priority = arguments['priority'] + + conversations = get_conversations(status, contact_id, priority) + + return 'No conversations found' unless conversations.exists? + + total_count = conversations.count + conversations = conversations.limit(100) + + <<~RESPONSE + #{total_count > 100 ? "Found #{total_count} conversations (showing first 100)" : "Total number of conversations: #{total_count}"} + #{conversations.map { |conversation| conversation.to_llm_text(include_contact_details: true) }.join("\n---\n")} + RESPONSE + end + + private + + def get_conversations(status, contact_id, priority) + conversations = permissible_conversations + conversations = conversations.where(contact_id: contact_id) if contact_id.present? + conversations = conversations.where(status: status) if status.present? + conversations = conversations.where(priority: priority) if priority.present? + conversations + end + + def permissible_conversations + Conversations::PermissionFilterService.new( + @assistant.account.conversations, + @user, + @assistant.account + ).perform + end + + def properties + { + contact_id: { + type: 'number', + description: 'Filter conversations by contact ID' + }, + status: { + type: 'string', + enum: %w[open resolved pending snoozed], + description: 'Filter conversations by status' + }, + priority: { + type: 'string', + enum: %w[low medium high urgent], + description: 'Filter conversations by priority' + } + } + end +end diff --git a/enterprise/app/services/enterprise/conversations/permission_filter_service.rb b/enterprise/app/services/enterprise/conversations/permission_filter_service.rb index 5da65ffa6..f55265a90 100644 --- a/enterprise/app/services/enterprise/conversations/permission_filter_service.rb +++ b/enterprise/app/services/enterprise/conversations/permission_filter_service.rb @@ -1,36 +1,37 @@ module Enterprise::Conversations::PermissionFilterService def perform - account_user = AccountUser.find_by(account_id: account.id, user_id: user.id) - permissions = account_user&.permissions || [] - user_role = account_user&.role + return filter_by_permissions(permissions) if user_has_custom_role? - # Skip filtering for administrators - return conversations if user_role == 'administrator' - # Skip filtering for regular agents (without custom roles/permissions) - return conversations if user_role == 'agent' && account_user&.custom_role_id.nil? - - filter_by_permissions(permissions) + super end private + def user_has_custom_role? + user_role == 'agent' && account_user&.custom_role_id.present? + end + + def permissions + account_user&.permissions || [] + end + def filter_by_permissions(permissions) # Permission-based filtering with hierarchy # conversation_manage > conversation_unassigned_manage > conversation_participating_manage if permissions.include?('conversation_manage') - conversations + accessible_conversations elsif permissions.include?('conversation_unassigned_manage') filter_unassigned_and_mine elsif permissions.include?('conversation_participating_manage') - conversations.assigned_to(user) + accessible_conversations.assigned_to(user) else Conversation.none end end def filter_unassigned_and_mine - mine = conversations.assigned_to(user) - unassigned = conversations.unassigned + mine = accessible_conversations.assigned_to(user) + unassigned = accessible_conversations.unassigned Conversation.from("(#{mine.to_sql} UNION #{unassigned.to_sql}) as conversations") .where(account_id: account.id) diff --git a/spec/enterprise/services/captain/tools/copilot/search_conversations_service_spec.rb b/spec/enterprise/services/captain/tools/copilot/search_conversations_service_spec.rb new file mode 100644 index 000000000..e60ad6f28 --- /dev/null +++ b/spec/enterprise/services/captain/tools/copilot/search_conversations_service_spec.rb @@ -0,0 +1,67 @@ +require 'rails_helper' + +RSpec.describe Captain::Tools::Copilot::SearchConversationsService do + let(:account) { create(:account) } + let(:user) { create(:user, role: 'administrator', account: account) } + let(:assistant) { create(:captain_assistant, account: account) } + let(:service) { described_class.new(assistant, user: user) } + + describe '#name' do + it 'returns the correct service name' do + expect(service.name).to eq('search_conversations') + end + end + + describe '#description' do + it 'returns the service description' do + expect(service.description).to eq('Search conversations based on parameters') + end + end + + describe '#parameters' do + it 'returns the correct parameter schema' do + params = service.parameters + expect(params[:type]).to eq('object') + expect(params[:properties]).to include(:contact_id, :status, :priority) + end + end + + describe '#execute' do + let(:contact) { create(:contact, account: account) } + let!(:open_conversation) { create(:conversation, account: account, contact: contact, status: 'open', priority: 'high') } + let!(:resolved_conversation) { create(:conversation, account: account, status: 'resolved', priority: 'low') } + + it 'returns all conversations when no filters are applied' do + result = service.execute({}) + expect(result).to include('Total number of conversations: 2') + expect(result).to include(open_conversation.to_llm_text(include_contact_details: true)) + expect(result).to include(resolved_conversation.to_llm_text(include_contact_details: true)) + end + + it 'filters conversations by status' do + result = service.execute({ 'status' => 'open' }) + expect(result).to include('Total number of conversations: 1') + expect(result).to include(open_conversation.to_llm_text(include_contact_details: true)) + expect(result).not_to include(resolved_conversation.to_llm_text(include_contact_details: true)) + end + + it 'filters conversations by contact_id' do + result = service.execute({ 'contact_id' => contact.id }) + expect(result).to include('Total number of conversations: 1') + expect(result).to include(open_conversation.to_llm_text(include_contact_details: true)) + expect(result).not_to include(resolved_conversation.to_llm_text(include_contact_details: true)) + end + + it 'filters conversations by priority' do + result = service.execute({ 'priority' => 'high' }) + expect(result).to include('Total number of conversations: 1') + expect(result).to include(open_conversation.to_llm_text(include_contact_details: true)) + expect(result).not_to include(resolved_conversation.to_llm_text(include_contact_details: true)) + end + + it 'returns appropriate message when no conversations are found' do + result = service.execute({ 'status' => 'snoozed' }) + expect(result).to eq('No conversations found') + end + end +end diff --git a/spec/enterprise/services/enterprise/conversations/permission_filter_service_spec.rb b/spec/enterprise/services/enterprise/conversations/permission_filter_service_spec.rb index b26832faf..0cfec97eb 100644 --- a/spec/enterprise/services/enterprise/conversations/permission_filter_service_spec.rb +++ b/spec/enterprise/services/enterprise/conversations/permission_filter_service_spec.rb @@ -9,6 +9,8 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do let(:admin) { create(:user, account: account, role: :administrator) } let(:agent) { create(:user, account: account, role: :agent) } let!(:inbox) { create(:inbox, account: account) } + let!(:inbox2) { create(:inbox, account: account) } + let!(:another_inbox_conversation) { create(:conversation, account: account, inbox: inbox2) } # This inbox_member is used to establish the agent's access to the inbox before { create(:inbox_member, user: agent, inbox: inbox) } @@ -25,16 +27,14 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do expect(result).to include(assigned_conversation) expect(result).to include(unassigned_conversation) expect(result).to include(another_assigned_conversation) - expect(result.count).to eq(3) + expect(result.count).to eq(4) end end context 'when user is a regular agent' do it 'returns all conversations in assigned inboxes' do - inbox_ids = agent.inboxes.where(account_id: account.id).pluck(:id) - result = Conversations::PermissionFilterService.new( - account.conversations.where(inbox_id: inbox_ids), + account.conversations, agent, account ).perform @@ -42,6 +42,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do expect(result).to include(assigned_conversation) expect(result).to include(unassigned_conversation) expect(result).to include(another_assigned_conversation) + expect(result).not_to include(another_inbox_conversation) expect(result.count).to eq(3) end end @@ -52,7 +53,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do # Create a new isolated test environment test_account = create(:account) test_inbox = create(:inbox, account: test_account) - + test_inbox2 = create(:inbox, account: test_account) # Create test agent test_agent = create(:user, account: test_account, role: :agent) create(:inbox_member, user: test_agent, inbox: test_inbox) @@ -66,6 +67,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do assigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: test_agent) unassigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: nil) other_assigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: create(:user, account: test_account)) + other_inbox_conversation = create(:conversation, account: test_account, inbox: test_inbox2, assignee: nil) # Run the test result = Conversations::PermissionFilterService.new( @@ -79,6 +81,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do expect(result).to include(assigned_conversation) expect(result).to include(unassigned_conversation) expect(result).to include(other_assigned_conversation) + expect(result).not_to include(other_inbox_conversation) end end @@ -87,6 +90,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do # Create a new isolated test environment test_account = create(:account) test_inbox = create(:inbox, account: test_account) + test_inbox2 = create(:inbox, account: test_account) # Create test agent test_agent = create(:user, account: test_account, role: :agent) @@ -101,6 +105,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do # Create some conversations other_conversation = create(:conversation, account: test_account, inbox: test_inbox) assigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: test_agent) + other_inbox_conversation = create(:conversation, account: test_account, inbox: test_inbox2, assignee: nil) # Run the test result = Conversations::PermissionFilterService.new( @@ -114,6 +119,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do expect(result.first.assignee).to eq(test_agent) expect(result).to include(assigned_conversation) expect(result).not_to include(other_conversation) + expect(result).not_to include(other_inbox_conversation) end end @@ -122,6 +128,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do # Create a new isolated test environment test_account = create(:account) test_inbox = create(:inbox, account: test_account) + test_inbox2 = create(:inbox, account: test_account) # Create test agent test_agent = create(:user, account: test_account, role: :agent) @@ -137,6 +144,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do assigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: test_agent) unassigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: nil) other_assigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: create(:user, account: test_account)) + other_inbox_conversation = create(:conversation, account: test_account, inbox: test_inbox2, assignee: nil) # Run the test result = Conversations::PermissionFilterService.new( @@ -152,6 +160,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do # Should NOT include conversations assigned to others expect(result).not_to include(other_assigned_conversation) + expect(result).not_to include(other_inbox_conversation) end end @@ -160,6 +169,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do # Create a new isolated test environment test_account = create(:account) test_inbox = create(:inbox, account: test_account) + test_inbox2 = create(:inbox, account: test_account) # Create test agent test_agent = create(:user, account: test_account, role: :agent) @@ -176,6 +186,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do assigned_to_agent = create(:conversation, account: test_account, inbox: test_inbox, assignee: test_agent) unassigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: nil) other_assigned_conversation = create(:conversation, account: test_account, inbox: test_inbox, assignee: create(:user, account: test_account)) + other_inbox_conversation = create(:conversation, account: test_account, inbox: test_inbox2, assignee: nil) # Run the test result = Conversations::PermissionFilterService.new( @@ -191,6 +202,7 @@ RSpec.describe Enterprise::Conversations::PermissionFilterService do expect(result).to include(unassigned_conversation) expect(result).to include(assigned_to_agent) expect(result).not_to include(other_assigned_conversation) + expect(result).not_to include(other_inbox_conversation) end end end