Conversation
ウォークスルーイベント発行フローが再構成され、Newspaper.publishの条件付き化と特定のコントローラアクションに対するアクション情報の追加が行われています。回答削除時のバックエンド呼び出しが削除され、不解決質問数キャッシュの取得とコールバック処理が最適化されました。 変更内容
シーケンス図sequenceDiagram
participant Client
participant AnswerCtrl as API::AnswersController
participant CorrectAnswerCtrl as API::CorrectAnswersController
participant Newspaper
participant CacheDestroyer as AnswerCacheDestroyer
participant Cache
Note over AnswerCtrl,CacheDestroyer: 旧フロー：create/update 時に常に発行
AnswerCtrl->>Newspaper: publish(:answer_save) [削除]
Note over AnswerCtrl,CacheDestroyer: 新フロー：destroy 時のみ CorrectAnswer で発行
Client->>AnswerCtrl: DELETE /answer/:id
AnswerCtrl->>AnswerCtrl: guard: is_a?(CorrectAnswer)?
alt is CorrectAnswer
AnswerCtrl->>Newspaper: publish(:answer_save, {answer:, action:})
Newspaper->>CacheDestroyer: notify
CacheDestroyer->>Cache: clear not_solved_question_count
CacheDestroyer->>CacheDestroyer: log action
else Not CorrectAnswer
AnswerCtrl->>AnswerCtrl: no publish
end
Note over CorrectAnswerCtrl: CorrectAnswersController: アクション情報追加
Client->>CorrectAnswerCtrl: POST/PATCH /correct_answer
CorrectAnswerCtrl->>Newspaper: publish({answer:, action: "ControllerClass#action"})
Newspaper->>CacheDestroyer: notify
CacheDestroyer->>Cache: evict cache
推定コードレビュー工数🎯 4 (複雑) | ⏱️ ~40 分 理由：複数のコントローラ、モデル、ビュー層にわたるイベント発行フロー変更。キャッシュ戦略の再構成。before_destroy コールバック追加による破棄フロー変更。構文エラー存在（correct_answers_controller.rb の update フロー）。ガード条件の追加による分岐ロジック増加。 関連の可能性がある Issue
関連の可能性がある PR
推奨レビュアー
ポエム
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
| deleteAnswer(answerId) | ||
| if (answerBadgeElement.classList.contains('correct-answer')) { | ||
| cancelBestAnswer(answerId, questionId) | ||
| const otherCancelBestAnswerButtons = document.querySelectorAll( |
There was a problem hiding this comment.
削除する回答がベストアンサーであった場合に、deleteAnswer()とcancelBestAnswer()が実行されるようになっており、2つのAPI(API::AnswerController#destroyとAPI::CorrectAnswerController#update)が同時実行されて、キャッシュクリアが2重に呼び出される状態になっていました。
回答レコード削除後にanswer.typeを変更する必要性はないため、cancelBestAnswer()の呼び出しを削除しました。
| Question.not_solved.not_wip.where(practice_id:).size | ||
| else | ||
| Question.not_solved.not_wip.size | ||
| ::Cache.not_solved_question_count |
There was a problem hiding this comment.
より近いスコープに、act-as-taggable-onというgemのTaggable::Cacheモジュールが存在し、そちらに解決しようとするため、トップレベルから定数解決するように記述しました。
Miya096jp
changed the title
|
お疲れ様です！ |
|
@Miya096jp |
|
申し訳ありませんでした😵 |
ryufuta
left a comment
ryufuta
left a comment
There was a problem hiding this comment.
@Miya096jp
お疲れ様です！
今回修正した箇所については一点軽微な指摘をしていますが、概ね大丈夫そうです。
以下、descriptionの内容と追加で修正した方が良さそうなコードについてコメントします。
descriptionについて
Issueの箇所
リンク先がこのPR自体になっているのでURLの修正をお願いします🙏
概要の箇所
管理者・メンターでログインした場合に、Q&Aページの未解決タブ、およびグローバルメニューのQ&Aに、未解決質問数のバッジが表示されるようになっています。
とありますが、一般ユーザーでログインしたときもグローバルメニューのQ&Aには未解決質問数のバッジが表示されるので誤解を招く書き方かもと思いました。
事前準備の箇所
開発環境でキャッシュを有効化するには
bin/rails dev:cache
を実行する必要があるので手順に追記した方が良いと思います。
"3. 初期データを更新する"の箇所から番号がずれているので合わせて対応をお願いします🙏
ログ監視コマンドの箇所
tail -f log/development.log | grep -E "(CACHE|AnswerCacheDestroyer)"
だとCACHE ActiveStorage::Blob Load (0.0ms)やCACHE RegularEventRepeatRule Load (0.0ms)が大量に出力されるので
tail -f log/development.log | grep -E "(CACHE MISS|AnswerCacheDestroyer)"
の方がわかりやすそうです。
検証項目の箇所
以下の項目が漏れています。
- 質問をWIPで保存 → ログ出力なし。現状でも期待通り動作しています。
- WIPの質問を削除 → ログ出力なしが期待する動作だと思いますが、現状では
[CACHE MISS]が出力されます。 - 解決済みの質問を削除 → ログ出力なしが期待する動作だと思いますが、現状では
[CACHE MISS]が出力されます。
補足: 質問作成/削除時のキャッシュクリアについて
Questionモデルのafter_saveとafter_destroyにQuestionCallbacksが指定されており、その中で直接キャッシュクリアされています。この部分についてはバグが隠れる複雑性もないと考えられるのでログは追加していません。
ここだけログを追加しない根拠としては弱い気がするのですが、かといって別途ログを追加するのはDRYでないので避けた方が良いと思います。
AnswerCacheDestroyerとNewspaperを使ってここも他と共通化するのが個人的には良さそうと思います。
元々はここもNewspaperに置き換えるはずが忘れられた可能性もあるので別のリファクタリングのIssueを作って報告しておき、必要ならそちらで誰かに対応してもらうことにすると良いのではと思います。（今ならNewspaperではなくActiveSupport::Notificationsへの置き換えですが）
追加の修正の提案
検証項目の箇所で指摘した
- WIPの質問を削除
- 解決済みの質問を削除
についてQuestionCallbacks#after_destroy内に適切な条件を追加して対応することになるかなと思います。
| }) | ||
| end | ||
| @answer.destroy | ||
| Newspaper.publish(:answer_destroy, { answer: @answer }) |
There was a problem hiding this comment.
Newspaper.publish()を@answer.destroyの前に実行する変更も同時に加えているのがなぜなのか気になりました。
- 回答を削除してからキャッシュを削除する方が自然
createやupdateとNewspaper.publish()の実行順序を合わせる方が読みやすい
という点で特に理由がなければ実行順序は変更しない方が良いかなと思いました。
There was a problem hiding this comment.
@answer.destroyの後に記述すると、レコードが削除されてしまうので、is_a?メソッドでNoMethodErrorになると思って順番を変えたというのが理由です。指摘していただいたおかげで理解が深まりました🙏
実際はdestroyアクションの中ではレコード削除後にオブジェクトがfreezeされ、以降は変更はできないものの、アクション内においては参照可能であるとわかったので、順番を元に戻しました。
なお、これに倣い、QuestionCallbacksのキャッシュクリアをQuestionController#Destroyに移動を試してみたのですが、その場合だと@question.not_wip?のほうはdestroy後もfreezeされたレコードを参照して条件分岐は有効であるものの、
@question.destroy
if @question.not_wip? && @question.unsolved?
関連テーブルのanswerの方はfreezeされず削除されてしまうため、@question.unsolved?の方がNoMethodErrorになることがわかり、断念しました。
Miya096jp
force-pushed
the
bug/use-cache-for-unsolved-badge
branch
2 times, most recently
from
c53807a to
4728eda
Compare
![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 (2)
test/models/question_test.rb (2)
52-57: STIの標準的なパターンに従うことを推奨します。
Answer.create!(type: 'CorrectAnswer')の代わりにCorrectAnswer.create!を使用することで、Rails の STI（Single Table Inheritance）の規約に従ったより明示的なコードになります。次のように修正することを検討してください：
- Answer.create!( + CorrectAnswer.create!( description: 'ベストアンサー', user: answerer, - question: solved_question, - type: 'CorrectAnswer' + question: solved_question )
71-76: 通常の回答にはtypeフィールドを設定しない方が明確です。
type: ''を明示的に設定するのではなく、typeフィールド自体を省略することで、通常のAnswerであることがより明確になります。Rails の STI では、基底クラスのインスタンスを作成する際はtypeを指定しないのが一般的です。次のように修正することを検討してください：
Answer.create!( description: '通常の回答', user: answerer, - question: unsolved_question, - type: '' + question: unsolved_question )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/question.rb(3 hunks)app/models/question_callbacks.rb(1 hunks)test/models/question_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/question.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/models/question_callbacks.rbtest/models/question_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/question_test.rb
🧠 Learnings (1)
📓 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 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
🧬 Code graph analysis (2)
app/models/question_callbacks.rb (2)
app/models/cache.rb (1)
delete_not_solved_question_count(62-64)app/models/question.rb (2)
not_wip?(53-55)unsolved?(57-59)
test/models/question_test.rb (1)
app/models/question.rb (1)
unsolved?(57-59)
⏰ 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 (3)
app/models/question_callbacks.rb (3)
4-10: LGTM！キャッシュクリアのログ追加が適切です。キャッシュクリアの動作を追跡するためのログが追加されており、デバッグに役立ちます。メソッド名も含まれているため、どこでキャッシュがクリアされたかが明確です。
12-17: LGTM！条件付きキャッシュクリアのロジックが正確です。
not_wip?とunsolved?の両方をチェックすることで、未解決質問数のキャッシュに実際に影響する質問のみでキャッシュをクリアしています。これは PR の目的である「不要なキャッシュクリアの防止」と一致しています。念のため、回答が1つもない質問を削除した場合の動作も期待通りか確認してください。
unsolved?は!answers.exists?(type: 'CorrectAnswer')を返すため、回答が0件の質問でもunsolved?は true を返すはずです。
19-21: LGTM！責務が明確になりました。キャッシュクリアが
before_destroyに移動したことで、after_destroyは通知の削除のみに責務が集中し、コードの意図がより明確になっています。
|
|
||
| after_save QuestionCallbacks.new | ||
| # before_destroy :clear_question_cache, prepend: true | ||
| before_destroy QuestionCallbacks.new, prepend: true |
There was a problem hiding this comment.
after_destroyに条件分岐を追加しようとしましたが、それだとコールバックの実行時にはすでにレコードが削除ずみになるためNoMethodErrorになることがわかりました。
代替案としてbefore_destroyを新たに定義し、そちらにキャッシュクリアと条件分岐を追加しました。
その際、関連テーブルにdependent: :destroyが指定されていることで、関連テーブルの削除がbefore_destroyの前に実行されてしまうため、
has_one :correct_answer, dependent: :destroy
before_destroyにprepend: trueを指定し、関連テーブルの削除より先に実行するようにしました。
There was a problem hiding this comment.
容易に条件を追加できると思っていましたが、私の見通しが間違っていて申し訳ありません🙇🏻♂️
このPRではここの実装はこれで良いと思うのですが、Callbacksが複雑になったのが気がかりなので、このPRと #9202 のマージ後に私の方でActiveSupport::Notificationsに置き換えるリファクタリングのIssueを作ろうと思います。
Callbacksが複雑化すると読みづらく変更しづらくなるのでNewspaperを導入したという経緯があります。
この経緯については以下が参考になります。
newspaperでActiveRecordのCallbacksを置き換える - komagataのブログ
Newspaperを読んでみた
この経緯を踏まえるとCallbacksをなるべく使わない、使うにしても単純にしておくことがチームの方針だと思います。
#9202 のマージ後にActiveSupport::Notificationsへ置き換えることで比較的容易にこの問題を解決できると思います。
There was a problem hiding this comment.
いえいえ、大事なことがいろいろ学べたのでとてもよかったです👍
Issueのご提案ありがとうございます🙏
私もここだけCallbacksが残っているのが疑問だったので、ここにも書きましたが、before_destroyのロジックをコントローラーに移設を試みて失敗しました。
私の理解だと、これををPub/Subとしてコントローラーに移そうとすると:
- 結局再キャッシュされると割り切って正確なキャッシュクリアを諦める(条件分岐を省く)
- correct_answerの値がdestroyされる前に取得&キープしておくインスタンスメソッドを定義し、before_destroy prepend: trueに指定し、値を条件判定に利用する
の2パターンを今のところ思いつくのですが、自信はありません。(特に1)
もし私がそのIssueを担当するとしたら、before_destroyのコールバックだけ、コメントに理由を残してそのまま残しておく方法をとるかなと思います🤔
もし同じようなことをお考えであれば、私がIssueを立てて、before_destroyのハマりポイントの詳細を書いておくとよいかなと思ったのですが、いかがでしょうか？
返事は率直で大丈夫です。
(ryufutaさんが立てられる場合も、経過はモニタリングするので特に理由などは書かなくても結構です)
There was a problem hiding this comment.
条件の追加を提案しておいてなんですが、質問が削除されること自体が稀なので複雑さを避けるために1でも良いとは思います。
2についてはどういう実装なのか理解できていませんが、複雑そうな印象を持ちました。（現状とあまり変わらない？）
私が考えていた実装は以下です。
実行はしていないので期待通り動作する保証はないです。
（ログ出力のためにAnswerCacheDestroyerの修正は必要）
app/controllers/questions_controller.rb
def destroy
# `@question.correct_answer`を実行するために先にキャッシュを削除
ActiveSupport::Notifications.instrument('question.destroy', question: @question) if !@question.wip? && @question.correct_answer.nil?
@question.destroy
redirect_to questions_url, notice: '質問を削除しました。'
endconfig/initializers/active_support_notifications.rb
ActiveSupport::Notifications.subscribe('question.destroy', AnswerCacheDestroyer.new)上の実装を採用するとしたら #9202 のマージ後でこのPRのレビューも私がしているので私がIssueを作ってそのままPRも担当するのが細かい説明抜きに取り組めて楽で良いと思います。
いずれにせよ、Callbacksをなくすリファクタリングにどこまでチームとして力を入れるか次第かもしれませんし、この後の大倉さんのレビューで現状で問題ないということになればそのままで良いのかもしれません。
There was a problem hiding this comment.
その実装で動きそうですね、まさに灯台元暗し...
書いて頂いたおかげで、一周回って一番シンプルな方法を思い出せました😅
2は、フラグを渡して処理するようなイメージでしたが、それこそ不要だったので、ご放念ください。
ありがとうございます、Issueの作成よろしくお願いいたします🙇🏻♂️
|
お疲れ様です！ |
ryufuta
left a comment
ryufuta
left a comment
There was a problem hiding this comment.
@Miya096jp
お疲れ様です。
私の見通しが間違っていたせいで、面倒な作業になってしまい申し訳ありませんでした🙇🏻♂️
Question#clear_question_cacheを実装後に削除したりといった試行錯誤の後が見えます。
私にも原因があるので申し訳ないのですが、個人の試行錯誤の過程は必要ならば個人のメモに残すに留めてコミットには残さない方が良いと思います。
レビューする際には余計な情報になりますし、後から履歴を追いづらくなります。
可能ならばコミットを整理していただきたいのですが、難しそうであればこのままで大丈夫です。
参考：調査をサクサク進めるために。伊藤淳一が考える「良いプルリクエスト、悪いプルリクエスト」
以下はdescriptionについて。
Issueのリンクが未訂正なので訂正をお願いします🙏
また「このPRが作成された経緯」はこのPRのレビューには不要な情報なので書かなくても良かったかなと思います。
この内容の一部はIssueのバッティングが発生する前にNewspaperの置き換えのIssue上で説明しておくと良かったことなので今となっては不要ですし、その後の作業の進め方については #9202 (comment) 以降で当事者間で話し合いが済んでいるので不要だと思いました。
せっかくなので削除しなくても良いですが、少なくともこの後レビューされる大倉さんにとってはノイズなので折りたたんで見せないようにするなどの配慮があった方が良いと思います。
セクションを折りたたんで情報を整理する - GitHub Docs に折りたたみの方法が載っています。
app/models/question.rb
Outdated
| before_validation :set_published_at, if: :will_be_published? | ||
|
|
||
| after_save QuestionCallbacks.new | ||
| # before_destroy :clear_question_cache, prepend: true |
There was a problem hiding this comment.
コメントアウトした行は不要なので削除してください🙏
その際、削除するコミットを新たに積むのではなく、コミットを整理してこの行を追加したコミットをコミット履歴から削除するのが望ましいですが、難しそうであれば今回は普通に削除して大丈夫です。
There was a problem hiding this comment.
必要な変更を含んでいたので、コミットを修正するかたちをとりました。
|
|
||
| after_save QuestionCallbacks.new | ||
| # before_destroy :clear_question_cache, prepend: true | ||
| before_destroy QuestionCallbacks.new, prepend: true |
There was a problem hiding this comment.
容易に条件を追加できると思っていましたが、私の見通しが間違っていて申し訳ありません🙇🏻♂️
このPRではここの実装はこれで良いと思うのですが、Callbacksが複雑になったのが気がかりなので、このPRと #9202 のマージ後に私の方でActiveSupport::Notificationsに置き換えるリファクタリングのIssueを作ろうと思います。
Callbacksが複雑化すると読みづらく変更しづらくなるのでNewspaperを導入したという経緯があります。
この経緯については以下が参考になります。
newspaperでActiveRecordのCallbacksを置き換える - komagataのブログ
Newspaperを読んでみた
この経緯を踏まえるとCallbacksをなるべく使わない、使うにしても単純にしておくことがチームの方針だと思います。
#9202 のマージ後にActiveSupport::Notificationsへ置き換えることで比較的容易にこの問題を解決できると思います。
app/models/question_callbacks.rb
Outdated
| end | ||
|
|
||
| def before_destroy(question) | ||
| return unless question.not_wip? && question.unsolved? |
There was a problem hiding this comment.
2重否定はわかりにくいので次のようにする方が良さそうです。
return if question.wip? || question.sloved?
app/models/question.rb
Outdated
| def not_wip? | ||
| wip == false | ||
| end |
There was a problem hiding this comment.
Active Recordで元々定義されているwip?メソッドを使えるのでこのメソッドは不要かなと思います。
app/models/question.rb
Outdated
| def unsolved? | ||
| !answers.exists?(type: 'CorrectAnswer') | ||
| end | ||
|
|
There was a problem hiding this comment.
solved?メソッドを定義する方がこの場合だと使いやすくわかりやすいかなと思います。
またsolved?と同じことはcorrect_answer.present?でできるので、そもそもメソッド定義自体がなくても良いかなと思います。
前者の方がわかりやすいですが、後者はリポジトリ内の他の箇所でもよく使われているので統一する意味でも望ましいですし、メソッドを定義しなければテストコードを書かずに済みます。
（ちなみにunsolved?と同じことはcorrect_answer.nil?でもできます）
There was a problem hiding this comment.
ありがとうございます、wip?とcorrect_answer.present?を使い、インスタンスメソッドとテストは削除しました。
test/models/question_test.rb
Outdated
| Answer.create!( | ||
| description: 'ベストアンサー', | ||
| user: answerer, | ||
| question: solved_question, | ||
| type: 'CorrectAnswer' | ||
| ) |
There was a problem hiding this comment.
テストコードを残すのであれば #9232 (review) のNitpick commentsにある提案通りに修正した方が良いです。
STIではActive Recordがtypeの値を適切に設定してくれます。
手動で設定するとtypoしてバグを生む可能性もあるので必要なければ手動で設定するのは避けた方が良いという理由もあるのかなと思います。
test/models/question_test.rb
Outdated
| Answer.create!( | ||
| description: '通常の回答', | ||
| user: answerer, | ||
| question: unsolved_question, | ||
| type: '' | ||
| ) |
There was a problem hiding this comment.
こちらもテストコードを残すのであれば #9232 (review) のNitpick commentsにある提案通りに修正をお願いします。
こちらに関してはtype: ''は誤りなので修正必須になります。
STIでは基底クラス（今の場合はAnswer）のインスタンスのtypeの値はnilで、DBにレコードが保存される際にはNULLになります。
correct_answers_controller.rb内のupdateメソッドでは誤って''を設定しており、こちらは現在私が修正のPRを作成しています。
Miya096jp
force-pushed
the
bug/use-cache-for-unsolved-badge
branch
from
f634aab to
69e7afc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/question_callbacks.rb (1)
8-9: ログ出力の順序を統一することを検討してください。
after_saveではログ出力がキャッシュクリアの前に実行されていますが、before_destroy（16行目）ではキャッシュクリアの後にログ出力が実行されています。一貫性のため、どちらかに統一することをお勧めします。例えば、以下のように
after_saveでもキャッシュクリア後にログ出力するように変更できます：- Rails.logger.info '[CACHE CLEARED#after_save] Cache destroyed for unsolved question count.' Cache.delete_not_solved_question_count + Rails.logger.info '[CACHE CLEARED#after_save] Cache destroyed for unsolved question count.'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/api/answers_controller.rb(1 hunks)app/controllers/api/correct_answers_controller.rb(2 hunks)app/javascript/initializeAnswer.js(0 hunks)app/models/answer_cache_destroyer.rb(1 hunks)app/models/cache.rb(1 hunks)app/models/question.rb(3 hunks)app/models/question_auto_closer.rb(1 hunks)app/models/question_callbacks.rb(1 hunks)app/views/application/_global_nav.slim(1 hunks)test/models/question_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/initializeAnswer.js
🚧 Files skipped from review as they are similar to previous changes (4)
- test/models/question_test.rb
- app/models/answer_cache_destroyer.rb
- app/models/cache.rb
- app/views/application/_global_nav.slim
🧰 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/answers_controller.rbapp/models/question_auto_closer.rbapp/models/question_callbacks.rbapp/models/question.rbapp/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/answers_controller.rbapp/controllers/api/correct_answers_controller.rb
📚 Learning: 2025-08-31T03:39:07.792Z
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/pair_works/_body.html.slim:85-85
Timestamp: 2025-08-31T03:39:07.792Z
Learning: app/views/pair_works/_body.html.slimテンプレートには2つの独立したif pair_work.solved?条件分岐があり、user変数は最初の分岐でのみ設定されるため、2番目の分岐での参照には注意が必要。
Applied to files:
app/models/question_callbacks.rb
🧬 Code graph analysis (2)
app/models/question_callbacks.rb (2)
app/models/cache.rb (1)
delete_not_solved_question_count(62-64)app/models/question.rb (2)
not_wip?(52-54)unsolved?(56-58)
app/models/question.rb (2)
app/models/question_callbacks.rb (1)
before_destroy(12-17)app/models/cache.rb (1)
not_solved_question_count(55-60)
⏰ 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 (8)
app/models/question_auto_closer.rb (1)
100-107: LGTM!イベントペイロードに
actionフィールドを追加する実装は正しく、他のファイル（コントローラ等）での実装パターンと一貫性があります。__method__を使用してメソッド名を取得する方法も適切です。app/controllers/api/answers_controller.rb (1)
47-55: LGTM!CorrectAnswerの削除時のみイベントを発行する実装は正しく、PRの目的に沿っています。
@answer.destroyの後にNewspaper.publishを実行する順序も適切です（過去のレビューコメントでの議論の通り、destroyされたオブジェクトはfreezeされますが参照可能です）。app/models/question.rb (4)
52-54: LGTM!
not_wip?メソッドの実装はシンプルで明確です。wipがfalseかどうかを判定する実装として適切です。
56-58: LGTM!
unsolved?メソッドの実装は正しく、CorrectAnswerが存在しないかどうかを適切にチェックしています。answers.exists?(type: 'CorrectAnswer')を使用することで、効率的にデータベースクエリを実行できます。
23-23: LGTM!
before_destroyコールバックにprepend: trueを指定することで、関連レコード（dependent: :destroyが指定されたanswersやcorrect_answer）が削除される前にコールバックが実行されるようになっています。これにより、question.unsolved?が正常に動作します。過去のレビューコメントでの議論の通り、この実装は適切です。
94-94: LGTM!
::Cacheを使用してトップレベルのCacheクラスを参照している実装は正しいです。過去のコメントで言及されている通り、act-as-taggable-ongem のTaggable::Cacheモジュールとの名前衝突を避けるために必要な記述です。app/models/question_callbacks.rb (1)
12-17: LGTM!
before_destroyメソッドの実装は正しく、WIPでない未解決の質問が削除される場合にのみキャッシュをクリアする条件分岐が適切に機能しています。question.not_wip? && question.unsolved?という条件は明確で読みやすい実装です。なお、過去のレビューコメントで二重否定を避ける提案がありましたが、現在の実装では
not_wip?とunsolved?という肯定形のメソッド名を使用しているため、可読性の問題はありません。app/controllers/api/correct_answers_controller.rb (1)
24-27: 構文エラーを修正してください。25行目の
answer:,は無効な構文です。ハッシュのキーに対する値が指定されていません。以下のdiffを適用して修正してください：
Newspaper.publish(:answer_save, { - answer:, + answer: answer, action: "#{self.class.name}##{action_name}" })Likely an incorrect or invalid review comment.
Miya096jp
force-pushed
the
bug/use-cache-for-unsolved-badge
branch
from
69e7afc to
422e1db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/models/question_callbacks.rb (1)
13-13: 過去のレビューコメントに従い、二重否定を避けてください。以前のレビューで指摘されたように、
question.not_wip? && question.unsolved?という二重否定は可読性が低いため、early return パターンを使用した方が読みやすくなります。過去のレビューコメントに基づいて修正してください:
- return unless question.not_wip? && question.unsolved? + return if question.wip? || question.solved?注:
solved?メソッドがQuestionモデルに存在しない場合は追加する必要があります(AI サマリーによるとunsolved?は追加されているため、solved?も追加することを推奨します)。
🧹 Nitpick comments (1)
app/models/question_callbacks.rb (1)
8-9: ログとキャッシュ削除の順序を統一してください。
after_saveではログ出力後にキャッシュ削除(8→9行目)、before_destroyではキャッシュ削除後にログ出力(15→16行目)と順序が逆になっています。デバッグ時の混乱を避けるため、順序を統一することを推奨します。統一する場合は、「キャッシュ削除→ログ出力」の順序に揃えることを推奨します:
- Rails.logger.info '[CACHE CLEARED#after_save] Cache destroyed for unsolved question count.' Cache.delete_not_solved_question_count + Rails.logger.info '[CACHE CLEARED#after_save] Cache destroyed for unsolved question count.'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/question.rb(3 hunks)app/models/question_callbacks.rb(1 hunks)test/models/question_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/question.rb
- test/models/question_test.rb
🧰 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/models/question_callbacks.rb
🧠 Learnings (2)
📓 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-08-31T03:39:07.792Z
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/pair_works/_body.html.slim:85-85
Timestamp: 2025-08-31T03:39:07.792Z
Learning: app/views/pair_works/_body.html.slimテンプレートには2つの独立したif pair_work.solved?条件分岐があり、user変数は最初の分岐でのみ設定されるため、2番目の分岐での参照には注意が必要。
Applied to files:
app/models/question_callbacks.rb
🧬 Code graph analysis (1)
app/models/question_callbacks.rb (2)
app/models/cache.rb (1)
delete_not_solved_question_count(62-64)app/models/question.rb (2)
not_wip?(52-54)unsolved?(56-58)
⏰ 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
| def before_destroy(question) | ||
| return unless question.not_wip? && question.unsolved? | ||
|
|
||
| Cache.delete_not_solved_question_count | ||
| Rails.logger.info '[CACHE CLEARED#before_destroy] Cache destroyed for unsolved question count.' | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
条件付きキャッシュクリアのロジックは正しいです。
before_destroy での条件付きキャッシュクリアは、WIP ではなく未解決の質問を削除する場合のみキャッシュをクリアするという意図が明確で、ロジックとして正しいです。ただし、上記のコメントで指摘した可読性の改善を適用してください。
念のため、Question モデルに before_destroy コールバックが正しく登録されているか確認してください(AI サマリーによると prepend: true オプション付きで登録されているはずです):
🏁 Script executed:
#!/bin/bash
# Description: Verify that before_destroy callback is registered in Question model
# Search for the callback registration in Question model
rg -n -A 2 -B 2 'before_destroy.*QuestionCallbacks' app/models/question.rbLength of output: 143
before_destroy コールバック登録を確認
app/models/question.rb の23行目に before_destroy QuestionCallbacks.new, prepend: true が正しく登録されていることを確認しました。引き続き、先述の可読性改善を適用してください。
🤖 Prompt for AI Agents
In app/models/question_callbacks.rb around lines 12 to 17, confirm that the
before_destroy callback is registered in app/models/question.rb at line 23 as
`before_destroy QuestionCallbacks.new, prepend: true`; then refactor this method
for readability: keep the guard/early-return but extract the condition into a
clearly named predicate or local variable (e.g.,
should_clear_not_solved_cache?(question)), call
Cache.delete_not_solved_question_count only when that predicate is true, and
make the log message clearer and consistent (include context such as question.id
and a concise descriptive message) while preserving existing behavior.
\#9128とのコンフリクトを解消
Miya096jp
force-pushed
the
bug/use-cache-for-unsolved-badge
branch
from
422e1db to
a59bf70
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/question_callbacks.rb (1)
12-17: 条件付きキャッシュクリアのロジックは正しいですが、可読性を向上できます。
before_destroyでの条件付きキャッシュクリアは、WIPではなく未解決の質問を削除する場合のみキャッシュをクリアするという意図が正しく実装されています。ただし、過去のレビューコメントでも指摘されているように、条件をより明確にするために述語メソッドに抽出することを検討してください。
例:
def before_destroy(question) return unless should_clear_not_solved_cache?(question) Cache.delete_not_solved_question_count Rails.logger.info '[CACHE CLEARED#before_destroy] Cache destroyed for unsolved question count.' end private def should_clear_not_solved_cache?(question) !question.wip? && question.correct_answer.blank? end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/controllers/api/answers_controller.rb(1 hunks)app/controllers/api/correct_answers_controller.rb(2 hunks)app/javascript/initializeAnswer.js(0 hunks)app/models/answer_cache_destroyer.rb(1 hunks)app/models/cache.rb(1 hunks)app/models/question.rb(2 hunks)app/models/question_auto_closer.rb(1 hunks)app/models/question_callbacks.rb(1 hunks)app/views/application/_global_nav.slim(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/initializeAnswer.js
🚧 Files skipped from review as they are similar to previous changes (4)
- app/views/application/_global_nav.slim
- app/models/question.rb
- app/controllers/api/correct_answers_controller.rb
- app/models/cache.rb
🧰 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/models/question_callbacks.rbapp/models/answer_cache_destroyer.rbapp/controllers/api/answers_controller.rbapp/models/question_auto_closer.rb
🧠 Learnings (2)
📚 Learning: 2025-08-31T03:39:07.792Z
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/pair_works/_body.html.slim:85-85
Timestamp: 2025-08-31T03:39:07.792Z
Learning: app/views/pair_works/_body.html.slimテンプレートには2つの独立したif pair_work.solved?条件分岐があり、user変数は最初の分岐でのみ設定されるため、2番目の分岐での参照には注意が必要。
Applied to files:
app/models/question_callbacks.rb
📚 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/answers_controller.rb
🧬 Code graph analysis (2)
app/models/question_callbacks.rb (1)
app/models/cache.rb (1)
delete_not_solved_question_count(62-64)
app/models/answer_cache_destroyer.rb (1)
app/models/cache.rb (1)
delete_not_solved_question_count(62-64)
⏰ 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 (4)
app/models/answer_cache_destroyer.rb (1)
6-8: デバッグ用のロギング追加が適切です。アクションの抽出とロギングにより、キャッシュクリアがどのコントローラ/モデルから呼び出されたかを追跡できるようになりました。デバッグ時に非常に有用な改善です。
app/controllers/api/answers_controller.rb (1)
47-54: CorrectAnswerの削除時のみキャッシュクリアを実行する条件分岐が正しいです。通常の回答削除時にはキャッシュクリアが不要であり、ベストアンサー削除時のみキャッシュクリアが必要という要件を正しく実装しています。
is_a?(CorrectAnswer)の判定はdestroy後でも正常に動作します（オブジェクトはfreezeされますが参照可能です）。app/models/question_auto_closer.rb (1)
100-105: イベント発行時のアクションフィールド追加が適切です。
__method__を使用してメソッド名を取得し、クラス名と組み合わせてアクションを特定可能にしています。これによりキャッシュクリアの発生源を追跡しやすくなり、デバッグが容易になります。app/models/question_callbacks.rb (1)
8-9: キャッシュクリア時のロギング追加が適切です。
after_saveでのキャッシュクリア時にログを出力することで、質問公開時のキャッシュ操作を追跡可能になりました。
|
お疲れ様です！ コミットの方はなんとか修正&整理してみました。単純にコミットの単位を細かくすることしか考えていなかったので、ここで認識を改められて本当によかったです。 PR作成の経緯の方は、自分の記録として残し、descriptionからは削除しました。 よろしくお願いいたします。 |
ryufuta
left a comment
ryufuta
left a comment
There was a problem hiding this comment.
@Miya096jp
修正ありがとうございます🙏
https://github.com/fjordllc/bootcamp/pull/9232/files#r2435126379 の質問に回答しました。
私からの追加の要望はないのでApproveします👍
|
|
||
| after_save QuestionCallbacks.new | ||
| # before_destroy :clear_question_cache, prepend: true | ||
| before_destroy QuestionCallbacks.new, prepend: true |
There was a problem hiding this comment.
条件の追加を提案しておいてなんですが、質問が削除されること自体が稀なので複雑さを避けるために1でも良いとは思います。
2についてはどういう実装なのか理解できていませんが、複雑そうな印象を持ちました。（現状とあまり変わらない？）
私が考えていた実装は以下です。
実行はしていないので期待通り動作する保証はないです。
（ログ出力のためにAnswerCacheDestroyerの修正は必要）
app/controllers/questions_controller.rb
def destroy
# `@question.correct_answer`を実行するために先にキャッシュを削除
ActiveSupport::Notifications.instrument('question.destroy', question: @question) if !@question.wip? && @question.correct_answer.nil?
@question.destroy
redirect_to questions_url, notice: '質問を削除しました。'
endconfig/initializers/active_support_notifications.rb
ActiveSupport::Notifications.subscribe('question.destroy', AnswerCacheDestroyer.new)上の実装を採用するとしたら #9202 のマージ後でこのPRのレビューも私がしているので私がIssueを作ってそのままPRも担当するのが細かい説明抜きに取り組めて楽で良いと思います。
いずれにせよ、Callbacksをなくすリファクタリングにどこまでチームとして力を入れるか次第かもしれませんし、この後の大倉さんのレビューで現状で問題ないということになればそのままで良いのかもしれません。
|
@ryufuta @okuramasafumi |
| @@ -3,6 +3,8 @@ | |||
| class AnswerCacheDestroyer | |||
| def call(payload) | |||
| _answer = payload[:answer] | |||
There was a problem hiding this comment.
このPRには関係ないですが、この変数は使われていないように見えますね。
There was a problem hiding this comment.
私もここはあえて書かなくてもいいのではと思って読んでいました。
There was a problem hiding this comment.
この後のPRの 66cd818 で回答のidをログに含めるために利用するよう変更しているのでここではそのままにして良いかなと思っていました。
|
@okuramasafumi @komagata |
|
@Miya096jp #9232 (comment) でキャッシュ削除処理をコールバックから 後のメンバーのためにもミーティングで話した内容を残しておきます。
|
|
ご確認ありがとうございます。お返事が遅くなってしまって申し訳ありません🙇🏻♂️ 確かに、キャッシュ削除に関してはDB処理と意味的にもタイミング的にも関わりが深いので、コールバックにまとめた方が良さそうですね。通知のように後々何かが変わる可能性とかもないでしょうし。 この件は、良い勉強にもなりそうですし、私の方でIssue立てておきます。 |
|
@Miya096jp |
|
うーん確かに、やっと置き換えたばかりですもんね。今すぐ直す必要性もないし、後の人に任せた方がいいかもしれませんね。 QuestionCallbascksの整理の件は、ちょっと考えて決めてみます。後はお任せください。いろいろとサポートしていただいて大変助かりました🙏 |
4 participants
#9221からの移行
ブランチ名の修正のため#9221 はクローズし、こちらで作業を継続します。
Issue
Q&A画面の未解決タブのバッジにキャッシュを使う #8990
概要
Q&Aページの未解決タブ、およびグローバルメニューのQ&Aに、未解決質問数のバッジが表示されるようになっています。このPRでは質問数のキャッシュ/キャッシュクリアの実装周りの以下のバグを修正しました。
グローバルメニューのバッジは全てのユーザーに対して表示されますが、Q&Aページの未解決タブのバッジは管理者・メンターでログインした場合のみ表示されます。
キャッシュ未使用のバグ
キャッシュクリアのバグ
関連Issue
newspaper gem置き換え - 回答・正解通知システム #9151
Q&Aを自動的にクローズする機能のバグ修正 #9024
事前準備
動作検証
ログ監視コマンド
検証項目
次の操作を行い、ログ出力をチェックしてください。
↳ 決定後リロード → [CACHE MISS]
↳ 取り消し後リロード → [CACHE MISS]
↳ 削除後リロード → [CACHE MISS]
補足: 質問作成/削除時のキャッシュクリアについて
Questionモデルのafter_saveとafter_destroyにQuestionCallbacksが指定されており、その中で直接キャッシュクリアされています。この部分についてはバグが隠れる複雑性もないと考えられるのでログは追加していません。処理の流れ:質問作成/削除 -> キャッシュクリア -> リダイレクト -> キャッシュ追記: より確実な検証のため、新たにログを追加しました。
質問の自動クローズ時の検証
TOKEN=hogeを追記http://localhost:3000/scheduler/daily/auto_close_questions?token=hogeにアクセスし定期処理を手動実行↳ ログ出力:
[AnswerCacheDestroyer] QuestionAutoCloser.publish_events↳ ログ出力:
[CACHE MISS]Summary by CodeRabbit
リリースノート
パフォーマンス改善
内部改善