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

Replace spree roles #13090

Merged
merged 6 commits into from
Jan 31, 2025
Merged
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
18 changes: 1 addition & 17 deletions app/controllers/spree/admin/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,9 @@ def index
end

def create
if params[:user]
roles = params[:user].delete("spree_role_ids")
end

@user = Spree::User.new(user_params)
if @user.save

if roles
@user.spree_roles = roles.compact_blank.collect{ |r| Spree::Role.find(r) }
end

flash[:success] = Spree.t(:created_successfully)
redirect_to edit_admin_user_path(@user)
else
Expand All @@ -39,15 +31,7 @@ def create
end

def update
if params[:user]
roles = params[:user].delete("spree_role_ids")
end

if @user.update(user_params)
if roles
@user.spree_roles = roles.compact_blank.collect{ |r| Spree::Role.find(r) }
end

flash[:success] = update_message
redirect_to edit_admin_user_path(@user)
else
Expand Down Expand Up @@ -131,7 +115,7 @@ def build_resource

def user_params
::PermittedAttributes::User.new(params).call(
%i[enterprise_limit show_api_key_view]
%i[admin enterprise_limit show_api_key_view]
)
end
end
Expand Down
13 changes: 0 additions & 13 deletions app/models/spree/role.rb

This file was deleted.

11 changes: 1 addition & 10 deletions app/models/spree/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,11 @@ class User < ApplicationRecord
belongs_to :ship_address, class_name: 'Spree::Address'
belongs_to :bill_address, class_name: 'Spree::Address'

has_and_belongs_to_many :spree_roles,
join_table: 'spree_roles_users',
class_name: "Spree::Role"

before_validation :set_login
after_create :associate_customers, :associate_orders
before_destroy :check_completed_orders

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

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

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

