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.

Sign up for GitHub

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

Jump to bottom

Improve API key on user edit page #2243

Merged
merged 4 commits into from
Oct 4, 2017
Merged

Improve API key on user edit page #2243

merged 4 commits into from
Oct 4, 2017

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Sep 29, 2017 •
edited
Loading

This improves a few aspects of how API keys are handled and displayed on the user edit page.

  • I18n translations for this page moved out of the API gem, and keys named relative to the view.
  • Confirmation dialogs added for clearing or regenerating the key.
  • API key is displayed for admins viewing their own user page
  • Fixed changed bad heading translation ("Stores -> User")
  • Embolden "Key:" and italicize "(hidden)"

@jhawthorn jhawthorn requested a review from tvdeyen September 29, 2017 21:47
@jhawthorn jhawthorn force-pushed the user_edit_page branch 2 times, most recently from 7d7d311 to 8453607 Compare September 29, 2017 21:53
@jhawthorn
Use view-relative translations for users edit page
3be93f6 
Previously translations for this page were stored in the solidus_api
gem.
@jhawthorn
Add confirmation to generate or clear api_keys
eaff27b 
These are distructive operations and we don't want admins to
accidentally misclick these.
@jhawthorn
Allow users to see their own api_key
c25fa5b 
This solves an occasional onboarding usability complaint: that it's hard
to find your own API key. Previously you could only find it by viewing
the page source or using the rails console, but it makes sense to
display it here.

API keys for other users remain hidden, regardless of your permissions.
@jhawthorn
Fix bad translation on users edit page
a297480
tvdeyen
tvdeyen reviewed Sep 29, 2017
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I love that, but we could allow stores to change the permission if we had one ;)

backend/app/views/spree/admin/users/edit.html.erb Outdated
<div id="current-api-key"><%= t('.key') %>: (<%= Spree.t('.hidden') %>)</div>
<div id="current-api-key">
<strong><%= t('.key') %>: </strong>
<% if @user == try_spree_current_user %>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe for easier customisation we should add a

can? :reveal_api_key, @user

permission?

@tvdeyen
Copy link
Member

tvdeyen commented Sep 29, 2017

Somehow my comment is on an outdated commit. Copy that here:

Maybe for easier customisation we should add a

can? :reveal_api_key, @user

permission?

@jhawthorn
Copy link
Contributor Author

@tvdeyen

Maybe for easier customisation we should add a

can? :reveal_api_key, @user
permission?

Because our default "admin" role is assigned SuperUser permissions they will be able to view all api_keys, a bigger change than I intended. Are we okay with that?

@tvdeyen
Copy link
Member

tvdeyen commented Oct 2, 2017

Because our default "admin" role is assigned SuperUser permissions they will be able to view all api_keys, a bigger change than I intended. Are we okay with that?

Revealing should only be possible for the owner. But I’m fine with Super Admins being able to reset the key. WDYT?

@jhawthorn
Copy link
Contributor Author

Revealing should only be possible for the owner. But I’m fine with Super Admins being able to reset the key. WDYT?

With the SuperUser permission, that isn't possible. We give the "admin" role the :manage permission, which allows all actions. https://github.com/solidusio/solidus/blob/master/core/lib/spree/permission_sets/super_user.rb#L5

@tvdeyen
Copy link
Member

tvdeyen commented Oct 4, 2017

With the SuperUser permission, that isn't possible. We give the "admin" role the :manage permission, which allows all actions.

Right, the super user thing... Something we should think about changing in the future.

Ok, for now we should do what you are proposing in this PR and compare the ownership right inside the view for the revealing. For clearing and resetting I think we could use a permission.

kennyadsl
kennyadsl approved these changes Oct 4, 2017
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

All good for me, just a non-blocking comment: maybe we should use another partial for api key management so that it's easier for stores to change its logic if needed.

gmacdougall
gmacdougall approved these changes Oct 4, 2017
View reviewed changes
Copy link
Member

@gmacdougall gmacdougall left a comment

Choose a reason for hiding this comment

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

👍

@gmacdougall gmacdougall merged commit 4efd9a1 into solidusio:master Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvdeyen tvdeyen tvdeyen left review comments

@gmacdougall gmacdougall gmacdougall approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jhawthorn @tvdeyen @kennyadsl @gmacdougall