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

Projects
None yet
@VorontsovIE
Contributor

VorontsovIE commented May 20, 2013

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

This comment has been minimized.

Show comment
Hide comment
@travisp

travisp 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.

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

This comment has been minimized.

Show comment
Hide comment
@VorontsovIE

VorontsovIE May 20, 2013

Contributor

@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.

Contributor

VorontsovIE commented May 20, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@VorontsovIE

VorontsovIE May 20, 2013

Contributor

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

Contributor

VorontsovIE commented May 20, 2013

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

@travisp

This comment has been minimized.

Show comment
Hide comment
@travisp

travisp 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"

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

This comment has been minimized.

Show comment
Hide comment
@VorontsovIE

VorontsovIE May 21, 2013

Contributor

@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

Contributor

VorontsovIE commented May 21, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@rafaelgoulart

rafaelgoulart Jun 20, 2013

Contributor

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

Contributor

rafaelgoulart commented Jun 20, 2013

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

@travisp

This comment has been minimized.

Show comment
Hide comment
@travisp

travisp 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".

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

This comment has been minimized.

Show comment
Hide comment
@taavo

taavo Jul 3, 2013

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@dim

dim Jul 11, 2013

Contributor

Any news? Would love to see this merged.

Contributor

dim commented Jul 11, 2013

Any news? Would love to see this merged.

@rolftimmermans

This comment has been minimized.

Show comment
Hide comment
@rolftimmermans

rolftimmermans Sep 12, 2013

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

rolftimmermans commented Sep 12, 2013

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

@strangeement

This comment has been minimized.

Show comment
Hide comment
@strangeement

strangeement Sep 28, 2013

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.

strangeement commented Sep 28, 2013

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

This comment has been minimized.

Show comment
Hide comment
@aaronchi

aaronchi Oct 14, 2013

+1

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

aaronchi commented Oct 14, 2013

+1

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

@onemanstartup onemanstartup referenced this pull request Nov 8, 2013

Merged

Update README.md #335

@evserykh

This comment has been minimized.

Show comment
Hide comment
@evserykh

evserykh commented Dec 1, 2013

👍

@abernardes

This comment has been minimized.

Show comment
Hide comment
@abernardes

abernardes Jan 24, 2014

🆙

Any chance of merge for this PR?

abernardes commented Jan 24, 2014

🆙

Any chance of merge for this PR?

@joelmoss

This comment has been minimized.

Show comment
Hide comment
@joelmoss

joelmoss Jan 24, 2014

Contributor

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

Contributor

joelmoss commented Jan 24, 2014

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

@killthekitten

This comment has been minimized.

Show comment
Hide comment
@killthekitten

killthekitten Feb 18, 2014

Ping @prijutme4ty

killthekitten commented Feb 18, 2014

Ping @prijutme4ty

@VorontsovIE

This comment has been minimized.

Show comment
Hide comment
@VorontsovIE

VorontsovIE Apr 4, 2014

Contributor

@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.

Contributor

VorontsovIE commented Apr 4, 2014

@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

Merge pull request #286 from prijutme4ty/strong_params
strong parameter support refined to allow standard Rails 4 notation

@joelmoss joelmoss merged commit 1e18617 into activeadmin:master Apr 4, 2014

@joelmoss

This comment has been minimized.

Show comment
Hide comment
@joelmoss

joelmoss Apr 4, 2014

Contributor

LGTM!

Contributor

joelmoss commented Apr 4, 2014

LGTM!

@VorontsovIE VorontsovIE deleted the VorontsovIE:strong_params branch Apr 4, 2014

@abernardes

This comment has been minimized.

Show comment
Hide comment
@abernardes

abernardes commented Apr 4, 2014

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment