Rewrite has_many form structure #2679

shekibobo · 2013-11-08T21:06:06Z

Potential solution for #2659.

I'm not sure if the important parts of this PR (cbd141d) are that much different from @igorbernstein's work in #2675, but it does manage to render a valid html structure.

On weird thing is, however, that the hidden id field for has_many get rendered as siblings to the fieldset that contains the other fields related to it, and I have no idea why.

<div class="has_many questions">
  <h3>Questions</h3>
  <fieldset class="inputs has_many_fields" />
  <input id="item_questions_attributes_0_id" ... />
  <fieldset class="inputs has_many_fields" />
  <input id="item_questions_attributes_1_id" ... />
</div>

Aside from this anomaly, this seems to work okay, solving both #2655 and #2659, although not exactly to the spec I proposed there. And apparently we can even support nesting inputs > has_many without generating invalid html. Again, not really sure why mine works and Igor's doesn't, but hey, there it is.

seanlinsley · 2013-11-08T21:14:17Z

lib/active_admin/form_builder.rb

+      is_being_wrapped = @already_in_an_inputs_block
+      @already_in_an_inputs_block = false
+
+      html = with_new_form_buffer do
        template.content_tag :div, class: "has_many #{assoc}" do


The has_many method is already a monster; why not abstract this wrapping behavior into a separate method? Something like:

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

I like it, I'll give it a shot.

shekibobo · 2013-11-08T21:31:37Z

@seanlinsley Any idea why the hidden id inputs aren't being rendered inside their fieldsets?

seanlinsley · 2013-11-08T21:46:02Z

I pulled down this code locally, and I'm getting this:

From this code in the AA test app:

ActiveAdmin.register Category do
  form do |f|
    f.semantic_errors *f.object.errors.keys
    f.inputs
    f.has_many :posts, allow_destroy: true do |p|
      p.input :date, as: :datepicker
    end
    f.actions
  end
end

shekibobo · 2013-11-08T21:47:16Z

Why do you have an f.inputs without arguments? Did you mean to wrap the has_many? Or does that do something I didn't realize?

seanlinsley · 2013-11-08T21:48:00Z

f.inputs without any arguments builds the default form, with all of your normal attributes

shekibobo · 2013-11-08T21:51:25Z

Well that is weird. I just tried that locally and it works just fine.

shekibobo · 2013-11-08T21:56:09Z

Sanity check: are you sure you saved the file with the p.input line in it before testing? Might be you saved it, added it, and then reloaded without saving it?

Sorry, I just want to make sure we're not missing anything because everything works perfectly on my end.

seanlinsley · 2013-11-08T22:02:28Z

Hmm. Looks like the :date attribute no longer existed, so it was dieing. Weirdly if I go back to master, I get the expected exception raised. But with these changes, that error gets swallowed.

seanlinsley · 2013-11-08T22:20:20Z

It doesn't look like the form block gets evaluated, but how could that be the case?

seanlinsley · 2013-11-08T22:29:23Z

I think it has to do with the fact that we're turning off an internal Formtastic variable dealing with f.inputs right as we're about to call f.inputs. If I set the variable to true, at the very least my f.has_many block will be called.

seanlinsley · 2013-11-08T22:30:21Z

What version of Formtastic are you using? I'm using 2.3.0.rc2

igorbernstein · 2013-11-09T01:55:51Z

possible source of the hidden id input:
https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/form_helper.rb#L1862

shekibobo · 2013-11-09T04:29:34Z

I just tried adding an input field for an attribute that didn't exist on my model, and I get 'undefined method broken'. Does it still ignore the error if you don't autoload the fields?

form do |f|
  f.inputs 'Details' do
    f.input :name
  end

  f.has_many :posts do |p|
    p.input :date, as: :datepicker
  end
  f.actions
end

I'm using formtastic 2.3.0.rc2.

shekibobo · 2013-11-09T04:32:18Z

