Fix broken has_many form html and javascript

app/assets/javascripts/active_admin/components/has_many.js.coffee

@@ -7,7 +7,7 @@ $ ->
   #
   $(document).on 'click', 'a.button.has_many_remove', (e)->
     e.preventDefault()
-    parent    = $(@).closest '.has_many'
+    parent    = $(@).closest '.has_many_container'
     to_remove = $(@).closest 'fieldset'
 
     parent.trigger 'has_many_remove:before', [ to_remove ]
@@ -28,7 +28,7 @@ $ ->
   $(document).on 'click', 'a.button.has_many_add', (e)->
     e.preventDefault()
     elem   = $(@)
-    parent = elem.closest '.has_many'
+    parent = elem.closest '.has_many_container'
     parent.trigger before_add = $.Event 'has_many_add:before'
 
     unless before_add.isDefaultPrevented()

app/assets/stylesheets/active_admin/_forms.css.scss

@@ -32,7 +32,7 @@ form {
       }
     }
 
-    ol > div.has_many {
+    ol > li.has_many_container {
       padding: 20px 10px;
       h3 {
         font-size: 12px;
@@ -52,6 +52,13 @@ form {
   }
 
   /* Nested Fieldsets and Legends */
+  li.has_many_container {
+    fieldset.has_many_fields {
+      margin: 10px 0;
+    }
+
+  }
+
   fieldset > ol > li {
     fieldset {
       position:relative;
@@ -70,15 +77,10 @@ form {
       }
 
       ol {
-        float:left;
-        width:74%;
-        margin:0;
-        padding:0 0 0 20%;
-
-        li {
-          padding:0;
-          border:0;
-        }
+        float: left;
+        width: 100%;
+        margin: 0;
+        padding: 0;
       }
     }
   }

lib/active_admin/form_builder.rb

@@ -71,18 +71,22 @@ def has_many(assoc, options = {}, &block)
         contents
       end
 
-      form_buffers.last << with_new_form_buffer do
-        template.content_tag :div, class: "has_many #{assoc}" do
-          unless builder_options.key?(:heading) && !builder_options[:heading]
-            form_buffers.last << template.content_tag(:h3) do
-              builder_options[:heading] || object.class.reflect_on_association(assoc).klass.model_name.human(count: 1.1)
-            end
+      html = without_wrapper do
+        unless builder_options.key?(:heading) && !builder_options[:heading]
+          form_buffers.last << template.content_tag(:h3) do
+            builder_options[:heading] || object.class.reflect_on_association(assoc).klass.model_name.human(count: 1.1)
           end
+        end
 
-          inputs options, &form_block
+        inputs options, &form_block
 
-          form_buffers.last << js_for_has_many(assoc, form_block, template, builder_options[:new_record]) if builder_options[:new_record]
-        end
+        form_buffers.last << js_for_has_many(assoc, form_block, template, builder_options[:new_record]) if builder_options[:new_record]
+      end
+
+      form_buffers.last << if @already_in_an_inputs_block
+        template.content_tag :li,  html, class: "has_many_container #{assoc}"
+      else
+        template.content_tag :div, html, class: "has_many_container #{assoc}"
       end
     end
 
@@ -132,6 +136,16 @@ def with_new_form_buffer
       return_value
     end
 
+    def without_wrapper
+      is_being_wrapped = @already_in_an_inputs_block
+      @already_in_an_inputs_block = false
+
+      html = with_new_form_buffer{ yield }
+
+      @already_in_an_inputs_block = is_being_wrapped
+      html
+    end
+
     # Capture the ADD JS
     def js_for_has_many(assoc, form_block, template, new_record)
       assoc_reflection = object.class.reflect_on_association assoc

spec/support/rails_template.rb

@@ -16,7 +16,9 @@
 inject_into_file 'app/models/post.rb', %q{
   belongs_to :category
   belongs_to :author, :class_name => 'User'
+  has_many :taggings
   accepts_nested_attributes_for :author
+  accepts_nested_attributes_for :taggings
   attr_accessible :author unless Rails::VERSION::MAJOR > 3 && !defined? ProtectedAttributes
 }, :after => 'class Post < ActiveRecord::Base'
 copy_file File.expand_path('../templates/post_decorator.rb', __FILE__), "app/models/post_decorator.rb"
@@ -51,6 +53,12 @@ def set_id
   end
 }, :after => 'class Tag < ActiveRecord::Base'
 
+generate :model, "tagging post_id:integer tag_id:integer"
+inject_into_file 'app/models/tagging.rb', %q{
+  belongs_to :post
+  belongs_to :tag
+}, :after => 'class Tagging < ActiveRecord::Base'
+
 # Configure default_url_options in test environment
 inject_into_file "config/environments/test.rb", "  config.action_mailer.default_url_options = { :host => 'example.com' }\n", :after => "config.cache_classes = true\n"
 

spec/unit/filters/resource_spec.rb

@@ -9,7 +9,7 @@
 
   it "should return the defaults if no filters are set" do
     resource.filters.keys.sort.should == [
-      :author, :body, :category, :created_at, :published_at, :starred, :title, :updated_at
+      :author, :body, :category, :created_at, :published_at, :starred, :taggings, :title, :updated_at
     ]
   end
 
@@ -95,7 +95,7 @@
       resource.add_filter :count, as: :string
 
       resource.filters.keys.sort.should == [
-        :author, :body, :category, :count, :created_at, :published_at, :starred, :title, :updated_at
+        :author, :body, :category, :count, :created_at, :published_at, :starred, :taggings, :title, :updated_at
       ]
     end
 

spec/unit/form_builder_spec.rb

@@ -312,12 +312,12 @@ def build_form(options = {}, form_object = Post.new, &block)
       end
 
       it "should add a link to remove new nested records" do
-        Capybara.string(body).should have_css '.has_many > fieldset > ol > li > a', href: '#',
+        Capybara.string(body).should have_css '.has_many_container > fieldset > ol > li > a', href: '#',
           content: 'Remove', class: 'button has_many_remove', data: {placeholder: 'NEW_POST_RECORD'}
       end
 
       it "should add a link to add new nested records" do
-        Capybara.string(body).should have_css(".has_many > fieldset > ol > li > a", :class => "button", :href => "#", :content => "Add New Post")
+        Capybara.string(body).should have_css(".has_many_container > fieldset > ol > li > a", :class => "button", :href => "#", :content => "Add New Post")
       end
     end
 
@@ -413,7 +413,7 @@ def build_form(options = {}, form_object = Post.new, &block)
         end
 
         it "should wrap the destroy field in an li with class 'has_many_delete'" do
-          Capybara.string(body).should have_css(".has_many > fieldset > ol > li.has_many_delete > input")
+          Capybara.string(body).should have_css(".has_many_container > fieldset > ol > li.has_many_delete > input")
         end
       end
 
@@ -437,6 +437,55 @@ def build_form(options = {}, form_object = Post.new, &block)
       end
     end
 
+    describe "with nesting" do
+      context "in an inputs block" do
+        let :body do
+          build_form({:url => '/categories'}, Category.new) do |f|
+            f.inputs "Field Wrapper" do
+              f.object.posts.build
+              f.has_many :posts do |p|
+                p.input :title
+              end
+            end
+          end
+        end
+
+        it "should wrap the has_many fieldset in an li" do
+          Capybara.string(body).should have_css("ol > li.has_many_container")
+        end
+
+        it "should have a direct fieldset child" do
+          Capybara.string(body).should have_css("li.has_many_container > fieldset")
+        end
+
+        it "should not contain invalid li children" do
+          Capybara.string(body).should_not have_css("div.has_many_container > li")
+        end
+      end
+
+      context "in another has_many block" do
+        let :body do
+          build_form({:url => '/categories'}, Category.new) do |f|
+            f.object.posts.build
+            f.has_many :posts do |p|
+              p.object.taggings.build
+              p.has_many :taggings do |t|
+                t.input :tag
+              end
+            end
+          end
+        end
+
+        it "should wrap the inner has_many fieldset in an ol > li" do
+          Capybara.string(body).should have_css(".has_many_container ol > li.has_many_container > fieldset")
+        end
+
+        it "should not contain invalid li children" do
+          Capybara.string(body).should_not have_css(".has_many_container div.has_many_container > li")
+        end
+      end
+    end
+
     pending "should render the block if it returns nil" do
       body = build_form({:url => '/categories'}, Category.new) do |f|
         f.object.posts.build