Fixes #1986 - Menu should register child regardless of AA resource load order #1988

Closed
wants to merge 1 commit into
from
View
@@ -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"
@@ -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
@@ -61,7 +61,22 @@ def items
def add_without_parent(item_options)
menu_item = ActiveAdmin::MenuItem.new(item_options, self)
+
+ #
+ # Depending on load order, might have an "empty" parent item already
+ # in the menu, that is just a label. We want to allow you to customize that if
+ # you are registering a resource that comes later, such as "Users" parent and
+ # "Posts" child. The "Users" menu is registered after posts, but posts said it
+ # already was a sub menu of users.
+ #
+ # This will correct that by assigning existing children to this new menu item.
+ #
+ if children.has_key? menu_item.id
+ menu_item.send :children=, self[menu_item.id].send(:children)
+ end
@seanlinsley
seanlinsley Mar 30, 2013 Active Admin member

What's with all the sends? Wouldn't this work?

menu_item.children = self[menu_item.id].children
@seanlinsley
seanlinsley Mar 30, 2013 Active Admin member

Private methods are only private if called out of context. A menu item can call its own "private" methods without any sending.

@seanlinsley
seanlinsley Mar 30, 2013 Active Admin member

Oh nevermind, I see this code just created a new menu item object.

+
children[menu_item.id] = menu_item
+
end
def add_with_parent(parent, item_options)
@@ -75,13 +90,18 @@ def children
@children ||= {}
end
+ def children=(kids)
+ @children = kids
+ end
@seanlinsley
seanlinsley Mar 30, 2013 Active Admin member

We now have this:

def children
  @children ||= {}
end

def children=(kids)
  @children = kids
end

Why not replace both of these with attr_accessor?

attr_accessor :children

Is there a real need to initialize @children with a hash?

Since AA depends on Rails, any places in the code base that checks if there are any children can do this instead:

children.present? # same as !children.blank?

And for inline operations:

children.presence.try :to_do_something
@seanlinsley
seanlinsley Mar 30, 2013 Active Admin member

Disregard the above comment. I implemented what I was looking for in e11fd6c for #2031. It goes something like this:

module MenuNode
  def initialize
    @children = {}
  end
end

class Menu
  include MenuNode
  def initialize
    super
    # ...
  end
end

I'm thinking we'll get your PR pulled in first, then I'll rebase mine.

+
def normalize_id(string)
- case string
+ raw_id = case string
when Proc
- string
+ string.call rescue string
else
- string.to_s.downcase.gsub(" ", "_")
+ string.to_s
end
+ raw_id.downcase.gsub( " ", '_' ).gsub( /[^a-z0-9_]/, '' )
@seanlinsley
seanlinsley Mar 30, 2013 Active Admin member

IMO this is more readable:

def normalize_id(id)
  (id.is_a?(Proc) ? id.call : id).to_s.downcase.gsub ' ', '_'
end

Note that I removed both the unconditional rescue and the regex removing non alphanumeric characters. Why are we doing either of those, and why here?

end
end
@@ -53,15 +53,6 @@ def parent?
!parent.nil?
end
- def dom_id
- case id
- when Proc
- id
- else
- id.to_s.gsub( " ", '_' ).gsub( /[^a-z0-9_]/, '' )
- end
- end
-
# Returns an array of the ancestory of this menu item
# The first item is the immediate parent fo the item
def ancestors
@@ -40,11 +40,11 @@ def build_menu
end
def build_menu_item(item)
- dom_id = case item.dom_id
+ dom_id = case item.id
when Proc,Symbol
- normalize_id call_method_or_proc_on(self, item.dom_id)
+ normalize_id call_method_or_proc_on(self, item.id)
else
- item.dom_id
+ item.id
end
li :id => dom_id do |li_element|
@@ -67,6 +67,10 @@ module ActiveAdmin
i
end
+ it "should contain 5 submenu items" do
+ item.items.count.should == 5
+ end
+
it "should give access to the menu item as an array" do
item['Blog'].label.should == 'Blog'
end
@@ -121,7 +125,7 @@ module ActiveAdmin
end # accessing ancestory
- describe ".generate_item_id" do
+ describe "#id" do
it "downcases the id" do
MenuItem.new(:id => "Identifier").id.should == "identifier"
@@ -131,8 +135,12 @@ module ActiveAdmin
MenuItem.new(:id => "An Id").id.should == "an_id"
end
- it "should return a proc if label was a proc" do
- MenuItem.new(:label => proc{ "Dynamic" }).id.should be_an_instance_of(Proc)
+ it "should render a proc if label was a proc" do
+ MenuItem.new(:label => proc{ "Dynamic Id" }).id.should == "dynamic_id"
+ end
+
+ it "should preserve the ID that was set if the label was a proc" do
+ MenuItem.new(:label => proc{ "Dynamic Id" }, :id => "static_id").id.should == "static_id"
end
end
@@ -50,6 +50,16 @@
menu["Admin"]["Projects"].should be_an_instance_of(ActiveAdmin::MenuItem)
end
+
+ it "should assign children regardless of resource file load order" do
+ menu = Menu.new
+ menu.add :parent => "Users", :label => "Posts"
+ menu.add :label => "Users", :url => '/some/url'
+
+ menu["Users"].url.should == '/some/url'
+ menu["Users"]["Posts"].should be_an_instance_of(ActiveAdmin::MenuItem)
+ end
+
end
end