Also, the specs @igorbernstein provided do apparently pass for this build. It appears we're still just failing on our devise password resets.

shekibobo · 2013-11-09T04:47:39Z

@seanlinsley I just figured out how to actually reproduce your test case on my machine and it throws the error just fine. Cannot reproduce. :(

Rails 4.0.1

shekibobo · 2013-11-09T05:11:03Z

I have no idea if the hidden input is anything I can do anything about. It doesn't hurt anything, it renders fine, it has all the right attributes, it's just not inside the related fieldset. It's only for associated models that already exist in the system (hence the id field). It's mostly just an annoyance, but I can accept it.

igorbernstein · 2013-11-09T17:01:28Z

lib/active_admin/form_builder.rb

-      options[:class] ||= ""
-      options[:class] << "inputs has_many_fields"
+      # remove options that should not render as attributes
+      builder_options = {new_record: true}.merge! options.extract!(:new_record, :allow_destroy, :heading)


This is broken:

{new_record: true}.merge!({}.extract!(:new_record, :allow_destroy, :heading)) # {:new_record=>nil, :allow_destroy=>nil, :heading=>nil}

you want to use slice instead

seanlinsley · 2013-11-09T17:12:22Z

I'm still having the same problem as last night. Pulling in these changes, I get an empty div tag, and when debugging it doesn't seem like my has_many block is ever actually run. But if I switch back to master everything works fine.

You're using Rails 4.0.1? I'm using 3.2.15, though I'd be surprised if that was causing this.

seanlinsley · 2013-11-09T17:25:22Z

Hmm, I worked backwards and this problem existed since the very first commit, 12922df7fe929088f3874c2d9298ec67a751bc0a

Maybe related to @igorbernstein's comment: #2679 (comment)

igorbernstein · 2013-11-09T17:27:11Z

I haven't had time to play with this yet. Can you reproduce the issue in a test case?

On Sat, Nov 9, 2013 at 12:12 PM, Sean Linsley notifications@github.com
wrote:

I'm still having the same problem as last night. Pulling in these changes, I get an empty div tag, and when debugging it doesn't seem like my has_many block is ever actually run. But if I switch back to master everything works fine.

You're using Rails 4.0.1? I'm using 3.2.15, though I'd be surprised if that was causing this.

Reply to this email directly or view it on GitHub:
#2679 (comment)

igorbernstein · 2013-11-09T17:34:09Z

Also, can you reproduce this issue on https://github.com/igorbernstein/active_admin/tree/2659-nested-has-many ?

On Sat, Nov 9, 2013 at 12:25 PM, Sean Linsley notifications@github.com
wrote:

Hmm, I worked backwards and this problem existed since the very first commit, 12922df7fe929088f3874c2d9298ec67a751bc0a

Maybe related to @igorbernstein's comment: #2679 (comment)

Reply to this email directly or view it on GitHub:
#2679 (comment)

seanlinsley · 2013-11-09T17:34:45Z

With these changes the form started showing up:

diff --git a/lib/active_admin/form_builder.rb b/lib/active_admin/form_builder.rb
index 2840c9a..e917810 100644
--- a/lib/active_admin/form_builder.rb
+++ b/lib/active_admin/form_builder.rb
@@ -50,8 +50,8 @@ module ActiveAdmin

     def has_many(assoc, options = {}, &block)
       # remove options that should not render as attributes
-      builder_options = {new_record: true}.merge! options.extract!(:new_record, :allow_destroy, :heading)
-      options = {for: assoc}.merge options
+      builder_options = {new_record: true}.merge! options.slice! :new_record, :allow_destroy, :heading
+      options         = {for: assoc      }.merge! options
       options[:class] = [options[:class], "inputs has_many_fields"].compact.join(' ')

shekibobo · 2013-11-09T17:43:14Z

I think options.slice! is too weird to actually use. options would become {new_record: ..., allow_destroy: ..., heading: ...}, and we return all the other options. Feels wrong. The intent is to grab the key-values we want from the original and delete them when we do, but only to get those keys when we actually extract them.

If extract! is creating nil entries on the extracted hash for keys that weren't in the original, then we just can't use it, and maybe we just iterate the keys into a new hash and delete them in the iterator.

seanlinsley · 2013-11-09T17:49:59Z

Yeah, it didn't behave like I expected anyway. This would be a better solution:

def has_many(assoc, options = {}, &block)
  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

igorbernstein · 2013-11-09T17:50:22Z

From phone so pardon the formatting...
Opts1 = opts.slice
Opts2 = opts.except

seanlinsley · 2013-11-10T22:11:39Z

@shekibobo I don't understand your points. My goal is to make the structure as simple and consistent as possible, whether or not you're wrapping in an inputs block. By using the CSS class has_many_container on a div or li as appropriate, I think it makes things much simpler.

... but I don't think we need the extra div if we can help it

What? The goal of this is to remove the currently-added div.has_many when in an inputs block, while still always having a containing element to hook JavaScript onto.

Hmm, maybe you didn't catch that because I didn't explicitly add that code change yesterday. I'm suggesting that, in addition to the changes above, we remove wrapping div:

template.content_tag :div, class: "has_many #{assoc}" do
  # ...

seanlinsley · 2013-11-10T22:20:50Z

The full changes being:

diff --git a/app/assets/javascripts/active_admin/components/has_many.js.coffee b/app/assets/javascripts/active_admin/components/has_many.js.coffee
index 7122c34..d32b439 100644
--- a/app/assets/javascripts/active_admin/components/has_many.js.coffee
+++ b/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()
diff --git a/lib/active_admin/form_builder.rb b/lib/active_admin/form_builder.rb
index 4fbc5e6..385dc90 100644
--- a/lib/active_admin/form_builder.rb
+++ b/lib/active_admin/form_builder.rb
@@ -72,20 +72,21 @@ module ActiveAdmin
       end

       html = without_wrapper 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
+        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
-      html = template.content_tag(:li, html, class: "has_many_container #{assoc}") if @already_in_an_inputs_block
-      form_buffers.last << html
     end

igorbernstein · 2013-11-10T23:06:36Z

I'd prefer more complex markup to more complex behavior. Most of the complexity of this ticket had to do with formtastic changing behavior based on context. If it were a voting matter, I'd vote for minimizing context sensitive behavior rather than depending more on it.

Any feedback on the additional commits on
https://github.com/igorbernstein/active_admin/compare/gregbell:09da327...2659-rewrite-has-many-form-structure?

They are rooted at the tip of @shekibobo PR so should apply cleanly

shekibobo · 2013-11-10T23:42:37Z

Hey, guys, sorry, I've been doing a weekend hackathon all weekend, so my coding brain is kinda fried right now. @seanlinsley Yeah, if we delete the div.has_many wrapper, I'm good with the final wrapping being in li or div depending on context.

@igorbernstein as I said, my brain is kind of fried. Can you summarize what your branch does differently or optimizes? I believe you can also submit a pull request directly to my branch if you want to do it that way.

igorbernstein · 2013-11-11T00:28:54Z

My commits are based on this PR and add the following:

first 2 commits just change around the code structure to make it easier to see the actual changes
wrapping the fieldsets in an ol as was discussed in has_many breaks when embedded in an inputs block #2659 (comment)
keeping the hidden id field in the fieldset as discussed here Rewrite has_many form structure #2679 (comment)
making sure to use the same formtastic options for existing fieldsets as the newly created js ones

I'll create another fork from your repo and repackage them as a PR

igorbernstein · 2013-11-11T00:36:34Z

For some reason your active_admin fork does not appear as an option in "Choose a Base Repository" so I can't send you a PR. You can pull in my changes using rebase

git remote add igor https://github.com/igorbernstein/active_admin.git
git fetch igor
git checkout 2659-rewrite-has-many-form-structure
git rebase --onto 2659-rewrite-has-many-form-structure 2659-rewrite-has-many-form-structure igor/2659-rewrite-has-many-form-structure

(I think that should work)

seanlinsley · 2013-11-11T00:52:55Z

@igorbernstein while 41ab134 makes sense, I don't seem to understand your other changes.

wrapping the fieldsets in an ol as was discussed in has_many breaks when embedded in an inputs block #2659 (comment)

But you're putting it inside a div; that comment suggested a full fieldset > ol > li structure.

keeping the hidden id field in the fieldset as discussed here Rewrite has_many form structure #2679 (comment)

Doesn't the normal one still get built?

igorbernstein · 2013-11-11T01:11:03Z

The first 2 changes don't actually do anything other than shift around code to make the rest of the changes more apparent.

The structure introduced in bed8f24 "wrap the fieldset in a ol" is div.has_many > ol > li which gets us most of the way to fieldset > ol > li. To switch it fieldset > ol > li you would just change the content_tag :div to content_tag :fieldset. I still disagree that a .has_many is a formset, but don't really care that much will leave it to you & @seanlinsley.

9aca4ae - "generate hidden inputs manually to keep markup consistent"
The normal field doesn't get built because of this check: https://github.com/igorbernstein/active_admin/commit/9aca4ae2e01607509b09fe54334b18df54a42663#diff-7c98cd4d1856918892df462522c53d4dR76

if has_many_form.object.persisted? && has_many_form.options.fetch(:include_id, true) &&  !has_many_form.emitted_hidden_id?
  has_many_form.input :id, as: :hidden
end

edit: copy/pasted the check for hidden field

igorbernstein · 2013-11-11T01:14:00Z

side note: the logic for the hidden input check comes from the link I posted earlier in the discussion:
https://github.com/rails/rails/blob/master/actionview/lib/action_view/helpers/form_helper.rb#L1862

seanlinsley · 2013-11-11T03:19:36Z

👎 on adding an ol; I think my suggestion ealier #2679 (comment) is a better option.

Though it makes the form code more complex, 👍 on the hidden ID change because it makes the HTML clearer.

I said it earlier, but to have a compact list 👍 on 41ab134 as we should have already been doing that.

seanlinsley · 2013-11-11T03:22:02Z

@shekibobo if you can update the failing tests so the no longer depend on the CSS inputs class you removed, maybe it would be best to get this merged so @igorbernstein can create a PR off of master.

shekibobo · 2013-11-11T03:37:45Z

Should I remove the div.has_many and do the final li.has_many_container wrapping at the end as well?

seanlinsley · 2013-11-11T03:41:21Z

Oh, sure if you want. I was mostly just saying that as I assumed you were busy :]

