Skip to content

FIX: Select better featured posters #39691

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.

Sign up for GitHub

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

Open
curi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FIX: Select better featured posters #39691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 77 additions & 20 deletions app/models/topic_featured_users.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

class TopicFeaturedUsers
FREQUENT_POSTER_COUNT = 2

attr_reader :topic

def initialize(topic)
Expand All @@ -11,33 +13,44 @@ def self.count
4
end

def self.recent_poster_count
count - FREQUENT_POSTER_COUNT
end

# Chooses which topic users to feature
def choose(args = {})
self.class.ensure_consistency!(topic.id.to_i)
update_participant_count
end

def user_ids
[
topic.featured_user1_id,
topic.featured_user2_id,
topic.featured_user3_id,
topic.featured_user4_id,
].uniq.compact
slot_user_ids.uniq.compact
end

def recent_user_ids
slot_user_ids.last(self.class.recent_poster_count).compact
end

def self.ensure_consistency!(topic_id = nil)
filter = "#{"AND t.id = #{topic_id.to_i}" if topic_id}"
filter2 = "#{"AND tt.id = #{topic_id.to_i}" if topic_id}"

# The topic list shows up to five posters in this order: the OP, two frequent posters,
# and two recent posters. Recent posters are excluded from frequent posters, and the OP
# is excluded from both. The latest poster is handled separately and replaces a recent
# poster unless the OP is the latest poster.

sql = <<SQL

