From ca83a27e954ea85b1907e8f2d4662840618bf30f Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Mon, 31 Mar 2025 19:30:02 -0700 Subject: [PATCH] chore(refactor): Improve conversation permission filtering (#11166) 1. Add permission filter service to separate permission filtering logic from conversation queries 2. Implement hierarchical permissions with cleaner logic: - conversation_manage gives access to all conversations - conversation_unassigned_manage gives access to unassigned and user's conversations - conversation_participating_manage gives access only to user's conversations --------- Co-authored-by: Pranav --- .gitignore | 3 + .../contacts/conversations_controller.rb | 24 +- .../v1/accounts/conversations_controller.rb | 2 +- app/finders/conversation_finder.rb | 6 + app/services/conversations/filter_service.rb | 22 +- .../permission_filter_service.rb | 17 ++ .../permission_filter_service.rb | 38 +++ .../contacts/conversations_controller_spec.rb | 123 ++++++++ .../permission_filter_service_spec.rb | 197 ++++++++++++ .../filter_service_frontend_alignment_spec.rb | 254 ++++++++++++++++ .../conversations/filter_service_spec.rb | 286 +++--------------- .../permission_filter_service_spec.rb | 47 +++ 12 files changed, 759 insertions(+), 260 deletions(-) create mode 100644 app/services/conversations/permission_filter_service.rb create mode 100644 enterprise/app/services/enterprise/conversations/permission_filter_service.rb create mode 100644 enterprise/spec/controllers/api/v1/accounts/contacts/conversations_controller_spec.rb create mode 100644 enterprise/spec/services/enterprise/conversations/permission_filter_service_spec.rb create mode 100644 spec/services/conversations/filter_service_frontend_alignment_spec.rb create mode 100644 spec/services/conversations/permission_filter_service_spec.rb diff --git a/.gitignore b/.gitignore index 77c4a4740..53deb62a8 100644 --- a/.gitignore +++ b/.gitignore @@ -91,3 +91,6 @@ yarn-debug.log* # Vite uses dotenv and suggests to ignore local-only env files. See # https://vitejs.dev/guide/env-and-mode.html#env-files *.local + +# Claude.ai config file +CLAUDE.md diff --git a/app/controllers/api/v1/accounts/contacts/conversations_controller.rb b/app/controllers/api/v1/accounts/contacts/conversations_controller.rb index de0ac4db9..fda19b8c2 100644 --- a/app/controllers/api/v1/accounts/contacts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/contacts/conversations_controller.rb @@ -1,17 +1,21 @@ class Api::V1::Accounts::Contacts::ConversationsController < Api::V1::Accounts::Contacts::BaseController def index - @conversations = Current.account.conversations.includes( + # Start with all conversations for this contact + conversations = Current.account.conversations.includes( :assignee, :contact, :inbox, :taggings - ).where(inbox_id: inbox_ids, contact_id: @contact.id).order(last_activity_at: :desc).limit(20) - end + ).where(contact_id: @contact.id) - private + # Apply permission-based filtering using the existing service + conversations = Conversations::PermissionFilterService.new( + conversations, + Current.user, + Current.account + ).perform - def inbox_ids - if Current.user.administrator? || Current.user.agent? - Current.user.assigned_inboxes.pluck(:id) - else - [] - end + # 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/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index 138c2bd68..8753918fc 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/conversations_controller.rb @@ -48,7 +48,7 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro end def filter - result = ::Conversations::FilterService.new(params.permit!, current_user).perform + result = ::Conversations::FilterService.new(params.permit!, current_user, current_account).perform @conversations = result[:conversations] @conversations_count = result[:count] rescue CustomExceptions::CustomFilter::InvalidAttribute, diff --git a/app/finders/conversation_finder.rb b/app/finders/conversation_finder.rb index 31f98e384..1694199ba 100644 --- a/app/finders/conversation_finder.rb +++ b/app/finders/conversation_finder.rb @@ -93,6 +93,12 @@ class ConversationFinder def find_all_conversations find_conversation_by_inbox + # Apply permission-based filtering + @conversations = Conversations::PermissionFilterService.new( + @conversations, + current_user, + current_account + ).perform filter_by_conversation_type if params[:conversation_type] @conversations end diff --git a/app/services/conversations/filter_service.rb b/app/services/conversations/filter_service.rb index d5156b531..21fe3c716 100644 --- a/app/services/conversations/filter_service.rb +++ b/app/services/conversations/filter_service.rb @@ -1,8 +1,8 @@ class Conversations::FilterService < FilterService ATTRIBUTE_MODEL = 'conversation_attribute'.freeze - def initialize(params, user, filter_account = nil) - @account = filter_account || Current.account + def initialize(params, user, account) + @account = account super(params, user) end @@ -24,9 +24,25 @@ class Conversations::FilterService < FilterService end def base_relation - @account.conversations.includes( + conversations = @account.conversations.includes( :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, + @account + ).perform end def current_page diff --git a/app/services/conversations/permission_filter_service.rb b/app/services/conversations/permission_filter_service.rb new file mode 100644 index 000000000..a8561dbdf --- /dev/null +++ b/app/services/conversations/permission_filter_service.rb @@ -0,0 +1,17 @@ +class Conversations::PermissionFilterService + attr_reader :conversations, :user, :account + + def initialize(conversations, user, account) + @conversations = conversations + @user = user + @account = account + end + + def perform + # The base implementation simply returns all conversations + # Enterprise edition extends this with permission-based filtering + conversations + end +end + +Conversations::PermissionFilterService.prepend_mod_with('Conversations::PermissionFilterService') diff --git a/enterprise/app/services/enterprise/conversations/permission_filter_service.rb b/enterprise/app/services/enterprise/conversations/permission_filter_service.rb new file mode 100644 index 000000000..5da65ffa6 --- /dev/null +++ b/enterprise/app/services/enterprise/conversations/permission_filter_service.rb @@ -0,0 +1,38 @@ +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 + + # 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) + end + + private + + def filter_by_permissions(permissions) + # Permission-based filtering with hierarchy + # conversation_manage > conversation_unassigned_manage > conversation_participating_manage + if permissions.include?('conversation_manage') + conversations + elsif permissions.include?('conversation_unassigned_manage') + filter_unassigned_and_mine + elsif permissions.include?('conversation_participating_manage') + conversations.assigned_to(user) + else + Conversation.none + end + end + + def filter_unassigned_and_mine + mine = conversations.assigned_to(user) + unassigned = conversations.unassigned + + Conversation.from("(#{mine.to_sql} UNION #{unassigned.to_sql}) as conversations") + .where(account_id: account.id) + end +end diff --git a/enterprise/spec/controllers/api/v1/accounts/contacts/conversations_controller_spec.rb b/enterprise/spec/controllers/api/v1/accounts/contacts/conversations_controller_spec.rb new file mode 100644 index 000000000..a5f200013 --- /dev/null +++ b/enterprise/spec/controllers/api/v1/accounts/contacts/conversations_controller_spec.rb @@ -0,0 +1,123 @@ +require 'rails_helper' + +RSpec.describe '/api/v1/accounts/{account.id}/contacts/:id/conversations enterprise', type: :request do + let(:account) { create(:account) } + let(:contact) { create(:contact, account: account) } + let(:inbox) { create(:inbox, account: account) } + let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: inbox) } + + describe 'GET /api/v1/accounts/{account.id}/contacts/:id/conversations with custom role permissions' do + context 'with user having custom role' do + let(:agent_with_custom_role) { create(:user, account: account, role: :agent) } + let(:custom_role) { create(:custom_role, account: account) } + + before do + create(:inbox_member, user: agent_with_custom_role, inbox: inbox) + end + + context 'with conversation_participating_manage permission' do + let(:assigned_conversation) do + create(:conversation, account: account, inbox: inbox, contact: contact, + contact_inbox: contact_inbox, assignee: agent_with_custom_role) + end + + before do + # Create a conversation assigned to this agent + assigned_conversation + + # Create another conversation that shouldn't be visible + create(:conversation, account: account, inbox: inbox, contact: contact, + contact_inbox: contact_inbox, assignee: create(:user, account: account, role: :agent)) + + # Set up permissions + custom_role.update!(permissions: %w[conversation_participating_manage]) + + # Associate the custom role with the agent + account_user = AccountUser.find_by(user: agent_with_custom_role, account: account) + account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'returns only conversations assigned to the agent' do + get "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/conversations", + headers: agent_with_custom_role.create_new_auth_token + + expect(response).to have_http_status(:success) + json_response = response.parsed_body + + # Should only return the conversation assigned to this agent + expect(json_response['payload'].length).to eq 1 + expect(json_response['payload'][0]['id']).to eq assigned_conversation.display_id + end + end + + context 'with conversation_unassigned_manage permission' do + let(:unassigned_conversation) do + create(:conversation, account: account, inbox: inbox, contact: contact, + contact_inbox: contact_inbox, assignee: nil) + end + + let(:assigned_conversation) do + create(:conversation, account: account, inbox: inbox, contact: contact, + contact_inbox: contact_inbox, assignee: agent_with_custom_role) + end + + before do + # Create the conversations + unassigned_conversation + assigned_conversation + create(:conversation, account: account, inbox: inbox, contact: contact, + contact_inbox: contact_inbox, assignee: create(:user, account: account, role: :agent)) + + # Set up permissions + custom_role.update!(permissions: %w[conversation_unassigned_manage]) + + # Associate the custom role with the agent + account_user = AccountUser.find_by(user: agent_with_custom_role, account: account) + account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'returns unassigned conversations AND conversations assigned to the agent' do + get "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/conversations", + headers: agent_with_custom_role.create_new_auth_token + + expect(response).to have_http_status(:success) + json_response = response.parsed_body + + # Should return both unassigned and assigned to this agent conversations + expect(json_response['payload'].length).to eq 2 + conversation_ids = json_response['payload'].pluck('id') + expect(conversation_ids).to include(unassigned_conversation.display_id) + expect(conversation_ids).to include(assigned_conversation.display_id) + end + end + + context 'with conversation_manage permission' do + before do + # Create multiple conversations + 3.times do + create(:conversation, account: account, inbox: inbox, contact: contact, + contact_inbox: contact_inbox) + end + + # Set up permissions + custom_role.update!(permissions: %w[conversation_manage]) + + # Associate the custom role with the agent + account_user = AccountUser.find_by(user: agent_with_custom_role, account: account) + account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'returns all conversations' do + get "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/conversations", + headers: agent_with_custom_role.create_new_auth_token + + expect(response).to have_http_status(:success) + json_response = response.parsed_body + + # Should return all conversations in this inbox + expect(json_response['payload'].length).to eq 3 + end + end + end + end +end diff --git a/enterprise/spec/services/enterprise/conversations/permission_filter_service_spec.rb b/enterprise/spec/services/enterprise/conversations/permission_filter_service_spec.rb new file mode 100644 index 000000000..b26832faf --- /dev/null +++ b/enterprise/spec/services/enterprise/conversations/permission_filter_service_spec.rb @@ -0,0 +1,197 @@ +require 'rails_helper' + +RSpec.describe Enterprise::Conversations::PermissionFilterService do + let(:account) { create(:account) } + # Create conversations with different states + let!(:assigned_conversation) { create(:conversation, account: account, inbox: inbox, assignee: agent) } + let!(:unassigned_conversation) { create(:conversation, account: account, inbox: inbox, assignee: nil) } + let!(:another_assigned_conversation) { create(:conversation, account: account, inbox: inbox, assignee: create(:user, account: account)) } + let(:admin) { create(:user, account: account, role: :administrator) } + let(:agent) { create(:user, account: account, role: :agent) } + let!(:inbox) { create(:inbox, account: account) } + + # This inbox_member is used to establish the agent's access to the inbox + before { create(:inbox_member, user: agent, inbox: inbox) } + + describe '#perform' do + context 'when user is an administrator' do + it 'returns all conversations' do + result = Conversations::PermissionFilterService.new( + account.conversations, + admin, + account + ).perform + + 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) + 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), + agent, + account + ).perform + + 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) + end + end + + context 'when user has conversation_manage permission' do + # Test with a new clean state for each test case + it 'returns all conversations' do + # Create a new isolated test environment + test_account = create(:account) + test_inbox = 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) + + # Create custom role with conversation_manage permission + test_custom_role = create(:custom_role, account: test_account, permissions: ['conversation_manage']) + account_user = AccountUser.find_by(user: test_agent, account: test_account) + account_user.update(role: :agent, custom_role: test_custom_role) + + # Create some conversations + 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)) + + # Run the test + result = Conversations::PermissionFilterService.new( + test_account.conversations, + test_agent, + test_account + ).perform + + # Should have access to all conversations + expect(result.count).to eq(3) + expect(result).to include(assigned_conversation) + expect(result).to include(unassigned_conversation) + expect(result).to include(other_assigned_conversation) + end + end + + context 'when user has conversation_participating_manage permission' do + it 'returns only conversations assigned to the agent' do + # Create a new isolated test environment + test_account = create(:account) + test_inbox = 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) + + # Create a custom role with only the conversation_participating_manage permission + test_custom_role = create(:custom_role, account: test_account, permissions: %w[conversation_participating_manage]) + + account_user = AccountUser.find_by(user: test_agent, account: test_account) + account_user.update(role: :agent, custom_role: test_custom_role) + + # 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) + + # Run the test + result = Conversations::PermissionFilterService.new( + test_account.conversations, + test_agent, + test_account + ).perform + + # Should only see conversations assigned to this agent + expect(result.count).to eq(1) + expect(result.first.assignee).to eq(test_agent) + expect(result).to include(assigned_conversation) + expect(result).not_to include(other_conversation) + end + end + + context 'when user has conversation_unassigned_manage permission' do + it 'returns unassigned conversations AND mine' do + # Create a new isolated test environment + test_account = create(:account) + test_inbox = 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) + + # Create a custom role with only the conversation_unassigned_manage permission + test_custom_role = create(:custom_role, account: test_account, permissions: %w[conversation_unassigned_manage]) + + account_user = AccountUser.find_by(user: test_agent, account: test_account) + account_user.update(role: :agent, custom_role: test_custom_role) + + # Create some conversations + 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)) + + # Run the test + result = Conversations::PermissionFilterService.new( + test_account.conversations, + test_agent, + test_account + ).perform + + # Should see unassigned conversations AND conversations assigned to this agent + expect(result.count).to eq(2) + expect(result).to include(unassigned_conversation) + expect(result).to include(assigned_conversation) + + # Should NOT include conversations assigned to others + expect(result).not_to include(other_assigned_conversation) + end + end + + context 'when user has both participating and unassigned permissions (hierarchical test)' do + it 'gives higher priority to unassigned_manage over participating_manage' do + # Create a new isolated test environment + test_account = create(:account) + test_inbox = 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) + + # Create a custom role with both participating and unassigned permissions + permissions = %w[conversation_participating_manage conversation_unassigned_manage] + test_custom_role = create(:custom_role, account: test_account, permissions: permissions) + + account_user = AccountUser.find_by(user: test_agent, account: test_account) + account_user.update(role: :agent, custom_role: test_custom_role) + + # Create some conversations + 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)) + + # Run the test + result = Conversations::PermissionFilterService.new( + test_account.conversations, + test_agent, + test_account + ).perform + + # Should behave the same as conversation_unassigned_manage test + # - Show both unassigned and assigned to this agent + # - Do not show conversations assigned to others + expect(result.count).to eq(2) + expect(result).to include(unassigned_conversation) + expect(result).to include(assigned_to_agent) + expect(result).not_to include(other_assigned_conversation) + end + end + end +end diff --git a/spec/services/conversations/filter_service_frontend_alignment_spec.rb b/spec/services/conversations/filter_service_frontend_alignment_spec.rb new file mode 100644 index 000000000..a34ed9643 --- /dev/null +++ b/spec/services/conversations/filter_service_frontend_alignment_spec.rb @@ -0,0 +1,254 @@ +## This spec is to ensure alignment between frontend and backend filters +# ref: https://github.com/chatwoot/chatwoot/pull/11111 + +require 'rails_helper' + +describe Conversations::FilterService do + describe 'Frontend alignment tests' do + let!(:account) { create(:account) } + let!(:user_1) { create(:user, account: account, role: :administrator) } + let!(:inbox) { create(:inbox, account: account) } + let!(:params) { { payload: [], page: 1 } } + + before do + account.conversations.destroy_all + + # Create inbox membership + create(:inbox_member, user: user_1, inbox: inbox) + + # Create custom attribute definition for conversation_type + create(:custom_attribute_definition, + attribute_key: 'conversation_type', + account: account, + attribute_model: 'conversation_attribute', + attribute_display_type: 'list', + attribute_values: %w[platinum silver gold regular]) + end + + context 'with A AND B OR C filter chain' do + let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } + let(:filter_payload) do + [ + { + attribute_key: 'status', + filter_operator: 'equal_to', + values: ['open'], + query_operator: 'AND' + }.with_indifferent_access, + { + attribute_key: 'priority', + filter_operator: 'equal_to', + values: ['urgent'], + query_operator: 'OR' + }.with_indifferent_access, + { + attribute_key: 'display_id', + filter_operator: 'equal_to', + values: ['12345'], + query_operator: nil + }.with_indifferent_access + ] + end + + before do + conversation.update!( + status: 'open', + priority: 'urgent', + display_id: '12345', + additional_attributes: { 'browser_language': 'en' } + ) + end + + it 'matches when all conditions are true' do + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + + it 'matches when first condition is false but third is true' do + conversation.update!(status: 'resolved', priority: 'urgent', display_id: '12345') + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + + it 'matches when first and second condition is false but third is true' do + conversation.update!(status: 'resolved', priority: 'low', display_id: '12345') + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + + it 'does not match when all conditions are false' do + conversation.update!(status: 'resolved', priority: 'low', display_id: '67890') + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 0 + end + end + + context 'with A OR B AND C filter chain' do + let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } + let(:filter_payload) do + [ + { + attribute_key: 'status', + filter_operator: 'equal_to', + values: ['open'], + query_operator: 'OR' + }.with_indifferent_access, + { + attribute_key: 'priority', + filter_operator: 'equal_to', + values: ['low'], + query_operator: 'AND' + }.with_indifferent_access, + { + attribute_key: 'display_id', + filter_operator: 'equal_to', + values: ['67890'], + query_operator: nil + }.with_indifferent_access + ] + end + + before do + conversation.update!( + status: 'open', + priority: 'urgent', + display_id: '12345', + additional_attributes: { 'browser_language': 'en' } + ) + end + + it 'matches when first condition is true' do + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + + it 'matches when second and third conditions are true' do + conversation.update!(status: 'resolved', priority: 'low', display_id: '67890') + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + end + + context 'with complex filter chain A AND B OR C AND D' do + let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } + let(:filter_payload) do + [ + { + attribute_key: 'status', + filter_operator: 'equal_to', + values: ['open'], + query_operator: 'AND' + }.with_indifferent_access, + { + attribute_key: 'priority', + filter_operator: 'equal_to', + values: ['urgent'], + query_operator: 'OR' + }.with_indifferent_access, + { + attribute_key: 'display_id', + filter_operator: 'equal_to', + values: ['67890'], + query_operator: 'AND' + }.with_indifferent_access, + { + attribute_key: 'browser_language', + filter_operator: 'equal_to', + values: ['tr'], + query_operator: nil + }.with_indifferent_access + ] + end + + before do + conversation.update!( + status: 'open', + priority: 'urgent', + display_id: '12345', + additional_attributes: { 'browser_language': 'en' }, + custom_attributes: { conversation_type: 'platinum' } + ) + end + + it 'matches when first two conditions are true' do + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + + it 'matches when last two conditions are true' do + conversation.update!( + status: 'resolved', + priority: 'low', + display_id: '67890', + additional_attributes: { 'browser_language': 'tr' } + ) + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + end + + context 'with mixed operators filter chain' do + let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } + let(:filter_payload) do + [ + { + attribute_key: 'status', + filter_operator: 'equal_to', + values: ['open'], + query_operator: 'AND' + }.with_indifferent_access, + { + attribute_key: 'priority', + filter_operator: 'equal_to', + values: ['urgent'], + query_operator: 'OR' + }.with_indifferent_access, + { + attribute_key: 'display_id', + filter_operator: 'equal_to', + values: ['67890'], + query_operator: 'AND' + }.with_indifferent_access, + { + attribute_key: 'conversation_type', + filter_operator: 'equal_to', + values: ['platinum'], + custom_attribute_type: '', + query_operator: nil + }.with_indifferent_access + ] + end + + before do + conversation.update!( + status: 'open', + priority: 'urgent', + display_id: '12345', + additional_attributes: { 'browser_language': 'en' }, + custom_attributes: { conversation_type: 'platinum' } + ) + end + + it 'matches when all conditions in the chain are true' do + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + + it 'does not match when the last condition is false' do + conversation.update!(custom_attributes: { conversation_type: 'silver' }) + params[:payload] = filter_payload + result = described_class.new(params, user_1, account).perform + expect(result[:conversations].length).to be 1 + end + end + end +end diff --git a/spec/services/conversations/filter_service_spec.rb b/spec/services/conversations/filter_service_spec.rb index 5afa92e77..7bfa5875d 100644 --- a/spec/services/conversations/filter_service_spec.rb +++ b/spec/services/conversations/filter_service_spec.rb @@ -23,7 +23,6 @@ describe Conversations::FilterService do before do create(:inbox_member, user: user_1, inbox: inbox) create(:inbox_member, user: user_2, inbox: inbox) - Current.account = account en_conversation_1.update!(custom_attributes: { conversation_additional_information: 'test custom data' }) en_conversation_2.update!(custom_attributes: { conversation_additional_information: 'test custom data', conversation_type: 'platinum' }) @@ -72,7 +71,7 @@ describe Conversations::FilterService do it 'filter conversations by additional_attributes and status' do params[:payload] = payload - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform conversations = Conversation.where("additional_attributes ->> 'browser_language' IN (?) AND status IN (?)", ['en'], [1, 2]) expect(result[:count][:all_count]).to be conversations.count end @@ -88,7 +87,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to eq 1 expect(result[:conversations][0][:id]).to eq conversation.id end @@ -107,7 +106,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to eq 2 expect(result[:conversations].pluck(:id)).to include(high_priority.id, urgent_priority.id) end @@ -127,7 +126,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform # Only include conversations with medium and low priority, excluding high and urgent expect(result[:conversations].length).to eq 2 @@ -137,7 +136,7 @@ describe Conversations::FilterService do it 'filter conversations by additional_attributes and status with pagination' do params[:payload] = payload params[:page] = 2 - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform conversations = Conversation.where("additional_attributes ->> 'browser_language' IN (?) AND status IN (?)", ['en'], [1, 2]) expect(result[:count][:all_count]).to be conversations.count end @@ -156,7 +155,7 @@ describe Conversations::FilterService do create(:conversation, account: account, inbox: inbox, assignee: user_1, campaign_id: campaign_1.id, status: 'pending', additional_attributes: { 'browser_language': 'tr' }) - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:count][:all_count]).to be 2 end @@ -174,7 +173,7 @@ describe Conversations::FilterService do create(:conversation, account: account, inbox: inbox, assignee: user_1, campaign_id: campaign_1.id, status: 'pending', additional_attributes: { 'browser_language': 'tr' }) - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:count][:all_count]).to be 1 expect(result[:conversations].first.additional_attributes['browser_language']).to eq 'fr' @@ -184,7 +183,7 @@ describe Conversations::FilterService do payload = [{ attribute_key: 'conversation_type', filter_operator: 'not_equal_to', values: 'platinum', query_operator: nil, custom_attribute_type: 'conversation_attribute' }.with_indifferent_access] params[:payload] = payload - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform conversations = Conversation.where( "custom_attributes ->> 'conversation_type' NOT IN (?) OR custom_attributes ->> 'conversation_type' IS NULL", ['platinum'] ) @@ -213,7 +212,7 @@ describe Conversations::FilterService do query_operator: nil }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:count][:all_count]).to be 1 end @@ -237,7 +236,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:count][:all_count]).to be 2 expect(result[:conversations].pluck(:campaign_id).sort).to eq [campaign_2.id, campaign_1.id].sort @@ -264,7 +263,7 @@ describe Conversations::FilterService do }.with_indifferent_access ] - expect { filter_service.new(params, user_1).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator) + expect { filter_service.new(params, user_1, account).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator) end end end @@ -296,7 +295,7 @@ describe Conversations::FilterService do query_operator: nil }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be 1 expect(result[:conversations][0][:id]).to be user_2_assigned_conversation.id end @@ -324,7 +323,7 @@ describe Conversations::FilterService do query_operator: nil }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be 1 expect(result[:conversations][0][:id]).to be user_2_assigned_conversation.id end @@ -346,7 +345,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be 1 end @@ -367,7 +366,7 @@ describe Conversations::FilterService do custom_attribute_type: nil }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be 1 end @@ -393,7 +392,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be 1 end end @@ -413,7 +412,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expected_count = Conversation.where('created_at > ?', DateTime.parse('2022-01-20')).count expect(result[:conversations].length).to be expected_count end @@ -435,7 +434,7 @@ describe Conversations::FilterService do custom_attribute_type: '' }.with_indifferent_access ] - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expected_count = Conversation.where("created_at > ? AND custom_attributes->>'conversation_type' = ?", DateTime.parse('2022-01-20'), 'platinum').count @@ -471,7 +470,7 @@ describe Conversations::FilterService do expected_count = Conversation.where("last_activity_at < ? AND custom_attributes->>'conversation_type' = ?", (Time.zone.today - 3.days), 'platinum').count - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be expected_count end @@ -488,7 +487,7 @@ describe Conversations::FilterService do expected_count = Conversation.where('last_activity_at < ?', (Time.zone.today - 2.days)).count - result = filter_service.new(params, user_1).perform + result = filter_service.new(params, user_1, account).perform expect(result[:conversations].length).to be expected_count end end @@ -522,240 +521,35 @@ describe Conversations::FilterService do end end - describe 'Frontend alignment tests' do + describe '#base_relation' do let!(:account) { create(:account) } - let!(:user_1) { create(:user, account: account) } - let!(:inbox) { create(:inbox, account: account) } + let!(:user_1) { create(:user, account: account, role: :agent) } + let!(:admin) { create(:user, account: account, role: :administrator) } + let!(:inbox_1) { create(:inbox, account: account) } + let!(:inbox_2) { create(:inbox, account: account) } let!(:params) { { payload: [], page: 1 } } before do account.conversations.destroy_all + + # Make user_1 a regular agent with access to inbox_1 only + create(:inbox_member, user: user_1, inbox: inbox_1) + + # Create conversations in both inboxes + create(:conversation, account: account, inbox: inbox_1) + create(:conversation, account: account, inbox: inbox_2) end - context 'with A AND B OR C filter chain' do - let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } - let(:filter_payload) do - [ - { - attribute_key: 'status', - filter_operator: 'equal_to', - values: ['open'], - query_operator: 'AND' - }.with_indifferent_access, - { - attribute_key: 'priority', - filter_operator: 'equal_to', - values: ['urgent'], - query_operator: 'OR' - }.with_indifferent_access, - { - attribute_key: 'display_id', - filter_operator: 'equal_to', - values: ['12345'], - query_operator: nil - }.with_indifferent_access - ] - end - - before do - conversation.update!( - status: 'open', - priority: 'urgent', - display_id: '12345', - additional_attributes: { 'browser_language': 'en' } - ) - end - - it 'matches when all conditions are true' do - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - - it 'matches when first condition is false but third is true' do - conversation.update!(status: 'resolved', priority: 'urgent', display_id: '12345') - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - - it 'matches when first and second condition is false but third is true' do - conversation.update!(status: 'resolved', priority: 'low', display_id: '12345') - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - - it 'does not match when all conditions are false' do - conversation.update!(status: 'resolved', priority: 'low', display_id: '67890') - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 0 - end + it 'returns all conversations for administrators, even for inboxes they are not members of' do + service = filter_service.new(params, admin, account) + result = service.perform + expect(result[:conversations].count).to eq 2 end - context 'with A OR B AND C filter chain' do - let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } - let(:filter_payload) do - [ - { - attribute_key: 'status', - filter_operator: 'equal_to', - values: ['open'], - query_operator: 'OR' - }.with_indifferent_access, - { - attribute_key: 'priority', - filter_operator: 'equal_to', - values: ['low'], - query_operator: 'AND' - }.with_indifferent_access, - { - attribute_key: 'display_id', - filter_operator: 'equal_to', - values: ['67890'], - query_operator: nil - }.with_indifferent_access - ] - end - - before do - conversation.update!( - status: 'open', - priority: 'urgent', - display_id: '12345', - additional_attributes: { 'browser_language': 'en' } - ) - end - - it 'matches when first condition is true' do - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - - it 'matches when second and third conditions are true' do - conversation.update!(status: 'resolved', priority: 'low', display_id: '67890') - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - end - - context 'with complex filter chain A AND B OR C AND D' do - let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } - let(:filter_payload) do - [ - { - attribute_key: 'status', - filter_operator: 'equal_to', - values: ['open'], - query_operator: 'AND' - }.with_indifferent_access, - { - attribute_key: 'priority', - filter_operator: 'equal_to', - values: ['urgent'], - query_operator: 'OR' - }.with_indifferent_access, - { - attribute_key: 'display_id', - filter_operator: 'equal_to', - values: ['67890'], - query_operator: 'AND' - }.with_indifferent_access, - { - attribute_key: 'browser_language', - filter_operator: 'equal_to', - values: ['tr'], - query_operator: nil - }.with_indifferent_access - ] - end - - before do - conversation.update!( - status: 'open', - priority: 'urgent', - display_id: '12345', - additional_attributes: { 'browser_language': 'en' }, - custom_attributes: { conversation_type: 'platinum' } - ) - end - - it 'matches when first two conditions are true' do - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - - it 'matches when last two conditions are true' do - conversation.update!( - status: 'resolved', - priority: 'low', - display_id: '67890', - additional_attributes: { 'browser_language': 'tr' } - ) - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - end - - context 'with mixed operators filter chain' do - let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) } - let(:filter_payload) do - [ - { - attribute_key: 'status', - filter_operator: 'equal_to', - values: ['open'], - query_operator: 'AND' - }.with_indifferent_access, - { - attribute_key: 'priority', - filter_operator: 'equal_to', - values: ['urgent'], - query_operator: 'OR' - }.with_indifferent_access, - { - attribute_key: 'display_id', - filter_operator: 'equal_to', - values: ['67890'], - query_operator: 'AND' - }.with_indifferent_access, - { - attribute_key: 'conversation_type', - filter_operator: 'equal_to', - values: ['platinum'], - custom_attribute_type: '', - query_operator: nil - }.with_indifferent_access - ] - end - - before do - conversation.update!( - status: 'open', - priority: 'urgent', - display_id: '12345', - additional_attributes: { 'browser_language': 'en' }, - custom_attributes: { conversation_type: 'platinum' } - ) - end - - it 'matches when all conditions in the chain are true' do - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end - - it 'does not match when the last condition is false' do - conversation.update!(custom_attributes: { conversation_type: 'silver' }) - params[:payload] = filter_payload - result = described_class.new(params, user_1).perform - expect(result[:conversations].length).to be 1 - end + it 'filters conversations by inbox membership for non-administrators' do + service = filter_service.new(params, user_1, account) + result = service.perform + expect(result[:conversations].count).to eq 1 end end end diff --git a/spec/services/conversations/permission_filter_service_spec.rb b/spec/services/conversations/permission_filter_service_spec.rb new file mode 100644 index 000000000..194387afc --- /dev/null +++ b/spec/services/conversations/permission_filter_service_spec.rb @@ -0,0 +1,47 @@ +require 'rails_helper' + +RSpec.describe Conversations::PermissionFilterService do + let(:account) { create(:account) } + let!(:conversation) { create(:conversation, account: account, inbox: inbox) } + let!(:another_conversation) { create(:conversation, account: account, inbox: inbox) } + let(:admin) { create(:user, account: account, role: :administrator) } + let(:agent) { create(:user, account: account, role: :agent) } + let!(:inbox) { create(:inbox, account: account) } + + # This inbox_member is used to establish the agent's access to the inbox + before { create(:inbox_member, user: agent, inbox: inbox) } + + describe '#perform' do + context 'when user is an administrator' do + it 'returns all conversations' do + result = described_class.new( + account.conversations, + admin, + account + ).perform + + expect(result).to include(conversation) + expect(result).to include(another_conversation) + expect(result.count).to eq(2) + end + end + + context 'when user is an agent' do + it 'returns all conversations with no further filtering' do + inbox_ids = agent.inboxes.where(account_id: account.id).pluck(:id) + + # The base implementation returns all conversations + # expecting the caller to filter by assigned inboxes + result = described_class.new( + account.conversations.where(inbox_id: inbox_ids), + agent, + account + ).perform + + expect(result).to include(conversation) + expect(result).to include(another_conversation) + expect(result.count).to eq(2) + end + end + end +end