From 1819041f5a932f394cd54f968eede61d0750ded7 Mon Sep 17 00:00:00 2001 From: "OMAR.A" <58332033+civilcoder55@users.noreply.github.com> Date: Thu, 29 Sep 2022 19:34:55 +0200 Subject: [PATCH] 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 --- app/builders/contact_inbox_builder.rb | 2 +- app/models/contact_inbox.rb | 16 +++- lib/regex_helper.rb | 4 + spec/builders/contact_inbox_builder_spec.rb | 92 +++++++++++++++------ spec/factories/contact_inbox.rb | 2 + spec/models/contact_inbox_spec.rb | 55 ++++++++++++ 6 files changed, 143 insertions(+), 28 deletions(-) diff --git a/app/builders/contact_inbox_builder.rb b/app/builders/contact_inbox_builder.rb index e7ae8b0aa..1b8782c51 100644 --- a/app/builders/contact_inbox_builder.rb +++ b/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 diff --git a/app/models/contact_inbox.rb b/app/models/contact_inbox.rb index dcbd2f56b..24f3b727a 100644 --- a/app/models/contact_inbox.rb +++ b/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) - errors.add(:source_id, 'invalid source id for twilio sms inbox. valid Regex /\+[1-9]\d{1,14}\z/') - elsif inbox.channel.medium == 'whatsapp' && !/whatsapp:\+[1-9]\d{1,14}\z/.match?(source_id) - errors.add(:source_id, 'invalid source id for twilio whatsapp inbox. valid Regex /whatsapp:\+[1-9]\d{1,14}\z/') + 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 #{TWILIO_CHANNEL_SMS_REGEX}") + 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 #{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 diff --git a/lib/regex_helper.rb b/lib/regex_helper.rb index 2bbd51809..ee452b352 100644 --- a/lib/regex_helper.rb +++ b/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 diff --git a/spec/builders/contact_inbox_builder_spec.rb b/spec/builders/contact_inbox_builder_spec.rb index 2de5b6b60..47210f6ca 100644 --- a/spec/builders/contact_inbox_builder_spec.rb +++ b/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.source_id).not_to be('+224213223422') + expect(contact_inbox.id).not_to eq(existing_contact_inbox.id) + 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.source_id).not_to be('whatsapp:+55555') + expect(contact_inbox.id).not_to eq(existing_contact_inbox.id) + 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.source_id).not_to be('+224213223422') + expect(contact_inbox.id).not_to eq(existing_contact_inbox.id) + 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.source_id).not_to be('xyc@xyc.com') + expect(contact_inbox.id).not_to eq(existing_contact_inbox.id) + 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.source_id).not_to be('test') + expect(contact_inbox.id).not_to eq(existing_contact_inbox.id) + 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 diff --git a/spec/factories/contact_inbox.rb b/spec/factories/contact_inbox.rb index 23abde2ca..e114a1567 100644 --- a/spec/factories/contact_inbox.rb +++ b/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 diff --git a/spec/models/contact_inbox_spec.rb b/spec/models/contact_inbox_spec.rb index 1c575f105..aba062980 100644 --- a/spec/models/contact_inbox_spec.rb +++ b/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