Merge pull request #1070 from house9/top-menu

adds TopMenuItem model which encapsulates top_menu parsing logic

app/controllers/list_controller.rb

@@ -14,16 +14,11 @@ class ListController < ApplicationController
         list_opts[:topic_ids] = params[:topic_ids].split(",").map(&:to_i)
       end
 
-      # html format means we need to farm exclude from the site options
-      if params[:format].blank? || params[:format] == "html"
-        #TODO objectify this stuff
-        SiteSetting.top_menu.split('|').each do |f|
-          s = f.split(",")
-          if s[0] == action_name || (action_name == "index" && s[0] == SiteSetting.homepage)
-            list_opts[:exclude_category] = s[1][1..-1] if s.length == 2
-          end
-        end
-      end
+      # html format means we need to parse exclude from the site options top menu
+      menu_item = SiteSetting.top_menu_items.select { |menu_item|
+        menu_item.query_should_exclude_category?(action_name, params[:format])
+      }.first
+      list_opts[:exclude_category] = menu_item.try(:filter)
       list_opts[:exclude_category] = params[:exclude_category] if params[:exclude_category].present?
 
       list = TopicQuery.new(current_user, list_opts).send("list_#{filter}")

app/models/site_setting.rb

@@ -253,14 +253,17 @@ class SiteSetting < ActiveRecord::Base
     min_private_message_post_length..max_post_length
   end
 
+  def self.top_menu_items
+    top_menu.split('|').map { |menu_item| TopMenuItem.new(menu_item) }
+  end
+
   def self.homepage
-    # TODO objectify this
-    top_menu.split('|')[0].split(',')[0]
+    top_menu_items[0].name
   end
 
   def self.anonymous_homepage
-    # TODO objectify this
-    top_menu.split('|').map{|f| f.split(',')[0] }.select{ |f| ['latest', 'hot', 'categories', 'category'].include? f}[0]
+    list = ['latest', 'hot', 'categories', 'category']
+    top_menu_items.map { |item| item.name }.select{ |item| list.include?(item) }.first
   end
 
 end

app/models/top_menu_item.rb

@@ -0,0 +1,72 @@
+# Public: Instances of TopMenuItem should be instantiated from segments contained in SiteSetting.top_menu.
+# Exposes relevant properties and methods that dictate which query methods should be called from the ListController.
+# Segment data should start with a route fragment in one of the following formats:
+#   a topic route, such as 'latest' or 'new' (see ListController for valid options)
+#   the literal string "categories"
+#   a specific category route, must start with 'category/' followed by the route, i.e. 'category/xyz'
+#
+# A topic route can optionally specify a single category to exclude using the '-category' option, i.e. 'new,-xyz'
+#
+# Examples
+#
+#   item = TopMenuItem.new('unread')
+#   item.name           # => "unread"
+#
+#   item = TopMenuItem.new('latest,-video')
+#   item.name           # => "latest"
+#   item.has_filter?    # => true
+#   item.filter         # => "video"
+#
+#   item = TopMenuItem.new('category/hardware')
+#   item.name                     # => "category"
+#   item.has_filter?              # => false
+#   item.has_specific_category?   # => true
+#   item.specific_category        # => "hardware"
+class TopMenuItem
+  def initialize(value)
+    parts = value.split(',')
+    @name = parts[0]
+    @filter = initialize_filter(parts[1])
+  end
+
+  attr_reader :name, :filter
+
+  def has_filter?
+    !filter.nil?
+  end
+
+  def has_specific_category?
+    name.split('/')[0] == 'category'
+  end
+
+  def specific_category
+    name.split('/')[1]
+  end
+
+  def query_should_exclude_category?(action_name, format)
+    if format.blank? || format == "html"
+      matches_action?(action_name) && has_filter?
+    else
+      false
+    end
+  end
+
+  def matches_action?(action_name)
+    return true if action_name == "index" && name == SiteSetting.homepage
+    return true if name == action_name
+    false
+  end
+
+  private
+
+  def initialize_filter(value)
+    if value
+      if value.start_with?('-')
+        value[1..-1] # all but the leading -
+      else
+        Rails.logger.warn "WARNING: found top_menu_item with invalid filter, ignoring '#{value}'..."
+        nil
+      end
+    end
+  end
+end

spec/controllers/list_controller_spec.rb

@@ -6,19 +6,22 @@ describe ListController do
   before do
     @user = Fabricate(:coding_horror)
     @post = Fabricate(:post, user: @user)
+
+    # forces tests down some code paths
+    SiteSetting.stubs(:top_menu).returns('latest,-video|new|unread|favorited|categories|category/beer')
   end
 
   describe 'indexes' do
 
     [:latest, :hot].each do |filter|
-      context '#{filter}' do
+      context "#{filter}" do
         before { xhr :get, filter }
         it { should respond_with(:success) }
       end
     end
 
     [:favorited, :read, :posted, :unread, :new].each do |filter|
-      context '#{filter}' do
+      context "#{filter}" do
         it { expect { xhr :get, filter }.to raise_error(Discourse::NotLoggedIn) }
       end
     end

spec/models/site_setting_spec.rb

@@ -63,7 +63,30 @@ describe SiteSetting do
     it 'is always the correct default' do
       expect(SiteSetting.contact_email).to eq('')
     end
+  end
 
+  describe "anonymous_homepage" do
+    it "returns latest" do
+      expect(SiteSetting.anonymous_homepage).to eq('latest')
+    end
+  end
+
+  describe "top_menu" do
+    before(:each) { SiteSetting.stubs(:top_menu).returns('one,-nope|two|three,-not|four,ignored|category/xyz') }
+
+    describe "items" do
+      let(:items) { SiteSetting.top_menu_items }
+
+      it 'returns TopMenuItem objects' do
+        expect(items[0]).to be_kind_of(TopMenuItem)
+      end
+    end
+
+    describe "homepage" do
+      it "has homepage" do
+        expect(SiteSetting.homepage).to eq('one')
+      end
+    end
   end
 
 end

spec/models/top_menu_item_spec.rb

@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe TopMenuItem do
+  before(:each) { SiteSetting.stubs(:top_menu).returns('one,-nope|two|three,-not|four,ignored|category/xyz') }
+  let(:items) { SiteSetting.top_menu_items }
+
+  it 'has name' do
+    expect(items[0].name).to eq('one')
+    expect(items[1].name).to eq('two')
+    expect(items[2].name).to eq('three')
+  end
+
+  it 'has a filter' do
+    expect(items[0].filter).to eq('nope')
+    expect(items[0].has_filter?).to be_true
+    expect(items[2].filter).to eq('not')
+    expect(items[2].has_filter?).to be_true
+  end
+
+  it 'does not have a filter' do
+    expect(items[1].filter).to be_nil
+    expect(items[1].has_filter?).to be_false
+    expect(items[3].filter).to be_nil
+    expect(items[3].has_filter?).to be_false
+  end
+
+  it "has a specific category" do
+    expect(items.first.has_specific_category?).to be_false
+    expect(items.last.has_specific_category?).to be_true
+  end
+
+  it "does not have a specific category" do
+    expect(items.first.specific_category).to be_nil
+    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, nil)).to be_true
+    end
+
+    it "does not exclude for json format" do
+      expect(items[0].query_should_exclude_category?(nil, 'json')).to be_false
+    end
+  end
+end