From e4b159dd54ae209602d488934a8081c513f84ef2 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Fri, 8 Jul 2022 10:28:09 +0200 Subject: [PATCH] chore: Fix contact model silently discarding invalid attributes (#4994) fixes: #4775 --- app/actions/contact_identify_action.rb | 7 ++++--- .../api/v1/widget/contacts_controller.rb | 3 ++- app/models/contact.rb | 12 ++++++++--- spec/actions/contact_identify_action_spec.rb | 18 +++++++++++++++++ .../api/v1/widget/contacts_controller_spec.rb | 2 +- spec/models/contact_spec.rb | 20 ++++--------------- 6 files changed, 38 insertions(+), 24 deletions(-) diff --git a/app/actions/contact_identify_action.rb b/app/actions/contact_identify_action.rb index ed90306eccb0..f19caac7748b 100644 --- a/app/actions/contact_identify_action.rb +++ b/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 @@ def mergable_phone_contact? 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 diff --git a/app/controllers/api/v1/widget/contacts_controller.rb b/app/controllers/api/v1/widget/contacts_controller.rb index b6f997b06c40..5138fe6755d7 100644 --- a/app/controllers/api/v1/widget/contacts_controller.rb +++ b/app/controllers/api/v1/widget/contacts_controller.rb @@ -36,7 +36,8 @@ def destroy_custom_attributes 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 diff --git a/app/models/contact.rb b/app/models/contact.rb index 53dc9d28182c..157878654c35 100644 --- a/app/models/contact.rb +++ b/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 @@ def self.resolved_contacts ).or(Current.account.contacts.where.not(identifier: [nil, ''])) end + def discard_invalid_attrs + phone_number_format + email_format + end + private def ip_lookup diff --git a/spec/actions/contact_identify_action_spec.rb b/spec/actions/contact_identify_action_spec.rb index 00599c65d6fa..d64731c3ecb7 100644 --- a/spec/actions/contact_identify_action_spec.rb +++ b/spec/actions/contact_identify_action_spec.rb @@ -115,5 +115,23 @@ 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 diff --git a/spec/controllers/api/v1/widget/contacts_controller_spec.rb b/spec/controllers/api/v1/widget/contacts_controller_spec.rb index 0a29d828578e..0b69c0859448 100644 --- a/spec/controllers/api/v1/widget/contacts_controller_spec.rb +++ b/spec/controllers/api/v1/widget/contacts_controller_spec.rb @@ -33,7 +33,7 @@ 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 diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb index 416e0e68d7de..a6893ae760d1 100644 --- a/spec/models/contact_spec.rb +++ b/spec/models/contact_spec.rb @@ -44,41 +44,29 @@ 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') - contact.reload - expect(contact.update!(name: 'test')).to eq true - expect(contact.phone_number).to eq '344234' + expect { contact.update!(phone_number: '123456789') }.to raise_error(ActiveRecord::RecordInvalid) 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