Checking user authorization

Question

  def user_not_authorized(exception)
    message = t('flash.access_denied')
    if exception.policy.class.to_s.underscore == 'group_policy' && current_user.students.size != 0
        current_user.students.each do |student|
          student.memberships.each do |membership|
            if membership.group_id == exception.record.id
              message = t('flash.pending_acceptance')
              break # and I would like to break from the outer loop too
            end
          end
        end
    end
    flash[:alert] = message
    redirect_to request.referrer || root_path
  end

This looks ugly to me. Is there a way to somehow compress those two each loops, where one should break both of them conditionally?

Answer 1

1. There's no reason (as far as I can tell) to do string comparison to check a class. #is_a? would presumably work fine too. And it'd be stricter, which is desirable.
2. There's no reason to check students.size. If there are no students, the loop just won't do anything, so why bother checking first? Conversely, if there are any students you're actually doing 2 database queries: One to get the count/size of students, and another to fetch the actual records.
3. Presuming non-ancient Rails, you can simply say current_user.students.memberships to get all the memberships in 1 query (admittedly with a couple of JOINs), rather than loop through students individually, and then getting their memberships.
4. Continuing from #3, you can (presumably) collapse the entire things into 1 database query.

I'd probably start with something like

def user_not_authorized(exception)
  redirect_path = request.referrer || root_path

  if exception.policy.is_a?(GroupPolicy)
    if current_user.students.memberships.where("group_id = ?", exception.record.id).any?
      redirect_to redirect_path, alert: t('flash.pending_acceptance') and return
    end
  end

  redirect_to redirect_path, alert: t('flash.access_denied')
end

Note that it doesn't actually load any records at all; it just checks if they're there or not.

In practice, I'd consider breaking it into separate methods where possible. It's not that great to have a naked query referencing a specific column name in the middle of your method.

Edit: Updated the code to return if the first redirect occurs, since you'd otherwise do two redirects in one action, which is not allowed. Thanks to tokland for catching that. Now the and return fix is about as quick and dirty as is gets, but check the comments for better ideas.

Answer 2

I moved the test into its own method to separate the logic. This still shows a Rubocop error of Assignment Branch Condition size for pending? is too high. [18.25/15] which you could fix by moving the nested iteration into its own method, but that struck me as more confusing than helpful. Note that creating the pending? method allows us to use both return and next to break out of the loop at different levels. Also note the use of a guard clause.

def user_not_authorized(exception)
  flash[:alert] = if pending?(exception)
                    t('flash.pending_acceptance')
                  else
                    t('flash.access_denied')
                  end
  redirect_to request.referrer || root_path
end

def pending?(exception)
  if exception.policy.class.to_s.underscore == 'group_policy' &&
     current_user.students.size != 0
    current_user.students.each do |student|
      student.memberships.each do |membership|
        next unless membership.group_id == exception.record.id
        return true
      end
    end
  end
  false
end