Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DocsのAPIで存在しないページにアクセスした際に404エラーが起こるようにした #5510

Closed
wants to merge 16 commits into from

Conversation

yuki-snow1823
Copy link
Contributor

@yuki-snow1823 yuki-snow1823 commented Sep 13, 2022

Issue

概要

https://bootcamp.fjord.jp/api/pages.json?page=12121212121
のような存在しないページ数を指定したときに404が出るようにしたい。

変更前

localhost:3000/api/pages.json?page=12121212121にアクセス

スクリーンショット 2022-10-30 18 47 44

変更後

スクリーンショット 2022-10-30 18 49 00

変更後

スクリーンショット 2022-10-30 13 33 01

変更確認方法

  1. ブランチbug/doc-api-error-404をローカルに取り込んでください。
  2. bin/rails sでローカル環境を立ち上げてください。
  3. localhost:3000/api/pages.json?page=12121212121にアクセスを行い、ActiveRecord::RecordNotFound エラーが出ていることを確認してください。

備考

本来エラーが返らない箇所にActiveRecord::RecordNotFoundが返るようにしたので、既存のsystemテストでエラーが出てしまうことを確認しました。下記の記事を参考にそのテストだけエラーを回避するようにしたのですが、その修正箇所が問題ないかなどを特に確認いただきたいです。

https://qiita.com/jnchito/items/37fcaf4486c4bdf78802

@yuki-snow1823
Copy link
Contributor Author

yuki-snow1823 commented Sep 13, 2022

🗒テストとDEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: api/pages/index.json (called from index at /Users/horikoshi/Desktop/study_space/bootcamp/app/controllers/api/pages_controller.rb:15)が解消できていない状態。

@yuki-snow1823
Copy link
Contributor Author

関係がありそうだと考えた記事
https://woshidan.hatenadiary.jp/entry/2021/03/07/195339

@komagata
Copy link
Member

@yuki-snow1823 この辺りが参考になるかもです〜

#4415

@yuki-snow1823 yuki-snow1823 marked this pull request as ready for review October 30, 2022 09:51
@yuki-snow1823
Copy link
Contributor Author

@AyakaTakashima
お疲れ様です🙏
こちらのレビューをお願いしてもよろしいでしょうか🙇‍♂️

@yuki-snow1823 yuki-snow1823 changed the title DocsのAPIでエラーを解消 DocsのAPIで存在しないページにアクセスした際に404エラーが起こるようにした Oct 30, 2022
@AyakaTakashima
Copy link
Contributor

@yuki-snow1823
ご連絡ありがとうございます!確認させていただきます!
明日〜明後日を目処に確認いたします🙇‍♀️

@AyakaTakashima AyakaTakashima self-requested a review November 1, 2022 09:38
@AyakaTakashima
Copy link
Contributor

AyakaTakashima commented Nov 1, 2022

@yuki-snow1823
お疲れ様です!
お伝えしたいことが1点、確認したいことが2点あります。

まずは、お伝えしたいこと1点目です。
レビューをリクエストする場合は、Reviewersに割り当てていただけると次回からレビューする人が楽になります!
割り当てられると、下記でレビューリクエストされたPRを見れるようになります。
https://github.com/pulls/review-requested

上記のreview requestsの一覧にない場合は、メンションされた時に通知されたメールを遡らなくてはいけないので大変です💦
私も2番目のPRで同じように忘れてしまったのでお気持ちわかります🥹
今回は私の方で割り当てておきましたので、次回から気をつけてみてください🙇‍♀️

続いて、確認したいこと1点目です。
「変更後」のスクショが2枚ありますが、挙動を確認して正しいものは一枚目なのかな?と思いましたが合ってますでしょうか...?
私の認識間違えでしたらすみません🙇‍♀️
2枚目のスクショも正しいものでしたら、確認点がいまいちわからなかったので教えていただけますと助かります🙇‍♀️

確認したいこと2点目は、仕様の要件についてです!
本issueは、下記画像のように404を表示せず、404エラーが返るだけで問題ない、という認識で合ってますでしょうか?
image

issueには、

https://bootcamp.fjord.jp/api/pages.json?page=12121212121
のような存在しないページ数を指定したときに404が出るようにしたい。

という記載だけで詳細が分かりませんでした💦
教えていただけますと助かります🙇‍♀️

@AyakaTakashima
Copy link
Contributor

AyakaTakashima commented Nov 1, 2022

@yuki-snow1823
続いて失礼いたします!!
一番最後の仕様要件の確認点なのですが、開発環境における404ページの表示について調べたところ
開発環境ではRoutingErrorエラーページの表示が通常で、ユーザー向けのページはされないものなのだと分かりました🙇‍♀️
知らずに不要な質問してしまい失礼いたしました🙇‍♀️勉強になりました!!

他の点のみ、確認していただければと思います!

Copy link
Contributor

@AyakaTakashima AyakaTakashima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コード確認しました!
なるほど...結合テストでAPIのテストを行っているので、Capybara.raise_server_errors = falseの記載を外すとテストが落ちちゃうんですね。
色々調べてみると、railsではAPIのテストは結合テストで行うのが一般的みたい(というか連結テストじゃないとできない...?)と理解しました。

自信はありませんが、挙動も確認できましたのでスクショの件確認できたらapproveさせていただきます!

@yuki-snow1823
Copy link
Contributor Author

yuki-snow1823 commented Nov 3, 2022

@AyakaTakashima
お疲れ様です!まずは丁寧なフィードバックありがとうございました🙏
ご指摘だけではなく、自分の誤りの推察やaya-kyanさんのご経験も織り交ぜてご指摘いただけたので、非常にありがたかったです。

レビューの内容に関しては

本issueは、下記画像のように404を表示せず、404エラーが返るだけで問題ない、という認識で合ってますでしょうか?

はい!こちらはその認識であっております!

ただ、テストでActiveRecord::RecordNotFoundを回避するための記述を多用してしまっているので、そこに関しては少々気になっておりApproveいただいた後 @komagata さんにも見ていただきたいと思っております。

Copy link
Contributor

@AyakaTakashima AyakaTakashima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuki-snow1823
お返事遅れてすみません!すでにapproveした気になっていました🙇‍♀️
自分の日報にこのissueのことを記載していたのですが、下記のようにbetachelseaさんの考えをいただきましたので記載させていただきます。
よかったら参考にしてみてください!
私からはapproveにさせていただきます!
image
引用元:https://bootcamp.fjord.jp/reports/62934

@yuki-snow1823
Copy link
Contributor Author

@AyakaTakashima
日報に書いてくださり、またその知見を共有してくださりありがとうございます!🙏
API単体のテストと、正確に呼び出されていた(おそらく結果が)ユーザーに利用されているかのテストという観点になるほどと思いました...!

今回のケースで言うと、単体の内容をみるべきだと思うのでCapybaraはOFFにするで良さそうかなと思っております。
Approveの確認とご連絡、ありがとうございました(__)

@komagata komagata self-assigned this Nov 9, 2022
@komagata komagata closed this Apr 12, 2023
@komagata komagata deleted the bug/doc-api-error-404 branch April 12, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants