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

ApiSecret model for Developer API #1369

Merged
merged 23 commits into from Jan 8, 2019
Merged

ApiSecret model for Developer API #1369

merged 23 commits into from Jan 8, 2019

Conversation

timorthi
Copy link
Contributor

@timorthi timorthi commented Dec 19, 2018

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

This PR lays down the foundation for a developer API feature. The expected outcome of this PR is to:

  • Have an ApiSecret model that contains metadata about a secret (i.e. the secret itself, rate limit, etc.)
  • Allow Users to have many ApiSecrets, and Organizations to have many ApiSecrets through Users
  • Allow users to generate and destroy tokens on demand via the user settings page

Related Tickets & Documents

#911

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

View when user has no access tokens:
no-tokens

After creating a token, the user will be flashed a message containing the secret. After that, secret is no longer shown anywhere:
created-1
created-2

The user can destroy/revoke the token and will be flashed a message after successful deletion:
revoked

Model validation errors on create:
screen shot 2018-12-25 at 1 35 24 pm

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

@timorthi timorthi mentioned this pull request Dec 19, 2018
@benhalpern
Copy link
Contributor

Looks great so far

@timorthi
Copy link
Contributor Author

timorthi commented Dec 23, 2018

@benhalpern I pushed a quick prototype (i.e. incomplete, untested code 😅) with a lot of assumptions from my side - let me know what kind of changes you'd like to make!

@timorthi timorthi changed the title [WIP] API Secret model for Developer API [WIP] ApiSecret model for Developer API Dec 23, 2018
@benhalpern
Copy link
Contributor

So far so good from what I see.

@timorthi timorthi changed the title [WIP] ApiSecret model for Developer API ApiSecret model for Developer API Dec 25, 2018
@rhymes
Copy link
Contributor

rhymes commented Dec 25, 2018

Wow @timorthi this looks great! Just a question, why limit the description to 30 chars?

@timorthi
Copy link
Contributor Author

timorthi commented Dec 25, 2018

@rhymes Thanks! I don't have a good reason for that, actually 😅 I'll remove it in a bit!

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Just one styling issue. Looks great otherwise!

app/assets/stylesheets/settings.scss Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Dec 27, 2018
@benhalpern
Copy link
Contributor

I agree with @Zhao-Andy's proposal and this should be mergeable at that point.

@benhalpern benhalpern self-assigned this Dec 27, 2018
@maestromac
Copy link
Member

This is great work @timorthi !

A slight UI Nit: When I was playing around with it, I didn't notice that the successful flash message included the API secret because I was looking for the secret somewhere in the middle of the screen and not as part of the top bar alert.

We don't have to worry about that just yet though. This first iteration is a job well done!

Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

👍

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Dec 27, 2018
Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

LGTM :)

<% if @user.api_secrets.empty? %>
<p>None yet!</p>
<% end %>
<% @user.api_secrets.order(created_at: :desc).each do |api_secret| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. What do you think of moving this query to the controller?

This way, the view doesn't have to know anything about the ordering logic.

t.timestamps
end
add_index :api_secrets, :secret, unique: true
add_index :api_secrets, :user_id
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a foreign key to the user_id column?

end

def destroy?
user_is_owner?
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. What do you think of renaming this method to is_owner? or even owner??

@benhalpern benhalpern merged commit 84bd3a6 into forem:master Jan 8, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 8, 2019
@timorthi timorthi deleted the 911-dev-api-keys branch January 8, 2019 17:35
@benhalpern benhalpern removed their assignment Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants