yutokyokutyo / sample_app

Rails Tutorial 研修用
2 stars 0 forks source link

【Exercises9_6_9】adminがadmin自身を削除しないようにする機能とテストを追加しました。 #23

Closed yutokyokutyo closed 10 years ago

yutokyokutyo commented 10 years ago

Rails Tutorial 【Exercises9_6_9】

内容

adminがadmin自身を削除しないようにする必要があるので、その問題を解決する機能とテストを追加しました。 以下の手順の通りに実装を行いました。

% bundle exec rspec spec/                                                                                                              (git)-[Exercises9_6_9]
...........................................................................................F

Failures:

  1) User pages prevent admin users from destroying themselves. submitting a DELETE request to the Users#destroy action 
     Failure/Error: specify { expect(response).to redirect_to(root_url) }
       Expected response to be a redirect to <http://www.example.com/> but was a redirect to <http://www.example.com/users>.
       Expected "http://www.example.com/" to be === "http://www.example.com/users".
     # ./spec/requests/user_pages_spec.rb:62:in `block (4 levels) in <top (required)>'

Finished in 4.25 seconds
92 examples, 1 failure

Failed examples:

rspec ./spec/requests/user_pages_spec.rb:62 # User pages prevent admin users from destroying themselves. submitting a DELETE request to the Users#destroy action 

Randomized with seed 40337

リファクタリング後

% bundle exec rspec spec/                                                                                                              (git)-[Exercises9_6_9]
............................................................................................

Finished in 4.15 seconds
92 examples, 0 failures

Randomized with seed 9757

演習内容

http://www.railstutorial.org/book/updating_and_deleting_users#sec-updating_deleting_exercises


この内容でMasterにマージしたいと思います。 レビューをお願いいたします。

@tacahilo @kitak @gs3 @keokent

お願い

user_pages_spec.rbのdescribeメソッドにおける名前付けに関してもレビューをお願いしたいです!

"prevent admin users from destroying themselves."

kitak commented 10 years ago

adminがdestroyアクションを行う際に、削除する対象がadminであれば、アプリケーションのホームにリダイレクトしていることを確かめるテストを作成しました。

今の実装だと、削除する対象がadminじゃなくても、同じURLに遷移すると思うのだけど、どうだろう。

勘違いしていた! ごめん。遷移先が異なってるね。

keokent commented 10 years ago

adminがdestroyアクションを行う際に、削除する対象がadminであれば、アプリケーションのホームにリダイレクトしていることを確かめるテストを作成しました。

@ryoma123 くんの方と仕様が違うみたいなんだけど、RailsTutorial で指定されてないのかな。 (指定されてなければ 遷移先が二人の間で違っていても問題なさそう)

kitak commented 10 years ago

@keokent

Modify the destroy action to prevent admin users from destroying themselves. (Write a test first.)

とあるので、仕様の指定はされてなさそう。

@yutokyokutyo

今後、ExercisesでPullReqだすときは、どういう仕様にしたか(なぜ、そういう仕様にしたか)書くとよいかもね。

keokent commented 10 years ago

Modify the destroy action to prevent admin users from destroying themselves. (Write a test first.) とあるので、仕様の指定はされてなさそう。

なら良さそうだ〜〜 :hamster:

hfm commented 10 years ago

:whale:

yutokyokutyo commented 10 years ago

@kitak @keokent

else 句 の内容を実行するときに、if式の条件にあるものも加えて、User.find(params[:id])が二度実行されることになるので、変数にいったんいれるとよいかも。

そうでした!昨日の午前中にご指摘いただいたはずなのに忘れておりました。。 先程、@userというインスタンス変数にいれることによりこの冗長性を排除するCommitを加えましので、ご確認していただけますでしょうか。

今後、ExercisesでPullReqだすときは、どういう仕様にしたか(なぜ、そういう仕様にしたか)書くとよいかもね。

はい!次回から書くことにします! 今回はあれかれ考えた結果、削除する対象がadminだった場合のみ、root_URLにリダイレクトさせるのが最適だと考えこのような仕様にしました。

あれこれ考えた経緯を以下のリンク先に記述しました。 昨日時間に迫られながら書いた文章なので、間違っていたり、読みづらい文章になっているかと思います。後で時間があるときに整理して、分かりやすい文章に校正します。

https://gist.github.com/yutokyokutyo/4fb61e4ff3d9bca35099

kitak commented 10 years ago

あれこれ考えた経緯を以下のリンク先に記述しました。 昨日時間に迫られながら書いた文章なので、間違っていたり、読みづらい文章になっているかと思います。後で時間があるときに整理して、分かりやすい文章に校正します。

こういうまとめ、よいと思います! :+1:

自分の思考の過程は、gistにまとめてもよいのだけど、適宜、PullReqのコメントに書いていってもよいと思います。kyokutyoのそのときの考えに、3ジニアズがすぐにフィードバックを返すことができるし、ひとつのページにまとまっていたほうが後から見返しやすいかな、と。

yutokyokutyo commented 10 years ago

@kitak

3ジニアズがすぐにフィードバックを返すことができるし、ひとつのページにまとまっていたほうが後から見返しやすいかな、と。

そうですね!! 次回からは自分がどう考えたのかという過程も、適宜コメントに書いていきます! ありがとうございます!

yutokyokutyo commented 10 years ago

@keokent @kitak

destroyアクションの中の変数について、インスタンス変数だった部分をローカル変数に修正しました。 この内容でマージしてもよろしいでしょうか? 何度も申し訳ないです。ご確認をお願いいたします!

keokent commented 10 years ago

destroyアクションの中の変数について、インスタンス変数だった部分をローカル変数に修正しました。 この内容でマージしてもよろしいでしょうか?

良さそうだ〜〜!! :hamster:

yutokyokutyo commented 10 years ago

良さそうとのことなのでマージいたします!:octocat: レビューをしていただいた皆様、自分のために貴重なお時間を割いていただき、本当にありがとうございました!!

keokent commented 10 years ago

:+1:

hfm commented 10 years ago

:+1: