add github login #101
Open
add github login #101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tony612
requested changes
Apr 4, 2017
lib/elixir_china/github.ex
Outdated
| end | ||
|
|
||
| def authorize_url! do | ||
| OAuth2.Client.authorize_url!(client(), scope: "user,public_repo") |
There was a problem hiding this comment.
这里的权限要的太多了吧？ 我试了一下，按你现在写的代码，应该用 user:email 就行了。 https://developer.github.com/v3/oauth/#scopes
lib/elixir_china/github.ex
Outdated
| OAuth2.Client.authorize_url!(client(), scope: "user,public_repo") | ||
| end | ||
|
|
||
| def get_token!(params \\ [], headers \\ [], opts \\ []) do |
There was a problem hiding this comment.
不太建议这里用这么多可选参数，使用起来选择太多，发而不太好用。而且你代码里其实也基本没用到什么默认参数的功能。可以把常用的参数作为必须的，比如 params。剩下的再考虑是可选吧
web/controllers/auth_controller.ex
Outdated
| email && (user = Repo.get_by(User, email: email)) -> | ||
| {:ok, user} | ||
| email && name && Repo.get_by(User, name: name) -> | ||
| {:ok, insert_user(email, generate_name(name))} |
There was a problem hiding this comment.
这里除了你这种做法（先根据 name 查 user），我想到还有一种做法是先直接插入数据，因为 name 有唯一索引，如果插入失败再生成 name 来创建，这样如果用户名不存在的话，就减少了查询。你觉得哪种好点？
web/templates/layout/app.html.eex
Outdated
| @@ -39,6 +39,9 @@ | |||
| <%= render @view_module, @view_template, assigns %> | |||
| </div> | |||
| <sidebar class="col-md-3" role="sidebar"> | |||
| <%= if @user_id == nil do %> | |||
| <%= render ElixirChina.SharedView, "_social_login.html", conn: @conn %> | |||
There was a problem hiding this comment.
我更倾向于把这个页面放在登录那里，不然到处都有这个登录按钮
There was a problem hiding this comment.
额，确实有点简单粗暴了，还是放在登录界面里面好一点。
tony612
reviewed
Apr 4, 2017
web/controllers/auth_controller.ex
Outdated
| with {:ok, user} <- insert_user(email, name) do | ||
| {:ok, user} | ||
| else | ||
| {:error, %Ecto.Changeset{errors: [email: {"has already been taken", []}]}} -> |
There was a problem hiding this comment.
这里我倒觉得可以直接先按 email 来查，毕竟插入的操作不需要做很多，用户通过 GitHub 登录一次后，以后都可以通过 email 找到。
DefiPanda
force-pushed
the
master
branch
8 times, most recently
from
3b8344d to
4678982
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.