Merge pull request #1442 from Retrospring/refactor/send-to-followers-individually

Split up question worker to send to an individual user rather than to all followers

app/controllers/ajax/question_controller.rb

@@ -1,19 +1,6 @@
 # frozen_string_literal: true
 
 class Ajax::QuestionController < AjaxController
-  def destroy
-    params.require :question
-
-    UseCase::Question::Destroy.call(
-      question_id:  params[:question],
-      current_user: current_user
-    )
-
-    @response[:status] = :okay
-    @response[:message] = t(".success")
-    @response[:success] = true
-  end
-
   def create
     params.require :question
     params.require :anonymousQuestion
@@ -24,14 +11,14 @@ class Ajax::QuestionController < AjaxController
     @response = {
       success: true,
       message: t(".success"),
-      status:  :okay
+      status:  :okay,
     }
 
     if user_signed_in? && params[:rcpt] == "followers"
       UseCase::Question::CreateFollowers.call(
         source_user_id:    current_user.id,
         content:           params[:question],
-        author_identifier: AnonymousBlock.get_identifier(request.remote_ip)
+        author_identifier: AnonymousBlock.get_identifier(request.remote_ip),
       )
       return
     end
@@ -41,7 +28,20 @@ class Ajax::QuestionController < AjaxController
       target_user_id:    params[:rcpt],
       content:           params[:question],
       anonymous:         params[:anonymousQuestion],
-      author_identifier: AnonymousBlock.get_identifier(request.remote_ip)
+      author_identifier: AnonymousBlock.get_identifier(request.remote_ip),
     )
   end
+
+  def destroy
+    params.require :question
+
+    UseCase::Question::Destroy.call(
+      question_id:  params[:question],
+      current_user:,
+    )
+
+    @response[:status] = :okay
+    @response[:message] = t(".success")
+    @response[:success] = true
+  end
 end

app/workers/question_worker.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+# @deprecated This is to be replaced by SendToInboxJob. Remaining here so that remaining QuestionWorker jobs can finish.
 class QuestionWorker
   include Sidekiq::Worker
 
@@ -35,7 +36,5 @@ class QuestionWorker
     false
   end
 
-  def muted?(user, question)
-    MuteRule.where(user:).any? { |rule| rule.applies_to? question }
-  end
+  def muted?(user, question) = MuteRule.where(user:).any? { |rule| rule.applies_to? question }
 end

app/workers/send_to_inbox_job.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class SendToInboxJob
+  include Sidekiq::Job
+
+  sidekiq_options queue: :question, retry: false
+
+  # @param follower_id [Integer] user id passed from Devise
+  # @param question_id [Integer] newly created question id
+  def perform(follower_id, question_id)
+    follower = User.includes(:web_push_subscriptions, :mute_rules, :muted_users).find(follower_id)
+    question = Question.includes(:user).find(question_id)
+    webpush_app = Rpush::App.find_by(name: "webpush")
+
+    return if skip_inbox?(follower, question)
+
+    inbox = Inbox.create(user_id: follower.id, question_id:, new: true)
+    follower.push_notification(webpush_app, inbox) if webpush_app
+  end
+
+  private
+
+  def skip_inbox?(follower, question)
+    return true if follower.inbox_locked?
+    return true if follower.banned?
+    return true if muted?(follower, question)
+    return true if follower.muting?(question.user)
+    return true if question.long? && !follower.profile.allow_long_questions
+
+    false
+  end
+
+  # @param [User] user
+  # @param [Question] question
+  def muted?(user, question) = user.mute_rules.any? { |rule| rule.applies_to? question }
+end

lib/use_case/question/create_followers.rb

@@ -13,13 +13,14 @@ module UseCase
           author_is_anonymous: false,
           author_identifier:,
           user:                source_user,
-          direct:              false
+          direct:              false,
         )
 
         increment_asked_count
         increment_metric
 
-        QuestionWorker.perform_async(source_user_id, question.id)
+        args = source_user.followers.map { |f| [f.id, question.id] }
+        SendToInboxJob.perform_bulk(args)
 
         {
           status:   201,
@@ -40,12 +41,12 @@ module UseCase
             anonymous: false,
             followers: true,
             generated: false,
-          }
+          },
         )
       end
 
       def source_user
-        @source_user ||= ::User.find(source_user_id)
+        @source_user ||= ::User.includes(:followers).find(source_user_id)
       end
     end
   end

spec/controllers/ajax/question_controller_spec.rb

@@ -48,21 +48,25 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do
       include_examples "returns the expected response"
     end
 
-    shared_examples "enqueues a QuestionWorker job" do |_expected_rcpt|
-      it "enqueues a QuestionWorker job" do
-        allow(QuestionWorker).to receive(:perform_async)
+    shared_examples "enqueues SendToInboxJob jobs" do
+      it "enqueues a SendToInboxJob job" do
+        allow(SendToInboxJob).to receive(:perform_bulk)
         subject
-        expect(QuestionWorker).to have_received(:perform_async).with(user.id, Question.last.id)
+        question_id = Question.last.id
+        bulk_args = followers.map { |f| [f.id, question_id] }
+        expect(SendToInboxJob).to have_received(:perform_bulk).with(bulk_args)
       end
 
       include_examples "returns the expected response"
     end
 
-    shared_examples "does not enqueue a QuestionWorker job" do
-      it "does not enqueue a QuestionWorker job" do
-        allow(QuestionWorker).to receive(:perform_async)
+    shared_examples "does not enqueue a SendToInboxJob job" do
+      it "does not enqueue a SendToInboxJob job" do
+        allow(SendToInboxJob).to receive(:perform_async)
+        allow(SendToInboxJob).to receive(:perform_bulk)
         subject
