FIX: default top timeframe was overriding best_periods_for

FIX: default top timeframe was overriding best_periods_for

diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb
index f05e1a4..b835366 100644
--- a/app/controllers/list_controller.rb
+++ b/app/controllers/list_controller.rb
@@ -433,24 +433,24 @@ class ListController < ApplicationController
   end
 
   def self.best_period_with_topics_for(previous_visit_at, category_id = nil, default_period = SiteSetting.top_page_default_timeframe)
-    best_periods_for(previous_visit_at, default_period.to_sym).each do |period|
+    best_periods_for(previous_visit_at, default_period.to_sym).find do |period|
       top_topics = TopTopic.where("#{period}_score > 0")
       top_topics = top_topics.joins(:topic).where("topics.category_id = ?", category_id) if category_id
       top_topics = top_topics.limit(SiteSetting.topics_per_period_in_top_page)
-      return period if top_topics.count == SiteSetting.topics_per_period_in_top_page
+      top_topics.count == SiteSetting.topics_per_period_in_top_page
     end
-
-    nil
   end
 
   def self.best_periods_for(date, default_period = :all)
-    date ||= 1.year.ago
+    return [default_period, :all].uniq unless date
+
     periods = []
-    periods << default_period if :all     != default_period
-    periods << :daily         if :daily   != default_period && date > 8.days.ago
-    periods << :weekly        if :weekly  != default_period && date > 35.days.ago
-    periods << :monthly       if :monthly != default_period && date > 180.days.ago
-    periods << :yearly        if :yearly  != default_period
+    periods << :daily     if date > (1.week + 1.day).ago
+    periods << :weekly    if date > (1.month + 1.week).ago
+    periods << :monthly   if date > (3.months + 3.weeks).ago
+    periods << :quarterly if date > (1.year + 1.month).ago
+    periods << :yearly    if date > 3.years.ago
+    periods << :all
     periods
   end
 
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index 987116c..587c680 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -566,52 +566,22 @@ RSpec.describe ListController do
   end
 
   describe "best_periods_for" do
-    it "returns yearly for more than 180 days" do
-      expect(ListController.best_periods_for(nil, :all)).to eq([:yearly])
-      expect(ListController.best_periods_for(180.days.ago, :all)).to eq([:yearly])
-    end
-
-    it "includes monthly when less than 180 days and more than 35 days" do
-      (35...180).each do |date|
-        expect(ListController.best_periods_for(date.days.ago, :all)).to eq([:monthly, :yearly])
-      end
-    end
-
-    it "includes weekly when less than 35 days and more than 8 days" do
-      (8...35).each do |date|
-        expect(ListController.best_periods_for(date.days.ago, :all)).to eq([:weekly, :monthly, :yearly])
-      end
-    end
-
-    it "includes daily when less than 8 days" do
-      (0...8).each do |date|
-        expect(ListController.best_periods_for(date.days.ago, :all)).to eq([:daily, :weekly, :monthly, :yearly])
-      end
-    end
-
-    it "returns default even for more than 180 days" do
-      expect(ListController.best_periods_for(nil, :monthly)).to eq([:monthly, :yearly])
-      expect(ListController.best_periods_for(180.days.ago, :monthly)).to eq([:monthly, :yearly])
-    end
-
-    it "returns default even when less than 180 days and more than 35 days" do
-      (35...180).each do |date|
-        expect(ListController.best_periods_for(date.days.ago, :weekly)).to eq([:weekly, :monthly, :yearly])
-      end
-    end
-
-    it "returns default even when less than 35 days and more than 8 days" do
-      (8...35).each do |date|
-        expect(ListController.best_periods_for(date.days.ago, :daily)).to eq([:daily, :weekly, :monthly, :yearly])
-      end
-    end
-
-    it "doesn't return default when set to all" do
-      expect(ListController.best_periods_for(nil, :all)).to eq([:yearly])
-    end
-
-    it "doesn't return value twice when matches default" do
-      expect(ListController.best_periods_for(nil, :yearly)).to eq([:yearly])
+    it "works" do
+      expect(ListController.best_periods_for(nil)).to eq([:all])
+      expect(ListController.best_periods_for(5.years.ago)).to eq([:all])
+      expect(ListController.best_periods_for(2.years.ago)).to eq([:yearly, :all])
+      expect(ListController.best_periods_for(6.months.ago)).to eq([:quarterly, :yearly, :all])
+      expect(ListController.best_periods_for(2.months.ago)).to eq([:monthly, :quarterly, :yearly, :all])
+      expect(ListController.best_periods_for(2.weeks.ago)).to eq([:weekly, :monthly, :quarterly, :yearly, :all])
+      expect(ListController.best_periods_for(2.days.ago)).to eq([:daily, :weekly, :monthly, :quarterly, :yearly, :all])
+    end
+
+    it "supports default period" do
+      expect(ListController.best_periods_for(nil, :yearly)).to eq([:yearly, :all])
+      expect(ListController.best_periods_for(nil, :quarterly)).to eq([:quarterly, :all])
+      expect(ListController.best_periods_for(nil, :monthly)).to eq([:monthly, :all])
+      expect(ListController.best_periods_for(nil, :weekly)).to eq([:weekly, :all])
+      expect(ListController.best_periods_for(nil, :daily)).to eq([:daily, :all])
     end
   end

GitHub sha: 33bc8c27

This commit has been mentioned on Discourse Meta. There might be relevant details there: