Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove cancancan customizations #3148

Closed
wants to merge 1 commit into from
Closed

Remove cancancan customizations #3148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/app/controllers/spree/api/addresses_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class AddressesController < Spree::Api::BaseController
before_action :find_order

def show
authorize! :read, @order, order_token
authorize! :show, @order, order_token
find_address
respond_with(@address)
end
Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/base_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,13 @@ def find_product(id)

def product_scope
if can?(:admin, Spree::Product)
scope = Spree::Product.with_deleted.accessible_by(current_ability, :read).includes(*product_includes)
scope = Spree::Product.with_deleted.accessible_by(current_ability).includes(*product_includes)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary anymore. The default is :index which is by default aliased by :read.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @coorasse! But if :read alias was :show shouldn't it be changed like this scope = Spree::Product.with_deleted.accessible_by(current_ability, :show).includes(*product_includes) to keep same logic? Isn't your change changing scope from show to index now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it changes from :show to :index but all the rules are defined for :read instead of :display which was including also :read. it's quite complicated, I know, I don't know why all these aliases were generated before. accessible_by(ability, :show) would not make sense because you want a list of objects. :show is for single instances.

Copy link

@jkojro jkojro Mar 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(current_ability, :read) made scope of products that user is authorized for action :show I gues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:read was aliased by :display and rules were (almost) always defined on :display. You can see that most of the changes are about that: https://github.com/solidusio/solidus/pull/3148/files#diff-82b30f937458e7c1052c326354dc18cbL8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that :index is the right thing here, as :show would need to check every single instance of a Product if its able to be shown to the user, what is definitely not what we want here.


unless params[:show_deleted]
scope = scope.not_deleted
end
else
scope = Spree::Product.accessible_by(current_ability, :read).available.includes(*product_includes)
scope = Spree::Product.accessible_by(current_ability).available.includes(*product_includes)
end

scope
Expand All @@ -159,7 +159,7 @@ def order_id

def authorize_for_order
@order = Spree::Order.find_by(number: order_id)
authorize! :read, @order, order_token
authorize! :show, @order, order_token
end

def lock_order
Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/countries_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class CountriesController < Spree::Api::BaseController

def index
@countries = Spree::Country.
accessible_by(current_ability, :read).
accessible_by(current_ability).
ransack(params[:q]).
result.
order('name ASC')
Expand All @@ -21,7 +21,7 @@ def index
end

def show
@country = Spree::Country.accessible_by(current_ability, :read).find(params[:id])
@country = Spree::Country.accessible_by(current_ability).find(params[:id])
respond_with(@country)
end
end
Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/credit_cards_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class CreditCardsController < Spree::Api::BaseController
def index
@credit_cards = user
.credit_cards
.accessible_by(current_ability, :read)
.accessible_by(current_ability)
.with_payment_profile
.ransack(params[:q]).result

Expand All @@ -29,7 +29,7 @@ def update

def user
if params[:user_id].present?
@user ||= Spree.user_class.accessible_by(current_ability, :read).find(params[:user_id])
@user ||= Spree.user_class.accessible_by(current_ability).find(params[:user_id])
end
end

Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/images_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ module Spree
module Api
class ImagesController < Spree::Api::BaseController
def index
@images = scope.images.accessible_by(current_ability, :read)
@images = scope.images.accessible_by(current_ability)
respond_with(@images)
end

def show
@image = Spree::Image.accessible_by(current_ability, :read).find(params[:id])
@image = Spree::Image.accessible_by(current_ability).find(params[:id])
respond_with(@image)
end

Expand Down
2 changes: 1 addition & 1 deletion api/app/controllers/spree/api/inventory_units_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def update
private

def inventory_unit
@inventory_unit ||= Spree::InventoryUnit.accessible_by(current_ability, :read).find(params[:id])
@inventory_unit ||= Spree::InventoryUnit.accessible_by(current_ability).find(params[:id])
end

def prepare_event
Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/option_types_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ module Api
class OptionTypesController < Spree::Api::BaseController
def index
if params[:ids]
@option_types = Spree::OptionType.includes(:option_values).accessible_by(current_ability, :read).where(id: params[:ids].split(','))
@option_types = Spree::OptionType.includes(:option_values).accessible_by(current_ability).where(id: params[:ids].split(','))
else
@option_types = Spree::OptionType.includes(:option_values).accessible_by(current_ability, :read).load.ransack(params[:q]).result
@option_types = Spree::OptionType.includes(:option_values).accessible_by(current_ability).load.ransack(params[:q]).result
end
respond_with(@option_types)
end

def show
@option_type = Spree::OptionType.accessible_by(current_ability, :read).find(params[:id])
@option_type = Spree::OptionType.accessible_by(current_ability).find(params[:id])
respond_with(@option_type)
end

Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/option_values_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ def destroy