There are now quite a few commits in this PR that are just updating earlier changes. Would you mind squashing some of the more obvious ones?

shekibobo · 2013-11-11T03:41:45Z

Yeah, no problem.

shekibobo · 2013-11-11T04:02:51Z

Squashed. Hoping these tests pass. If they don't I'm going to take a look and fix them in the morning.

seanlinsley · 2013-11-11T04:40:30Z

Yeah, it looks like some of the unit tests aren't quite accurate. Anyway, til tomorrow!

coveralls · 2013-11-11T15:23:43Z

Coverage decreased (-0.23%) when pulling 2f72a7b on shekibobo:2659-rewrite-has-many-form-structure into 329dda8 on gregbell:master.

seanlinsley · 2013-11-11T15:42:20Z

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

@@ -22,13 +22,13 @@ $ ->
  #       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)->


you missed the code example right above this one :)

[skip ci]

shekibobo · 2013-11-11T15:44:19Z

💚 💚 💚 💚

seanlinsley · 2013-11-11T15:51:45Z

🐙

…ructure Rewrite has_many form structure

shekibobo · 2013-11-11T15:52:39Z

Yay, I can move on with my life! 😃

seanlinsley reviewed Nov 8, 2013
View reviewed changes

igorbernstein mentioned this pull request Nov 9, 2013

