Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only show form input fields in permit_params #5722

Conversation

marclerodrigues
Copy link

@marclerodrigues marclerodrigues commented Mar 30, 2019

Closes #5390.
Closes #3308.

Adds an option to the resource class named build_form_with_permitted_params_only in order to avoid breaking existing applications that depends on the current behavior. Once this is set to true the rendered form will only contain the fields specified in the permit_params method.

@marclerodrigues marclerodrigues force-pushed the 5390-only-show-input-fields-in-permit-params branch 2 times, most recently from 0328a3b to 6228cff Compare March 31, 2019 16:25
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this idea.

And... I'm not sure we need this setting. If people are not configuring permitted params, the currently rendered form inputs won't work anyways, right?

@activeadmin/maintainers Thoughts?

lib/active_admin/resource_dsl.rb Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez force-pushed the 5390-only-show-input-fields-in-permit-params branch from 6228cff to e5f16cb Compare July 26, 2019 10:33
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now passing, but I still wonder the same thing. Do we need a setting here? Isn't this the behaviour everyone expects?

permit_params do
permitted = []
permitted << :title
permitted << :body
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should improve these docs to showcase when this version can be useful over a static list.

@deivid-rodriguez
Copy link
Member

In other words, how could this break existing applications? It will definitely remove existing form inputs from default forms, but those inputs should be ignored since they're not permitted at the controller level... 🤔.

@javierjulio
Copy link
Member

I can see why we'd want to drive the form from params but params can be complex. What happens here with nested params? Does something go wrong or it works or is just ignored?


if active_admin_config.build_form_with_permitted_params_only
f.inputs do
controller.form_params.each do |param|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if permit params are defined with nested attributes (accepts_nested_attributes_for) ?

@deivid-rodriguez
Copy link
Member

Both good questions, thanks! I think this PR needs more work, so I'll try to have a closer look and address your concerns.

@ngouy
Copy link
Contributor

ngouy commented Apr 1, 2021

👀

@javierjulio
Copy link
Member

As mentioned in the issue, I don't think this is worth doing even for the simple case of just one level of params. This can get quite complex. I haven't seen any movement on this other than "that would be nice" and I think it's just because it's easier to declare the form and customize as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only show form input fields in permit_params permit_params should guide forms?
5 participants