DRY out broadcast! method (#1946)

### What changed and Why

This PR reduces complexity and duplication by handling the message
creation in `BroadcastRequestJob` instead of only handling planned
requests there.

---------

Co-authored-by: Samuel Oey <so@capinside.com>

app/components/request_metrics/request_metrics.rb

@@ -21,7 +21,7 @@ def metrics
       [
         {
           value: stats[:counts][:contributors],
-          total: stats[:counts][:recipients],
+          total: request_for_info.organization.contributors.active.with_tags(request_for_info.tag_list).count,
           label: I18n.t('components.request_metrics.contributors', count: stats[:counts][:contributors]),
           icon: 'single-03'
         },

app/controllers/requests_controller.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 # TODO: Refactor to remove the need to disable rubocop
-# rubocop:disable Metrics/ClassLength
 class RequestsController < ApplicationController
   before_action :set_request, only: %i[show edit update notifications destroy messages_by_contributor stats]
   before_action :notifications_params, only: :notifications
@@ -22,17 +21,7 @@ def create
     resize_image_files if request_params[:files].present?
     @request = @organization.requests.new(request_params.merge(user: current_user))
     if @request.save
-      recipient_count = @request.organization.contributors.active.with_tags(@request.tag_list).count
-      if @request.planned?
-        redirect_to organization_requests_path(@organization, filter: :planned), flash: {
-          success: I18n.t('request.schedule_request_success',
-                          count: recipient_count,
-                          scheduled_datetime: I18n.l(@request.schedule_send_for, format: :long))
-        }
-      else
-        redirect_to organization_request_path(@organization.id, @request),
-                    flash: { success: I18n.t('request.success', count: recipient_count) }
-      end
+      trigger_broadcast_and_redirect(@request)
     else
       render :new, status: :unprocessable_entity
     end
@@ -47,10 +36,7 @@ def edit; end
   def update
     @request.files.purge_later if @request.files.attached? && request_params[:files].blank?
     if @request.update(request_params)
-      success_message, filter = update_request_redirect_specifics
-      redirect_to organization_requests_path(@request.organization_id, filter: filter), flash: {
-        success: success_message
-      }
+      trigger_broadcast_and_redirect(@request)
     else
       render :edit, status: :unprocessable_entity
     end
@@ -163,16 +149,18 @@ def filtered_requests
     end
   end
 
-  def update_request_redirect_specifics
-    if @request.planned?
-      [I18n.t('request.schedule_request_success',
-              count: @request.organization.contributors.active.with_tags(@request.tag_list).count,
-              scheduled_datetime: I18n.l(@request.schedule_send_for, format: :long)),
-       :planned]
+  def trigger_broadcast_and_redirect(request)
+    recipient_count = @request.organization.contributors.active.with_tags(request.tag_list).count
+    run_at = request.trigger_broadcast
+    if run_at
+      redirect_to organization_requests_path(@organization, filter: :planned), flash: {
+        success: I18n.t('request.schedule_request_success',
+                        count: recipient_count,
+                        scheduled_datetime: I18n.l(run_at, format: :long))
+      }
     else
-      [I18n.t('request.success', count: @request.organization.contributors.active.with_tags(@request.tag_list).count),
-       nil]
+      redirect_to organization_request_path(@organization.id, request),
+                  flash: { success: I18n.t('request.success', count: recipient_count) }
     end
   end
 end
-# rubocop:enable Metrics/ClassLength

app/jobs/broadcast_request_job.rb

@@ -7,11 +7,7 @@ def perform(request_id)
     request = Request.where(id: request_id).first
     return unless request
     return if request.broadcasted_at.present?
-
-    if request.planned? # rescheduled for future after this job was created
-      BroadcastRequestJob.delay(run_at: request.schedule_send_for).perform_later(request.id)
-      return
-    end
+    return if request.planned? # rescheduled for future
 
     request.organization.contributors.active.with_tags(request.tag_list).each do |contributor|
       message = Message.new(
@@ -22,7 +18,9 @@ def perform(request_id)
         broadcasted: true
       )
       message.files = Request.attach_files(request.files) if request.files.attached?
+
       message.save!
+      message.send!
     end
     request.update(broadcasted_at: Time.current)
   end

app/models/message.rb

@@ -36,7 +36,14 @@ class Message < ApplicationRecord
   validates :raw_data, presence: true, if: -> { sent_from_contributor? }
   validates :unknown_content, inclusion: { in: [true, false] }
 
-  after_create_commit :send_if_outbound, :notify_recipient
+  after_create_commit :notify_recipient
+
+  def send!
+    [PostmarkAdapter::Outbound, SignalAdapter::Outbound, TelegramAdapter::Outbound, ThreemaAdapter::Outbound,
+     WhatsAppAdapter::Delegator.new(organization)].each do |adapter|
+      adapter.send!(self)
+    end
+  end
 
   def reply?
     sent_from_contributor?
@@ -86,13 +93,4 @@ def notify_recipient
     end
   end
   # rubocop:enable Metrics/AbcSize
-
-  def send_if_outbound
-    return if manually_created? || reply?
-
-    [PostmarkAdapter::Outbound, SignalAdapter::Outbound, TelegramAdapter::Outbound, ThreemaAdapter::Outbound,
-     WhatsAppAdapter::Delegator.new(organization)].each do |adapter|
-      adapter.send!(self)
-    end
-  end
 end

app/models/request.rb

@@ -27,9 +27,7 @@ class Request < ApplicationRecord
   acts_as_taggable_on :tags
   acts_as_taggable_tenant :organization_id
 
-  after_create :broadcast_request
-
-  after_update_commit :broadcast_updated_request
+  after_create :notify_recipient
 
   delegate :replies, to: :messages
   delegate :outbound, to: :messages
@@ -49,6 +47,16 @@ def stats
     }
   end
 
+  def trigger_broadcast
+    if planned?
+      BroadcastRequestJob.delay(run_at: schedule_send_for).perform_later(id)
+      schedule_send_for
+    else
+      BroadcastRequestJob.perform_later(id)
+      nil
+    end
+  end
+
   def planned?
     schedule_send_for.present? && schedule_send_for > Time.current
   end
@@ -68,29 +76,6 @@ def messages_by_contributor
       .transform_values { |messages| messages.sort_by(&:created_at) }
   end
 
-  # rubocop:disable Metrics/AbcSize
-  def self.broadcast!(request)
-    if request.planned?
-      BroadcastRequestJob.delay(run_at: request.schedule_send_for).perform_later(request.id)
-      RequestScheduled.with(request_id: request.id,
-                            organization_id: request.organization.id).deliver_later(request.organization.users + User.admin.all)
-    else
-      request.organization.contributors.active.with_tags(request.tag_list).each do |contributor|
-        message = Message.new(
-          sender: request.user,
-          recipient: contributor,
-          text: request.personalized_text(contributor),
-          request: request,
-          broadcasted: true
-        )
-        message.files = attach_files(request.files) if request.files.attached?
-        message.save!
-      end
-      request.update(broadcasted_at: Time.current)
-    end
-  end
-  # rubocop:enable Metrics/AbcSize
-
   def self.attach_files(files)
     files.map do |file|
       message_file = Message::File.new
@@ -101,13 +86,10 @@ def self.attach_files(files)
 
   private
 
-  def broadcast_request
-    Request.broadcast!(self)
-  end
-
-  def broadcast_updated_request
-    return unless saved_change_to_schedule_send_for?
+  def notify_recipient
+    return unless planned?
 
-    Request.broadcast!(self)
+    RequestScheduled.with(request_id: id,
+                          organization_id: organization.id).deliver_later(organization.users + User.admin.all)
   end
 end

spec/jobs/broadcast_request_job_spec.rb

@@ -35,16 +35,6 @@
 
     context 'given a request has been rescheduled for the future' do
       before { request.update(schedule_send_for: 1.day.from_now) }
-      let(:expected_params) { { request_id: request.id } }
-
-      it 'enqueues a job to broadcast the request, and broadcast it when called again' do
-        expect { subject.call }.to change(DelayedJob, :count).from(0).to(1)
-        expect(Delayed::Job.last.run_at).to be_within(1.second).of(request.schedule_send_for)
-
-        Timecop.travel(1.day.from_now + 2.minutes)
-
-        expect { subject.call }.to change { request.reload.broadcasted_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
-      end
 
       it 'does not create a Message instance' do
         expect { subject.call }.not_to change(Message, :count)
@@ -54,5 +44,98 @@
         expect { subject.call }.not_to(change { request.reload.broadcasted_at })
       end
     end
+
+    context 'given a request that is to be sent out now' do
+      describe 'given contributors from multipile organizations' do
+        before(:each) do
+          create(:contributor, id: 1, email: 'somebody@example.org', organization: request.organization)
+          create(:contributor, id: 2, email: nil, telegram_id: 22, organization: request.organization)
+          create(:contributor, id: 3)
+        end
+
+        it "schedules jobs to send out message with contributor's channel" do
+          subject.call
+
+          expect(ActionMailer::MailDeliveryJob).to have_been_enqueued.with(
+            'PostmarkAdapter::Outbound',
+            'message_email',
+            'deliver_now', # How ActionMailer works in test environment, even though in production we call deliver_later
+            {
+              params: { message: request.messages.where(recipient_id: 1).first, organization: request.organization },
+              args: []
+            }
+          )
+          expect(TelegramAdapter::Outbound::Text).to have_been_enqueued.with({ organization_id: request.organization.id,
+                                                                               contributor_id: 2,
+                                                                               text: request.text,
+                                                                               message: request.messages.where(recipient_id: 2).first })
+        end
+
+        it 'only sends to contributors of the organization' do
+          expect { subject.call }.to change(Message, :count).from(0).to(2)
+                                                            .and (change { Message.pluck(:recipient_id).sort }).from([]).to([1, 2])
+        end
+
+        it 'assigns the user of the request as the sender of the message' do
+          expect { subject.call }.to (change { Message.pluck(:sender_id) }).from([]).to([request.user.id, request.user.id])
+        end
+
+        it 'marks the messages as broadcasted' do
+          expect { subject.call }.to (change { Message.pluck(:broadcasted) }).from([]).to([true, true])
+        end
+
+        context 'and has files attached' do
+          before do
+            request.files.attach(
+              io: Rails.root.join('example-image.png').open,
+              filename: 'example-image.png'
+            )
+          end
+
+          it 'attaches the files to the messages' do
+            expect { subject.call }.to (change { Message::File.count }).from(0).to(2)
+            Message.find_each do |message|
+              message.files.each do |file|
+                expect(file.attachment).to be_attached
+              end
+            end
+          end
+        end
+      end
+
+      describe 'given a request with a tag_list' do
+        let(:request) do
+          create(:request,
+                 title: 'Hitchhiker’s Guide',
+                 text: 'What is the answer to life, the universe, and everything?',
+                 tag_list: 'programmer',
+                 broadcasted_at: nil)
+        end
+
+        before do
+          create(:contributor, id: 4, organization: request.organization, tag_list: ['programmer'])
+          create(:contributor, id: 5, organization: request.organization, tag_list: ['something_else'])
+          create(:contributor, id: 6, organization: request.organization)
+        end
+
+        it 'only sends to contributors tagged with the tag' do
+          expect { subject.call }.to change(Message, :count).from(0).to(1)
+                                                            .and (change { Message.pluck(:recipient_id) }).from([]).to([4])
+        end
+      end
+
+      describe 'given contributors who are deactivated' do
+        before(:each) do
+          create(:contributor, :inactive, id: 7, organization: request.organization)
+          create(:contributor, id: 8, organization: request.organization)
+          create(:contributor, :inactive, id: 9, telegram_id: 24, organization: request.organization)
+        end
+
+        it 'only sends to active contributors' do
+          expect { subject.call }.to change(Message, :count).from(0).to(1)
+                                                            .and (change { Message.pluck(:recipient_id) }).from([]).to([8])
+        end
+      end
+    end
   end
 end

spec/models/message_spec.rb

@@ -90,10 +90,24 @@
     end
   end
 
+  describe '#send!' do
+    subject { message.send! }
+
+    let(:message) { create(:message, :outbound, recipient: create(:contributor, :whats_app_contributor)) }
+
+    it 'enqueues a job to send the message' do
+      expect { subject }.to(have_enqueued_job(WhatsAppAdapter::TwilioOutbound::Text).on_queue('default').with do |params|
+        expect(params[:organization_id]).to eq(message.organization_id)
+        expect(params[:contributor_id]).to eq(message.contributor.id)
+        expect(params[:text]).to include(message.contributor.first_name)
+        expect(params[:text]).to include(message.request.title)
+      end)
+    end
+  end
+
   describe '#after_commit(on: :commit)' do
     let!(:user) { create(:user) }
-    let!(:request) { create(:request, user: user, organization: organization) }
-    let(:message) { create(:message, sender: user, recipient: recipient, broadcasted: broadcasted, request: request) }
+    let!(:request) { create(:request, user: user, organization: organization, broadcasted_at: nil) }
     let(:organization) do
       create(:organization, name: '100eyes', telegram_bot_api_key: 'TELEGRAM_BOT_API_KEY', telegram_bot_username: 'USERNAME')
     end
@@ -112,9 +126,8 @@
 
       describe '#blocked' do
         subject do
-          perform_enqueued_jobs { message }
-          message.reload
-          message.blocked
+          perform_enqueued_jobs { BroadcastRequestJob.perform_later(request.id) }
+          request.messages.where(recipient_id: recipient.id).first.reload.blocked
         end
 
         it { should be(false) }
@@ -128,6 +141,7 @@
     describe '#notify_recipient' do
       subject { message }
 
+      let(:message) { create(:message, sender: user, recipient: recipient, broadcasted: broadcasted, request: request) }
       let!(:admin) { create(:user, admin: true) }
 
       before do