BUGFIX: only redirect new users to top page once

Actually, new users will still be redirected to the top page during the first 30 seconds of their first visit.

app/jobs/regular/update_top_redirection.rb

@@ -0,0 +1,11 @@
+module Jobs
+
+  class UpdateTopRedirection < Jobs::Base
+
+    def execute(args)
+      user = User.where(id: args[:user_id]).first
+      user.update_column(:last_redirected_to_top_at, args[:redirected_at])
+    end
+  end
+
+end

app/jobs/regular/update_user_info.rb

@@ -1,12 +0,0 @@
-module Jobs
-
-  class UpdateUserInfo < Jobs::Base
-
-    def execute(args)
-      user = User.where(id: args[:user_id]).first
-      user.update_last_seen!
-      user.update_ip_address!(args[:ip_address])
-    end
-  end
-
-end

app/models/user.rb

@@ -586,14 +586,35 @@ class User < ActiveRecord::Base
     return unless SiteSetting.top_menu =~ /top/i
     # there should be enough topics
     return unless SiteSetting.has_enough_topics_to_redirect_to_top
-    # new users
-    return I18n.t('redirected_to_top_reasons.new_user') if trust_level == 0 &&
-      created_at > SiteSetting.redirect_new_users_to_top_page_duration.days.ago
-    # long-time-no-see user
-    return I18n.t('redirected_to_top_reasons.not_seen_in_a_month') if last_seen_at && last_seen_at < 1.month.ago
+
+    if !seen_before? || (trust_level == 0 && !redirected_to_top_yet?)
+      update_last_redirected_to_top!
+      return I18n.t('redirected_to_top_reasons.new_user')
+    elsif last_seen_at < 1.month.ago
+      update_last_redirected_to_top!
+      return I18n.t('redirected_to_top_reasons.not_seen_in_a_month')
+    end
+
+    # no reason
     nil
   end
 
+  def redirected_to_top_yet?
+    last_redirected_to_top_at.present?
+  end
+
+  def update_last_redirected_to_top!
+    key = "user:#{id}:update_last_redirected_to_top"
+    delay = SiteSetting.active_user_rate_limit_secs
+
+    # only update last_redirected_to_top_at once every minute
+    return unless $redis.setnx(key, "1")
+    $redis.expire(key, delay)
+
+    # delay the update
+    Jobs.enqueue_in(delay / 2, :update_top_redirection, user_id: self.id, redirected_at: Time.zone.now)
+  end
+
   protected
 
   def cook

app/serializers/current_user_serializer.rb

@@ -66,7 +66,7 @@ class CurrentUserSerializer < BasicUserSerializer
   end
 
   def include_redirected_to_top_reason?
-    object.should_be_redirected_to_top
+    object.redirected_to_top_reason.present?
   end
 
 end

config/locales/server.en.yml

@@ -679,7 +679,6 @@ en:
     topics_per_period_in_top_summary: "How many topics loaded on the top topics summary"
     topics_per_period_in_top_page: "How many topics loaded on the top topics page"
     redirect_users_to_top_page: "Automatically redirect new & long-time-no-see users to top page"
-    redirect_new_users_to_top_page_duration: "Number of days during which new users are automatically redirect to the top page"
 
     enable_badges: "Enable the badge system (experimental)"
 

config/site_settings.yml

@@ -75,7 +75,6 @@ basic:
     default: 30
   topics_per_period_in_top_summary: 20
   topics_per_period_in_top_page: 50
-  redirect_new_users_to_top_page_duration: 7
   category_featured_topics:
     client: true
     default: 3

db/migrate/20140505145918_add_last_redirected_to_top_at_to_users.rb

@@ -0,0 +1,5 @@
+class AddLastRedirectedToTopAtToUsers < ActiveRecord::Migration
+  def change
+    add_column :users, :last_redirected_to_top_at, :datetime
+  end
+end

spec/models/user_spec.rb

@@ -1100,59 +1100,67 @@ describe User do
 
   end
 
-  describe "redirected_to_top_reason" do
+  describe ".redirected_to_top_reason" do
     let!(:user) { Fabricate(:user) }
 
-    it "should have no reason when redirect_users_to_top_page is disabled" do
+    it "should have no reason when `SiteSetting.redirect_users_to_top_page` is disabled" do
       SiteSetting.expects(:redirect_users_to_top_page).returns(false)
       user.redirected_to_top_reason.should == nil
     end
 
-    context "redirect_users_to_top_page is enabled" do
-      before { SiteSetting.stubs(:redirect_users_to_top_page).returns(true) }
+    context "when `SiteSetting.redirect_users_to_top_page` is enabled" do
+      before { SiteSetting.expects(:redirect_users_to_top_page).returns(true) }
 
-      it "should have no reason when top is not in the top_menu" do
+      it "should have no reason when top is not in the `SiteSetting.top_menu`" do
         SiteSetting.expects(:top_menu).returns("latest")
         user.redirected_to_top_reason.should == nil
       end
 
-      it "should have no reason when there isn't enough topics" do
-        SiteSetting.expects(:top_menu).returns("latest|top")
-        SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(false)
-        user.redirected_to_top_reason.should == nil
-      end
+      context "and when top is in the `SiteSetting.top_menu`" do
+        before { SiteSetting.expects(:top_menu).returns("latest|top") }
 
-      describe "new users" do
-        before do
-          user.expects(:trust_level).returns(0)
-          user.stubs(:last_seen_at).returns(1.day.ago)
-          SiteSetting.expects(:top_menu).returns("latest|top")
-          SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(true)
-          SiteSetting.expects(:redirect_new_users_to_top_page_duration).returns(7)
-        end
-
-        it "should have a reason for newly created user" do
-          user.expects(:created_at).returns(5.days.ago)
-          user.redirected_to_top_reason.should == I18n.t('redirected_to_top_reasons.new_user')
-        end
-
-        it "should not have a reason for newly created user" do
-          user.expects(:created_at).returns(10.days.ago)
+        it "should have no reason when there aren't enough topics" do
+          SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(false)
           user.redirected_to_top_reason.should == nil
         end
-      end
 
-      describe "old users" do
-        before do
-          user.stubs(:trust_level).returns(1)
-          SiteSetting.expects(:top_menu).returns("latest|top")
-          SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(true)
+        context "and when there are enough topics" do
+          before { SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(true) }
+
+          describe "a new user" do
+            before do
+              user.stubs(:trust_level).returns(0)
+              user.stubs(:last_seen_at).returns(5.minutes.ago)
+            end
+
+            it "should have a reason for the first visit" do
+              user.expects(:last_redirected_to_top_at).returns(nil)
+              user.expects(:update_last_redirected_to_top!).once
+
+              user.redirected_to_top_reason.should == I18n.t('redirected_to_top_reasons.new_user')
+            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
+
+              user.redirected_to_top_reason.should == nil
+            end
+          end
+
+          describe "an older user" do
+            before { user.stubs(:trust_level).returns(1) }
+
+            it "should have a reason when the user hasn't been seen in a month" do
+              user.last_seen_at = 2.months.ago
+              user.expects(:update_last_redirected_to_top!).once
+
+              user.redirected_to_top_reason.should == I18n.t('redirected_to_top_reasons.not_seen_in_a_month')
+            end
+          end
+
         end
 
-        it "should have a reason for long-time-no-see users" do
-          user.last_seen_at = 2.months.ago
-          user.redirected_to_top_reason.should == I18n.t('redirected_to_top_reasons.not_seen_in_a_month')
-        end
       end
 
     end