Merge pull request #1742 from tactilenews/1741_add_missing_specs_twil…

…io_webhooks_refactor_to_dry_deactivate_logic

app/adapters/postmark_adapter/outbound.rb

@@ -11,11 +11,7 @@ class Outbound < ApplicationMailer
         contributor = Contributor.find_by(email: email_address)
         next unless contributor
 
-        contributor.update(deactivated_at: Time.current)
-        ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-        User.admin.find_each do |admin|
-          contributor_marked_as_inactive!(admin, contributor)
-        end
+        MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id)
       end
     end
 

app/adapters/signal_adapter/api.rb

@@ -13,7 +13,7 @@ def perform_request(request, contributor)
           yield response if block_given?
         else
           error_message = JSON.parse(response.body)['error']
-          mark_contributor_as_inactive(contributor) if error_message.match?(/Unregistered user/)
+          MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id) if error_message.match?(/Unregistered user/)
           exception = SignalAdapter::BadRequestError.new(error_code: response.code, message: error_message)
           context = {
             code: response.code,
@@ -24,16 +24,6 @@ def perform_request(request, contributor)
           ErrorNotifier.report(exception, context: context)
         end
       end
-
-      private
-
-      def mark_contributor_as_inactive(contributor)
-        contributor.update(deactivated_at: Time.current)
-        ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-        User.admin.find_each do |admin|
-          PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
-        end
-      end
     end
   end
 end

app/adapters/telegram_adapter/outbound/photo.rb

@@ -10,11 +10,7 @@ class Photo < ApplicationJob
         contributor = message&.recipient
         return unless contributor
 
-        contributor.update(deactivated_at: Time.current)
-        ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-        User.admin.find_each do |admin|
-          PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
-        end
+        MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id)
       end
 
       attr_reader :telegram_id, :message

app/adapters/telegram_adapter/outbound/text.rb

@@ -10,11 +10,7 @@ class Text < ApplicationJob
         contributor = message&.recipient
         return unless contributor
 
-        contributor.update(deactivated_at: Time.current)
-        ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-        User.admin.find_each do |admin|
-          PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
-        end
+        MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id)
       end
 
       def perform(contributor_id:, text:, message: nil)  # rubocop:disable Lint/UnusedMethodArgument

app/adapters/threema_adapter/outbound/file.rb

@@ -13,12 +13,7 @@ class File < ApplicationJob
           contributor = Contributor.where('lower(threema_id) = ?', threema_id.downcase).first
           return unless contributor
 
-          contributor.deactivated_at = Time.current
-          contributor.save(validate: false)
-          ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-          User.admin.find_each do |admin|
-            PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
-          end
+          MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id)
         end
         ErrorNotifier.report(exception, tags: tags)
       end

app/adapters/threema_adapter/outbound/text.rb

@@ -13,12 +13,7 @@ class Text < ApplicationJob
           contributor = Contributor.where('lower(threema_id) = ?', threema_id.downcase).first
           return unless contributor
 
-          contributor.deactivated_at = Time.current
-          contributor.save(validate: false)
-          ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-          User.admin.find_each do |admin|
-            PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
-          end
+          MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id)
         end
         ErrorNotifier.report(exception, tags: tags)
       end

app/adapters/whats_app_adapter/twilio_error.rb

@@ -2,8 +2,11 @@
 
 module WhatsAppAdapter
   class TwilioError < StandardError
-    def initialize(error_code:)
-      super("Error occurred for WhatsApp with error code: #{error_code}")
+    def initialize(error_code:, message: nil, url: nil)
+      error_text = "Error occurred for WhatsApp with error code: #{error_code}"
+      error_text.concat(" with message: #{message}") if message
+      error_text.concat(" originated from url #{url}") if url
+      super(error_text)
     end
   end
 end

app/controllers/whats_app/webhook_controller.rb

@@ -9,6 +9,7 @@ class WebhookController < ApplicationController
     INVALID_MESSAGE_RECIPIENT_ERROR_CODE = 63_024 # https://www.twilio.com/docs/api/errors/63024
 
     def message
+      head :ok
       adapter = WhatsAppAdapter::TwilioInbound.new
 
       adapter.on(WhatsAppAdapter::TwilioInbound::UNKNOWN_CONTRIBUTOR) do |whats_app_phone_number|
@@ -40,17 +41,21 @@ def message
     end
 
     def errors
+      head :ok
       return unless error_params['Level'] == 'ERROR'
 
