fix MenuItem label sorting of procs

@macfanatic's changes in 452956e changed label procs to be evaluated in the view context,
but this broke `MenuItem` sorting. This commit pushes the logic back down into `MenuItem`,
and provides a way to pass in an optional context in which to evaluate symbols and procs.

+ forces `MenuItem`-specific logic out of `TabbedNavigation`
+ `items` now handles both sorting and display decisions. Call `children` if you want them all
+ `add` was refactored for clarity
+ integrates fixes from activeadmin#1988

Author: seanlinsley

features/menu.feature

@@ -37,3 +37,17 @@ Feature: Menu
     Then I should see a menu item for "Custom Menu"
     When I follow "Custom Menu"
     Then I should be on the admin dashboard page
+
+  Scenario: Adding a resource as a sub menu item
+    Given a configuration of:
+    """
+      ActiveAdmin.register User
+      ActiveAdmin.register Post do
+        menu :parent => 'User'
+      end
+    """
+    When I am on the dashboard
+    Then I should see a menu item for "Users"
+    When I follow "Users"
+    Then the "Users" tab should be selected
+    And I should see a nested menu item for "Posts"

features/step_definitions/menu_steps.rb

@@ -5,3 +5,7 @@
 Then /^I should not see a menu item for "([^"]*)"$/ do |name|
   page.should_not have_css('#tabs li a', :text => name)
 end
+
+Then /^I should see a nested menu item for "([^"]*)"$/ do |name|
+  page.should have_css('#tabs > li > ul > li > a', :text => name)
+end

lib/active_admin/menu.rb

@@ -5,8 +5,8 @@ module ActiveAdmin
   # To build a new menu:
   #
   #   menu = Menu.new do |m|
-  #     m.add :label => "Dashboard", :url => "/"
-  #     m.add :label => "Admin", :url => "/admin"
+  #     m.add label: 'Dashboard', url: '/'
+  #     m.add label: 'Users',     url: '/users'
   #   end
   #
   # If you're interested in configuring a menu item, take a look at the
@@ -15,80 +15,91 @@ module ActiveAdmin
   class Menu
 
     def initialize
+      super # MenuNode
       yield(self) if block_given?
     end
 
     module MenuNode
+      def initialize
+        @children = {}
+      end
+
+      def [](id)
+        @children[normalize_id(id)]
+      end
+      def []=(id, child)
+        @children[normalize_id(id)] = child
+      end
 
-      # Add a new MenuItem to the menu
+      # Recursively builds any given menu items. There are two syntaxes supported,
+      # as shown in the below examples. Both create an identical menu structure.
       #
-      # Example:
+      # Example 1:
       #   menu = Menu.new
-      #   menu.add :label => "Dashboard" do |dash|
-      #     dash.add :label => "My Child Dashboard"
+      #   menu.add label: 'Dashboard' do |dash|
+      #     dash.add label: 'My Child Dashboard'
       #   end
       #
-      # @param [Hash] menu_items Add as many menu items as you pass in
-      def add(item_options)
-        parent_label = item_options[:parent]
-
-        if parent_label
-          child_options = item_options.except(:parent)
-          menu_item = add_with_parent(parent_label, child_options)
+      # Example 2:
+      #   menu = Menu.new
+      #   menu.add label:  'Dashboard'
+      #   menu.add parent: 'Dashboard', label: 'My Child Dashboard'
+      #
+      def add(options)
+        item = if parent = options.delete(:parent)
+          (self[parent] || add(:label => parent)).add options
         else
-          menu_item = add_without_parent(item_options)
+          _add options.merge :parent => self
         end
 
-        yield(menu_item) if block_given?
+        yield(item) if block_given?
 
-        menu_item
-      end
-
-      def [](id)
-        children.fetch(normalize_id(id))
+        item
       end
 
+      # Whether any children match the given item.
       def include?(item)
-        items.include?(item)
+        @children.values.include? item
       end
 
-      # @return Sorted [Array] of [MenuItem]
-      def items
-        children.values.sort
+      # Used in the UI to visually distinguish which menu item is selected.
+      def current?(item)
+        self == item || include?(item)
       end
 
