kyoshida-aim / RailsTraining

Ruby on Rails研修のPR作成・進捗管理用
https://ky-railstraining-2019.herokuapp.com/
1 stars 0 forks source link

ステップ22: ユーザの管理画面を実装しよう #36

Closed kyoshida-aim closed 5 years ago

kyoshida-aim commented 5 years ago

概要

理由

確認方法

ユーザー一覧 image

ユーザー登録 image

ユーザー詳細 image

やっていないこと

相談事項

vividmuimui commented 5 years ago

このリポジトリでは ci skipつけなくてよいと思います。1人しか使わないので。 チームのリポジトリの場合、CIのキューがつまるとかあるから考慮が必要かもですが。

vividmuimui commented 5 years ago

https://github.com/kyoshida-aim/RailsTraining/pull/36/commits/d9477d16baf96b025c75ba1eb46b36af03f30cd7 このコミット、 if: :password以外にも変更入ってますが、変更内容って意図的ややつですか?

vividmuimui commented 5 years ago

ほかはLGTM

kyoshida-aim commented 5 years ago

d9477d1 このコミット、 if: :password以外にも変更入ってますが、変更内容って意図的ややつですか?

https://github.com/kyoshida-aim/RailsTraining/blob/56d1554b3b8808626b1b69f26f2d071d42bed20c/app/controllers/admin/users_controller.rb#L34

ここの行以外は、パスワードのバリデーションを見直している際に意図的に修正したものになります。 (そもそもバリデーションに引っかかっていた原因が違うので変更しなくても動作してたかも)

↑の行に関しては単純にrender先を間違えていたことに修正中に気づいて修正したものです こっちはコミット分けるべきでしたかね...

vividmuimui commented 5 years ago

ユーザー一覧から管理権限を操作するときにパスワードのバリデーションに引っかかっていたのを修正ッテコミットメッセージのコミットの中に、それ以外の修正が入ってると、意図的なのか間違って混ざってしまったのかわからないので、確認した感じです コミット分けてる or コミットメッセージをいい感じにしておけば良いと思います。 けど、今回はもう直さずマージしちゃって良さそう 👍