プラクティス一覧ページを表示した際、適切なカテゴリにスクロールされないバグを修正#9014
Conversation
WalkthroughUserCoursePractice が直接の関連経路でカテゴリを辿る代わりに、与えられたプラクティス集合からコース順で重複除去されたカテゴリを返す OrderedCategoriesFromPracticesQuery を使用するよう実装が置換され、未開始プラクティス取得の呼び出しスタイルと ORDER BY 順序も調整され、関連するテストとテストヘルパーが追加された。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as UserCoursePractice
participant Q as OrderedCategoriesFromPracticesQuery
participant DB as DB
U->>Q: call(user:, practices:)
activate Q
alt practices blank
Q-->>U: ActiveRecord::Relation.none
else practices present
Q->>DB: SELECT categories\nJOIN courses_categories\nWHERE course_id = user.course_id\nAND category_id IN (practice→category ids)\nORDER BY courses_categories.position ASC, categories.id ASC
DB-->>Q: categories (deduped, ordered)
Q-->>U: ActiveRecord::Relation
end
deactivate Q
U-->>U: .first で対象カテゴリを選択
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.81.1)app/queries/ordered_categories_from_practices_query.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in app/queries/user_unstarted_practices_query.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in app/models/user_course_practice.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in
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. Comment |
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
2 times, most recently
from
29d2e8b to
28995cc
Compare
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
2 times, most recently
from
50de492 to
c7288fc
Compare
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
4 times, most recently
from
c5708aa to
ccb12d9
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: 1
🧹 Nitpick comments (7)
app/queries/user_unstarted_practices_query.rb (1)
20-20: 同順位時の安定ソートを追加してください同じ
courses_categories.positionかつcategories_practices.positionの並びが複数ある場合、DB実装依存で順序が不安定になります。安定化のため ID をタイブレークに追加することを推奨します。以下の差分を提案します:
- .order('courses_categories.position ASC, categories_practices.position ASC') + .order('courses_categories.position ASC, categories_practices.position ASC, practices.id ASC')test/queries/category_from_practices_query_test.rb (2)
12-12: 型アサートはassert_kind_ofを使用すると可読性が上がります失敗時のメッセージが明確になります。
- assert result.is_a?(ActiveRecord::Relation) + assert_kind_of ActiveRecord::Relation, result @@ - assert result.is_a?(ActiveRecord::Relation) + assert_kind_of ActiveRecord::Relation, result @@ - assert result.is_a?(ActiveRecord::Relation) + assert_kind_of ActiveRecord::Relation, resultAlso applies to: 27-28, 36-36
47-53: 順序検証は配列化せずpluckで簡潔に余計なオブジェクト化を避けつつ、期待順を直接比較できます。
- categories_array = result.to_a - first_category = categories_array.first - second_category = categories_array.second - - assert_equal categories(:category2).id, first_category.id - assert_equal categories(:category4).id, second_category.id + assert_equal [categories(:category2).id, categories(:category4).id], result.pluck(:id)test/models/user_course_practice_test.rb (2)
57-68: こちらも同様にfixture参照へ
533_964_039をfixture参照へ置換してください。- assert_equal 533_964_039, user_course_practice.category_active_or_unstarted_practice.id + # 例: category4 が次カテゴリの場合 + assert_equal categories(:category4).id, user_course_practice.category_active_or_unstarted_practice.id
44-55: マジックナンバーをfixture参照に置き換え
テスト内の固定ID（917_504_053）はfixture変更に弱いため、以下のようにfixture参照に置き換えてください。- assert_equal 917_504_053, user_course_practice.category_active_or_unstarted_practice.id + assert_equal categories(:該当カテゴリ).id, user_course_practice.category_active_or_unstarted_practice.idapp/queries/category_from_practices_query.rb (2)
14-22: ORDER BYの生SQLを避けて安定ソートを付与（同順位時の揺れ防止）
'courses_categories.position ASC'は生SQL依存で、同一positionが並ぶ場合に並びが不安定です。ハッシュ構文＋IDタイブレークで安定化しましょう。適用差分:
def query return relation.none if @practices.blank? relation .joins(:courses_categories) .where(courses_categories: { course_id: @user.course_id }) .where(id: category_ids_with_practices) - .order('courses_categories.position ASC') + .order(courses_categories: { position: :asc }) + .order(id: :asc) end
24-29: サブクエリのSELECTで生SQLを排し、ARのdistinctを使用
select('DISTINCT categories.id')はDB方言に寄ります。ARのselect(:id).distinctで可搬性と可読性を上げましょう（結果は同等）。適用差分:
def category_ids_with_practices Category .joins(:practices) .where(practices: { id: practice_ids }) - .select('DISTINCT categories.id') + .select(:id) + .distinct end
📜 Review details
Configuration used: Path: .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 (6)
app/models/user_course_practice.rb(1 hunks)app/queries/category_from_practices_query.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)test/models/user_course_practice_test.rb(2 hunks)test/queries/category_from_practices_query_test.rb(1 hunks)test/supports/learning_helper.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/queries/user_unstarted_practices_query.rbapp/models/user_course_practice.rbapp/queries/category_from_practices_query.rbtest/queries/category_from_practices_query_test.rbtest/supports/learning_helper.rbtest/models/user_course_practice_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/queries/category_from_practices_query_test.rbtest/supports/learning_helper.rbtest/models/user_course_practice_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で対応すべきである。
🧬 Code graph analysis (2)
test/supports/learning_helper.rb (1)
app/models/practice.rb (1)
status(95-105)
test/models/user_course_practice_test.rb (2)
test/supports/learning_helper.rb (2)
set_learning_status(10-12)complete_all_practices_in_category(4-8)app/models/user_course_practice.rb (1)
category_active_or_unstarted_practice(35-41)
🔇 Additional comments (9)
app/queries/user_unstarted_practices_query.rb (2)
20-20: カテゴリ優先のソート変更は妥当（スクロール不具合の根本要因に合致）
courses_categories.position→categories_practices.positionの順での並び替えは、本PRの目的（先頭カテゴリへ正しくスクロール）に整合しています。
15-21: 呼び出し元はテストと user_course_practice のみ
UserUnstartedPracticesQuery はtest/queries/user_unstarted_practices_query_test.rbとapp/models/user_course_practice.rb#unstarted_practicesでのみ使われています。他に呼び出し元はないため、現状では副作用は発生しません。UI 表示や.first取得時に重複が問題にならないことを確認し、必要であれば.distinctの追加をご検討ください。app/models/user_course_practice.rb (2)
101-101: QueryObject への委譲は適切（再利用性・テスト容易性が向上）
CategoryFromPracticesQueryの採用で取得責務が明確化されました。OKです。
105-105: 未着手プラクティス側も同様にOK未着手のカテゴリ導出も QueryObject に統一され、挙動の一貫性が担保されています。
test/queries/category_from_practices_query_test.rb (1)
1-79: 網羅性の高いテスト追加は適切空/nil、順序、ユーザーコース制約、重複排除の各ケースを押さえており、回帰を抑止できます。
test/supports/learning_helper.rb (1)
3-12: テスト用ヘルパの切り出しは妥当
find_or_initialize_by + update!により既存/新規の両ケースを一手に扱えており、テストの可読性が向上。test/models/user_course_practice_test.rb (1)
5-5: LearningHelper の導入は適切テストの前提状態構築が簡潔になっています。
app/queries/category_from_practices_query.rb (2)
3-5: QueryObjectの導入は適切（責務の分離と再利用性が向上）PR目的（取得・ソートの明確化）に沿った設計で、
Patterns::Queryの利用もガイドラインと合致しています。
14-22:@user.course_idは常に存在するためガード不要
Userモデルのbelongs_to :courseにoptional: trueが指定されておらず、Rails 5+のバリデーションで必須となるため、course_idがnilとなることはありません。追加のblank?ガードは不要です。
|
@tyrrell-IH お手隙の際で構いませんのでよろしくお願い致します🙇♂️ |
|
@jun-kondo |
|
@tyrrell-IH |
|
@jun-kondo |
tyrrell-IH
left a comment
tyrrell-IH
left a comment
There was a problem hiding this comment.
@jun-kondo
レビューさせていただきました！
レビューした感想としては、全体の構成がよくできていて、レビューしていてとても勉強になりました。
そのままapproveでも良いのかなと思いましたが、一応少しコメントさせていただきました。
よかったらご検討ください
| end | ||
|
|
||
| def set_learning_status(user, practice, status) | ||
| Learning.find_or_initialize_by(user:, practice:).tap { |l| l.update!(status:) } |
There was a problem hiding this comment.
find_or_initialize_byというメソッドを知りませんでした、勉強になります🙏
There was a problem hiding this comment.
更新処理を連結する場合だとfind_or_create_byに対してDBの書き込みが1回で済みますね👀
There was a problem hiding this comment.
find_or_create_byだとレコードが見つからなかった時点でINSERTが実行されてしまうんですね、なるほど。。
| result = CategoryFromPracticesQuery.new(user:, practices:).call | ||
|
|
||
| assert_kind_of ActiveRecord::Relation, result | ||
| assert result.count.positive? |
There was a problem hiding this comment.
result.count.positive?はresult.any?の方が読みやすいのかなと思いました。
result.countで発行されるクエリはSELECT COUNT(*) FROM ~なのに対し
result.any?は SELECT 1 AS one FROM "categories" LIMIT $1 [["LIMIT", 1]]のような感じで1件だけ検索するので、
少し軽量かなと思いました。
There was a problem hiding this comment.
ありがとうございます。
調べてみると、ブロック無しのany?とexists?は同じクエリが発行されるようなので、exists?の方を採用しました👀
またその他の箇所の存在確認にcountを使う必要の無い箇所も修正しました。
| @@ -0,0 +1,35 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class CategoryFromPracticesQuery < Patterns::Query | |||
There was a problem hiding this comment.
このQueryオブジェクトにおいて、カテゴリーの順番を保証することが重要な要素のような気がするので、
OrderedCategoriesFromPracticesQueryとかの方が良いのかなと思いました。
There was a problem hiding this comment.
ありがとうございます😃
そうですね👀
クラス名を変更します。併せてファイル名や使用箇所部分も変更しました。
| class CategoryFromPracticesQueryTest < ActiveSupport::TestCase | ||
| test 'should return categories associated with practices and user course' do | ||
| user = users(:komagata) # course1のユーザー | ||
| practices = [practices(:practice1), practices(:practice2)] |
There was a problem hiding this comment.
CategoryFromPracticesQueryにpracticesとして渡す値は配列ではなくActiveRecord::Relationではないでしょうか？
テストの際にも
practices = Practice.where(id: [practices(:practice1).id, practices(:practice2).id])のようにしてActiveRecord::Relationを渡して検証する方が良いのでは？と思いました。
There was a problem hiding this comment.
ありがとうございます👀
そうですね。今回作成したQueryObjectクラスはActiveRecord::Relationが来ることを前提にしているので、Arrayを渡すのはおかしいですね。
こちら修正します。
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
from
8c647f1 to
745ecc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/models/user_course_practice_test.rb (1)
9-9: ヘルパ導入による可読性向上への賛同（既出コメントと同旨）過去指摘の通り、前処理の切り出しは読みやすさに効いています。
🧹 Nitpick comments (10)
test/queries/ordered_categories_from_practices_query_test.rb (3)
27-27: アサーションメッセージの表現を改善しましょう「practice」ではなく「カテゴリ」を主語にした方が意図が伝わります。
- assert_empty result, 'practiceが含まれています。' + assert_empty result, '空であるべき結果にカテゴリが含まれています。'Also applies to: 36-36
46-46: countよりsizeの方が無駄なSQLを避けやすいです未ロード時はcountと同等ですが、既にロード済みの可能性もあるためsizeを推奨。
- assert_equal 2, result.count + assert_equal 2, result.size
67-67: ID指定の一貫性を揃えましょう他と同様に
.idで統一すると読み手の混乱を避けられます（動作は現状でも問題ありません）。- practices = Practice.where(id: [practices(:practice2).id, practices(:practice4), practices(:practice9).id]) # 全てcategory4 + practices = Practice.where(id: [practices(:practice2).id, practices(:practice4).id, practices(:practice9).id]) # 全てcategory4app/queries/ordered_categories_from_practices_query.rb (4)
17-22: 重複行の混入に備えてdistinctを付与結合条件のユニーク制約が将来崩れた場合でも結果集合の重複を防げます。
relation .joins(:courses_categories) .where(courses_categories: { course_id: @user.course_id }) .where(id: category_ids_with_practices) + .distinct .order('courses_categories.position ASC, categories.id ASC')
24-34: 大規模ID集合でも効率的に扱えるようにIDs取得をサブクエリ化
pluckで配列化せず、サブクエリを渡すとメモリ効率とSQL最適化の余地が増えます。配列渡しにも対応します。- def practice_ids - @practices.pluck(:id) - end + def practice_ids + return @practices.select(:id) if @practices.is_a?(ActiveRecord::Relation) + Practice.where(id: Array(@practices)).select(:id) + end
14-16: 念のためuserのnilガードも追加現状の呼び出しではnil想定はありませんが、防御的に。
- return relation.none if @practices.blank? + return relation.none if @practices.blank? || @user.blank?
17-22: インデックス最適化の検討（パフォーマンス助言）高負荷環境を想定し、以下の複合インデックスを検討ください。
- courses_categories(course_id, position)
- categories_practices(practice_id, category_id) または (category_id, practice_id)
test/models/user_course_practice_test.rb (3)
44-55: テスト初期化のコスト削減を検討（コールバック不要ならdelete_all）
Learningに依存するコールバックがなければ速度重視でdelete_allも選択肢です。現状のdestroy_allでも正しさはOK。- user.learnings.destroy_all + user.learnings.delete_all # ※コールバック不要な場合のみ
54-55: オブジェクト同士で比較すると失敗時メッセージが分かりやすいですIDではなくレコード比較にすると差分表示が親切です。
- assert_equal categories(:category2).id, user_course_practice.category_active_or_unstarted_practice.id + assert_equal categories(:category2), user_course_practice.category_active_or_unstarted_practice
57-68: 状態遷移をassert_changesで一発検証開始カテゴリから次カテゴリへの変化を明示できます。
- current_category = user_course_practice.category_active_or_unstarted_practice - complete_all_practices_in_category(user, current_category) - user_course_practice = UserCoursePractice.new(user) - - assert_equal categories(:category4).id, user_course_practice.category_active_or_unstarted_practice.id + current_category = user_course_practice.category_active_or_unstarted_practice + assert_changes -> { UserCoursePractice.new(user).category_active_or_unstarted_practice.id }, + from: current_category.id, to: categories(:category4).id do + complete_all_practices_in_category(user, current_category) + end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/models/user_course_practice.rb(1 hunks)app/queries/ordered_categories_from_practices_query.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)test/models/user_course_practice_test.rb(2 hunks)test/queries/ordered_categories_from_practices_query_test.rb(1 hunks)test/supports/learning_helper.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/queries/user_unstarted_practices_query.rb
- app/models/user_course_practice.rb
- test/supports/learning_helper.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/queries/ordered_categories_from_practices_query_test.rbapp/queries/ordered_categories_from_practices_query.rbtest/models/user_course_practice_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/queries/ordered_categories_from_practices_query_test.rbtest/models/user_course_practice_test.rb
🧬 Code graph analysis (1)
test/models/user_course_practice_test.rb (2)
test/supports/learning_helper.rb (2)
set_learning_status(10-12)complete_all_practices_in_category(4-8)app/models/user_course_practice.rb (1)
category_active_or_unstarted_practice(35-41)
⏰ 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 (3)
test/queries/ordered_categories_from_practices_query_test.rb (2)
6-19: 網羅性の高い正当性テスト、良いです関連カテゴリの抽出と存在確認、並びにフィクスチャの前提が明確で読みやすいです。
52-55: 確認済み — 'unity-course' フィクスチャキーは存在しますtest/fixtures/users.yml の 1092 行に
unity-course:が見つかりました。綴りの修正は不要です。test/models/user_course_practice_test.rb (1)
5-5: LearningHelperの読み込み追加、良いです前処理の簡潔化に寄与しておりテスト意図が明瞭です。
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
from
745ecc6 to
3b766d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
test/queries/ordered_categories_from_practices_query_test.rb (8)
8-9: where にそのままレコードを渡せるため .id は不要（簡潔化）ActiveRecord は where(id: [records]) で id を解決します。.id を省いて可読性を上げられます。
- practices = Practice.where(id: [practices(:practice1).id, practices(:practice2).id]) + practices = Practice.where(id: [practices(:practice1), practices(:practice2)])
16-18: pluck(:id) より result.ids が簡潔で意図が明確Rails の Relation#ids は PK のみを pluck するシンタックスシュガーです。
- category_ids = result.pluck(:id) - assert_includes category_ids, categories(:category2).id - assert_includes category_ids, categories(:category4).id + ids = result.ids + assert_includes ids, categories(:category2).id + assert_includes ids, categories(:category4).id
21-29: 空入力の検証はOK。none? を使うと意図がより明確none? は「空集合を返す Relation」を直接表現できます。
- assert_empty result, '空であるべき結果にカテゴリが含まれています。' - assert_kind_of ActiveRecord::Relation, result + assert_kind_of ActiveRecord::Relation, result + assert_predicate result, :none?, '空であるべき結果にカテゴリが含まれています。'
31-38: nil 入力時も none? で明確に上と同様に、表現を統一しておくと読みやすいです。
- assert_empty result, '空であるべき結果にカテゴリが含まれています。' - assert_kind_of ActiveRecord::Relation, result + assert_kind_of ActiveRecord::Relation, result + assert_predicate result, :none?, '空であるべき結果にカテゴリが含まれています。'
46-50: クエリ数を減らしつつ順序の検証を簡潔にsize と pluck で二度クエリが走り得ます。ids にまとめると読みやすく、無駄も減らせます。
- assert_equal 2, result.size + ids = result.ids + assert_equal 2, ids.size @@ - assert_equal [categories(:category2).id, categories(:category4).id], result.pluck(:id) + assert_equal [categories(:category2).id, categories(:category4).id], ids
40-50: セカンダリソート（categories.id ASC）の退行検知を追加すると安心courses_categories.position が同値のケースで categories.id ASC が効いているかを検証するテストがあると将来の変更に強くなります（fixtures に同順位がなければ別途準備 or 後続PRでOK）。
71-76: 重複排除の検証を1アサーションに集約uniq と件数・先頭要素の三段チェックは冗長です。期待するID配列をそのまま比較すると簡潔です。
- category_ids = result.pluck(:id) - assert_equal category_ids.uniq.size, category_ids.size - - assert_equal 1, category_ids.size - assert_equal categories(:category4).id, category_ids.first + ids = result.ids + assert_equal [categories(:category4).id], ids
54-54: fixture 名の可読性（任意）test/fixtures/users.yml に 'unity-course:' として定義されています（行1092–1094）。users(:'unity-course') はユーザーを指しますが、unity_course_user のようにすると可読性が上がります。参照が多ければ現状維持で可。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/models/user_course_practice.rb(1 hunks)app/queries/ordered_categories_from_practices_query.rb(1 hunks)test/models/user_course_practice_test.rb(2 hunks)test/queries/ordered_categories_from_practices_query_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/user_course_practice.rb
- test/models/user_course_practice_test.rb
- app/queries/ordered_categories_from_practices_query.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/queries/ordered_categories_from_practices_query_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/queries/ordered_categories_from_practices_query_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 (2)
test/queries/ordered_categories_from_practices_query_test.rb (2)
5-13: 基本シナリオの網羅性は十分（LGTM）Relation 返却・存在確認・カテゴリ包含までのHappy Pathを押さえており、意図が明確です。
61-63: コース差分のフィルタリング確認は妥当（LGTM）exists?/not exists? の対で意図が明瞭です。
|
@tyrrell-IH レビューとは関係ない場所で1箇所だけnitpickのコメントを参考にテストの書き方を修正しました。(3b766d9) お手隙の際にご確認宜しくお願い致します🙏 |
|
@komagata |
|
@tyrrell-IH |
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
from
3b766d9 to
88ce4c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/supports/learning_helper.rb (1)
4-8: 一括更新はトランザクションで原子化し、配列を返すとテストで扱いやすいです途中で1件でも失敗した場合に中途半端な状態が残るのを防げます。戻り値として更新したLearningの配列を返すとアサーションもしやすいです。
- def complete_all_practices_in_category(user, category) - category.practices.each do |practice| - set_learning_status(user, practice, :complete) - end - end + def complete_all_practices_in_category(user, category) + ActiveRecord::Base.transaction do + category.practices.map { |practice| set_learning_status(user, practice, :complete) } + end + endapp/models/user_course_practice.rb (2)
35-41: アクティブ基準でカテゴリが見つからなかった場合のフォールバックを追加
@user.active_practices.present?でもクエリ結果が空になるケースではnilを返してしまい、未着手側へのフォールバックが実行されません。結果の有無で分岐するようにすると堅牢です。参考実装（変更範囲外のためサンプル）:
def category_active_or_unstarted_practice if @user.active_practices.present? category = category_having_active_practice return category if category.present? end if unstarted_practices.present? return category_having_unstarted_practice end end
101-102: QueryObject呼び出しの共通化を検討してください
category_having_active_practiceとcategory_having_unstarted_practiceで同一ロジックが重複しているため、privateメソッド（例:ordered_category_from(practices))へ抽出すると保守性が向上します。OrderedCategoriesFromPracticesQuery#callは常にActiveRecord::Relationを返すため、追加のnilチェックは不要です。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/models/user_course_practice.rb(1 hunks)app/queries/ordered_categories_from_practices_query.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)test/models/user_course_practice_test.rb(2 hunks)test/queries/ordered_categories_from_practices_query_test.rb(1 hunks)test/supports/learning_helper.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/queries/ordered_categories_from_practices_query_test.rb
- app/queries/ordered_categories_from_practices_query.rb
- test/models/user_course_practice_test.rb
- app/queries/user_unstarted_practices_query.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/supports/learning_helper.rbapp/models/user_course_practice.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/supports/learning_helper.rb
🧬 Code graph analysis (1)
test/supports/learning_helper.rb (1)
app/models/practice.rb (1)
status(94-104)
⏰ 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/supports/learning_helper.rb (1)
10-12: find_or_initialize_by + update! の選択は妥当。DBの一意制約だけ確認させてください新規作成時に1回の書き込みで済む構成で良いです。一方で（テストでは並行実行は稀ですが）重複Learning防止のため、
[user_id, practice_id]の一意制約（DBインデックス or モデルバリデーション）があるか確認を。#!/bin/bash # Learningモデルの一意性バリデーションとDBユニークインデックスを確認します echo "== Learning model validations ==" fd -t f learning.rb app | while read -r f; do echo "-> $f" rg -n -C2 -e 'validates\s+:user_id' -e 'validates\s+:practice_id' -e 'validates_uniqueness_of' "$f" || true rg -n -C2 -e 'uniqueness:\s*{[^}]*scope:\s*:practice_id' "$f" || true rg -n -C2 -e 'validates\s+:user_id,\s*uniqueness:.*scope:\s*:practice_id' "$f" || true done echo echo "== DB unique index on learnings(user_id, practice_id) ==" rg -n -C2 -e 'add_index .*:learnings.*\[:user_id,\s*:practice_id\]' db/migrate || true rg -n -C2 -e 'add_index .*:learnings.*unique:\s*true' db/migrate || true
|
@jun-kondo |
|
@tyrrell-IH |
|
@okuramasafumi |
okuramasafumi
left a comment
okuramasafumi
left a comment
There was a problem hiding this comment.
https://github.com/Selleo/pattern?tab=readme-ov-file#query
ここのREADMEに書かれているに書かれているUsageの部分と違う書き方になっているのが気になりました。既存のQueryオブジェクトも同様の書き方になっているのでこのプルリクエストの問題ではないのですが、本来は全部READMEに揃えたほうがよいと思います。
揃えるかどうかについては一度議論してみるのが良いと思いますが、いったんこのままでいくのもアリかもしれません。その場合はもう一度レビュー依頼を出していただければと思います。
それ以外は大丈夫です、テストがしっかり書かれていてとても良いと思います！
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
from
88ce4c0 to
1737183
Compare
|
@okuramasafumi
背景として、メンバーはQueryObject · fjordllc/bootcamp Wikiのドキュメントを参考に書いているからだと思います。
一旦「レビューで今回のコメントがありましたー」という形でミーティングで共有して追々意見を頂いてみようと思います。 チーム開発的に記法がバラバラだと良くないということであれば元に戻します。 |
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
from
1737183 to
0c72a03
Compare
- 共通ロジックを新規作成したOrderedCategoriesFromPracticesQueryクラスに抽出 - courses_categoriesモデルの並び順の指定をアソシエーションからorderメソッドを使用するよう変更 - カテゴリ並び順を明示的に指定しクエリの可読性を向上 - OrderedCategoriesFromPracticesQueryTestを新規作成しカテゴリ取得ロジックのテストを追加
- テストケースを着手中プラクティスがある場合とない場合に分割 - テストで複数カテゴリのシナリオをカバーするために`LearningHelper`を導入 - プラクティス完了ステータス設定メソッドをHelperに抽出 - テストケースのコメントを追加し、取得カテゴリとプラクティスの関連性を明確化
- 初期化メソッドは親クラスのものを使用する - 渡された引数は`options`変数からハッシュのキーで取得する - 必須引数`user`のバリデーションを追加した
- `new(...).call`ではなく親クラス`Patterns::Query`の`self.call`を使う
jun-kondo
force-pushed
the
bug/scroll-to-category-in-practices-page
branch
from
0c72a03 to
6b53070
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/supports/learning_helper.rb (1)
4-8: 任意: 戻り値の追加を検討してみてください。現在のメソッドは暗黙的に配列を返しますが、明示的に更新されたLearningレコードの配列や件数を返すと、テストでの検証がより柔軟になる可能性があります。
例えば、以下のように修正できます:
def complete_all_practices_in_category(user, category) - category.practices.each do |practice| + category.practices.map do |practice| set_learning_status(user, practice, :complete) end endtest/queries/ordered_categories_from_practices_query_test.rb (1)
6-19: テストケースの構造と内容が良好です。基本的な動作を適切にテストしており、以下の点が評価できます：
- ActiveRecord::Relationの型チェック
- 結果の存在確認
- 期待されるカテゴリIDの包含確認
- 日本語コメントによる明確な説明
必要に応じて、結果の正確なサイズを検証するアサーション（例：
assert_equal 2, result.size）を追加することで、テストの厳密性をさらに高めることができます。</thinking_end -->
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/models/user_course_practice.rb(1 hunks)app/queries/ordered_categories_from_practices_query.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)test/models/user_course_practice_test.rb(2 hunks)test/queries/ordered_categories_from_practices_query_test.rb(1 hunks)test/queries/user_unstarted_practices_query_test.rb(5 hunks)test/supports/learning_helper.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/models/user_course_practice_test.rb
- app/queries/ordered_categories_from_practices_query.rb
- app/queries/user_unstarted_practices_query.rb
- app/models/user_course_practice.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/queries/ordered_categories_from_practices_query_test.rbtest/queries/user_unstarted_practices_query_test.rbtest/supports/learning_helper.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/queries/ordered_categories_from_practices_query_test.rbtest/queries/user_unstarted_practices_query_test.rbtest/supports/learning_helper.rb
🧬 Code graph analysis (2)
test/queries/ordered_categories_from_practices_query_test.rb (1)
app/queries/ordered_categories_from_practices_query.rb (2)
user(30-35)practices(37-39)
test/supports/learning_helper.rb (1)
app/models/practice.rb (1)
status(94-104)
⏰ 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 (8)
test/supports/learning_helper.rb (2)
3-13: LGTM! テストヘルパーとして適切な実装です。新しいテストヘルパーモジュール
LearningHelperの実装は明確で、テストデータのセットアップに適しています。学習ステータスの操作を簡潔に記述でき、テストの可読性が向上します。
10-12: LGTM! 適切なActiveRecordパターンの使用です。
find_or_initialize_byとupdate!の組み合わせは、過去のレビューコメントでも言及されている通り、レコードが存在しない場合にINSERTを1回で済ませる効率的な実装です。tapを使用してLearningインスタンスを返すのも、テストでの更なるセットアップに活用できて便利です。test/queries/user_unstarted_practices_query_test.rb (1)
13-13: LGTM! Queryオブジェクトの呼び出し方法が統一されています。インスタンスベースの呼び出しからクラスメソッド
callへの変更は、rails-patterns gem のQuery機能の標準的なパターンに準拠しており、適切です。テストロジックとアサーションに変更がないため、既存のテストカバレッジが保たれています。PRコメントで言及されているチーム内での記法統一の議論も進行中とのことで、この変更はその方向性と整合しています。
Also applies to: 25-25, 34-34, 43-43, 52-53
test/queries/ordered_categories_from_practices_query_test.rb (5)
21-29: エッジケースのテストが適切です。
Practice.noneを使用した空のプラクティス集合のテストケースが正しく実装されています。assert_emptyとassert_kind_ofの両方でチェックしており、日本語のアサーションメッセージも明確です。</thinking_end -->
31-38: nilケースのテストが適切です。
practices: nilのエッジケースを適切にテストしており、空の結果とActiveRecord::Relationの型を確認しています。</thinking_end -->
40-50: 順序テストが厳密に実装されています。
courses_categories.positionによるソート順を正確に検証しており、結果のサイズと具体的なID配列の順序を確認しています。コメントによる期待値の説明も明確です。</thinking_end -->
52-63: コースフィルタリングのテストが効果的です。異なるコースに所属する2人のユーザーで同じプラクティスをテストすることで、ユーザーのコースに基づくフィルタリングが正しく機能していることを検証しています。
</thinking_end -->
65-76: 重複除去のテストが包括的です。同じカテゴリに属する複数のプラクティスを使用して、カテゴリの重複除去が正しく機能していることを検証しています。Line 72の
category_ids.uniq.sizeとの比較による一意性チェックが効果的です。</thinking_end -->
|
@okuramasafumi @komagata |
|
@jun-kondo (CC: @okuramasafumi ) マージしました〜！ |
|
@komagata |
4 participants
Issue
概要
BootCampアプリではサイドのナビゲーションバーのプラクティスのリンクのアンカーにプラクティスの所属するカテゴリのidが入ることで、リンクをクリックしたときにカテゴリの位置までスクロールした状態でプラクティス一覧ページに遷移できるもようになっています。
このアンカーに入るカテゴリを特定するロジックを変更しました。
具体的には、カテゴリテーブルから着手中or未着手のプラクティスが所属するすべてのカテゴリかつ、ユーザーの受講しているコースに含まれるカテゴリに限定して抽出し、プラクティス一覧でページの並び順と矛盾しないようソートする専用のクエリを作成し、従来のアソシエーション経由でのカテゴリ取得から変更しました。
変更前ソートやコースによるカテゴリの限定がされておらず、下記の不具合が発生しておりそれらの解消を目的としています。
また変更により下記の動作になるようにしています。
※ この機能の最初の実装時にはRailsコース以外がなかったためカテゴリが複数のコースを持つことが想定されていなかった。プラクティスからアソシエーションでカテゴリを取得する従来の方法だと、コース関係なくプラクティスに紐づくすべてのカテゴリを取ってきてしまう。
変更確認方法
ブランチをローカルに取り込む
ローカルサーバーの立ち上げ
着手中のプラクティスがない場合
ユーザー
hatsunoでログイン(パスワード:testtest)プラクティス一覧ページに行き下記を確認する
着手ステータスのプラクティスがないことを確認する
Mac OS XカテゴリのOS X Mountain Lionをクリーンインストールするが未着手になっていることを確認する
どこでも良いのでアプリ内別のページに遷移し、サイドナビゲーションバーのプラクティスをクリックしプラクティスページに戻る
Mac OS Xカテゴリのすべてのプラクティスを修了にする
どこでも良いのでアプリ内別のページに遷移し、サイドナビゲーションバーのプラクティスをクリックしプラクティスページに戻る
※ ステータス変更時はナビゲーションバーのリンクが更新されないので、別ページに遷移してからプラクティスページのリンクをクリックするか、リンクを2回クリックするとスクロール位置が更新されます。
着手中のプラクティスが複数ある場合
ユーザー
hatsunoでログイン(パスワード:testtest)プラクティス一覧ページに行き下記を確認する
Rubyカテゴリのrubyをインストールするを着手にする
どこでも良いのでアプリ内別のページに遷移し、サイドナビゲーションバーのプラクティスをクリックしプラクティスページに戻る
UNIXカテゴリのTerminalの基礎を覚えるを着手にする
どこでも良いのでアプリ内別のページに遷移し、サイドナビゲーションバーのプラクティスをクリックしプラクティスページに戻る
※ ステータス変更時はナビゲーションバーのリンクが更新されないので、別ページに遷移してからプラクティスページのリンクをクリックするか、リンクを2回クリックするとスクロール位置が更新されます。
Screenshot
着手中のプラクティスがない場合
Mac OS Xカテゴリのすべてのプラクティスを修了すると、次回からのプラクティスページ一覧に遷移したときは次のUNIXカテゴリに遷移された状態で表示されます。
着手中のプラクティスが複数ある場合
rubyをインストールするを着手したとき(1つめの着手プラクティス)Terminalの基礎を覚えるを着手すると(2つめの着手プラクティス)
次回からのプラクティスページ一覧に遷移したときは次のUNIXカテゴリに遷移された状態で表示されます。
Summary by CodeRabbit
バグ修正
リファクタ
新機能（内部）
テスト