BUGFIX: proper handling of /none subcategory

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

@@ -1,5 +1,5 @@
 /**
-  The controller for discoverying "Top" topics
+  The controller for discoverying 'Top' topics
 
   @class DiscoveryTopController
   @extends Discourse.Controller
@@ -10,14 +10,14 @@ Discourse.DiscoveryTopController = Discourse.ObjectController.extend({
   category: null,
 
   redirectedToTopPageReason: function() {
-    // no need for a reason if the default homepage is "top"
-    if (Discourse.Utilities.defaultHomepage() === "top") { return null; }
+    // no need for a reason if the default homepage is 'top'
+    if (Discourse.Utilities.defaultHomepage() === 'top') { return null; }
     // check if the user is authenticated
     if (Discourse.User.current()) {
-      if (Discourse.User.currentProp("trust_level") === 0) {
-        return I18n.t("filters.top.redirect_reasons.new_user");
-      } else if (!Discourse.User.currentProp("hasBeenSeenInTheLastMonth")) {
-        return I18n.t("filters.top.redirect_reasons.not_seen_in_a_month");
+      if (Discourse.User.currentProp('trust_level') === 0) {
+        return I18n.t('filters.top.redirect_reasons.new_user');
+      } else if (!Discourse.User.currentProp('hasBeenSeenInTheLastMonth')) {
+        return I18n.t('filters.top.redirect_reasons.not_seen_in_a_month');
       }
     }
     // no reason detected
@@ -27,14 +27,16 @@ Discourse.DiscoveryTopController = Discourse.ObjectController.extend({
   hasDisplayedAllTopLists: Em.computed.and('content.yearly', 'content.monthly', 'content.weekly', 'content.daily'),
 
   showMoreUrl: function(period) {
-    var url = "", category = this.get("category");
-    if (category) { url += category.get("url") + "/l"; }
-    url += "/top/" + period;
+    var url = '', category = this.get('category');
+    if (category) {
+      url = '/category/' + Discourse.Category.slugFor(category) + (this.get('noSubcategories') ? '/none' : '') + '/l';
+    }
+    url += '/top/' + period;
     return url;
   },
 
-  showMoreDailyUrl: function() { return this.showMoreUrl("daily"); }.property("category.url"),
-  showMoreWeeklyUrl: function() { return this.showMoreUrl("weekly"); }.property("category.url"),
-  showMoreMonthlyUrl: function() { return this.showMoreUrl("monthly"); }.property("category.url"),
-  showMoreYearlyUrl: function() { return this.showMoreUrl("yearly"); }.property("category.url"),
+  showMoreDailyUrl: function() { return this.showMoreUrl('daily'); }.property('category', 'noSubcategories'),
+  showMoreWeeklyUrl: function() { return this.showMoreUrl('weekly'); }.property('category', 'noSubcategories'),
+  showMoreMonthlyUrl: function() { return this.showMoreUrl('monthly'); }.property('category', 'noSubcategories'),
+  showMoreYearlyUrl: function() { return this.showMoreUrl('yearly'); }.property('category', 'noSubcategories'),
 });

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

@@ -1,5 +1,5 @@
 /**
-  Handles the routes related to "Top"
+  Handles the routes related to 'Top'
 
   @class DiscoveryTopRoute
   @extends Discourse.Route
@@ -29,7 +29,7 @@ Discourse.DiscoveryTopRoute = Discourse.Route.extend({
 });
 
 /**
-  Handles the routes related to "Top" within a category
+  Handles the routes related to 'Top' within a category
 
   @class DiscoveryTopCategoryRoute
   @extends Discourse.Route
@@ -44,7 +44,7 @@ Discourse.DiscoveryTopCategoryRoute = Discourse.Route.extend({
   afterModel: function(model) {
     var self = this,
                noSubcategories = this.get('no_subcategories'),
-               filterMode = "category/" + Discourse.Category.slugFor(model) + (noSubcategories ? "/none" : "") + "/l/top";
+               filterMode = 'category/' + Discourse.Category.slugFor(model) + (noSubcategories ? '/none' : '') + '/l/top';
 
     this.controllerFor('search').set('searchContext', model);
 
@@ -70,8 +70,12 @@ Discourse.DiscoveryTopCategoryRoute = Discourse.Route.extend({
     var topList = this.get('topList');
     var filterText = I18n.t('filters.top.title');
     Discourse.set('title', I18n.t('filters.with_category', {filter: filterText, category: model.get('name').capitalize()}));
-    this.controllerFor('discoveryTop').setProperties({ model: topList, category: model });
     this.controllerFor('navigationCategory').set('canCreateTopic', topList.get('can_create_topic'));
+    this.controllerFor('discoveryTop').setProperties({
+      model: topList,
+      category: model,
+      noSubcategories: this.get('no_subcategories')
+    });
     this.set('topList', null);
   },
 

app/controllers/list_controller.rb

@@ -1,22 +1,40 @@
 class ListController < ApplicationController
 
+  skip_before_filter :check_xhr
+
+  @@categories = [
+    # filtered topics lists
+    Discourse.filters.map { |f| "#{f}_category".to_sym },
+    Discourse.filters.map { |f| "#{f}_category_none".to_sym },
+    # top summary
+    :top_category,
+    :top_category_none,
+    # top pages (ie. with a period)
+    TopTopic.periods.map { |p| "top_#{p}_category".to_sym },
+    TopTopic.periods.map { |p| "top_#{p}_category_none".to_sym },
+    # category feeds
+    :category_feed,
+  ].flatten
+
+  before_filter :set_category, only: @@categories
+
   before_filter :ensure_logged_in, except: [
     :topics_by,
     # anonymous filters
-    Discourse.anonymous_filters, Discourse.anonymous_filters.map { |f| "#{f}_feed".to_sym },
-    # category
-    :category, :category_feed,
+    Discourse.anonymous_filters,
+    Discourse.anonymous_filters.map { |f| "#{f}_feed".to_sym },
+    # categories
+    @@categories,
     # top
-    :top_lists, TopTopic.periods.map { |p| "top_#{p}".to_sym }
+    :top,
+    TopTopic.periods.map { |p| "top_#{p}".to_sym }
   ].flatten
 
-  before_filter :set_category, only: [:category, :category_none, :category_feed]
-  skip_before_filter :check_xhr
-
   # Create our filters
   Discourse.filters.each do |filter|
-    define_method(filter) do
+    define_method(filter) do |options = nil|
       list_opts = build_topic_list_options
+      list_opts.merge!(options) if options
       user = list_target_user
       list = TopicQuery.new(user, list_opts).public_send("list_#{filter}")
       list.more_topics_url = construct_url_with(filter, list_opts)
@@ -26,6 +44,14 @@ class ListController < ApplicationController
       end
       respond(list)
     end
+
+    define_method("#{filter}_category") do
+      self.send(filter, { category: @category.id })
+    end
+
+    define_method("#{filter}_category_none") do
+      self.send(filter, { category: @category.id, no_subcategories: true })
+    end
   end
 
   Discourse.anonymous_filters.each do |filter|
@@ -55,14 +81,6 @@ class ListController < ApplicationController
     end
   end
 
-  def category
-    category_response
-  end
-
-  def category_none
-    category_response(no_subcategories: true)
-  end
-
   def category_feed
     guardian.ensure_can_see!(@category)
     discourse_expires_in 1.minute
@@ -72,6 +90,7 @@ class ListController < ApplicationController
     @description = "#{I18n.t('topics_in_category', category: @category.name)} #{@category.description}"
     @atom_link = "#{Discourse.base_url}/category/#{@category.slug}.rss"
     @topic_list = TopicQuery.new.list_new_in_category(@category)
+
     render 'list', formats: [:rss]
   end
 
@@ -81,11 +100,13 @@ class ListController < ApplicationController
     redirect_to latest_path, :status => 301
   end
 
-  def top_lists
+  def top(options = nil)
     discourse_expires_in 1.minute
 
-    options = build_topic_list_options
-    top = generate_top_lists(options)
+    top_options = build_topic_list_options
+    top_options.merge!(options) if options
+
+    top = generate_top_lists(top_options)
 
     respond_to do |format|
       format.html do
@@ -99,28 +120,38 @@ class ListController < ApplicationController
     end
   end
 
+  def top_category
+    options = { category: @category.id }
+    top(options)
+  end
+
+  def top_category_none
+    options = { category: @category.id, no_subcategories: true }
+    top(options)
+  end
+
   TopTopic.periods.each do |period|
-    define_method("top_#{period}") do
-      options = build_topic_list_options
-      options[:per_page] = SiteSetting.topics_per_period_in_top_page
+    define_method("top_#{period}") do |options = nil|
+      top_options = build_topic_list_options
+      top_options.merge!(options) if options
+      top_options[:per_page] = SiteSetting.topics_per_period_in_top_page
       user = list_target_user
-      list = TopicQuery.new(user, options).public_send("list_top_#{period}")
-      list.more_topics_url = construct_url_with(period, options, "top")
+      list = TopicQuery.new(user, top_options).public_send("list_top_#{period}")
+      list.more_topics_url = construct_url_with(period, top_options, "top")
       respond(list)
     end
+
+    define_method("top_#{period}_category") do
+      self.send("top_#{period}", { category: @category.id })
+    end
+
+    define_method("top_#{period}_category_none") do
+      self.send("top_#{period}", { category: @category.id, no_subcategories: true })
+    end
   end
 
   protected
 
-  def category_response(extra_opts=nil)
-    list_opts = build_topic_list_options
-    list_opts.merge!(extra_opts) if extra_opts
-    query = TopicQuery.new(current_user, list_opts)
-    list = query.list_latest
-    list.more_topics_url = construct_url_with(:latest, list_opts)
-    respond(list)
-  end
-
   def respond(list)
     discourse_expires_in 1.minute
 
@@ -158,12 +189,13 @@ class ListController < ApplicationController
     if parent_slug_or_id.present?
       parent_category_id = Category.where(slug: parent_slug_or_id).pluck(:id).first ||
                            Category.where(id: parent_slug_or_id.to_i).pluck(:id).first
-
       raise Discourse::NotFound.new if parent_category_id.blank?
     end
 
     @category = Category.where(slug: slug_or_id, parent_category_id: parent_category_id).includes(:featured_users).first ||
                 Category.where(id: slug_or_id.to_i, parent_category_id: parent_category_id).includes(:featured_users).first
+
+    raise Discourse::NotFound.new if @category.blank?
   end
 
   def build_topic_list_options

app/views/list/list.rss.erb

@@ -7,28 +7,30 @@
     <link><%= @link %></link>
     <description><%= @description %></description>
     <%= "<language>#{lang}</language>" if lang %>
-    <lastBuildDate><%= @topic_list.topics.first.created_at.rfc2822 %></lastBuildDate>
-    <atom:link href="<%= @atom_link %>" rel="self" type="application/rss+xml" />
-    <% @topic_list.topics.each do |topic| %>
-      <% topic_url = Discourse.base_url + topic.relative_url -%>
-      <item>
-        <title><%= topic.title %></title>
-        <author><%= "#{site_email} (@#{topic.user.username}#{" #{topic.user.name}" if topic.user.name.present?})" -%></author>
-        <category><%= topic.category.name %></category>
-        <description><![CDATA[
-          <p><%= t('author_wrote', author: link_to(topic.user.name, userpage_url(topic.user))).html_safe %></p>
-          <blockquote>
-            <%= topic.posts.first.cooked.html_safe %>
-          </blockquote>
-          <p><%= t 'num_posts' %> <%= topic.posts_count %></p>
-          <p><%= t 'num_participants' %> <%= topic.participant_count %></p>
-          <p><%= link_to t('read_full_topic'), topic_url %></p>
-        ]]></description>
-        <link><%= topic_url %></link>
-        <pubDate><%= topic.created_at.rfc2822 %></pubDate>
-        <guid><%= topic_url %></guid>
-        <source url="<%= topic_url %>.rss"><%= topic.title %></source>
-      </item>
+    <% if @topic_list.topics && @topic_list.topics.length > 0 %>
+      <lastBuildDate><%= @topic_list.topics.first.created_at.rfc2822 %></lastBuildDate>
+      <atom:link href="<%= @atom_link %>" rel="self" type="application/rss+xml" />
+      <% @topic_list.topics.each do |topic| %>
+        <% topic_url = Discourse.base_url + topic.relative_url -%>
+        <item>
+          <title><%= topic.title %></title>
+          <author><%= "#{site_email} (@#{topic.user.username}#{" #{topic.user.name}" if topic.user.name.present?})" -%></author>
+          <category><%= topic.category.name %></category>
+          <description><![CDATA[
+            <p><%= t('author_wrote', author: link_to(topic.user.name, userpage_url(topic.user))).html_safe %></p>
+            <blockquote>
+              <%= topic.posts.first.cooked.html_safe %>
+            </blockquote>
+            <p><%= t 'num_posts' %> <%= topic.posts_count %></p>
+            <p><%= t 'num_participants' %> <%= topic.participant_count %></p>
+            <p><%= link_to t('read_full_topic'), topic_url %></p>
+          ]]></description>
+          <link><%= topic_url %></link>
+          <pubDate><%= topic.created_at.rfc2822 %></pubDate>
+          <guid><%= topic_url %></guid>
+          <source url="<%= topic_url %>.rss"><%= topic.title %></source>
+        </item>
+      <% end %>
     <% end %>
   </channel>
 </rss>

config/routes.rb

@@ -200,24 +200,23 @@ Discourse::Application.routes.draw do
 
   resources :categories, :except => :show
   get "category/:id/show" => "categories#show"
-  post "category/:category_id/move" => "categories#move", as: "category_move"
-  get "category/:category.rss" => "list#category_feed", format: :rss, as: "category_feed"
-  get "category/:category" => "list#category"
-  get "category/:category/none" => "list#category_none"
-  get "category/:parent_category/:category" => "list#category"
+  post "category/:category_id/move" => "categories#move"
+  get "category/:category.rss" => "list#category_feed", format: :rss
+  get "category/:parent_category/:category.rss" => "list#category_feed", format: :rss
+  get "category/:category" => "list#latest_category"
+  get "category/:category/none" => "list#latest_category_none"
+  get "category/:parent_category/:category" => "list#latest_category"
 
-  get "top" => "list#top_lists"
-  get "category/:category/l/top" => "list#top_lists"
-  get "category/:category/none/l/top" => "list#top_lists"
-  get "category/:parent_category/:category/l/top" => "list#top_lists"
+  get "top" => "list#top"
+  get "category/:category/l/top" => "list#top_category"
+  get "category/:category/none/l/top" => "list#top_category_none"
+  get "category/:parent_category/:category/l/top" => "list#top_category"
 
   TopTopic.periods.each do |period|
     get "top/#{period}" => "list#top_#{period}"
-    get "top/#{period}/more" => "list#top_#{period}"
-    get "category/:category/l/top/#{period}" => "list#top_#{period}"
-    get "category/:category/l/top/#{period}/more" => "list#top_#{period}"
-    get "category/:parent_category/:category/l/top/#{period}" => "list#top_#{period}"
-    get "category/:parent_category/:category/l/top/#{period}/more" => "list#top_#{period}"
+    get "category/:category/l/top/#{period}" => "list#top_#{period}_category"
+    get "category/:category/none/l/top/#{period}" => "list#top_#{period}_category_none"
+    get "category/:parent_category/:category/l/top/#{period}" => "list#top_#{period}_category"
   end
 
   Discourse.anonymous_filters.each do |filter|
@@ -227,10 +226,12 @@ Discourse::Application.routes.draw do
   Discourse.filters.each do |filter|
     get "#{filter}" => "list##{filter}"
     get "#{filter}/more" => "list##{filter}"
-    get "category/:category/l/#{filter}" => "list##{filter}"
-    get "category/:category/l/#{filter}/more" => "list##{filter}"
-    get "category/:parent_category/:category/l/#{filter}" => "list##{filter}"
-    get "category/:parent_category/:category/l/#{filter}/more" => "list##{filter}"
+    get "category/:category/l/#{filter}" => "list##{filter}_category"
+    get "category/:category/l/#{filter}/more" => "list##{filter}_category"
+    get "category/:category/none/l/#{filter}" => "list##{filter}_category_none"
+    get "category/:category/none/l/#{filter}/more" => "list##{filter}_category_none"
+    get "category/:parent_category/:category/l/#{filter}" => "list##{filter}_category"
+    get "category/:parent_category/:category/l/#{filter}/more" => "list##{filter}_category"
   end
 
   get "search" => "search#query"
@@ -303,6 +304,6 @@ Discourse::Application.routes.draw do
   # special case for categories
   root to: "categories#index", constraints: HomePageConstraint.new("categories"), :as => "categories_index"
   # special case for top
-  root to: "list#top_lists", constraints: HomePageConstraint.new("top"), :as => "top_lists"
+  root to: "list#top", constraints: HomePageConstraint.new("top"), :as => "top_lists"
 
 end

lib/topic_query.rb

@@ -129,7 +129,11 @@ class TopicQuery
   end
 
   def list_new_in_category(category)
-    create_list(:new_in_category, unordered: true) {|l| l.where(category_id: category.id).by_newest.first(25)}
+    create_list(:new_in_category, unordered: true) do |list|
+      list.where(category_id: category.id)
+          .by_newest
+          .first(25)
+    end
   end
 
   def self.new_filter(list, treat_as_new_topic_start_date)

spec/controllers/list_controller_spec.rb

@@ -58,7 +58,7 @@ describe ListController do
 
       context 'with access to see the category' do
         before do
-          xhr :get, :category, category: category.slug
+          xhr :get, :latest_category, category: category.slug
         end
 
         it { should respond_with(:success) }
@@ -66,7 +66,7 @@ describe ListController do
 
       context 'with a link that includes an id' do
         before do
-          xhr :get, :category, category: "#{category.id}-#{category.slug}"
+          xhr :get, :latest_category, category: "#{category.id}-#{category.slug}"
         end
 
         it { should respond_with(:success) }
@@ -77,7 +77,7 @@ describe ListController do
         let!(:other_category) { Fabricate(:category, name: "#{category.id} name") }
 
         before do
-          xhr :get, :category, category: other_category.slug
+          xhr :get, :latest_category, category: other_category.slug
         end
 
         it { should respond_with(:success) }
@@ -92,7 +92,7 @@ describe ListController do
 
         context 'when parent and child are requested' do
           before do
-            xhr :get, :category, parent_category: category.slug, category: sub_category.slug
+            xhr :get, :latest_category, parent_category: category.slug, category: sub_category.slug
           end
 
           it { should respond_with(:success) }
@@ -100,7 +100,7 @@ describe ListController do
 
         context 'when child is requested with the wrong parent' do
           before do
-            xhr :get, :category, parent_category: 'not_the_right_slug', category: sub_category.slug
+            xhr :get, :latest_category, parent_category: 'not_the_right_slug', category: sub_category.slug
           end
 
           it { should_not respond_with(:success) }