Migrate cancan to pundit [open review]#520
Conversation
… Pundit in the Application Controller.
…g the same method. Let's try a smooth migration.
…mit show by default.
…ged spec for protected attributes, when we try to update a protected attribute now we ignore the protected one and update the permited ones.
…he admin controller.
…rojectPolicy using Pundit
|
@devton @adrianob @josemarluedke & @joaomilho please take a look. This is a first draft I've made of a new privilege system for Catarse. This PR would start a smooth migration, so it would be the first of several and it would imply Pundit and CanCan coexisting for a while. I've chosen the ProjectsController as a starting point for it's a central piece with some attribute restrictions and complex rules. |
|
Changes Unknown when pulling 45cde19 on diogob:migrate_cancan_to_pundit into * on catarse:master*. |
There was a problem hiding this comment.
The only thing I disagree for now is this line, we should specify the project attributes even for admins. With that change, I guest it should be as the example on pundit documentation, return only an array with the params and let the controller deal with the rest.
There was a problem hiding this comment.
Yeah, I thought the same initially but it seemed a bit clumsy to specify all fields considering that I'm an admin and I have the permit!
But I guess I can return all fields dynamically when user is an admin.
There was a problem hiding this comment.
I've done this but our code is still a bit different from the Pundit example, as the permitted attributes is done by inherited resources as seen here:
https://github.com/josevalim/inherited_resources/blob/master/lib/inherited_resources/base_helpers.rb#L321
|
Is there anyway to make the pundit and inherited_resources work together instead of saying |
|
Also we should create specs for the policies. |
There was a problem hiding this comment.
dont forget to remove before merging
There was a problem hiding this comment.
Thanks, I'll remove this.
|
Overall I think Pundit is the way to go. |
|
@josemarluedke We'll aim for a better integration with inherited resources after migrating everything from CanCan, but that's the idea ;) |
|
@josemarluedke & @joaomilho regarding the policies specs I'm in doubt. Because the controller specs give us a good coverage (except in cases where we use policies in views). Nevertheless I feel more confortable creating specs with Pundit than I was with that loathsome ability_spec. So do you think it's worth to spec every policy? |
|
I'm not sure unit testing policies is a good thing. I have to think more about it. Do you have a stance on it? Anyway, it's not hard http://thunderboltlabs.com/blog/2013/03/27/testing-pundit-policies-with-rspec/ (well, testing belongs/has is also easy but controversial) |
|
I'm using "unit testing" as a verb. :/ |
|
Well, I have been thinking about this since @diogob said about not having specs for it. I'm not sure anymore about testing each policy separated. But if we choose to not test it, we need make sure that controllers specs cover all cases of permission. Also the policy on views is other open question at this point. |
…eturn just a plain hash with permitted attributes
…nd create priviledges can be the same
|
@josemarluedke @joaomilho I've decided to create the policy specs as we use the project policy in some views. I still need to add some commits to this PR migrating the view policy checks. |
|
cool, are you helping the brazilian guy? |
|
@joaomilho brazilian guy? |
|
@diogob maybe using this gem (https://github.com/yakko/before_actions) from a friend of mine could help you with DRY on the controller (and get rid of Inherited Resources too). +1 for specs for policies. |
|
@luizfonseca why get rid of inherited_resources ? We could also DRY the controller using it in a better way ;) |
… spec on project policy for it.
|
Nice! |
Migrate cancan to pundit [open review]
6 participants
This is an example of how we could implement our privileges system using Pundit instead of CanCan. I've replaced CanCan only in ProjectsController to make this PR smaller. Doing so we can make a smooth migration analysing and discussing smaller changes and removing cancan only after everything is working properly with Pundit.
It's important to observe that later refactorings will be necessary to DRY the code after the migration is complete.