Skip to content

Commit

Permalink
Merge pull request #13047 from mkllnk/spree-roles
Browse files Browse the repository at this point in the history
Clarify that our only user role is "admin" and simplify code
  • Loading branch information
filipefurtad0 authored Jan 17, 2025
2 parents 5726eef + 5db8cb4 commit 3c1dd10
Show file tree
Hide file tree
Showing 27 changed files with 34 additions and 50 deletions.
2 changes: 1 addition & 1 deletion app/controllers/admin/products_v3_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def fetch_products

def product_scope
user = spree_current_user
scope = if user.has_spree_role?("admin") || user.enterprises.present?
scope = if user.admin? || user.enterprises.present?
Spree::Product
else
Spree::Product.active
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/api/v0/variants_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ def product

def scope
if @product
variants = if current_api_user.has_spree_role?("admin") || params[:show_deleted]
variants = if current_api_user.admin? || params[:show_deleted]
@product.variants.with_deleted
else
@product.variants
end
else
variants = Spree::Variant.where(nil)
if current_api_user.has_spree_role?("admin")
if current_api_user.admin?
unless params[:show_deleted]
variants = Spree::Variant.active
end
Expand Down
6 changes: 0 additions & 6 deletions app/controllers/spree/admin/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ class UsersController < ::Admin::ResourceController

# http://spreecommerce.com/blog/2010/11/02/json-hijacking-vulnerability/
before_action :check_json_authenticity, only: :index
before_action :load_roles, only: [:edit, :new, :update, :create,
:generate_api_key, :clear_api_key]

def index
respond_with(@collection) do |format|
Expand Down Expand Up @@ -123,10 +121,6 @@ def sign_in_if_change_own_password
sign_in(@user, event: :authentication, bypass: true)
end

def load_roles
@roles = Spree::Role.where(nil)
end

def new_email_unconfirmed?
params[:user][:email] != @user.email
end
Expand Down
2 changes: 1 addition & 1 deletion app/helpers/shared_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ def enterprise_user?
end

def admin_user?
spree_current_user&.has_spree_role? 'admin'
spree_current_user&.admin?
end

def current_shop_products_path
Expand Down
2 changes: 1 addition & 1 deletion app/models/enterprise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ class Enterprise < ApplicationRecord
}

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
joins(:enterprise_roles).where(enterprise_roles: { user_id: user.id })
Expand Down
2 changes: 1 addition & 1 deletion app/models/enterprise_fee.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class EnterpriseFee < ApplicationRecord
scope :for_enterprises, lambda { |enterprises| where(enterprise_id: enterprises) }

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
where(enterprise_id: user.enterprises.select(&:id))
Expand Down
2 changes: 1 addition & 1 deletion app/models/enterprise_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class EnterpriseGroup < ApplicationRecord
scope :by_position, -> { order('position ASC') }
scope :on_front_page, -> { where(on_front_page: true) }
scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
where(owner_id: user.id)
Expand Down
2 changes: 1 addition & 1 deletion app/models/exchange.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class Exchange < ApplicationRecord
}

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
joins("LEFT JOIN enterprises senders ON senders.id = exchanges.sender_id").
Expand Down
4 changes: 2 additions & 2 deletions app/models/order_cycle.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class OrderCycle < ApplicationRecord
}

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
where(coordinator_id: user.enterprises.to_a)
Expand All @@ -93,7 +93,7 @@ class OrderCycle < ApplicationRecord

# Return order cycles that user coordinates, sends to or receives from
scope :visible_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
with_exchanging_enterprises_outer.
Expand Down
2 changes: 1 addition & 1 deletion app/models/spree/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def initialize(user)

user ||= Spree::User.new

if user.respond_to?(:has_spree_role?) && user.has_spree_role?('admin')
if user.try(:admin?)
can :manage, :all
else
can [:index, :read], Country
Expand Down
2 changes: 1 addition & 1 deletion app/models/spree/line_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class LineItem < ApplicationRecord

# -- Scopes
scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
# Find line items that are from orders distributed by the user or supplied by the user
Expand Down
4 changes: 2 additions & 2 deletions app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def states
}

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
# Find orders that are distributed by the user or have products supplied by the user
Expand All @@ -140,7 +140,7 @@ def states
}

scope :distributed_by_user, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
where(spree_orders: { distributor_id: user.enterprises.select(&:id) })
Expand Down
2 changes: 1 addition & 1 deletion app/models/spree/product.rb
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class Product < ApplicationRecord
scope :by_name, -> { order('spree_products.name') }

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
in_supplier(user.enterprises)
Expand Down
5 changes: 5 additions & 0 deletions app/models/spree/role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,10 @@ module Spree
class Role < ApplicationRecord
has_and_belongs_to_many :users, join_table: 'spree_roles_users',
class_name: "Spree::User"

# The only role we have at the moment:
def self.admin
Spree::Role.find_or_create_by(name: 'admin')
end
end
end
2 changes: 1 addition & 1 deletion app/models/spree/shipping_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class ShippingMethod < ApplicationRecord
after_save :touch_distributors

