From 8d2e5041bc9f9062fe448f44d4750e8ee88b2f61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 17 Jan 2014 23:52:06 +0100 Subject: [PATCH] BUGFIX: proper handling of /none subcategory --- .../controllers/discovery_top_controller.js | 30 +++--- .../discourse/routes/discovery_top_routes.js | 12 ++- app/controllers/list_controller.rb | 100 ++++++++++++------ app/views/list/list.rss.erb | 46 ++++---- config/routes.rb | 39 +++---- lib/topic_query.rb | 6 +- spec/controllers/list_controller_spec.rb | 10 +- 7 files changed, 144 insertions(+), 99 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/discovery_top_controller.js b/app/assets/javascripts/discourse/controllers/discovery_top_controller.js index 4c95239cc..ff77d8e05 100644 --- a/app/assets/javascripts/discourse/controllers/discovery_top_controller.js +++ b/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'), }); diff --git a/app/assets/javascripts/discourse/routes/discovery_top_routes.js b/app/assets/javascripts/discourse/routes/discovery_top_routes.js index 54accbc8a..ea34efa8e 100644 --- a/app/assets/javascripts/discourse/routes/discovery_top_routes.js +++ b/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); }, diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 29a0e273a..bd0a1c1b3 100644 --- a/app/controllers/list_controller.rb +++ b/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 diff --git a/app/views/list/list.rss.erb b/app/views/list/list.rss.erb index eaf7c608b..542a67545 100644 --- a/app/views/list/list.rss.erb +++ b/app/views/list/list.rss.erb @@ -7,28 +7,30 @@ <%= @link %> <%= @description %> <%= "#{lang}" if lang %> - <%= @topic_list.topics.first.created_at.rfc2822 %> - - <% @topic_list.topics.each do |topic| %> - <% topic_url = Discourse.base_url + topic.relative_url -%> - - <%= topic.title %> - <%= "#{site_email} (@#{topic.user.username}#{" #{topic.user.name}" if topic.user.name.present?})" -%> - <%= topic.category.name %> - <%= t('author_wrote', author: link_to(topic.user.name, userpage_url(topic.user))).html_safe %>

-
- <%= topic.posts.first.cooked.html_safe %> -
-

<%= t 'num_posts' %> <%= topic.posts_count %>

-

<%= t 'num_participants' %> <%= topic.participant_count %>

-

<%= link_to t('read_full_topic'), topic_url %>

- ]]> - <%= topic_url %> - <%= topic.created_at.rfc2822 %> - <%= topic_url %> - <%= topic.title %> - + <% if @topic_list.topics && @topic_list.topics.length > 0 %> + <%= @topic_list.topics.first.created_at.rfc2822 %> + + <% @topic_list.topics.each do |topic| %> + <% topic_url = Discourse.base_url + topic.relative_url -%> + + <%= topic.title %> + <%= "#{site_email} (@#{topic.user.username}#{" #{topic.user.name}" if topic.user.name.present?})" -%> + <%= topic.category.name %> + <%= t('author_wrote', author: link_to(topic.user.name, userpage_url(topic.user))).html_safe %>

+
+ <%= topic.posts.first.cooked.html_safe %> +
+

<%= t 'num_posts' %> <%= topic.posts_count %>

+

<%= t 'num_participants' %> <%= topic.participant_count %>

+

<%= link_to t('read_full_topic'), topic_url %>

+ ]]> + <%= topic_url %> + <%= topic.created_at.rfc2822 %> + <%= topic_url %> + <%= topic.title %> + + <% end %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index 70d390e2e..eecb2cd8b 100644 --- a/config/routes.rb +++ b/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 diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 2a6b58b51..accaf07f4 100644 --- a/lib/topic_query.rb +++ b/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) diff --git a/spec/controllers/list_controller_spec.rb b/spec/controllers/list_controller_spec.rb index a53e07235..467654f4a 100644 --- a/spec/controllers/list_controller_spec.rb +++ b/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) }