From 698d1daa5732bc3a8cc3715ef2865538ff95f8aa Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 7 Jan 2025 12:45:40 +1100 Subject: [PATCH 1/6] Add admin attribute to users --- db/migrate/20250107013958_add_admin_to_spree_users.rb | 8 ++++++++ db/schema.rb | 1 + 2 files changed, 9 insertions(+) create mode 100644 db/migrate/20250107013958_add_admin_to_spree_users.rb diff --git a/db/migrate/20250107013958_add_admin_to_spree_users.rb b/db/migrate/20250107013958_add_admin_to_spree_users.rb new file mode 100644 index 0000000000..057a43425f --- /dev/null +++ b/db/migrate/20250107013958_add_admin_to_spree_users.rb @@ -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 diff --git a/db/schema.rb b/db/schema.rb index ea7efe830f..e8dd1e69ac 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -945,6 +945,7 @@ ActiveRecord::Schema[7.0].define(version: 2025_01_13_055412) do 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" From 920002e084c6c649eeec2f6a59630123e9736cf3 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 7 Jan 2025 13:37:46 +1100 Subject: [PATCH 2/6] Copy admin attribute to users --- ...107014617_copy_admin_attribute_to_users.rb | 13 ++++++++++++ ...4617_copy_admin_attribute_to_users_spec.rb | 21 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 db/migrate/20250107014617_copy_admin_attribute_to_users.rb create mode 100644 spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb diff --git a/db/migrate/20250107014617_copy_admin_attribute_to_users.rb b/db/migrate/20250107014617_copy_admin_attribute_to_users.rb new file mode 100644 index 0000000000..869c20d11f --- /dev/null +++ b/db/migrate/20250107014617_copy_admin_attribute_to_users.rb @@ -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 diff --git a/spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb b/spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb new file mode 100644 index 0000000000..1391b66897 --- /dev/null +++ b/spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative "../../db/migrate/#{File.basename(__FILE__, '_spec.rb')}" + +RSpec.describe CopyAdminAttributeToUsers do + describe "#up" do + it "marks current admins as admin" do + admin = create(:admin_user) + enterprise_user = create(:enterprise_user) + customer = create(:user) + + expect { subject.up }.to change { + admin.reload.admin + }.from(false).to(true) + + expect(enterprise_user.reload.admin).to eq false + expect(customer.reload.admin).to eq false + end + end +end From d49cea5e3d805bdc44e5a8f7ea8ef3daad208a93 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 7 Jan 2025 13:43:31 +1100 Subject: [PATCH 3/6] Use admin flag instead of user role --- .../spree/admin/users_controller.rb | 18 +--------------- app/models/spree/user.rb | 11 +--------- app/views/spree/admin/users/_form.html.haml | 9 ++------ config/locales/en.yml | 2 +- db/default/users.rb | 2 +- .../admin/order_cycles_controller_spec.rb | 6 +----- .../admin/payment_methods_controller_spec.rb | 2 -- .../spree/admin/users_controller_spec.rb | 7 +++---- spec/factories/user_factory.rb | 2 +- spec/lib/reports/customers_report_spec.rb | 13 ++---------- .../order_cycle_management_report_spec.rb | 6 +----- .../products_and_inventory_report_spec.rb | 13 ++---------- ...4617_copy_admin_attribute_to_users_spec.rb | 21 ------------------- spec/models/enterprise_group_spec.rb | 1 - spec/models/enterprise_spec.rb | 1 - spec/models/exchange_spec.rb | 6 +----- spec/models/order_cycle_spec.rb | 3 +-- spec/models/spree/ability_spec.rb | 16 ++------------ spec/models/spree/product_spec.rb | 1 - spec/models/spree/user_spec.rb | 4 ++-- spec/support/controller_helper.rb | 7 +------ .../configuration/shipping_categories_spec.rb | 1 - spec/system/admin/multilingual_spec.rb | 4 +--- 23 files changed, 24 insertions(+), 132 deletions(-) delete mode 100644 spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb diff --git a/app/controllers/spree/admin/users_controller.rb b/app/controllers/spree/admin/users_controller.rb index 4591f1f5d5..00fd64e8a9 100644 --- a/app/controllers/spree/admin/users_controller.rb +++ b/app/controllers/spree/admin/users_controller.rb @@ -20,17 +20,9 @@ module Spree 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 @@ -39,15 +31,7 @@ module Spree 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 @@ -131,7 +115,7 @@ module Spree 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 diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index 371ab636f7..72f2a3c80a 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -18,15 +18,11 @@ module Spree 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 @@ -58,11 +54,6 @@ module Spree 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) diff --git a/app/views/spree/admin/users/_form.html.haml b/app/views/spree/admin/users/_form.html.haml index 4aac09f35f..2064cb4f74 100644 --- a/app/views/spree/admin/users/_form.html.haml +++ b/app/views/spree/admin/users/_form.html.haml @@ -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" diff --git a/config/locales/en.yml b/config/locales/en.yml index 48ba87aee5..07a974d49a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -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" diff --git a/db/default/users.rb b/db/default/users.rb index 23ba8f549b..85fdd0379c 100644 --- a/db/default/users.rb +++ b/db/default/users.rb @@ -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:" @@ -81,6 +80,7 @@ def read_user_attributes end { + admin: true, password:, password_confirmation: password, email:, diff --git a/spec/controllers/admin/order_cycles_controller_spec.rb b/spec/controllers/admin/order_cycles_controller_spec.rb index b91bf12377..35cefae5d1 100644 --- a/spec/controllers/admin/order_cycles_controller_spec.rb +++ b/spec/controllers/admin/order_cycles_controller_spec.rb @@ -431,11 +431,7 @@ module Admin 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 diff --git a/spec/controllers/spree/admin/payment_methods_controller_spec.rb b/spec/controllers/spree/admin/payment_methods_controller_spec.rb index e6d3288603..7124e9fecd 100644 --- a/spec/controllers/spree/admin/payment_methods_controller_spec.rb +++ b/spec/controllers/spree/admin/payment_methods_controller_spec.rb @@ -235,8 +235,6 @@ module Spree 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 = [] new_user.enterprise_roles.build(enterprise:).save new_user.save new_user diff --git a/spec/controllers/spree/admin/users_controller_spec.rb b/spec/controllers/spree/admin/users_controller_spec.rb index 97f0155f93..ddcaf3e0df 100644 --- a/spec/controllers/spree/admin/users_controller_spec.rb +++ b/spec/controllers/spree/admin/users_controller_spec.rb @@ -10,23 +10,22 @@ RSpec.describe Spree::Admin::UsersController do 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') + user.update!(admin: true) spree_post :index expect(response).to render_template :index end it "allows admins to update a user's show api key view" do - user.spree_roles << Spree::Role.find_or_create_by(name: 'admin') + user.update!(admin: true) 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') + user.update!(admin: true) spree_put :update, id: test_user.id, user: { password: "blah", password_confirmation: "" } expect(response).to render_template :edit diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index 5646f839ab..03d612bac4 100644 --- a/spec/factories/user_factory.rb +++ b/spec/factories/user_factory.rb @@ -35,7 +35,7 @@ FactoryBot.define do end factory :admin_user do - spree_roles { [Spree::Role.find_or_create_by!(name: 'admin')] } + admin { true } end factory :oidc_user do diff --git a/spec/lib/reports/customers_report_spec.rb b/spec/lib/reports/customers_report_spec.rb index 19f7283a78..9705260873 100644 --- a/spec/lib/reports/customers_report_spec.rb +++ b/spec/lib/reports/customers_report_spec.rb @@ -7,11 +7,7 @@ module Reporting 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 @@ -198,12 +194,7 @@ module Reporting 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, {} } diff --git a/spec/lib/reports/order_cycle_management_report_spec.rb b/spec/lib/reports/order_cycle_management_report_spec.rb index 17136d5942..a5883b8d27 100644 --- a/spec/lib/reports/order_cycle_management_report_spec.rb +++ b/spec/lib/reports/order_cycle_management_report_spec.rb @@ -10,11 +10,7 @@ module Reporting 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 diff --git a/spec/lib/reports/products_and_inventory_report_spec.rb b/spec/lib/reports/products_and_inventory_report_spec.rb index 4962220a94..3eaca20d01 100644 --- a/spec/lib/reports/products_and_inventory_report_spec.rb +++ b/spec/lib/reports/products_and_inventory_report_spec.rb @@ -7,11 +7,7 @@ module Reporting 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 @@ -72,7 +68,6 @@ module Reporting let(:enterprise_user) do user = create(:user) user.enterprise_roles.create(enterprise: supplier) - user.spree_roles = [] user.save! user end @@ -259,11 +254,7 @@ module Reporting 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 diff --git a/spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb b/spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb deleted file mode 100644 index 1391b66897..0000000000 --- a/spec/migrations/20250107014617_copy_admin_attribute_to_users_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require_relative "../../db/migrate/#{File.basename(__FILE__, '_spec.rb')}" - -RSpec.describe CopyAdminAttributeToUsers do - describe "#up" do - it "marks current admins as admin" do - admin = create(:admin_user) - enterprise_user = create(:enterprise_user) - customer = create(:user) - - expect { subject.up }.to change { - admin.reload.admin - }.from(false).to(true) - - expect(enterprise_user.reload.admin).to eq false - expect(customer.reload.admin).to eq false - end - end -end diff --git a/spec/models/enterprise_group_spec.rb b/spec/models/enterprise_group_spec.rb index 7e7c5fd95c..6ce1aa856f 100644 --- a/spec/models/enterprise_group_spec.rb +++ b/spec/models/enterprise_group_spec.rb @@ -77,7 +77,6 @@ RSpec.describe EnterpriseGroup do it "finds a user's enterprise groups" do user = create(:user) - user.spree_roles = [] eg1 = create(:enterprise_group, owner: user) eg2 = create(:enterprise_group) diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index de519e52e8..1d05f99bd6 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -651,7 +651,6 @@ RSpec.describe Enterprise do 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 diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index b790d47356..2377e3618e 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -107,11 +107,7 @@ RSpec.describe Exchange do 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 } diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 873f6730ad..395be0a985 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -62,8 +62,7 @@ RSpec.describe OrderCycle do 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]) diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index be228abfca..24b1adae26 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -9,14 +9,6 @@ RSpec.describe Spree::Ability do let(:subject) { Spree::Ability.new(user) } let(:token) { nil } - before do - user.spree_roles.clear - end - - after(:each) { - user.spree_roles = [] - } - context 'for general resource' do let(:resource) { Object.new } @@ -43,7 +35,7 @@ RSpec.describe Spree::Ability do context 'with admin user' do it 'should be able to admin' do - user.spree_roles << Spree::Role.find_or_create_by(name: 'admin') + user.update!(admin: true) expect(subject).to be_able_to :admin, resource expect(subject).to be_able_to :index, resource_order expect(subject).to be_able_to :show, resource_product @@ -303,7 +295,6 @@ RSpec.describe Spree::Ability do # create supplier_enterprise1 user without full admin access let(:user) do user = create(:user) - user.spree_roles = [] s1.enterprise_roles.build(user:).save user end @@ -487,7 +478,6 @@ RSpec.describe Spree::Ability do context "when is a distributor enterprise user" do let(:user) do user = create(:user) - user.spree_roles = [] d1.enterprise_roles.build(user:).save user end @@ -699,7 +689,6 @@ RSpec.describe Spree::Ability do context 'Order Cycle co-ordinator, distributor enterprise manager' do let(:user) do user = create(:user) - user.spree_roles = [] d1.enterprise_roles.build(user:).save user end @@ -738,7 +727,6 @@ RSpec.describe Spree::Ability do context 'enterprise manager' do let(:user) do user = create(:user) - user.spree_roles = [] s1.enterprise_roles.build(user:).save user end @@ -797,7 +785,7 @@ RSpec.describe Spree::Ability do let(:manage_actions) { [:admin, :index, :read, :update, :bulk_update, :bulk_reset] } describe "when admin" do - before { user.spree_roles << Spree::Role.find_or_create_by!(name: 'admin') } + before { user.update!(admin: true) } it "should have permission" do is_expected.to have_ability(manage_actions, for: variant_override) diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 26fef88569..a4cf3d06bf 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -512,7 +512,6 @@ module Spree it "shows only products for given user" do user = create(:user) - user.spree_roles = [] e1.enterprise_roles.build(user:).save product = Product.managed_by user diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb index a2a0ab3897..ea9531f707 100644 --- a/spec/models/spree/user_spec.rb +++ b/spec/models/spree/user_spec.rb @@ -209,11 +209,11 @@ RSpec.describe Spree::User do end describe '#admin?' do - it 'returns true when the user has an admin spree role' do + it 'returns true when the user has an admin role' do expect(create(:admin_user).admin?).to be_truthy end - it 'returns false when the user does not have an admin spree role' do + it 'returns false when the user does not have an admin role' do expect(create(:user).admin?).to eq(false) end end diff --git a/spec/support/controller_helper.rb b/spec/support/controller_helper.rb index c7cc66b6e2..f2f9b61bed 100644 --- a/spec/support/controller_helper.rb +++ b/spec/support/controller_helper.rb @@ -3,11 +3,7 @@ module OpenFoodNetwork module ControllerHelper def controller_login_as_admin - @admin_user ||= begin - user = create(:user) - user.spree_roles << Spree::Role.find_or_create_by!(name: 'admin') - user - end + @admin_user ||= create(:admin_user) allow(controller).to receive_messages(spree_current_user: @admin_user) end @@ -15,7 +11,6 @@ module OpenFoodNetwork def controller_login_as_enterprise_user(enterprises) @enterprise_user ||= begin user = create(:user) - user.spree_roles = [] enterprises.each do |enterprise| enterprise.enterprise_roles.create!(user:) end diff --git a/spec/system/admin/configuration/shipping_categories_spec.rb b/spec/system/admin/configuration/shipping_categories_spec.rb index 7e884d0aeb..ea2fc73142 100644 --- a/spec/system/admin/configuration/shipping_categories_spec.rb +++ b/spec/system/admin/configuration/shipping_categories_spec.rb @@ -5,7 +5,6 @@ require 'system_helper' RSpec.describe "Shipping Categories" do include AuthenticationHelper include WebHelper - let(:admin_role) { Spree::Role.find_or_create_by!(name: 'admin') } let(:admin_user) { create(:user) } context 'user visits shipping categories page' do diff --git a/spec/system/admin/multilingual_spec.rb b/spec/system/admin/multilingual_spec.rb index e8de421d75..0f9f0900bd 100644 --- a/spec/system/admin/multilingual_spec.rb +++ b/spec/system/admin/multilingual_spec.rb @@ -5,11 +5,9 @@ require 'system_helper' RSpec.describe 'Multilingual' do include AuthenticationHelper include WebHelper - let(:admin_role) { Spree::Role.find_or_create_by!(name: 'admin') } - let(:admin_user) { create(:user) } + let(:admin_user) { create(:admin_user) } before do - admin_user.spree_roles << admin_role login_as admin_user visit spree.admin_dashboard_path end From ea9a5c8dd538a214927d90bb2d87d5ee06d0c26e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 22 Jan 2025 14:45:19 +1100 Subject: [PATCH 4/6] Remove class Spree::Role --- app/models/spree/role.rb | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 app/models/spree/role.rb diff --git a/app/models/spree/role.rb b/app/models/spree/role.rb deleted file mode 100644 index 9e34c4d8d2..0000000000 --- a/app/models/spree/role.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -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 From bb040812aa35dc255d7e541d6a426a5af78954aa Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 24 Jan 2025 14:49:18 +1100 Subject: [PATCH 5/6] Refactor spec --- .../spree/admin/users_controller_spec.rb | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/spec/controllers/spree/admin/users_controller_spec.rb b/spec/controllers/spree/admin/users_controller_spec.rb index ddcaf3e0df..f040604cb6 100644 --- a/spec/controllers/spree/admin/users_controller_spec.rb +++ b/spec/controllers/spree/admin/users_controller_spec.rb @@ -3,36 +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) end - it 'should grant access to users with an admin role' do - user.update!(admin: true) - 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.update!(admin: true) - 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 "re-renders the edit form if error" do - user.update!(admin: true) - spree_put :update, id: test_user.id, user: { password: "blah", password_confirmation: "" } + it 'should grant access to users with an admin role' do + spree_post :index + expect(response).to render_template :index + end - expect(response).to render_template :edit + 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 + spree_put :update, id: test_user.id, user: { password: "blah", password_confirmation: "" } + + 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 From f6bd1c49eefc77ec26a5ddd3369767a36546d43c Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 24 Jan 2025 14:52:54 +1100 Subject: [PATCH 6/6] Apply suggestions from code review --- spec/models/spree/ability_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 24b1adae26..1339a39328 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -34,6 +34,8 @@ RSpec.describe Spree::Ability do let(:fakedispatch_ability) { Spree::Ability.new(fakedispatch_user) } context 'with admin user' do + let(:user) { create(:admin_user) } + it 'should be able to admin' do user.update!(admin: true) expect(subject).to be_able_to :admin, resource @@ -785,7 +787,7 @@ RSpec.describe Spree::Ability do let(:manage_actions) { [:admin, :index, :read, :update, :bulk_update, :bulk_reset] } describe "when admin" do - before { user.update!(admin: true) } + let(:user) { create(:admin_user) } it "should have permission" do is_expected.to have_ability(manage_actions, for: variant_override)