-
Notifications
You must be signed in to change notification settings - Fork 137
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
Improve README #114
Improve README #114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few things in here to revisit
README.md
Outdated
require 'auth0' | ||
|
||
class AllUsersController < ApplicationController | ||
before_action :set_api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation should be 2 spaces in. This looks like it may be a tab. This may have happened when I copied the example to you from a shoddy copy out of my terminal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason you didn't go with the memoized API we chatted about being idiomatic Rails?
class AllUsersController < ApplicationController
# Get all users from Auth0 with "auth0" in their email.
def index
@params = {
q: "email:*auth0*",
fields: 'email,user_id,name',
include_fields: true,
page: 0,
per_page: 50
}
@users = auth0.users @params
end
private
# Setup the Auth0 API connection.
def auth0
@auth0 ||= Auth0Client.new(
client_id: ENV['AUTH0_RUBY_CLIENT_ID'],
token: ENV['AUTH0_RUBY_API_TOKEN'],
domain: ENV['AUTH0_RUBY_DOMAIN'],
api_version: 2,
timeout: 15 # optional, defaults to 10s
)
end
end
This will allow a user to reference auth0
method as many times as they want without having to establish a new connection or concern themselves with setting it up in a before_action
.
README.md
Outdated
@@ -137,7 +137,7 @@ Auth0 helps you to: | |||
|
|||
## Issue Reporting | |||
|
|||
If you have found a bug or if you have a feature request, please report them at this repository issues section. Please do not report security vulnerabilities on the public GitHub issue tracker. The [Responsible Disclosure Program](https://auth0.com/whitehat) details the procedure for disclosing security issues. | |||
If you find a bug or have a feature request, please report them in this repository's Issues tab. Please do not report security vulnerabilities on the public GitHub issue tracker. The [Responsible Disclosure Program](https://auth0.com/whitehat) details the procedure for disclosing security issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider linking to Issues tab.
a59ba64
to
09e63fe
Compare
RE: Whitespace ... I'm using a MD editor and RE: Memoization ... I see what you're doing there now. I think I was still doing memoization but using a RE: Issues tab ... added! |
README.md
Outdated
|
||
private | ||
|
||
# before_action: Setup the Auth0 API connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove before_action
portion of this since it's inaccurate.
README.md
Outdated
require 'auth0' | ||
|
||
class AllUsersController < ApplicationController | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can drop this whitespace
README.md
Outdated
``` | ||
|
||
## API Documentation | ||
Note the `auth0_client` method that returns the API client. If API access is required across multiple controllers, this would be best used in a helper function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A helper function would need to be declared at the base class level I believe in order to be shared to the view. This function would need declared at a base class level to be shared across controllers. We may not want to provide this suggestion as helper functions are a bit controversial in general.
09e63fe
to
e6ff82e
Compare
@machuga - Fixed up! Thanks for your patience with this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes #102