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