fix: "wa_source_id" function return value (#5451)

- Fix contact inbox builder returning invalid WhatsApp source id
- Add specs to cover source id validations

Co-authored-by: Sojan Jose <sojan@pepalo.com>

app/builders/contact_inbox_builder.rb

@@ -31,7 +31,7 @@ class ContactInboxBuilder
     return unless @contact.phone_number
 
     # whatsapp doesn't want the + in e164 format
-    "#{@contact.phone_number}.delete('+')"
+    @contact.phone_number.delete('+').to_s
   end
 
   def twilio_source_id

app/models/contact_inbox.rb

@@ -22,6 +22,7 @@
 
 class ContactInbox < ApplicationRecord
   include Pubsubable
+  include RegexHelper
   validates :inbox_id, presence: true
   validates :contact_id, presence: true
   validates :source_id, presence: true
@@ -51,10 +52,10 @@ class ContactInbox < ApplicationRecord
 
   def validate_twilio_source_id
     # https://www.twilio.com/docs/glossary/what-e164#regex-matching-for-e164
-    if inbox.channel.medium == 'sms' && !/\+[1-9]\d{1,14}\z/.match?(source_id)
+    if inbox.channel.medium == 'sms' && !TWILIO_CHANNEL_SMS_REGEX.match?(source_id)
-      errors.add(:source_id, 'invalid source id for twilio sms inbox. valid Regex /\+[1-9]\d{1,14}\z/')
+      errors.add(:source_id, "invalid source id for twilio sms inbox. valid Regex #{TWILIO_CHANNEL_SMS_REGEX}")
-    elsif inbox.channel.medium == 'whatsapp' && !/whatsapp:\+[1-9]\d{1,14}\z/.match?(source_id)
+    elsif inbox.channel.medium == 'whatsapp' && !TWILIO_CHANNEL_WHATSAPP_REGEX.match?(source_id)
-      errors.add(:source_id, 'invalid source id for twilio whatsapp inbox. valid Regex /whatsapp:\+[1-9]\d{1,14}\z/')
+      errors.add(:source_id, "invalid source id for twilio whatsapp inbox. valid Regex #{TWILIO_CHANNEL_WHATSAPP_REGEX}")
     end
   end
 
@@ -62,8 +63,15 @@ class ContactInbox < ApplicationRecord
     errors.add(:source_id, "invalid source id for Email inbox. valid Regex #{Devise.email_regexp}") unless Devise.email_regexp.match?(source_id)
   end
 
+  def validate_whatsapp_source_id
+    return if WHATSAPP_CHANNEL_REGEX.match?(source_id)
+
+    errors.add(:source_id, "invalid source id for whatsapp inbox. valid Regex #{WHATSAPP_CHANNEL_REGEX}")
+  end
+
   def valid_source_id_format?
     validate_twilio_source_id if inbox.channel_type == 'Channel::TwilioSms'
     validate_email_source_id if inbox.channel_type == 'Channel::Email'
+    validate_whatsapp_source_id if inbox.channel_type == 'Channel::Whatsapp'
   end
 end

lib/regex_helper.rb

@@ -6,4 +6,8 @@ module RegexHelper
   # shouldn't start with a underscore or hyphen
   UNICODE_CHARACTER_NUMBER_HYPHEN_UNDERSCORE = Regexp.new('\A[\p{L}\p{N}]+[\p{L}\p{N}_-]+\Z')
   MENTION_REGEX = Regexp.new('\[(@[\w_. ]+)\]\(mention://(?:user|team)/\d+/(.*?)+\)')
+
+  TWILIO_CHANNEL_SMS_REGEX = Regexp.new('^\+\d{1,14}\z')
+  TWILIO_CHANNEL_WHATSAPP_REGEX = Regexp.new('^whatsapp:\+\d{1,14}\z')
+  WHATSAPP_CHANNEL_REGEX = Regexp.new('^\d{1,14}\z')
 end

spec/builders/contact_inbox_builder_spec.rb

@@ -17,7 +17,7 @@ describe ::ContactInboxBuilder do
           source_id: contact.phone_number
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'does not create contact inbox when contact inbox already exists with phone number and source id is not provided' do
@@ -27,7 +27,7 @@ describe ::ContactInboxBuilder do
           inbox_id: twilio_inbox.id
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'creates a new contact inbox when different source id is provided' do
@@ -38,8 +38,8 @@ describe ::ContactInboxBuilder do
           source_id: '+224213223422'
         ).perform
 
-        expect(contact_inbox.id).not_to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).not_to eq(existing_contact_inbox.id)
-        expect(contact_inbox.source_id).not_to be('+224213223422')
+        expect(contact_inbox.source_id).to eq('+224213223422')
       end
 
       it 'creates a contact inbox with contact phone number when source id not provided and no contact inbox exists' do
@@ -48,7 +48,7 @@ describe ::ContactInboxBuilder do
           inbox_id: twilio_inbox.id
         ).perform
 
-        expect(contact_inbox.source_id).not_to be(contact.phone_number)
+        expect(contact_inbox.source_id).to eq(contact.phone_number)
       end
     end
 
