Conversation
WalkthroughNewspaperによるイベント発行・購読をActiveSupport::Notificationsへ移行。コントローラとモデルのpublish呼び出しをinstrumentに変更し、イニシャライザでの購読先をAS::Notificationsに再登録。各Notifierのcallシグネチャを5引数形式に対応させ、既存の処理内容は維持。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Controller as Controller (create/update)
participant AS as ActiveSupport::Notifications
participant Notifier as *Notifier
User->>Controller: 操作（例：正答作成/卒業更新/カムバック）
Controller->>AS: instrument("*.save|*.update", payload)
Note over AS: 非同期/同期で購読者に配信
AS->>Notifier: call(name, started, finished, id, payload)
Notifier->>Notifier: payloadから対象抽出・通知処理
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)config/initializers/active_support_notifications.rbrubocop-minitest extension supports plugin, specify app/controllers/comeback_controller.rbrubocop-minitest extension supports plugin, specify app/controllers/graduation_controller.rbrubocop-minitest extension supports plugin, specify
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
ryufuta
force-pushed
the
chore/replace-newspaper-in-correct_answer-graduation-comeback
branch
2 times, most recently
from
4e3bdae to
6b5dde6
Compare
ryufuta
force-pushed
the
chore/replace-newspaper-in-correct_answer-graduation-comeback
branch
from
6b5dde6 to
e206393
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/correct_answers_controller.rb (1)
18-22: NameErrorの致命的バグ:@answerが未定義
update内でanswerローカルを更新後に@answerを参照しています。実行時に未定義参照になります。ローカル変数に合わせて修正してください。def update answer = @question.answers.find(params[:answer_id]) answer.update!(type: '') - Newspaper.publish(:answer_save, { answer: @answer }) + Newspaper.publish(:answer_save, { answer: answer }) end
🧹 Nitpick comments (1)
app/controllers/graduation_controller.rb (1)
12-12: イベント発火位置が適切更新成功後に
graduation.updateを発火しており一貫しています。必要に応じて「確定後」を保証したい場合はafter_commitでの発火も検討ください（任意）。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/controllers/api/correct_answers_controller.rb(1 hunks)app/controllers/comeback_controller.rb(1 hunks)app/controllers/graduation_controller.rb(1 hunks)app/models/comeback_notifier.rb(1 hunks)app/models/correct_answer_notifier.rb(1 hunks)app/models/graduation_notifier.rb(1 hunks)app/models/question_auto_closer.rb(1 hunks)config/initializers/active_support_notifications.rb(2 hunks)config/initializers/newspaper.rb(0 hunks)
💤 Files with no reviewable changes (1)
- config/initializers/newspaper.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/controllers/api/correct_answers_controller.rbapp/models/question_auto_closer.rbapp/controllers/graduation_controller.rbapp/models/graduation_notifier.rbapp/models/comeback_notifier.rbapp/models/correct_answer_notifier.rbconfig/initializers/active_support_notifications.rbapp/controllers/comeback_controller.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
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 を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
Learnt from: komagata
PR: fjordllc/bootcamp#9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
app/controllers/graduation_controller.rbapp/controllers/comeback_controller.rb
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
app/controllers/graduation_controller.rbapp/controllers/comeback_controller.rb
🔇 Additional comments (6)
app/models/question_auto_closer.rb (1)
100-103: 削除不可 — :answer_save の購読が残っているため Newspaper.publish は消さないでくださいconfig/initializers/newspaper.rb に Newspaper.subscribe(:answer_save, answer_cache_destroyer) が存在します。かつ以下で Newspaper.publish(:answer_save, ...) が使われています:
- app/models/question_auto_closer.rb:101
- app/controllers/api/correct_answers_controller.rb:10,21
- app/controllers/api/answers_controller.rb:33,42
対応方針: 購読者（AnswerCacheDestroyer 等）を先に ActiveSupport::Notifications 側へ移行し、初期化ファイルの subscribe を置換してから Newspaper.publish を削除すること。不要確認が取れるまで旧経路は残してください。
Likely an incorrect or invalid review comment.
app/models/correct_answer_notifier.rb (1)
4-4: AS::Notificationsの5引数シグネチャ対応、問題なし購読コールバックの引数順・未使用変数のプレフィックス運用ともに適切です。
config/initializers/active_support_notifications.rb (1)
7-7: 新規サブスクリプションの追加、妥当ですイベント名・購読先ともに目的に合致しています。既存と同様
to_prepare内での登録ですが、開発環境の再読み込みで重複登録しないかだけ念のため確認ください（本プロジェクトの既存方針に倣うならこのままでOK）。Also applies to: 34-35
app/controllers/comeback_controller.rb (1)
15-15: イベント発火の順序が妥当
comeback!直後にcomeback.updateを発火しており、Notiferの前提とも整合しています。app/models/comeback_notifier.rb (1)
4-4: 5引数シグネチャへの対応OKペイロードの取り扱いのみで十分で、互換性も問題ありません。
app/models/graduation_notifier.rb (1)
4-4: 5引数シグネチャへの対応OK既存ロジックを保ったままインタフェースだけ揃えられています。
| if @answer.save | ||
| Newspaper.publish(:answer_save, { answer: @answer }) | ||
| Newspaper.publish(:correct_answer_save, { answer: @answer }) | ||
| ActiveSupport::Notifications.instrument('correct_answer.save', answer: @answer) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
二重イベント発火の可能性（Newspaper + Notifications）
Newspaper.publish(:answer_save, …)とinstrument('correct_answer.save', …)の併用は意図的ですか？移行完了後は旧経路を外すほうが安全です。少なくとも重複通知が発生していないか確認をお願いします。
削除する場合の最小差分:
- Newspaper.publish(:answer_save, { answer: @answer })
ActiveSupport::Notifications.instrument('correct_answer.save', answer: @answer)確認用:
🏁 Script executed:
#!/bin/bash
rg -n "Newspaper\.subscribe\([^)]*answer_save" -C2Length of output: 390
Newspaper.subscribe(:answer_save) が残っています — publish の削除は危険です（要修正）
config/initializers/newspaper.rb:5 に Newspaper.subscribe(:answer_save, answer_cache_destroyer) が存在します。コントローラから Newspaper.publish(:answer_save, ...) を削除すると AnswerCacheDestroyer（キャッシュ破棄等）が呼ばれなくなります。
対応案（いずれかを選択）:
- Newspaper.publish を維持する（既存サブスクライバに依存）。
- AnswerCacheDestroyer を ActiveSupport::Notifications ('correct_answer.save' 等) に移行してから config/initializers/newspaper.rb の subscribe を削除／更新。
🤖 Prompt for AI Agents
In app/controllers/api/correct_answers_controller.rb around line 11, removing
Newspaper.publish(:answer_save, ...) breaks the existing
Newspaper.subscribe(:answer_save, answer_cache_destroyer) defined in
config/initializers/newspaper.rb so AnswerCacheDestroyer no longer runs; either
restore the Newspaper.publish(:answer_save, answer: @answer) call here to keep
current subscribers working, or migrate the AnswerCacheDestroyer to listen to
ActiveSupport::Notifications by subscribing to the 'correct_answer.save' event
(update AnswerCacheDestroyer to accept the AS::Notifications payload and change
the initializer to remove or update the Newspaper.subscribe accordingly); ensure
the chosen approach preserves the event payload shape and side effects (cache
invalidation) and update tests if needed.
There was a problem hiding this comment.
@coderabbitai
削除したのはNewspaper.publish(:correct_answer_save, { answer: @answer })です。
Newspaper.publish(:answer_save, { answer: @answer })は削除していないので問題ありません。
There was a problem hiding this comment.
@ryufuta その通りですね。申し訳ありませんでした。
diffを再確認すると、削除されているのは確かに :correct_answer_save イベントのみで、:answer_save イベントは残されていることが分かります。つまり既存の AnswerCacheDestroyer は引き続き正常に動作し、キャッシュ無効化処理も維持されます。
私の前回の分析が間違っていました。この実装は問題ありません。
🧠 Learnings used
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
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 を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
Learnt from: komagata
PR: fjordllc/bootcamp#9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
ryufuta
left a comment
ryufuta
left a comment
There was a problem hiding this comment.
@coderabbitai
#9015 (review) のCaution内の指摘については #9155 で既に対応中なので本PRでは対応しません。
|
@matuaya |
|
@ryufuta |
matuaya
left a comment
matuaya
left a comment
There was a problem hiding this comment.
遅くなってしまい申し訳ありません💦
動作バッチリ問題ありませんでした😊Approveさせていただきます！
|
@matuaya |
|
@komagata |
komagata
deleted the
chore/replace-newspaper-in-correct_answer-graduation-comeback
branch
3 participants
Issue
概要
下記の3クラスの実装をnewspaperからActiveSupport::Notificationsに置き換えました。
変更確認方法
chore/replace-newspaper-in-correct_answer-graduation-comebackをローカルに取り込むforeman start -f Procfile.devでローカルサーバーを起動CorrectAnswerNotifier
Q&Aの質問作成者がベストアンサーを選択すると、質問作成者以外の「Watch中」のユーザーに通知が届くことを確認します。
Q&Aを自動的にクローズした際にもベストアンサー決定の通知が飛ぶため、こちらも確認します。
定期処理を実行できるようにするためローカル環境に環境変数
TOKENを設定しておく必要があります。ローカル環境に環境変数を設定するには
dotenv-railsというgemを使用すると便利です。以下はこのgemを使用する場合の手順です。
（
Gemfileに追記済みのため改めてインストールする必要はない）.env.localファイルを作成TOKEN=hogeを追記以下は自動的にクローズした際のベストアンサー決定の通知の確認手順です。
GraduationNotifier
管理者が現役生のステータスを「卒業」にすると、管理者とメンターに通知が届くことを確認します。
※ #9114 のバグが原因でkimuraとhatsunoを卒業させようとするとエラーが発生するためその他の現役生で動作確認を実施してください。
ComebackNotifier
休会中のユーザーが復帰すると、管理者とメンターに通知が届くことを確認します。
ログインページの「休会からの復帰」リンクから遷移できます
Screenshot
リファクタリングのためUIの変更はありません。
Summary by CodeRabbit