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

@@ -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."

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

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)

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