管理者は自分以外のユーザーのGitHubアカウント連携を解除できるように修正 #7819
Conversation
|
@hirano-vm4 |
Shrimprin
changed the title
|
お疲れ様です！レビュー承知しました。少しお時間いただければと思います。来週中に確認してコメントいれますね🙌 |
|
@hirano-vm4 |
There was a problem hiding this comment.
すいません！バタバタしており、大変遅くなりました🙏
実際の挙動・テスト・コードそれぞれ確認させてもらいLGTMだと思いました〜！
1点だけ概要の部分だけ以下コメントさせてもらいました🙌軽微で、かつ個人的な感想の部分なのでApproveもしております😄
変更確認方法について
丁寧に書かれていてわかりやすかったです！
5番のリンク先からは編集画面にすぐに移動できなかったので、直接http://localhost:3000/admin/users/169446854/edit を記載したらもっとスムーズかな？と感じました〜
例
|
@hirano-vm4 変更確認方法についても大変参考になりました！ |
|
@komagata |
| if !admin_login? && user != current_user | ||
| redirect_to root_path, alert: '管理者としてログインしてください' | ||
| return | ||
| end | ||
|
|
||
| user.update(github_id: nil) | ||
| redirect_to user_path(user), notice: 'GitHubとの連携を解除しました。' |
There was a problem hiding this comment.
ガード節以外ではreturnを使わず下記のような感じの方が読みやすいのではないかと思いました。
| if !admin_login? && user != current_user | |
| redirect_to root_path, alert: '管理者としてログインしてください' | |
| return | |
| end | |
| user.update(github_id: nil) | |
| redirect_to user_path(user), notice: 'GitHubとの連携を解除しました。' | |
| if !admin_login? && user != current_user | |
| redirect_to root_path, alert: '管理者としてログインしてください' | |
| else | |
| user.update(github_id: nil) | |
| redirect_to user_path(user), notice: 'GitHubとの連携を解除しました。' | |
| end |
There was a problem hiding this comment.
コメントありがとうございます。
ご提案いただいたコードの方が読みやすいと感じましたので修正しました。
| @@ -33,4 +33,24 @@ class Authentication::GithubSystemTest < ApplicationSystemTestCase | |||
| visit '/current_user/edit' | |||
| assert_link 'GitHub アカウントを登録する' | |||
| end | |||
|
|
|||
| test 'release other user GitHub account' do | |||
There was a problem hiding this comment.
fixtureは全てのtest caseでinsertされるので一つのtest caseでだけ必要な場合はこの行あたりでデータを作るとsystem test全体が遅くならないのでよいかもです。
There was a problem hiding this comment.
machidaさんと相談させていただき、実機ブラウザでの確認用も兼ねてfixtureで作成しました。
どのように対応すべきかご助言いただけますでしょうか。
- 今回作成したfixtureは現状のまま
- 今回作成したfixtureを削除し、確認用のデータは
rails cなどで手動で作成する - 今回作成したfixtureを削除し、適当な既存のfixtureの
github_idに値を設定して、そのfuxtureを実機確認用に使う
There was a problem hiding this comment.
Shrimprin
force-pushed
the
feature/administrator-release-users-github
branch
from
a0b48c2 to
6cc8fa2
Compare
|
@komagata |
|
@Shrimprin マージコミットがたくさん入っているようなので修正してみてください～ |
- テストが重くなるため、動作確認用のfixtureを削除し、既存のfixtureにgithub_idを追加
Shrimprin
force-pushed
the
feature/administrator-release-users-github
branch
from
6cc8fa2 to
8fd7037
Compare
|
@komagata |
|
@komagata |
Issue
概要
管理者は自分以外のユーザーのGitHubアカウントの連携を解除できるようにしました。
変更確認方法
feature/administrator-release-users-githubをローカルに取り込むbin/setupを実行foreman start -f Procfile.devでローカルサーバを立ち上げgithub-admintaroでログインGItHubアカウントの登録を解除するをクリックし、下記を確認するhatsunoのプロフィールページにリダイレクトし、GitHubとの連携を解除しました。というメッセージが表示されるhatsunoの編集画面で、GitHubアカウントの登録が解除されているScreenshot
変更前
root_path（ダッシュボード）にリダイレクト変更後