chore: Enhance contact merge action for identified users (#4886)

- Discard conflicting keys 
- Do not merge if there is already an identified contact

Co-authored-by: Pranav Raj S <pranav@chatwoot.com>
This commit is contained in:
Sojan Jose 2022-06-23 15:48:56 +05:30 committed by GitHub
parent d5ddc9d76c
commit f71980bd95
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 311 additions and 114 deletions

View file

@ -1,7 +1,16 @@
# retain_original_contact_name: false / true
# In case of setUser we want to update the name of the identified contact,
# which is the default behaviour
#
# But, In case of contact merge during prechat form contact update.
# We don't want to update the name of the identified original contact.
class ContactIdentifyAction
pattr_initialize [:contact!, :params!]
pattr_initialize [:contact!, :params!, { retain_original_contact_name: false }]
def perform
@attributes_to_update = [:identifier, :name, :email, :phone_number]
ActiveRecord::Base.transaction do
merge_if_existing_identified_contact
merge_if_existing_email_contact
@ -18,49 +27,88 @@ class ContactIdentifyAction
end
def merge_if_existing_identified_contact
@contact = merge_contact(existing_identified_contact, @contact) if merge_contacts?(existing_identified_contact, @contact)
return unless merge_contacts?(existing_identified_contact, :identifier)
process_contact_merge(existing_identified_contact)
end
def merge_if_existing_email_contact
@contact = merge_contact(existing_email_contact, @contact) if merge_contacts?(existing_email_contact, @contact)
return unless merge_contacts?(existing_email_contact, :email)
process_contact_merge(existing_email_contact)
end
def merge_if_existing_phone_number_contact
@contact = merge_contact(existing_phone_number_contact, @contact) if merge_contacts?(existing_phone_number_contact, @contact)
return unless merge_contacts?(existing_phone_number_contact, :phone_number)
return unless mergable_phone_contact?
process_contact_merge(existing_phone_number_contact)
end
def process_contact_merge(mergee_contact)
@contact = merge_contact(mergee_contact, @contact)
@attributes_to_update.delete(:name) if retain_original_contact_name
end
def existing_identified_contact
return if params[:identifier].blank?
@existing_identified_contact ||= Contact.where(account_id: account.id).find_by(identifier: params[:identifier])
@existing_identified_contact ||= account.contacts.find_by(identifier: params[:identifier])
end
def existing_email_contact
return if params[:email].blank?
@existing_email_contact ||= Contact.where(account_id: account.id).find_by(email: params[:email])
@existing_email_contact ||= account.contacts.find_by(email: params[:email])
end
def existing_phone_number_contact
return if params[:phone_number].blank?
@existing_phone_number_contact ||= Contact.where(account_id: account.id).find_by(phone_number: params[:phone_number])
@existing_phone_number_contact ||= account.contacts.find_by(phone_number: params[:phone_number])
end
def merge_contacts?(existing_contact, _contact)
existing_contact && existing_contact.id != @contact.id
def merge_contacts?(existing_contact, key)
return if existing_contact.blank?
return true if params[:identifier].blank?
# we want to prevent merging contacts with different identifiers
if existing_contact.identifier.present? && existing_contact.identifier != params[:identifier]
# we will remove attribute from update list
@attributes_to_update.delete(key)
return false
end
true
end
# case: contact 1: email: 1@test.com, phone: 123456789
# params: email: 2@test.com, phone: 123456789
# we don't want to overwrite 1@test.com since email parameter takes higer priority
def mergable_phone_contact?
return true if params[:email].blank?
if existing_phone_number_contact.email.present? && existing_phone_number_contact.email != params[:email]
@attributes_to_update.delete(:phone_number)
return false
end
true
end
def update_contact
params_to_update = 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.slice(:name, :email, :identifier, :phone_number).reject do |_k, v|
v.blank?
end.merge({ custom_attributes: custom_attributes, additional_attributes: additional_attributes }))
@contact.update!(params_to_update)
ContactAvatarJob.perform_later(@contact, params[:avatar_url]) if params[:avatar_url].present?
end
def merge_contact(base_contact, merge_contact)
return base_contact if base_contact.id == merge_contact.id
ContactMergeAction.new(
account: account,
base_contact: base_contact,
@ -69,14 +117,14 @@ class ContactIdentifyAction
end
def custom_attributes
params[:custom_attributes] ? @contact.custom_attributes.deep_merge(params[:custom_attributes].stringify_keys) : @contact.custom_attributes
return @contact.custom_attributes if params[:custom_attributes].blank?
(@contact.custom_attributes || {}).deep_merge(params[:custom_attributes].stringify_keys)
end
def additional_attributes
if params[:additional_attributes]
@contact.additional_attributes.deep_merge(params[:additional_attributes].stringify_keys)
else
@contact.additional_attributes
end
return @contact.additional_attributes if params[:additional_attributes].blank?
(@contact.additional_attributes || {}).deep_merge(params[:additional_attributes].stringify_keys)
end
end

View file

@ -44,38 +44,6 @@ class Api::V1::Widget::BaseController < ApplicationController
}
end
def update_contact(email)
contact_with_email = @current_account.contacts.find_by(email: email)
if contact_with_email
@contact = ::ContactMergeAction.new(
account: @current_account,
base_contact: contact_with_email,
mergee_contact: @contact
).perform
else
@contact.update!(email: email)
update_contact_name
end
end
def update_contact_phone_number(phone_number)
contact_with_phone_number = @current_account.contacts.find_by(phone_number: phone_number)
if contact_with_phone_number
@contact = ::ContactMergeAction.new(
account: @current_account,
base_contact: contact_with_phone_number,
mergee_contact: @contact
).perform
else
@contact.update!(phone_number: phone_number)
update_contact_name
end
end
def update_contact_name
@contact.update!(name: contact_name) if contact_name.present?
end
def contact_email
permitted_params.dig(:contact, :email)&.downcase
end

