FEATURE: add period filter in top topics route for tags. (#13415)

And also move all the "top topics by period" routes to query string param.

/top/monthly => /top?period=monthly
/c/:slug/:id/l/top/monthly => /c/:slug/:id/l/top?period=monthly
/tag/:slug/l/top/daily => /tag/:slug/l/top?period=daily (new)

app/assets/javascripts/discourse/app/controllers/discovery-sortable.js

@@ -13,6 +13,7 @@ export const queryParams = {
   before: { replace: true, refreshModel: true },
   bumped_before: { replace: true, refreshModel: true },
   f: { replace: true, refreshModel: true },
+  period: { replace: true, refreshModel: true },
 };
 
 // Basic controller options

app/assets/javascripts/discourse/app/controllers/discovery.js

@@ -37,9 +37,10 @@ export default Controller.extend({
       }/l`;
     }
 
-    url += "/top/" + period;
+    url += "/top";
 
-    const queryParams = this.router.currentRoute.queryParams;
+    let queryParams = this.router.currentRoute.queryParams;
+    queryParams.period = period;
     if (Object.keys(queryParams).length) {
       url =
         `${url}?` +

app/assets/javascripts/discourse/app/controllers/discovery/topics.js

@@ -1,12 +1,4 @@
-import {
+import { alias, empty, equal, gt, not, readOnly } from "@ember/object/computed";
-  alias,
-  empty,
-  equal,
-  gt,
-  not,
-  notEmpty,
-  readOnly,
-} from "@ember/object/computed";
 import BulkTopicSelection from "discourse/mixins/bulk-topic-selection";
 import DiscoveryController from "discourse/controllers/discovery";
 import I18n from "I18n";
@@ -140,7 +132,7 @@ const controllerOpts = {
   allLoaded: empty("model.more_topics_url"),
   latest: endWith("model.filter", "latest"),
   new: endWith("model.filter", "new"),
-  top: notEmpty("period"),
+  top: endWith("model.filter", "top"),
   yearly: equal("period", "yearly"),
   quarterly: equal("period", "quarterly"),
   monthly: equal("period", "monthly"),

app/assets/javascripts/discourse/app/controllers/tag-show.js

@@ -8,6 +8,7 @@ import Topic from "discourse/models/topic";
 import { alias } from "@ember/object/computed";
 import bootbox from "bootbox";
 import { queryParams } from "discourse/controllers/discovery-sortable";
+import { endWith } from "discourse/lib/computed";
 
 export default Controller.extend(BulkTopicSelection, FilterModeMixin, {
   application: controller(),
@@ -27,6 +28,8 @@ export default Controller.extend(BulkTopicSelection, FilterModeMixin, {
   max_posts: null,
   q: null,
   showInfo: false,
+  top: endWith("list.filter", "top"),
+  period: alias("list.for_period"),
 
   @discourseComputed(
     "canCreateTopic",
@@ -131,8 +134,30 @@ export default Controller.extend(BulkTopicSelection, FilterModeMixin, {
         this.setProperties({ order, ascending: false });
       }
 
+      let params = { order, ascending: this.ascending };
+      if (this.period) {
+        params.period = this.period;
+      }
+
       this.transitionToRoute({
-        queryParams: { order, ascending: this.ascending },
+        queryParams: params,
+      });
+    },
+
+    changePeriod(p) {
+      this.set("period", p);
+
+      let params = { period: this.period };
+
+      if (!(this.order === "default" && this.ascending === false)) {
+        params = Object.assign(params, {
+          order: this.order,
+          ascending: this.ascending,
+        });
+      }
+
+      this.transitionToRoute({
+        queryParams: params,
       });
     },
 

app/assets/javascripts/discourse/app/mixins/filter-mode.js

@@ -36,13 +36,7 @@ export default Mixin.create({
 
     set(key, value) {
       this.set("rawFilterMode", value);
-      const parts = value.split("/");
+      this.set("filterType", value.split("/").pop());
-
-      if (parts.length >= 2 && parts[parts.length - 2] === "top") {
-        this.set("filterType", "top");
-      } else {
-        this.set("filterType", parts.pop());
-      }
 
       return value;
     },

app/assets/javascripts/discourse/app/pre-initializers/dynamic-route-builders.js

@@ -34,7 +34,18 @@ export default {
       app[
         `Discovery${filterCapitalized}CategoryNoneController`
       ] = DiscoverySortableController.extend();
-      if (filter !== "top") {
+
+      if (filter === "top") {
+        app.DiscoveryTopRoute = buildTopicRoute("top", {
+          actions: {
+            willTransition() {
+              User.currentProp("should_be_redirected_to_top", false);
+              User.currentProp("redirected_to_top.reason", null);
+              return this._super(...arguments);
+            },
+          },
+        });
+      } else {
         app[`Discovery${filterCapitalized}Route`] = buildTopicRoute(filter);
       }
 
@@ -46,38 +57,6 @@ export default {
       ] = buildCategoryRoute(filter, { no_subcategories: true });
     });
 
-    app.DiscoveryTopRoute = buildTopicRoute("top", {
-      actions: {
-        willTransition() {
-          User.currentProp("should_be_redirected_to_top", false);
-          User.currentProp("redirected_to_top.reason", null);
-          return this._super(...arguments);
-        },
-      },
-    });
-
-    site.get("periods").forEach((period) => {
-      const periodCapitalized = period.capitalize();
-      app[
-        `DiscoveryTop${periodCapitalized}Controller`
-      ] = DiscoverySortableController.extend();
-      app[
-        `DiscoveryTop${periodCapitalized}CategoryController`
-      ] = DiscoverySortableController.extend();
-      app[
-        `DiscoveryTop${periodCapitalized}CategoryNoneController`
-      ] = DiscoverySortableController.extend();
-      app[`DiscoveryTop${periodCapitalized}Route`] = buildTopicRoute(
-        "top/" + period
-      );
-      app[`DiscoveryTop${periodCapitalized}CategoryRoute`] = buildCategoryRoute(
-        "top/" + period
-      );
-      app[
-        `DiscoveryTop${periodCapitalized}CategoryNoneRoute`
-      ] = buildCategoryRoute("top/" + period, { no_subcategories: true });
-    });
-
     app["TagsShowCategoryRoute"] = TagShowRoute.extend();
     app["TagsShowCategoryNoneRoute"] = TagShowRoute.extend({
       noSubcategories: true,

app/assets/javascripts/discourse/app/routes/app-route-map.js

@@ -27,14 +27,7 @@ export default function () {
   });
 
   this.route("discovery", { path: "/", resetNamespace: true }, function () {
-    // top
+    // top by periods - legacy route
-    this.route("top");
-    this.route("topCategoryNone", {
-      path: "/c/*category_slug_path_with_id/none/l/top",
-    });
-    this.route("topCategory", { path: "/c/*category_slug_path_with_id/l/top" });
-
-    // top by periods
     Site.currentProp("periods").forEach((period) => {
       const top = "top" + period.capitalize();
 
@@ -47,7 +40,7 @@ export default function () {
       });
     });
 
-    // filters (e.g. bookmarks, posted, read, unread, latest)
+    // filters (e.g. bookmarks, posted, read, unread, latest, top)
     Site.currentProp("filters").forEach((filter) => {
       this.route(filter, { path: "/" + filter });
       this.route(filter + "CategoryNone", {

app/assets/javascripts/discourse/app/routes/build-category-route.js

@@ -148,8 +148,7 @@ export default (filterArg, params) => {
         category = model.category,
         canCreateTopic = topics.get("can_create_topic"),
         canCreateTopicOnCategory =
-          canCreateTopic && category.get("permission") === PermissionType.FULL,
+          canCreateTopic && category.get("permission") === PermissionType.FULL;
-        filter = this.filter(category);
 
       this.controllerFor("navigation/category").setProperties({
         canCreateTopicOnCategory: canCreateTopicOnCategory,
@@ -162,7 +161,7 @@ export default (filterArg, params) => {
         category,
         period:
           topics.get("for_period") ||
-          (filter.indexOf("/") > 0 ? filter.split("/")[1] : ""),
+          (model.modelParams && model.modelParams.period),
         selected: [],
         noSubcategories: params && !!params.no_subcategories,
         expandAllPinned: true,

app/assets/javascripts/discourse/app/routes/build-topic-route.js

@@ -130,9 +130,7 @@ export default function (filter, extras) {
         const topicOpts = {
           model,
           category: null,
-          period:
+          period: model.get("for_period") || model.get("params.period"),
-            model.get("for_period") ||
-            (filter.indexOf("top/") >= 0 ? filter.split("/")[1] : ""),
           selected: [],
           expandAllPinned: false,
           expandGloballyPinned: true,

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

@@ -7,6 +7,7 @@ import OpenComposer from "discourse/mixins/open-composer";
 import User from "discourse/models/user";
 import { scrollTop } from "discourse/mixins/scroll-top";
 import { setTopicList } from "discourse/lib/topic-list-tracker";
+import Site from "discourse/models/site";
 
 export default DiscourseRoute.extend(OpenComposer, {
   queryParams: {
@@ -19,6 +20,7 @@ export default DiscourseRoute.extend(OpenComposer, {
 
   beforeModel(transition) {
     const url = transition.intent.url;
+    let matches;
     if (
       (url === "/" || url === "/latest" || url === "/categories") &&
       transition.targetName.indexOf("discovery.top") === -1 &&
@@ -26,7 +28,19 @@ export default DiscourseRoute.extend(OpenComposer, {
     ) {
       User.currentProp("should_be_redirected_to_top", false);
       const period = User.currentProp("redirected_to_top.period") || "all";
-      this.replaceWith(`discovery.top${period.capitalize()}`);
+      this.replaceWith("discovery.top", {
+        queryParams: {
+          period: period,
+        },
+      });
+    } else if (url && (matches = url.match(/top\/(.*)$/))) {
+      if (Site.currentProp("periods").includes(matches[1])) {
+        this.replaceWith("discovery.top", {
+          queryParams: {
+            period: matches[1],
+          },
+        });
+      }
     }
   },
 

app/assets/javascripts/discourse/app/templates/tags/show.hbs

@@ -45,21 +45,26 @@
       <div class="full-width">
         <div id="list-area">
           {{#unless loading}}
-            {{#if list.topics}}
+            {{#discovery-topics-list
-              {{#discovery-topics-list
+              model=list
-                model=list
+              refresh=(action "refresh")
-                refresh=(action "refresh")
+              autoAddTopicsToBulkSelect=autoAddTopicsToBulkSelect
-                autoAddTopicsToBulkSelect=autoAddTopicsToBulkSelect
+              bulkSelectEnabled=bulkSelectEnabled
-                bulkSelectEnabled=bulkSelectEnabled
+              addTopicsToBulkSelect=(action "addTopicsToBulkSelect")
-                addTopicsToBulkSelect=(action "addTopicsToBulkSelect")
+              as |discoveryTopicList|
-                as |discoveryTopicList|
+            }}
+              {{#if top}}
+                <div class="top-lists">
+                  {{period-chooser period=period action=(action "changePeriod") fullDay=false}}
+                </div>
+              {{/if}}
+              {{bulk-select-button
+                selected=selected
+                action=(action "refresh")
+                category=category
               }}
-                {{bulk-select-button
-                  selected=selected
-                  action=(action "refresh")
-                  category=category
-                }}
 
+              {{#if list.topics}}
                 {{topic-list
                   topics=list.topics
                   canBulkSelect=canBulkSelect
@@ -74,8 +79,8 @@
                   onScroll=discoveryTopicList.saveScrollPosition
                   scrollOnLoad=true
                 }}
-              {{/discovery-topics-list}}
+              {{/if}}
-            {{/if}}
+            {{/discovery-topics-list}}
 
             <footer class="topic-list-bottom">
               {{topic-dismiss-buttons position="bottom" selectedTopics=selected

app/assets/javascripts/discourse/tests/acceptance/redirect-to-top-test.js

@@ -2,13 +2,13 @@ import {
   acceptance,
   updateCurrentUser,
 } from "discourse/tests/helpers/qunit-helpers";
-import { currentRouteName, visit } from "@ember/test-helpers";
+import { currentURL, visit } from "@ember/test-helpers";
 import DiscoveryFixtures from "discourse/tests/fixtures/discovery-fixtures";
 import { test } from "qunit";
 
 acceptance("Redirect to Top", function (needs) {
   needs.pretender((server, helper) => {
-    server.get("/top/weekly.json", () => {
+    server.get("/top.json?period=weekly", () => {
       return helper.response(DiscoveryFixtures["/latest.json"]);
     });
     server.get("/top/monthly.json", () => {
@@ -30,11 +30,7 @@ acceptance("Redirect to Top", function (needs) {
     });
 
     await visit("/categories");
-    assert.equal(
+    assert.equal(currentURL(), "/top?period=weekly", "it works for categories");
-      currentRouteName(),
-      "discovery.topWeekly",
-      "it works for categories"
-    );
   });
 
   test("redirects latest to monthly top", async function (assert) {
@@ -47,11 +43,7 @@ acceptance("Redirect to Top", function (needs) {
     });
 
     await visit("/latest");
-    assert.equal(
+    assert.equal(currentURL(), "/top?period=monthly", "it works for latest");
-      currentRouteName(),
-      "discovery.topMonthly",
-      "it works for latest"
-    );
   });
 
   test("redirects root to All top", async function (assert) {
@@ -64,6 +56,6 @@ acceptance("Redirect to Top", function (needs) {
     });
 
     await visit("/");
-    assert.equal(currentRouteName(), "discovery.topAll", "it works for root");
+    assert.equal(currentURL(), "/top?period=all", "it works for root");
   });
 });

app/assets/javascripts/discourse/tests/acceptance/topic-discovery-test.js

@@ -124,7 +124,7 @@ acceptance("Topic Discovery", function (needs) {
     await periodChooser.selectRowByValue("yearly");
 
     assert.ok(
-      DiscourseURL.routeTo.calledWith("/top/yearly?f=foo&d=bar"),
+      DiscourseURL.routeTo.calledWith("/top?f=foo&d=bar&period=yearly"),
       "it keeps the query params"
     );
   });

app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js

@@ -35,6 +35,7 @@ export default {
         "latest",
         "unread",
         "new",
+        "top",
         "starred",
         "read",
         "posted",

app/assets/javascripts/discourse/tests/helpers/site.js

@@ -40,6 +40,7 @@ PreloadStore.store("site", {
     "latest",
     "unread",
     "new",
+    "top",
     "starred",
     "read",
     "posted",

app/controllers/list_controller.rb

@@ -84,6 +84,7 @@ class ListController < ApplicationController
       if Discourse.anonymous_filters.include?(filter)
         @description = SiteSetting.site_description
         @rss = filter
+        @rss_description = filter
 
         # Note the first is the default and we don't add a title
         if (filter.to_s != current_homepage) && use_crawler_layout?
@@ -216,7 +217,8 @@ class ListController < ApplicationController
     @link = "#{Discourse.base_url}/top"
     @atom_link = "#{Discourse.base_url}/top.rss"
     @description = I18n.t("rss_description.top")
-    @topic_list = TopicQuery.new(nil).list_top_for(SiteSetting.top_page_default_timeframe.to_sym)
+    period = params[:period] || SiteSetting.top_page_default_timeframe.to_sym
+    @topic_list = TopicQuery.new(nil).list_top_for(period)
 
     render 'list', formats: [:rss]
   end
@@ -253,7 +255,8 @@ class ListController < ApplicationController
 
   def top(options = nil)
     options ||= {}
-    period = ListController.best_period_for(current_user.try(:previous_visit_at), options[:category])
+    period = params[:period]
+    period ||= ListController.best_period_for(current_user.try(:previous_visit_at), options[:category])
     public_send("top_#{period}", options)
   end
 
@@ -276,7 +279,9 @@ class ListController < ApplicationController
       list.for_period = period
       list.more_topics_url = construct_url_with(:next, top_options)
       list.prev_topics_url = construct_url_with(:prev, top_options)
-      @rss = "top_#{period}"
+      @rss = "top"
+      @params = { period: period }
+      @rss_description = "top_#{period}"
 
       if use_crawler_layout?
         @title = I18n.t("js.filters.top.#{period}.title") + " - #{SiteSetting.title}"
@@ -302,8 +307,8 @@ class ListController < ApplicationController
 
       @description = I18n.t("rss_description.top_#{period}")
       @title = "#{SiteSetting.title} - #{@description}"
-      @link = "#{Discourse.base_url}/top/#{period}"
+      @link = "#{Discourse.base_url}/top?period=#{period}"
-      @atom_link = "#{Discourse.base_url}/top/#{period}.rss"
+      @atom_link = "#{Discourse.base_url}/top.rss?period=#{period}"
       @topic_list = TopicQuery.new(nil).list_top_for(period)
 
       render 'list', formats: [:rss]
@@ -338,6 +343,7 @@ class ListController < ApplicationController
     end
 
     route_params[:username] = UrlHelper.encode_component(params[:username]) if params[:username].present?
+    route_params[:period] = params[:period] if params[:period].present?
     route_params
   end
 

app/controllers/tags_controller.rb

@@ -89,7 +89,9 @@ class TagsController < ::ApplicationController
       @list = nil
 
       if filter == :top
-        @list = TopicQuery.new(current_user, list_opts).public_send("list_top_for", SiteSetting.top_page_default_timeframe.to_sym)
+        period = params[:period] || SiteSetting.top_page_default_timeframe.to_sym
+        @list = TopicQuery.new(current_user, list_opts).public_send("list_top_for", period)
+        @list.for_period = period
       else
         @list = TopicQuery.new(current_user, list_opts).public_send("list_#{filter}")
       end

app/models/api_key_scope.rb

@@ -8,10 +8,6 @@ class ApiKeyScope < ActiveRecord::Base
     def list_actions
       actions = %w[list#category_feed]
 
-      TopTopic.periods.each do |p|
-        actions.concat(["list#category_top_#{p}", "list#top_#{p}", "list#top_#{p}_feed"])
-      end
-
       %i[latest unread new top].each { |f| actions.concat(["list#category_#{f}", "list##{f}"]) }
 
       actions

app/serializers/top_list_serializer.rb

@@ -1,22 +0,0 @@
-# frozen_string_literal: true
-
-class TopListSerializer < ApplicationSerializer
-
-  attributes :can_create_topic
-
-  def can_create_topic
-    scope.can_create?(Topic)
-  end
-
-  TopTopic.periods.each do |period|
-    attribute period
-
-    define_method(period) do
-      if resolved = object.public_send(period)
-        TopicListSerializer.new(resolved, scope: scope).as_json
-      end
-    end
-
-  end
-
-end

app/views/list/list.erb

@@ -136,7 +136,7 @@
 <% if @rss %>
   <% content_for :head do %>
     <%= auto_discovery_link_tag(:rss, "#{Discourse.base_url}/posts.rss", title: I18n.t("rss_description.posts")) %>
-    <%= auto_discovery_link_tag(:rss, { action: "#{@rss}_feed" }, title: I18n.t("rss_description.#{@rss}")) %>
+    <%= auto_discovery_link_tag(:rss, { action: "#{@rss}_feed", params: @params || {} }, title: I18n.t("rss_description.#{@rss_description}")) %>
   <% end %>
 <% end %>
 

config/routes.rb

@@ -705,8 +705,8 @@ Discourse::Application.routes.draw do
       get "/none" => "list#category_none_latest"
 
       TopTopic.periods.each do |period|
-        get "/none/l/top/#{period}" => "list#category_none_top_#{period}", as: "category_none_top_#{period}"
+        get "/none/l/top/#{period}", to: redirect("/none/l/top?period=#{period}", status: 301)
-        get "/l/top/#{period}" => "list#category_top_#{period}", as: "category_top_#{period}"
+        get "/l/top/#{period}", to: redirect("/l/top?period=#{period}", status: 301)
       end
 
       Discourse.filters.each do |filter|
@@ -721,8 +721,9 @@ Discourse::Application.routes.draw do
     get "hashtags" => "hashtags#show"
 
     TopTopic.periods.each do |period|
-      get "top/#{period}.rss" => "list#top_#{period}_feed", format: :rss
+      get "top/#{period}.rss", to: redirect("top.rss?period=#{period}", status: 301)
-      get "top/#{period}" => "list#top_#{period}"
+      get "top/#{period}.json", to: redirect("top.json?period=#{period}", status: 301)
+      get "top/#{period}", to: redirect("top?period=#{period}", status: 301)
     end
 
     Discourse.anonymous_filters.each do |filter|

spec/requests/api/topics_spec.rb

@@ -743,7 +743,7 @@ describe 'topics' do
     end
   end
 
-  path '/top/{flag}.json' do
+  path '/top.json?period={flag}' do
     get 'Get the top topics filtered by a flag' do
       tags 'Topics'
       consumes 'application/json'

spec/requests/list_controller_spec.rb

@@ -412,7 +412,7 @@ RSpec.describe ListController do
 
     TopTopic.periods.each do |period|
       it "renders #{period} top RSS" do
-        get "/top/#{period}.rss"
+        get "/top.rss?period=#{period}"
         expect(response.status).to eq(200)
         expect(response.media_type).to eq('application/rss+xml')
       end

spec/requests/tags_controller_spec.rb

@@ -683,11 +683,13 @@ describe TagsController do
     fab!(:category) { Fabricate(:category) }
     fab!(:topic) { Fabricate(:topic, category: category) }
     fab!(:tag_topic)  { Fabricate(:topic, category: category, tags: [tag]) }
+    fab!(:tag_topic2)  { Fabricate(:topic, category: category, tags: [tag]) }
 
     before do
       SiteSetting.top_page_default_timeframe = 'all'
       TopTopic.create!(topic: topic, all_score: 1)
       TopTopic.create!(topic: tag_topic, all_score: 1)
+      TopTopic.create!(topic: tag_topic2, daily_score: 1)
     end
 
     it "can filter by tag" do
@@ -698,6 +700,16 @@ describe TagsController do
       expect(topic_ids).to eq([tag_topic.id])
     end
 
+    it "can filter by tag and period" do
+      get "/tag/#{tag.name}/l/top.json?period=daily"
+      expect(response.status).to eq(200)
+
+      list = response.parsed_body["topic_list"]
+      topic_ids = list["topics"].map { |topic| topic["id"] }
+      expect(topic_ids).to eq([tag_topic2.id])
+      expect(list["for_period"]).to eq("daily")
+    end
+
     it "can filter by both category and tag" do
       get "/tags/c/#{category.slug}/#{category.id}/#{tag.name}/l/top.json"
       expect(response.status).to eq(200)