mirror of
https://github.com/discourse/discourse.git
synced 2025-02-06 11:28:18 +00:00
c0293339b8
Previously, Jobs::EnqueueDigestEmails would enqueue a digest job for every user, even if there are no topics to send. The digest job would exit, no email would send, and last_emailed_at would not change. 30 minutes later, Jobs::EnqueueDigestEmails would run again and re-enqueue jobs for the same users. 120fa8ad introduced a temporary mitigation for this issue, by randomly selecting a subset of those users each time. This commit adds a new `digest_attempted_at` column to the `user_stats` table. This column is updated every time a digest job completes for a user. Using this, we can avoid scheduling digest jobs for the same user every 30 minutes. This also removes the random user selection in 120fa8ad, and instead prioritizes users who had digests attempted the longest time ago.
194 lines
6.8 KiB
Ruby
194 lines
6.8 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
require 'rails_helper'
|
|
|
|
describe Jobs::EnqueueDigestEmails do
|
|
|
|
describe '#target_users' do
|
|
|
|
context 'disabled digests' do
|
|
before { SiteSetting.default_email_digest_frequency = 0 }
|
|
let!(:user_no_digests) { Fabricate(:active_user, last_emailed_at: 8.days.ago, last_seen_at: 10.days.ago) }
|
|
|
|
it "doesn't return users with email disabled" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(user_no_digests.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context 'unapproved users' do
|
|
|
|
before do
|
|
SiteSetting.must_approve_users = true
|
|
end
|
|
|
|
let!(:unapproved_user) { Fabricate(:active_user, approved: false, last_emailed_at: 8.days.ago, last_seen_at: 10.days.ago) }
|
|
|
|
it 'should enqueue the right digest emails' do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(false)
|
|
|
|
# As a moderator
|
|
unapproved_user.update_column(:moderator, true)
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true)
|
|
|
|
# As an admin
|
|
unapproved_user.update(admin: true, moderator: false)
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true)
|
|
|
|
# As an approved user
|
|
unapproved_user.update(admin: false, moderator: false, approved: true)
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true)
|
|
end
|
|
end
|
|
|
|
context 'staged users' do
|
|
let!(:staged_user) { Fabricate(:active_user, staged: true, last_emailed_at: 1.year.ago, last_seen_at: 1.year.ago) }
|
|
|
|
it "doesn't return staged users" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(staged_user.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context 'recently emailed' do
|
|
let!(:user_emailed_recently) { Fabricate(:active_user, last_emailed_at: 6.days.ago) }
|
|
|
|
it "doesn't return users who have been emailed recently" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(user_emailed_recently.id)).to eq(false)
|
|
end
|
|
|
|
end
|
|
|
|
context "inactive user" do
|
|
let!(:inactive_user) { Fabricate(:user, active: false) }
|
|
|
|
it "doesn't return users who have been emailed recently" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(inactive_user.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context "suspended user" do
|
|
let!(:suspended_user) { Fabricate(:user, suspended_till: 1.week.from_now, suspended_at: 1.day.ago) }
|
|
|
|
it "doesn't return users who are suspended" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(suspended_user.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context 'visited the site this week' do
|
|
let(:user_visited_this_week) { Fabricate(:active_user, last_seen_at: 6.days.ago) }
|
|
|
|
it "doesn't return users who have been emailed recently" do
|
|
user = user_visited_this_week
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(user.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context 'visited the site a year ago' do
|
|
let!(:user_visited_a_year_ago) { Fabricate(:active_user, last_seen_at: 370.days.ago) }
|
|
|
|
it "doesn't return the user who have not visited the site for more than 365 days" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(user_visited_a_year_ago.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context 'regular users' do
|
|
let!(:user) { Fabricate(:active_user, last_seen_at: 360.days.ago) }
|
|
|
|
it "returns the user" do
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids).to eq([user.id])
|
|
end
|
|
end
|
|
|
|
context 'too many bounces' do
|
|
let!(:bounce_user) { Fabricate(:active_user, last_seen_at: 6.month.ago) }
|
|
|
|
it "doesn't return users with too many bounces" do
|
|
bounce_user.user_stat.update(bounce_score: SiteSetting.bounce_score_threshold + 1)
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(bounce_user.id)).to eq(false)
|
|
end
|
|
end
|
|
|
|
context "no primary email" do
|
|
let!(:user) { Fabricate(:active_user, last_seen_at: 2.months.ago) }
|
|
|
|
it "doesn't return users with no primary emails" do
|
|
UserEmail.where(user: user).delete_all
|
|
expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(user.id)).to eq(false)
|
|
end
|
|
end
|
|
end
|
|
|
|
describe '#execute' do
|
|
|
|
let(:user) { Fabricate(:user) }
|
|
|
|
it "limits jobs enqueued per max_digests_enqueued_per_30_mins_per_site" do
|
|
user1 = Fabricate(:user, last_seen_at: 2.months.ago, last_emailed_at: 2.months.ago)
|
|
user2 = Fabricate(:user, last_seen_at: 2.months.ago, last_emailed_at: 2.months.ago)
|
|
|
|
user1.user_stat.update(digest_attempted_at: 2.week.ago)
|
|
user2.user_stat.update(digest_attempted_at: 3.weeks.ago)
|
|
|
|
global_setting :max_digests_enqueued_per_30_mins_per_site, 1
|
|
|
|
expect_enqueued_with(job: :user_email, args: { type: :digest, user_id: user2.id }) do
|
|
expect { Jobs::EnqueueDigestEmails.new.execute(nil) }.to change(Jobs::UserEmail.jobs, :size).by (1)
|
|
end
|
|
|
|
# The job didn't actually run, so fake the user_stat update
|
|
user2.user_stat.update(digest_attempted_at: Time.zone.now)
|
|
|
|
expect_enqueued_with(job: :user_email, args: { type: :digest, user_id: user1.id }) do
|
|
expect { Jobs::EnqueueDigestEmails.new.execute(nil) }.to change(Jobs::UserEmail.jobs, :size).by (1)
|
|
end
|
|
|
|
user1.user_stat.update(digest_attempted_at: Time.zone.now)
|
|
|
|
expect_not_enqueued_with(job: :user_email) do
|
|
Jobs::EnqueueDigestEmails.new.execute(nil)
|
|
end
|
|
end
|
|
|
|
context "digest emails are enabled" do
|
|
before do
|
|
Jobs::EnqueueDigestEmails.any_instance.expects(:target_user_ids).returns([user.id])
|
|
end
|
|
|
|
it "enqueues the digest email job" do
|
|
SiteSetting.disable_digest_emails = false
|
|
|
|
expect_enqueued_with(job: :user_email, args: { type: :digest, user_id: user.id }) do
|
|
Jobs::EnqueueDigestEmails.new.execute({})
|
|
end
|
|
end
|
|
end
|
|
|
|
context "private email" do
|
|
before do
|
|
Jobs::EnqueueDigestEmails.any_instance.expects(:target_user_ids).never
|
|
SiteSetting.private_email = true
|
|
end
|
|
|
|
it "doesn't return users with email disabled" do
|
|
expect_not_enqueued_with(job: :user_email, args: { type: :digest, user_id: user.id }) do
|
|
Jobs::EnqueueDigestEmails.new.execute({})
|
|
end
|
|
end
|
|
end
|
|
|
|
context "digest emails are disabled" do
|
|
before do
|
|
Jobs::EnqueueDigestEmails.any_instance.expects(:target_user_ids).never
|
|
SiteSetting.disable_digest_emails = true
|
|
end
|
|
|
|
it "does not enqueue the digest email job" do
|
|
expect_not_enqueued_with(job: :user_email, args: { type: :digest, user_id: user.id }) do
|
|
Jobs::EnqueueDigestEmails.new.execute({})
|
|
end
|
|
end
|
|
end
|
|
|
|
end
|
|
|
|
end
|