-      payload = JSON.parse(error_params['Payload'])
-      parameters = payload.with_indifferent_access.dig(:webhook, :request, :parameters)
-      exception = WhatsAppAdapter::TwilioError.new(error_code: payload['error_code'])
+      payload = JSON.parse(error_params['Payload']).deep_transform_keys(&:underscore).with_indifferent_access
+      parameters = payload.dig(:webhook, :request, :parameters)
+      more_info = payload[:more_info]
+      message = more_info&.dig(:msg)
+      url = more_info&.dig(:url)
+      exception = WhatsAppAdapter::TwilioError.new(error_code: payload['error_code'], message: message, url: url)
       ErrorNotifier.report(exception,
                            context: {
                              channel_to_address: parameters&.dig(:channelToAddress),
-                             more_info: payload['more_info'],
+                             more_info: more_info,
                              error_sid: error_params['Sid'],
-                             message_sid: parameters&.dig(:messageSid)
+                             message_sid: parameters&.dig(:message_sid)
                            })
     end
 
@@ -59,8 +64,11 @@ def status
       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)
-        handle_invalid_message_recipient(whats_app_phone_number)
+        MarkInactiveContributorInactiveJob.perform_later(contributor_id: contributor.id)
         return
       end
       exception = WhatsAppAdapter::MessageDeliveryUnsuccessfulError.new(status: status_params['MessageStatus'],
@@ -99,17 +107,6 @@ def handle_request_to_receive_message(contributor, twilio_message_sid)
       WhatsAppAdapter::TwilioOutbound.send!(message || contributor.received_messages.first)
     end
 
-    def handle_invalid_message_recipient(whats_app_phone_number)
-      contributor = Contributor.find_by(whats_app_phone_number: whats_app_phone_number)
-      return unless contributor
-
-      contributor.update(deactivated_at: Time.current)
-      ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
-      User.admin.find_each do |admin|
-        PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
-      end
-    end
-
     def send_requested_message(contributor, twilio_message_sid)
       message_text = fetch_message_from_twilio(twilio_message_sid)
 

app/jobs/mark_inactive_contributor_inactive_job.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class MarkInactiveContributorInactiveJob < ApplicationJob
+  queue_as :mark_inactive_contributor_inactive
+
+  def perform(contributor_id:)
+    contributor = Contributor.where(id: contributor_id).first
+    return unless contributor
+
+    contributor.deactivated_at = Time.current
+    contributor.save(validate: false)
+    ContributorMarkedInactive.with(contributor_id: contributor.id).deliver_later(User.all)
+    User.admin.find_each do |admin|
+      PostmarkAdapter::Outbound.contributor_marked_as_inactive!(admin, contributor)
+    end
+  end
+end

spec/adapters/signal_adapter/api_spec.rb

@@ -52,23 +52,11 @@
 
           subject { -> { api.perform_request(request, recipient) } }
 
-          it 'marks the contributor as inactive' do
-            expect { subject.call }.to change { recipient.reload.deactivated_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
-          end
-
-          it_behaves_like 'an ActivityNotification', 'ContributorMarkedInactive'
-
-          it 'enqueues a job to inform admin' do
-            expect { subject.call }.to have_enqueued_job.on_queue('default').with(
-              'PostmarkAdapter::Outbound',
-              'contributor_marked_as_inactive_email',
-              'deliver_now', # How ActionMailer works in test environment, even though in production we call deliver_later
-              {
-                params: { admin: an_instance_of(User), contributor: recipient },
-                args: []
-              }
-            ).exactly(2).times
-          end
+          it {
+            is_expected.to have_enqueued_job(MarkInactiveContributorInactiveJob).with do |params|
+              expect(params[:contributor_id].to(eq(recipient.id)))
+            end
+          }
         end
       end
     end

spec/jobs/mark_inactive_contributor_inactive_job_spec.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe MarkInactiveContributorInactiveJob do
+  describe '#perform_later(contributor_id:)' do
+    subject { -> { described_class.new.perform(contributor_id: contributor.id) } }
+
+    let!(:admin) { create_list(:user, 2, admin: true) }
+    let!(:non_admin_user) { create(:user) }
+
+    context 'given an unknown contributor' do
+      let(:contributor) { Contributor.new(id: 12_345) }
+
+      # if a contributor is deleted from the db before the job is run,
+      # then we should not try to run the job, but exit early.
+      it { is_expected.not_to raise_error(NoMethodError) }
+    end
+
+    context 'given a known contributor' do
+      let(:contributor) { create(:contributor) }
+
+      it { is_expected.to change { contributor.reload.deactivated_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone)) }
+      it_behaves_like 'an ActivityNotification', 'ContributorMarkedInactive'
+      it 'enqueues a job to inform admin' do
+        expect { subject.call }.to have_enqueued_job.on_queue('default').with(
+          'PostmarkAdapter::Outbound',
+          'contributor_marked_as_inactive_email',
+          'deliver_now', # How ActionMailer works in test environment, even though in production we call deliver_later
+          {
+            params: { admin: an_instance_of(User), contributor: contributor },
+            args: []
+          }
+        ).exactly(2).times
+      end
+    end
+  end
+end

spec/requests/whats_app/webhook_spec.rb

@@ -79,6 +79,12 @@
         create(:message, request: request, recipient: contributor)
       end
 
+      it 'returns 200' do
+        subject.call
+
+        expect(response).to have_http_status(200)
+      end
+
       context 'no message template sent' do
         it 'creates a messsage' do
           expect { subject.call }.to change(Message, :count).from(2).to(3)
@@ -215,8 +221,116 @@
                                                             whats_app_phone_number: whats_app_phone_number, message: params['ErrorMessage'])
     end
 