# Send devise-based user emails asyncronously via ActiveJob
# See: https://github.com/heartcombo/devise/tree/v3.5.10#activejob-integration
def send_devise_notification(notification, *args)
Expand Down
9 changes: 2 additions & 7 deletions app/views/spree/admin/users/_form.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,8 @@
= f.email_field :email, class: "fullwidth"
= error_message_on :user, :email
.field
= label_tag nil, t(".roles")
%ul
- [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
= hidden_field_tag "user[spree_role_ids][]", ""
= f.label :admin, t(".admin")
= f.check_box :admin
= f.field_container :locale do
= f.label :locale, t(".locale")
= f.select :locale, locale_options, class: "fullwidth"
Expand Down
2 changes: 1 addition & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4660,7 +4660,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using
form:
disabled: "Disabled?"
email: "Email"
roles: "Roles"
admin: "Super admin?"
enterprise_limit: "Enterprise Limit"
confirm_password: "Confirm Password"
password: "Password"
Expand Down
2 changes: 1 addition & 1 deletion db/default/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ def create_admin_user
ValidEmail2::Address.define_method(:valid_mx?) { true }

if admin.save
admin.spree_roles << Spree::Role.admin
say "New admin user persisted!"
else
say "There was some problems with persisting new admin user:"
Expand All @@ -81,6 +80,7 @@ def read_user_attributes
end

{
admin: true,
password:,
password_confirmation: password,
email:,
Expand Down
8 changes: 8 additions & 0 deletions db/migrate/20250107013958_add_admin_to_spree_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true

# We'll replace our only role "admin" with a simple flag.
class AddAdminToSpreeUsers < ActiveRecord::Migration[7.0]
def change
add_column :spree_users, :admin, :boolean, default: false, null: false
end
end
13 changes: 13 additions & 0 deletions db/migrate/20250107014617_copy_admin_attribute_to_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

class CopyAdminAttributeToUsers < ActiveRecord::Migration[7.0]
def up
execute <<~SQL.squish
UPDATE spree_users SET admin = true WHERE id IN (
SELECT user_id FROM spree_roles_users WHERE role_id IN (
SELECT id FROM spree_roles WHERE name = 'admin'
)
)
SQL
end
end
1 change: 1 addition & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,7 @@
t.string "provider"
t.string "uid"
t.datetime "terms_of_service_accepted_at"
t.boolean "admin", default: false, null: false
t.index ["confirmation_token"], name: "index_spree_users_on_confirmation_token", unique: true
t.index ["email"], name: "email_idx_unique", unique: true
t.index ["persistence_token"], name: "index_users_on_persistence_token"
Expand Down
6 changes: 1 addition & 5 deletions spec/controllers/admin/order_cycles_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -431,11 +431,7 @@ def build_resource

describe "notifying producers" do
let(:user) { create(:user) }
let(:admin_user) do
user = create(:user)
user.spree_roles << Spree::Role.find_or_create_by!(name: 'admin')
user
end
let(:admin_user) { create(:admin_user) }
let(:order_cycle) { create(:simple_order_cycle) }

before do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,6 @@ class GatewayWithPassword < PaymentMethod
let(:user) do
new_user = create(:user, email: 'enterprise@hub.com', password: 'blahblah',
password_confirmation: 'blahblah', )
# for some reason unbeknown to me, this new user gets admin permissions by default.
new_user.spree_roles = []
Copy link
Member

Choose a reason for hiding this comment

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

It's reassuring to see this is no longer necessary!

new_user.enterprise_roles.build(enterprise:).save
new_user.save
new_user
Expand Down
40 changes: 21 additions & 19 deletions spec/controllers/spree/admin/users_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,39 @@
require 'spec_helper'

RSpec.describe Spree::Admin::UsersController do
context '#authorize_admin' do
describe '#authorize_admin' do
let(:user) { create(:user) }
let(:test_user) { create(:user) }

before do
allow(controller).to receive_messages spree_current_user: user
allow(Spree::User).to receive(:find).with(test_user.id.to_s).and_return(test_user)
user.spree_roles.clear
end

it 'should grant access to users with an admin role' do
user.spree_roles << Spree::Role.find_or_create_by(name: 'admin')
spree_post :index
expect(response).to render_template :index
end
context "as a super admin" do
let(:user) { create(:admin_user) }
let(:test_user) { create(:user) }

it "allows admins to update a user's show api key view" do
user.spree_roles << Spree::Role.find_or_create_by(name: 'admin')
spree_put :update, id: test_user.id, user: { show_api_key_view: true }
expect(response).to redirect_to spree.edit_admin_user_path(test_user)
end
before do
allow(Spree::User).to receive(:find).with(test_user.id.to_s).and_return(test_user)
end

it 'should grant access to users with an admin role' do
spree_post :index
expect(response).to render_template :index
end

it "allows admins to update a user's show api key view" do
spree_put :update, id: test_user.id, user: { show_api_key_view: true }
expect(response).to redirect_to spree.edit_admin_user_path(test_user)
end

it "re-renders the edit form if error" do
user.spree_roles << Spree::Role.find_or_create_by(name: 'admin')
spree_put :update, id: test_user.id, user: { password: "blah", password_confirmation: "" }
it "re-renders the edit form if error" do
spree_put :update, id: test_user.id, user: { password: "blah", password_confirmation: "" }

expect(response).to render_template :edit
expect(response).to render_template :edit
end
end

it 'should deny access to users without an admin role' do
allow(user).to receive_messages admin?: false
spree_post :index
expect(response).to redirect_to('/unauthorized')
end
Expand Down
2 changes: 1 addition & 1 deletion spec/factories/user_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
end

factory :admin_user do
spree_roles { [Spree::Role.find_or_create_by!(name: 'admin')] }
admin { true }
end

factory :oidc_user do
Expand Down
13 changes: 2 additions & 11 deletions spec/lib/reports/customers_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ module Reports
module Customers
RSpec.describe Base do
context "as a site admin" do
let(:user) do
user = create(:user)
user.spree_roles << Spree::Role.find_or_create_by!(name: 'admin')
user
end
let(:user) { create(:admin_user) }
subject { Base.new user, {} }

describe "addresses report" do
Expand Down Expand Up @@ -198,12 +194,7 @@ module Customers
end

context "as an enterprise user" do
let(:user) do
user = create(:user)
user.spree_roles = []
user.save!
user
end
let(:user) { create(:user) }

subject { Base.new user, {} }

Expand Down
6 changes: 1 addition & 5 deletions spec/lib/reports/order_cycle_management_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@ module OrderCycleManagement
subject { Base.new(user, params) }
let(:params) { {} }

let(:user) do
user = create(:user)
user.spree_roles << Spree::Role.find_or_create_by!(name: "admin")
user
end
let(:user) { create(:admin_user) }

describe "fetching orders" do
it 'calls the OutstandingBalanceQuery query object' do
Expand Down
13 changes: 2 additions & 11 deletions spec/lib/reports/products_and_inventory_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ module Reports
module ProductsAndInventory
RSpec.describe Base do
context "As a site admin" do
let(:user) do
user = create(:user)
user.spree_roles << Spree::Role.find_or_create_by!(name: 'admin')
user
end
let(:user) { create(:admin_user) }
subject do
Base.new user, {}
end
Expand Down Expand Up @@ -72,7 +68,6 @@ module ProductsAndInventory
let(:enterprise_user) do
user = create(:user)
user.enterprise_roles.create(enterprise: supplier)
user.spree_roles = []
user.save!
user
end
Expand Down Expand Up @@ -259,11 +254,7 @@ module ProductsAndInventory
end

RSpec.describe AllProducts do
let(:user) do
user = create(:user)
user.spree_roles << Spree::Role.find_or_create_by!(name: 'admin')
user
end
let(:user) { create(:admin_user) }
let(:report) do
AllProducts.new user, { fields_to_hide: [] }
end
Expand Down
1 change: 0 additions & 1 deletion spec/models/enterprise_group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@

it "finds a user's enterprise groups" do
user = create(:user)
user.spree_roles = []
eg1 = create(:enterprise_group, owner: user)
eg2 = create(:enterprise_group)

Expand Down
1 change: 0 additions & 1 deletion spec/models/enterprise_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,6 @@
describe "managed_by" do
it "shows only enterprises for given user" do
user = create(:user)
user.spree_roles = []
e1 = create(:enterprise)
e2 = create(:enterprise)
e1.enterprise_roles.build(user:).save
Expand Down
6 changes: 1 addition & 5 deletions spec/models/exchange_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,7 @@
let(:oc) { create(:simple_order_cycle, coordinator:) }

describe "finding exchanges managed by a particular user" do
let(:user) do
user = create(:user)
user.spree_roles = []
user
end
let(:user) { create(:user) }

before { Exchange.destroy_all }

Expand Down
3 changes: 1 addition & 2 deletions spec/models/order_cycle_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@
it "finds order cycles accessible by a user" do
e1 = create(:enterprise, is_primary_producer: true, sells: "any")
e2 = create(:enterprise, is_primary_producer: true, sells: "any")
user = create(:user, enterprises: [e2], spree_roles: [])
user.spree_roles = []
user = create(:user, enterprises: [e2])

oc_coordinated = create(:simple_order_cycle, coordinator: e2)
oc_sent = create(:simple_order_cycle, suppliers: [e2])
Expand Down
Loading
Loading