chore: Fix contact model silently discarding invalid attributes (#4994)

fixes: #4775

app/actions/contact_identify_action.rb

@@ -6,7 +6,7 @@
 # We don't want to update the name of the identified original contact.
 
 class ContactIdentifyAction
-  pattr_initialize [:contact!, :params!, { retain_original_contact_name: false }]
+  pattr_initialize [:contact!, :params!, { retain_original_contact_name: false, discard_invalid_attrs: false }]
 
   def perform
     @attributes_to_update = [:identifier, :name, :email, :phone_number]
@@ -97,12 +97,13 @@ class ContactIdentifyAction
   end
 
   def update_contact
-    params_to_update = params.slice(*@attributes_to_update).reject do |_k, v|
+    @contact.attributes = params.slice(*@attributes_to_update).reject do |_k, v|
       v.blank?
     end.merge({ custom_attributes: custom_attributes, additional_attributes: additional_attributes })
     # blank identifier or email will throw unique index error
     # TODO: replace reject { |_k, v| v.blank? } with compact_blank when rails is upgraded
-    @contact.update!(params_to_update)
+    @contact.discard_invalid_attrs if discard_invalid_attrs
+    @contact.save!
     ContactAvatarJob.perform_later(@contact, params[:avatar_url]) if params[:avatar_url].present?
   end
 

app/controllers/api/v1/widget/contacts_controller.rb

@@ -36,7 +36,8 @@ class Api::V1::Widget::ContactsController < Api::V1::Widget::BaseController
   def identify_contact(contact)
     contact_identify_action = ContactIdentifyAction.new(
       contact: contact,
-      params: permitted_params.to_h.deep_symbolize_keys
+      params: permitted_params.to_h.deep_symbolize_keys,
+      discard_invalid_attrs: true
     )
     @contact = contact_identify_action.perform
   end

app/models/contact.rb

@@ -28,10 +28,12 @@ class Contact < ApplicationRecord
   include Labelable
 
   validates :account_id, presence: true
-  validates :email, allow_blank: true, uniqueness: { scope: [:account_id], case_sensitive: false }
+  validates :email, allow_blank: true, uniqueness: { scope: [:account_id], case_sensitive: false },
+                    format: { with: Devise.email_regexp, message: 'Invalid email' }
   validates :identifier, allow_blank: true, uniqueness: { scope: [:account_id] }
   validates :phone_number,
-            allow_blank: true, uniqueness: { scope: [:account_id] }
+            allow_blank: true, uniqueness: { scope: [:account_id] },
+            format: { with: /\+[1-9]\d{1,14}\z/, message: 'Should be in e164 format' }
   validates :name, length: { maximum: 255 }
 
   belongs_to :account
@@ -42,7 +44,6 @@ class Contact < ApplicationRecord
   has_many :messages, as: :sender, dependent: :destroy_async
   has_many :notes, dependent: :destroy_async
   before_validation :prepare_contact_attributes
-  before_save :phone_number_format, :email_format
   after_create_commit :dispatch_create_event, :ip_lookup
   after_update_commit :dispatch_update_event
   after_destroy_commit :dispatch_destroy_event
@@ -134,6 +135,11 @@ class Contact < ApplicationRecord
     ).or(Current.account.contacts.where.not(identifier: [nil, '']))
   end
 
+  def discard_invalid_attrs
+    phone_number_format
+    email_format
+  end
+
   private
 
   def ip_lookup

spec/actions/contact_identify_action_spec.rb

@@ -115,5 +115,23 @@ describe ::ContactIdentifyAction do
         expect { contact.reload }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end
+
+    context 'when discard_invalid_attrs is set to false' do
+      it 'will not update the name of the existing contact' do
+        params = { email: 'blah blah blah', name: 'new name' }
+        expect do
+          described_class.new(contact: contact, params: params, retain_original_contact_name: true).perform
+        end.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
+
+    context 'when discard_invalid_attrs is set to true' do
+      it 'will not update the name of the existing contact' do
+        params = { phone_number: 'blahblah blah', name: 'new name' }
+        described_class.new(contact: contact, params: params, discard_invalid_attrs: true).perform
+        expect(contact.reload.name).to eq 'new name'
+        expect(contact.phone_number).to eq nil
+      end
+    end
   end
 end

spec/controllers/api/v1/widget/contacts_controller_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe '/api/v1/widget/contacts', type: :request do
               as: :json
 
         expect(response).to have_http_status(:success)
-        expected_params = { contact: contact, params: params }
+        expected_params = { contact: contact, params: params, discard_invalid_attrs: true }
         expect(ContactIdentifyAction).to have_received(:new).with(expected_params)
         expect(identify_action).to have_received(:perform)
       end

spec/models/contact_spec.rb

@@ -44,41 +44,29 @@ RSpec.describe Contact do
     end
   end
 
-  # rubocop:disable Rails/SkipsModelValidations
   context 'when phone number format' do
-    it 'will not throw error for existing invalid phone number' do
+    it 'will throw error for existing invalid phone number' do
       contact = create(:contact)
-      contact.update_column(:phone_number, '344234')
+      expect { contact.update!(phone_number: '123456789') }.to raise_error(ActiveRecord::RecordInvalid)
-      contact.reload
-      expect(contact.update!(name: 'test')).to eq true
-      expect(contact.phone_number).to eq '344234'
     end
 
     it 'updates phone number when adding valid phone number' do
       contact = create(:contact)
-      contact.update_column(:phone_number, '344234')
-      contact.reload
       expect(contact.update!(phone_number: '+12312312321')).to eq true
       expect(contact.phone_number).to eq '+12312312321'
     end
   end
 
   context 'when email format' do
-    it 'will not throw error for existing invalid email' do
+    it 'will throw error for existing invalid email' do
       contact = create(:contact)
-      contact.update_column(:email, 'ssfdasdf <test@test')
+      expect { contact.update!(email: '<2324234234') }.to raise_error(ActiveRecord::RecordInvalid)
-      contact.reload
-      expect(contact.update!(name: 'test')).to eq true
-      expect(contact.email).to eq 'ssfdasdf <test@test'
     end
 
     it 'updates email when adding valid email' do
       contact = create(:contact)
-      contact.update_column(:email, 'ssfdasdf <test@test')
-      contact.reload
       expect(contact.update!(email: 'test@test.com')).to eq true
       expect(contact.email).to eq 'test@test.com'
     end
   end
-  # rubocop:enable Rails/SkipsModelValidations
 end