GitHubログインまで #2

Merged
merged 5 commits into from Mar 28, 2013

Conversation

Projects
None yet
5 participants
@shu0115
Owner

shu0115 commented Mar 28, 2013

No description provided.

+ # セッション/トップコントローラ以外で
+ if params[:controller] != "sessions" and params[:controller] != "top"
+ # 未ログインであれば
+ if current_user.blank?

This comment has been minimized.

Show comment Hide comment
@tbaba

tbaba Mar 28, 2013

user_signed_in?的なメソッドを作っておくと便利なんじゃないかなぁと思いました。

@tbaba

tbaba Mar 28, 2013

user_signed_in?的なメソッドを作っておくと便利なんじゃないかなぁと思いました。

This comment has been minimized.

Show comment Hide comment
@shu0115

shu0115 Mar 28, 2013

Owner

そうですね。
毎回current_userチェックするのもあんまりスマートじゃないですね。
作ってみますー。

@shu0115

shu0115 Mar 28, 2013

Owner

そうですね。
毎回current_userチェックするのもあんまりスマートじゃないですね。
作ってみますー。

config/application.rb
@@ -7,6 +7,12 @@
module P4d
class Application < Rails::Application
+ # For Heroku
+ config.assets.initialize_on_precompile = false

This comment has been minimized.

Show comment Hide comment
@tkawa

tkawa Mar 28, 2013

Owner

Rails 4 ではなくなったらしいです。

https://devcenter.heroku.com/articles/rails-asset-pipeline

In Rails 4.x this option has been removed and is no longer needed.

@tkawa

tkawa Mar 28, 2013

Owner

Rails 4 ではなくなったらしいです。

https://devcenter.heroku.com/articles/rails-asset-pipeline

In Rails 4.x this option has been removed and is no longer needed.

This comment has been minimized.

Show comment Hide comment
@shu0115

shu0115 Mar 28, 2013

Owner

おお、ありがとうございます。
やっとこれ書かなくて済むようになったんですね。。

@shu0115

shu0115 Mar 28, 2013

Owner

おお、ありがとうございます。
やっとこれ書かなくて済むようになったんですね。。

@@ -2,4 +2,39 @@ class ApplicationController < ActionController::Base
# Prevent CSRF attacks by raising an exception.
# For APIs, you may want to use :null_session instead.
protect_from_forgery with: :exception
+ before_filter :authorize # ログイン認証

This comment has been minimized.

Show comment Hide comment
@ppworks

ppworks Mar 28, 2013

Owner

認証 なら authenticate
authorize なら 認可 ですね。

このケースだと、認可というコメントが正しいと思います。

@ppworks

ppworks Mar 28, 2013

Owner

認証 なら authenticate
authorize なら 認可 ですね。

このケースだと、認可というコメントが正しいと思います。

This comment has been minimized.

Show comment Hide comment
@shu0115

shu0115 Mar 28, 2013

Owner

やってることが認証なのでメソッド名の方をauthenticateに修正しました。

@shu0115

shu0115 Mar 28, 2013

Owner

やってることが認証なのでメソッド名の方をauthenticateに修正しました。

+ if params[:controller] != "sessions" and params[:controller] != "top"
+ unless user_signed_in?
+ # リクエストURL保管
+ session[:request_url] = request.url

This comment has been minimized.

Show comment Hide comment
@ppworks

ppworks Mar 28, 2013

Owner

GET以外も保存して平気ですか?

@ppworks

ppworks Mar 28, 2013

Owner

GET以外も保存して平気ですか?

This comment has been minimized.

Show comment Hide comment
@shu0115

shu0115 Mar 28, 2013

Owner

未ログインで表示される画面が今はトップしか無いので、とりあえず今のところは制限掛けなくても良いかなと思ってます。
(pull requestしてもらっても良いですが。)

@shu0115

shu0115 Mar 28, 2013

Owner

未ログインで表示される画面が今はトップしか無いので、とりあえず今のところは制限掛けなくても良いかなと思ってます。
(pull requestしてもらっても良いですが。)

+ # ログイン認証
+ def authorize
+ # セッション/トップコントローラ以外で
+ if params[:controller] != "sessions" and params[:controller] != "top"

This comment has been minimized.

Show comment Hide comment
@ppworks

ppworks Mar 28, 2013

Owner

sessions, top コントローラーで
skip_filter: authorize するのとどっちが分かりやすいかみたいなところはありますねー。

@ppworks

ppworks Mar 28, 2013

Owner

sessions, top コントローラーで
skip_filter: authorize するのとどっちが分かりやすいかみたいなところはありますねー。

This comment has been minimized.

Show comment Hide comment
@shu0115

shu0115 Mar 28, 2013

Owner

paramsの値で分けるのはちょっと暫定的な対処な感じがするので、skip_before_filterに変えました。

@shu0115

shu0115 Mar 28, 2013

Owner

paramsの値で分けるのはちょっと暫定的な対処な感じがするので、skip_before_filterに変えました。

+ private
+
+ # ログイン認証
+ def authorize

This comment has been minimized.

Show comment Hide comment
@ppworks

ppworks Mar 28, 2013

Owner

current_user, user_signed_in?
user に特化している感じするので、 user_authenticate にしておいたほうが統一感あるかも!?

@ppworks

ppworks Mar 28, 2013

Owner

current_user, user_signed_in?
user に特化している感じするので、 user_authenticate にしておいたほうが統一感あるかも!?

This comment has been minimized.

Show comment Hide comment
@shu0115

shu0115 Mar 28, 2013

Owner

これはauthenticatesigned_inする対象がそもそもユーザ以外を対象とする事はあまり無いだろう、と思うので、user_signed_in?の方をsigned_in?に変更しました。

@shu0115

shu0115 Mar 28, 2013

Owner

これはauthenticatesigned_inする対象がそもそもユーザ以外を対象とする事はあまり無いだろう、と思うので、user_signed_in?の方をsigned_in?に変更しました。

This comment has been minimized.

Show comment Hide comment
@ppworks

ppworks Mar 28, 2013

Owner

いいですね!👌

@ppworks

ppworks Mar 28, 2013

Owner

いいですね!👌

@ppworks

This comment has been minimized.

Show comment Hide comment
@ppworks

ppworks Mar 28, 2013

Owner

ナイス🍜
リファクタリング

mergeして良い感じします。

Owner

ppworks commented Mar 28, 2013

ナイス🍜
リファクタリング

mergeして良い感じします。

@satococoa

This comment has been minimized.

Show comment Hide comment
@satococoa

satococoa Mar 28, 2013

Owner

ありがとう

Owner

satococoa commented Mar 28, 2013

ありがとう

satococoa added a commit that referenced this pull request Mar 28, 2013

@satococoa satococoa merged commit 921bf60 into master Mar 28, 2013

@satococoa satococoa deleted the omniauth_login branch Mar 28, 2013

@satococoa satococoa referenced this pull request Mar 28, 2013

Closed

github ログイン #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment