DEV: Publish reviewable claimed topic message to groups instead of users (#19188)

I'm hesitant to call this a performance improvement since claiming a
reviewable is probably rare. However, this commit cuts out two DB
queries each time we have to publish a reviewable claimed message. More
importantly, publishing to groups scales much better than publishing to
users because we esstentially cap the number ids we have to load into
memory.
This commit is contained in:
Alan Guo Xiang Tan
2022-11-25 08:07:29 +08:00
committed by GitHub
parent 498fa14347
commit 45f3e9f19e
2 changed files with 44 additions and 14 deletions

View File

@@ -38,11 +38,10 @@ class ReviewableClaimedTopicsController < ApplicationController
private private
def notify_users(topic, claimed_by) def notify_users(topic, claimed_by)
user_ids = User.staff.pluck(:id) group_ids = Set.new([Group::AUTO_GROUPS[:staff]])
if SiteSetting.enable_category_group_moderation? && group_id = topic.category&.reviewable_by_group_id.presence if SiteSetting.enable_category_group_moderation? && group_id = topic.category&.reviewable_by_group_id.presence
user_ids.concat(GroupUser.where(group_id: group_id).pluck(:user_id)) group_ids.add(group_id)
user_ids.uniq!
end end
if claimed_by.present? if claimed_by.present?
@@ -51,6 +50,6 @@ class ReviewableClaimedTopicsController < ApplicationController
data = { topic_id: topic.id } data = { topic_id: topic.id }
end end
MessageBus.publish("/reviewable_claimed", data, user_ids: user_ids) MessageBus.publish("/reviewable_claimed", data, group_ids: group_ids.to_a)
end end
end end

View File

@@ -23,15 +23,41 @@ RSpec.describe ReviewableClaimedTopicsController do
it "works" do it "works" do
SiteSetting.reviewable_claiming = 'optional' SiteSetting.reviewable_claiming = 'optional'
messages = MessageBus.track_publish { post "/reviewable_claimed_topics.json", params: params } messages = MessageBus.track_publish("/reviewable_claimed") do
post "/reviewable_claimed_topics.json", params: params
expect(response.status).to eq(200) expect(response.status).to eq(200)
end
expect(ReviewableClaimedTopic.where(user_id: moderator.id, topic_id: topic.id).exists?).to eq(true) expect(ReviewableClaimedTopic.where(user_id: moderator.id, topic_id: topic.id).exists?).to eq(true)
expect(topic.reviewables.first.history.where(reviewable_history_type: ReviewableHistory.types[:claimed]).size).to eq(1) expect(topic.reviewables.first.history.where(reviewable_history_type: ReviewableHistory.types[:claimed]).size).to eq(1)
expect(messages.size).to eq(1) expect(messages.size).to eq(1)
expect(messages[0].channel).to eq("/reviewable_claimed")
expect(messages[0].data[:topic_id]).to eq(topic.id) message = messages[0]
expect(messages[0].data[:user][:id]).to eq(moderator.id)
expect(message.data[:topic_id]).to eq(topic.id)
expect(message.data[:user][:id]).to eq(moderator.id)
expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff])
end
it "publishes reviewable claimed changes to the category moderators of the topic's category" do
SiteSetting.enable_category_group_moderation = true
SiteSetting.reviewable_claiming = 'optional'
group = Fabricate(:group)
topic.category.update!(reviewable_by_group: group)
messages = MessageBus.track_publish("/reviewable_claimed") do
post "/reviewable_claimed_topics.json", params: params
expect(response.status).to eq(200)
end
expect(messages.size).to eq(1)
message = messages[0]
expect(message.data[:topic_id]).to eq(topic.id)
expect(message.data[:user][:id]).to eq(moderator.id)
expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff], group.id)
end end
it "works with deleted topics" do it "works with deleted topics" do
@@ -73,15 +99,20 @@ RSpec.describe ReviewableClaimedTopicsController do
it "works" do it "works" do
SiteSetting.reviewable_claiming = 'optional' SiteSetting.reviewable_claiming = 'optional'
messages = MessageBus.track_publish { delete "/reviewable_claimed_topics/#{claimed.topic_id}.json" } messages = MessageBus.track_publish("/reviewable_claimed") do
delete "/reviewable_claimed_topics/#{claimed.topic_id}.json"
expect(response.status).to eq(200) expect(response.status).to eq(200)
end
expect(ReviewableClaimedTopic.where(topic_id: claimed.topic_id).exists?).to eq(false) expect(ReviewableClaimedTopic.where(topic_id: claimed.topic_id).exists?).to eq(false)
expect(topic.reviewables.first.history.where(reviewable_history_type: ReviewableHistory.types[:unclaimed]).size).to eq(1) expect(topic.reviewables.first.history.where(reviewable_history_type: ReviewableHistory.types[:unclaimed]).size).to eq(1)
expect(messages.size).to eq(1) expect(messages.size).to eq(1)
expect(messages[0].channel).to eq("/reviewable_claimed")
expect(messages[0].data[:topic_id]).to eq(topic.id) message = messages[0]
expect(messages[0].data[:user]).to eq(nil)
expect(message.data[:topic_id]).to eq(topic.id)
expect(message.data[:user]).to eq(nil)
expect(message.group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff])
end end
it "works with deleted topics" do it "works with deleted topics" do