FIX: select appropriate period when redirecting to top

app/assets/javascripts/discourse/controllers/discovery/topics.js.es6

@@ -10,7 +10,7 @@ const controllerOpts = {
   canStar: Em.computed.alias('controllers.discovery/topics.currentUser.id'),
   showTopicPostBadges: Em.computed.not('controllers.discovery/topics.new'),
 
-  redirectedReason: Em.computed.alias('currentUser.redirected_to_top_reason'),
+  redirectedReason: Em.computed.alias('currentUser.redirected_to_top.reason'),
 
   order: 'default',
   ascending: false,

app/assets/javascripts/discourse/pre-initializers/dynamic-route-builders.js.es6

@@ -24,7 +24,7 @@ export default {
         willTransition: function() {
           this._super();
           Discourse.User.currentProp("should_be_redirected_to_top", false);
-          Discourse.User.currentProp("redirected_to_top_reason", null);
+          Discourse.User.currentProp("redirected_to_top.reason", null);
           return true;
         }
       }

app/assets/javascripts/discourse/routes/discovery.js.es6

@@ -15,7 +15,8 @@ const DiscoveryRoute = Discourse.Route.extend(OpenComposer, {
         transition.targetName.indexOf("discovery.top") === -1 &&
         Discourse.User.currentProp("should_be_redirected_to_top")) {
       Discourse.User.currentProp("should_be_redirected_to_top", false);
-      this.replaceWith("discovery.top");
+      const period = Discourse.User.currentProp("redirect_to_top.period") || "all";
+      this.replaceWith(`discovery.top${period.capitalize()}`);
     }
   },
 

app/jobs/regular/update_top_redirection.rb

@@ -3,8 +3,7 @@ module Jobs
   class UpdateTopRedirection < Jobs::Base
 
     def execute(args)
-      user = User.find_by(id: args[:user_id])
+      if user = User.find_by(id: args[:user_id])
-      if user
         user.update_column(:last_redirected_to_top_at, args[:redirected_at])
       end
     end

app/models/site_setting.rb

@@ -88,17 +88,6 @@ class SiteSetting < ActiveRecord::Base
     use_https? ? "https" : "http"
   end
 
-  def self.has_enough_topics_to_redirect_to_top
-    TopTopic.periods.each do |period|
-      topics_per_period = TopTopic.where("#{period}_score > 0")
-                                  .limit(SiteSetting.topics_per_period_in_top_page)
-                                  .count
-      return true if topics_per_period >= SiteSetting.topics_per_period_in_top_page
-    end
-    # nothing
-    false
-  end
-
   def self.default_categories_selected
     [
       SiteSetting.default_categories_watching.split("|"),
@@ -107,6 +96,15 @@ class SiteSetting < ActiveRecord::Base
     ].flatten.to_set
   end
 
+  def self.min_redirected_to_top_period
+    TopTopic.sorted_periods.each do |p|
+      period = p[0]
+      return period if TopTopic.topics_per_period(period) >= SiteSetting.topics_per_period_in_top_page
+    end
+    # not enough topics
+    nil
+  end
+
 end
 
 # == Schema Information

app/models/top_topic.rb

@@ -1,7 +1,15 @@
+require_dependency "distributed_memoizer"
+
 class TopTopic < ActiveRecord::Base
 
   belongs_to :topic
 
+  def self.topics_per_period(period)
+    DistributedMemoizer.memoize("#{Discourse.current_hostname}_topics_per_period_#{period}", 1.day) do
+      TopTopic.where("#{period}_score > 0").count
+    end.to_i
+  end
+
   # The top topics we want to refresh often
   def self.refresh_daily!
     transaction do
@@ -35,6 +43,10 @@ class TopTopic < ActiveRecord::Base
     @@periods ||= [:all, :yearly, :quarterly, :monthly, :weekly, :daily].freeze
   end
 
+  def self.sorted_periods
+    ascending_periods ||= Enum.new(:daily, :weekly, :monthly, :quarterly, :yearly, :all)
+  end
+
   def self.sort_orders
     @@sort_orders ||= [:posts, :views, :likes, :op_likes].freeze
   end

app/models/user.rb

@@ -698,26 +698,32 @@ class User < ActiveRecord::Base
   end
 
   def should_be_redirected_to_top
-    redirected_to_top_reason.present?
+    redirected_to_top.present?
   end
 
-  def redirected_to_top_reason
+  def redirected_to_top
     # redirect is enabled
     return unless SiteSetting.redirect_users_to_top_page
     # top must be in the top_menu
-    return unless SiteSetting.top_menu =~ /top/i
+    return unless SiteSetting.top_menu =~ /(^|\|)top(\||$)/i
-    # there should be enough topics
+    # not enough topics
-    return unless SiteSetting.has_enough_topics_to_redirect_to_top
+    return unless period = SiteSetting.min_redirected_to_top_period
 
     if !seen_before? || (trust_level == 0 && !redirected_to_top_yet?)
       update_last_redirected_to_top!
-      return I18n.t('redirected_to_top_reasons.new_user')
+      return {
+        reason: I18n.t('redirected_to_top_reasons.new_user'),
+        period: period
+      }
     elsif last_seen_at < 1.month.ago
       update_last_redirected_to_top!
-      return I18n.t('redirected_to_top_reasons.not_seen_in_a_month')
+      return {
+        reason: I18n.t('redirected_to_top_reasons.not_seen_in_a_month'),
+        period: period
+      }
     end
 
-    # no reason
+    # don't redirect to top
     nil
   end
 

app/serializers/current_user_serializer.rb

@@ -23,7 +23,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :no_password,
              :can_delete_account,
              :should_be_redirected_to_top,
-             :redirected_to_top_reason,
+             :redirected_to_top,
              :disable_jump_reply,
              :custom_fields,
              :muted_category_ids,
@@ -81,8 +81,8 @@ class CurrentUserSerializer < BasicUserSerializer
     true
   end
 
-  def include_redirected_to_top_reason?
+  def include_redirected_to_top?
-    object.redirected_to_top_reason.present?
+    object.redirected_to_top.present?
   end
 
   def custom_fields

lib/discourse.rb

@@ -207,7 +207,7 @@ module Discourse
   end
 
   def self.base_url
-    return base_url_no_prefix + base_uri
+    base_url_no_prefix + base_uri
   end
 
   def self.enable_readonly_mode

lib/distributed_memoizer.rb

@@ -30,8 +30,7 @@ class DistributedMemoizer
         end
 
       ensure
-        # NOTE: delete regardless so next one in does not need to wait MAX_WAIT
+        # NOTE: delete regardless so next one in does not need to wait MAX_WAIT again
-        #   again
         redis.del(redis_lock_key)
       end
     end
@@ -49,6 +48,7 @@ class DistributedMemoizer
   end
 
   protected
+
   def self.get_lock(redis, redis_lock_key)
     redis.watch(redis_lock_key)
     current = redis.get(redis_lock_key)
@@ -61,6 +61,6 @@ class DistributedMemoizer
     end
 
     redis.unwatch
-    return result == ["OK"]
+    result == ["OK"]
   end
 end

spec/models/user_spec.rb

@@ -994,23 +994,23 @@ describe User do
     let!(:user) { Fabricate(:user) }
 
     it "should be redirected to top when there is a reason to" do
-      user.expects(:redirected_to_top_reason).returns("42")
+      user.expects(:redirected_to_top).returns({ reason: "42" })
       expect(user.should_be_redirected_to_top).to eq(true)
     end
 
     it "should not be redirected to top when there is no reason to" do
-      user.expects(:redirected_to_top_reason).returns(nil)
+      user.expects(:redirected_to_top).returns(nil)
       expect(user.should_be_redirected_to_top).to eq(false)
     end
 
   end
 
-  describe ".redirected_to_top_reason" do
+  describe ".redirected_to_top" do
     let!(:user) { Fabricate(:user) }
 
     it "should have no reason when `SiteSetting.redirect_users_to_top_page` is disabled" do
       SiteSetting.expects(:redirect_users_to_top_page).returns(false)
-      expect(user.redirected_to_top_reason).to eq(nil)
+      expect(user.redirected_to_top).to eq(nil)
     end
 
     context "when `SiteSetting.redirect_users_to_top_page` is enabled" do
@@ -1018,19 +1018,20 @@ describe User do
 
       it "should have no reason when top is not in the `SiteSetting.top_menu`" do
         SiteSetting.expects(:top_menu).returns("latest")
-        expect(user.redirected_to_top_reason).to eq(nil)
+        expect(user.redirected_to_top).to eq(nil)
       end
 
       context "and when top is in the `SiteSetting.top_menu`" do
         before { SiteSetting.expects(:top_menu).returns("latest|top") }
 
-        it "should have no reason when there aren't enough topics" do
+        it "should have no reason when there are not enough topics" do
-          SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(false)
+          SiteSetting.expects(:min_redirected_to_top_period).returns(nil)
-          expect(user.redirected_to_top_reason).to eq(nil)
+          expect(user.redirected_to_top).to eq(nil)
         end
 
-        context "and when there are enough topics" do
+        context "and there are enough topics" do
-          before { SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(true) }
+
+          before { SiteSetting.expects(:min_redirected_to_top_period).returns(:monthly) }
 
           describe "a new user" do
             before do
@@ -1042,14 +1043,17 @@ describe User do
               user.expects(:last_redirected_to_top_at).returns(nil)
               user.expects(:update_last_redirected_to_top!).once
 
-              expect(user.redirected_to_top_reason).to eq(I18n.t('redirected_to_top_reasons.new_user'))
+              expect(user.redirected_to_top).to eq({
+                reason: I18n.t('redirected_to_top_reasons.new_user'),
+                period: :monthly
+              })
             end
 
             it "should not have a reason for next visits" do
               user.expects(:last_redirected_to_top_at).returns(10.minutes.ago)
               user.expects(:update_last_redirected_to_top!).never
 
-              expect(user.redirected_to_top_reason).to eq(nil)
+              expect(user.redirected_to_top).to eq(nil)
             end
           end
 
@@ -1060,8 +1064,12 @@ describe User do
               user.last_seen_at = 2.months.ago
               user.expects(:update_last_redirected_to_top!).once
 
-              expect(user.redirected_to_top_reason).to eq(I18n.t('redirected_to_top_reasons.not_seen_in_a_month'))
+              expect(user.redirected_to_top).to eq({
+                reason: I18n.t('redirected_to_top_reasons.not_seen_in_a_month'),
+                period: :monthly
+              })
             end
+
           end
 
         end