has_many breaks when embedded in an inputs block #2659

Closed

This was referenced Nov 9, 2013

Prevent has_many options from rendering as input attrs #2661

Closed

Allow for displaying has_many in panels. #2660

Closed

igorbernstein mentioned this pull request Nov 9, 2013

nested has many #2675

Closed

igorbernstein reviewed Nov 9, 2013
View reviewed changes

igorbernstein mentioned this pull request Nov 11, 2013

View layer overhaul #2689

Closed

Cleanup options that shouldn't render as html attrs

e12b62a

Fix broken has_many form html and javascript

2f72a7b

seanlinsley reviewed Nov 11, 2013
View reviewed changes

Update inline-docs on has-many js

cb223f6

[skip ci]

seanlinsley added a commit that referenced this pull request Nov 11, 2013

Merge pull request #2679 from shekibobo/2659-rewrite-has-many-form-st…

edd1829

…ructure Rewrite has_many form structure

seanlinsley merged commit edd1829 into activeadmin:master Nov 11, 2013

This was referenced Nov 11, 2013

Add jquery callbacks for has_many field added and removed #1767

Closed

Refactor the has_many form #2645

Merged

igorbernstein mentioned this pull request Nov 12, 2013

sortable has_many #2656

Merged

Rewrite has_many form structure #2679

