From 77657fd41bfb106170c35a5a60007ee32e73b58e Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Mon, 4 Dec 2023 16:52:43 +0100 Subject: [PATCH 1/4] Handle freeform message not allowed errors - by sending a message template... We fetch the failed message and attempt to find the message in the db by its text. If found, a template is scheduled to be sent out from this message's info --- .../whats_app_adapter/twilio_outbound.rb | 16 +++---- .../whats_app/webhook_controller.rb | 12 ++++++ spec/requests/whats_app/webhook_spec.rb | 42 +++++++++++++++++++ 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/app/adapters/whats_app_adapter/twilio_outbound.rb b/app/adapters/whats_app_adapter/twilio_outbound.rb index 1fa0986f5..22c90d0b2 100644 --- a/app/adapters/whats_app_adapter/twilio_outbound.rb +++ b/app/adapters/whats_app_adapter/twilio_outbound.rb @@ -10,7 +10,7 @@ def send!(message) if freeform_message_permitted?(recipient) send_message(recipient, message) else - send_message_template(recipient, message) + send_message_template!(recipient, message) end end @@ -50,6 +50,13 @@ def send_resubscribe_error_message!(contributor) text: I18n.t('adapter.shared.resubscribe.failure')) end + def send_message_template!(recipient, message) + recipient.update(whats_app_message_template_sent_at: Time.current) + text = I18n.t("adapter.whats_app.request_template.new_request_#{time_of_day}_#{rand(1..3)}", first_name: recipient.first_name, + request_title: message.request.title) + WhatsAppAdapter::Outbound::Text.perform_later(contributor_id: recipient.id, text: text) + end + private def contributor_can_receive_messages?(recipient) @@ -83,13 +90,6 @@ def freeform_message_permitted?(recipient) responding_to_template_message || latest_message_received_within_last_24_hours end - def send_message_template(recipient, message) - recipient.update(whats_app_message_template_sent_at: Time.current) - text = I18n.t("adapter.whats_app.request_template.new_request_#{time_of_day}_#{rand(1..3)}", first_name: recipient.first_name, - request_title: message.request.title) - WhatsAppAdapter::Outbound::Text.perform_later(contributor_id: recipient.id, text: text) - end - def send_message(recipient, message) files = message.files diff --git a/app/controllers/whats_app/webhook_controller.rb b/app/controllers/whats_app/webhook_controller.rb index 3b58b3424..94c05b39d 100644 --- a/app/controllers/whats_app/webhook_controller.rb +++ b/app/controllers/whats_app/webhook_controller.rb @@ -7,6 +7,7 @@ class WebhookController < ApplicationController skip_before_action :require_login, :verify_authenticity_token UNSUCCESSFUL_DELIVERY = %w[undelivered failed].freeze INVALID_MESSAGE_RECIPIENT_ERROR_CODE = 63_024 # https://www.twilio.com/docs/api/errors/63024 + FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE = 63_016 # https://www.twilio.com/docs/api/errors/63016 def message head :ok @@ -71,6 +72,9 @@ def status MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id) return end + if status_params['ErrorCode'].to_i.eql?(FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE) + handle_freeform_message_not_allowed_error(contributor, status_params['MessageSid']) + end exception = WhatsAppAdapter::MessageDeliveryUnsuccessfulError.new(status: status_params['MessageStatus'], whats_app_phone_number: whats_app_phone_number, message: status_params['ErrorMessage']) @@ -124,5 +128,13 @@ def fetch_message_from_twilio(twilio_message_sid) ErrorNotifier.report(e) nil end + + def handle_freeform_message_not_allowed_error(contributor, twilio_message_sid) + message_text = fetch_message_from_twilio(twilio_message_sid) + message = Message.find_by(text: message_text) + return unless message + + WhatsAppAdapter::TwilioOutbound.send_message_template!(contributor, message) + end end end diff --git a/spec/requests/whats_app/webhook_spec.rb b/spec/requests/whats_app/webhook_spec.rb index 413e292c3..55af6749d 100644 --- a/spec/requests/whats_app/webhook_spec.rb +++ b/spec/requests/whats_app/webhook_spec.rb @@ -286,6 +286,48 @@ end } end + + context 'due to a freeform message not allowed error' do + let!(:request) do + create(:request, title: 'I failed to send', text: 'Hey {{FIRST_NAME}}, because it was sent outside the allowed window') + end + let(:valid_account_sid) { 'VALID_ACCOUNT_SID' } + let(:valid_api_key_sid) { 'VALID_API_KEY_SID' } + let(:valid_api_key_secret) { 'VALID_API_KEY_SECRET' } + let(:mock_twilio_rest_client) { instance_double(Twilio::REST::Client) } + let(:messages_double) { double(Twilio::REST::Api::V2010::AccountContext::MessageInstance, body: body_text) } + let(:body_text) { 'no message with this text saved' } + + before do + subject.call + allow(Twilio::REST::Client).to receive(:new).and_return(mock_twilio_rest_client) + allow(mock_twilio_rest_client).to receive(:messages).with('someSid').and_return(messages_double) + allow(messages_double).to receive(:fetch).and_return(messages_double) + end + + it 'reports it as an error, as we want to track specifics to when this occurs' do + expect(Sentry).to receive(:capture_exception).with(exception) + + subject.call + end + + it 'given message cannot be found by Twilio message sid body' do + expect { subject.call }.not_to have_enqueued_job(WhatsAppAdapter::Outbound::Text) + end + + describe 'given a message is found by Twilio message sid body' do + let!(:message) { create(:message, text: body_text, request: request) } + let(:body_text) { "Hey #{contributor.first_name}, because it was sent outside the allowed window" } + + it 'enqueues the Text job with WhatsApp template' do + expect { subject.call }.to(have_enqueued_job(WhatsAppAdapter::Outbound::Text).on_queue('default').with do |params| + expect(params[:contributor_id]).to eq(contributor.id) + expect(params[:text]).to include(contributor.first_name) + expect(params[:text]).to include(message.request.title) + end) + end + end + end end end end From bfc255f05b40d09e43e44e77c80c93c1be8327df Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Tue, 5 Dec 2023 18:24:58 +0100 Subject: [PATCH 2/4] Fix failing spec, extend, and improve them --- .../whats_app/webhook_controller.rb | 2 +- spec/requests/whats_app/webhook_spec.rb | 46 ++++++++++++------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/controllers/whats_app/webhook_controller.rb b/app/controllers/whats_app/webhook_controller.rb index 94c05b39d..df3db3629 100644 --- a/app/controllers/whats_app/webhook_controller.rb +++ b/app/controllers/whats_app/webhook_controller.rb @@ -72,7 +72,7 @@ def status MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id) return end - if status_params['ErrorCode'].to_i.eql?(FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE) + if status_params['ErrorCode'].to_i.eql?(FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE) && status_params['MessageStatus'].eql?('failed') handle_freeform_message_not_allowed_error(contributor, status_params['MessageSid']) end exception = WhatsAppAdapter::MessageDeliveryUnsuccessfulError.new(status: status_params['MessageStatus'], diff --git a/spec/requests/whats_app/webhook_spec.rb b/spec/requests/whats_app/webhook_spec.rb index 55af6749d..49f352ddf 100644 --- a/spec/requests/whats_app/webhook_spec.rb +++ b/spec/requests/whats_app/webhook_spec.rb @@ -202,8 +202,8 @@ 'ChannelInstallSid' => 'someChannelInstallSid', 'ChannelPrefix' => 'whatsapp', 'ChannelToAddress' => whats_app_phone_number.to_s, - 'ErrorCode' => '63016', - 'ErrorMessage' => freeform_message_not_allowed_error_message, + 'ErrorCode' => '60228', + 'ErrorMessage' => 'Template was not found', 'From' => "whatsapp:#{Setting.whats_app_server_phone_number}", 'MessageSid' => 'someSid', 'MessageStatus' => 'failed', @@ -213,9 +213,7 @@ 'To' => "whatsapp:#{whats_app_phone_number}" } end - let(:freeform_message_not_allowed_error_message) do - 'Twilio Error: Failed to send freeform message because you are outside the allowed window.. Generated new message with sid: someSid' - end + let(:exception) do WhatsAppAdapter::MessageDeliveryUnsuccessfulError.new(status: params['MessageStatus'], whats_app_phone_number: whats_app_phone_number, message: params['ErrorMessage']) @@ -260,6 +258,21 @@ describe 'given a known contributor' do let!(:contributor) { create(:contributor, whats_app_phone_number: whats_app_phone_number) } + let!(:request) do + create(:request, title: 'I failed to send', text: 'Hey {{FIRST_NAME}}, because it was sent outside the allowed window') + end + let(:valid_account_sid) { 'VALID_ACCOUNT_SID' } + let(:valid_api_key_sid) { 'VALID_API_KEY_SID' } + let(:valid_api_key_secret) { 'VALID_API_KEY_SECRET' } + let(:mock_twilio_rest_client) { instance_double(Twilio::REST::Client) } + let(:messages_double) { double(Twilio::REST::Api::V2010::AccountContext::MessageInstance, body: body_text) } + let(:body_text) { 'no message with this text saved' } + + before do + allow(Twilio::REST::Client).to receive(:new).and_return(mock_twilio_rest_client) + allow(mock_twilio_rest_client).to receive(:messages).with('someSid').and_return(messages_double) + allow(messages_double).to receive(:fetch).and_return(messages_double) + end describe 'given a failed message delivery' do it 'reports the error with the error message' do @@ -288,21 +301,13 @@ end context 'due to a freeform message not allowed error' do - let!(:request) do - create(:request, title: 'I failed to send', text: 'Hey {{FIRST_NAME}}, because it was sent outside the allowed window') + let(:freeform_message_not_allowed_error_message) do + 'Twilio Error: Failed to send freeform message because you are outside the allowed window.' end - let(:valid_account_sid) { 'VALID_ACCOUNT_SID' } - let(:valid_api_key_sid) { 'VALID_API_KEY_SID' } - let(:valid_api_key_secret) { 'VALID_API_KEY_SECRET' } - let(:mock_twilio_rest_client) { instance_double(Twilio::REST::Client) } - let(:messages_double) { double(Twilio::REST::Api::V2010::AccountContext::MessageInstance, body: body_text) } - let(:body_text) { 'no message with this text saved' } before do - subject.call - allow(Twilio::REST::Client).to receive(:new).and_return(mock_twilio_rest_client) - allow(mock_twilio_rest_client).to receive(:messages).with('someSid').and_return(messages_double) - allow(messages_double).to receive(:fetch).and_return(messages_double) + params['ErrorCode'] = '63016' + params['ErrorMessage'] = freeform_message_not_allowed_error_message end it 'reports it as an error, as we want to track specifics to when this occurs' do @@ -326,6 +331,13 @@ expect(params[:text]).to include(message.request.title) end) end + + context 'given an undelivered status' do + before { params['MessageStatus'] = 'undelivered' } + it 'is expected not to schedule a job as it would send out twice, one for undelivered and one for failed' do + expect { subject.call }.not_to have_enqueued_job(WhatsAppAdapter::Outbound::Text) + end + end end end end From 3fa93417b635192e6fe228b20a7e274bc508a8a5 Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Tue, 5 Dec 2023 18:36:14 +0100 Subject: [PATCH 3/4] Improve organization of code --- .../whats_app/webhook_controller.rb | 41 +++++++++++-------- spec/requests/whats_app/webhook_spec.rb | 1 + 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/app/controllers/whats_app/webhook_controller.rb b/app/controllers/whats_app/webhook_controller.rb index df3db3629..1d04b4c29 100644 --- a/app/controllers/whats_app/webhook_controller.rb +++ b/app/controllers/whats_app/webhook_controller.rb @@ -5,6 +5,8 @@ class WebhookController < ApplicationController include WhatsAppHandleCallbacks skip_before_action :require_login, :verify_authenticity_token + before_action :set_contributor, only: :status + UNSUCCESSFUL_DELIVERY = %w[undelivered failed].freeze INVALID_MESSAGE_RECIPIENT_ERROR_CODE = 63_024 # https://www.twilio.com/docs/api/errors/63024 FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE = 63_016 # https://www.twilio.com/docs/api/errors/63016 @@ -62,23 +64,7 @@ def errors def status head :ok - return unless status_params['MessageStatus'].in?(UNSUCCESSFUL_DELIVERY) - - whats_app_phone_number = status_params['To'].split('whatsapp:').last - contributor = Contributor.find_by(whats_app_phone_number: whats_app_phone_number) - return unless contributor - - if status_params['ErrorCode'].to_i.eql?(INVALID_MESSAGE_RECIPIENT_ERROR_CODE) - MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id) - return - end - if status_params['ErrorCode'].to_i.eql?(FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE) && status_params['MessageStatus'].eql?('failed') - handle_freeform_message_not_allowed_error(contributor, status_params['MessageSid']) - end - exception = WhatsAppAdapter::MessageDeliveryUnsuccessfulError.new(status: status_params['MessageStatus'], - whats_app_phone_number: whats_app_phone_number, - message: status_params['ErrorMessage']) - ErrorNotifier.report(exception, context: { message_sid: status_params['MessageSid'] }) + handle_unsuccessful_delivery if status_params['MessageStatus'].in?(UNSUCCESSFUL_DELIVERY) end private @@ -99,6 +85,11 @@ def status_params :EventType, :From, :MessageSid, :MessageStatus, :SmsSid, :SmsStatus, :StructuredMessage, :To) end + def set_contributor + whats_app_phone_number = status_params['To'].split('whatsapp:').last + @contributor = Contributor.find_by(whats_app_phone_number: whats_app_phone_number) + end + def handle_unknown_contributor(whats_app_phone_number) exception = WhatsAppAdapter::UnknownContributorError.new(whats_app_phone_number: whats_app_phone_number) ErrorNotifier.report(exception) @@ -136,5 +127,21 @@ def handle_freeform_message_not_allowed_error(contributor, twilio_message_sid) WhatsAppAdapter::TwilioOutbound.send_message_template!(contributor, message) end + + def handle_unsuccessful_delivery + return unless @contributor + + if status_params['ErrorCode'].to_i.eql?(INVALID_MESSAGE_RECIPIENT_ERROR_CODE) + MarkInactiveContributorInactiveJob.perform_later(contributor_id: @contributor.id) + return + end + if status_params['ErrorCode'].to_i.eql?(FREEFORM_MESSAGE_NOT_ALLOWED_ERROR_CODE) && status_params['MessageStatus'].eql?('failed') + handle_freeform_message_not_allowed_error(@contributor, status_params['MessageSid']) + end + exception = WhatsAppAdapter::MessageDeliveryUnsuccessfulError.new(status: status_params['MessageStatus'], + whats_app_phone_number: @contributor.whats_app_phone_number, + message: status_params['ErrorMessage']) + ErrorNotifier.report(exception, context: { message_sid: status_params['MessageSid'] }) + end end end diff --git a/spec/requests/whats_app/webhook_spec.rb b/spec/requests/whats_app/webhook_spec.rb index 49f352ddf..b965cd344 100644 --- a/spec/requests/whats_app/webhook_spec.rb +++ b/spec/requests/whats_app/webhook_spec.rb @@ -318,6 +318,7 @@ it 'given message cannot be found by Twilio message sid body' do expect { subject.call }.not_to have_enqueued_job(WhatsAppAdapter::Outbound::Text) + expect { subject.call }.not_to raise_error end describe 'given a message is found by Twilio message sid body' do From 5a58fbdfdd31a5016ef83f6d44c039df849449d6 Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Tue, 5 Dec 2023 18:59:02 +0100 Subject: [PATCH 4/4] Fix the scope of variables/mocks --- spec/requests/whats_app/webhook_spec.rb | 27 +++++++++++-------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/spec/requests/whats_app/webhook_spec.rb b/spec/requests/whats_app/webhook_spec.rb index b965cd344..8cbfda1c1 100644 --- a/spec/requests/whats_app/webhook_spec.rb +++ b/spec/requests/whats_app/webhook_spec.rb @@ -258,21 +258,6 @@ describe 'given a known contributor' do let!(:contributor) { create(:contributor, whats_app_phone_number: whats_app_phone_number) } - let!(:request) do - create(:request, title: 'I failed to send', text: 'Hey {{FIRST_NAME}}, because it was sent outside the allowed window') - end - let(:valid_account_sid) { 'VALID_ACCOUNT_SID' } - let(:valid_api_key_sid) { 'VALID_API_KEY_SID' } - let(:valid_api_key_secret) { 'VALID_API_KEY_SECRET' } - let(:mock_twilio_rest_client) { instance_double(Twilio::REST::Client) } - let(:messages_double) { double(Twilio::REST::Api::V2010::AccountContext::MessageInstance, body: body_text) } - let(:body_text) { 'no message with this text saved' } - - before do - allow(Twilio::REST::Client).to receive(:new).and_return(mock_twilio_rest_client) - allow(mock_twilio_rest_client).to receive(:messages).with('someSid').and_return(messages_double) - allow(messages_double).to receive(:fetch).and_return(messages_double) - end describe 'given a failed message delivery' do it 'reports the error with the error message' do @@ -301,11 +286,23 @@ end context 'due to a freeform message not allowed error' do + let!(:request) do + create(:request, title: 'I failed to send', text: 'Hey {{FIRST_NAME}}, because it was sent outside the allowed window') + end + let(:valid_account_sid) { 'VALID_ACCOUNT_SID' } + let(:valid_api_key_sid) { 'VALID_API_KEY_SID' } + let(:valid_api_key_secret) { 'VALID_API_KEY_SECRET' } + let(:mock_twilio_rest_client) { instance_double(Twilio::REST::Client) } + let(:messages_double) { double(Twilio::REST::Api::V2010::AccountContext::MessageInstance, body: body_text) } + let(:body_text) { 'no message with this text saved' } let(:freeform_message_not_allowed_error_message) do 'Twilio Error: Failed to send freeform message because you are outside the allowed window.' end before do + allow(Twilio::REST::Client).to receive(:new).and_return(mock_twilio_rest_client) + allow(mock_twilio_rest_client).to receive(:messages).with('someSid').and_return(messages_double) + allow(messages_double).to receive(:fetch).and_return(messages_double) params['ErrorCode'] = '63016' params['ErrorMessage'] = freeform_message_not_allowed_error_message end