Skip to content

Commit be146b2

Browse files
authored
FIX: Optimise and update queries, shorter frequency (#20)
This commit solves a few issues: - Currently due to the 14.day schedule, any sites with `remind_mark_solution_last_post_age` of 1 will not be able to receive notifications. This bumps the frequency to a day - On a site with hundreds of thousands of topics, this loads all the ids into memory `unsolved_topic_ids.push(*Topic.where(category_id: category_ids).pluck(:id))` 😱 - Many plucks (N+1) - (Potentially) Large array subtractions are inefficient and could be done in the database - Moves to custom fields as per discourse/discourse-solved#342
1 parent 500e949 commit be146b2

File tree

2 files changed

+42
-38
lines changed

2 files changed

+42
-38
lines changed

app/jobs/scheduled/mark_as_solution.rb

Lines changed: 33 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,61 +2,59 @@
22

33
module Jobs
44
class MarkAsSolution < ::Jobs::Scheduled
5-
every 14.days
5+
every 1.day
66

77
def execute(_args = nil)
8-
return false unless SiteSetting.solved_enabled
9-
return false unless SiteSetting.solved_reminders_plugin_enabled
8+
return false unless SiteSetting.solved_enabled && SiteSetting.solved_reminders_plugin_enabled
109
Rails.logger.warn("Running scheduled job to send notifications to mark a post a solution.")
1110

12-
unsolved_topic_ids = []
11+
base_query =
12+
Topic
13+
.listable_topics
14+
.where(closed: false, archived: false, visible: true)
15+
.where("posts_count > 0")
16+
.where("last_post_user_id <> user_id")
17+
.where("last_posted_at > ?", SiteSetting.remind_mark_solution_last_post_age.days.ago)
18+
.where(
19+
"NOT EXISTS (SELECT 1 FROM discourse_solved_solved_topics dsst WHERE dsst.topic_id = topics.id)",
20+
)
1321

14-
if SiteSetting.allow_solved_on_all_topics
15-
unsolved_topic_ids = unsolved_topic_ids.push(*Topic.pluck(:id))
16-
else
22+
if !SiteSetting.allow_solved_on_all_topics
1723
category_ids =
18-
CategoryCustomField.where(name: "enable_accepted_answers", value: "true").pluck(
24+
CategoryCustomField.where(name: "enable_accepted_answers", value: "true").select(
1925
:category_id,
2026
)
2127

22-
unsolved_topic_ids =
23-
unsolved_topic_ids.push(*Topic.where(category_id: category_ids).pluck(:id))
24-
25-
if SiteSetting.enable_solved_tags.present?
26-
allowed_tags_ids = Tag.where_name(SiteSetting.enable_solved_tags.split("|")).pluck(:id)
27-
unsolved_topic_ids =
28-
unsolved_topic_ids.push(
29-
*TopicTag.where(tag_id: allowed_tags_ids).distinct.pluck(:topic_id),
30-
)
31-
end
28+
tag_topic_ids =
29+
if SiteSetting.enable_solved_tags.present?
30+
TopicTag.where(
31+
tag_id: Tag.where_name(SiteSetting.enable_solved_tags.split("|")).select(:id),
32+
).select(:topic_id)
33+
end
34+
35+
base_query =
36+
if tag_topic_ids
37+
base_query.where("category_id IN (?) OR id IN (?)", category_ids, tag_topic_ids)
38+
else
39+
base_query.where(category_id: category_ids)
40+
end
3241
end
3342

34-
solved_topic_ids = TopicCustomField.where(name: "accepted_answer_post_id").pluck(:topic_id)
35-
unsolved_topic_ids = unsolved_topic_ids - solved_topic_ids if solved_topic_ids.present?
43+
cutoff_date = SiteSetting.remind_mark_solution_after_days.days.ago
3644

37-
unsolved_topics =
38-
Topic
39-
.listable_topics
40-
.where(id: unsolved_topic_ids)
41-
.where(closed: false, archived: false, visible: true)
42-
.where("posts_count > 0")
43-
.where("last_post_user_id <> user_id")
44-
.where("last_posted_at > ?", SiteSetting.remind_mark_solution_last_post_age.days.ago)
45-
46-
unsolved_topics.each do |topic|
47-
if (topic&.last_posted_at || Date.today) <
48-
SiteSetting.remind_mark_solution_after_days.days.ago
49-
# create a new reminder PM
45+
discobot = User.find(-2)
46+
base_query
47+
.where("COALESCE(last_posted_at, current_date) < ?", cutoff_date)
48+
.find_each do |topic|
5049
PostCreator.create!(
51-
User.find(-2),
50+
discobot,
5251
title: I18n.t("mark_as_solution.title"),
5352
raw: "#{I18n.t("mark_as_solution.message")}\n\n#{topic.url}",
5453
archetype: Archetype.private_message,
5554
target_usernames: [topic.user.username],
5655
skip_validations: true,
5756
)
5857
end
59-
end
6058
end
6159
end
6260
end

spec/jobs/mark_as_solution_spec.rb

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616

1717
context "when the topic is not solved" do
1818
it "should send the PM to user to mark a post as solution" do
19-
expect { described_class.new.execute({}) }.to change { Topic.count }.by(1)
19+
expect { described_class.new.execute({}) }.to change {
20+
Topic.where(archetype: Archetype.private_message).count
21+
}.by(1)
2022
expect(Topic.last.title).to eq(I18n.t("mark_as_solution.title"))
2123
end
2224
end
@@ -25,14 +27,18 @@
2527
before { DiscourseSolved.accept_answer!(post, Discourse.system_user) }
2628

2729
it "should not send the PM to user" do
28-
expect { described_class.new.execute({}) }.to not_change { Topic.count }
30+
expect { described_class.new.execute({}) }.to not_change {
31+
Topic.where(archetype: Archetype.private_message).count
32+
}
2933
end
3034
end
3135

3236
context "when the plugin is disabled" do
3337
it "should not send the PM to user" do
3438
SiteSetting.solved_reminders_plugin_enabled = false
35-
expect { described_class.new.execute({}) }.to not_change { Topic.count }
39+
expect { described_class.new.execute({}) }.to not_change {
40+
Topic.where(archetype: Archetype.private_message).count
41+
}
3642
end
3743
end
3844
end

0 commit comments

Comments
 (0)