Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Dec 11, 2023
Merged
5 changes: 2 additions & 3 deletions app/workers/question_worker.rb
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -35,7 +36,5 @@ def skip_inbox?(follower, question, user)
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
36 changes: 36 additions & 0 deletions app/workers/send_to_inbox_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# frozen_string_literal: true

class SendToInboxJob
include Sidekiq::Worker

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
9 changes: 5 additions & 4 deletions lib/use_case/question/create_followers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ def call
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,
Expand All @@ -40,12 +41,12 @@ def increment_metric
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
Expand Down
19 changes: 14 additions & 5 deletions spec/controllers/ajax/question_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,13 @@
include_examples "returns the expected response"
end

shared_examples "enqueues a QuestionWorker job" do |_expected_rcpt|
shared_examples "enqueues QuestionWorker jobs" do
it "enqueues a QuestionWorker job" do
allow(QuestionWorker).to receive(:perform_async)
allow(QuestionWorker).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(QuestionWorker).to have_received(:perform_bulk).with(bulk_args)
end

include_examples "returns the expected response"
Expand All @@ -61,8 +63,10 @@
shared_examples "does not enqueue a QuestionWorker job" do
it "does not enqueue a QuestionWorker job" do
allow(QuestionWorker).to receive(:perform_async)
allow(QuestionWorker).to receive(:perform_bulk)
subject
expect(QuestionWorker).not_to have_received(:perform_async)
expect(QuestionWorker).not_to have_received(:perform_bulk)
end

include_examples "returns the expected response"
Expand Down Expand Up @@ -194,21 +198,26 @@

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 QuestionWorker jobs"
end

context "when anonymousQuestion is false" do
let(:anonymous_question) { "false" }
let(:expected_question_anonymous) { false }

include_examples "creates the question", false
include_examples "enqueues a QuestionWorker job", "followers"
include_examples "enqueues QuestionWorker jobs"
end
end

Expand Down
15 changes: 12 additions & 3 deletions spec/lib/use_case/question/create_followers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,20 @@
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 }

Expand All @@ -21,7 +28,9 @@
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(QuestionWorker).to have_enqueued_sidekiq_job(target_user.id, subject[:resource].id)
end
end

it "increments the asked count" do
Expand Down
101 changes: 101 additions & 0 deletions spec/workers/send_to_inbox_job_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Loading