form.has_many cannot hide 'remove button' on new records

[omitter]

I pre-build the attributes of associated objects for a new object and don't won't the customers to change their count. That's why I need to remove the 'remove' button (and the 'add new' button, but that works fine). My code looks like this: f.has_many :sub_model, allow_destroy: false, new_record: false. Unfortunately this still generates the 'remove' buttons. I know, most of the time that is wanted, but when I explicitly pass false to the allow_destroy, I except them to disappear.

I fixed this issue by patching the source in activeadmin-1.3.0/lib/active_admin/form_builder.rb:
in has_many_actions:

     if has_many_form.object.new_record?

to

     if has_many_form.object.new_record? && builder_options[:allow_destroy] != false

If there are no regressions, it would be great if you could add that upstream. Thanks for considering :)

[varyonic]

I think this is a reasonable customization for users to make if they want and expect this behavior. Feel free to submit as a pull and we'll see if it is OK with Travis. I would want to see more supportive feedback from others before merging such a change.

[omitter]

Great, I created a pull request, let me know if there's still anything that needs to be done.
PR: #5452

[chumakoff]

This change is not good. :allow_destroy option is intended for destroying persisted records. Your problem is related to new records.

What if someone doesn't want to destroy persisted records, but wants to be able to add and delete (by 'remove' button) new records? Setting :allow_destroy to false in order to prevent persisted records destruction will hide the 'remove' button of the new record.

[omitter]

:allow_destroy option is intended for destroying persisted records. Your problem is related to new records.

That's true. Unfortunately, it's impossible right now to configure the destroy button for new records, that's why I am suggesting this. Also, isn't it strange that the new_record and allow_destroy option behave differently on new records?

What if someone doesn't want to destroy persisted records, but wants to be able to add and delete (by 'remove' button) new records? Setting :allow_destroy to false in order to prevent persisted records destruction will hide the 'remove' button of the new record.

I have a few ideas:

1. Don't make the allow_destroy option distinguish between new records or persistent records at all. It it's true/false the button is shown/not shown. This would be the simplest straight-forward solution. For your use-case, you'd need allow_destroy: -> (c) { c.new_record? }. Probably this should be the default if nothing is set.
2. Instead of checking only for builder_options[:allow_destroy] == false, check for builder_options[:new_record] == false too. If neither new records nor deleting records are allowed, this button shouldn't appear.

I favour option 1 because it's explicit—you get what you expect—but I guess that's too much of a compatibility break. Is option 2 fine for you?

[chumakoff]

No, sorry, I don't like the option 2. As I have already sad, the :allow_destroy option has it's own purpose and shouldn't be mixed with "Remove new record" logic. This is my opinion.

You should also keep in mind that not only boolean can be passed as the :allow_destroy option, but also String, Symbol, Proc.

By the way, using Rails's :fields_for method instead of Formtastic's :inputs will probably work for you. Try it.

f.fields_for :sub_models do ...

[omitter]

Well then, maybe consider option 1 for active_admin 2.0. I think it's much more straight-forward and gives the user more control.

Of course this could also be implemented with another option (:no_destroy_button), but having :allow_destroy and :no_destroy_button is confusing on it's own. (Thus I favour option 1.)

Thanks for your suggestion with f.fields_for. f.semantic_fields_for does the job for me.

[TomasBarry]

Is there any update on this issue or has it gone stale? I see the one PR mentioned above (#5452) does that need a re-visit?

[JSFernandes]

I agree that allow_destroy should be kept separate from the behaviour mentioned in this issue. With that said, having the option to hide that "remove button" would be helpful. Would it be alright if I made a new PR adding a new option, perhaps named remove_new_records?

[javierjulio]

I would accept a PR for that. There may be something for it already.