3クラス分のnewspaperをActiveSupport::notificationsに置換#8973
Conversation
|
""" Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant ActiveSupport
participant Notifier
Controller->>ActiveSupport: Notifications.instrument('event.name', payload)
ActiveSupport->>Notifier: call(_name, _started, _finished, _unique_id, payload)
Notifier->>Notifier: payloadを処理
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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/regular_events_controller.rbrubocop-minitest extension supports plugin, specify app/models/sign_up_notifier.rbrubocop-minitest extension supports plugin, specify app/controllers/users_controller.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 (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ 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 (1)
app/models/sign_up_notifier.rb (1)
4-4: パラメータ名の不整合を修正してくださいメソッドシグネチャの変更は適切ですが、
_started_パラメータに末尾のアンダースコアが余分に付いています。他のパラメータやTimesChannelCreatorとの一貫性を保つため、_startedに修正することを推奨します。- def call(_name, _started_, _finished, _unique_id, payload) + def call(_name, _started, _finished, _unique_id, payload)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/regular_events_controller.rb(1 hunks)app/controllers/users_controller.rb(3 hunks)app/models/regular_event_update_notifier.rb(1 hunks)app/models/sign_up_notifier.rb(1 hunks)app/models/times_channel_creator.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)config/initializers/newspaper.rb(0 hunks)test/models/times_channel_creator_test.rb(2 hunks)
💤 Files with no reviewable changes (1)
- config/initializers/newspaper.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/controllers/regular_events_controller.rbconfig/initializers/active_support_notifications.rbapp/controllers/users_controller.rbapp/models/sign_up_notifier.rbapp/models/times_channel_creator.rbapp/models/regular_event_update_notifier.rbtest/models/times_channel_creator_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/times_channel_creator_test.rb
🧠 Learnings (5)
📓 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
app/controllers/regular_events_controller.rb (1)
Learnt from: reckyy
PR: #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: #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/users_controller.rb (2)
Learnt from: reckyy
PR: #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: reckyy
PR: #8356
File: db/fixtures/users.yml:823-826
Timestamp: 2025-07-15T12:33:04.885Z
Learning: fjordllc/bootcampプロジェクトでは、trainee: trueは「研修に所属していたユーザー」全体を示すフラグであり、研修中のユーザーと研修を終了したユーザーの両方を含む。そのため、trainee: trueとtraining_completed_atの併存は意図された仕様であり、排他的な関係ではない。
app/models/sign_up_notifier.rb (1)
Learnt from: reckyy
PR: #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が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
🧬 Code Graph Analysis (2)
app/controllers/users_controller.rb (1)
app/models/user.rb (1)
student?(641-643)
test/models/times_channel_creator_test.rb (1)
app/models/user.rb (1)
student_or_trainee?(690-692)
🔇 Additional comments (12)
app/models/times_channel_creator.rb (1)
4-4: ActiveSupport::Notificationsの標準的なメソッドシグネチャへの変更が適切ですNewspaperからの移行に伴い、ActiveSupport::Notificationsの標準的な5つのパラメータ（
_name,_started,_finished,_unique_id,payload）を受け取るように変更されています。使用しないパラメータにアンダースコアプレフィックスを付けるのはRubyの慣例に従っており、適切な実装です。app/controllers/regular_events_controller.rb (1)
47-47: Newspaperからの移行が適切に実装されています
Newspaper.publish(:regular_event_update, ...)からActiveSupport::Notifications.instrument('regular_event.update', ...)への変更が正しく行われています。イベント名がシンボルからドット区切りの文字列に変更され、ActiveSupport::Notificationsの慣例に従っています。ペイロードの構造は維持されているため、既存の通知ロジックとの互換性が保たれています。config/initializers/active_support_notifications.rb (1)
9-11: 新しいActiveSupport::Notifications購読が適切に追加されています3つの新しいイベント購読（
'student_or_trainee.create','user.create','regular_event.update'）が正しく追加されています。各購読で.newを使用してnotifierクラスの新しいインスタンスを作成しているのは適切で、既存のパターンと一貫性があります。イベント名もActiveSupport::Notificationsの慣例に従ったドット区切りの文字列形式になっています。test/models/times_channel_creator_test.rb (5)
13-13: テストの呼び出しが新しいメソッドシグネチャに適切に更新されています
TimesChannelCreator#callの新しいメソッドシグネチャに合わせて、テストの呼び出しが正しく更新されています。使用しない最初の4つのパラメータにはnilを渡し、最後のペイロードはキーワード引数として渡すという実装は適切です。
20-20: テストの呼び出しが新しいメソッドシグネチャに適切に更新されています2つ目のテストケースでも同様に、新しいメソッドシグネチャに合わせた呼び出しが正しく実装されています。
33-33: 例外テストも新しいメソッドシグネチャに適切に更新されています管理者ユーザーでの例外テストも新しいメソッドシグネチャに合わせて正しく更新されています。
40-40: メンターユーザーでの例外テストも適切に更新されていますメンターユーザーでの例外テストケースも新しいメソッドシグネチャに合わせて正しく更新されています。
47-47: アドバイザーユーザーでの例外テストも適切に更新されていますアドバイザーユーザーでの例外テストケースも新しいメソッドシグネチャに合わせて正しく更新されています。
app/controllers/users_controller.rb (3)
69-69: ActiveSupport::Notificationsへの移行が正しく実装されています
Newspaper.publish(:user_create, user: @user)からActiveSupport::Notifications.instrument('user.create', user: @user)への変更により、イベント通知システムが適切に移行されています。イベント名の命名規則もActiveSupportの標準的なドット区切り形式に従っています。
105-105: 研修生向けイベント通知の移行が適切です研修生（trainee）の場合のみ
student_or_trainee.createイベントを発火する条件分岐が正しく実装されています。無料ユーザー作成フローでの適切な場所でのイベント発火になっています。
151-151: 学生向けイベント通知の移行が適切です有料ユーザー作成フローにおいて、学生（student）の場合のみ
student_or_trainee.createイベントを発火する条件分岐が正しく実装されています。支払い処理完了後の適切なタイミングでのイベント発火になっています。app/models/regular_event_update_notifier.rb (1)
4-4: ActiveSupport::Notificationsの標準インターフェースに正しく対応
callメソッドの引数がActiveSupport::Notificationsのsubscriberインターフェース（name, started, finished, unique_id, payload）に正しく対応しています。使用しない引数にアンダースコア接頭辞を付けることで、意図が明確になっています。
|
お疲れ様です！ |
Miya096jp
changed the title
|
@Miya096jp プルリクエストをざっと拝見しましたが、読みやすくて良いですね🙌 |
|
ありがとうございます！ |
|
@Miya096jp |
Miya096jp
force-pushed
the
chore/replace-newspaper-with-notifications
branch
from
c9be074 to
3062c30
Compare
jun-kondo
left a comment
jun-kondo
left a comment
There was a problem hiding this comment.
@Miya096jp
確認しました。Approveしますー🙆♂️
|
ありがとうございます！🙌 |
|
お疲れ様です！ |
|
@Miya096jp conflictの修正をお願いします。 |
Miya096jp
force-pushed
the
chore/replace-newspaper-with-notifications
branch
from
3062c30 to
6a28937
Compare
|
修正しました！ |
|
@Miya096jp conflictの修正をお願いします。 |
\#8938とのコンフリクトを解消 \#8921, #8922とのコンフリクトを解消
Miya096jp
force-pushed
the
chore/replace-newspaper-with-notifications
branch
from
6a28937 to
65e7ee0
Compare
|
修正しました！ |
|
お疲れ様です！ こちら8/19のリリースでしたが、本番環境での動作確認を依頼させていただくのを忘れておりました。大変申し訳ございません。 SignUpNotifierのみ、動作確認をお願いいたします🙇🏻♂️ |
|
@Miya096jp 遅くなってもうしわけありません。確認しました〜！ |
3 participants
Issue
概要
下記の3クラスの実装をnewspaperからActiveSupport::Notificationsに置換
SignUpNotifier
RegularEventUpdateNotifier
TimesChannelCreator
動作確認に新規ユーザーの登録が必要なので、
次のテスト用クレジットカード番号を使ってください。
関連Issue
[分報]入会時に分報チャンネルを自動で作りたい。 #6103
受講生（現役生または研修生）が入会したときに分報チャンネルを自動で作成する #6185
変更確認方法
bin/setupを実行foreman start -f Procfile.devでサーバーを起動↓以降の手順
SignUpNotifier
RegularEventUpdateNotifier
TimesChannelCreator
検証の準備
1.DiscordのサーバーIDを取得
a. まず動作確認用のDiscordサーバーを新規作成する
b. サーバーIDを取得する
作成したサーバーのアイコンを右クリック > メニューのIDをコピーをクリックして取得
2. カテゴリーIDの取得
a. カテゴリーを作成
サーバーのアイコンをクリック > サーバー名の右側の「∨」をクリックしメニューを表示 > メニューからカテゴリーを作成を選択し、カテゴリーを作成する(カテゴリー名は任意)
b. 開発者モードをON
ユーザー設定(歯車マーク)を開く > 詳細設定を選択 > 開発者モードをONに変更
c. カテゴリーIDの取得
カテゴリー名を右クリック > IDをコピーをクリックしてカテゴリーIDを控える
3. Botの設定とトークンの取得
a. アプリケーションを登録
DEVELOPER PORTAL にアクセスし、画面右上のNew Applicationからアプリケーションを登録
b.Install Linkの設定
左メニューペインのInstallationをクリック > Install LinkのプルダウンからNoneを選択
c. Botの設定とトークンの取得
左メニューペインのBotをクリックしてBot設定ページを開く
Reset TokenをクリックしTOKENを発行し控える
次の設定を変更する
4. Botをサーバーに追加する
a. 左メニューペインのOAuth2をクリックしてOAuth2画面を開く
b. OAuth2 URL Generator配下のbotにチェックを入れる
c. BOT PERMISSIONS配下のMinage ChannelsとKick Membersにチェックを入れる
d. 画面下部のGENERATED URLペインに表示されるURLをコピー
e. URLにアクセスする
f. サーバーを追加: からサーバーを選択しはいをクリック
g. 「チャンネルの管理」と「メンバーをキック」の権限がついていることを確認したら認証をクリック
5. 環境変数を設定する
次のコマンドをターミナルで実行するか、もしくはdirenvを使用する。
export DISCORD_GUILD_ID=サーバーID
export DISCORD_TIMES_CHANNEL_CATEGORY_ID=カテゴリーID
export DISCORD_BOT_TOKEN=BOTのトークン
direnv
direnvのインストールとsetup
Summary by CodeRabbit
Summary by CodeRabbit
新機能
リファクタ
テスト