Refactor permitted attributes method#502
Refactor permitted attributes method#502Linuus merged 3 commits intovarvet:masterfrom pcriv:refactor-permitted-attributes-method
Conversation
|
@Linuus could you also check this PR when you are back? |
|
@pablocrivella Could you just rebase this on master so we make Travis happy? |
|
@Linuus Done |
|
|
||
| If you have defined an action-specific method on your policy for the current action, the `permitted_attributes` helper will call it instead of calling `permitted_attributes` on your controller. | ||
|
|
||
| If you have a different way of fetching the parameters other than the standard way: |
There was a problem hiding this comment.
Redundancy: "different", "other", "standard".
There was a problem hiding this comment.
Any suggestions on how to phrase this?
There was a problem hiding this comment.
Yes, but there's perhaps a more fundamental problem with this PR. See below. :)
There was a problem hiding this comment.
"If you need to fetch parameters based on namespaces different from the suggested one, override the below method and return an instance of ActionController::Parameters."
| end | ||
| ``` | ||
|
|
||
| You can override it this method like this: |
| "permitted_attributes" | ||
| end | ||
| params.require(param_key).permit(*policy.public_send(method_name)) | ||
| pundit_params_for(record).permit(*policy.public_send(method_name)) |
There was a problem hiding this comment.
@pablocrivella we still need to pass the Rails controller specific params implementation in, right? I don't think Hash has a permit instance method.
There was a problem hiding this comment.
True, this would only work with an instance of ActionController::Parameters for the permit method to work
| ```ruby | ||
| # If you don't want to use require | ||
| def pundit_params_for(record) | ||
| params.fetch(PolicyFinder.new(record).param_key, {}) |
There was a problem hiding this comment.
Does fetch still return the Rails controller specific params implementation? @pablocrivella
There was a problem hiding this comment.
Yes it does:
ams = ActionController::Parameters.new()
=> <ActionController::Parameters {} permitted: false>
ams.fetch(:something, {}).class
=> ActionController::Parameters
There was a problem hiding this comment.
Yeah, I didn't think this would work actually. According to the docs: http://edgeapi.rubyonrails.org/classes/ActionController/Parameters.html#method-i-fetch it should return the value given to fetch. However, when looking at the code you can clearly see that the value is converted to an instance of ActionController::Parameters (if possible). So I guess it should work :)
|
@ce07c3 Updated |
| params.fetch(PolicyFinder.new(record).param_key, {}) | ||
| end | ||
|
|
||
| # If you are using something like the jsonapi spec |
There was a problem hiding this comment.
"JSON API" seems to be the official name @pablocrivella (I'm picky 👼)
There was a problem hiding this comment.
@ce07c3 I missed this one! I would have changed it since i'm also picky like that 😅
|
This looks like a pretty good solution to me. I'll just have to think about it a bit. Could you squash the two readme updates please? |
| pundit_params_for(record).permit(*policy.public_send(method_name)) | ||
| end | ||
|
|
||
| def pundit_params_for(record) |
There was a problem hiding this comment.
We need to add some documentation for this method.
|
Thanks! |
3 participants
Closes #469
Closes #396