Proper Coding Question

Good Morning Everyone…

I have a coding question just so I am clear on the proper way to do this…

To start, I have an app that I use Pundit in to control access to data, so I have a lot of calls that are like policy_scope(User)

What is the proper way to populate a select on a form with this data?

Initially I was just using policy_scope(User).collect

However I don’t think it is proper to use this in a view, is it?

If not, what is the proper way to pull this data and provide it to the form?

Recently I have moved some of these to helpers and created a method like “users_for_select”

John

There is an collection_select form builder that will do the collect for you. So:

<%= f.collection_select :user, policy_scope(User), :id, :name %>

If you are not using a form builder then options_from_collection_for_select is useful for the same purpose.

If those helpers cannot be used directly for some reason I create a custom helper.

For example if I wanted to include the user’s security role as a data attribute for some sort of UJS behavior I might have:

def user_options selected
policy_scope(User).collect do |user|

content_tag ‘option’, user.name,

value: user.id,
selected: user.id == selected,
data: { role: user.role }

end.join.html_safe

end

Then in the view I might have:

<%= form.select :user_id, user_options form.object.user_id %>

Excellent…

So it’s OK to call policy_scope(User) from a view?

That’s what I was confused by…

Hi John,

firstly - I don’t know pundit, so this is only general advice.

secondly - we’re definitely in the realm of taste and preference here rather than ‘ok’ and ‘not ok’

having said that, my two pence:

the rails style guide says ‘Never call the model layer directly from a view’

https://github.com/rubocop-hq/rails-style-guide#no-direct-model-view

I like to think of views as pieces that will do their job of displaying a thing with minimal worrying about the details of the thing. The view shouldn’t be worrying about things like user policy

also I wouldn’t want authorisation logic in the views just because it is easy to lose track of (and that stuff is important).

my suggestion would be

a) You say you’re calling a lot of policy_scope(User)

this sounds like something where a lot of your calls probably need to find a single scoped user at the start. If this is the case, use a before_action

before_action :get_policy_scoped_user, only: [:show, :update, :destroy]

I’d be tempted to have that before_action assigning an @policy_user which I can then pass down to the view.

this keeps the authorisation stuff in the controller, and the view can just display a user’s stuff without worrying about what is allowed

so - back to your select. I’d write the view so that it will work when passed any kind of user and doesn’t know or care what kind of user it is dealing with.

It just so happens that your controller only passes down a scoped user.

Ok, that makes sense…

Everything is working fine the way it is coded but I am fairly new to RoR and want to learn proper practices.

One of the things I do with pundit is to filter the data by a particular School District, so in the “scope” of Pundit I look to see what role the user has and filter the data presented by the districts they are allowed to see.

I’ll look at moving some of that to the models instead.

For example, here is a scope section from one of my policies

class Scope < Scope

def resolve

if @user.is_global_admin?

scope.all

elsif @user.is_facilitator?

scope.by_districts(@user.district_ids)

elsif @user.is_district_admin(@session[:global_district_id])

scope.by_districts(@user.district_ids)

elsif @user.is_district_user(@session[:global_district_id])

scope.by_districts(@user.district_ids)

elsif @user.is_district_viewer(@session[:global_building_id])

scope.by_districts(@user.district_ids)

elsif @user.is_building_admin(@session[:global_building_id])

scope.by_buildings(@user.building_ids)

elsif @user.is_building_user(@session[:global_building_id])

scope.by_buildings(@user.building_ids)

elsif @user.is_building_viewer(@session[:global_building_id])

scope.by_buildings(@user.building_ids)

elsif @user.is_classroom_user

scope.where(id: @session[:global_building_id])

elsif @user.is_classroom_viewer

scope.where(id: @session[:global_building_id])

else

scope.none

end

end

end

I agree with Rob that it is in the realm of taste and preference. I wouldn’t have something like this in my view:

users = User.where(active: true).join(:transactions).merge(Transaction.where(‘total > 1000’))

form.collection_select users, :id, :name

This would be putting model level stuff in my view as the guide Rob linked to recommended against. That first line should be wrapped up in a model scope or something.

But in the case where I simply want all scoped users I think putting policy_scope(User) in the view is ok. You could create a helper method maybe like:

def scoped_users
policy_scope User

end

Then in the view pass scoped_users to collection_select. But is that really much different than policy_scope(User). IMHO the view is less interacting with the model layer but declaratively specifying to the helper how it should interact with the model layer.

Also the fact that Pundit has explicitly created this helper method of policy_scope means it considers it ok to call from the view.

Again this is getting in the realm of tasted. If I started coding in an app and I saw that the policy_scope was called in the controller and the result was passed to the view I wouldn’t think bad of it.

Eric

again - taste and preference here.

my issue with policy_scope(User) in the view isn’t that it is complex code, clearly it isn’t.

it’s more that I don’t want my view to know or care about authorisation.

I think when you’re talking about a select ‘policy_scope(User).collect’, you’re asking the view to run the following

A) get a bunch of users

B) make sure you only get the ones you’re allowed

C) do something with that info

I don’t like having step B in the view, and would move that to the model (or if appropriate the controller).

then the view logic will be more like

A) take whatever thing I’m given

b) display something about that thing

again - taste and preference here.

my issue with `policy_scope(User)` in the view isn't that it is complex code, clearly it isn't.

it's more that I don't want my view to know or care about authorisation.

I think when you're talking about a select 'policy_scope(User).collect', you're asking the view to run the following

A) get a bunch of users
B) make sure you only get the ones you're allowed
C) do something with that info

I don't like having step B in the view, and would move that to the model (or if appropriate the controller).

All good points. My issue with having this in the model is that you then have to pass that separate (orthogonal) User model into the process from the controller, since the model has no idea who is using it, or when. The controller knows all of these things already, so I put it there (or in a helper, where it is running in the same space).

Walter