Rewrite has_many form structure

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

@@ -2,12 +2,12 @@ $ ->
   # Provides a before-removal hook:
   # $ ->
   #   # This is a good place to tear down JS plugins to prevent memory leaks.
-  #   $(document).on 'has_many_remove:before', '.has_many', (e, fieldset)->
+  #   $(document).on 'has_many_remove:before', '.has_many_container', (e, fieldset)->
   #     fieldset.find('.select2').select2 'destroy'
   #
   $(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 ]
@@ -16,19 +16,19 @@ $ ->
   # Provides before and after creation hooks:
   # $ ->
   #   # The before hook allows you to prevent the creation of new records.
-  #   $(document).on 'has_many_add:before', '.has_many', (e)->
+  #   $(document).on 'has_many_add:before', '.has_many_container', (e)->
   #     if $(@).children('fieldset').length >= 3
   #       alert "you've reached the maximum number of items"
   #       e.preventDefault()
   #
   #   # The after hook is a good place to initialize JS plugins and the like.
-  #   $(document).on 'has_many_add:after', '.has_many', (e, fieldset)->
+  #   $(document).on 'has_many_add:after', '.has_many_container', (e, fieldset)->
   #     fieldset.find('select').chosen()
   #
   $(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

@@ -49,9 +49,11 @@ def commit_action_with_cancel_link
     end
 
     def has_many(assoc, options = {}, &block)
-      options = {for: assoc, new_record: true}.merge options
-      options[:class] ||= ""
-      options[:class] << "inputs has_many_fields"
+      # remove options that should not render as attributes
+      custom_settings = :new_record, :allow_destroy, :heading
+      builder_options = {new_record: true}.merge! options.slice  *custom_settings
+      options         = {for: assoc      }.merge! options.except *custom_settings
+      options[:class] = [options[:class], "inputs has_many_fields"].compact.join(' ')
 
       # Add Delete Links
       form_block = proc do |has_many_form|
@@ -62,25 +64,29 @@ def has_many(assoc, options = {}, &block)
           contents << template.content_tag(:li) do
             template.link_to I18n.t('active_admin.has_many_remove'), "#", class: 'button has_many_remove'
           end
-        elsif options[:allow_destroy]
+        elsif builder_options[:allow_destroy]
           has_many_form.input :_destroy, as: :boolean, wrapper_html: {class: 'has_many_delete'},
                                                        label: I18n.t('active_admin.has_many_delete')
         end
         contents
       end
 
-      form_buffers.last << with_new_form_buffer do
-        template.content_tag :div, class: "has_many #{assoc}" do
-          unless options.key?(:heading) && !options[:heading]
-            form_buffers.last << template.content_tag(:h3) do
-              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, options[:new_record]) if 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
 
@@ -130,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