WITH cte as (
WITH poster_stats as (
SELECT
t.id,
t.user_id as topic_user_id,
t.last_post_user_id,
p.user_id,
COUNT(*) post_count,
MAX(p.created_at) last_post_date,
ROW_NUMBER() OVER(PARTITION BY t.id ORDER BY COUNT(*) DESC, MAX(p.created_at) DESC) as rank
ROW_NUMBER() OVER(PARTITION BY t.id ORDER BY MAX(p.created_at) DESC) as recent_rank
FROM topics t
JOIN posts p ON p.topic_id = t.id
WHERE p.deleted_at IS NULL AND
Expand All @@ -46,13 +59,48 @@ def self.ensure_consistency!(topic_id = nil)
p.user_id <> t.user_id AND
p.user_id <> t.last_post_user_id
#{filter}
GROUP BY t.id, p.user_id
GROUP BY t.id, t.user_id, t.last_post_user_id, p.user_id
),

selected_recent_posters as (
SELECT id, user_id, recent_rank + #{FREQUENT_POSTER_COUNT} as rank
FROM poster_stats
WHERE topic_user_id = last_post_user_id AND recent_rank <= #{recent_poster_count}

UNION ALL

SELECT id, user_id, recent_rank + #{FREQUENT_POSTER_COUNT} as rank
FROM poster_stats
WHERE topic_user_id <> last_post_user_id AND recent_rank <= #{recent_poster_count - 1}
),

cte2 as (
SELECT id, user_id, ROW_NUMBER() OVER(PARTITION BY id ORDER BY last_post_date ASC) as rank
FROM cte
WHERE rank <= #{count}
selected_frequent_posters as (
SELECT
id,
user_id,
ROW_NUMBER() OVER(PARTITION BY id ORDER BY post_count DESC, last_post_date DESC) as rank
FROM poster_stats
WHERE topic_user_id = last_post_user_id AND recent_rank > #{recent_poster_count}

UNION ALL

SELECT
id,
user_id,
ROW_NUMBER() OVER(PARTITION BY id ORDER BY post_count DESC, last_post_date DESC) as rank
FROM poster_stats
WHERE topic_user_id <> last_post_user_id AND recent_rank > #{recent_poster_count - 1}
),

selected_topic_posters as (
SELECT id, user_id, rank
FROM selected_frequent_posters
WHERE rank <= #{FREQUENT_POSTER_COUNT}

UNION ALL

SELECT id, user_id, rank
FROM selected_recent_posters
)

UPDATE topics tt
Expand All @@ -64,13 +112,13 @@ def self.ensure_consistency!(topic_id = nil)
FROM topics AS tt2
LEFT OUTER JOIN (
SELECT
c.id,
MAX(case when c.rank = 1 then c.user_id end) featured_user1,
MAX(case when c.rank = 2 then c.user_id end) featured_user2,
MAX(case when c.rank = 3 then c.user_id end) featured_user3,
MAX(case when c.rank = 4 then c.user_id end) featured_user4
FROM cte2 as c
GROUP BY c.id
selected_topic_posters.id,
MAX(case when selected_topic_posters.rank = 1 then selected_topic_posters.user_id end) featured_user1,
MAX(case when selected_topic_posters.rank = 2 then selected_topic_posters.user_id end) featured_user2,
MAX(case when selected_topic_posters.rank = 3 then selected_topic_posters.user_id end) featured_user3,
MAX(case when selected_topic_posters.rank = 4 then selected_topic_posters.user_id end) featured_user4
FROM selected_topic_posters
GROUP BY selected_topic_posters.id
) x ON x.id = tt2.id
WHERE tt.id = tt2.id AND
(
Expand All @@ -87,6 +135,15 @@ def self.ensure_consistency!(topic_id = nil)

private

def slot_user_ids
[
topic.featured_user1_id,
topic.featured_user2_id,
topic.featured_user3_id,
topic.featured_user4_id,
]
end

def update_participant_count
DB.exec(<<~SQL, topic_id: topic.id)
UPDATE topics
Expand Down
13 changes: 12 additions & 1 deletion app/models/topic_posters_summary.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def self.translations
original_poster: I18n.t(:original_poster),
most_recent_poster: I18n.t(:most_recent_poster),
frequent_poster: I18n.t(:frequent_poster),
recent_poster: I18n.t(:recent_poster),
joiner: I18n.t(:poster_description_joiner),
}
end
Expand Down Expand Up @@ -58,7 +59,13 @@ def descriptions_by_id(ids: nil)

while id = ids.shift
result[id] ||= []
result[id] << @translations[:frequent_poster]
description =
if recent_poster_user_ids.include?(id)
@translations[:recent_poster]
else
@translations[:frequent_poster]
end
result[id] << description
end

result
Expand Down Expand Up @@ -93,6 +100,10 @@ def user_ids
[topic.user_id, topic.last_post_user_id, *topic.featured_user_ids]
end

def recent_poster_user_ids
@recent_poster_user_ids ||= topic.featured_users.recent_user_ids
end

def user_lookup
@user_lookup ||= options[:user_lookup] || UserLookup.new(user_ids)
end
Expand Down
1 change: 1 addition & 0 deletions config/locales/server.en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3020,6 +3020,7 @@ en:
original_poster: "Original Poster"
most_recent_poster: "Most Recent Poster"
frequent_poster: "Frequent Poster"
recent_poster: "Recent Poster"
# Example: "Original Poster, Most Recent Poster"
poster_description_joiner: ", "

Expand Down
187 changes: 183 additions & 4 deletions spec/models/topic_featured_users_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
t.reload

expect(t.featured_user1_id).to eq(p2.user_id)
expect(t.featured_user2_id).to eq(p4.user_id)
expect(t.featured_user3_id).to eq(nil)
expect(t.featured_user2_id).to eq(nil)
expect(t.featured_user3_id).to eq(p4.user_id)
expect(t.featured_user4_id).to eq(nil)

# after removing a post
Expand All @@ -32,9 +32,188 @@
TopicFeaturedUsers.ensure_consistency!
t.reload

expect(t.featured_user1_id).to eq(p4.user_id)
expect(t.featured_user1_id).to eq(nil)
expect(t.featured_user2_id).to eq(nil)
expect(t.featured_user3_id).to eq(nil)
expect(t.featured_user3_id).to eq(p4.user_id)
expect(t.featured_user4_id).to eq(nil)
end

it "keeps a high-count recent poster in a visible featured slot" do
topic_creator = Fabricate(:user)
latest_poster = Fabricate(:user)
frequent_recent_poster = Fabricate(:user)
frequent_older_poster = Fabricate(:user)
less_frequent_older_poster = Fabricate(:user)
recent_poster = Fabricate(:user)
older_poster = Fabricate(:user)

topic = Fabricate(:topic, user: topic_creator)
Fabricate(:post, topic: topic, user: topic_creator, created_at: 7.days.ago)
Fabricate(:post, topic: topic, user: older_poster, created_at: 6.days.ago)

4.times do |index|
Fabricate(
:post,
topic: topic,
user: less_frequent_older_poster,
created_at: (5.days.ago + index.minutes),
)
end

5.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_older_poster,
created_at: (4.days.ago + index.minutes),
)
end

6.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_recent_poster,
created_at: (2.days.ago + index.minutes),
)
end

