From fe656fb04d9f23509b8ba1e4c8fd0f429faeb95e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 21 Sep 2015 20:28:20 +0200 Subject: [PATCH] FIX: select appropriate period when redirecting to top --- .../controllers/discovery/topics.js.es6 | 2 +- .../dynamic-route-builders.js.es6 | 2 +- .../discourse/routes/discovery.js.es6 | 3 +- app/jobs/regular/update_top_redirection.rb | 3 +- app/models/site_setting.rb | 20 +++++------ app/models/top_topic.rb | 12 +++++++ app/models/user.rb | 22 +++++++----- app/serializers/current_user_serializer.rb | 6 ++-- lib/discourse.rb | 2 +- lib/distributed_memoizer.rb | 6 ++-- spec/models/user_spec.rb | 34 ++++++++++++------- 11 files changed, 68 insertions(+), 44 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 b/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 index 28a663e38..99f0a4b02 100644 --- a/app/assets/javascripts/discourse/controllers/discovery/topics.js.es6 +++ b/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, diff --git a/app/assets/javascripts/discourse/pre-initializers/dynamic-route-builders.js.es6 b/app/assets/javascripts/discourse/pre-initializers/dynamic-route-builders.js.es6 index 812e2aed9..cbc4cdce5 100644 --- a/app/assets/javascripts/discourse/pre-initializers/dynamic-route-builders.js.es6 +++ b/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; } } diff --git a/app/assets/javascripts/discourse/routes/discovery.js.es6 b/app/assets/javascripts/discourse/routes/discovery.js.es6 index 98d182c42..83b5b4186 100644 --- a/app/assets/javascripts/discourse/routes/discovery.js.es6 +++ b/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()}`); } }, diff --git a/app/jobs/regular/update_top_redirection.rb b/app/jobs/regular/update_top_redirection.rb index b682fe924..496f86f58 100644 --- a/app/jobs/regular/update_top_redirection.rb +++ b/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 + if user = User.find_by(id: args[:user_id]) user.update_column(:last_redirected_to_top_at, args[:redirected_at]) end end diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index 8dbcb329c..ad84ea681 100644 --- a/app/models/site_setting.rb +++ b/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 diff --git a/app/models/top_topic.rb b/app/models/top_topic.rb index 6cfd43759..4181c39bc 100644 --- a/app/models/top_topic.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index b9ca14e32..fdca6f167 100644 --- a/app/models/user.rb +++ b/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 - # there should be enough topics - return unless SiteSetting.has_enough_topics_to_redirect_to_top + return unless SiteSetting.top_menu =~ /(^|\|)top(\||$)/i + # not enough topics + 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 diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 7236c2dc9..c576025cd 100644 --- a/app/serializers/current_user_serializer.rb +++ b/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? - object.redirected_to_top_reason.present? + def include_redirected_to_top? + object.redirected_to_top.present? end def custom_fields diff --git a/lib/discourse.rb b/lib/discourse.rb index 1acf9abb6..a087120ee 100644 --- a/lib/discourse.rb +++ b/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 diff --git a/lib/distributed_memoizer.rb b/lib/distributed_memoizer.rb index ead419caa..c5e7031d3 100644 --- a/lib/distributed_memoizer.rb +++ b/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 - # again + # NOTE: delete regardless so next one in does not need to wait MAX_WAIT 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index be2b49e56..87f730ba5 100644 --- a/spec/models/user_spec.rb +++ b/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 - SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(false) - expect(user.redirected_to_top_reason).to eq(nil) + it "should have no reason when there are not enough topics" do + SiteSetting.expects(:min_redirected_to_top_period).returns(nil) + expect(user.redirected_to_top).to eq(nil) end - context "and when there are enough topics" do - before { SiteSetting.expects(:has_enough_topics_to_redirect_to_top).returns(true) } + context "and there are enough topics" do + + 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