-        expect(QuestionWorker).not_to have_received(:perform_async)
+        expect(SendToInboxJob).not_to have_received(:perform_async)
+        expect(SendToInboxJob).not_to have_received(:perform_bulk)
       end
 
       include_examples "returns the expected response"
@@ -194,13 +198,18 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do
 
       context "when rcpt is followers" do
         let(:rcpt) { "followers" }
+        let(:followers) { FactoryBot.create_list(:user, 3) }
+
+        before do
+          followers.each { |follower| follower.follow(user) }
+        end
 
         context "when anonymousQuestion is true" do
           let(:anonymous_question) { "true" }
           let(:expected_question_anonymous) { false }
 
           include_examples "creates the question", false
-          include_examples "enqueues a QuestionWorker job", "followers"
+          include_examples "enqueues SendToInboxJob jobs"
         end
 
         context "when anonymousQuestion is false" do
@@ -208,7 +217,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do
           let(:expected_question_anonymous) { false }
 
           include_examples "creates the question", false
-          include_examples "enqueues a QuestionWorker job", "followers"
+          include_examples "enqueues SendToInboxJob jobs"
         end
       end
 
@@ -375,7 +384,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do
           }
         end
 
-        include_examples "does not enqueue a QuestionWorker job"
+        include_examples "does not enqueue a SendToInboxJob job"
       end
 
       context "when rcpt is an invalid value" do

spec/lib/use_case/question/create_followers_spec.rb

@@ -6,13 +6,20 @@ describe UseCase::Question::CreateFollowers do
   subject do
     UseCase::Question::CreateFollowers.call(
       source_user_id:    source_user.id,
-      content:           content,
-      author_identifier: author_identifier
+      content:,
+      author_identifier:,
     )
   end
 
   context "user is logged in" do
+    before do
+      followers.each do |target_user|
+        target_user.follow source_user
+      end
+    end
+
     let(:source_user) { create(:user) }
+    let(:followers) { create_list(:user, 5) }
     let(:content) { "content" }
     let(:author_identifier) { nil }
 
@@ -21,7 +28,9 @@ describe UseCase::Question::CreateFollowers do
     end
 
     it "enqueues a QuestionWorker job" do
-      expect(QuestionWorker).to have_enqueued_sidekiq_job(source_user.id, subject[:resource].id)
+      followers.each do |target_user|
+        expect(SendToInboxJob).to have_enqueued_sidekiq_job(target_user.id, subject[:resource].id)
+      end
     end
 
     it "increments the asked count" do

spec/workers/send_to_inbox_job_spec.rb

@@ -0,0 +1,101 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe SendToInboxJob do
+  describe "#perform" do
+    let(:user) { FactoryBot.create(:user) }
+    let(:user_id) { user.id }
+    let(:content) { Faker::Lorem.sentence }
+    let(:question) { FactoryBot.create(:question, content:, user:) }
+    let(:question_id) { question.id }
+    let(:follower) { FactoryBot.create(:user) }
+    let(:follower_id) { follower.id }
+
+    before do
+      follower.follow(user)
+    end
+
+    subject { described_class.new.perform(follower_id, question_id) }
+
+    it "places the question in the inbox of the user's followers" do
+      expect { subject }
+        .to(
+          change { Inbox.where(user_id: follower_id, question_id:, new: true).count }
+            .from(0)
+            .to(1),
+        )
+    end
+
+    it "respects mute rules" do
+      question.content = "Some spicy question text"
+      question.save
+
+      MuteRule.create(user_id: follower_id, muted_phrase: "spicy")
+
+      subject
+      expect(Inbox.where(user_id: follower_id, question_id:, new: true).count).to eq(0)
+    end
+
+    it "respects inbox locks" do
+      follower.update(privacy_lock_inbox: true)
+
+      subject
+      expect(Inbox.where(user_id: follower_id, question_id:, new: true).count).to eq(0)
+    end
+
+    it "does not send questions to banned users" do
+      follower.ban
+
+      subject
+      expect(Inbox.where(user_id: follower_id, question_id:, new: true).count).to eq(0)
+    end
+
+    context "receiver has push notifications enabled" do
+      before do
+        Rpush::Webpush::App.create(
+          name:        "webpush",
+          certificate: { public_key: "AAAA", private_key: "AAAA", subject: "" }.to_json,
+          connections: 1,
+        )
+
+        WebPushSubscription.create!(
+          user:         follower,
+          subscription: {
+            endpoint: "This will not be used",
+            keys:     {},
+          },
+        )
+      end
+
+      it "sends notifications" do
+        expect { subject }
+          .to(
+            change { Rpush::Webpush::Notification.count }
+              .from(0)
+              .to(1),
+          )
+      end
+    end
+
+    context "long question" do
+      let(:content) { "x" * 1000 }
+
+      it "sends to recipients who allow long questions" do
+        follower.profile.update(allow_long_questions: true)
+
+        expect { subject }
+          .to(
+            change { Inbox.where(user_id: follower_id, question_id:, new: true).count }
+              .from(0)
+              .to(1),
+          )
+      end
+
+      it "does not send to recipients who do not allow long questions" do
+        subject
+        expect(Inbox.where(user_id: follower_id, question_id:, new: true).count).to eq(0)
+      end
+    end
+  end
+end