Conversation
Walkthroughイベント通知の仕組みを Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller (Inquiries/Works)
participant ActiveSupport as ActiveSupport::Notifications
participant Notifier as Notifier (各種 Notifierクラス)
Controller->>ActiveSupport: instrument('came.inquiry' or 'work.destroy', payload)
ActiveSupport->>Notifier: call(_name, _started, _finished, _unique_id, payload)
Notifier->>Notifier: payloadを処理
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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)app/controllers/works_controller.rbrubocop-minitest extension supports plugin, specify app/controllers/inquiries_controller.rbrubocop-minitest extension supports plugin, specify app/models/inquiry_notifier.rbrubocop-minitest extension supports plugin, specify
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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)
app/models/work_notification_destroyer.rb (1)
5-8: エラーハンドリングの追加を検討現在の実装では
payload[:work]がnilの場合や、work.idが期待される形式でない場合のエラーハンドリングがありません。def call(_name, _started, _finished, _unique_id, payload) work = payload[:work] + return if work.nil? Notification.where(link: "/works/#{work.id}").destroy_all endapp/models/inquiry_notifier.rb (1)
10-14: 送信者ユーザーの存在チェックを追加することを検討現在の実装では
NOTIFICATION_SENDER_NAMEで指定されたユーザーが存在しない場合、senderがnilになる可能性があります。sender = User.find_by(login_name: NOTIFICATION_SENDER_NAME) +return if sender.nil? + User.admins.each do |receiver| ActivityDelivery.with(inquiry:, receiver:, sender:).notify(:came_inquiry) end
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/inquiries_controller.rb(1 hunks)app/controllers/reports_controller.rb(2 hunks)app/controllers/works_controller.rb(1 hunks)app/models/inquiry_notifier.rb(1 hunks)app/models/report_notifier.rb(1 hunks)app/models/work_notification_destroyer.rb(1 hunks)config/initializers/active_support_notifications.rb(1 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
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (4)
📓 Common learnings
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
config/initializers/active_support_notifications.rb (1)
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
app/controllers/works_controller.rb (1)
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/notifiers/activity_notifier.rb:357-373
Timestamp: 2025-07-01T04:30:19.565Z
Learning: matching_pair_workメソッドは、ペア確定時に送信される通知のため、pair_work.buddy_idは必ず存在することがビジネスロジックで保証されている。そのため、User.find(pair_work.buddy_id)でのエラーハンドリングは不要。
app/models/work_notification_destroyer.rb (1)
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
🔇 Additional comments (11)
app/models/report_notifier.rb (1)
4-4: ActiveSupport::Notificationsの仕様に適合したメソッドシグネチャに正しく更新されていますメソッドシグネチャが
ActiveSupport::Notificationsのsubscriberインターフェースに適合するよう正しく更新されています。使用しないパラメータにアンダースコアのプレフィックスを付けているのも良い実装です。app/controllers/inquiries_controller.rb (1)
16-16: NewspaperからActiveSupport::Notificationsへの移行が正しく実装されていますイベント発行方法が
Newspaper.publishからActiveSupport::Notifications.instrumentに正しく変更され、イベント名もドット記法の'came.inquiry'に統一されています。config/initializers/active_support_notifications.rb (1)
9-12: 新しいActiveSupport::Notificationsサブスクリプションが正しく追加されていますRailsアプリケーションリローダーブロック内に適切に配置され、イベント名もコントローラーで使用されているものと一致しています。既存のサブスクリプションパターンに従った実装です。
app/controllers/works_controller.rb (2)
47-47:work.destroyイベントの移行が正しく実装されています
Newspaper.publishからActiveSupport::Notifications.instrumentへの変更が正しく実装されています。
30-30: Newspaper.publish(:work_create) の使用は意図的です
config/initializers/newspaper.rbに以下のようにサブスクライブ定義が残っているため、createアクションでNewspaper.publish(:work_create, …)を呼び出しているのは仕様どおりです。移行漏れではなく、work_createイベントはまだNewspaper経由で通知される設計となっています。
- config/initializers/newspaper.rb:
Newspaper.subscribe(:work_create, WorkNotifier.new)よって、このまま
Newspaper.publishを使い続けて問題ありません。app/controllers/reports_controller.rb (3)
62-62:report.createイベントの追加が正しく実装されています
ActiveSupport::Notifications.instrumentを使用した新しいイベント発行が適切に追加されています。
79-79:report.updateイベントの追加が正しく実装されています
ActiveSupport::Notifications.instrumentを使用した新しいイベント発行が適切に追加されています。
62-64: Newspaper.publishとActiveSupport::Notificationsの並行発行は移行フェーズで意図的かご確認ください現在、
app/controllers/reports_controller.rbの以下箇所で両方のイベント発行が残っています。これは意図的な移行期間中の実装でしょうか？• create アクション（62–64行目）
• update アクション（79–81行目）加えて以下点をご確認・ご検討ください：
- 移行完了後に
Newspaper.publishを削除するタイミング／計画の有無- 全イベントリスナーのActiveSupportへの移行状況
- README等ドキュメントへの移行方針の明記
app/models/work_notification_destroyer.rb (1)
4-4: ActiveSupport::Notificationsの標準的なsubscriberインターフェースに正しく対応メソッドシグネチャがActiveSupport::Notificationsの標準形式に正しく更新されています。未使用パラメータにアンダースコアプレフィックスを使用している点も適切です。
app/models/inquiry_notifier.rb (2)
6-6: ActiveSupport::Notificationsのsubscriberインターフェースに正しく適応メソッドシグネチャがActiveSupport::Notificationsの標準形式に適切に更新されています。
8-8: 適切なnilチェック
inquiryオブジェクトのnilチェックが適切に実装されており、防御的プログラミングの良い例です。
|
@matuaya |
|
|
Tip For best results, initiate chat on the files or code changes. @jun-kondo さん、詳細なご説明ありがとうございます！
このような業務ロジックの背景を教えていただき、とても参考になりました。今後のレビューでより適切な提案ができるよう、この情報を記録させていただきます。 ✏️ Learnings added
🧠 Learnings used |
jun-kondo
force-pushed
the
chore/replace-newspaper-with-activesupport_notifications-in-reportnotifier-worknotificationdestroyer-inquirynotifier
branch
from
9d8c0a4 to
ba4dc91
Compare
|
@jun-kondo |
|
@matuaya |
matuaya
left a comment
matuaya
left a comment
There was a problem hiding this comment.
確認しました！
Approveさせていただきます😊
komagata
deleted the
chore/replace-newspaper-with-activesupport_notifications-in-reportnotifier-worknotificationdestroyer-inquirynotifier
branch
3 participants
Issue
概要
下記の3クラスの実装を、newspaperからActiveSupport::Notificationsに置き換えました。
変更確認方法
事前準備
ブランチをローカルに取り込み切り替え
ローカルサーバーの立ち上げ
ログインユーザーを何度も切り替えるので
any_loginの機能を使うことをおすすめします。下記使用ユーザーのログインネーム。パスワードはいずれも
testtestReportNotifierでのイベント動作確認
ReportNotifierでのイベント動作確認
ReportNotifierはユーザーが日報投稿したときに通知を行うクラスhttp://localhost:3000/
hajimeでログインし、日報を今日の日付で作成、本文に@kimuraを含めて提出kimuraでログインするWorkNotificationDestroyerでのイベント動作確認
WorkNotificationDestroyerでのイベント動作確認
WorkNotificationDestroyerは投稿したポートフォリオをユーザーが削除したときに、ポートフォリオ投稿の通知を削除するクラスhttp://localhost:3000/
kimuraでログインhttp://example.comで入力kimuraがポートフォリオを追加した通知があることを確認kimuraでログインして、2.で追加したポートフォリオを削除InquiryNotifierでのイベント動作確認
InquiryNotifierでのイベント動作確認
InquiryNotifierは問い合わせフォームから問い合わせがあった際、管理者側に通知を行うクラスhttp://localhost:3000/
/inquiry/newでお問い合わせフォームのページに移動Screenshot
内部的な変更なのでスクリーンショットはありません。
Summary by CodeRabbit
リファクタリング
その他