View file

@ -1,14 +1,27 @@
class Api::V1::Widget::ContactsController < Api::V1::Widget::BaseController
before_action :process_hmac, only: [:update]
include WidgetHelper
before_action :validate_hmac, only: [:set_user]
def show; end
def update
contact_identify_action = ContactIdentifyAction.new(
contact: @contact,
params: permitted_params.to_h.deep_symbolize_keys
)
@contact = contact_identify_action.perform
identify_contact(@contact)
end
def set_user
contact = nil
if a_different_contact?
@contact_inbox, @widget_auth_token = build_contact_inbox_with_token(@web_widget)
contact = @contact_inbox.contact
else
contact = @contact
end
@contact_inbox.update(hmac_verified: true) if should_verify_hmac? && valid_hmac?
identify_contact(contact)
end
# TODO : clean up this with proper routes delete contacts/custom_attributes
@ -20,12 +33,22 @@ class Api::V1::Widget::ContactsController < Api::V1::Widget::BaseController
private
def process_hmac
def identify_contact(contact)
contact_identify_action = ContactIdentifyAction.new(
contact: contact,
params: permitted_params.to_h.deep_symbolize_keys
)
@contact = contact_identify_action.perform
end
def a_different_contact?
@contact.identifier.present? && @contact.identifier != permitted_params[:identifier]
end
def validate_hmac
return unless should_verify_hmac?
render json: { error: 'HMAC failed: Invalid Identifier Hash Provided' }, status: :unauthorized unless valid_hmac?
@contact_inbox.update(hmac_verified: true)
end
def should_verify_hmac?

View file

@ -14,8 +14,11 @@ class Api::V1::Widget::ConversationsController < Api::V1::Widget::BaseController
end
def process_update_contact
update_contact(contact_email) if @contact.email.blank? && contact_email.present?
update_contact_phone_number(contact_phone_number) if @contact.phone_number.blank? && contact_phone_number.present?
@contact = ContactIdentifyAction.new(
contact: @contact,
params: { email: contact_email, phone_number: contact_phone_number, name: contact_name },
retain_original_contact_name: true
).perform
end
def update_last_seen

View file

@ -15,7 +15,10 @@ class Api::V1::Widget::MessagesController < Api::V1::Widget::BaseController
def update
if @message.content_type == 'input_email'
@message.update!(submitted_email: contact_email)
update_contact(contact_email)
ContactIdentifyAction.new(
contact: @contact,
params: { email: contact_email }
).perform
else
@message.update!(message_update_params[:message])
end

