codeforamerica / ohana-api

The open source API directory of community social services.
http://ohana-api-demo.herokuapp.com/api
BSD 3-Clause "New" or "Revised" License
185 stars 344 forks source link

Update Pundit policies and authorizations #433

Closed monfresh closed 6 years ago

monfresh commented 6 years ago

Why: For completeness

monfresh commented 6 years ago

cc @md5

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.04%) to 99.221% when pulling adf4b82d65d6663c1e342d95e3c10d5f638eba44 on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.

md5 commented 6 years ago

Two more things.

First, you may want to add after_action :verify_authorized to ApplicationController. This is mainly useful in testing and development to ensure that authorize is called or explicitly skipped in all controller actions.

Second, I still find the fact that the scopes in this project are returning the results of pluck instead of ActiveRecord::Relation instances to be odd. Generally, I've seen scopes like this:

class OrganizationPolicy < ApplicationPolicy
  class Scope < Scope
    def resolve
      if user.super_admin?
        scope.all
      else
        scope.with_locations(location_ids)
      end
    end
  end
end
coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.04%) to 99.222% when pulling efc7992bf3cb2bb3f072f2cef0e365b482eb3785 on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.04%) to 99.225% when pulling c0be70d465fb4d72002ddc09719fdb26f4c3eb6e on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.

monfresh commented 6 years ago

@md5 Thanks for the great feedback! I made some changes here: https://github.com/codeforamerica/ohana-api/pull/433/commits/af62fb5d5044e327b1a4edc8b360d957277423ed

Let me know what you think.

As for returning an Array via pluck instead of ActiveRecord::Relations, I believe it's for performance reasons when paginating and ordering the results in the view.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.04%) to 99.225% when pulling f587145d771684e1c3c4b6995b18409824f419e8 on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.02%) to 99.281% when pulling af62fb5d5044e327b1a4edc8b360d957277423ed on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.02%) to 99.283% when pulling b5eb2f74944be2c8c79f46a6a7a01f429d63b0ad on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.

md5 commented 6 years ago

Cool. Glad to help.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.02%) to 99.283% when pulling b0743ad26391245dfc51232299be5299918f09d6 on update-pundit into 24400d3c48d2211f847707046a4a8d016229b9e7 on master.