Proper Coding Question

[John Sanderbeck]

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

[Eric Anderson]

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 %>

[John Sanderbeck]

Excellent...

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

That's what I was confused by...

[Rob Jonson]

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.

 

[John Sanderbeck]

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

[Eric Anderson]

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

[Rob Jonson]

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

[Walter Lee Davis]

> On Oct 5, 2018, at 4:17 AM, Rob Jonson <rob.j...@gmail.com> wrote:
>
> 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

>
> then the view logic will be more like
>
> A) take whatever thing I'm given
> b) display something about that thing
>
>
>
>
>
>
>
>
>
>

> --
> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-ta...@googlegroups.com.
> To post to this group, send email to rubyonra...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-talk/191a9289-750b-4b52-8fb2-219717ec9acb%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.