Conversation
WalkthroughHomeControllerのダッシュボードでの同僚研修生の日報取得ロジックをインラインクエリからクエリオブジェクト Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as HomeController
participant Query as ColleagueTraineesRecentReportsQuery
participant DB as Database
Controller->>Query: new(current_user).call.limit(10)
Query->>DB: JOIN users\nWHERE user_id IN (colleague_trainee_ids)\nAND NOT wip\nAND with_avatar\nORDER BY reported_on DESC
DB-->>Query: Report records
Query-->>Controller: 結果を返す
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
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/home_controller.rbrubocop-minitest extension supports plugin, specify app/queries/colleague_trainees_recent_reports_query.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 skipped from review as they are similar to previous changes (2)
⏰ 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
|
hirokiej
force-pushed
the
refactor-colleague-trainees-recent-reports-query
branch
3 times, most recently
from
0fa74b5 to
6fbe791
Compare
![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/controllers/home_controller.rb (1)
49-50: Query objectの使用は適切ですが、冗長なorderingがあります。複雑なActiveRecordクエリをQuery objectに抽出したのは、コーディングガイドラインに従った良いリファクタリングです。ただし、
ColleagueTraineesRecentReportsQuery内で既にrecentスコープを使用してreported_on: :descでソートしているため、50行目の.order(reported_on: :desc)は冗長です。冗長なorderingを削除することを検討してください：
- colleague_trainees_reports = ColleagueTraineesRecentReportsQuery.new(current_user:).call - @colleague_trainees_recent_reports = colleague_trainees_reports.order(reported_on: :desc).limit(10) + @colleague_trainees_recent_reports = ColleagueTraineesRecentReportsQuery.new(current_user:).call.limit(10)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/home_controller.rb(1 hunks)app/models/report.rb(1 hunks)app/queries/colleague_trainees_recent_reports_query.rb(1 hunks)test/fixtures/reports.yml(1 hunks)test/queries/colleague_trainees_recent_reports_query_test.rb(1 hunks)
🧰 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/models/report.rbapp/controllers/home_controller.rbtest/queries/colleague_trainees_recent_reports_query_test.rbapp/queries/colleague_trainees_recent_reports_query.rb
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/fixtures/reports.ymltest/queries/colleague_trainees_recent_reports_query_test.rb
🧠 Learnings (6)
📓 Common learnings
Learnt from: jun-kondo
PR: fjordllc/bootcamp#8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`@report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
app/models/report.rb (1)
Learnt from: jun-kondo
PR: #8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトのapp/controllers/reports_controller.rbにおいて、createとupdateアクションは両方とも@report.save_uniquelyを使用し、同じ:report_saveイベントと'report.save'イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
test/fixtures/reports.yml (1)
Learnt from: jun-kondo
PR: #8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトのapp/controllers/reports_controller.rbにおいて、createとupdateアクションは両方とも@report.save_uniquelyを使用し、同じ:report_saveイベントと'report.save'イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
app/controllers/home_controller.rb (1)
Learnt from: jun-kondo
PR: #8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトのapp/controllers/reports_controller.rbにおいて、createとupdateアクションは両方とも@report.save_uniquelyを使用し、同じ:report_saveイベントと'report.save'イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
test/queries/colleague_trainees_recent_reports_query_test.rb (2)
Learnt from: kitarou888
PR: #8277
File: test/models/searcher_test.rb:326-331
Timestamp: 2025-07-10T12:59:27.807Z
Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特にonly_meのような機能では、異なる検索条件（空文字と具体的な検索語）を使い分けることで、より広範囲な動作保証が可能となる。
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/queries/colleague_trainees_recent_reports_query.rb (1)
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の併存は意図された仕様であり、排他的な関係ではない。
⏰ 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)
- GitHub Check: build_and_test
🔇 Additional comments (9)
app/models/report.rb (1)
63-63: 新しいscopeの追加が適切です。
recentスコープはreported_onの降順でソートする明確な責務を持ち、新しいQuery objectで適切に使用されています。既存のdefault_orderスコープと一貫した実装になっています。test/fixtures/reports.yml (1)
354-360: テスト用フィクスチャの追加が適切です。WIPの日報を除外するロジックをテストするためのフィクスチャとして適切に設計されています。既存のパターンに従った実装です。
test/queries/colleague_trainees_recent_reports_query_test.rb (3)
6-15: 基本的な機能テストが適切に実装されています。同僚研修生の日報が含まれ、関係のない日報が除外されることを適切にテストしています。フィクスチャの使用も適切です。
17-24: WIP除外のテストが適切です。新しく追加された
report75フィクスチャを使用してWIPの日報が除外されることを確認しています。
26-33: ソート順のテストが適切に実装されています。
recentスコープによる降順ソートが正しく機能することを検証しています。日付の配列を比較する方法は明確で理解しやすいです。app/queries/colleague_trainees_recent_reports_query.rb (4)
3-4: Query objectの基本構造が適切です。
rails-patternsgemのQuery patternを正しく使用し、Reportモデルを対象としています。コーディングガイドラインに従った良い実装です。
8-15: クエリロジックが適切に実装されています。
joins(:user)でユーザーテーブルとの結合- 同僚研修生IDでのフィルタリング
- WIPの除外
recentスコープの使用- パフォーマンスを考慮したeager loading
すべて適切に実装されており、Fat Controllerを避ける良いリファクタリングです。
17-20: カスタムイニシャライザが適切です。
current_userパラメータを必須にし、親クラスの初期化も適切に呼び出しています。
22-24: ヘルパーメソッドが適切に実装されています。同僚研修生のIDを取得するロジックが明確に分離されており、可読性が向上しています。
hirokiej
force-pushed
the
refactor-colleague-trainees-recent-reports-query
branch
from
6fbe791 to
b857057
Compare
|
パフォーマンステストは #8930 のやり取りでなしという方針となりました。
|
|
@jun-kondo さん |
|
@hirokiej |
hirokiej
force-pushed
the
refactor-colleague-trainees-recent-reports-query
branch
from
b857057 to
dd747a5
Compare
|
@hirokiej |
jun-kondo
left a comment
jun-kondo
left a comment
There was a problem hiding this comment.
お疲れ様ですー
すみません。Approveになっていませんでした🙏
おそらくCIが通るようになってると思うので進められると思います👀
hirokiej
force-pushed
the
refactor-colleague-trainees-recent-reports-query
branch
from
dd747a5 to
c2fe65d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/system/training_completion_test.rb (1)
87-96: 明示的なログインヘルパに統一すると意図がさらに明確にこのテスト内では直前に
visit_with_auth '/reports/new', 'kensyu'を実行しているためセッションは維持されますが、visit new_training_completion_pathもvisit_with_authに揃えると、将来ヘルパの挙動やセッション初期化が変わった場合でも意図が明確で壊れにくくなります。（他の同趣旨テストもvisit_with_authで統一されています）提案差分:
- visit new_training_completion_path + visit_with_auth new_training_completion_path, 'kensyu'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/controllers/home_controller.rb(1 hunks)app/queries/colleague_trainees_recent_reports_query.rb(1 hunks)test/fixtures/reports.yml(1 hunks)test/queries/colleague_trainees_recent_reports_query_test.rb(1 hunks)test/system/training_completion_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/queries/colleague_trainees_recent_reports_query_test.rb
- test/fixtures/reports.yml
- app/queries/colleague_trainees_recent_reports_query.rb
- app/controllers/home_controller.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:
test/system/training_completion_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/system/training_completion_test.rb
⏰ 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)
- GitHub Check: build_and_test
🔇 Additional comments (1)
test/system/training_completion_test.rb (1)
90-96: confirmの扱いと検証ポイントの切り出しが適切（堅牢性が向上）
- confirmダイアログは
page.accept_confirmで包み、完了メッセージのassert_textをブロック外へ出したのは正しいです。クリック後の画面更新を待ってから検証でき、フレークを避けられます。assert_not @user.reports.wip.exists?への置き換えも、件数差分ではなく期待状態（WIPが1件も残っていない）を直接担保できるので、仕様に対して強いテストになっています。travel_toを使っているため、assert_equal Time.current, @user.reload.training_completed_atも安定して比較できています。この変更は、昨夜以降のテスト不安定化（WIP件数差分の前提崩れ）に対して良い方向の修正だと思います。
|
@jun-kondo さん fixtureを増やした影響で、一部テストコードを修正しました。そちらのご確認をよろしくお願いいたします🙇 |
jun-kondo
left a comment
jun-kondo
left a comment
There was a problem hiding this comment.
お疲れ様ですー🍵
確認しました🙌
追加の変更箇所について、問題ないと思います。
1点コメントさせて頂きました。お手数ですがご確認頂ければ幸いです🙏
| end | ||
|
|
||
| assert_text '研修終了手続きが完了しました' | ||
| assert_not @user.reports.wip.exists? |
There was a problem hiding this comment.
変更の理由について下記の理解であっていますでしょうか👀
変更前: テスト内でWIP状態の日報を一つ作成し、研修修了手続きの前後でのWIP日報の差分が-1になるアサーションだった。
機能としては研修終了手続きを完了するとすべてのWIP状態の日報が削除されるが、fixtureでユーザーkensyuに紐づくWIPの日報を一つ追加したので差分が-2になりテストが落ちるようになったのでassert_not @user.reports.wip.exists?存在確認に変更。
すべてのWIP状態の日報が削除されるなら、差分の確認より存在確認の方が良さそうですね👀
There was a problem hiding this comment.
その理解であってます！
あとは今後、fixtureでWIPの日報をn個作成する度にテストが失敗し、-nで差分を調整しなければならず、保守性の観点からもexistsにすべきだと思いました💡
|
@jun-kondo さん @komagata さん |
hirokiej
force-pushed
the
refactor-colleague-trainees-recent-reports-query
branch
from
c2fe65d to
e2478c2
Compare
|
@komagata さん |
3 participants
Issue
概要
HomeControllerにあった同僚研修生の最新日報取得ロジックをrails-patternsgemのQueryオブジェクトを使ってリファクタリングしました。参考: QueryObject · fjordllc/bootcamp Wiki
変更確認方法
refactor-colleague-trainees-recent-reports-queryをローカルに取り込むsenpaiでログインID:
senpaiPASS:
testtest3.ダッシュボードの下にスクロールをし、「研修生の最新の日報」があることを確認
4.
kensyuがsenpaiの同僚なので、その日報が降順(新→旧)になっていることを確認。Screenshot
※内部的なリファクタリングであり、見た目の変更はないためスクリーンショットはありません。
Summary by CodeRabbit
リファクタリング
新機能
テスト