-      private
-
-      def add_without_parent(item_options)
-        menu_item = ActiveAdmin::MenuItem.new(item_options, self)
-        children[menu_item.id] = menu_item
-      end
-
-      def add_with_parent(parent, item_options)
-        parent_id = normalize_id(parent)
-        parent = children.fetch(parent_id){ add(:label => parent) }
-
-        parent.add(item_options)
+      # Returns sorted array of menu items that should be displayed in this context.
+      # Sorts by priority first, then alphabetically by label if needed.
+      def items(context = nil)
+        @children.values.select{ |i| i.display?(context) }.sort do |a,b|
+          result = a.priority       <=> b.priority
+          result = a.label(context) <=> b.label(context) if result == 0
+          result
+        end
       end
 
-      def children
-        @children ||= {}
+      attr_reader :children
+      private
+      attr_writer :children
+
+      # The method that actually adds new menu items. Called by the public method.
+      # If this ID is already taken, transfer the children of the existing item to the new item.
+      def _add(options)
+        item = ActiveAdmin::MenuItem.new(options)
+        item.send :children=, self[item.id].children if self[item.id]
+        self[item.id] = item
       end
 
-      def normalize_id(string)
-        case string
-        when Proc
-          string
+      def normalize_id(id)
+        case id
+        when String, Symbol
+          id.to_s.downcase.gsub ' ', '_'
         else
-          string.to_s.downcase.gsub(" ", "_")
+          raise TypeError, "#{id.class} isn't supported as a Menu ID"
         end
       end
-
     end
 
     include MenuNode
 
   end
-
-
 end

lib/active_admin/menu_item.rb

@@ -1,84 +1,96 @@
-module ActiveAdmin
+require 'active_admin/view_helpers/method_or_proc_helper'
 
+module ActiveAdmin
   class MenuItem
     include Menu::MenuNode
+    include MethodOrProcHelper
 
-    attr_accessor :id, :label, :url, :priority, :display_if_block, :html_options
-    attr_reader :parent
+    attr_reader :html_options, :parent, :priority
 
-    # Build a new menu item
+    # Builds a new menu item
     #
     # @param [Hash] options The options for the menu
     #
-    # @option options [String, Proc] :label
-    #         The label to display for this menu item. It can either be a String or a 
-    #         Proc. If the option is Proc, it is called each time the label is requested.
+    # @option options [String, Symbol, Proc] :label
+    #         The label to display for this menu item.
+    #         Default: Titleized Resource Name
     #
     # @option options [String] :id
-    #         A custom id to reference this menu item with. If empty an id is automatically 
-    #         generated for you.
+    #         A custom id to reference this menu item with.
+    #         Default: underscored_resource_name
     #
-    # @option options [String, Symbol] :url
-    #         A string or symbol representing the url for this item. If it's a symbol, the
-    #         view will automatically call the method for you.
+    # @option options [String, Symbol, Proc] :url
+    #         The URL this item will link to.
     #
     # @option options [Integer] :priority
-    #         MenuItems are sorted by priority then by label. The lower the priority, the 
-    #         earlier in the menu the item will be displayed.
+    #         The lower the priority, the earlier in the menu the item will be displayed.
     #         Default: 10
     #
-    # @option options [Proc] :if
-    #         A block for the view to call to decide if this menu item should be displayed.
-    #         The block should return true of false
+    # @option options [Symbol, Proc] :if
+    #         This decides whether the menu item will be displayed. Evaluated on each request.
     #
     # @option options [Hash] :html_options
     #         A hash of options to pass to `link_to` when rendering the item
     #
-    # @param [ActiveAdmin::MenuItem] parent The parent menu item of this item
+    # @option [ActiveAdmin::MenuItem] :parent
+    #         This menu item's parent. It will be displayed nested below its parent.
     #
