How could I could refactor a Pundit Policy to make it more DRY?

Question

I'm new to using Pundit with Rails and have a new policy for my Artist model that is working as I expect it to, but I'm not clear on a good way to refactor it to make it more DRY. Specifically, it seems that I'm calling authorize @artist way too many times in artists_controller.rb and that there's a lot of code duplication in my artist_policy.rb.

For context, an Artist has a name, such as "Claude Monet" and that's it.

Here's my artist_policy.rb: https://gist.github.com/leemcalilly/799d5f9136b92fcf92c6074e6a28bfdb

And, my application_policy.rb: https://gist.github.com/leemcalilly/09d37a42c6f2500f98be3f1518c945e9

And here's my artists_controller.rb: https://gist.github.com/leemcalilly/c0dd8f33416b002f3b4c9a7baf0a3a75

And, models/artist.rb: https://gist.github.com/leemcalilly/c190322af41f3e91739b53391d8b7834

Is the way I currently have it working ok because I'm clearing expressing each policy (in the same way that some duplicate code across integration tests is ok), or should I refactor this? If so, is there a standard way that people structure their Pundit policies that I'm missing?

Answer 1

It seems that I'm calling authorize @artist way too many times in artists_controller.rb

Honestly, I think what you have there is fine.

There are a few ways that you could try to be clever about this and "automatically call authorize" for each controller action, but (warning: opinion-based answer) from past experience I've found that such attempts to make it more DRY adds significant confusion. Especially when you end up writing some controller action(s) that don't need authorising, or need authorising in an unusual way.

There's a lot of code duplication in my artist_policy.rb

One step at time... Here's the original:

class ArtistPolicy < ApplicationPolicy
  attr_reader :user, :artist

  def initialize(user, artist)
    @user   = user
    @artist = artist
  end

  def create?
    if user.admin? || user.moderator? || user.contributor?
      true
    elsif user.banned?
      false
    end
  end

  def update?
    if user.admin? || user.moderator? || user.contributor? && user.id == @artist.user_id
      true
    elsif user.banned?
      false
    end
  end

  def destroy?
    if user.admin? || user.moderator? || user.contributor? && user.id == @artist.user_id
      true
    elsif user.banned?
      false
    end
  end
end

It's unnecessary to define your own initialize method like that, so long as you're happy to reference the more generic variable name: record, instead of artist (which should be defined in the ApplicationPolicy):

class ArtistPolicy < ApplicationPolicy
  def create?
    if user.admin? || user.moderator? || user.contributor?
      true
    elsif user.banned?
      false
    end
  end

  def update?
    if user.admin? || user.moderator? || user.contributor? && user.id == record.user_id
      true
    elsif user.banned?
      false
    end
  end

  def destroy?
    if user.admin? || user.moderator? || user.contributor? && user.id == record.user_id
      true
    elsif user.banned?
      false
    end
  end
end

Next, in situations like this, it's fine to reference one policy rule from another - so long as they apply equally to user types:

class ArtistPolicy < ApplicationPolicy
  def create?
    if user.admin? || user.moderator? || user.contributor?
      true
    elsif user.banned?
      false
    end
  end

  def update?
    if user.admin? || user.moderator? || user.contributor? && user.id == record.user_id
      true
    elsif user.banned?
      false
    end
  end

  def destroy?
    update?
  end
end

Next, note that the record.user_id is the logged in user, for the create action! So you can simplify this even further:

class ArtistPolicy < ApplicationPolicy
  def create?
    if user.admin? || user.moderator? || user.contributor? && user.id == record.user_id
      true
    elsif user.banned?
      false
    end
  end

  def update?
    create?
  end

  def destroy?
    create?
  end
end

And lastly, the logic in that method is actually little wrong. (You could have picked this up with tests...) If a user is an admin and they are banned, then you still presumably want this to return false, not true. With this in mind, we can fix + simplify the code again to:

class ArtistPolicy < ApplicationPolicy
  def create?
    return false if user.banned?
    user.admin? || user.moderator? || user.contributor? && user.id == record.user_id
  end

  def update?
    create?
  end

  def destroy?
    create?
  end
end