Conversation
|
""" Walkthrough新たにユーザーのプラクティス進捗やチェックのコピーを担うクラス・サービス・プレゼンターが追加されました。進捗の個別・一括移行、コピー状況の可視化、チェックの重複防止付きコピーなどを担うロジックと、それらのテストが実装されています。 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PracticeProgressMigrator
participant PracticeProgressPresenter
participant CopyPracticeProgress
participant CopyCheck
User->>PracticeProgressMigrator: migrate_all()
PracticeProgressMigrator->>PracticeProgressPresenter: completed_practices
loop 各完了プラクティス
PracticeProgressMigrator->>CopyPracticeProgress: call(original, copied)
CopyPracticeProgress->>CopyCheck: call(original_product, copied_product)
CopyCheck-->>CopyPracticeProgress: checks copied/skipped
CopyPracticeProgress-->>PracticeProgressMigrator: progress copied
end
PracticeProgressMigrator-->>User: migration result
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.75.5)db/migrate/20250618144325_add_foreign_key_to_practices_source_id.rbrubocop-minitest extension supports plugin, specify db/schema.rbrubocop-minitest extension supports plugin, specify test/models/practice_test.rbrubocop-minitest extension supports plugin, specify
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
Gemfile (1)
65-65: バージョンを固定して再現性を担保しましょう
rails-patternsをバージョン指定なしで追加すると、将来の gem 更新による挙動変化や互換性問題を CI／本番環境で拾いづらくなります。~>などで最小限のバージョンを固定しておくことを推奨します。-gem 'rails-patterns' +gem 'rails-patterns', '~> 0.2'config/application.rb (1)
24-26: eager_load 設定の統一を検討
config.paths.add "app/presenters", eager_load: trueは動作上問題ありませんが、Zeitwerk ではconfig.eager_load_paths << Rails.root.join('app/presenters')を用いる例が一般的です。
既存の"lib"ディレクトリ設定と書式を合わせることで可読性が向上します。app/models/course.rb (2)
4-6: DEFAULT_COURSE のエイリアス化が二重管理になりやすい
DEFAULT_COURSE = RAILS_COURSEとして別名を用意していますが、今後タイトルが変わった際に両定数を更新し忘れるリスクがあります。
RAILS_COURSEを削除してDEFAULT_COURSEだけを使用する- あるいは
DEFAULT_COURSEを廃止し、呼び出し側でCourse.rails_courseを利用など、定数を一つに集約する設計をご検討ください。
19-21:rails_courseとdefault_courseの責務が重複両メソッドとも実装が
find_by(title: …)で実質同じです。メソッドを一本化して呼び出し側で適切に命名した方が可読性と保守性が高まります。config/routes/admin.rb (1)
10-14: リソース名を複数形またはresourceに統一すると可読性が向上します
resources :practice_progressだと URL/ヘルパが...practice_progress_indexとなり Rails の慣習的な複数形とズレます。
- １件ずつ扱うなら
resource :practice_progress- 複数扱うなら
resources :practice_progressesに変更することを検討してください。既存コードへの影響はヘルパ名とパスのみです。
app/queries/completed_learnings_query.rb (1)
10-14: ハードコード文字列より enum を利用すると保守性が高まります
where(status: 'complete')は enum 名変更時に壊れます。-.where(status: 'complete', courses_categories: { course: @course }) +.where(status: Learning.statuses[:complete], courses_categories: { course: @course })また
order('learnings.updated_at asc')はorder(updated_at: :asc)の方が ARel ベースで安全です。app/models/practice.rb (1)
54-57:source_idに DB 制約・インデックスを追加することを推奨します
自己参照関連を多用するクエリ性能と整合性確保のため、practices.source_idへの外部キー制約（ON DELETE SET NULL）とインデックスを追加してください。app/views/admin/users/practice_progress/index.html.slim (1)
48-55: ビューでのロジック肥大化を抑えてください
ruby:ブロックで複数の変数を計算しており、ビューが読みづらくなっています。
Presenter かヘルパーに計算を委譲し、Slim テンプレートはできるだけ表示専用にすると保守性が向上します。test/system/admin/users/practice_progress_test.rb (1)
47-50: コメントアウトされた期待は削除を検討してください意図しない将来仕様を示唆する不要なコメントはテストの可読性を下げます。不要なら消してください。
app/presenters/practice_progress_presenter.rb (1)
13-26:includesのネストが複雑です深いネストをそのまま書くと読み取りが困難になります。
スコープや private メソッドに分割し、目的ごとにincludes_rails_course_associationsなど命名すると理解しやすくなります。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/copilot/guidelines.yml(1 hunks)Gemfile(1 hunks)app/controllers/admin/users/practice_progress_controller.rb(1 hunks)app/models/course.rb(2 hunks)app/models/practice.rb(2 hunks)app/models/practice_progress_migrator.rb(1 hunks)app/models/user.rb(1 hunks)app/presenters/practice_progress_presenter.rb(1 hunks)app/queries/completed_learnings_query.rb(1 hunks)app/views/admin/users/practice_progress/index.html.slim(1 hunks)config/application.rb(1 hunks)config/routes/admin.rb(1 hunks)test/models/practice_progress_migrator_test.rb(1 hunks)test/system/admin/users/practice_progress_test.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/models/practice.rb (2)
app/models/course.rb (1)
has_many(3-22)app/models/practices_movie.rb (1)
belongs_to(3-6)
app/models/practice_progress_migrator.rb (2)
app/presenters/practice_progress_presenter.rb (1)
completed_practices(13-26)app/models/practice.rb (2)
learning(126-128)status(94-104)
app/presenters/practice_progress_presenter.rb (1)
app/models/practice.rb (1)
learning(126-128)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
Gemfile (1)
65-65: Rails 6.1 / Ruby 3.1 との互換性確認を
rails-patternsは gem の歴史が浅く、Rails 7 系での利用例が多い印象です。Rails 6.1.7.10 & Ruby 3.1.6 環境での動作保証が公式に明記されているかご確認ください。
問題があればフォーク／バージョンピンでの対応も検討をお願いします。.github/copilot/guidelines.yml (1)
1-13: ガイドライン追加を確認しました日本語レビューの指針が明確になり、運用ルールが共有しやすくなる点を評価します。
app/models/user.rb (1)
591-593: LGTM – メソッド移動のみ
practice_ids_skippedの実装変更はなく、N+1 も発生しません。問題ありません。test/models/practice_progress_migrator_test.rb (1)
1-14: テストカバレッジが充実しており素晴らしいです
主要な分岐・エラーケースを網羅しており、回帰防止に寄与します。
| def bulk_copy | ||
| migrator = PracticeProgressMigrator.new(@user) | ||
| result = migrator.migrate_all | ||
|
|
||
| redirect_to admin_user_practice_progress_index_path(@user), notice: result[:message] | ||
| end |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@komagata ありがとうございます！bulk_copy メソッドでの成否判定の修正、お疲れさまでした。これで管理者にとってより分かりやすいフィードバックが提供されますね。👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/admin/users/practice_progress_batches_controller.rb (1)
15-17:find例外の扱いを検討してください
User.findが見つからない場合ActiveRecord::RecordNotFoundが発生し、管理画面全体が 404 レンダリングされます。管理者向けとはいえ、専用のエラーページやメッセージを出したい場合はfind_by+ 早期 return か rescue ハンドリングを検討しても良いでしょう。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/admin/users/practice_progress_batches_controller.rb(1 hunks)app/controllers/admin/users/practice_progress_controller.rb(1 hunks)app/views/admin/users/practice_progress/index.html.slim(1 hunks)config/initializers/inflections.rb(1 hunks)config/routes/admin.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/initializers/inflections.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- config/routes/admin.rb
- app/controllers/admin/users/practice_progress_controller.rb
- app/views/admin/users/practice_progress/index.html.slim
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/views/admin/users/practice_progress/show.html.slim (2)
1-8: ページタイトルが汎用的すぎて意味が伝わりづらい
- title '管理ページ'だと画面固有の意図が読み取りづらく、SEO やブラウザタブの識別性も低いです。
例：'学習進捗コピー - 管理'など、機能を示すタイトルへ変更を検討してください。
91-97:rel属性にnoreferrerを追加するとより安全
target="_blank"に対しrel="noopener"は入っていますが、noreferrerも付与しておくとリファラ漏洩も防げます。test/system/admin/users/practice_progress_test.rb (2)
417-485: テストコードが重複しており保守負荷が高い同様の前準備（オリジナル/コピー作成・学習/提出物生成）が繰り返されています。
setupメソッドやヘルパーとして共通化すると読みやすくなります。
98-108:Time.current.to_iだけではタイトルの一意性が保証されない可能性同一秒内に複数レコードを作成すると衝突します。
SecureRandom.hex(4)などを併用して衝突耐性を高めると安心です。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Gemfile(1 hunks)app/controllers/admin/users/practice_progress_batches_controller.rb(1 hunks)app/controllers/admin/users/practice_progress_controller.rb(1 hunks)app/models/practice_progress_migrator.rb(1 hunks)app/views/admin/users/practice_progress/show.html.slim(1 hunks)config/routes/admin.rb(1 hunks)test/system/admin/users/practice_progress_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Gemfile
- config/routes/admin.rb
- app/controllers/admin/users/practice_progress_batches_controller.rb
- app/models/practice_progress_migrator.rb
- app/controllers/admin/users/practice_progress_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
app/views/admin/users/practice_progress/show.html.slim (1)
70-75:Details
❌ Incorrect review comment
confirm ダイアログが Turbo 環境で発火しない恐れ
Rails 7 + Turbo を使用している場合
data: { confirm: '...' }は無視され、
data: { turbo_confirm: '...' }が必要です。UJS を併用していないか要確認。- data: { confirm: '進捗をコピーしますか？' } + data: { turbo_confirm: '進捗をコピーしますか？' }
Turbo環境とUJS併用の有無を確認するため、以下のスクリプトを実行します。
🏁 Script executed:
#!/bin/bash # Rails本体とturbo-railsの導入状況を確認 grep -E "gem ['\"]rails['\"]" Gemfile grep -E "gem ['\"]turbo-rails['\"]" Gemfile || echo "turbo-rails not found in Gemfile" # importmapでpinされているかを確認 grep -R "pin \"turbo-rails\"" config/importmap.rb || echo "config/importmap.rb に turbo-rails の pin 設定なし" # JavaScript側のTurboおよびRails UJSのimport状況を確認 grep -R "import.*turbo" -n app/javascript || echo "app/javascript内に turbo の import 見つからず" grep -R "import.*@rails/ujs" -n app/javascript || echo "app/javascript内に @rails/ujs の import 見つからず"Length of output: 670
Turbo未導入のRails 6.1環境ではdata-confirmが有効です
Gemfileにturbo-railsがなく、JavaScriptにも Turbo が import されていないため、
data: { confirm: '…' }は Rails UJS のdata-confirm属性として動作します。
data-turbo-confirmへの変更は不要です。Likely an incorrect or invalid review comment.
test/system/admin/users/practice_progress_test.rb (1)
565-568: 複数同名リンクが存在すると誤クリックの可能性
click_link '全ての進捗をコピー'は最初にマッチしたリンクを押します。
将来的に同一文言が増えた場合にテストが不安定になるため、href:で限定するかwithinでスコープを絞ってください。
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/views/admin/users/practice_progress/show.html.slim (1)
48-55: N+1問題が解決されています過去のレビューで指摘されたN+1問題は、Presenterパターンの導入とeager loadingにより適切に解決されています。
learning.practiceはPresenterのcompleted_practicesメソッドで事前読み込みされており、その他のデータもキャッシュメカニズムにより効率的に取得されています。
🧹 Nitpick comments (4)
app/presenters/practice_progress_presenter.rb (4)
39-43: クエリのパフォーマンスを改善できます
completed_practices.pluck(:practice_id)の結果をインスタンス変数にキャッシュすることで、複数のメソッドで同じpluck操作を繰り返すのを避けられます。+ def practice_ids + @practice_ids ||= completed_practices.pluck(:practice_id) + end + def migration_candidates - ids = completed_practices.pluck(:practice_id) + ids = practice_ids completed_practices.joins(:practice).where(practices: { source_id: nil }) .where.not(practice_id: Practice.where(source_id: ids).pluck(:source_id)) end
60-60: 前述のpractice_idsキャッシュを活用してくださいここでも
completed_practices.pluck(:practice_id)を使用しています。前述のpractice_idsメソッドを使用することで一貫性とパフォーマンスが向上します。
75-80: キャッシュ実装は良いですが、さらに改善できます
practice_idsメソッドを活用することで、パフォーマンスがさらに向上します。def product_for(learning) - @products_cache ||= user.products.where(practice_id: completed_practices.pluck(:practice_id)) + @products_cache ||= user.products.where(practice_id: practice_ids) .includes(:practice, :checks) .index_by(&:practice_id) @products_cache[learning.practice_id] end
117-126: privateメソッドでもpractice_idsキャッシュを活用してください両メソッドで
completed_practices.pluck(:practice_id)を呼び出しています。practice_idsメソッドを使用することで、パフォーマンスが向上します。def copy_destinations - @copy_destinations ||= Practice.where(source_id: completed_practices.pluck(:practice_id)) + @copy_destinations ||= Practice.where(source_id: practice_ids) .pluck(:source_id) .to_set end def copy_destination_practices - @copy_destination_practices ||= Practice.where(source_id: completed_practices.pluck(:practice_id)) + @copy_destination_practices ||= Practice.where(source_id: practice_ids) .includes(:learnings, :products, learnings: :user, products: %i[user checks]) .index_by(&:source_id) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/presenters/practice_progress_presenter.rb(1 hunks)app/views/admin/users/practice_progress/show.html.slim(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/presenters/practice_progress_presenter.rb (1)
app/models/practice.rb (1)
learning(125-127)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
app/presenters/practice_progress_presenter.rb (2)
13-26: 効率的なeager loadingの実装を評価します関連データの事前読み込みが適切に実装されており、N+1クエリ問題を回避できています。
103-104: エイリアスの使用が適切ですメソッド名の一貫性を保つためのエイリアス定義は良い実践です。
app/views/admin/users/practice_progress/show.html.slim (3)
1-26: ヘッダー構造が適切に実装されています管理画面の標準的な構造に従っており、ナビゲーションも適切です。
70-75: 進捗コピー機能の実装が適切です確認ダイアログによる誤操作防止も実装されており、ユーザビリティに配慮されています。
97-100: 全進捗コピー機能が適切に実装されています重要な操作であることを示す警告色のボタンと確認ダイアログにより、誤操作を防ぐ設計になっています。
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
app/interactors/copy_practice_progress.rb (1)
37-41:messageキーの上書きで最終ステップの文言しか残りません複数ステップの成功メッセージを残したい場合、
merge_resultでハッシュをそのまま上書きすると最後に実行したインタラクタの:messageだけが残ります。メッセージを蓄積する、もしくは構造化（配列・ハッシュ）して持たせる設計をご検討ください。- result.to_h.each { |key, value| context[key] = value } + result.to_h.each do |key, value| + if key == :message + context[:messages] ||= [] + context[:messages] << value + else + context[key] = value + end + endtest/interactors/copy_product_test.rb (1)
62-71: 期待メッセージがプロダクトではなくラーニングを指しています
CopyProductのテストで"Learning copied successfully (no product to copy)"を期待すると意図が伝わりづらく、実装変更時に誤検知を招きます。プロダクト用のメッセージに合わせて修正してください。- assert_equal 'Learning copied successfully (no product to copy)', result.message + assert_equal 'No product to copy', result.messagetest/interactors/copy_check_test.rb (2)
41-47: メッセージが他インタラクタ用の文言になっています
CopyCheck単体では 学習・プロダクト のコピーは行わないため、
'Learning, product, and checks copied successfully'は誤解を招きます。
テストも実装もChecks copied successfully等に合わせましょう。
68-74: 上記と同様にメッセージが不適切ですこちらのケースも正しい文言へ更新してください。
app/models/practice_progress_migrator.rb (1)
40-46: ループ内でのPractice.find_byが N+1 の原因に
process_all_practicesでは完了済み Learning 毎にPractice.find_by(source_id: …)を発行しています。完了済みが多い場合にクエリが急増します。
あらかじめ target practice を一括取得しハッシュで参照するか、Practice.where(source_id: completed_practices.pluck(:practice_id))でまとめて取得するとパフォーマンスが向上します。app/interactors/copy_learning.rb (1)
21-25: Interactor の必須キーは DSL で宣言すると簡潔現在は手動で presence チェックを行っていますが、
Interactorにはrequired_context :user, :from_practice, :to_practiceの DSL があり、重複を避けられます。可読性向上のため検討ください。
app/interactors/copy_product.rb (1)
35-37: メッセージ文言が学習ではなく Product を指すべき
CopyProduct内で Product が存在しない場合のメッセージが
'Learning copied successfully (no product to copy)'となっており文脈と合いません。誤解を招くため「Product」に修正することを推奨します。- context.message = 'Learning copied successfully (no product to copy)' + context.message = 'No product found, skipping product copy'test/interactors/copy_practice_progress_test.rb (3)
12-16: fixture クリア処理が広すぎる／重複している可能性
@user.products.destroy_allでdependent: :destroyが設定されていればCheckも削除されるはずです。さらにCheck.where(checkable_type: 'Product').destroy_allで全ユーザー分のCheckを消しており、他テストへの副作用を起こす恐れがあります。テストの isolation はトランザクションで担保されるため、ここではユーザー関連だけを削除する、あるいはそもそも削除処理を省く方が安全です。-@user.products.destroy_all -Check.where(checkable_type: 'Product').destroy_all +# プロダクト削除のみで十分 +@user.products.destroy_all
20-26: status 文字列のハードコーディング
Learningに enum が定義されている場合、'complete'の直接指定ではなく:completeシンボルを使うと将来的なリネーム時に安全です。
35-37: メッセージの完全一致 Assert は brittle
文言が変わるとテストが壊れやすいため、assert_matchでキーフレーズのみ確認するか、成功フラグだけを検証する方が保守性が高まります。-assert_equal 'Learning, product, and checks copied successfully', result.message +assert_match(/copied successfully/, result.message)test/interactors/copy_learning_test.rb (2)
11-13: データ削除は最小限に
トランザクションフィクスチャが有効であれば@user.learnings.destroy_allは不要です。速度向上と意図の明確化のため削除を検討してください。
85-87: スタブの副作用を抑えるためスコープを限定
Learning.stub :create!は全テストで有効になります。ブロック内限定とはいえ、ネストされたメソッド呼び出しで思わぬ影響を与える可能性があるため、stubではなくmochaのような expectation ライブラリで対象メソッドだけを stub する、もしくはCopyLearningクラスに DI する手法がより安全です。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Gemfile(2 hunks)app/interactors/copy_check.rb(1 hunks)app/interactors/copy_learning.rb(1 hunks)app/interactors/copy_practice_progress.rb(1 hunks)app/interactors/copy_product.rb(1 hunks)app/models/practice_progress_migrator.rb(1 hunks)test/interactors/copy_check_test.rb(1 hunks)test/interactors/copy_learning_test.rb(1 hunks)test/interactors/copy_practice_progress_test.rb(1 hunks)test/interactors/copy_product_test.rb(1 hunks)test/models/practice_progress_migrator_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Gemfile
- test/models/practice_progress_migrator_test.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/interactors/copy_learning.rb (4)
app/interactors/copy_product.rb (3)
include(3-70)call(6-17)validate_inputs(21-25)app/interactors/copy_practice_progress.rb (2)
include(3-45)call(6-18)app/interactors/copy_check.rb (2)
include(3-86)call(6-12)app/models/practice.rb (1)
status(93-103)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
app/interactors/copy_learning.rb (1)
55-61: コピーする属性がstatusのみで十分か再確認を
Learningにはstarted_at,completed_at,remarksなど他メタ情報が存在する場合があります。進捗コピーの目的が「完了状態の再現」であればタイムスタンプも合わせて複製しないと履歴が失われる恐れがあります。要件を確認のうえ必要であれば追加してください。app/interactors/copy_check.rb (1)
72-83: コピー時にチェックの付帯情報が落ちる可能性
Check.create!でuserとcheckableしかコピーしていません。
モデルにcomment,created_at,updated_atなど追加属性が存在する場合、監査用途で困る可能性があります。必要に応じて属性コピーをご検討ください。test/interactors/copy_practice_progress_test.rb (1)
111-120: パラメータ不足テストの整合性を要確認
userだけnilを渡しているのに、期待エラーメッセージではuser, from_practice, to_practiceすべて欠如としています。実装側と食い違っていないか確認してください。test/interactors/copy_learning_test.rb (1)
70-78: エラーメッセージの期待値が実態と合致しているか確認
ここでもuser, from_practice, to_practiceすべて不足というメッセージを期待していますが、userのみnilです。実装の仕様を再確認し、テストを合わせてください。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/interactors/copy_check_test.rb (1)
12-15:destroy_allの多用はテストの速度と副作用を悪化させます
destroy_allは各レコードのコールバックを実行するため遅く、他テストの fixture に副作用を与える可能性があります。トランザクションで巻かれているテストならdelete_allで十分か、もしくは対象レコードを個別に作成する方式へ変更するとテスト実行時間の短縮と安全性の両立が期待できます。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/interactors/copy_check.rb(1 hunks)app/models/practice_progress_migrator.rb(1 hunks)test/interactors/copy_check_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/practice_progress_migrator.rb
- app/interactors/copy_check.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (3)
test/interactors/copy_check_test.rb (3)
40-44: 期待メッセージが CopyCheck の責務と合っているか確認してください
assert_equal 'Learning, product, and checks copied successfully', result.messageとしていますが、CopyCheck は checks のコピーのみを担当するクラスのはずです。上位 Interactor のメッセージを流用していないかご確認ください。責務に合った文言、あるいはassert_matchに切り替えることで将来の変更に強くなります。
89-98: 必須パラメータ欠如時に success? を期待する設計を再確認
original_productがnilの場合でもresult.success?を想定していますが、呼び出し側でバグを見落とす恐れがあります。エラー扱いにすべきか、設計意図を再確認のうえテスト期待値の修正を検討してください。
35-38: Interactor へ Context オブジェクトを渡さない形に修正された点を確認
ハッシュ引数でCopyCheck.callを呼んでおり、過去の指摘事項が解消されています。👍
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/presenters/practice_progress_presenter.rb (2)
26-30:includesの指定が冗長で意図が読み取りにくい
includes(%i[learnings products], learnings: :user, products: %i[user checks])と
配列とハッシュを併用して同じ関連 (learnings,products) を重複指定しています。
動作に支障はありませんが、可読性が下がりレビューコストが増えます。- .includes(%i[learnings products], learnings: :user, products: %i[user checks]) + .includes( + learnings: :user, + products: %i[user checks] + )
80-87: キャッシュキーにpractice.idだけを用いると衝突の危険性
@copied_practices_cache[practice.id]は course が異なる複数の Presenter
インスタンスを同じユーザで生成した場合に衝突する可能性があります。
course もキーに含めれば安全です。- @copied_practices_cache[practice.id] ||= practice.copied_practices + cache_key = "#{@course.id}-#{practice.id}" + @copied_practices_cache[cache_key] ||= practice.copied_practices
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/interactors/copy_check.rb(1 hunks)app/presenters/practice_progress_presenter.rb(1 hunks)test/interactors/copy_check_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/interactors/copy_check.rb
- test/interactors/copy_check_test.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
komagata
force-pushed
the
feature/copy-done-practice
branch
from
0b6aac3 to
a671168
Compare
- PracticeProgressMigratorのリファクタリング用にinteractor gemを導入 - 単一責任の原則に従った設計パターンの実装準備
- CopyLearning: Learning単体のコピー処理 - CopyProduct: Product単体のコピー処理 - CopyCheck: Check単体のコピー処理（N+1クエリ最適化済み） - CopyPracticeProgress: 全体を統括するトランザクション管理 - 各Interactorの包括的なテストを実装 - 単一責任の原則に従った設計
- Interactorパターンを使用したシンプルな設計 - 改善されたエラーハンドリング（RuntimeError回避） - migrate_allでの一括処理機能 - 包括的なテストカバレッジ
- クエリ最適化済みのPracticeProgressPresenter - completed_practice_idsのインスタンス変数キャッシュでクエリ削減 - practice_status_forのN+1クエリ問題修正 - migration_candidatesのサブクエリ最適化 - CompletedLearningsQueryで責務分離
- practice_progress_controller: 個別ユーザーの進捗表示 - practice_progress_batches_controller: 一括マイグレーション機能 - 管理画面用のビューテンプレート - システムテストで動作確認
- Course: copied_practicesの関連付け - Practice: copied_practicesとsource関連付け - User: 進捗管理に必要な関連付けの拡張
- コード生成品質向上のためのガイドライン設定
komagata
force-pushed
the
feature/copy-done-practice
branch
from
e0741a0 to
8989e11
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
app/models/course.rb (2)
4-6: 定数の二重管理を排除しましょう
RAILS_COURSEとDEFAULT_COURSEが同じ値を保持しており冗長です。どちらか一方に統一すれば保守コストを削減できます。
19-21: 存在しない場合は例外で検知したい
find_byは見つからないとnilを返します。想定外のnilによる後続エラーを防ぐためfind_by!へ変更し、早期に不整合を検知することを推奨します。app/controllers/admin/users/practice_progress_batches_controller.rb (2)
6-14: 大量データは非同期ジョブで処理を
migrate_allはユーザーの完了プラクティスを全件コピーします。件数が多い場合はブラウザタイムアウトや DB ロックが懸念されるため、ActiveJob等でバックグラウンドジョブ化し、完了時に通知する構成を検討してください。
18-20: 想定外例外のフォールバックが欲しい
migrate_allで rescue されない例外が発生すると 500 になります。ここでrescue StandardError => eを追加し、エラーをログ出力した上でalert返却する保険を置くと運用時の調査が容易です。app/queries/completed_learnings_query.rb (1)
16-19:courseがnilのケースを明示的に防ぐ
@courseが渡されない場合は全件ヒットしないだけで静かに失敗します。raise ArgumentError, 'course is required'等でガードを追加するとバグが埋もれにくくなります。test/interactors/copy_learning_test.rb (2)
15-37:assert_differenceで件数変化を明確に
コピーで Learning が 1 件増えることをassert_difference 'Learning.count', 1 do … endで検証するとテスト意図が読み取りやすくなります。
81-88: 例外スタブはexpectsを使うと厳密
Learning.stubで例外を投げていますが、Learning.expects(:create!).raises(...)を使うと「メソッドが呼ばれた」ことも同時に検証でき、テストが堅牢になります。test/interactors/copy_practice_progress_test.rb (1)
35-37: メッセージの完全一致アサートは brittle です
CopyPracticeProgress内で最後に実行されたサブインタラクタによりcontext.messageが上書きされるため、実装の微修正だけでテストが壊れる恐れがあります。
成功可否だけに絞るか、assert_matchで部分一致に留めるほうが保守しやすくなります。app/interactors/copy_practice_progress.rb (2)
7-14:Interactor::Failureを rescue していない点に注意
merge_result内でcontext.fail!が呼ばれるとInteractor::Failureが発生しますが、本クラスでは rescue していません。
Interactor 自体が失敗時に例外を握り潰してsuccess? == falseのコンテキストを返す仕様なので問題は出ませんが、救済用に ActiveRecord だけを rescue している現状はややアンバランスです。
rescue StandardError => eでまとめるか、コメントで意図を明示すると読み手の理解が早まります。
33-40:merge_resultが message を上書きしてしまいます3 つのサブインタラクタのうち最後（CopyCheck）の
messageが必ず残るため、途中で発生した「スキップしました」等の情報が欠落します。
複数メッセージを配列で蓄積するか、呼び出し側で使わないと割り切ってmessageをクリアする、などポリシーを決めておくと後々混乱しません。test/interactors/copy_product_test.rb (1)
34-39: DB 変化をassert_differenceで検証すると明確になります生成された Product の存在確認だけでなく、件数増加を
assert_difference('Product.count', 1)でアサートすると意図がより伝わりやすくなります。app/controllers/admin/users/practice_progress_controller.rb (2)
15-18:exists?→findの二重クエリを回避できます
Practice.exists?で存在確認後、migrator.migrate内で再度Practice.findしているため同じレコードを 2 回検索しています。
コントローラ側でpractice = Practice.find_by(id: practice_id)を取得し、そのまま migrator に渡す形に変えると 1 クエリで済みます。
35-37: Strong Parameters のrequireを使うと意図が明確params.require(:practice_id).permit!とすることで必須パラメータであることが宣言的に示せます。現在の
params.permit(:practice_id)では欠落を見逃す可能性があります。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/copilot/guidelines.yml(1 hunks)Gemfile(2 hunks)app/controllers/admin/users/practice_progress_batches_controller.rb(1 hunks)app/controllers/admin/users/practice_progress_controller.rb(1 hunks)app/interactors/copy_check.rb(1 hunks)app/interactors/copy_learning.rb(1 hunks)app/interactors/copy_practice_progress.rb(1 hunks)app/interactors/copy_product.rb(1 hunks)app/models/course.rb(2 hunks)app/models/practice.rb(1 hunks)app/models/practice_progress_migrator.rb(1 hunks)app/models/user.rb(1 hunks)app/presenters/practice_progress_presenter.rb(1 hunks)app/queries/completed_learnings_query.rb(1 hunks)app/views/admin/users/practice_progress/show.html.slim(1 hunks)config/application.rb(1 hunks)config/initializers/inflections.rb(1 hunks)config/routes/admin.rb(1 hunks)test/interactors/copy_check_test.rb(1 hunks)test/interactors/copy_learning_test.rb(1 hunks)test/interactors/copy_practice_progress_test.rb(1 hunks)test/interactors/copy_product_test.rb(1 hunks)test/models/practice_progress_migrator_test.rb(1 hunks)test/system/admin/users/practice_progress_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- config/initializers/inflections.rb
- .github/copilot/guidelines.yml
- config/application.rb
- app/models/user.rb
- Gemfile
🚧 Files skipped from review as they are similar to previous changes (4)
- test/interactors/copy_check_test.rb
- app/models/practice_progress_migrator.rb
- app/interactors/copy_check.rb
- app/presenters/practice_progress_presenter.rb
🧰 Additional context used
🧬 Code Graph Analysis (5)
app/queries/completed_learnings_query.rb (1)
app/presenters/practice_progress_presenter.rb (1)
initialize(6-9)
app/controllers/admin/users/practice_progress_batches_controller.rb (2)
app/controllers/admin/users/practice_progress_controller.rb (3)
before_action(3-38)create(12-27)set_user(31-33)app/models/practice_progress_migrator.rb (1)
migrate_all(23-35)
app/interactors/copy_product.rb (3)
app/interactors/copy_check.rb (2)
include(3-93)call(6-15)app/interactors/copy_learning.rb (3)
include(3-65)call(6-17)validate_inputs(21-25)app/interactors/copy_practice_progress.rb (2)
include(3-41)call(6-14)
app/interactors/copy_practice_progress.rb (3)
app/interactors/copy_check.rb (2)
include(3-93)call(6-15)app/interactors/copy_learning.rb (2)
include(3-65)call(6-17)app/interactors/copy_product.rb (2)
include(3-70)call(6-17)
app/controllers/admin/users/practice_progress_controller.rb (2)
app/controllers/admin/users/practice_progress_batches_controller.rb (3)
before_action(3-21)create(6-14)set_user(18-20)app/models/practice_progress_migrator.rb (1)
migrate(8-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (8)
config/routes/admin.rb (1)
10-11: ルーティング追加は問題なし
RESTful に整理されており既存ヘルパと衝突しません。app/views/admin/users/practice_progress/show.html.slim (3)
70-76: I18n に切り出してくださいconfirm メッセージ
'進捗をコピーしますか？'がハードコードされています。
t('.confirm_copy_progress')など I18n に乗せると多言語対応・テスト容易性が向上します。
[ suggest_optional_refactor ]
98-100: 全件コピーの confirm も同様に I18n 化をこちらも
'全ての進捗をコピーしますか？'が直書きです。併せて locale ファイルへ。
[ suggest_optional_refactor ]
63-67: 表示用固定文字列の散在を整理セル内の
'なし','未着手'が複数箇所で直書きされています。
t('common.none'),t('common.not_started')など共通キーで一元管理すると保守コストが下がります。
[ suggest_nitpick ]Also applies to: 77-83, 84-96
app/interactors/copy_learning.rb (1)
38-43: 同時実行で重複レコードが発生する恐れ
existing_learningを確認した後にLearning.create!していますが、並行リクエストが入ると UNIQUE 制約違反になる可能性があります。
Learning.find_or_create_by!(user:, practice:)もしくは DB 制約を rescue してスキップする処理を追加すると安全です。
[ suggest_essential_refactor ]Also applies to: 54-61
app/interactors/copy_product.rb (1)
46-55: 既存 Product を上書きすべき要件と実装が食い違っています
existing_product_found?でコピーをスキップしていますが、システムテストでは既存 Product の body 更新を期待しています。
要件が「上書き」なら下記のように更新してから return してください。- context.copied_product = context.existing_product - context.message = 'Product already exists, skipping copy' + context.existing_product.update!(body: context.original_product.body, + wip: context.original_product.wip) + context.copied_product = context.existing_product + context.message = 'Product updated successfully'[ raise_critical_issue ]
test/system/admin/users/practice_progress_test.rb (1)
417-485: テストが落ちる見込みがあります既存 Product の body を
'Updated submission'へ更新することを期待していますが、現行CopyProductは更新ロジックを持ちません。
Interactor を修正するか、期待値を見直してください。
[ request_verification ]test/models/practice_progress_migrator_test.rb (1)
122-129: テスト間で期待動作が矛盾していますここでは「既存 Product を保持する」ことをテストしていますが、system テストでは「上書きする」挙動を期待しています。
仕様を明確化し、両テストを一致させてください。
[ request_verification ]
| # Practice copy relationships | ||
| has_many :copied_practices, class_name: 'Practice', foreign_key: 'source_id', dependent: :nullify, inverse_of: :source_practice | ||
| belongs_to :source_practice, class_name: 'Practice', foreign_key: 'source_id', optional: true, inverse_of: :copied_practices | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
自己参照関連のガードを追加してください
source_practice が自身を指し示すループや多段コピーチェーンを防ぐバリデーションが無いので、不正データが混入すると再帰クエリで無限ループを招く恐れがあります。
validate :source_id_cannot_be_self などで自己参照を弾くことを推奨します。併せて source_id にはインデックスを張っておくとコピー検索時のパフォーマンスも向上します。
🤖 Prompt for AI Agents
In app/models/practice.rb around lines 54 to 57, add a validation method named
source_id_cannot_be_self to prevent the source_practice from referencing itself,
which can cause infinite recursion in queries. Implement this validation to
check if source_id equals the record's own id and add an error if so. Also,
ensure that a database index is created on the source_id column to improve
performance when querying copied practices.
There was a problem hiding this comment.
@komagata ありがとうございます！
コードを確認したところ、指摘した両方の点が適切に修正されていることが確認できました：
✅ 自己参照防止バリデーション: validate :source_id_cannot_be_self が追加され、source_id が自身のIDと同じ場合にエラーを追加する実装になっています
✅ パフォーマンス改善: source_id カラムにインデックスが追加されており、コピー検索時のパフォーマンスが向上します
バリデーションロジックも適切にガードクローズが設定されており、nil 値の処理も考慮されています。これで無限ループのリスクが回避され、安全に Practice のコピー関係を扱えるようになりました。
- 既存データがある場合はスキップする仕様に合わせてテストを更新 - test名を「updates」から「skips」に変更 - 期待値を「データが更新される」から「既存データが保持される」に修正
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/system/admin/users/practice_progress_test.rb (6)
15-16: 重複回避のための destroy_all はコスト大 & 可読性低下毎回
Learning.where(...).destroy_allでレコードを全消ししていますが、対象が 1 件であることが分かっているならdelete_byで十分です。また、find_or_create_byで済むシナリオなら削除自体を省略出来ます。
細かいですが大量テスト時の実行時間短縮と意図の明確化に繋がります。
98-106: Time.current.to_i での一意性保証はレースに弱い
timestamp = Time.current.to_iは 1 秒内に複数テストが走ると衝突の可能性があります。SecureRandom.uuidなど衝突しない値を使うか、Factory の sequence を利用するとより堅牢です。-require 'securerandom' -timestamp = Time.current.to_i +require 'securerandom' +timestamp = SecureRandom.uuid
260-304: CSS セレクタの揺れを統一すると読みやすさが向上します
within 'tbody tr'とwithin 'tr'の 2 パターンが混在しています。DOM 構造が変わった際にテストが部分的に落ちる恐れがあるので、共通 helper を作成して行セレクト方式を一本化することを推奨します。
392-401: 遷移先のパスも合わせて検証すると回帰に強くなりますフラッシュメッセージだけでなく、
assert_current_pathでリダイレクト先が意図通り/admin/users/:id/practice_progressに戻っているか確認すると UI 変更による誤動作を早期検知できます。
478-485: 「更新されないこと」を検証するなら件数チェックも追加したい既存レコードが維持されたことを body 文字列で確認していますが、
assert_no_difference 'Product.where(...).count'も加えると「新規に複製されていない」ことまで保証できます。
565-587: 一括コピー後の副次効果をより網羅的に確認できます学習・提出物は確認していますが、
Checkレコードもコピー対象であれば同様に件数・内容を assert するとテストの網羅性が上がります。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/system/admin/users/practice_progress_test.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/system/admin/users/practice_progress_test.rb (1)
app/models/practice.rb (2)
product(137-139)status(93-103)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
- PracticeProgressMigratorのmigrate_allメソッドでトランザクション処理を改善 - PracticeProgressBatchesControllerでInteractor結果のハンドリング修正 - CopyProductでメッセージを簡潔化 - CopyCheckでメッセージ設定ロジックを簡素化 - テストファイルでデータクリーンアップ処理を改善 - CompletedLearningsQueryクラスを追加
- 実装で設定される「Product not found – skipping copy」に合わせて テストの期待値を「No product to copy」から修正 - 実装とテストの整合性を確保
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/models/practice_test.rb (2)
59-65: ハードコードされたエラーメッセージは将来的に壊れやすいです
'cannot reference itself'を直接比較すると、i18n で文言が変更された場合にテストが落ちます。
エラー種別シンボルを用いたerrors.added?での検証に差し替えると堅牢になります。- assert_includes practice.errors[:source_id], 'cannot reference itself' + # バリデーション側で `errors.add(:source_id, :self_reference)` としている前提 + assert practice.errors.added?(:source_id, :self_reference)
67-80: 失敗時のデバッグ容易性を高めるアサーションメッセージの追加を検討してください
assert practice.valid?だけだと失敗時に何が原因か分かりにくいため、practice.errors.full_messagesを渡すと調査が速くなります。- assert practice.valid? + assert practice.valid?, practice.errors.full_messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/models/practice.rb(2 hunks)db/migrate/20250618142946_add_index_on_source_id_to_practices.rb(1 hunks)db/schema.rb(3 hunks)test/models/practice_test.rb(1 hunks)test/system/admin/users/practice_progress_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- db/migrate/20250618142946_add_index_on_source_id_to_practices.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/system/admin/users/practice_progress_test.rb
- app/models/practice.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
db/schema.rb (1)
902-910: 以下のスクリプトでdb/schema.rb内のusersテーブルに対する全てのadd_index定義を再確認してください。#!/bin/bash # usersテーブルに関する全ての add_index 定義を検索 rg -n 'add_index.*"users"' db/schema.rb
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
db/migrate/20250618143741_add_unique_index_to_users_email_and_login_name.rb (1)
3-4: 既存インデックス・重複レコードの存在確認を
login_name列には既にユニークインデックスindex_users_on_login_nameが存在している可能性があります。重複インデックスを作成しようとするとエラーになりますので、if_not_exists: trueで安全にしておくと安心です。重複レコードの有無もご確認ください。- add_index :users, :login_name, unique: true + add_index :users, :login_name, unique: true, if_not_exists: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
db/migrate/20250618143741_add_unique_index_to_users_email_and_login_name.rb(1 hunks)db/schema.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- db/schema.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
db/migrate/20250618143741_add_unique_index_to_users_email_and_login_name.rb (1)
3-4: まず、PostgreSQL を使っているか確認するために以下のスクリプトを実行してください:#!/bin/bash # config/database.yml に PostgreSQL アダプタが設定されているか確認 grep -R "adapter:\s*postgresql" -n config/database.yml || echo "No PostgreSQL adapter found in config/database.yml" # Gemfile に pg gem が含まれているか確認 grep -R "gem ['\"]pg['\"]" -n Gemfile || echo "No pg gem found in Gemfile"
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/queries/completed_learnings_query.rb(1 hunks)db/migrate/20250618144325_add_foreign_key_to_practices_source_id.rb(1 hunks)db/schema.rb(4 hunks)test/models/practice_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/models/practice_test.rb
- app/queries/completed_learnings_query.rb
- db/schema.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
| def change | ||
| add_foreign_key :practices, :practices, column: :source_id | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
外部キーの即時検証によるテーブルロックを回避しましょう
add_foreign_key はデフォルトで制約を同時に検証するため、大きいテーブルでは書き込みをブロックする時間が長くなる恐れがあります。validate: false を付けて制約だけ先に追加し、別マイグレーションで validate_foreign_key :practices, :source_id を行うとロック時間を短縮できます。既存データに不整合がないことも事前に確認してください。
- add_foreign_key :practices, :practices, column: :source_id
+ # 検証は後段で実施してロック時間を最小化
+ add_foreign_key :practices, :practices, column: :source_id, validate: false（必要に応じて on_delete: :nullify 等の振る舞いも検討してください）
外部キーの即時検証によるテーブルロックを回避しましょう
add_foreign_key はデフォルトで制約を同時に検証するため、大きいテーブルでは書き込みをブロックする時間が長くなる恐れがあります。validate: false を付けて制約だけ先に追加し、別マイグレーションで validate_foreign_key :practices, :source_id を行うとロック時間を短縮できます。既存データに不整合がないことも事前に確認してください。
- add_foreign_key :practices, :practices, column: :source_id
+ # 検証は後段で実施してロック時間を最小化
+ add_foreign_key :practices, :practices, column: :source_id, validate: false（必要に応じて on_delete: :nullify 等の振る舞いも検討してください）
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def change | |
| add_foreign_key :practices, :practices, column: :source_id | |
| end | |
| def change | |
| # 検証は後段で実施してロック時間を最小化 | |
| add_foreign_key :practices, :practices, column: :source_id, validate: false | |
| end |
🤖 Prompt for AI Agents
In db/migrate/20250618144325_add_foreign_key_to_practices_source_id.rb around
lines 2 to 4, the add_foreign_key call immediately validates the foreign key
constraint, which can cause long table locks on large tables. To fix this, add
the option validate: false to the add_foreign_key method to defer validation,
then create a separate migration to run validate_foreign_key :practices,
:source_id. Also, verify beforehand that existing data has no inconsistencies.
Optionally, consider adding on_delete behavior like on_delete: :nullify if
appropriate.
概要
管理者がユーザーのRailsエンジニアコースでの学習進捗をReスキル講座認定版にコピーできる機能を追加しました。
主な変更内容
CompletedLearningsQueryクラスでクエリロジックを分離PracticeProgressMigratorクラスで学習状況とプロダクトデータのコピー処理技術的な改善
テスト計画
Summary by CodeRabbit
新機能
テスト