Rewrite has_many form structure #2679

Conversation

shekibobo commented Nov 8, 2013

seanlinsley Nov 8, 2013

Choose a reason for hiding this comment

shekibobo Nov 8, 2013

Choose a reason for hiding this comment

shekibobo commented Nov 8, 2013

seanlinsley commented Nov 8, 2013

shekibobo commented Nov 8, 2013

seanlinsley commented Nov 8, 2013

shekibobo commented Nov 8, 2013

shekibobo commented Nov 8, 2013

seanlinsley commented Nov 8, 2013

seanlinsley commented Nov 8, 2013

seanlinsley commented Nov 8, 2013

seanlinsley commented Nov 8, 2013

igorbernstein commented Nov 9, 2013

shekibobo commented Nov 9, 2013

shekibobo commented Nov 9, 2013

shekibobo commented Nov 9, 2013

shekibobo commented Nov 9, 2013

igorbernstein Nov 9, 2013

Choose a reason for hiding this comment

seanlinsley commented Nov 9, 2013

seanlinsley commented Nov 9, 2013

igorbernstein commented Nov 9, 2013

You're using Rails 4.0.1? I'm using 3.2.15, though I'd be surprised if that was causing this.

igorbernstein commented Nov 9, 2013

Maybe related to @igorbernstein's comment: #2679 (comment)

seanlinsley commented Nov 9, 2013

shekibobo commented Nov 9, 2013

seanlinsley commented Nov 9, 2013

igorbernstein commented Nov 9, 2013

seanlinsley commented Nov 10, 2013

seanlinsley commented Nov 10, 2013

igorbernstein commented Nov 10, 2013

shekibobo commented Nov 10, 2013

igorbernstein commented Nov 11, 2013

igorbernstein commented Nov 11, 2013

seanlinsley commented Nov 11, 2013

igorbernstein commented Nov 11, 2013

igorbernstein commented Nov 11, 2013

seanlinsley commented Nov 11, 2013

seanlinsley commented Nov 11, 2013

shekibobo commented Nov 11, 2013

seanlinsley commented Nov 11, 2013

shekibobo commented Nov 11, 2013

shekibobo commented Nov 11, 2013

seanlinsley commented Nov 11, 2013

coveralls commented Nov 11, 2013

seanlinsley Nov 11, 2013

Choose a reason for hiding this comment

shekibobo commented Nov 11, 2013

seanlinsley commented Nov 11, 2013

shekibobo commented Nov 11, 2013