scope :managed_by, lambda { |user|
if user.has_spree_role?('admin')
if user.admin?
where(nil)
else
joins(:distributors).
Expand Down
11 changes: 2 additions & 9 deletions app/models/spree/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ class User < ApplicationRecord
after_create :associate_customers, :associate_orders
before_destroy :check_completed_orders

roles_table_name = Role.table_name

scope :admin, lambda { includes(:spree_roles).where("#{roles_table_name}.name" => "admin") }
scope :admin, lambda { includes(:spree_roles).where("spree_roles.name" => "admin") }

has_many :enterprise_roles, dependent: :destroy
has_many :enterprises, through: :enterprise_roles
Expand Down Expand Up @@ -60,14 +58,9 @@ def self.admin_created?
User.admin.count > 0
end

# Whether a user has a role or not.
def has_spree_role?(role_in_question)
spree_roles.where(name: role_in_question.to_s).any?
end

# Checks whether the specified user is a superadmin, with full control of the instance
def admin?
has_spree_role?('admin')
spree_roles.any? { |role| role.name == "admin" }
end

# Send devise-based user emails asyncronously via ActiveJob
Expand Down
2 changes: 1 addition & 1 deletion app/queries/product_scope_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def products_for_producers
end

def product_scope
if @user.has_spree_role?("admin") || @user.enterprises.present?
if @user.admin? || @user.enterprises.present?
scope = Spree::Product
if @params[:show_deleted]
scope = scope.with_deleted
Expand Down
2 changes: 1 addition & 1 deletion app/views/spree/admin/users/_form.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
.field
= label_tag nil, t(".roles")
%ul
- @roles.each do |role|
- [Spree::Role.admin].each do |role|
%li
= check_box_tag "user[spree_role_ids][]", role.id, @user.spree_roles.include?(role), id: "user_spree_role_#{role.name}"
= label_tag role.name
Expand Down
3 changes: 1 addition & 2 deletions db/default/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ def create_admin_user
ValidEmail2::Address.define_method(:valid_mx?) { true }

if admin.save
role = Spree::Role.find_or_create_by(name: 'admin')
admin.spree_roles << role
admin.spree_roles << Spree::Role.admin
say "New admin user persisted!"
else
say "There was some problems with persisting new admin user:"
Expand Down
4 changes: 0 additions & 4 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
# We need mail_configuration to create a user account, because it sends a confirmation email.
MailConfiguration.apply!

puts "[db:seed] Seeding Roles"
Spree::Role.where(:name => "admin").first_or_create
Spree::Role.where(:name => "user").first_or_create

puts "[db:seed] Seeding Countries"
unless Spree::Country.find_by(iso: ENV['DEFAULT_COUNTRY_CODE'])
require File.join(File.dirname(__FILE__), 'default', 'countries')
Expand Down
1 change: 0 additions & 1 deletion spec/controllers/admin/order_cycles_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ module Admin
order_cycles: 6,
proxy_orders: 1,
schedules: 1,
spree_roles: 9,
spree_variants: 8,
tags: 1
},
Expand Down
8 changes: 4 additions & 4 deletions spec/controllers/api/v0/products_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

before do
allow(current_api_user)
.to receive(:has_spree_role?).with("admin").and_return(false)
.to receive(:admin?).and_return(false)
end

it "gets a single product" do
Expand Down Expand Up @@ -96,7 +96,7 @@
context "as an administrator" do
before do
allow(current_api_user)
.to receive(:has_spree_role?).with("admin").and_return(true)
.to receive(:admin?).and_return(true)
end

it "can create a new product" do
Expand Down Expand Up @@ -153,7 +153,7 @@
context 'as a normal user' do
before do
allow(current_api_user)
.to receive(:has_spree_role?).with("admin").and_return(false)
.to receive(:admin?).and_return(false)
end

it 'denies access' do
Expand Down Expand Up @@ -204,7 +204,7 @@
context 'as an administrator' do
before do
allow(current_api_user)
.to receive(:has_spree_role?).with("admin").and_return(true)
.to receive(:admin?).and_return(true)
end

it 'responds with a successful response' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
id: nil,
owned_groups: nil)
allow(user).to receive_messages(enterprises: [create(:enterprise)],
has_spree_role?: true,
admin?: true,
locale: nil)
allow(controller).to receive_messages(spree_current_user: user)

Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/spree/admin/users_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
end

it 'should deny access to users without an admin role' do
allow(user).to receive_messages has_spree_role?: false
allow(user).to receive_messages admin?: false
spree_post :index
expect(response).to redirect_to('/unauthorized')
end
Expand Down
2 changes: 0 additions & 2 deletions spec/lib/reports/orders_and_distributors_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,6 @@
subject # build context first

expect { subject.table_rows }.to query_database [
"Spree::Role Exists?",
"Spree::Role Exists?",
"SQL",
"Spree::LineItem Load",
"Spree::PaymentMethod Load",
Expand Down
2 changes: 1 addition & 1 deletion spec/models/spree/ability_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
let(:resource) { Object.new }

context 'with admin user' do
before(:each) { allow(user).to receive(:has_spree_role?).and_return(true) }
before(:each) { allow(user).to receive(:admin?).and_return(true) }
it_should_behave_like 'access granted'
it_should_behave_like 'index allowed'
end
Expand Down
2 changes: 1 addition & 1 deletion spec/services/permissions/order_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ module Permissions
let(:permissions) { Permissions::Order.new(user, search_params) }

before do
allow(user).to receive(:has_spree_role?) { "admin" }
allow(user).to receive(:admin?) { "admin" }
end

it "only returns line items from completed, " \
Expand Down

0 comments on commit 3c1dd10

Please sign in to comment.