Conversation
|
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/reports_controller.rbrubocop-minitest extension supports plugin, specify app/controllers/admin/users_controller.rbrubocop-minitest extension supports plugin, specify app/controllers/api/practices/learning_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 (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
|
jun-kondo
changed the title
jun-kondo
force-pushed
the
chore/replace-newspaper-with-activesupport_notifications-in-sad_streak_updater-and-learning_cache_destroyer
branch
from
cc4bc46 to
35dfbce
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/admin/users_controller.rb(1 hunks)app/controllers/api/practices/learning_controller.rb(1 hunks)app/controllers/reports_controller.rb(3 hunks)app/models/learning_cache_destroyer.rb(1 hunks)app/models/sad_streak_updater.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
⚙️ 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/practices/learning_controller.rbapp/controllers/admin/users_controller.rbconfig/initializers/active_support_notifications.rbapp/models/sad_streak_updater.rbapp/controllers/reports_controller.rbapp/models/learning_cache_destroyer.rb
🧠 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
app/controllers/api/practices/learning_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が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
app/controllers/admin/users_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が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
🔇 Additional comments (7)
app/controllers/api/practices/learning_controller.rb (1)
32-32: ActiveSupport::Notificationsへの移行が正しく実装されていますNewspaper.publishからActiveSupport::Notifications.instrumentへの変更が適切に行われています。イベント名'learning.create'とペイロード構造{user: learning.user}も正しく設定されています。
config/initializers/active_support_notifications.rb (1)
10-16: イベントハンドラーの購読設定が適切に実装されていますSadStreakUpdaterとLearningCacheDestroyerのActiveSupport::Notifications購読設定が正しく行われています。インスタンスの再利用と適切なイベント名の使用により、効率的な実装となっています。
app/models/sad_streak_updater.rb (1)
4-7: ActiveSupport::Notifications対応のメソッドシグネチャが正しく実装されていますcallメソッドがActiveSupport::Notificationsのサブスクライバーインターフェースに適合するよう正しく更新されています。未使用パラメータのアンダースコアプレフィックスも適切で、内部ロジックは変更されていないため動作の一貫性が保たれています。
app/controllers/admin/users_controller.rb (1)
50-50: ActiveSupport::Notificationsへの移行が正しく実装されていますNewspaper.publishからActiveSupport::Notifications.instrumentへの変更が適切に行われています。イベント名'learning.destroy'とペイロード構造{user:}も正しく設定されており、LearningCacheDestroyerが期待通りに動作します。
app/models/learning_cache_destroyer.rb (1)
4-8: ActiveSupport::Notifications対応のメソッドシグネチャが正しく実装されていますcallメソッドがActiveSupport::Notificationsのサブスクライバーインターフェースに適合するよう正しく更新されています。内部のキャッシュ削除とログ出力ロジックは変更されておらず、動作の一貫性が保たれています。
app/controllers/reports_controller.rb (2)
90-90: 完全移行が適切に実装されています
destroyアクションではNewspaper.publishからActiveSupport::Notifications.instrumentへの完全な移行が行われており、これは適切です。ペイロード形式も一貫性が保たれています。
80-80: ペイロード形式の整合性を確認済みですSadStreakUpdater が新しい
instrument('report.save', report: @report)のペイロード形式（payload[:report]）に対応していることを確認しました。
- app/models/sad_streak_updater.rb
def call(_name, _started, _finished, _unique_id, payload)内でreport = payload[:report]を取得- config/initializers/active_support_notifications.rb
ActiveSupport::Notifications.subscribe('report.save', sad_streak_updater)で正しくサブスクライブ他に追加対応は不要です。
…ications-in-sad_streak_updater-and-learning_cache_destroyer` Docstrings generation was requested by @jun-kondo. * #8977 (comment) The following files were modified: * `app/controllers/admin/users_controller.rb` * `app/controllers/api/practices/learning_controller.rb` * `app/controllers/reports_controller.rb` * `app/models/learning_cache_destroyer.rb` * `app/models/sad_streak_updater.rb`
|
Note Generated docstrings for this pull request at #8981 |
jun-kondo
force-pushed
the
chore/replace-newspaper-with-activesupport_notifications-in-sad_streak_updater-and-learning_cache_destroyer
branch
2 times, most recently
from
ced0d44 to
b1e4676
Compare
|
@smallmonkeykey |
|
@jun-kondo |
|
@smallmonkeykey |
smallmonkeykey
left a comment
smallmonkeykey
left a comment
There was a problem hiding this comment.
@jun-kondo
コメントしましたので、ご確認お願いします🙏
| ActiveSupport::Notifications.subscribe('report.create', sad_streak_updater) | ||
| ActiveSupport::Notifications.subscribe('report.update', sad_streak_updater) |
There was a problem hiding this comment.
背景を確認させてください！
今回、元々の :report_save イベントを report.create と report.update に分けて処理されている意図を教えていただけますか？
私としては、一つのイベントとして report.save として書き換えるでも良いかなと思ったのですが、今回分けることにした理由があれば教えていただけると嬉しいです🙏
There was a problem hiding this comment.
背景としては、当初はreport.saveで書いていたのですが、メインの変更を取り込んだときにコンフリクトが起きて、コントローラにあるイベントの発行側のコード(ActiveSupport::Notifications.instrument)がreport.createとreport.updateでマージされていることに気がついた、というものです。担当箇所が一部他のissueと被っていた感じです。
調べたところ、こちらのプルリクエストでのやり取りを見つけました👀Pull Request #8869
こちらコメントのリンクです👀
- FirstReportNotifier及び他2クラスのnewspaperの利用をActiveSupport::Notificationsに置き換える #8869 (comment)
- FirstReportNotifier及び他2クラスのnewspaperの利用をActiveSupport::Notificationsに置き換える #8869 (comment)
迷ったのですが、この書き方で既に合意が取れているのが上記のコメントのやり取りでわかったのでそれに合わせました。
事前に説明しておくべきでした。申し訳ありません🙏
smallmonkeykey
left a comment
smallmonkeykey
left a comment
There was a problem hiding this comment.
@jun-kondo
説明してくださってありがとうございます！納得しました🙆
Approveさせていただきます！
|
@smallmonkeykey |
jun-kondo
force-pushed
the
chore/replace-newspaper-with-activesupport_notifications-in-sad_streak_updater-and-learning_cache_destroyer
branch
from
b1e4676 to
d2ef468
Compare
|
@smallmonkeykey @komagata |
|
@jun-kondo conflictの解消をお願いします。 |
jun-kondo
force-pushed
the
chore/replace-newspaper-with-activesupport_notifications-in-sad_streak_updater-and-learning_cache_destroyer
branch
from
d2ef468 to
5965505
Compare
|
@komagata |
komagata
deleted the
chore/replace-newspaper-with-activesupport_notifications-in-sad_streak_updater-and-learning_cache_destroyer
branch
3 participants
Issue
概要
以下の2クラスをnewspaperの利用をActiveSupport::Notificationsに移行しました。
変更確認方法
ブランチをローカルに取り込む
ローカルサーバーの立ち上げ
以下の手順でSadStreakUpdater、LearningCacheDestroyerが移行前と変わらず動作することを確認する
SadStreakUpdater
日報の新規作成、更新、削除のイベントが発生すると、オブザーバーの
SadStreakUpdaterクラスが日報情報を受け取りupdate_sad_streakメソッドを実行してユーザー情報を更新する更新されたユーザー情報をコンソール上で確認する
日報の作成、更新 イベントでの動作確認
ユーザー:
take8でログイン(パスワード: testtest)ターミナルで
コンソールでユーザー:
take8のsad_streak属性がfalseであることを確認する(http://localhost:3000/reports/new) にアクセスし、今日の気分sosoで日報を新規作成(プラクティス、日報タイトル、本文は任意)
コンソールでユーザー:
take8のsad_streak属性がfalseであることを確認するsad_streakがfalseになることを確認先ほど作成した日報の編集ページへアクセスし、今日の気分をsadに変更
コンソールでユーザー:
take8のsad_streak属性がtrueになることを確認するsad_streakがtrueになることを確認日報の削除イベントでの動作確認
アプリ上で先程追加した日報を削除する
コンソールでユーザー:
take8のsad_streak属性がfalseであることを確認するsad_streakがfalseになることを確認LearningCacheDestroyer
ユーザーがプラクティスの進捗ステータスを更新、管理者がユーザーを削除のイベントが発生するとオブザーバーの
LearningCacheDestroyerクラスがユーザー情報を受け取りキャッシュを削除する処理が実行されるターミナル上のdevelopmentログに
[LearningCacheDestroyer] Cache destroyed for user (ユーザーID)が出力されるので、それを確認するプラクティス進捗ステータス更新時の動作確認
ターミナルで
を実行し、developmentログを監視する
ユーザー:
hajimeでログイン(パスワード: testtest)着手中の「OS X Mountain Lionをクリーンインストールする」のプラクティスページに行く (http://localhost:3000/practices/315059988)
ページ上部のステータスを「着手」から「未着手」変更する
[LearningCacheDestroyer] Cache destroyed for user 253826460ユーザー削除イベントでの動作確認
ターミナルで
を実行し、developmentログを監視する
ユーザー:
komagataでログイン(パスワード: testtest)yameoの管理者用ユーザーページにアクセス(http://localhost:3000/users/888299165)し、退会済みユーザーの`yameo`を管理者ページから削除する
「このユーザーを削除する」のリンクをクリックして、確認のダイアログでOKをクリック
[LearningCacheDestroyer] Cache destroyed for user 888299165Screenshot
内部的な変更なのでスクリーンショットはありません。
Summary by CodeRabbit
リファクタ
その他