-    describe 'given a failed message delivery' do
-      it 'reports the error with the error message' do
+    it 'returns 200' do
+      subject.call
+
+      expect(response).to have_http_status(200)
+    end
+
+    describe 'given an unknown contributor' do
+      it 'does not report it as an error, as it is not actionable' do
+        expect(Sentry).not_to receive(:capture_exception)
+
+        subject.call
+      end
+
+      context 'due to an invalid message recipient error' do
+        it { is_expected.not_to have_enqueued_job(MarkInactiveContributorInactiveJob) }
+      end
+    end
+
+    describe 'given a known contributor' do
+      let!(:contributor) { create(:contributor, whats_app_phone_number: whats_app_phone_number) }
+
+      describe 'given a failed message delivery' do
+        it 'reports the error with the error message' do
+          expect(Sentry).to receive(:capture_exception).with(exception)
+
+          subject.call
+        end
+
+        context 'due to an invalid message recipient error' do
+          before do
+            params['ErrorCode'] = '63024'
+            params['ErrorMessage'] = 'Twilio Error: Invalid message recipient. Generated new message with sid: someSid'
+          end
+
+          it 'does not report it as an error, as it is not actionable' do
+            expect(Sentry).not_to receive(:capture_exception)
+
+            subject.call
+          end
+
+          it {
+            is_expected.to have_enqueued_job(MarkInactiveContributorInactiveJob).with do |params|
+              expect(params[:contributor_id]).to eq(contributor.id)
+            end
+          }
+        end
+      end
+    end
+  end
+
+  describe '#errors' do
+    subject { -> { post whats_app_errors_path, params: params } }
+
+    let(:url) { 'https://example.100ey.es/whats_app/webhook' }
+    let(:error_payload) do
+      {
+        'error_code' => '21408',
+        'more_info' => {
+          'ErrorCode' => '21408',
+          'LogLevel' => 'ERROR',
+          'Msg' => 'Got HTTP 404 response to https://example.100ey.es/twilio/voice',
+          'url' => url
+        },
+        'webhook' => {
+          'request' => {
+            'url' => url,
+            'parameters' => {
+              'MessageSid' => 'someMessageSid'
+            }
+          }
+        }
+      }
+    end
+    let(:params) do
+      {
+        'AccountSid' => 'someAccountSid',
+        'Level' => 'ERROR',
+        'ParentAccountSid' => 'someParentAccountSid',
+        'Payload' => error_payload.to_json,
+        'PayloadType' => 'application/json',
+        'Sid' => 'someSid',
+        'Timestamp' => Time.current.to_i
+      }
+    end
+    let(:exception) do
+      WhatsAppAdapter::TwilioError.new(error_code: error_payload['error_code'],
+                                       message: error_payload['more_info']['Msg'],
+                                       url: error_payload['more_info']['url'])
+    end
+
+    it 'returns 200' do
+      subject.call
+
+      expect(response).to have_http_status(200)
+    end
+
+    it 'reports the error with error code, message, and url' do
+      expect(Sentry).to receive(:capture_exception).with(exception)
+
+      subject.call
+    end
+
+    context 'given more_info is not provided' do
+      before { error_payload.delete('more_info') }
+
+      let(:exception) do
+        WhatsAppAdapter::TwilioError.new(error_code: error_payload['error_code'])
+      end
+
+      it 'reports the error with error code' do
         expect(Sentry).to receive(:capture_exception).with(exception)
 
         subject.call