BUGFIX: proper handling of top_menu_items

2014-02-03 16:08:00 +01:00 · 2014-02-03 16:08:00 +01:00 · d2974c2a15
parent 632cd44c2f
commit d2974c2a15
5 changed files with 77 additions and 85 deletions
--- 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

--- 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)
--- 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"
--- 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) }
--- 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