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.

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

Clarify that our only user role is "admin" and simplify code #13047

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 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" }
Copy link
Member

Choose a reason for hiding this comment

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

Good solution. I would have simply reached for elvis (@admin ||= ), but this is no more queries, and ensures that if the roles are updated during a request, we get the up-to-date value.

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
Loading