From d2974c2a1522381b3c9e1eec21bf57e1cb4fb687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 3 Feb 2014 16:08:00 +0100 Subject: [PATCH] BUGFIX: proper handling of top_menu_items --- app/controllers/list_controller.rb | 82 ++++++++++++++++-------- app/models/top_menu_item.rb | 12 +--- config/routes.rb | 30 ++++----- spec/controllers/list_controller_spec.rb | 10 +-- spec/models/top_menu_item_spec.rb | 28 -------- 5 files changed, 77 insertions(+), 85 deletions(-) diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 2ae03554f3c..89344bb75fe 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -4,14 +4,18 @@ class ListController < ApplicationController @@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, + Discourse.filters.map { |f| "category_#{f}".to_sym }, + Discourse.filters.map { |f| "category_none_#{f}".to_sym }, + Discourse.filters.map { |f| "parent_category_category_#{f}".to_sym }, + Discourse.filters.map { |f| "parent_category_category_none_#{f}".to_sym }, + # top summaries + :category_top, + :category_none_top, + :parent_category_category_top, # 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 }, + TopTopic.periods.map { |p| "category_top_#{p}".to_sym }, + TopTopic.periods.map { |p| "category_none_top_#{p}".to_sym }, + TopTopic.periods.map { |p| "parent_category_category_top_#{p}".to_sym }, # category feeds :category_feed, ].flatten @@ -37,7 +41,7 @@ class ListController < ApplicationController 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) + list.more_topics_url = construct_url_with(list_opts) if Discourse.anonymous_filters.include?(filter) @description = SiteSetting.site_description @rss = filter @@ -45,13 +49,21 @@ class ListController < ApplicationController respond(list) end - define_method("#{filter}_category") do + define_method("category_#{filter}") do self.send(filter, { category: @category.id }) end - define_method("#{filter}_category_none") do + define_method("category_none_#{filter}") do self.send(filter, { category: @category.id, no_subcategories: true }) end + + define_method("parent_category_category_#{filter}") do + self.send(filter, { category: @category.id }) + end + + define_method("parent_category_category_none_#{filter}") do + self.send(filter, { category: @category.id }) + end end Discourse.anonymous_filters.each do |filter| @@ -75,7 +87,7 @@ class ListController < ApplicationController guardian.ensure_can_see_private_messages!(target_user.id) unless action == :topics_by list = generate_list_for(action.to_s, target_user, list_opts) url_prefix = "topics" unless action == :topics_by - url = construct_url_with(action, list_opts, url_prefix) + url = construct_url_with(list_opts, url_prefix) list.more_topics_url = url_for(url) respond(list) end @@ -120,16 +132,21 @@ class ListController < ApplicationController end end - def top_category + def category_top options = { category: @category.id } top(options) end - def top_category_none + def category_none_top options = { category: @category.id, no_subcategories: true } top(options) end + def parent_category_category_top + options = { category: @category.id } + top(options) + end + TopTopic.periods.each do |period| define_method("top_#{period}") do |options = nil| top_options = build_topic_list_options @@ -137,17 +154,21 @@ class ListController < ApplicationController top_options[:per_page] = SiteSetting.topics_per_period_in_top_page user = list_target_user list = TopicQuery.new(user, top_options).public_send("list_top_#{period}") - list.more_topics_url = construct_url_with(period, top_options, "top") + list.more_topics_url = construct_url_with(top_options) respond(list) end - define_method("top_#{period}_category") do + define_method("category_top_#{period}") do self.send("top_#{period}", { category: @category.id }) end - define_method("top_#{period}_category_none") do + define_method("category_none_top_#{period}") do self.send("top_#{period}", { category: @category.id, no_subcategories: true }) end + + define_method("parent_category_category_#{period}") do + self.send("top_#{period}", { category: @category.id }) + end end protected @@ -171,10 +192,12 @@ class ListController < ApplicationController end end - def next_page_params(opts=nil) - opts = opts || {} + def next_page_params(opts = nil) + opts ||= {} route_params = { format: 'json', page: params[:page].to_i + 1 } - route_params[:sort_order] = opts[:sort_order] if opts[:sort_order].present? + route_params[:category] = @category.slug if @category + route_params[:parent_category] = @category.parent_category.slug if @category && @category.parent_category + route_params[:sort_order] = opts[:sort_order] if opts[:sort_order].present? route_params[:sort_descending] = opts[:sort_descending] if opts[:sort_descending].present? route_params end @@ -199,15 +222,11 @@ class ListController < ApplicationController end def build_topic_list_options - menu_items = SiteSetting.top_menu_items - menu_item = menu_items.select { |item| item.query_should_exclude_category?(action_name) }.first - menu_item = nil if menu_item.try(:filter).try(:parameterize) == params[:category] - # exclude_category = 1. from params / 2. parsed from top menu / 3. nil options = { page: params[:page], topic_ids: param_to_integer_list(:topic_ids), - exclude_category: (params[:exclude_category] || menu_item.try(:filter)), + exclude_category: (params[:exclude_category] || select_menu_item.try(:filter)), category: params[:category], sort_order: params[:sort_order], sort_descending: params[:sort_descending], @@ -218,6 +237,17 @@ class ListController < ApplicationController options end + def select_menu_item + menu_item = SiteSetting.top_menu_items.select do |mu| + (mu.has_specific_category? && mu.specific_category == @category.try(:slug)) || + action_name == mu.name || + (action_name.include?("top") && mu.name == "top") + end.first + + menu_item = nil if menu_item.try(:has_specific_category?) && menu_item.specific_category == @category.try(:slug) + menu_item + end + def list_target_user if params[:user_id] && guardian.is_staff? User.find(params[:user_id].to_i) @@ -230,8 +260,8 @@ class ListController < ApplicationController TopicQuery.new(current_user, opts).send("list_#{action}", target_user) end - def construct_url_with(action, opts, url_prefix=nil) - method = url_prefix.blank? ? "#{action}_path" : "#{url_prefix}_#{action}_path" + def construct_url_with(opts, url_prefix = nil) + method = url_prefix.blank? ? "#{action_name}_path" : "#{url_prefix}_#{action_name}_path" public_send(method, opts.merge(next_page_params(opts))) end diff --git a/app/models/top_menu_item.rb b/app/models/top_menu_item.rb index c7b4ff44a82..302a26452b3 100644 --- a/app/models/top_menu_item.rb +++ b/app/models/top_menu_item.rb @@ -32,7 +32,7 @@ class TopMenuItem attr_reader :name, :filter def has_filter? - !filter.nil? + filter.present? end def has_specific_category? @@ -43,16 +43,6 @@ class TopMenuItem name.split('/')[1] end - def query_should_exclude_category?(action_name) - matches_action?(action_name) && has_filter? - end - - def matches_action?(action_name) - return true if action_name == "index" && name == SiteSetting.homepage - return true if action_name.start_with?(name) - false - end - private def initialize_filter(value) diff --git a/config/routes.rb b/config/routes.rb index 08b4903eb92..c22ac90fb8b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -204,20 +204,20 @@ Discourse::Application.routes.draw do 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 "category/:category" => "list#category_latest" + get "category/:category/none" => "list#category_none_latest" + get "category/:parent_category/:category" => "list#parent_category_category_latest" 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" + get "category/:category/l/top" => "list#category_top", as: "category_top" + get "category/:category/none/l/top" => "list#category_none_top", as: "category_none_top" + get "category/:parent_category/:category/l/top" => "list#parent_category_category_top", as: "parent_category_category_top" TopTopic.periods.each do |period| get "top/#{period}" => "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" + get "category/:category/l/top/#{period}" => "list#category_top_#{period}", as: "category_top_#{period}" + get "category/:category/none/l/top/#{period}" => "list#category_none_top_#{period}", as: "category_none_top_#{period}" + get "category/:parent_category/:category/l/top/#{period}" => "list#parent_category_category_top_#{period}", as: "parent_category_category_top_#{period}" end Discourse.anonymous_filters.each do |filter| @@ -227,12 +227,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}_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" + get "category/:category/l/#{filter}" => "list#category_#{filter}", as: "category_#{filter}" + get "category/:category/l/#{filter}/more" => "list#category_#{filter}" + get "category/:category/none/l/#{filter}" => "list#category_none_#{filter}", as: "category_none_#{filter}" + get "category/:category/none/l/#{filter}/more" => "list#category_none_#{filter}" + get "category/:parent_category/:category/l/#{filter}" => "list#parent_category_category_#{filter}", as: "parent_category_category_#{filter}" + get "category/:parent_category/:category/l/#{filter}/more" => "list#parent_category_category_#{filter}" end get "search" => "search#query" diff --git a/spec/controllers/list_controller_spec.rb b/spec/controllers/list_controller_spec.rb index 467654f4a59..41dd68ef78b 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, :latest_category, category: category.slug + xhr :get, :category_latest, 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, :latest_category, category: "#{category.id}-#{category.slug}" + xhr :get, :category_latest, 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, :latest_category, category: other_category.slug + xhr :get, :category_latest, 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, :latest_category, parent_category: category.slug, category: sub_category.slug + xhr :get, :category_latest, 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, :latest_category, parent_category: 'not_the_right_slug', category: sub_category.slug + xhr :get, :category_latest, parent_category: 'not_the_right_slug', category: sub_category.slug end it { should_not respond_with(:success) } diff --git a/spec/models/top_menu_item_spec.rb b/spec/models/top_menu_item_spec.rb index 084e7790f0e..fc921791d44 100644 --- a/spec/models/top_menu_item_spec.rb +++ b/spec/models/top_menu_item_spec.rb @@ -34,32 +34,4 @@ describe TopMenuItem do expect(items.last.specific_category).to eq('xyz') end - describe "matches_action?" do - it "does not match index on other pages" do - expect(TopMenuItem.new('xxxx').matches_action?("index")).to be_false - end - - it "matches index on homepage" do - expect(items[0].matches_action?("index")).to be_true - end - - it "matches current action" do - expect(items[1].matches_action?("two")).to be_true - end - - it "does not match current action" do - expect(items[1].matches_action?("one")).to be_false - end - end - - describe "query_should_exclude_category" do - before(:each) do - items[0].stubs(:matches_action?).returns(true) - items[0].stubs(:has_filter?).returns(true) - end - - it "excludes category" do - expect(items[0].query_should_exclude_category?(nil)).to be_true - end - end end