@@ -64,7 +64,7 @@ describe ::ContactInboxBuilder do
           source_id: "whatsapp:#{contact.phone_number}"
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'does not create contact inbox when contact inbox already exists with phone number and source id is not provided' do
@@ -74,7 +74,7 @@ describe ::ContactInboxBuilder do
           inbox_id: twilio_inbox.id
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'creates a new contact inbox when different source id is provided' do
@@ -85,8 +85,8 @@ describe ::ContactInboxBuilder do
           source_id: 'whatsapp:+555555'
         ).perform
 
-        expect(contact_inbox.id).not_to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).not_to eq(existing_contact_inbox.id)
-        expect(contact_inbox.source_id).not_to be('whatsapp:+55555')
+        expect(contact_inbox.source_id).to eq('whatsapp:+555555')
       end
 
       it 'creates a contact inbox with contact phone number when source id not provided and no contact inbox exists' do
@@ -95,7 +95,53 @@ describe ::ContactInboxBuilder do
           inbox_id: twilio_inbox.id
         ).perform
 
-        expect(contact_inbox.source_id).not_to be("whatsapp:#{contact.phone_number}")
+        expect(contact_inbox.source_id).to eq("whatsapp:#{contact.phone_number}")
+      end
+    end
+
+    describe 'whatsapp inbox' do
+      let(:whatsapp_inbox) { create(:channel_whatsapp, account: account, sync_templates: false, validate_provider_config: false).inbox }
+
+      it 'does not create contact inbox when contact inbox already exists with the source id provided' do
+        existing_contact_inbox = create(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: contact.phone_number&.delete('+'))
+        contact_inbox = described_class.new(
+          contact_id: contact.id,
+          inbox_id: whatsapp_inbox.id,
+          source_id: contact.phone_number&.delete('+')
+        ).perform
+
+        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+      end
+
+      it 'does not create contact inbox when contact inbox already exists with phone number and source id is not provided' do
+        existing_contact_inbox = create(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: contact.phone_number&.delete('+'))
+        contact_inbox = described_class.new(
+          contact_id: contact.id,
+          inbox_id: whatsapp_inbox.id
+        ).perform
+
+        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+      end
+
+      it 'creates a new contact inbox when different source id is provided' do
+        existing_contact_inbox = create(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: contact.phone_number&.delete('+'))
+        contact_inbox = described_class.new(
+          contact_id: contact.id,
+          inbox_id: whatsapp_inbox.id,
+          source_id: '555555'
+        ).perform
+
+        expect(contact_inbox.id).not_to be(existing_contact_inbox.id)
+        expect(contact_inbox.source_id).not_to be('555555')
+      end
+
+      it 'creates a contact inbox with contact phone number when source id not provided and no contact inbox exists' do
+        contact_inbox = described_class.new(
+          contact_id: contact.id,
+          inbox_id: whatsapp_inbox.id
+        ).perform
+
+        expect(contact_inbox.source_id).to eq(contact.phone_number&.delete('+'))
       end
     end
 
@@ -111,7 +157,7 @@ describe ::ContactInboxBuilder do
           source_id: contact.phone_number
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'does not create contact inbox when contact inbox already exists with phone number and source id is not provided' do
@@ -121,7 +167,7 @@ describe ::ContactInboxBuilder do
           inbox_id: sms_inbox.id
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'creates a new contact inbox when different source id is provided' do
@@ -132,8 +178,8 @@ describe ::ContactInboxBuilder do
           source_id: '+224213223422'
         ).perform
 
-        expect(contact_inbox.id).not_to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).not_to eq(existing_contact_inbox.id)
-        expect(contact_inbox.source_id).not_to be('+224213223422')
+        expect(contact_inbox.source_id).to eq('+224213223422')
       end
 
       it 'creates a contact inbox with contact phone number when source id not provided and no contact inbox exists' do
@@ -142,7 +188,7 @@ describe ::ContactInboxBuilder do
           inbox_id: sms_inbox.id
         ).perform
 
-        expect(contact_inbox.source_id).not_to be(contact.phone_number)
+        expect(contact_inbox.source_id).to eq(contact.phone_number)
       end
     end
 
@@ -158,7 +204,7 @@ describe ::ContactInboxBuilder do
           source_id: contact.email
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'does not create contact inbox when contact inbox already exists with email and source id is not provided' do
@@ -168,7 +214,7 @@ describe ::ContactInboxBuilder do
           inbox_id: email_inbox.id
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'creates a new contact inbox when different source id is provided' do
@@ -179,8 +225,8 @@ describe ::ContactInboxBuilder do
           source_id: 'xyc@xyc.com'
         ).perform
 
-        expect(contact_inbox.id).not_to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).not_to eq(existing_contact_inbox.id)
-        expect(contact_inbox.source_id).not_to be('xyc@xyc.com')
+        expect(contact_inbox.source_id).to eq('xyc@xyc.com')
       end
 
       it 'creates a contact inbox with contact email when source id not provided and no contact inbox exists' do
