From 0c35b8b420eb5df3f356a6f44e6dc1c2ad559ed3 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Tue, 16 Apr 2019 17:51:57 +1000 Subject: [PATCH] FEATURE: add suggested_topics_unread_max_days_old This new site setting determines the maximum age of unread topics in suggested. By default if you have any unread topics older than 90 days they will be omitted from suggested. This change was added for 2 reasons: 1. A performance safeguard, some users tend to collect a huge amount of read state so it becomes super expensive to find unread 2. People who collect a large amount of unread are much more interested in recent unread topics vs ancient unread topics, this makes suggested more relevant Also, this is a minor speed up for tests cause 3 expensive tests became 1. --- config/locales/server.en.yml | 1 + config/site_settings.yml | 4 ++ lib/topic_query.rb | 17 ++++++- spec/components/topic_query_spec.rb | 76 ++++++++++++++++++++--------- 4 files changed, 72 insertions(+), 26 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 530546f9c85..332b4a6b4bb 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1567,6 +1567,7 @@ en: suggested_topics: "Number of suggested topics shown at the bottom of a topic." limit_suggested_to_category: "Only show topics from the current category in suggested topics." suggested_topics_max_days_old: "Suggested topics should not be more than n days old." + suggested_topics_unread_max_days_old: "Suggested unread topics should not be more than n days old." clean_up_uploads: "Remove orphan unreferenced uploads to prevent illegal hosting. WARNING: you may want to back up of your /uploads directory before enabling this setting." clean_orphan_uploads_grace_period_hours: "Grace period (in hours) before an orphan upload is removed." diff --git a/config/site_settings.yml b/config/site_settings.yml index 02925176c0f..3c45ced5c78 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -141,6 +141,10 @@ basic: max: 2000 limit_suggested_to_category: default: false + suggested_topics_unread_max_days_old: + default: 90 + min: 0 + max: 10000 suggested_topics_max_days_old: default: 365 min: 7 diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 0a861a0179c..b004583ef1a 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -222,7 +222,15 @@ class TopicQuery )) unless builder.full? else - builder.add_results(unread_results(topic: topic, per_page: builder.results_left), :high) + + builder.add_results( + unread_results( + topic: topic, + per_page: builder.results_left, + max_age: SiteSetting.suggested_topics_unread_max_days_old + ), :high + ) + builder.add_results(new_results(topic: topic, per_page: builder.category_results_left)) unless builder.full? end end @@ -470,12 +478,17 @@ class TopicQuery .order('CASE WHEN topics.user_id = tu.user_id THEN 1 ELSE 2 END') if @user - # micro optimisation so we don't load up all of user stats which we do not need unread_at = DB.query_single( "select first_unread_at from user_stats where user_id = ?", @user.id).first + if max_age = options[:max_age] + max_age_date = max_age.days.ago + unread_at ||= max_age_date + unread_at = unread_at > max_age_date ? unread_at : max_age_date + end + # perf note, in the past we tried doing this in a subquery but performance was # terrible, also tried with a join and it was bad result = result.where("topics.updated_at >= ?", unread_at) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index b1503b316f3..c41e87ba6f8 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -3,7 +3,12 @@ require 'topic_view' describe TopicQuery do + # TODO: this let! here has impact on all tests + # it indeed happens first, but is not obvious later in the tests we depend on the user being + # created so early otherwise finding new topics does not work + # we should remove the let! here and use freeze time to communicate how the clock moves let!(:user) { Fabricate(:coding_horror) } + let(:creator) { Fabricate(:user) } let(:topic_query) { TopicQuery.new(user) } @@ -734,12 +739,16 @@ describe TopicQuery do context 'when logged in' do + def suggested_for(topic) + topic_query.list_suggested_for(topic).topics.map { |t| t.id } + end + let(:topic) { Fabricate(:topic) } let(:suggested_topics) { tt = topic # lets clear cache once category is created - working around caching is hard clear_cache! - topic_query.list_suggested_for(tt).topics.map { |t| t.id } + suggested_for(tt) } it "should return empty results when there is nothing to find" do @@ -839,7 +848,19 @@ describe TopicQuery do end context 'with some existing topics' do - let!(:partially_read) { Fabricate(:post, user: creator).topic } + + let!(:old_partially_read) { + topic = Fabricate(:post, user: creator).topic + Fabricate(:post, user: creator, topic: topic) + topic + } + + let!(:partially_read) { + topic = Fabricate(:post, user: creator).topic + Fabricate(:post, user: creator, topic: topic) + topic + } + let!(:new_topic) { Fabricate(:post, user: creator).topic } let!(:fully_read) { Fabricate(:post, user: creator).topic } let!(:closed_topic) { Fabricate(:topic, user: creator, closed: true) } @@ -849,42 +870,49 @@ describe TopicQuery do let!(:fully_read_archived) { Fabricate(:post, user: creator).topic } before do - user.user_option.auto_track_topics_after_msecs = 0 - user.user_option.save - TopicUser.update_last_read(user, partially_read.id, 0, 0, 0) + user.user_option.update!( + auto_track_topics_after_msecs: 0, + new_topic_duration_minutes: User::NewTopicDuration::ALWAYS + ) + + freeze_time 3.weeks.from_now + + TopicUser.update_last_read(user, old_partially_read.id, 1, 1, 0) + TopicUser.update_last_read(user, partially_read.id, 1, 1, 0) TopicUser.update_last_read(user, fully_read.id, 1, 1, 0) TopicUser.update_last_read(user, fully_read_closed.id, 1, 1, 0) TopicUser.update_last_read(user, fully_read_archived.id, 1, 1, 0) + fully_read_closed.closed = true fully_read_closed.save fully_read_archived.archived = true fully_read_archived.save + + old_partially_read.update!(updated_at: 2.weeks.ago) + partially_read.update!(updated_at: Time.now) + end - it "returns unread, then new, then random" do - SiteSetting.suggested_topics = 7 - expect(suggested_topics[0]).to eq(partially_read.id) - expect(suggested_topics[1, 3]).to include(new_topic.id) - expect(suggested_topics[1, 3]).to include(closed_topic.id) - expect(suggested_topics[1, 3]).to include(archived_topic.id) + it "operates correctly" do - # The line below appears to randomly fail, no idea why need to restructure test - #expect(suggested_topics[4]).to eq(fully_read.id) - # random doesn't include closed and archived - end + # Note, this is a pretty slow integration test + # it tests that suggested is returned in the expected order + # hence we run suggested_for twice here to save on all the setup - it "won't return new or fully read if there are enough partially read topics" do - SiteSetting.suggested_topics = 1 - expect(suggested_topics).to eq([partially_read.id]) - end - - it "won't return fully read if there are enough partially read topics and new topics" do SiteSetting.suggested_topics = 4 + SiteSetting.suggested_topics_unread_max_days_old = 7 + expect(suggested_topics[0]).to eq(partially_read.id) - expect(suggested_topics[1, 3]).to include(new_topic.id) - expect(suggested_topics[1, 3]).to include(closed_topic.id) - expect(suggested_topics[1, 3]).to include(archived_topic.id) + expect(suggested_topics[1, 3]).to contain_exactly(new_topic.id, closed_topic.id, archived_topic.id) + + expect(suggested_topics.length).to eq(4) + + SiteSetting.suggested_topics = 2 + SiteSetting.suggested_topics_unread_max_days_old = 15 + + expect(suggested_for(topic)).to contain_exactly(partially_read.id, old_partially_read.id) end + end end end