From b792db9d92dedd5bcd9386f4169fb6c4ee1b55aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Mon, 3 Jun 2019 20:17:25 +0200
Subject: [PATCH] FIX: redirect to top was always redirecting to 'All'

---
 .../discourse/routes/discovery.js.es6         | 14 ++---
 app/models/site_setting.rb                    | 12 ++---
 .../acceptance/redirect-to-top-test.js.es6    | 54 +++++++++++++------
 3 files changed, 49 insertions(+), 31 deletions(-)

diff --git a/app/assets/javascripts/discourse/routes/discovery.js.es6 b/app/assets/javascripts/discourse/routes/discovery.js.es6
index aa71f49ece4..32926561b6c 100644
--- a/app/assets/javascripts/discourse/routes/discovery.js.es6
+++ b/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()}`);
     }
   },
diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb
index 19c4d2e1e55..fa57bbd990c 100644
--- a/app/models/site_setting.rb
+++ b/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)
diff --git a/test/javascripts/acceptance/redirect-to-top-test.js.es6 b/test/javascripts/acceptance/redirect-to-top-test.js.es6
index 2be06cacf1b..c1c7e66f855 100644
--- a/test/javascripts/acceptance/redirect-to-top-test.js.es6
+++ b/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");
 });