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

Add ability to define roles where auto-api-key create is desired. #480

Merged
merged 1 commit into from
Dec 2, 2015
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
76 changes: 61 additions & 15 deletions api/spec/models/spree/legacy_user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,29 +26,75 @@ module Spree
expect { user.clear_spree_api_key }.to change(user, :spree_api_key).to be_blank
end

context "admin role auto-api-key grant" do # so the admin user can do admin api actions
let(:user) { create(:user) }
before { expect(user.spree_roles).to be_blank }
subject { user.spree_roles << role }
context "auto-api-key grant" do
context "after role user create" do
let(:user) { create(:user) }
before { expect(user.spree_roles).to be_blank }
subject { user.spree_roles << role }

context "admin role" do
let(:role) { create(:role, name: "admin") }
context "roles_for_auto_api_key default" do
let(:role) { create(:role, name: "admin") }

context "the user has no api key" do
before { user.clear_spree_api_key! }
it { expect { subject }.to change { user.reload.spree_api_key }.from(nil) }
end

context "the user already has an api key" do
before { user.generate_spree_api_key! }
it { expect { subject }.not_to change { user.reload.spree_api_key } }
end
end

context "roles_for_auto_api_key is defined" do
let (:role) { create(:role, name: 'hobbit') }
let(:undesired_role) { create(:role, name: "foo") }

before {
user.clear_spree_api_key!
Spree::Config.roles_for_auto_api_key = ['hobbit']
}

context "the user has no api key" do
before { user.clear_spree_api_key! }
it { expect { subject }.to change { user.reload.spree_api_key }.from(nil) }
it { expect { user.spree_roles << undesired_role }.not_to change { user.reload.spree_api_key } }
end

context "the user already has an api key" do
before { user.generate_spree_api_key! }
it { expect { subject }.not_to change { user.reload.spree_api_key } }
context "for all roles" do
let (:role) { create(:role, name: 'hobbit') }
let (:other_role) { create(:role, name: 'wizard') }
let (:other_user) { create(:user) }

before {
user.clear_spree_api_key!
other_user.clear_spree_api_key!
Spree::Config.generate_api_key_for_all_roles = true
}

it { expect { subject }.to change { user.reload.spree_api_key }.from(nil) }
it { expect { other_user.spree_roles << other_role }.to change { other_user.reload.spree_api_key }.from(nil) }
end
end

context "non-admin role" do
let(:role) { create(:role, name: "foo") }
before { user.clear_spree_api_key! }
it { expect { subject }.not_to change { user.reload.spree_api_key } }
context "after user create" do
let(:user) { LegacyUser.new }

context "generate_api_key_for_all_roles" do
it "does not grant api key default" do
expect(user.spree_api_key).to eq(nil)

user.save!
expect(user.spree_api_key).to eq(nil)
end

it "grants an api key on create when set to true" do
Spree::Config.generate_api_key_for_all_roles = true

expect(user.spree_api_key).to eq(nil)

user.save!
expect(user.spree_api_key).not_to eq(nil)
end
end
end
end
end
Expand Down
10 changes: 10 additions & 0 deletions core/app/models/concerns/spree/user_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ module UserMethods
has_many :store_credit_events, through: :store_credits
money_methods :total_available_store_credit

after_create :auto_generate_spree_api_key

include Spree::RansackableAttributes unless included_modules.include?(Spree::RansackableAttributes)

self.whitelisted_ransackable_associations = %w[addresses]
Expand All @@ -34,6 +36,14 @@ def has_spree_role?(role_in_question)
spree_roles.any? { |role| role.name == role_in_question.to_s }
end

def auto_generate_spree_api_key
return if !respond_to?(:spree_api_key) || spree_api_key.present?

if Spree::Config.generate_api_key_for_all_roles || (spree_roles.map(&:name) & Spree::Config.roles_for_auto_api_key).any?
generate_spree_api_key!
end
end

# @return [Spree::Order] the most-recently-created incomplete order
# since the customer's last complete order.
def last_incomplete_spree_order(store: nil, only_frontend_viewable: true)
Expand Down
11 changes: 11 additions & 0 deletions core/app/models/spree/app_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ class AppConfiguration < Preferences::Configuration
# charged (default: +14+)
preference :expedited_exchanges_days_window, :integer, default: 14

# @!attribute [rw] generate_api_key_for_all_roles
# @return [Boolean] Allow generating api key automatically for user
# at role_user creation for all roles. (default: +false+)
preference :generate_api_key_for_all_roles, :boolean, default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I (sort of) asked for this, but seeing the implementation I don't think this makes sense. Most users signing up to a store will have no role at all. This option will grant an api key to any user with any role (but they must have a role), which is probably not as useful and I think the distinction will be confusing.

I would still (eventually) like to see something for granting all users an API key, but I'd be 👍 just removing this option

Copy link
Contributor

Choose a reason for hiding this comment

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

We could move the auto-generation to the user model and then get the benefit from this preference. That would give the solution you desire.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be great


# @!attribute [rw] layout
# @return [String] template to use for layout on the frontend (default: +"spree/layouts/spree_application"+)
preference :layout, :string, default: 'spree/layouts/spree_application'
Expand Down Expand Up @@ -193,6 +198,12 @@ class AppConfiguration < Preferences::Configuration
# @return [Integer] default: 365
preference :return_eligibility_number_of_days, :integer, default: 365

# @!attribute [rw] roles_for_auto_api_key
# @return [Array] An array of roles where generating an api key for a user
# at role_user creation is desired when user has one of these roles.
# (default: +['admin']+)
preference :roles_for_auto_api_key, :array, default: ['admin']

# @!attribute [rw] shipping_instructions
# @return [Boolean] Request instructions/info for shipping (default: +false+)
preference :shipping_instructions, :boolean, default: false
Expand Down
8 changes: 3 additions & 5 deletions core/app/models/spree/role_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@ class RoleUser < Spree::Base
belongs_to :role, class_name: "Spree::Role"
belongs_to :user, class_name: Spree::UserClassHandle.new

after_save :generate_admin_api_key
after_create :auto_generate_spree_api_key

private

def generate_admin_api_key
if role.admin? && user.respond_to?(:spree_api_key) && !user.spree_api_key
user.generate_spree_api_key!
end
def auto_generate_spree_api_key
user.try!(:auto_generate_spree_api_key)
end
end
end