diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index e680ce8d683..e4e037170ca 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -171,7 +171,7 @@ class ListController < ApplicationController def top(options=nil) options ||= {} - period = ListController.best_period_for(current_user.try(:previous_visit_at), options[:category]) + period = ListController.best_period_for(current_user.try(:previous_visit_at), options[:category], SiteSetting.top_page_default_timeframe.to_sym) send("top_#{period}", options) end @@ -327,14 +327,14 @@ class ListController < ApplicationController exclude_category_ids.pluck(:id) end - def self.best_period_for(previous_visit_at, category_id=nil) + def self.best_period_for(previous_visit_at, category_id=nil, default_period=nil) best_periods_for(previous_visit_at).each do |period| top_topics = TopTopic.where("#{period}_score > 0") top_topics = top_topics.joins(:topic).where("topics.category_id = ?", category_id) if category_id return period if top_topics.count >= SiteSetting.topics_per_period_in_top_page end - # default period is yearly - SiteSetting.top_page_default_timeframe.to_sym + + default_period end def self.best_periods_for(date) diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index dcc74ecfc31..2a2c04ac468 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -97,13 +97,8 @@ 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 + def self.min_redirected_to_top_period(duration=nil) + return ListController.best_period_for(duration) end def self.email_polling_enabled? diff --git a/app/models/user_option.rb b/app/models/user_option.rb index 1071026c183..99be107d215 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -90,7 +90,7 @@ class UserOption < ActiveRecord::Base # top must be in the top_menu return unless SiteSetting.top_menu =~ /(^|\|)top(\||$)/i # not enough topics - return unless period = SiteSetting.min_redirected_to_top_period + return unless period = SiteSetting.min_redirected_to_top_period(1.days.ago) if !user.seen_before? || (user.trust_level == 0 && !redirected_to_top_yet?) update_last_redirected_to_top! diff --git a/spec/controllers/list_controller_spec.rb b/spec/controllers/list_controller_spec.rb index c07c6ad2f73..03768b08536 100644 --- a/spec/controllers/list_controller_spec.rb +++ b/spec/controllers/list_controller_spec.rb @@ -226,6 +226,63 @@ describe ListController do end end + describe "best_period_for" do + + context "has_top_topics_and_not_seen_recently" do + + SiteSetting.topics_per_period_in_top_page = 2 + SiteSetting.top_page_default_timeframe = 'daily' + + let!(:t1) { Fabricate(:topic) } + let!(:t2) { Fabricate(:topic) } + let!(:t3) { Fabricate(:topic) } + + before do + TopTopic.refresh! + end + + it "should_return_a_time_period" do + expect(ListController.best_period_for(nil, nil, SiteSetting.top_page_default_timeframe.to_sym)).not_to eq(nil) + end + + end + + context "has_top_topics_and_seen_recently" do + + SiteSetting.topics_per_period_in_top_page = 2 + SiteSetting.top_page_default_timeframe = 'daily' + + let!(:t1) { Fabricate(:topic) } + let!(:t2) { Fabricate(:topic) } + let!(:t3) { Fabricate(:topic) } + + before do + TopTopic.refresh! + end + + it "should_return_a_time_period" do + expect(ListController.best_period_for(1.month.ago, nil, SiteSetting.top_page_default_timeframe.to_sym)).not_to eq(nil) + end + + end + + context "does_not_have_top_topics" do + + SiteSetting.topics_per_period_in_top_page = 2 + SiteSetting.top_page_default_timeframe = 'daily' + + before do + TopTopic.refresh! + end + + it "should_not_return_a_time_period" do + expect(ListController.best_period_for(1.month.ago, nil, nil)).to eq(nil) + end + + end + + end + describe "best_periods_for" do it "returns yearly for more than 180 days" do