@@ -189,7 +235,7 @@ describe ::ContactInboxBuilder do
           inbox_id: email_inbox.id
         ).perform
 
-        expect(contact_inbox.source_id).not_to be(contact.email)
+        expect(contact_inbox.source_id).to eq(contact.email)
       end
     end
 
@@ -205,7 +251,7 @@ describe ::ContactInboxBuilder do
           source_id: 'test'
         ).perform
 
-        expect(contact_inbox.id).to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).to eq(existing_contact_inbox.id)
       end
 
       it 'creates a new contact inbox when different source id is provided' do
@@ -216,8 +262,8 @@ describe ::ContactInboxBuilder do
           source_id: 'test'
         ).perform
 
-        expect(contact_inbox.id).not_to be(existing_contact_inbox.id)
+        expect(contact_inbox.id).not_to eq(existing_contact_inbox.id)
-        expect(contact_inbox.source_id).not_to be('test')
+        expect(contact_inbox.source_id).to eq('test')
       end
 
       it 'creates a contact inbox with SecureRandom.uuid when source id not provided and no contact inbox exists' do

spec/factories/contact_inbox.rb

@@ -15,6 +15,8 @@ def generate_source_id(contact_inbox)
     contact_inbox.inbox.channel.medium == 'sms' ? Faker::PhoneNumber.cell_phone_in_e164 : "whatsapp:#{Faker::PhoneNumber.cell_phone_in_e164}"
   when 'Channel::Email'
     "#{SecureRandom.uuid}@acme.inc"
+  when 'Channel::Whatsapp'
+    Faker::PhoneNumber.cell_phone_in_e164.delete('+')
   else
     SecureRandom.uuid
   end

spec/models/contact_inbox_spec.rb

@@ -37,4 +37,59 @@ RSpec.describe ContactInbox do
       expect(obj.pubsub_token).to eq(new_token)
     end
   end
+
+  describe 'validations' do
+    context 'when source_id' do
+      it 'validates whatsapp channel source_id' do
+        whatsapp_inbox = create(:channel_whatsapp, sync_templates: false, validate_provider_config: false).inbox
+        contact = create(:contact)
+        valid_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: '1234567890')
+        ci_character_in_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: '1234567890aaa')
+        ci_plus_in_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: '+1234567890')
+        expect(valid_source_id.valid?).to be(true)
+        expect(ci_character_in_source_id.valid?).to be(false)
+        expect(ci_character_in_source_id.errors.full_messages).to eq(
+          ['Source invalid source id for whatsapp inbox. valid Regex (?-mix:^\\d{1,14}\\z)']
+        )
+        expect(ci_plus_in_source_id.valid?).to be(false)
+        expect(ci_plus_in_source_id.errors.full_messages).to eq(
+          ['Source invalid source id for whatsapp inbox. valid Regex (?-mix:^\\d{1,14}\\z)']
+        )
+      end
+
+      it 'validates twilio sms channel source_id' do
+        twilio_sms_inbox = create(:channel_twilio_sms).inbox
+        contact = create(:contact)
+        valid_source_id = build(:contact_inbox, contact: contact, inbox: twilio_sms_inbox, source_id: '+1234567890')
+        ci_character_in_source_id = build(:contact_inbox, contact: contact, inbox: twilio_sms_inbox, source_id: '+1234567890aaa')
+        ci_without_plus_in_source_id = build(:contact_inbox, contact: contact, inbox: twilio_sms_inbox, source_id: '1234567890')
+        expect(valid_source_id.valid?).to be(true)
+        expect(ci_character_in_source_id.valid?).to be(false)
+        expect(ci_character_in_source_id.errors.full_messages).to eq(
+          ['Source invalid source id for twilio sms inbox. valid Regex (?-mix:^\\+\\d{1,14}\\z)']
+        )
+        expect(ci_without_plus_in_source_id.valid?).to be(false)
+        expect(ci_without_plus_in_source_id.errors.full_messages).to eq(
+          ['Source invalid source id for twilio sms inbox. valid Regex (?-mix:^\\+\\d{1,14}\\z)']
+        )
+      end
+
+      it 'validates twilio whatsapp channel source_id' do
+        twilio_whatsapp_inbox = create(:channel_twilio_sms, medium: :whatsapp).inbox
+        contact = create(:contact)
+        valid_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:+1234567890')
+        ci_character_in_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:+1234567890aaa')
+        ci_without_plus_in_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:1234567890')
+        expect(valid_source_id.valid?).to be(true)
+        expect(ci_character_in_source_id.valid?).to be(false)
+        expect(ci_character_in_source_id.errors.full_messages).to eq(
+          ['Source invalid source id for twilio whatsapp inbox. valid Regex (?-mix:^whatsapp:\\+\\d{1,14}\\z)']
+        )
+        expect(ci_without_plus_in_source_id.valid?).to be(false)
+        expect(ci_without_plus_in_source_id.errors.full_messages).to eq(
+          ['Source invalid source id for twilio whatsapp inbox. valid Regex (?-mix:^whatsapp:\\+\\d{1,14}\\z)']
+        )
+      end
+    end
+  end
 end