View file

@ -1,5 +1,7 @@
# TODO : Delete this and associated spec once 'api/widget/config' end point is merged
class WidgetsController < ActionController::Base
include WidgetHelper
before_action :set_global_config
before_action :set_web_widget
before_action :set_token
@ -40,11 +42,8 @@ class WidgetsController < ActionController::Base
def build_contact
return if @contact.present?
@contact_inbox = @web_widget.create_contact_inbox(additional_attributes)
@contact_inbox, @token = build_contact_inbox_with_token(@web_widget, additional_attributes)
@contact = @contact_inbox.contact
payload = { source_id: @contact_inbox.source_id, inbox_id: @web_widget.inbox.id }
@token = ::Widget::TokenService.new(payload: payload).generate_token
end
def additional_attributes

View file

@ -0,0 +1,9 @@
module WidgetHelper
def build_contact_inbox_with_token(web_widget, additional_attributes = {})
contact_inbox = web_widget.create_contact_inbox(additional_attributes)
payload = { source_id: contact_inbox.source_id, inbox_id: web_widget.inbox.id }
token = ::Widget::TokenService.new(payload: payload).generate_token
[contact_inbox, token]
end
end

View file

@ -33,6 +33,12 @@ import {
import { isFlatWidgetStyle } from './settingsHelper';
import { popoutChatWindow } from '../widget/helpers/popoutHelper';
const updateAuthCookie = cookieContent =>
Cookies.set('cw_conversation', cookieContent, {
expires: 365,
sameSite: 'Lax',
});
export const IFrameHelper = {
getUrl({ baseUrl, websiteToken }) {
return `${baseUrl}/widget?website_token=${websiteToken}`;
@ -134,10 +140,7 @@ export const IFrameHelper = {
events: {
loaded: message => {
Cookies.set('cw_conversation', message.config.authToken, {
expires: 365,
sameSite: 'Lax',
});
updateAuthCookie(message.config.authToken);
window.$chatwoot.hasLoaded = true;
IFrameHelper.sendMessage('config-set', {
locale: window.$chatwoot.locale,
@ -178,6 +181,10 @@ export const IFrameHelper = {
setBubbleText(window.$chatwoot.launcherTitle || message.label);
},
setAuthCookie({ data: { widgetAuthToken } }) {
updateAuthCookie(widgetAuthToken);
},
toggleBubble: state => {
let bubbleState = {};
if (state === 'open') {

View file

@ -83,12 +83,11 @@ export default {
const { websiteToken, locale, widgetColor } = window.chatwootWebChannel;
this.setLocale(locale);
this.setWidgetColor(widgetColor);
setHeader(window.authToken);
if (this.isIFrame) {
this.registerListeners();
this.sendLoadedEvent();
setHeader('X-Auth-Token', window.authToken);
} else {
setHeader('X-Auth-Token', window.authToken);
this.fetchOldConversations();
this.fetchAvailableAgents(websiteToken);
this.setLocale(getLocale(window.location.search));
@ -253,7 +252,7 @@ export default {
} else if (message.event === 'remove-label') {
this.$store.dispatch('conversationLabels/destroy', message.label);
} else if (message.event === 'set-user') {
this.$store.dispatch('contacts/update', message);
this.$store.dispatch('contacts/setUser', message);
} else if (message.event === 'set-custom-attributes') {
this.$store.dispatch(
'contacts/setCustomAttributes',

View file

@ -6,8 +6,11 @@ export default {
get() {
return API.get(buildUrl('widget/contact'));
},
update(identifier, userObject) {
return API.patch(buildUrl('widget/contact'), {
update(userObject) {
return API.patch(buildUrl('widget/contact'), userObject);
},
setUser(identifier, userObject) {
return API.patch(buildUrl('widget/contact/set_user'), {
identifier,
...userObject,
});

View file

@ -6,7 +6,7 @@ export const API = axios.create({
withCredentials: false,
});
export const setHeader = (key, value) => {
export const setHeader = (value, key = 'X-Auth-Token') => {
API.defaults.headers.common[key] = value;
};

View file

@ -10,14 +10,16 @@ export const arrayToHashById = array =>
return newMap;
}, {});
export const sendMessage = msg => {
window.parent.postMessage(
`chatwoot-widget:${JSON.stringify({ ...msg })}`,
'*'
);
};
export const IFrameHelper = {
isIFrame: () => window.self !== window.top,
sendMessage: msg => {
window.parent.postMessage(
`chatwoot-widget:${JSON.stringify({ ...msg })}`,
'*'
);
},
sendMessage,
isAValidEvent: e => {
const isDataAString = typeof e.data === 'string';
const isAValidWootEvent =

View file

@ -1,11 +1,23 @@
import { IFrameHelper } from 'widget/helpers/utils';
import { sendMessage } from 'widget/helpers/utils';
import ContactsAPI from '../../api/contacts';
import { SET_USER_ERROR } from '../../constants/errorTypes';
import { setHeader } from '../../helpers/axios';
const state = {
currentUser: {},
};
const SET_CURRENT_USER = 'SET_CURRENT_USER';
const parseErrorData = error =>
error && error.response && error.response.data ? error.response.data : error;
export const updateWidgetAuthToken = widgetAuthToken => {
if (widgetAuthToken) {
setHeader(widgetAuthToken);
sendMessage({
event: 'setAuthCookie',
data: { widgetAuthToken },
});
}
};
export const getters = {
getCurrentUser(_state) {
@ -22,13 +34,21 @@ export const actions = {
// Ignore error
}
},
update: async ({ dispatch }, { identifier, user: userObject }) => {
update: async ({ dispatch }, { user }) => {
try {
await ContactsAPI.update(user);
dispatch('get');
} catch (error) {
// Ignore error
}
},
setUser: async ({ dispatch }, { identifier, user: userObject }) => {
try {
const {
email,
name,
avatar_url,
identifier_hash,
identifier_hash: identifierHash,
phone_number,
company_name,
city,
@ -41,7 +61,7 @@ export const actions = {
email,
name,
avatar_url,
identifier_hash,
identifier_hash: identifierHash,
phone_number,
additional_attributes: {
company_name,
@ -52,24 +72,19 @@ export const actions = {
},
custom_attributes,
};
await ContactsAPI.update(identifier, user);
const {
data: { widget_auth_token: widgetAuthToken },
} = await ContactsAPI.setUser(identifier, user);
updateWidgetAuthToken(widgetAuthToken);
dispatch('get');
if (identifier_hash) {
if (identifierHash || widgetAuthToken) {
dispatch('conversation/clearConversations', {}, { root: true });
dispatch('conversation/fetchOldConversations', {}, { root: true });
dispatch('conversationAttributes/getAttributes', {}, { root: true });
}
} catch (error) {
const data =
error && error.response && error.response.data
? error.response.data
: error;
IFrameHelper.sendMessage({
event: 'error',
errorType: SET_USER_ERROR,
data,
});
const data = parseErrorData(error);
sendMessage({ event: 'error', errorType: SET_USER_ERROR, data });
}
},
setCustomAttributes: async (_, customAttributes = {}) => {

View file

@ -1,21 +1,26 @@
import { API } from 'widget/helpers/axios';
import { sendMessage } from 'widget/helpers/utils';
import { actions } from '../../contacts';
const commit = jest.fn();
const dispatch = jest.fn();
jest.mock('widget/helpers/axios');
jest.mock('widget/helpers/utils', () => ({
sendMessage: jest.fn(),
}));
describe('#actions', () => {
describe('#update', () => {
it('sends correct actions', async () => {
describe('#setUser', () => {
it('sends correct actions if contact object is refreshed ', async () => {
const user = {
email: 'thoma@sphadikam.com',
name: 'Adu Thoma',
avatar_url: '',
identifier_hash: 'random_hex_identifier_hash',
};
API.patch.mockResolvedValue({ data: { pubsub_token: 'token' } });
await actions.update({ commit, dispatch }, { identifier: 1, user });
API.patch.mockResolvedValue({ data: { widget_auth_token: 'token' } });
await actions.setUser({ commit, dispatch }, { identifier: 1, user });
expect(sendMessage.mock.calls).toEqual([
[{ data: { widgetAuthToken: 'token' }, event: 'setAuthCookie' }],
]);
expect(commit.mock.calls).toEqual([]);
expect(dispatch.mock.calls).toEqual([
['get'],
@ -24,5 +29,52 @@ describe('#actions', () => {
['conversationAttributes/getAttributes', {}, { root: true }],
]);
});
it('sends correct actions if identifierHash is passed ', async () => {
const user = {
email: 'thoma@sphadikam.com',
name: 'Adu Thoma',
avatar_url: '',
identifier_hash: '12345',
};
API.patch.mockResolvedValue({ data: { id: 1 } });
await actions.setUser({ commit, dispatch }, { identifier: 1, user });
expect(sendMessage.mock.calls).toEqual([]);
expect(commit.mock.calls).toEqual([]);
expect(dispatch.mock.calls).toEqual([
['get'],
['conversation/clearConversations', {}, { root: true }],
['conversation/fetchOldConversations', {}, { root: true }],
['conversationAttributes/getAttributes', {}, { root: true }],
]);
});
it('does not call sendMessage if contact object is not refreshed ', async () => {
const user = {
email: 'thoma@sphadikam.com',
name: 'Adu Thoma',
avatar_url: '',
};
API.patch.mockResolvedValue({ data: { id: 1 } });
await actions.setUser({ commit, dispatch }, { identifier: 1, user });
expect(sendMessage.mock.calls).toEqual([]);
expect(commit.mock.calls).toEqual([]);
expect(dispatch.mock.calls).toEqual([['get']]);
});
});
describe('#update', () => {
it('sends correct actions', async () => {
const user = {
email: 'thoma@sphadikam.com',
name: 'Adu Thoma',
avatar_url: '',
identifier_hash: 'random_hex_identifier_hash',
};
API.patch.mockResolvedValue({ data: { id: 1 } });
await actions.update({ commit, dispatch }, { identifier: 1, user });
expect(commit.mock.calls).toEqual([]);
expect(dispatch.mock.calls).toEqual([['get']]);
});
});
});

View file

@ -0,0 +1,5 @@
json.id @contact.id
json.name @contact.name
json.email @contact.email
json.phone_number @contact.phone_number
json.widget_auth_token @widget_auth_token if @widget_auth_token.present?

View file

@ -199,6 +199,7 @@ Rails.application.routes.draw do
resource :contact, only: [:show, :update] do
collection do
post :destroy_custom_attributes
patch :set_user
end
end
resources :inbox_members, only: [:index]

View file

@ -48,13 +48,22 @@ describe ::ContactIdentifyAction do
context 'when contact with same email exists' do
it 'merges the current contact to email contact' do
existing_email_contact = create(:contact, account: account, email: 'test@test.com')
params = { email: 'test@test.com' }
existing_email_contact = create(:contact, account: account, email: 'test@test.com', name: 'old name')
params = { name: 'new name', email: 'test@test.com' }
result = described_class.new(contact: contact, params: params).perform
expect(result.id).to eq existing_email_contact.id
expect(result.name).to eq existing_email_contact.name
expect(result.name).to eq 'new name'
expect { contact.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'will not merge the current contact to email contact if identifier of email contact is different' do
existing_email_contact = create(:contact, account: account, identifier: '1', email: 'test@test.com')
params = { identifier: '2', email: 'test@test.com' }
result = described_class.new(contact: contact, params: params).perform
expect(result.id).not_to eq existing_email_contact.id
expect(result.identifier).to eq params[:identifier]
expect(result.email).to eq nil
end
end
context 'when contact with same phone_number exists' do
@ -66,6 +75,24 @@ describe ::ContactIdentifyAction do
expect(result.name).to eq existing_phone_number_contact.name
expect { contact.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'will not merge the current contact to phone contact if identifier of phone contact is different' do
existing_phone_number_contact = create(:contact, account: account, identifier: '1', phone_number: '+919999888877')
params = { identifier: '2', phone_number: '+919999888877' }
result = described_class.new(contact: contact, params: params).perform
expect(result.id).not_to eq existing_phone_number_contact.id
expect(result.identifier).to eq params[:identifier]
expect(result.email).to eq nil
end
it 'will not overide the phone contacts email when params contains different email' do
existing_phone_number_contact = create(:contact, account: account, email: '1@test.com', phone_number: '+919999888877')
params = { email: '2@test.com', phone_number: '+919999888877' }
result = described_class.new(contact: contact, params: params).perform
expect(result.id).not_to eq existing_phone_number_contact.id
expect(result.email).to eq params[:email]
expect(result.phone_number).to eq nil
end
end
context 'when contacts with blank identifiers exist and identify action is called with blank identifier' do
@ -77,5 +104,16 @@ describe ::ContactIdentifyAction do
expect(result.name).to eq 'new name'
end
end
context 'when retain_original_contact_name is set to true' do
it 'will not update the name of the existing contact' do
existing_email_contact = create(:contact, account: account, name: 'old name', email: 'test@test.com')
params = { email: 'test@test.com', name: 'new name' }
result = described_class.new(contact: contact, params: params, retain_original_contact_name: true).perform
expect(result.id).to eq existing_email_contact.id
expect(result.name).to eq 'old name'
expect { contact.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
end

View file

@ -82,12 +82,34 @@ RSpec.describe '/api/v1/widget/contacts', type: :request do
expect(response).to have_http_status(:success)
end
end
end
describe 'PATCH /api/v1/widget/contact/set_user' do
let(:params) { { website_token: web_widget.website_token, identifier: 'test' } }
let(:web_widget) { create(:channel_widget, account: account, hmac_mandatory: true) }
let(:correct_identifier_hash) { OpenSSL::HMAC.hexdigest('sha256', web_widget.hmac_token, params[:identifier].to_s) }
let(:incorrect_identifier_hash) { 'test' }
context 'when the current contact identifier is different from param identifier' do
before do
contact.update(identifier: 'random')
end
it 'return a new contact for the provided identifier' do
patch '/api/v1/widget/contact/set_user',
params: params.merge(identifier_hash: correct_identifier_hash),
headers: { 'X-Auth-Token' => token },
as: :json
body = JSON.parse(response.body)
expect(body['id']).not_to eq(contact.id)
expect(body['widget_auth_token']).not_to eq(nil)
expect(Contact.find(body['id']).contact_inboxes.first.hmac_verified?).to eq(true)
end
end
context 'with mandatory hmac' do
let(:identify_action) { double }
let(:web_widget) { create(:channel_widget, account: account, hmac_mandatory: true) }
let(:correct_identifier_hash) { OpenSSL::HMAC.hexdigest('sha256', web_widget.hmac_token, params[:identifier].to_s) }
let(:incorrect_identifier_hash) { 'test' }
before do
allow(ContactIdentifyAction).to receive(:new).and_return(identify_action)
@ -95,7 +117,7 @@ RSpec.describe '/api/v1/widget/contacts', type: :request do
end
it 'returns success when correct identifier hash is provided' do
patch '/api/v1/widget/contact',
patch '/api/v1/widget/contact/set_user',
params: params.merge(identifier_hash: correct_identifier_hash),
headers: { 'X-Auth-Token' => token },
as: :json
@ -104,7 +126,7 @@ RSpec.describe '/api/v1/widget/contacts', type: :request do
end
it 'returns error when incorrect identifier hash is provided' do
patch '/api/v1/widget/contact',
patch '/api/v1/widget/contact/set_user',
params: params.merge(identifier_hash: incorrect_identifier_hash),
headers: { 'X-Auth-Token' => token },
as: :json
@ -113,7 +135,7 @@ RSpec.describe '/api/v1/widget/contacts', type: :request do
end
it 'returns error when identifier hash is blank' do
patch '/api/v1/widget/contact',
patch '/api/v1/widget/contact/set_user',
params: params.merge(identifier_hash: ''),
headers: { 'X-Auth-Token' => token },
as: :json
@ -122,7 +144,7 @@ RSpec.describe '/api/v1/widget/contacts', type: :request do
end
it 'returns error when identifier hash is not provided' do
patch '/api/v1/widget/contact',
patch '/api/v1/widget/contact/set_user',
params: params,
headers: { 'X-Auth-Token' => token },
as: :json