-    def initialize(options = {}, parent = nil)
-      @label        = options[:label]
-      @id           = normalize_id(options[:id] || label)
-      @url          = options[:url] || "#"
-      @priority     = options[:priority] || 10
-      @html_options = options[:html_options] || {}
-      @parent       = parent
-
-      @display_if_block = options[:if]
+    # NOTE: for :label, :url, and :if
+    # These options are evaluated in the view context at render time. Symbols are called
+    # as methods on `self`, and Procs are exec'd within `self`.
+    # Here are some examples of what you can do:
+    #
+    #   menu if:  :admin?
+    #   menu url: :new_book_path
+    #   menu url: :awesome_helper_you_defined
+    #   menu label: ->{ User.some_method }
+    #   menu label: ->{ I18n.t 'menus.user' }
+    #
+    def initialize(options = {})
+      super() # MenuNode
+      @label          = options[:label]
+      @dirty_id       = options[:id]           || options[:label]
+      @url            = options[:url]          || '#'
+      @priority       = options[:priority]     || 10
+      @html_options   = options[:html_options] || {}
+      @should_display = options[:if]           || proc{true}
+      @parent         = options[:parent]
 
       yield(self) if block_given? # Builder style syntax
     end
 
-    def parent?
-      !parent.nil?
+    def id
+      @id ||= normalize_id @dirty_id
     end
 
-    def dom_id
-      case id
-      when Proc
-        id
-      else
-        id.to_s.gsub( " ", '_' ).gsub( /[^a-z0-9_]/, '' )
-      end
+    def label(context = nil)
+      render_in_context context, @label
     end
 
-    # Returns an array of the ancestory of this menu item
-    # The first item is the immediate parent fo the item
-    def ancestors
-      return [] unless parent?
-      [parent, parent.ancestors].flatten
+    def url(context = nil)
+      render_in_context context, @url
     end
 
-    def <=>(other)
-      result = priority <=> other.priority
-      result = label.to_s <=> other.label.to_s if result == 0
-      result
+    # Don't display if the :if option passed says so
+    # Don't display if the link isn't real, we have children, and none of the children are being displayed.
+    def display?(context = nil)
+      return false unless render_in_context(context, @should_display)
+      return false if     !real_url?(context) && @children.any? && !items(context).any?
+      true
     end
 
-    # Returns the display if block. If the block was not explicitly defined
-    # a default block always returning true will be returned.
-    def display_if_block
-      @display_if_block || lambda { |_| true }
+    # Returns an array of the ancestory of this menu item.
+    # The first item is the immediate parent of the item.
+    def ancestors
+      parent ? [parent, parent.ancestors].flatten : []
+    end
+
+    private
+
+    # URL is not nil, empty, or '#'
+    def real_url?(context = nil)
+      url = url context
+      url.present? && url != '#'
     end
 
   end

lib/active_admin/resource/menu.rb

@@ -19,15 +19,15 @@ def menu_item_options
         @menu_item_options ||= default_menu_options
       end
 
-      # The default menu options to pass through to MenuItem.new
       def default_menu_options
+        # These local variables are accessible to the procs.
         menu_resource_class = respond_to?(:resource_class) ? resource_class : self
-        the_resource = self # Make it available in the proc
+        resource = self
         {
-          :id     => resource_name.plural,
-          :label  => proc{ plural_resource_label },
-          :url    => proc{ the_resource.route_collection_path(params) },
-          :if     => proc { authorized?(:read, menu_resource_class)  }
+          :id    => resource_name.plural,
+          :label => proc{ resource.plural_resource_label },
+          :url   => proc{ resource.route_collection_path(params) },
+          :if    => proc{ authorized?(:read, menu_resource_class) }
         }
       end
 

lib/active_admin/view_helpers/method_or_proc_helper.rb

@@ -75,4 +75,19 @@ def render_or_call_method_or_proc_on(obj, string_symbol_or_proc, options = {})
       string_symbol_or_proc
     end
   end
+
+  # This method is different from the others in that it calls `instance_eval` on the reciever,
+  # passing it the proc. This evaluates the proc in the context of the reciever, thus changing
+  # what `self` means inside the proc.
+  def render_in_context(context, obj)
+    context ||= self # default to `self`
+    case obj
+    when Proc
+      context.instance_exec &obj
+    when Symbol
+      context.send obj
+    else
+      obj
+    end
+  end
 end