From c21d3f41d0bb7509b66c51e1b864c34d92762830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 5 May 2014 19:00:40 +0200 Subject: [PATCH] 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 | 11 +++ app/jobs/regular/update_user_info.rb | 12 --- app/models/user.rb | 31 +++++-- app/serializers/current_user_serializer.rb | 2 +- config/locales/server.en.yml | 1 - config/site_settings.yml | 1 - ..._add_last_redirected_to_top_at_to_users.rb | 5 ++ spec/models/user_spec.rb | 80 ++++++++++--------- 8 files changed, 87 insertions(+), 56 deletions(-) create mode 100644 app/jobs/regular/update_top_redirection.rb delete mode 100644 app/jobs/regular/update_user_info.rb create mode 100644 db/migrate/20140505145918_add_last_redirected_to_top_at_to_users.rb diff --git a/app/jobs/regular/update_top_redirection.rb b/app/jobs/regular/update_top_redirection.rb new file mode 100644 index 000000000..8de2f03c0 --- /dev/null +++ b/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 diff --git a/app/jobs/regular/update_user_info.rb b/app/jobs/regular/update_user_info.rb deleted file mode 100644 index 01357528d..000000000 --- a/app/jobs/regular/update_user_info.rb +++ /dev/null @@ -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 diff --git a/app/models/user.rb b/app/models/user.rb index 12b47f6f7..c9b60a9bc 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 9becf6ab2..903043204 100644 --- a/app/serializers/current_user_serializer.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5f635562d..8363d0f31 100644 --- a/config/locales/server.en.yml +++ b/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)" diff --git a/config/site_settings.yml b/config/site_settings.yml index 0503e3573..964ce74ac 100644 --- a/config/site_settings.yml +++ b/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 diff --git a/db/migrate/20140505145918_add_last_redirected_to_top_at_to_users.rb b/db/migrate/20140505145918_add_last_redirected_to_top_at_to_users.rb new file mode 100644 index 000000000..8c09b88da --- /dev/null +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0e3408376..050d14dff 100644 --- a/spec/models/user_spec.rb +++ b/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