Fabricate(:post, topic: topic, user: recent_poster, created_at: 36.hours.ago)
latest_post = Fabricate(:post, topic: topic, user: latest_poster, created_at: 1.day.ago)
topic.update_columns(last_post_user_id: latest_post.user_id)

TopicFeaturedUsers.ensure_consistency!

expect(topic.reload.featured_user_ids.take(4)).to eq(
[frequent_recent_poster.id, frequent_older_poster.id, recent_poster.id],
)
end

it "includes the most recent non-OP non-latest poster in a visible featured slot" do
topic_creator = Fabricate(:user)
latest_poster = Fabricate(:user)
frequent_poster1 = Fabricate(:user)
frequent_poster2 = Fabricate(:user)
frequent_poster3 = Fabricate(:user)
recent_poster = Fabricate(:user)

topic = Fabricate(:topic, user: topic_creator)
Fabricate(:post, topic: topic, user: topic_creator, created_at: 7.days.ago)

5.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster1,
created_at: (6.days.ago + index.minutes),
)
end

4.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster2,
created_at: (5.days.ago + index.minutes),
)
end

3.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster3,
created_at: (4.days.ago + index.minutes),
)
end

Fabricate(:post, topic: topic, user: recent_poster, created_at: 2.days.ago)
latest_post = Fabricate(:post, topic: topic, user: latest_poster, created_at: 1.day.ago)
topic.update_columns(last_post_user_id: latest_post.user_id)

TopicFeaturedUsers.ensure_consistency!

expect(topic.reload.featured_user_ids.take(4)).to eq(
[frequent_poster1.id, frequent_poster2.id, recent_poster.id],
)
end

it "picks the most recent visible non-OP poster when the latest poster's posts are all hidden" do
topic_creator = Fabricate(:user)
ghost_latest_poster = Fabricate(:user)
recent_visible_poster = Fabricate(:user)
frequent_poster = Fabricate(:user)

topic = Fabricate(:topic, user: topic_creator)
Fabricate(:post, topic: topic, user: topic_creator, created_at: 5.days.ago)

3.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster,
created_at: (4.days.ago + index.minutes),
)
end

Fabricate(:post, topic: topic, user: recent_visible_poster, created_at: 2.days.ago)

ghost_post = Fabricate(:post, topic: topic, user: ghost_latest_poster, created_at: 1.day.ago)
topic.update_columns(last_post_user_id: ghost_latest_poster.id)
ghost_post.update_columns(deleted_at: Time.now)

TopicFeaturedUsers.ensure_consistency!

expect(topic.reload.featured_user_ids.take(4)).to eq(
[frequent_poster.id, recent_visible_poster.id],
)
end

it "puts the recent poster after frequent posters when the OP is the latest poster" do
topic_creator = Fabricate(:user)
frequent_poster1 = Fabricate(:user)
frequent_poster2 = Fabricate(:user)
frequent_poster3 = Fabricate(:user)
recent_poster = Fabricate(:user)

topic = Fabricate(:topic, user: topic_creator)
Fabricate(:post, topic: topic, user: topic_creator, created_at: 7.days.ago)

5.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster1,
created_at: (6.days.ago + index.minutes),
)
end

4.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster2,
created_at: (5.days.ago + index.minutes),
)
end

3.times do |index|
Fabricate(
:post,
topic: topic,
user: frequent_poster3,
created_at: (4.days.ago + index.minutes),
)
end

Fabricate(:post, topic: topic, user: recent_poster, created_at: 2.days.ago)
latest_post = Fabricate(:post, topic: topic, user: topic_creator, created_at: 1.day.ago)
topic.update_columns(last_post_user_id: latest_post.user_id)

TopicFeaturedUsers.ensure_consistency!

expect(topic.reload.featured_user_ids.take(4)).to eq(
[frequent_poster1.id, frequent_poster2.id, recent_poster.id, frequent_poster3.id],
)
end
end
Loading