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.

• pmichna: Seems nice. However, there is an error undefined method memberships, so the current_user.students.memberships solution doesn't work.
• Flambino: @pmichna In that case, add a has_many :student_memberships, through: :students, source: :memberships to the user model, and you can simply say current_user.student_memberships
• tokland: Two minor notes: 1) I'd move all this DB logic to models. Of course, I understand you are just trying to show what's the idea. 2) As I'm a fanatic of structured conditional branches, IMO the last redirect should go in an else.
• Flambino: @tokland Definitely agree with regard to moving stuff to models. But which one(s) should have responsibility for tying all this together? I didn't dare guess, hence the vague note about "breaking it into separate methods". As for #2, it's a good point. I'll leave it as-is, because the alternatives are to duplicate the last redirect line, or combine the conditions into one horribly long line.
• Flambino: @tokland Oops, you're correct of course! You can do a redirect_to ... and return, which is what I was thinking of, but didn't actually write... I'm so used to conditional redirects either being simple if record.save-stuff, or that they're in filters. I'll fix that, though it should of course be an else branch, as you said.

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