diff --git a/app/builders/contact_inbox_builder.rb b/app/builders/contact_inbox_builder.rb index ffa45db2e..4d51700a8 100644 --- a/app/builders/contact_inbox_builder.rb +++ b/app/builders/contact_inbox_builder.rb @@ -12,50 +12,11 @@ class ContactInboxBuilder private def generate_source_id - case @inbox.channel_type - when 'Channel::TwilioSms' - twilio_source_id - when 'Channel::Whatsapp' - wa_source_id - when 'Channel::Email' - email_source_id - when 'Channel::Sms' - phone_source_id - when 'Channel::Api', 'Channel::WebWidget' - SecureRandom.uuid - else - raise "Unsupported operation for this channel: #{@inbox.channel_type}" - end - end - - def email_source_id - raise ActionController::ParameterMissing, 'contact email' unless @contact.email - - @contact.email - end - - def phone_source_id - raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number - - @contact.phone_number - end - - def wa_source_id - raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number - - # whatsapp doesn't want the + in e164 format - @contact.phone_number.delete('+').to_s - end - - def twilio_source_id - raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number - - case @inbox.channel.medium - when 'sms' - @contact.phone_number - when 'whatsapp' - "whatsapp:#{@contact.phone_number}" - end + ContactInbox::SourceIdService.new( + contact: @contact, + channel_type: @inbox.channel_type, + medium: @inbox.channel.try(:medium) + ).generate end def create_contact_inbox @@ -91,7 +52,7 @@ class ContactInboxBuilder def new_source_id if @inbox.whatsapp? || @inbox.sms? || @inbox.twilio? - "whatsapp:#{@source_id}#{rand(100)}" + "#{@source_id}#{rand(100)}" else "#{rand(10)}#{@source_id}" end diff --git a/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb b/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb index d985c8a73..bde9a4f0c 100644 --- a/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb +++ b/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb @@ -9,6 +9,8 @@ class Api::V1::Accounts::Contacts::ContactInboxesController < Api::V1::Accounts: source_id: params[:source_id], hmac_verified: hmac_verified? ).perform + rescue ArgumentError => e + render json: { error: e.message }, status: :unprocessable_entity end private diff --git a/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js b/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js index 57e819245..45abef1e0 100644 --- a/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js +++ b/app/javascript/dashboard/components-next/NewConversation/helpers/composeConversationHelper.js @@ -25,6 +25,11 @@ export const generateLabelForContactableInboxesList = ({ channelType === INBOX_TYPES.TWILIO || channelType === INBOX_TYPES.WHATSAPP ) { + // Handled separately for Twilio Inbox where phone number is not mandatory. + // You can send message to a contact with Messaging Service Id. + if (!phoneNumber) { + return name; + } return `${name} (${phoneNumber})`; } return name; diff --git a/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js b/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js index 3de1fad0d..37ae5fe29 100644 --- a/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js +++ b/app/javascript/dashboard/components-next/NewConversation/helpers/specs/composeConversationHelper.spec.js @@ -8,8 +8,8 @@ vi.mock('dashboard/api/contacts'); describe('composeConversationHelper', () => { describe('generateLabelForContactableInboxesList', () => { const contact = { - name: 'John Doe', - email: 'john@example.com', + name: 'Priority Inbox', + email: 'hello@example.com', phoneNumber: '+1234567890', }; @@ -19,7 +19,7 @@ describe('composeConversationHelper', () => { ...contact, channelType: INBOX_TYPES.EMAIL, }) - ).toBe('John Doe (john@example.com)'); + ).toBe('Priority Inbox (hello@example.com)'); }); it('generates label for twilio inbox', () => { @@ -28,7 +28,14 @@ describe('composeConversationHelper', () => { ...contact, channelType: INBOX_TYPES.TWILIO, }) - ).toBe('John Doe (+1234567890)'); + ).toBe('Priority Inbox (+1234567890)'); + + expect( + helpers.generateLabelForContactableInboxesList({ + name: 'Priority Inbox', + channelType: INBOX_TYPES.TWILIO, + }) + ).toBe('Priority Inbox'); }); it('generates label for whatsapp inbox', () => { @@ -37,7 +44,7 @@ describe('composeConversationHelper', () => { ...contact, channelType: INBOX_TYPES.WHATSAPP, }) - ).toBe('John Doe (+1234567890)'); + ).toBe('Priority Inbox (+1234567890)'); }); it('generates label for other inbox types', () => { @@ -46,7 +53,7 @@ describe('composeConversationHelper', () => { ...contact, channelType: 'Channel::Api', }) - ).toBe('John Doe'); + ).toBe('Priority Inbox'); }); }); diff --git a/app/services/base/send_on_channel_service.rb b/app/services/base/send_on_channel_service.rb index 298f60962..58f225ef0 100644 --- a/app/services/base/send_on_channel_service.rb +++ b/app/services/base/send_on_channel_service.rb @@ -12,8 +12,10 @@ class Base::SendOnChannelService def perform validate_target_channel + return unless outgoing_message? return if invalid_message? + return if invalid_source_id? perform_reply end @@ -49,6 +51,29 @@ class Base::SendOnChannelService message.private? || outgoing_message_originated_from_channel? end + def invalid_source_id? + return false unless channels_to_validate? + + return false if contact_inbox.source_id == expected_source_id + + message.update!(status: :failed, external_error: I18n.t('errors.channel_service.invalid_source_id')) + true + end + + def expected_source_id + ContactInbox::SourceIdService.new( + contact: contact, + channel_type: inbox.channel_type, + medium: inbox.channel.try(:medium) + ).generate + rescue ArgumentError + nil + end + + def channels_to_validate? + inbox.sms? || inbox.whatsapp? || inbox.email? || inbox.twilio? + end + def validate_target_channel raise 'Invalid channel service was called' if inbox.channel.class != channel_class end diff --git a/app/services/contact_inbox/source_id_service.rb b/app/services/contact_inbox/source_id_service.rb new file mode 100644 index 000000000..995bc37a9 --- /dev/null +++ b/app/services/contact_inbox/source_id_service.rb @@ -0,0 +1,55 @@ +class ContactInbox::SourceIdService + pattr_initialize [:contact!, :channel_type!, { medium: nil }] + + def generate + case channel_type + when 'Channel::TwilioSms' + twilio_source_id + when 'Channel::Whatsapp' + wa_source_id + when 'Channel::Email' + email_source_id + when 'Channel::Sms' + phone_source_id + when 'Channel::Api', 'Channel::WebWidget' + SecureRandom.uuid + else + raise ArgumentError, "Unsupported operation for this channel: #{channel_type}" + end + end + + private + + def email_source_id + raise ArgumentError, 'contact email required' unless contact.email + + contact.email + end + + def phone_source_id + raise ArgumentError, 'contact phone number required' unless contact.phone_number + + contact.phone_number + end + + def wa_source_id + raise ArgumentError, 'contact phone number required' unless contact.phone_number + + # whatsapp doesn't want the + in e164 format + contact.phone_number.delete('+').to_s + end + + def twilio_source_id + raise ArgumentError, 'contact phone number required' unless contact.phone_number + raise ArgumentError, 'medium required for Twilio channel' if medium.blank? + + case medium + when 'sms' + contact.phone_number + when 'whatsapp' + "whatsapp:#{contact.phone_number}" + else + raise ArgumentError, "Unsupported Twilio medium: #{medium}" + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 4a66a4bc2..27ba48cba 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -67,6 +67,9 @@ en: invalid_message_type: 'Invalid message type. Action not permitted' slack: invalid_channel_id: 'Invalid slack channel. Please try again' + channel_service: + invalid_source_id: "This conversation may have originally belonged to a different contact but is now showing here due to a merge or update. You won't be able to continue this conversation. Please create a new conversation to proceed." + inboxes: imap: socket_error: Please check the network connection, IMAP address and try again. diff --git a/spec/builders/contact_inbox_builder_spec.rb b/spec/builders/contact_inbox_builder_spec.rb index f0e3deb31..b6519f130 100644 --- a/spec/builders/contact_inbox_builder_spec.rb +++ b/spec/builders/contact_inbox_builder_spec.rb @@ -59,7 +59,7 @@ describe ContactInboxBuilder do contact: contact, inbox: twilio_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -117,7 +117,7 @@ describe ContactInboxBuilder do contact: contact, inbox: twilio_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -174,7 +174,7 @@ describe ContactInboxBuilder do contact: contact, inbox: whatsapp_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -232,7 +232,7 @@ describe ContactInboxBuilder do contact: contact, inbox: sms_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact phone number') + end.to raise_error(ArgumentError, 'contact phone number required') end end @@ -290,7 +290,7 @@ describe ContactInboxBuilder do contact: contact, inbox: email_inbox ).perform - end.to raise_error(ActionController::ParameterMissing, 'param is missing or the value is empty: contact email') + end.to raise_error(ArgumentError, 'contact email required') end end diff --git a/spec/services/contact_inbox/source_id_service_spec.rb b/spec/services/contact_inbox/source_id_service_spec.rb new file mode 100644 index 000000000..94fe8a384 --- /dev/null +++ b/spec/services/contact_inbox/source_id_service_spec.rb @@ -0,0 +1,117 @@ +require 'rails_helper' + +RSpec.describe ContactInbox::SourceIdService do + let(:contact) { create(:contact, email: 'test@example.com', phone_number: '+1234567890') } + + describe '#generate' do + context 'when channel is TwilioSms' do + let(:channel_type) { 'Channel::TwilioSms' } + + context 'with SMS medium' do + subject { described_class.new(contact: contact, channel_type: channel_type, medium: 'sms') } + + it 'returns phone number as source id' do + expect(subject.generate).to eq(contact.phone_number) + end + end + + context 'with WhatsApp medium' do + subject { described_class.new(contact: contact, channel_type: channel_type, medium: 'whatsapp') } + + it 'returns whatsapp prefixed phone number as source id' do + expect(subject.generate).to eq("whatsapp:#{contact.phone_number}") + end + end + + context 'with invalid medium' do + subject { described_class.new(contact: contact, channel_type: channel_type, medium: 'invalid') } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'Unsupported Twilio medium: invalid') + end + end + + context 'without medium' do + subject { described_class.new(contact: contact, channel_type: channel_type) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'medium required for Twilio channel') + end + end + end + + context 'when channel is Whatsapp' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Whatsapp') } + + it 'returns phone number without + as source id' do + expect(subject.generate).to eq(contact.phone_number.delete('+')) + end + + context 'when contact has no phone number' do + let(:contact) { create(:contact, phone_number: nil) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'contact phone number required') + end + end + end + + context 'when channel is Email' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Email') } + + it 'returns email as source id' do + expect(subject.generate).to eq(contact.email) + end + + context 'when contact has no email' do + let(:contact) { create(:contact, email: nil) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'contact email required') + end + end + end + + context 'when channel is SMS' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Sms') } + + it 'returns phone number as source id' do + expect(subject.generate).to eq(contact.phone_number) + end + + context 'when contact has no phone number' do + let(:contact) { create(:contact, phone_number: nil) } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'contact phone number required') + end + end + end + + context 'when channel is Api' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Api') } + + it 'returns a UUID as source id' do + allow(SecureRandom).to receive(:uuid).and_return('uuid-123') + expect(subject.generate).to eq('uuid-123') + end + end + + context 'when channel is WebWidget' do + subject { described_class.new(contact: contact, channel_type: 'Channel::WebWidget') } + + it 'returns a UUID as source id' do + allow(SecureRandom).to receive(:uuid).and_return('uuid-123') + expect(subject.generate).to eq('uuid-123') + end + end + + context 'when channel is unsupported' do + subject { described_class.new(contact: contact, channel_type: 'Channel::Unknown') } + + it 'raises ArgumentError' do + expect { subject.generate }.to raise_error(ArgumentError, 'Unsupported operation for this channel: Channel::Unknown') + end + end + end +end diff --git a/spec/services/sms/send_on_sms_service_spec.rb b/spec/services/sms/send_on_sms_service_spec.rb index d2dc1f24a..52c5d89b1 100644 --- a/spec/services/sms/send_on_sms_service_spec.rb +++ b/spec/services/sms/send_on_sms_service_spec.rb @@ -5,8 +5,9 @@ describe Sms::SendOnSmsService do context 'when a valid message' do let(:sms_request) { double } let!(:sms_channel) { create(:channel_sms) } - let!(:contact_inbox) { create(:contact_inbox, inbox: sms_channel.inbox, source_id: '+123456789') } - let!(:conversation) { create(:conversation, contact_inbox: contact_inbox, inbox: sms_channel.inbox) } + let!(:contact) { create(:contact, phone_number: '+123456789') } + let!(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: sms_channel.inbox, source_id: '+123456789') } + let!(:conversation) { create(:conversation, contact_inbox: contact_inbox, inbox: sms_channel.inbox, contact: contact) } it 'calls channel.send_message' do message = create(:message, message_type: :outgoing, content: 'test', diff --git a/spec/services/twilio/send_on_twilio_service_spec.rb b/spec/services/twilio/send_on_twilio_service_spec.rb index beae54b57..af74a46e7 100644 --- a/spec/services/twilio/send_on_twilio_service_spec.rb +++ b/spec/services/twilio/send_on_twilio_service_spec.rb @@ -13,9 +13,11 @@ describe Twilio::SendOnTwilioService do let!(:twilio_whatsapp) { create(:channel_twilio_sms, medium: :whatsapp, account: account) } let!(:twilio_inbox) { create(:inbox, channel: twilio_sms, account: account) } let!(:twilio_whatsapp_inbox) { create(:inbox, channel: twilio_whatsapp, account: account) } - let!(:contact) { create(:contact, account: account) } - let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: twilio_inbox) } + let!(:contact) { create(:contact, account: account, phone_number: '+123456789') } + let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: twilio_inbox, source_id: '+123456789') } + let(:whatsapp_contact_inbox) { create(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:+123456789') } let(:conversation) { create(:conversation, contact: contact, inbox: twilio_inbox, contact_inbox: contact_inbox) } + let(:whatsapp_conversation) { create(:conversation, contact: contact, inbox: twilio_whatsapp_inbox, contact_inbox: whatsapp_contact_inbox) } before do allow(Twilio::REST::Client).to receive(:new).and_return(twilio_client) @@ -71,7 +73,7 @@ describe Twilio::SendOnTwilioService do allow(message_record_double).to receive(:sid).and_return('1234') message = build( - :message, message_type: 'outgoing', inbox: twilio_whatsapp_inbox, account: account, conversation: conversation + :message, message_type: 'outgoing', inbox: twilio_whatsapp_inbox, account: account, conversation: whatsapp_conversation ) attachment = message.attachments.new(account_id: message.account_id, file_type: :image) attachment.file.attach(io: Rails.root.join('spec/assets/avatar.png').open, filename: 'avatar.png', content_type: 'image/png') diff --git a/spec/services/whatsapp/send_on_whatsapp_service_spec.rb b/spec/services/whatsapp/send_on_whatsapp_service_spec.rb index 88f3f7740..deb7d6a3f 100644 --- a/spec/services/whatsapp/send_on_whatsapp_service_spec.rb +++ b/spec/services/whatsapp/send_on_whatsapp_service_spec.rb @@ -18,8 +18,9 @@ describe Whatsapp::SendOnWhatsappService do context 'when a valid message' do let(:whatsapp_request) { instance_double(HTTParty::Response) } let!(:whatsapp_channel) { create(:channel_whatsapp, sync_templates: false) } - let!(:contact_inbox) { create(:contact_inbox, inbox: whatsapp_channel.inbox, source_id: '123456789') } - let!(:conversation) { create(:conversation, contact_inbox: contact_inbox, inbox: whatsapp_channel.inbox) } + let!(:contact) { create(:contact, phone_number: '+123456789') } + let!(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: whatsapp_channel.inbox, source_id: '123456789') } + let!(:conversation) { create(:conversation, contact: contact, contact_inbox: contact_inbox, inbox: whatsapp_channel.inbox) } let(:api_key) { 'test_key' } let(:headers) { { 'D360-API-KEY' => api_key, 'Content-Type' => 'application/json' } } let(:template_body) do @@ -35,14 +36,12 @@ describe Whatsapp::SendOnWhatsappService do } end - let(:success_response) { { 'messages' => [{ 'id' => '123456789' }] }.to_json } + let(:success_response) { { 'messages' => [{ 'id' => 'message-123456789' }] }.to_json } it 'calls channel.send_message when with in 24 hour limit' do # to handle the case of 24 hour window limit. - create(:message, message_type: :incoming, content: 'test', - conversation: conversation) - message = create(:message, message_type: :outgoing, content: 'test', - conversation: conversation) + create(:message, message_type: :incoming, content: 'test', conversation: conversation) + message = create(:message, message_type: :outgoing, content: 'test', conversation: conversation) stub_request(:post, 'https://waba.360dialog.io/v1/messages') .with( @@ -52,7 +51,7 @@ describe Whatsapp::SendOnWhatsappService do .to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') end it 'calls channel.send_template when after 24 hour limit' do @@ -66,7 +65,7 @@ describe Whatsapp::SendOnWhatsappService do ).to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') end it 'calls channel.send_template if template_params are present' do @@ -79,7 +78,7 @@ describe Whatsapp::SendOnWhatsappService do ).to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') end it 'calls channel.send_template when template has regexp characters' do @@ -106,7 +105,18 @@ describe Whatsapp::SendOnWhatsappService do ).to_return(status: 200, body: success_response, headers: { 'content-type' => 'application/json' }) described_class.new(message: message).perform - expect(message.reload.source_id).to eq('123456789') + expect(message.reload.source_id).to eq('message-123456789') + end + + context 'when source_id validation is required' do + let(:message) { create(:message, conversation: conversation, message_type: :outgoing) } + + it 'marks message as failed when source_ids do not match' do + contact_inbox.update!(source_id: '1234567890') + described_class.new(message: message).perform + expect(message.reload.status).to eq('failed') + expect(message.external_error).to include('This conversation may have originally belonged to a different contact') + end end end end