FIX: redirect to top was always redirecting to 'All'

app/assets/javascripts/discourse/routes/discovery.js.es6

@@ -11,16 +11,16 @@ export default Discourse.Route.extend(OpenComposer, {
   },
 
   beforeModel(transition) {
+    const user = Discourse.User;
+    const url = transition.intent.url;
+
     if (
-      (transition.intent.url === "/" ||
-        transition.intent.url === "/latest" ||
-        transition.intent.url === "/categories") &&
+      (url === "/" || url === "/latest" || url === "/categories") &&
       transition.targetName.indexOf("discovery.top") === -1 &&
-      Discourse.User.currentProp("should_be_redirected_to_top")
+      user.currentProp("should_be_redirected_to_top")
     ) {
-      Discourse.User.currentProp("should_be_redirected_to_top", false);
-      const period =
-        Discourse.User.currentProp("redirect_to_top.period") || "all";
+      user.currentProp("should_be_redirected_to_top", false);
+      const period = user.currentProp("redirected_to_top.period") || "all";
       this.replaceWith(`discovery.top${period.capitalize()}`);
     }
   },

app/models/site_setting.rb

@@ -77,12 +77,12 @@ class SiteSetting < ActiveRecord::Base
 
   def self.should_download_images?(src)
     setting = disabled_image_download_domains
-    return true unless setting.present?
+    return true if setting.blank?
 
     host = URI.parse(src).host
-    return !(setting.split('|').include?(host))
+    !setting.split("|").include?(host)
   rescue URI::Error
-    return true
+    true
   end
 
   def self.scheme
@@ -99,11 +99,7 @@ class SiteSetting < ActiveRecord::Base
   end
 
   def self.min_redirected_to_top_period(duration)
-    period = ListController.best_period_with_topics_for(duration)
-    return period if period
-
-    # not enough topics
-    nil
+    ListController.best_period_with_topics_for(duration)
   end
 
   def self.queue_jobs=(val)

test/javascripts/acceptance/redirect-to-top-test.js.es6

@@ -3,14 +3,51 @@ import DiscoveryFixtures from "fixtures/discovery_fixtures";
 
 acceptance("Redirect to Top", {
   pretend(server, helper) {
+    server.get("/top/weekly.json", () => {
+      return helper.response(DiscoveryFixtures["/latest.json"]);
+    });
+    server.get("/top/monthly.json", () => {
+      return helper.response(DiscoveryFixtures["/latest.json"]);
+    });
     server.get("/top/all.json", () => {
       return helper.response(DiscoveryFixtures["/latest.json"]);
     });
   }
 });
 
-function setupUser() {
+QUnit.test("redirects categories to weekly top", async assert => {
   logIn();
+
+  replaceCurrentUser({
+    should_be_redirected_to_top: true,
+    redirected_to_top: {
+      period: "weekly",
+      reason: "Welcome back!"
+    }
+  });
+
+  await visit("/categories");
+  assert.equal(currentPath(), "discovery.topWeekly", "it works for categories");
+});
+
+QUnit.test("redirects latest to monthly top", async assert => {
+  logIn();
+
+  replaceCurrentUser({
+    should_be_redirected_to_top: true,
+    redirected_to_top: {
+      period: "monthly",
+      reason: "Welcome back!"
+    }
+  });
+
+  await visit("/latest");
+  assert.equal(currentPath(), "discovery.topMonthly", "it works for latest");
+});
+
+QUnit.test("redirects root to All top", async assert => {
+  logIn();
+
   replaceCurrentUser({
     should_be_redirected_to_top: true,
     redirected_to_top: {
@@ -18,22 +55,7 @@ function setupUser() {
       reason: "Welcome back!"
     }
   });
-}
 
-QUnit.test("redirects categories to top", async assert => {
-  setupUser();
-  await visit("/categories");
-  assert.equal(currentPath(), "discovery.topAll", "it works for categories");
-});
-
-QUnit.test("redirects latest to top", async assert => {
-  setupUser();
-  await visit("/latest");
-  assert.equal(currentPath(), "discovery.topAll", "it works for latest");
-});
-
-QUnit.test("redirects root to top", async assert => {
-  setupUser();
   await visit("/");
   assert.equal(currentPath(), "discovery.topAll", "it works for root");
 });