Product.unchecked_no_replied_productsが未チェックかつ他者のコメントがない提出物を参照するように修正#8999
Product.unchecked_no_replied_productsが未チェックかつ他者のコメントがない提出物を参照するように修正#8999
Conversation
WalkthroughProduct.unchecked_no_replied_products スコープの内部実装が変更され、算出した未返信の ID 集合に対して Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant Test as TestSuite
participant Product as ProductModel
participant DB
end
Test->>Product: Product.unchecked_no_replied_products
Product->>DB: SELECT ids for no_replied_products (self_last_commented + no_comments)
Product->>DB: SELECT ids FROM unchecked scope
Product->>Product: intersect IDs (no_replied_products_ids ∩ unchecked_ids)
Product->>DB: SELECT * WHERE id IN (intersection) ORDER BY published_at ASC, id ASC
Product-->>Test: ActiveRecord::Relation (結果)
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/models/product.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 (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
changed the title
![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 (2)
app/models/product.rb(1 hunks)test/models/product_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:
test/models/product_test.rbapp/models/product.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/product_test.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Learnt from: kitarou888
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:326-331
Timestamp: 2025-07-10T12:59:27.807Z
Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特に`only_me`のような機能では、異なる検索条件（空文字と具体的な検索語）を使い分けることで、より広範囲な動作保証が可能となる。
Learnt from: masyuko0222
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:0-0
Timestamp: 2025-07-07T12:46:01.650Z
Learning: テストにおいて、`result.all? { |condition| }` でコレクションの全要素が条件を満たすことを確認できれば、その条件を満たさない要素が含まれていないことも論理的に保証されるため、追加のアサーションは冗長となる。
Learnt from: thmz337
PR: fjordllc/bootcamp#8590
File: app/models/movie.rb:22-24
Timestamp: 2025-07-12T09:56:13.360Z
Learning: Rails scopeでnilやfalseを返すと、NoMethodErrorは発生せず、代わりにModel.allが返される。これによりメソッドチェインは継続するが、全レコードが読み込まれるため意図しない動作となる可能性がある。
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
🔇 Additional comments (1)
app/models/product.rb (1)
64-66: 適切な修正です！
unchecked_no_replied_productsスコープが正しく修正されています。未チェックの提出物のIDと他者からのコメントがない提出物のIDの積集合を取ることで、両方の条件を満たす提出物のみを確実に返すようになりました。この実装により、PRの目的である「未チェックかつ他者のコメントがない提出物を参照する」が正確に実現されています。
| test '.unchecked_no_replied_products' do | ||
| unchecked_no_replied_products = Product.unchecked_no_replied_products | ||
|
|
||
| only_self_replied_product = products(:product1) | ||
| checked_product = products(:product2) | ||
| unchecked_replied_product = products(:product3) | ||
|
|
||
| assert_includes unchecked_no_replied_products, only_self_replied_product | ||
| assert_not_includes unchecked_no_replied_products, checked_product | ||
| assert_not_includes unchecked_no_replied_products, unchecked_replied_product | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
適切なテスト実装です！
新しく追加された.unchecked_no_replied_productsのテストケースは、修正されたスコープの動作を適切に検証しています。テスト名が英語で記述されており、コーディングガイドラインに従っています。
テストでは以下の3つのシナリオを検証しており、包括的です：
- 自分のコメントのみの未チェック提出物（含まれるべき）
- チェック済み提出物（除外されるべき）
- 他者からのコメントがある未チェック提出物（除外されるべき）
フィクスチャデータが各テストケースの前提条件と一致していることを確認してください。
フィクスチャデータを確認するために以下のスクリプトを実行してください：
🏁 Script executed:
#!/bin/bash
# Description: Verify fixture data matches test assumptions for products used in the test
echo "Checking fixture data for products used in unchecked_no_replied_products test..."
# Check product1 (should be unchecked with only self-replies)
echo "=== Product1 (only_self_replied_product) ==="
rg -A 10 -B 2 "product1:" test/fixtures/products.yml
# Check product2 (should be checked)
echo "=== Product2 (checked_product) ==="
rg -A 10 -B 2 "product2:" test/fixtures/products.yml
# Check product3 (should be unchecked with replies from others)
echo "=== Product3 (unchecked_replied_product) ==="
rg -A 10 -B 2 "product3:" test/fixtures/products.yml
# Also check for any related comments or checks
echo "=== Related comments ==="
rg "product[1-3]" test/fixtures/comments.yml | head -20
echo "=== Related checks ==="
rg "product[1-3]" test/fixtures/checks.yml | head -20Length of output: 1980
.unchecked_no_replied_productsテストのフィクスチャ要修正
以下の点を修正してください：
- test/fixtures/products.yml
product3に現在checker_id: "<%= ActiveRecord::FixtureSet.identify(:komagata) %>"が設定されており、未チェック前提（unchecked_replied_product）と矛盾しています。
→checker_id: nilに書き換え、未チェック状態としてください。
- test/fixtures/comments.yml
product3へのコメントが他ユーザーによるもの（user: …がproduct3.userと異なる）であることを再確認してください。
これらを修正後、改めてテストが以下のシナリオを正しく検証できることを確認してください：
- 自分のみのコメントあり & 未チェック → 含まれる
- チェック済み → 含まれない
- 他者コメントあり & 未チェック → 含まれない
🤖 Prompt for AI Agents
In test/models/product_test.rb around lines 203 to 213, the test for
`.unchecked_no_replied_products` relies on fixture data that is inconsistent.
Update test/fixtures/products.yml by changing `product3`'s `checker_id` to nil
to reflect an unchecked state. Then verify in test/fixtures/comments.yml that
comments on `product3` are from users different from `product3.user` to
represent replies from others. After these fixture corrections, rerun the test
to ensure it correctly verifies the intended scenarios.
1dd7573 to
9736a10
Compare
|
お疲れ様です。こちらレビューをお願いできないでしょうか？ |
|
お疲れ様です、依頼ありがとうございます！ |
|
ありがとうございます！よろしくお願いします🙇 |
|
お疲れ様です！ 概要について本来あるべき姿
生じているバグ
質問1「チェック済み/未チェックに関わらず、コメントがない提出物をすべて抽出してしまう」という解釈でいいですか？ 変更確認方法について
手順4に対して、手順7で「4の提出物が含まれていることを確認する」とありますが、4の提出物は、「未チェックかつコメントあり」なので、抽出されないのではないかと思います(実際やってみたら抽出されなかった) 手順5に対して、手順8で「5の提出物が含まれていないことを確認する」とありますが、5の提出は「コメントなしかつチェック済み」なのでバグが修正されていれば、含まれていないで正しいと思います。(こちらは抽出された) 質問2手順4は誤りでしょうか？ もしDiscriptionに誤りがあれば修正をお願いします。私が理解できていない部分があれば教えて頂けたらと思います。よろしくお願いします！ |
|
お疲れ様です！ご確認ありがとうございます。
おっしゃる通りです。本来は、
こちら私の方でもやってみたのですが、抽出されるようです。提出物に付けたコメントは、
こちら私の方で5の手順を行ったところ、8で抽出されませんでした。 変更確認方法も変更させていただきます。 |
|
ありがとうございます！ その上で、Discriptionを見て、「この変更手順であっているのかな？」とモヤり、そのモヤりの原因を言語化できずにさらにモヤっていたのですが、やっと整理できたのでお伝えします。 まず、概要を見て、コメント判定は問題ない前提と理解しました。その上で、手順4の 実装を見た後に、「自分コメは他者コメに含まない」ケースを回帰的にカバーする意図とわかり、手順4は適切だと理解しました。 モヤった原因概要のメソッドの説明文、 例えば、「メンター・管理者が、未チェックの提出物のうち、 "メンター・管理者のコメントがない" / "提出者のコメントに返信していない"ものがわかるようにするためのメソッド」など、「誰が」「いつ」「何のために」使うメソッドなのか具体的にわかるような書き方であればストンと腑に落ちたように思いました。 あとちょっとでレビュー終わりますので、もう少しお待ちください！ |
| created_at: <%= 9.day.ago %> | ||
| updated_at: <%= 9.day.ago %> | ||
| published_at: <%= 9.day.ago %> | ||
| commented_at: <%= 9.day.ago %> |
There was a problem hiding this comment.
こちらは判定に影響はないようなのですが、追記した理由があれば教えていだけますか？
There was a problem hiding this comment.
ご確認ありがとうございます。
提出者以外のコメントがないと言う条件で提出物を抽出するということを、以下を組み合わせることで行っています。
- 提出者はコメントしているが、管理者・メンターからのコメントがない
- そもそも誰もコメントしていない
2については、commented_atがnilであることによって判定するようにしています。
product10については、他者からのコメントがある提出物であるので、commented_atに値が入っていなければなりません。
そうでなければ2の条件に合致してしまい抽出されることになってしまいます。
不明な点、誤りがあればご指摘のほどよろしくお願いいたします🙇♀️
There was a problem hiding this comment.
ご回答ありがとうございます！
また少し疑問が出てきてしまったので、教えてください🙏
product10については、他者からのコメントがある提出物であるので、commented_atに値が入っていなければなりません
Commentableで紐づいているのになんでcreated_atがいるのかな？と思ったのですが、そもそもscopeでcreated_at = nilで判定してるということを忘れてました… ご教示ありがとうございます
！
2については、commented_atがnilであることによって判定するようにしています。
これはproduct2が、assert_notをパスすることをもって確認しているということでしょうか？
- 提出者はコメントしているが、管理者・メンターからのコメントがない
これはproduct1で行っているのでしょうか？もしそうであればproduct1にcreated_atが必要なように思いました。
There was a problem hiding this comment.
お疲れ様です。ご確認ありがとうございます。
これはproduct2が、assert_notをパスすることをもって確認しているということでしょうか？
created_atではなく、commented_atのことで良いでしょうか?
commented_atがnilであることによって判定するようにしています。と書いていますが、こちらのテストケースをカバーするテストがなかったため、追加させていただきました。
これはproduct1で行っているのでしょうか？もしそうであればproduct1にcreated_atが必要なように思いました。
こちらもcommented_atのことで良いでしょうか？おっしゃる通り、product1にcommented_atが必要です。修正させていただきました。
ご指摘ありがとうございました🙇♀️現在、テストが通らないため、webp周りの問題が解消され次第、改めてレビューのご依頼をさせていただきます。
以上よろしくお願いいたします。
There was a problem hiding this comment.
お疲れ様です！無事テストが通りましたので、レビューをお願いできますでしょうか。
以前の変更から、ご指摘事項(cdderabbitも含む)も踏まえて以下の箇所を変更しています。
-
products.ymlの修正
以前ご指摘いただいた点について、コメントがある提出物にcommented_atを追記させていただきました。その他のテストデータについても見直しを行い、整合性を取れるように修正しました。commentd_atを追記したことで、1件テストが通らなくなったため、そちらも合わせて修正を行っています。 -
unchecked_no_replied_productsの修正
修正前までは、
uncheckedな提出物のidとno_repliedな提出物のidを結合- 該当提出物を抽出
としていましたが、coderabbitの指摘を加味して、以下の通り変更を行いました。
uncheckedな提出物に対して、no_repliedな提出物のidで絞り込みを行う。
こうすることで、pluckメソッドによる、idの配列をメモリに展開することがなくなり、scope自体もスッキリしたと思います。
以上ご確認よろしくお願いいたします。
thmz337
force-pushed
the
bug/do-not-include-checked-products
branch
2 times, most recently
from
9bda333 to
9b88c06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/models/product.rb (2)
58-66: N+1/全件ロードを回避するため、DBで完結するクエリに置き換えましょう現状の実装は以下の懸念があります:
- where.not(...).filter により Ruby 側でレコードを全件走査
- product.comments.last による N+1 クエリ（コメントを関連 preload していない）
- 大量の pluck/map によるメモリ使用量増大
- スコープ合成時にDB最適化が効きにくい
「他者から未返信」の判定は、既存のカラムである commented_at と self_last_commented_at を比較することでSQL上で行えます（「コメントなし」または「最後のコメントが提出者自身」）。以下のように置き換えることで、1クエリで完結し、N+1 と全件ロードを解消できます。
- scope :unchecked_no_replied_products, lambda { - self_last_commented_products = where.not(commented_at: nil).filter do |product| - product.comments.last.user_id == product.user.id - end - no_comments_products = where(commented_at: nil) - no_replied_products_ids = (self_last_commented_products + no_comments_products).map(&:id) - unchecked_products_ids = unchecked.pluck(:id) - unchecked_no_replied_ids = no_replied_products_ids & unchecked_products_ids - where(id: unchecked_no_replied_ids).order(published_at: :asc, id: :asc) - } + scope :unchecked_no_replied_products, lambda { + unchecked + .where('commented_at IS NULL OR commented_at = self_last_commented_at') + .ascending_by_date_of_publishing_and_id + }補足:
- もし古いデータで self_last_commented_at が未設定の可能性がある場合は、移行時にバックフィルするか、暫定的に last_comments サブクエリで最終コメントの投稿者を判定する実装にすることも検討可能です（既存の self_assigned_no_replied_product_ids の CTE を参考にできます）。
58-63: 最後のコメント判定に comments.last は使わない方が安全
comments.lastはデフォルト順序に依存し、最新コメント（updated_at）の保証がありません。アプリはcommented_atを最新コメントの updated_at に同期しているため、commented_at = self_last_commented_atによる判定の方が堅牢です（時系列の逆転や ORDER 指定漏れの影響を排除できます）。上記のリファクタでこの懸念は解消されます。
🧹 Nitpick comments (1)
app/models/product.rb (1)
41-41: unchecked スコープの pluck を避け、サブクエリ/結合で表現しませんか現状の実装は
pluckにより全件を配列化してから NOT IN するため、件数が多いとメモリ負荷・SQL 長大化の懸念があります。checksの関連が既にあるため、以下のように書き換えると効率的で読みやすくなります。- scope :unchecked, -> { where.not(id: Check.where(checkable_type: 'Product').pluck(:checkable_id)) } + scope :unchecked, -> { where.missing(:checks) }Rails 6.1+ で
where.missing(:association)が利用可能です。等価で、DB側で最適化されやすいです。
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/product.rb(1 hunks)test/fixtures/products.yml(2 hunks)test/models/product_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/models/product_test.rb
- test/fixtures/products.yml
🧰 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/models/product.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)
app/models/product.rb (1)
64-66: 未チェック条件の追加は妥当（ロジック修正OK）no_replied_products_ids と unchecked_products_ids の積集合で絞り込むことで、「未チェック」かつ「他者から未返信」の要件が満たされています。最終クエリで order(published_at: :asc, id: :asc) している点も要件に整合しています。
|
こちらの修正を適用すると、あるメンターが提出物を提出して、そのメンター自身が提出物にコメントをした際、 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 975 🏁 Script executed: Length of output: 510 🏁 Script executed: Length of output: 1311 🏁 Script executed: Length of output: 79 🏁 Script executed: Length of output: 681 🏁 Script executed: Length of output: 6614 🏁 Script executed: Length of output: 740 🏁 Script executed: Length of output: 1414 🏁 Script executed: Length of output: 550 🏁 Script executed: Length of output: 46 🏁 Script executed: Length of output: 3283 🏁 Script executed: Length of output: 608 🏁 Script executed: Length of output: 1761 🏁 Script executed: Length of output: 1258 🏁 Script executed: Length of output: 1385 @thmz337 あなたの指摘は正しいです。これは重要な問題だと思います。 現在の def update_last_commented_at(comment)
if comment
if comment.user.mentor
update_columns(mentor_last_commented_at: comment.updated_at)
elsif comment.user == user
update_columns(self_last_commented_at: comment.updated_at)
end
else
update_columns(mentor_last_commented_at: nil, self_last_commented_at: nil)
end
end問題の原因：
影響： self_last_commented_products = where.not(commented_at: nil).filter do |product|
product.comments.last.user_id == product.user.id
endこの場合は大丈夫そうですが、将来的に この問題を修正するには、メンターが自分の提出物にコメントした場合に両方のフィールドを適切に設定するか、優先順位を見直す必要があります。 |
|
この点を踏まえて、 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@thmz337 現在の 理由は以下の通りです： 現在の実装の動作self_last_commented_products = where.not(commented_at: nil).filter do |product|
product.comments.last.user_id == product.user.id
endこの部分では：
メンター自己コメントのケースメンターが自分の提出物にコメントした場合：
結論
このメソッドに関しては追加の修正は不要です。 🏁 Script executed: Length of output: 81 🏁 Script executed: Length of output: 1051 🏁 Script executed: Length of output: 1194 🏁 Script executed: Length of output: 1504 @thmz337 実装を確認しました。 現在の実装の詳細分析scope :unchecked_no_replied_products, lambda {
self_last_commented_products = where.not(commented_at: nil).filter do |product|
product.comments.last.user_id == product.user.id
end
no_comments_products = where(commented_at: nil)
no_replied_products_ids = (self_last_commented_products + no_comments_products).map(&:id)
unchecked_products_ids = unchecked.pluck(:id)
unchecked_no_replied_ids = no_replied_products_ids & unchecked_products_ids
where(id: unchecked_no_replied_ids).order(published_at: :asc, id: :asc)
}メンター自己コメントケースでの動作メンターが自分の提出物にコメントした場合：
結論この実装は：
従って、
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/fixtures/products.yml (1)
86-87:commented_at付与の意図は過去コメントで解消済み「他者コメントのある提出物に
commented_atが必要」という点は、過去のやり取りで理由が説明されており納得です。現状の追記はその意図に沿っています。
🧹 Nitpick comments (3)
test/fixtures/products.yml (3)
1-7: 「自分のみコメント」ケースの表現として妥当／タイムスタンプ整合性は任意で補強を検討
- product1 に
mentor_last_commented_atとcommented_atを付与したのは、提出者本人（今回はメンター）だけがコメントしているケースの再現として妥当です。- ただし、product1 は
published_atが未設定のままなので、時系列の一貫性（「公開後にコメントが付く」）や ORDER BY published_at の安定性の観点では、任意でpublished_at/created_at/updated_atをcommented_atより過去に揃えておくと安全です（NULL の並び順がDB依存で先頭に来るケースを避けられます）。以下のように時系列を補強することを検討ください（任意）:
product1: # 自分(メンター)のコメントあり(2つ) practice: practice1 user: mentormentaro body: テストの提出物1です。 checker_id: nil + created_at: <%= 2.day.ago %> + updated_at: <%= 2.day.ago %> + published_at: <%= 2.day.ago %> mentor_last_commented_at: <%= 1.day.ago %> commented_at: <%= 1.day.ago %>
79-87: 見出し表現のゆれ（「のコメントあり」/「からのコメントあり」）の統一を提案他のエントリ（例: product9, product67）は「からのコメントあり」を使っている一方、ここは「のコメントあり」になっています。検索や将来の読みやすさのために表現統一を提案します（挙動には影響しない軽微な指摘）。
-product10: # 他者(メンター)のコメントあり +product10: # 他者(メンター)からのコメントあり
89-97: 見出し表現の統一（任意）product10 同様、見出しの表現を統一しておくと読み手に優しいです。
-product11: # 他者(メンター)のコメントあり +product11: # 他者(メンター)からのコメントあり
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/product.rb(1 hunks)test/fixtures/products.yml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/product.rb
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/fixtures/products.yml
⏰ 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 (4)
test/fixtures/products.yml (4)
18-26: チェック済み＋他者（メンター）コメントありの条件付けが明確で良いです
commented_atとmentor_last_commented_atの付与により、「返信あり」扱いが明確になっています。unchecked_no_replied_products から除外されるべきデータとして整合的です。
69-77: LGTM: 「チェック済み＋他者コメントあり」のデータ整備が適切
commented_at/mentor_last_commented_atの付与でスコープの意図どおり振る舞う状態が確保されています。
384-385: 担当＋返信済みのケース明示として適切
product63にcommented_at/mentor_last_commented_atを追加したことで、「返信済み」条件がデータ上も明確になりました。関連テストの期待に合致しています。
407-413: LGTM: チェック済み＋他者（メンター）コメントありの明示
checker_idとcommented_at/mentor_last_commented_atの組み合わせで本データが対象スコープから外れることが明確です。問題ありません。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/integration/api/products_test.rb (3)
62-64: self_assigned の並び順変更が仕様どおりかを要確認＋根拠の明文化を期待順の入れ替え自体は受け入れ可能ですが、このテストは self_assigned エンドポイントの順序保証を検証しています。今回のPRは unchecked_no_replied_products の修正が主目的のため、self_assigned の順序が「仕様として意図的に」変わったのか、あるいはフィクスチャ（commented_at/mentor_last_commented_at 等）の更新に伴う付随的な変化なのかを確認してください。後者であれば、テストの直上に「この順序が成り立つ根拠（ソートキー）」をコメントで明示しておくと将来の保守性が上がります。
62-64: タイトルではなくIDで順序を検証して脆さを低減practice.title は文言変更の影響を受けやすいので、可能なら product ID ベースでの順序検証に切り替えることをおすすめします（API レスポンスに id が含まれている前提）。その場合は以下のように書き換えられます。
- expected = products(:product15, :product62, :product64, :product63).map { |product| product.practice.title } - actual = response.parsed_body['products'].map { |product| product['practice']['title'] } + expected = [products(:product15).id, products(:product62).id, products(:product64).id, products(:product63).id] + actual = response.parsed_body['products'].map { |product| product['id'] }
57-57: テスト名の末尾スペースを削除見落としや差分ノイズの原因になりがちなので、末尾スペースは削除しておきましょう。
- test 'should return products in order ' do + test 'should return products in order' do
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integration/api/products_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:
test/integration/api/products_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/integration/api/products_test.rb
🧬 Code Graph Analysis (1)
test/integration/api/products_test.rb (1)
app/models/practice.rb (1)
product(139-141)
⏰ 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
|
お疲れ様です。ご確認ありがとうございました🙇 こちらレビューをお願いできますでしょうか？ |
thmz337
force-pushed
the
bug/do-not-include-checked-products
branch
from
ec66fe4 to
be62c98
Compare
3 participants
Issue
概要
Product.unchecked_no_replied_productsについて、本来期待する動作になるよう修正を行いました。何に使用するメソッドであるか
メンター・管理者が、提出物の内、
合格判定を行っていないかつ(提出者以外の)コメントがない物を分かるようにするためのメソッド。期待する動作
上記の通り、提出物の内、
合格判定を行っていないかつ(提出者以外の)コメントがない物を抽出すること。現在の動作
期待する動作の内、
合格判定を行っていないという条件が満たされていないため、合格判定が行われている(checked)な提出物も抽出されてしまう。今回行った修正
提出物を抽出する際の条件に、
合格判定を行っていないを加えました。変更確認方法
bug/do-not-include-checked-productsをローカルに取り込むrails test test/models/product_test.rbを実行し、テストが通ることを確認する。foreman start -f Procfile.devでサーバーを起動提出物を作成したユーザーでコメントを残すrails cを入力し、コンソールを起動するProduct.unchecked_no_replied_productsを実行し、4の提出物が含まれていることを確認するProduct.unchecked_no_replied_productsを実行し、5の提出物が含まれていないことを確認するSummary by CodeRabbit
バグ修正
テスト
テストデータ