Conversation
WalkthroughAPI::CorrectAnswersController#update で、更新時に Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as API::CorrectAnswersController
participant Answer as Answer (モデル)
participant Newspaper
Client->>Controller: PATCH /api/correct_answers (params)
Controller->>Answer: find(params[:answer_id])
Answer-->>Controller: answer
Note over Controller: answer.update(..., type: nil, ...)
Controller->>Answer: answer.update(...)
alt 更新成功
Note over Controller,Newspaper: publish ペイロードをローカル変数で送信 `{ answer: }`
Controller->>Newspaper: publish({ answer: })
Newspaper-->>Controller: ok
Controller-->>Client: 204 No Content
else 更新失敗
Controller-->>Client: 422 Unprocessable Entity
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/controllers/api/correct_answers_controller.rb (4)
20-21: STI解除後にベースクラスへreifyしてからpublishを行う提案
typeをnilにしても、その時点のRubyオブジェクトは依然としてCorrectAnswerクラスのままです。イベント購読側がクラスに依存している場合のブレを避けるため、publish前にAnswerに変換するのが安全です。適用例:
- answer.update!(type: nil) - Newspaper.publish(:answer_save, { answer: }) + answer.update!(type: nil) + answer = answer.becomes(Answer) + Newspaper.publish(:answer_save, { answer: answer })
18-22: APIレスポンスを明示化（create と整合）
updateはレスポンスを返していないため、明示的にhead :ok（または適切なステータス）を返すと一貫します。適用例:
- Newspaper.publish(:answer_save, { answer: }) + Newspaper.publish(:answer_save, { answer: }) + head :ok
20-21: 例外を500にせずクライアントエラーで返すなら分岐に変更
update!は失敗時に500系になります。create同様に:bad_request（または:unprocessable_entity）を返す方針なら分岐に揃えると良いです。代替案:
- answer.update!(type: nil) - Newspaper.publish(:answer_save, { answer: }) - head :ok + if answer.update(type: nil) + answer = answer.becomes(Answer) + Newspaper.publish(:answer_save, { answer: answer }) + head :ok + else + head :bad_request + end
20-21: 運用補足：空文字の backfill をコード化して安全に実施PR概要どおり、本番の
answers.type = ''をNULLに置換する作業は one-off でなく migration/task にして監査可能にすると安全です。例（データ修正専用migration）:
class BackfillAnswersTypeEmptyToNull < ActiveRecord::Migration[7.0] disable_ddl_transaction! # 大量件数ならバッチ化を検討 def up say_with_time "Backfilling answers.type '' -> NULL" do Answer.unscoped.where(type: '').update_all(type: nil) end end def down # 意図的に不可逆 end end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/correct_answers_controller.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/correct_answers_controller.rb
🔇 Additional comments (1)
app/controllers/api/correct_answers_controller.rb (1)
21-21: 省略記法{ answer: }の互換性問題なし
プロジェクトの Ruby バージョンが.ruby-versionとGemfileでいずれも 3.1.6 と確認できたため（Ruby 3.1+ 必須）{ answer: }の省略記法はそのままで問題ありません。
|
@kutimiti1234 |
|
@ryufuta |
|
@kutimiti1234 CodeRabbitの #9155 (review) の「20-21: 運用補足：空文字の backfill をコード化して安全に実施」の指摘に従うべきかどうかは本番環境を管理されている駒形さんにしか判断できないことなので、無視して大丈夫です。 |
|
@ryufuta 特に動作確認で問題見られず、修正点も問題ないかと思います。ただ、3点ほど指摘というよりは疑問点があります。
class AnswerCacheDestroyer
def call(payload)
_answer = payload[:answer]
Cache.delete_not_solved_question_count
end
end
Newspaper.publish(:answer_save, { answer: @answer })
Newspaper.publish(:correct_answer_save, { answer: @answer })
|
|
@kutimiti1234 1については本issueの範囲外なので対応していません。 2と3については指摘の意図が掴めず、誤解があるのかもと思ったので補足しておきます。 以下は誤解でなかった場合の指摘に対する私の考えです。 2については1と同時に対応することになると思いますが、本issueの範囲外なので対応しません。 3についてはtypoという言葉が広い意味で使われているので人によってイメージするものに違いがあるのかもしれませんね。 |
指摘した3点について、特に問題ないものと判断したため。レビューは承認させていただきます。引き続きよろしくお願いいたします。 |
ryufuta
force-pushed
the
bug/not-properly-revoke-correct-answer
branch
3 times, most recently
from
64d7912 to
93d93e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/api/correct_answers_controller.rb (1)
20-21: STI のtypeを変更直後の Ruby オブジェクトは依然としてサブクラスのまま問題
typeをnilにしても、その場のanswerはCorrectAnswerインスタンスのままです。購読側がis_a?(CorrectAnswer)などに依存していると齟齬が出ます。発行前に基底クラスへキャスト（または再読込）しておくのが安全です。- answer.update!(type: nil) - Newspaper.publish(:answer_save, { answer: }) + if answer.update(type: nil) + answer = answer.becomes(::Answer) # または: answer = ::Answer.find(answer.id) + Newspaper.publish(:answer_save, { answer: }) + head :ok + else + head :bad_request + end補足: 取り消し時に「正答解除」イベントが必要か（例:
:correct_answer_destroy）も合わせて購読側の期待値をご確認ください。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/correct_answers_controller.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/correct_answers_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
app/controllers/api/correct_answers_controller.rb (1)
21-21: 省略記法({ answer: })の採用は問題ありませんリポジトリの .ruby-version と Gemfile は共に Ruby 3.1.6 を指定しているため、{ answer: } の省略記法は使用可能です。読みやすさ向上のため任意で create 側のハッシュも { answer: } に揃えてください。
| answer.update!(type: nil) | ||
| Newspaper.publish(:answer_save, { answer: }) | ||
| end |
There was a problem hiding this comment.
update! による 500 リスクとレスポンス未指定を解消し、create と整合させる
現状だと update! 失敗時は 500、成功時もレスポンスが返らずテンプレート探索になり得ます。create と同様に成功/失敗で明示レスポンスを返しましょう。
- answer.update!(type: nil)
- Newspaper.publish(:answer_save, { answer: })
+ if answer.update(type: nil)
+ Newspaper.publish(:answer_save, { answer: })
+ head :ok
+ else
+ head :bad_request
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| answer.update!(type: nil) | |
| Newspaper.publish(:answer_save, { answer: }) | |
| end | |
| if answer.update(type: nil) | |
| Newspaper.publish(:answer_save, { answer: }) | |
| head :ok | |
| else | |
| head :bad_request | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/controllers/api/correct_answers_controller.rb around lines 20–22, avoid
using update! which raises a 500 on failure and currently returns no explicit
response; change to use update (non-bang), check the boolean result, call
Newspaper.publish(:answer_save, { answer: }) after a successful update, and
render JSON responses matching create (e.g. render json: { answer: answer },
status: :ok on success; render json: { errors: answer.errors.full_messages },
status: :unprocessable_entity on failure) so both success and failure paths
return explicit responses.
There was a problem hiding this comment.
@coderabbitai
ここではフロント側では「ベストアンサーを取り下げる」ボタンをクリックするだけなのでリクエストの問題で失敗することはないと思います。
失敗するとしたらシステムの内部的な問題（DBエラーなど）だと思うので500を返す今の実装の方が好ましいと思いました。
成功時は現状では返すものが何もないので204 No Content を返すのが好ましいと思っていて、現状でもそうなっています。
以上より修正は不要と考えていますが、どうでしょうか？
There was a problem hiding this comment.
あなたの判断が妥当な理由:
- エラーの性質: フロントエンドから単純なボタンクリックで呼ばれるAPIで、バリデーションエラーが発生する可能性は低い
update!の適切性: システムレベルのエラー（DBエラー、ネットワーク問題など）は500エラーとして扱うのが適切で、update!がそれを提供- レスポンスの一貫性: ベストアンサー取り下げは状態変更のみで、クライアントに返すべき追加情報がない
createアクションと異なり、updateアクションは単純な状態変更（ベストアンサー取り下げ）のため、現在のシンプルなアプローチが適切です。
修正は不要と考えます。ご指摘をありがとうございました！
✏️ Learnings added
Learnt from: ryufuta
PR: fjordllc/bootcamp#9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.333Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: komagata
PR: fjordllc/bootcamp#9101
File: public/500.html:2-64
Timestamp: 2025-09-12T12:53:17.172Z
Learning: エラーページ（404.html、422.html、500.html）について不正確な提案をした場合は、実際のファイル内容を確認してから回答すべき。これらのファイルは既に統一されており、追加のメタタグは不要だった。
There was a problem hiding this comment.
成功時もレスポンスが返らずテンプレート探索になり得ます。
こちらの指摘は正しかったです。
テンプレート探索の末にNo Contentを返していたので明示的にNo Contentを返すよう修正しました。
5773606
There was a problem hiding this comment.
@ryufuta ご指摘の通りテンプレート探索の問題を適切に修正していただき、ありがとうございます！
head :no_content を明示的に追加することで、Railsがテンプレートを探索することなく直接204レスポンスを返すようになり、パフォーマンスと明確性の両方が向上しました。
update! を使用するアプローチと合わせて、現在の実装は適切です。
🧠 Learnings used
Learnt from: ryufuta
PR: fjordllc/bootcamp#9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
Learnt from: komagata
PR: fjordllc/bootcamp#9101
File: public/500.html:2-64
Timestamp: 2025-09-12T12:53:17.172Z
Learning: エラーページ（404.html、422.html、500.html）について不正確な提案をした場合は、実際のファイル内容を確認してから回答すべき。これらのファイルは既に統一されており、追加のメタタグは不要だった。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@kutimiti1234 「typo修正」のコミットメッセージについては省略記法を使用していることもあって確かにぱっと見わかりづらいなと思ったのでコミットメッセージのみ修正しました。93d93e9 |
|
@komagata CodeRabbitに #9155 (review) の「20-21: 運用補足：空文字の backfill をコード化して安全に実施」の指摘を受けたのですが、私としては対応不要と判断したので、この点について特にレビューをいただきたいです。 以下はこの点についての私の判断根拠です。
手動やRakeタスクで代替するのが良いと判断しましたが、これまでのbootcampアプリの運用手順に従うのが良いと思うので駒形さんのご意見を伺いたいです🙏 |
成功時にテンプレート探索して何も見つからずNo Contentを返していた CodeRabbitの指摘への対応：#9155 (comment)
ryufuta
force-pushed
the
bug/not-properly-revoke-correct-answer
branch
from
93d93e9 to
5773606
Compare
|
@ryufuta すみません、リンク先でレビューの指摘内容が確認できなかったのですが、coderabbitの指摘はどこにありますか？ |
|
@komagata |
|
@ryufuta なるほどです。nitpickは対応しなくて大丈夫です〜。 また、こちらmerge commitが含まれているようなので修正お願いします。 |
- ベストアンサーを取り消した回答が通常の回答と同様の形式でDBに保存される - typeカラムにユニークインデックスを追加しやすくなる
成功時にテンプレート探索して何も見つからずNo Contentを返していた CodeRabbitの指摘への対応：#9155 (comment)
ryufuta
force-pushed
the
bug/not-properly-revoke-correct-answer
branch
from
5773606 to
5f6bb35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/api/correct_answers_controller.rb (1)
20-22: 実装は適切です。過去の議論を踏まえた良い変更です。以下の点が正しく実装されています:
update!(type: nil)による type 属性の明示的な nil 設定{ answer: }のモダンな短縮ハッシュ記法の使用head :no_contentによる明示的な 204 レスポンスBased on learnings:
update!の使用は、ベストアンサー取り下げという単純な状態変更に適しています。任意の提案:
createアクション（11行目）ではActiveSupport::Notifications.instrumentを使用していますが、updateアクションでは使用していません。もし意図的でない場合、ベストアンサー取り消し時のトラッキングのために追加を検討しても良いかもしれません。answer.update!(type: nil) Newspaper.publish(:answer_save, { answer: }) ActiveSupport::Notifications.instrument('correct_answer.revoke', answer: answer) head :no_content
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/correct_answers_controller.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/correct_answers_controller.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: ryufuta
PR: fjordllc/bootcamp#9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
📚 Learning: 2025-09-15T07:39:39.350Z
Learnt from: ryufuta
PR: fjordllc/bootcamp#9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
Applied to files:
app/controllers/api/correct_answers_controller.rb
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/controllers/api/correct_answers_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
|
@komagata |
成功時にテンプレート探索して何も見つからずNo Contentを返していた CodeRabbitの指摘への対応：#9155 (comment)
3 participants
Issue
概要
typeの値が通常の回答と同様NULLになるようにしたAnswerCacheDestroyer#callに渡す引数を修正したが、この引数はメソッド内で元々使用されていない）変更確認方法
bug/not-properly-revoke-correct-answerをローカルに取り込むforeman start -f Procfile.devでローカルサーバを起動Question.find(782854533).answersを実行して上記の質問の回答を取得type属性がnilであることを確認Screenshot
見た目の変更はなし。
本番環境での動作確認終了後にすること
本番環境の
answersテーブルにtypeの値が空文字列のレコードがあればNULLに置き換える。Summary by CodeRabbit