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

strong parameter support refined to allow standard Rails 4 notation #286

Merged
merged 1 commit into from
Apr 4, 2014

Conversation

VorontsovIE
Copy link
Contributor

This commit allows one to use standard rails 4 params.require().permit() notation.
It's done in such a way that this doesn't clash with permitted_params if it's already defined. But if it doesn't, then default permitted_params falls back to a permitting method with resource-specific name.
Method user should define is the same as scaffold generates and thus the same as other gems such as cancan will try to handle.

@travisp
Copy link

travisp commented May 20, 2013

I think it's a good idea, but the tests should probably test the "required" support. Some quick testing seems to show that it works fine for the "update" controller action.. but it fails for me on the "new" controller action: a 'Required parameter missing' error occurs on the new action, which isn't normal rails behavior. The new action builds a resource and calls resource_params.

@VorontsovIE
Copy link
Contributor Author

@travisp Thank you for a response! Actually I didn't tested it with strong_parameters, and did only testing with stubs. I'll try to fix it.

@VorontsovIE
Copy link
Contributor Author

@travisp Can you please show code which caused the failure? In my tests everything works.

@travisp
Copy link

travisp commented May 21, 2013

@prijutme4ty Your code testing the new action should be "get :new, {}" because the new action typically does not have any parameters passed to it.

This test fails:

  def test_resource_params_from_new
    get :new, {}
    assert_response :success
    assert assigns(:widget)
  end

Results in: "Expected response to be a <:success>, but was <400>" and response.body after the get request contains "Required parameter missing: hotel"

@VorontsovIE
Copy link
Contributor Author

@travisp Understood the problem. I believe, it's due to inherited_resources using strong_parameter sanitization on new action, while it's not a common practice. I'll try to distinguish new and create action when call #build_resource

@rafaelgoulart
Copy link
Contributor

I'm having the same problem with the new action.

@travisp
Copy link

travisp commented Jun 20, 2013

@rafaelgoulart The code still hasn't been updated to deal with it. I might take a shot in a few weeks if nobody else gets to it: it would be very nice if inherited_resources could work with standard strong_parameters syntax and would also allow the use of "requires".

@taavo
Copy link
Contributor

taavo commented Jul 3, 2013

I've come to the conclusion that the API I used in #260 was stupid, and this one is much better. I'll help out if my schedule opens up.

@dim
Copy link
Contributor

dim commented Jul 11, 2013

Any news? Would love to see this merged.

@rolftimmermans
Copy link

This proposed API makes so much more sense. The current strong parameters solution is pretty awkward to use correctly.

@strangeement
Copy link

It would be pretty helpful to add a note to the README meanwhile.

As I understand it (after some testing and debugging inside IR), permitted_params must not use params.require(:model).permit(...), but rather params.permit(...). When require is set, IR receives empty parameters from BaseHelpers.build_resource_params so the model is empty.

@aaronchi
Copy link

+1

To address the new issue, you can skip building params if request.get?

@onemanstartup onemanstartup mentioned this pull request Nov 8, 2013
@evserykh
Copy link

evserykh commented Dec 1, 2013

👍

@dreoliv
Copy link

dreoliv commented Jan 24, 2014

🆙

Any chance of merge for this PR?

@joelmoss
Copy link
Contributor

If @prijutme4ty can make the PR merge cleanly, I'd be happy to merge it.

@killthekitten
Copy link

Ping @prijutme4ty

@VorontsovIE
Copy link
Contributor Author

@travisp @joelmoss Tried to resolve issue with new action by rescuing an exception. More detailed explanation in a comment before #permitted_params method. Is it acceptable?
Everybody, sorry for such a long delay.

joelmoss added a commit that referenced this pull request Apr 4, 2014
strong parameter support refined to allow standard Rails 4 notation
@joelmoss joelmoss merged commit 1e18617 into activeadmin:master Apr 4, 2014
@joelmoss
Copy link
Contributor

joelmoss commented Apr 4, 2014

LGTM!

@VorontsovIE VorontsovIE deleted the strong_params branch April 4, 2014 08:55
@dreoliv
Copy link

dreoliv commented Apr 4, 2014

❤️

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.