3クラス分のnewspaperをActiveSupport::Notificationsに移行#8847
Conversation
|
""" Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller
participant ASNotifications as ActiveSupport::Notifications
participant Subscriber1 as AnswerNotifier / NotifierToWatchingUser / EventOrganizerWatcher / RegularEventOrganizerWatcher
Controller->>ASNotifications: instrument("answer.create" or "event.create" or "regular_event.create", payload)
ASNotifications-->>Subscriber1: call(_name, _started, _finished, _unique_id, payload)
Subscriber1->>Subscriber1: 通知/監視処理を実行
Possibly related issues
Possibly related PRs
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.75.5)app/controllers/regular_events_controller.rbrubocop-minitest extension supports plugin, specify app/models/regular_event_organizer_watcher.rbrubocop-minitest extension supports plugin, specify config/initializers/active_support_notifications.rbrubocop-minitest extension supports plugin, specify 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
e-yanagita-gs
changed the title
|
お疲れ様です。 こちらのIssueに関連して、実装方針の確認でお時間をいただきたくコメントいたしました。 発生した問題の概要タスク対象の3クラスの移行を完了しテストを実行したところ、直接修正していない エラーの根本原因デバッグを進めた結果、原因は 以前のコードでは、この間違った通知を しかし、今回のリファクタリングで 提案する解決策この問題を根本的に解決し、コードをより堅牢で分かりやすい状態にするため、以下の対応を実装したいと考えております。
この対応により、 ご相談この対応は、新しいクラスを作成することになり、当初のタスク「既存3クラスの置き換え」の範囲を少し超えるものと認識しております。 つきましては、アプリケーションの健全性を高めるために、上記方針で実装を進めてもよろしいか、ご確認いただけますと幸いです。 お忙しいところ恐れ入りますが、よろしくお願いいたします。 |
|
@e-yanagita-gs なるほどです。仰っている形で対応お願いします。 |
|
@komagata |
|
@zamami |
|
@e-yanagita-gs |
|
@zamami |
zamami
left a comment
zamami
left a comment
There was a problem hiding this comment.
@e-yanagita-gs
レビュー遅くなって申し訳ありません。
こちらの内容を参考に問題なく実装されていると思います。
動作確認も無事できました！
エラーのデバッグお疲れ様でした。
解決策の提案、相談内容は読んでてとても勉強になりました。
今後のPRのやりとりでの参考にさせていただきます。
私の方からは特に問題ないと思いますのでApproveとさせていただきます！
|
@komagata |
3 participants
Issue
概要
アプリケーション内でPub/Sub（出版/購読）機構を担っている外部gem
newspaperへの依存を削減するため、Rails標準機能であるActiveSupport::Notificationsへ置き換えるリファクタリングを行いました。今回は、以下の3つの購読者（Subscriber）クラス及び、関連するイベント発行部分を対象としました。
EventOrganizerWatcherAnswerNotifierNotifierToWatchingUser変更確認方法
chore/replace-to-activesupport-notificationsブランチをローカルに取り込みます。foreman start -f Procfile.devでサーバーを立ち上げます。1.
EventOrganizerWatcherの動作確認イベントを新規作成した際に、そのイベントの作成者自身を、そのイベントの「ウォッチャー」（更新通知を受け取る人）として自動的に登録する機能です。
この自動登録が正しく行われ、その結果、他のユーザーからのコメント通知が作成者に届くことを確認します。
確認項目:
/notifications）にアクセスし、「（あなたのイベント名）にユーザーBさんがコメントしました」という趣旨の通知が届いていることを確認する。2.
AnswerNotifierの動作確認質問に新しい回答がついた時に、その質問の投稿者以外の関係者（主にウォッチャー）に「新しい回答がありました」という通知を送る機能です。
質問をウォッチしているユーザー（質問者ではない）に対して、この通知が正しく届くことを確認します。
確認項目:
/notifications）にアクセスし、「（ユーザーAさんの質問）にユーザーCさんが回答しました」という趣旨の通知が届いていることを確認する。3.
NotifierToWatchingUserの動作確認質問に新しい回答がついた時に、より条件を絞って「純粋なウォッチャー」にだけ通知を送る機能です。通知が重複しないよう、ウォッチャーが「回答者自身」や「回答文中でメンションされた人」である場合は、通知対象から除外します。
「純粋なウォッチャー」には通知が届き、「回答者でもあるウォッチャー」には（この機能からは）通知が届かない、という除外ロジックが正しく働くことを確認します。
NotifierToWatchingUserからの「ウォッチ中の質問に新しい回答がありました」という趣旨の通知は届いていないこと。※変更前（
mainブランチなど）の挙動と、変更後の挙動が同じであることをご確認ください。Screenshot
※内部的なリファクタリングであり、見た目の変更はないためスクリーンショットはありません。
Summary by CodeRabbit