- 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.
- 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.
- 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 JOIN
s), rather than loop through students individually, and then getting their memberships.
- 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.