Add ability to define roles where auto-api-key create is desired. #480
Conversation
|
This is a good change. Just a few thoughts I'm not sure Should (or could) it be possible to use this preference to generate an API key for all users, regardless of role? iirc some stores do that |
@jhawthorn agreed on the naming. any alternatives come to mind?
seems reasonable to me. |
|
@jhawthorn changed the name to Also added in |
|
👍 |
| # @!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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
@jhawthorn @athal7 moved the generate_api_call to the user model |
| 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! } | ||
| context "with un-desired role" do |
There was a problem hiding this comment.
is this supposed to be inside the context "roles_for_auto_api_key is defined" ?
|
@athal7 updated to |
|
👍 |
|
@jhawthorn mind taking another look when you get the chance? 🌾 🍘 🍚 |
| @@ -34,6 +36,14 @@ def has_spree_role?(role_in_question) | |||
| spree_roles.any? { |role| role.name == role_in_question.to_s } | |||
| end | |||
|
|
|||
| def generate_api_key(role: nil) | |||
There was a problem hiding this comment.
I think the naming here is a little ambiguous, I don't think a developer would be able to infer that user.generate_spree_api_key always generates an api key, but user.generate_api_key only generates one if the user is eligible. Maybe a name like auto_generate_spree_api_key or similar would clear that up.
I'd also love if this looked through the roles rather than needing the role to be passed in, just to keep the api as simple as possible. We have the knowledge in this object of what roles we belong to so we might as well use that. Maybe something like (spree_roles.map(&:name) & Spree::Config.roles_for_auto_api_key).any?.
Other than those two comments this is awesome 👍
There was a problem hiding this comment.
@jhawthorn that (spree_roles.map(&:name) & Spree::Config.roles_for_auto_api_key).any? is so great. Thanks for that. Agreed on the name. Making the changes now.
* Right now we only generate an api key for a new RoleUser's user when a user has the admin role. It is desired to be able to define other roles so that we can generate an api key for a new RoleUser's user if it doesn't exist and the role is defined in whitelisted_roles_for_api_key
|
@jhawthorn and @athal7 made changes to reflect hawthorn's comments in regards to the naming of the |
|
Awesome. Looks good to me 👍 |
|
👍 |
Add ability to define roles where auto-api-key create is desired.
has the admin role.
roles_for_auto_api_keyroles_for_auto_api_keydefaults to ['admin'] so that current behavior will be maintained.generate_api_key_for_all_roleswhich defaults tofalse. if desired behavior is for all roles to auto generate an api key, thengenerate_api_key_for_all_rolescan be set totrue.