search

jatwell93 / rubyproject

1 stars 1 forks source link

DRY'ing workout & recipe controller #37

Closed Mirv closed 7 years ago

Mirv commented 7 years ago

Two issues

Clarity of the method defintion name - what you are doing this this function is a redirect, so the name should not imply it holds the admin_user ... the name should be something like only_admin ...but I'm not able to come up with a good example name - as I'm not sure why you have this or what it does?

Can you explain what the redirect is supposed to do? There's a ton of errors like 15 in the test.log since I turned it back on regarding this - so whatever you're trying to do it's also generating load on the server.

Repeating - you have this defined twice - I think we could move this definition to application_controler.rb

    def admin_user
      redirect_to recipes_path unless current_user.admin?
    end
jatwell93 commented 7 years ago

Thats another ting left over from the rails online course i was doing when i originally started this app. Makes sense to me now to move it to the application controller.

At first it was supposed to control what normal users could edit etc. but i think you mentioned a while ago that there are better ways to handle this?

Mirv commented 7 years ago

pundit gem is a good way to handle authorization too - it allows for a few levels or profiles/roles type situations. I'd say moving it as is to the application_controller.rb would be a mistake. If we moved it there, we'd just move the definition of it...then call it from the right spots...it might not belong defined in application_controller to be honest so I'd say leave for now...

jatwell93 commented 7 years ago

Just spent some time reading the documentation and some articles about pundit, looks really interesting. Seems less complex then cancancan to implement with a lot of customisation we could add later.