NewsPaperからActiveSupport::Notificationsに3クラス分実装を置き換える#8935
Conversation
Walkthroughユーザー作成時の Changes
Sequence Diagram(s)sequenceDiagram
participant C as UsersController
participant DB as User / DB
participant OldN as SignUpNotifier (removed)
rect rgba(200,230,255,0.4)
Note right of C: 旧フロー（削除）
C->>OldN: instrument 'user.create' (payload: user)
OldN->>DB: set unsubscribe_email_token on user
C->>DB: save user
end
rect rgba(200,255,200,0.3)
Note right of C: 新フロー（現在）
C->>C: generate unsubscribe_email_token (SecureRandom.urlsafe_base64)
C->>DB: assign token & save user
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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/users_controller.rbrubocop-minitest extension supports plugin, specify Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
thmz337
force-pushed
the
chore/replace-creator-and-notifier
branch
from
a3932b2 to
2228e3c
Compare
|
@thmz337 |
|
ありがとうございます！以後忘れないようにします。 |
|
お疲れ様です！こちらレビュー可能でしょうか？ |
|
レビューさせていただきます👍 土日祝が稼働できない可能性があるので、お返しできるのが週明けになると思います🙏 |
|
お疲れ様です！ご確認ありがとうございます。 |
There was a problem hiding this comment.
@thmz337
レビューしました！
動作自体はOKでした！
コメントを1件残したのでご確認ください。
その他でもう2点確認したいことがあり、以下に書いてありますのでご確認ください。
私の指摘した事項について、わからない点、間違っている点等ありましたら遠慮なくおっしゃってください🙏
前提
現在のSignUpNotifierクラスは
「ユーザー登録時に通知を行うクラス」ではなく、「メール配信停止用の一意なトークンを生成して、ユーザーオブジェクトの属性に設定するクラス」ではないかと思います。
例えば手元でActiveSupport::Notifications.subscribe('user.create', SignUpNotifier.new)をコメントアウトしても、ユーザー登録時の通知は有効になっているようでした。
実際にユーザー登録時の通知を出しているのはこの辺かなと思います。
確認したいこと1
まず、
SignUpNotifierの通知確認
- トップ画面から参加登録を押下し、ユーザー登録を行う。
- 登録を行ったユーザー以外でログインし、通知が来ていることを確認する。
という確認方法では、前述した通り'user.create'イベントが発行されて、それが適切に購読されているかどうかの判別はつかないので、記載を変更した方が良いのかなと思いました。
私はSignUpNotifierクラスのcallメソッド定義内にRails.logger.info "[Debug] SignUpNotifierが呼ばれました"と書いて確認しました。
確認したいこと2
今回のissueはnewspaperをActiveSupport::Notificationsに置き換えるという目的なので、この実装でも問題はないと思いますが、通知機能を備えていないSignUpNotifierをそのまま残しておいていいのか疑問に思いました。
また、本来ActiveSupport::Notifications.instrument('user.create', user: @user)のようなイベントは@user.saveの後で発行される方が良いのではと思いますが、この実装では@user.saveの前に呼ばれています。
これはuser.unsubscribe_email_token = SecureRandom.urlsafe_base64を@user.saveの前に処理したいからだと思いますが、この辺はsub/pub機構の使い方としておかしいのでは?🤔と思いました。
今後の対応としては
- このPRでは何もしないが別のissueを立てる。
- このPRで修正を加える。
例えば、SignUpNotifierクラスを削除してcreateアクション内の68行目くらいに
@user.unsubscribe_email_token = SecureRandom.urlsafe_base64と書いてしまう等 - 何もしない
等があると思いますが、対応についてのご意見を賜りたいです🙏
| link: "/regular_events/#{@regular_event.id}", | ||
| kind: Notification.kinds[:regular_event_updated] | ||
| ) | ||
|
|
There was a problem hiding this comment.
この空行の削除はリファクタリングと捉えていいですか？
意図しない修正なら戻しておいた方がいいのかなと思ったので一応の確認です🙏
There was a problem hiding this comment.
ご指摘ありがとうございます。こちらは私のミスです。
変更前の状態に戻しました。
thmz337
force-pushed
the
chore/replace-creator-and-notifier
branch
2 times, most recently
from
61b2f32 to
90f93a6
Compare
|
お疲れ様です！ご確認ありがとうございました。 確認したいこと1おっしゃる通りです。変更確認方法を修正しました。 確認したいこと2こちらもおっしゃる通りかと思います。今回、 以上ご確認よろしくお願いいたします。 |
|
@thmz337 |
|
@thmz337 私としては、SignUpNotifierを削除したので動作確認の必要は無いとの認識でした。 |
|
お疲れ様です。ご確認ありがとうございました。
こちらについて、動作確認を残した意図としては、トークンの生成と設定を 以上ご確認よろしくお願いいたします。 |
|
@thmz337
動作確認してまたご連絡します〜 |
|
すいません、モデルではなくてコントローラですね。 |
|
ありがとうございます！ ご確認よろしくお願いいたします。 |
|
@thmz337 conflictの修正をお願いします。 |
thmz337
force-pushed
the
chore/replace-creator-and-notifier
branch
from
90f93a6 to
22715ce
Compare
|
修正しましたので、よろしくお願いいたします。 |
|
@thmz337 conflictの修正をお願いします。 |
thmz337
force-pushed
the
chore/replace-creator-and-notifier
branch
2 times, most recently
from
ec263c8 to
629504f
Compare
|
お疲れ様です。conflictを修正しましたので、ご確認よろしくお願いいたします。 |
|
@thmz337 テストが失敗しているようです。テストが成功していることを確認してからレビュー依頼するようにお願います〜。 |
|
失礼しました。テスト成功後ご連絡させていただきます。 |
thmz337
force-pushed
the
chore/replace-creator-and-notifier
branch
from
629504f to
cd6e551
Compare
|
お疲れ様です。こちらご確認をお願いできますでしょうか？ |
| @user.uploaded_avatar = user_params[:avatar] | ||
|
|
||
| ActiveSupport::Notifications.instrument('user.create', user: @user) | ||
| @user.unsubscribe_email_token = SecureRandom.urlsafe_base64 |
There was a problem hiding this comment.
お疲れ様です。ご確認ありがとうございます。
こちらの処理でSignUpNotifierを呼び出しているのですが、行っている処理はメール配信停止用のトークンを発行して、ユーザーに設定を行うことです。
@tyrrell-IH さんともやり取りをしたのですが、クラス名と異なり通知機能を備えていない（通知は別の箇所で行っています）ため、メール配信停止トークンの設定は別途切り出し、
( @user.unsubscribe_email_token = SecureRandom.urlsafe_base64の箇所です)該当箇所を削除しました。
以上よろしくお願いいたします。
thmz337
force-pushed
the
chore/replace-creator-and-notifier
branch
from
cd6e551 to
fd09bb9
Compare
|
RegularEventUpdateNotifier のステージング確認ができました🎉 
|
|
早速のご対応ありがとうございます！ |
|
@thmz337 
|
|
ありがとうございます！ |
4 participants
Issue
概要
下記の2クラスの実装を、newspaperからActiveSupport::Notificationsに置き換えました。
また、下記のクラスを削除しました。
変更確認方法
chore/replace-creator-and-notifierをローカルに取り込むTimesChannelCreatorのログ確認[Discord API] #{ユーザー名}の分報チャンネルが作成できませんでした。というアラートが出ていることを確認する。(開発環境では分報チャンネルを作成できないため)komagata)でログインする。[Discord API] #{ユーザー名}の分報チャンネルが作成できませんでした。というアラートが出ていることを確認する。RegularEventUpdateNotifierのメール確認SignUpNotifier削除後のメール通知解除動作確認komagataでログインし、コメントに返信をする。メールでの通知オフはこちらからをクリックしメールの配信停止を行う。Summary by CodeRabbit
新機能
バグ修正
リファクタリング