def scope
if params[:option_type_id]
@scope ||= Spree::OptionType.find(params[:option_type_id]).option_values.accessible_by(current_ability, :read)
@scope ||= Spree::OptionType.find(params[:option_type_id]).option_values.accessible_by(current_ability)
else
@scope ||= Spree::OptionValue.accessible_by(current_ability, :read).load
@scope ||= Spree::OptionValue.accessible_by(current_ability).load
end
end

Expand Down
2 changes: 1 addition & 1 deletion api/app/controllers/spree/api/orders_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def empty
end

def index
authorize! :index, Order
authorize! :admin, Order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

orders_includes = [
:user,
:payments,
Expand Down
2 changes: 1 addition & 1 deletion api/app/controllers/spree/api/payments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def void

def find_order
@order = Spree::Order.find_by(number: order_id)
authorize! :read, @order, order_token
authorize! :show, @order, order_token
end

def find_payment
Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/product_properties_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class ProductPropertiesController < Spree::Api::BaseController
def index
@product_properties = @product.
product_properties.
accessible_by(current_ability, :read).
accessible_by(current_ability).
ransack(params[:q]).
result

Expand Down Expand Up @@ -59,14 +59,14 @@ def destroy

def find_product
@product = super(params[:product_id])
authorize! :read, @product
authorize! :show, @product
end

def product_property
if @product
@product_property ||= @product.product_properties.find_by(id: params[:id])
@product_property ||= @product.product_properties.includes(:property).where(spree_properties: { name: params[:id] }).first
authorize! :read, @product_property
authorize! :show, @product_property
end
end

Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/properties_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class PropertiesController < Spree::Api::BaseController
before_action :find_property, only: [:show, :update, :destroy]

def index
@properties = Spree::Property.accessible_by(current_ability, :read)
@properties = Spree::Property.accessible_by(current_ability)

if params[:ids]
ids = params[:ids].split(",").flatten
Expand Down Expand Up @@ -59,9 +59,9 @@ def destroy
private

def find_property
@property = Spree::Property.accessible_by(current_ability, :read).find(params[:id])
@property = Spree::Property.accessible_by(current_ability).find(params[:id])
rescue ActiveRecord::RecordNotFound
@property = Spree::Property.accessible_by(current_ability, :read).find_by!(name: params[:id])
@property = Spree::Property.accessible_by(current_ability).find_by!(name: params[:id])
end

def property_params
Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/resource_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Spree::Api::ResourceController < Spree::Api::BaseController
before_action :load_resource, only: [:show, :update, :destroy]

def index
collection_scope = model_class.accessible_by(current_ability, :read)
collection_scope = model_class.accessible_by(current_ability)
if params[:ids]
ids = params[:ids].split(",").flatten
collection_scope = collection_scope.where(id: ids)
Expand Down Expand Up @@ -65,7 +65,7 @@ def destroy
protected

def load_resource
@object = model_class.accessible_by(current_ability, :read).find(params[:id])
@object = model_class.accessible_by(current_ability).find(params[:id])
instance_variable_set("@#{object_name}", @object)
end

Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/return_authorizations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def index

@return_authorizations = @order.
return_authorizations.
accessible_by(current_ability, :read).
accessible_by(current_ability).
ransack(params[:q]).
result

Expand All @@ -44,7 +44,7 @@ def new

def show
authorize! :admin, ReturnAuthorization
@return_authorization = @order.return_authorizations.accessible_by(current_ability, :read).find(params[:id])
@return_authorization = @order.return_authorizations.accessible_by(current_ability).find(params[:id])
respond_with(@return_authorization)
end

Expand All @@ -70,7 +70,7 @@ def cancel

def load_order
@order ||= Spree::Order.find_by!(number: order_id)
authorize! :read, @order
authorize! :show, @order
end

def return_authorization_params
Expand Down
2 changes: 1 addition & 1 deletion api/app/controllers/spree/api/shipments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def find_order_on_create
# TODO: Can remove conditional here once deprecated #find_order is removed.
unless @order.present?
@order = Spree::Order.find_by!(number: params[:shipment][:order_id])
authorize! :read, @order
authorize! :show, @order
end
end

Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/states_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ def show

def scope
if params[:country_id]
@country = Spree::Country.accessible_by(current_ability, :read).find(params[:country_id])
@country.states.accessible_by(current_ability, :read)
@country = Spree::Country.accessible_by(current_ability).find(params[:country_id])
@country.states.accessible_by(current_ability)
else
Spree::State.accessible_by(current_ability, :read)
Spree::State.accessible_by(current_ability)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion api/app/controllers/spree/api/stock_items_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def load_stock_location

def scope
includes = { variant: [{ option_values: :option_type }, :product] }
@stock_location.stock_items.accessible_by(current_ability, :read).includes(includes)
@stock_location.stock_items.accessible_by(current_ability).includes(includes)
end

def stock_item_params
Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/stock_locations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ module Spree
module Api
class StockLocationsController < Spree::Api::BaseController
def index
authorize! :read, StockLocation
authorize! :index, StockLocation

@stock_locations = StockLocation.
accessible_by(current_ability, :read).
accessible_by(current_ability).
order('name ASC').
ransack(params[:q]).
result
Expand Down Expand Up @@ -49,7 +49,7 @@ def destroy
private

def stock_location
@stock_location ||= Spree::StockLocation.accessible_by(current_ability, :read).find(params[:id])
@stock_location ||= Spree::StockLocation.accessible_by(current_ability).find(params[:id])
end

def stock_location_params
Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/stock_movements_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class StockMovementsController < Spree::Api::BaseController
before_action :stock_location, except: [:update, :destroy]

def index
authorize! :read, StockMovement
authorize! :index, StockMovement
@stock_movements = paginate(scope.ransack(params[:q]).result)
respond_with(@stock_movements)
end
Expand All @@ -29,11 +29,11 @@ def create
private

def stock_location
@stock_location ||= Spree::StockLocation.accessible_by(current_ability, :read).find(params[:stock_location_id])
@stock_location ||= Spree::StockLocation.accessible_by(current_ability).find(params[:stock_location_id])
end

def scope
@stock_location.stock_movements.accessible_by(current_ability, :read)
@stock_location.stock_movements.accessible_by(current_ability)
end

def stock_movement_params
Expand Down
6 changes: 3 additions & 3 deletions api/app/controllers/spree/api/stores_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ class StoresController < Spree::Api::BaseController
before_action :get_store, except: [:index, :create]

def index
authorize! :read, Store
@stores = Spree::Store.accessible_by(current_ability, :read).all
authorize! :index, Store
@stores = Spree::Store.accessible_by(current_ability).all
respond_with(@stores)
end

Expand All @@ -32,7 +32,7 @@ def update
end

def show
authorize! :read, @store
authorize! :show, @store
respond_with(@store)
end

Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/taxonomies_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ def destroy

def taxonomies
@taxonomies = Taxonomy.
accessible_by(current_ability, :read).
accessible_by(current_ability).
order('name').
includes(root: :children).
ransack(params[:q]).
result
end

def taxonomy
@taxonomy ||= Spree::Taxonomy.accessible_by(current_ability, :read).find(params[:id])
@taxonomy ||= Spree::Taxonomy.accessible_by(current_ability).find(params[:id])
end

def taxonomy_params
Expand Down
8 changes: 4 additions & 4 deletions api/app/controllers/spree/api/taxons_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ def index
if taxonomy
@taxons = taxonomy.root.children
elsif params[:ids]
@taxons = Spree::Taxon.accessible_by(current_ability, :read).where(id: params[:ids].split(','))
@taxons = Spree::Taxon.accessible_by(current_ability).where(id: params[:ids].split(','))
else
@taxons = Spree::Taxon.accessible_by(current_ability, :read).order(:taxonomy_id, :lft).ransack(params[:q]).result
@taxons = Spree::Taxon.accessible_by(current_ability).order(:taxonomy_id, :lft).ransack(params[:q]).result
end

unless params[:without_children]
Expand Down Expand Up @@ -96,12 +96,12 @@ def default_per_page

def taxonomy
if params[:taxonomy_id].present?
@taxonomy ||= Spree::Taxonomy.accessible_by(current_ability, :read).find(params[:taxonomy_id])
@taxonomy ||= Spree::Taxonomy.accessible_by(current_ability).find(params[:taxonomy_id])
end
end

def taxon
@taxon ||= taxonomy.taxons.accessible_by(current_ability, :read).find(params[:id])
@taxon ||= taxonomy.taxons.accessible_by(current_ability).find(params[:id])
end

def taxon_params
Expand Down
4 changes: 2 additions & 2 deletions api/app/controllers/spree/api/variants_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def update
private

def product
@product ||= Spree::Product.accessible_by(current_ability, :read).friendly.find(params[:product_id]) if params[:product_id]
@product ||= Spree::Product.accessible_by(current_ability).friendly.find(params[:product_id]) if params[:product_id]
end

def scope
Expand All @@ -69,7 +69,7 @@ def scope

in_stock_only = ActiveRecord::Type::Boolean.new.cast(params[:in_stock_only])
suppliable_only = ActiveRecord::Type::Boolean.new.cast(params[:suppliable_only])
variants = variants.accessible_by(current_ability, :read)
variants = variants.accessible_by(current_ability)
if in_stock_only || cannot?(:view_out_of_stock, Spree::Variant)
variants = variants.in_stock
elsif suppliable_only
Expand Down
Loading