Add :allow_destroy option to has_many forms

[shekibobo]

This option will add a boolean field "Remove" to the
end of each has_many fieldset that is an existing
record. This provides a workaround for issue #1990,
and allows for cleaner form coding.

Instead of the following, which would not work because
of the issue mentioned above:

form do |f|
  f.inputs do
    f.has_many :children do |child_f|
      child_f.input :name
      child_f.input :_destroy, :as => :boolean, :label => "Remove", :wrapper_html => {:class => 'has_many_remove'} unless child_f.object.new_record?
    end
  end
end

we could simply write:

form do |f|
  f.inputs do
    f.has_many :children, :allow_destroy => true do |child_f|
      child_f.input :name
    end
  end
end

[macfanatic]

You mind adding a section to this doc page on the has_many form input and your new option?

https://github.com/gregbell/active_admin/blob/master/docs/5-forms.md

[shekibobo]

Absolutely. I've got a draft written locally already, but I'll finish it up later today.

[macfanatic]

Great, I'll merge this in once you have that up here.

[seanlinsley]

On the translations, what's different between has_many_delete and has_many_remove? Where are each used?

[seanlinsley]

• this is using 1.9 hash syntax, but we're still supporting 1.8 for now
• can you put this on multiple lines? Something like:

has_many_form.input :_destroy, :as => :boolean, :wrapper_html => {:class => 'has_many_remove'},
                                                :label => I18n.t('active_admin.has_many_remove')

[shekibobo]

has_many_delete is used for the delete button text for new records on a has_many form.
has_many_remove is used for the remove checkbox for an existing record.

In most of the examples of people writing this form element, I've seen them use the term "Remove" rather than "Delete", so I went with what I've seen most often, and at the very least giving users the option to use different terminology for either one.

[seanlinsley]

You went from one extreme to the other. Could you please emulate this version?

has_many_form.input :_destroy, :as => :boolean, :wrapper_html => {:class => 'has_many_remove'},
                                                :label => I18n.t('active_admin.has_many_remove')

[seanlinsley]

Also, please squash your commits.

[shekibobo]

Will this be good or do you want it all squashed into one?

[seanlinsley]

Hmm... in your PR description you say:

Instead of the following, which would not work because of the issue mentioned above

What did you mean by that? If you mean the nil problems, that's been resolved as of #1996.

The example you originally gave should work just fine now.

[shekibobo]

As of master, this spec still fails. I just tested the above scenario in my app using the lastest master, and I'm still seeing the issue. That being said, it's looking to me like #1990 may be related, but independent of #1996. Perhaps whatever was done to fix the form buffer on inputs needs to be applied to the has_many form builder option as well?

[seanlinsley]

That may be the case... I'm going to take a look at that in the morning.

One other thing; where did you get the translations that are in this PR? Did you manually get them from a web service?

UPDATE: I opened #2108 to deal with this problem.

[shekibobo]

I ran them through Google Translate. Most of them were either translated to the same as "Delete", while some matched similar to 'Clear' in clear_filters. What's the usual source you use?

[seanlinsley]

Can you wrap this last line so it's consistent with the rest of the file?

[shekibobo]

Updated the commit with wrapped lines. Anything else?

[seanlinsley]

Nope, looks good. Thanks for taking the time to do this! 💙

[ball-hayden]

Would it be sensible to use a delete button with a hidden field, and remove the item with javascript rather than a checkbox?

[seanlinsley]

@ball-hayden, assuming you mean doing an AJAX request, that wouldn't fit into the Rails form paradigm.

On a Rails form, no changes are final until everything in the form passes validations. If you use AJAX to delete children, what happens if the user realized that before they pushed submit, they had deleted the wrong child? With AJAX, too bad, it's gone. But with a Rails form, just reload the page.

[ball-hayden]

No - I didn't mean AJAX. I simply meant javascript that hides the element and set a hidden "_removed" field to true (much in the way that ryanb's nested_form gem does).

[shekibobo]

ActiveAdmin already does this for new records with the Delete button. This is a simple way to add the remove checkbox to relationships that have the :allow_destroy => true option set on them. Since this is commonly done with a checkbox, it seemed like the best way to go for the defaults. If you wanted to customize how you delete existing collection items through the form, you can simply skip the :allow_destroy option in the has_many form and add your own implementation.

[ball-hayden]

Of course. I just wondered if, for consistency, a delete button may be a better option.

[shekibobo]

I think the issue with hiding a persisted record after flagging it for removal is the inability to undo that action before submitting the form. Perhaps if you wanted to add a style that highlights items checked for removal with, say, a red background or something, that might make sense, but I think it's folly to hide them without any chance to undo it.

[ball-hayden]

That's sensible. I'll do that. Thanks.