提出物のテンプレートを作成・編集できるようにする#10045
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPractice に紐づく ProductTemplate モデルとマイグレーションを追加し、Practice 側に has_one と accepts_nested_attributes_for を設定。コントローラでフォーム用の初期化と許可パラメータを追加し、フォーム／商品画面／システムテストを更新しています。 Changesプロダクトテンプレート機能実装
sequenceDiagram
participant Mentor as Mentor
participant Controller as Mentor::PracticesController
participant Practice as Practice
participant Template as ProductTemplate
participant DB as Database
Mentor->>Controller: プラクティス作成/編集ページへアクセス
Controller->>Practice: build_product_template (new) / fetch product_template (edit)
Controller->>Mentor: フォーム（fields_for: product_template.description）を表示
Mentor->>Controller: フォーム送信 (product_template_attributes 含む)
Controller->>Practice: accepts_nested_attributes_for 経由で product_template を作成/更新/削除
Practice->>DB: product_templates テーブルへ INSERT/UPDATE/DELETE
Controller->>Mentor: リダイレクト/レスポンス
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issuesSuggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🚀 Review AppURL: https://bootcamp-pr-10045-fvlfu45apq-an.a.run.app
|
koguchi-e
force-pushed
the
feature/product-template
branch
3 times, most recently
from
fb2a06d to
ee3e9a5
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 (4)
app/models/product_template.rb (1)
7-7: ⚡ Quick win
validates :practiceは冗長です。Rails 5以降では
belongs_to関連が自動的に存在を検証するため、line 7 のvalidates :practice, presence: trueは不要です。♻️ 提案する修正
class ProductTemplate < ApplicationRecord belongs_to :practice validates :description, presence: true - validates :practice, presence: true end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/product_template.rb` at line 7, Remove the redundant explicit presence validation for the practice association in ProductTemplate: delete the line validating :practice (the validates :practice, presence: true) since the belongs_to :practice association already enforces presence in Rails 5+; leave the belongs_to :practice declaration intact and run model specs to confirm behavior.app/models/practice.rb (1)
30-30: 💤 Low value
reject_ifブロック内のキーはシンボルを使用することを検討してください。
attributes['description']は文字列キーを使用していますが、Railsの慣習ではattributes[:description]のようにシンボルキーを使う方が一般的です。ただし、ネストされた属性では文字列とシンボル両方が渡される可能性があるため、安全のためにattributes['description'].blank? && attributes[:description].blank?またはattributes.dig('description').blank? && attributes.dig(:description).blank?とするか、現状のまま文字列キーのみで十分な場合もあります。💡 参考: より安全な実装例
accepts_nested_attributes_for :product_template, update_only: true, allow_destroy: true, reject_if: proc { |attributes| attributes['description'].blank? && attributes[:description].blank? }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/practice.rb` at line 30, reject_if block for accepts_nested_attributes_for :product_template uses attributes['description'] (string key) which may miss symbol keys; update the proc used in the accepts_nested_attributes_for call (the reject_if proc for :product_template) to check both string and symbol keys (e.g., ensure you test attributes['description'].blank? && attributes[:description].blank? or use attributes.dig('description').blank? && attributes.dig(:description).blank?) so the blank check is robust for either key form.db/migrate/20260520061059_create_product_templates.rb (1)
5-5: ⚡ Quick win
descriptionカラムにnull: false制約を追加することを検討してください。モデルで
validates :description, presence: trueを定義しているため、データベースレベルでもnull: false制約を追加することでデータ整合性をより強固にできます。🔧 提案する修正
create_table :product_templates do |t| t.references :practice, null: false, foreign_key: true - t.text :description + t.text :description, null: false t.timestamps end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/migrate/20260520061059_create_product_templates.rb` at line 5, マイグレーション CreateProductTemplates の description カラムに null: false 制約を追加してください — 現在の行 t.text :description を null: false に変更し、既存レコードに NULL が残っている可能性がある場合はマイグレーション内で事前にデフォルト値を入れるかバックフィル（例: update_all）してから制約を追加する処理を入れてください（マイグレーション名/クラス: CreateProductTemplates、カラム名: description を参照）。test/system/practices/mentor_test.rb (1)
30-42: ⚡ Quick winテンプレート未設定時の検証を追加することを検討してください
このテストはテンプレートなしでプラクティスを作成できることを確認していますが、さらに作成後に
new_product_path(practice_id: ...)へ遷移してproduct[body]フィールドが空であることを検証すると、テンプレート未設定時の動作がより明確になります。検証の追加例
assert_text 'プラクティスを作成しました' + + practice = Practice.find_by(title: 'テンプレなし') + visit new_product_path(practice_id: practice.id) + assert_field 'product[body]', with: '' end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/system/practices/mentor_test.rb` around lines 30 - 42, Extend the test 'can create practice without product template' to after asserting 'プラクティスを作成しました' follow the redirect to the product creation page for the new practice (the equivalent of new_product_path(practice_id: ...)), locate the product form (e.g. form[name=product]) and assert that the 'product[body]' field is blank so the behavior with no product template is explicitly verified; use the created practice's id from the page or by querying Practice.last to build the product path and then assert_field 'product[body]', with: '' (or assert_equal '' on its value).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/mentor/practices_controller.rb`:
- Around line 67-68:
ストロングパラメータでネストされたテンプレート削除が無効になっているので、practice_params（または該当の強いパラメータメソッド）内の
product_template_attributes に :_destroy を追加してください（現在
product_template_attributes: %i[id description] を product_template_attributes:
%i[id description _destroy] のように変更）。モデル側で allow_destroy: true を使っている
product_template のネストされた属性が正しく削除されるようにすること。
---
Nitpick comments:
In `@app/models/practice.rb`:
- Line 30: reject_if block for accepts_nested_attributes_for :product_template
uses attributes['description'] (string key) which may miss symbol keys; update
the proc used in the accepts_nested_attributes_for call (the reject_if proc for
:product_template) to check both string and symbol keys (e.g., ensure you test
attributes['description'].blank? && attributes[:description].blank? or use
attributes.dig('description').blank? && attributes.dig(:description).blank?) so
the blank check is robust for either key form.
In `@app/models/product_template.rb`:
- Line 7: Remove the redundant explicit presence validation for the practice
association in ProductTemplate: delete the line validating :practice (the
validates :practice, presence: true) since the belongs_to :practice association
already enforces presence in Rails 5+; leave the belongs_to :practice
declaration intact and run model specs to confirm behavior.
In `@db/migrate/20260520061059_create_product_templates.rb`:
- Line 5: マイグレーション CreateProductTemplates の description カラムに null: false
制約を追加してください — 現在の行 t.text :description を null: false に変更し、既存レコードに NULL
が残っている可能性がある場合はマイグレーション内で事前にデフォルト値を入れるかバックフィル（例:
update_all）してから制約を追加する処理を入れてください（マイグレーション名/クラス: CreateProductTemplates、カラム名:
description を参照）。
In `@test/system/practices/mentor_test.rb`:
- Around line 30-42: Extend the test 'can create practice without product
template' to after asserting 'プラクティスを作成しました' follow the redirect to the product
creation page for the new practice (the equivalent of
new_product_path(practice_id: ...)), locate the product form (e.g.
form[name=product]) and assert that the 'product[body]' field is blank so the
behavior with no product template is explicitly verified; use the created
practice's id from the page or by querying Practice.last to build the product
path and then assert_field 'product[body]', with: '' (or assert_equal '' on its
value).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a1c943f-f00c-4fd6-b011-61c37ecf6593
📒 Files selected for processing (9)
app/controllers/mentor/practices_controller.rbapp/models/practice.rbapp/models/product_template.rbapp/views/mentor/practices/_form.html.slimapp/views/products/_form.html.slimdb/migrate/20260520061059_create_product_templates.rbdb/schema.rbtest/fixtures/product_templates.ymltest/system/practices/mentor_test.rb
koguchi-e
force-pushed
the
feature/product-template
branch
from
57dabdf to
c59279d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/system/practices/mentor_test.rb (1)
43-43: ⚡ Quick win
Practice.last依存を避けてテストを安定化してください。
Practice.last.idは他データの影響を受けやすいので、作成時に使った属性（例: title）で対象を特定する方が安全です。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/system/practices/mentor_test.rb` at line 43, Replace the fragile Practice.last.id usage by locating the specific Practice you created in the test (or capturing the created record) and using its id; e.g. use the Practice instance returned by your factory (the variable assigned from create(:practice, title: "...")) or do Practice.find_by(title: "the test title").id instead of Practice.last.id so the test targets the exact record reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/practice.rb`:
- Around line 29-30: Add a DB-level uniqueness constraint for the has_one
association by creating a migration that adds a UNIQUE INDEX on
product_templates.practice_id (and handle nullable behavior if applicable), and
if existing duplicates may exist write a data migration/cleanup step to remove
or consolidate duplicate product_template rows before adding the index; update
the Practice model (has_one :product_template) only if any validations/foreign
key options need adjustment but ensure product_templates.practice_id is
constrained uniquely at the database level to enforce the has_one invariant.
In `@app/views/products/_form.html.slim`:
- Line 12: The assignment to template uses `@product.practice` without guarding
against nil, so change the expression that sets template (the line with template
= `@product.practice.product_template`&.description) to safely navigate practice
as well (use safe navigation on `@product.practice` before calling
product_template) so it won't raise when `@product.practice` is nil; keep the
existing safe navigation on product_template and description.
---
Nitpick comments:
In `@test/system/practices/mentor_test.rb`:
- Line 43: Replace the fragile Practice.last.id usage by locating the specific
Practice you created in the test (or capturing the created record) and using its
id; e.g. use the Practice instance returned by your factory (the variable
assigned from create(:practice, title: "...")) or do Practice.find_by(title:
"the test title").id instead of Practice.last.id so the test targets the exact
record reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6c263c7-d89c-418e-ace1-6590d16bfdc9
📒 Files selected for processing (9)
app/controllers/mentor/practices_controller.rbapp/models/practice.rbapp/models/product_template.rbapp/views/mentor/practices/_form.html.slimapp/views/products/_form.html.slimdb/migrate/20260520061059_create_product_templates.rbdb/schema.rbtest/fixtures/product_templates.ymltest/system/practices/mentor_test.rb
✅ Files skipped from review due to trivial changes (3)
- db/migrate/20260520061059_create_product_templates.rb
- test/fixtures/product_templates.yml
- db/schema.rb
koguchi-e
force-pushed
the
feature/product-template
branch
2 times, most recently
from
9449be6 to
b8e233a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db/migrate/20260520061059_create_product_templates.rb`:
- Line 10: The migration currently creates an automatic index for the
practice_id via t.references :practice and then adds another add_index
:product_templates, :practice_id, unique: true which can conflict; fix by
removing the separate add_index line and instead declare the unique index on the
reference itself (change t.references :practice to t.references :practice,
index: { unique: true }) so only one uniquely-named index
(index_product_templates_on_practice_id) is created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9ffc97b-12d5-43ea-b062-8677bd5487a5
📒 Files selected for processing (4)
app/controllers/mentor/practices_controller.rbapp/models/product_template.rbdb/migrate/20260520061059_create_product_templates.rbtest/system/practices/mentor_test.rb
💤 Files with no reviewable changes (1)
- app/models/product_template.rb
koguchi-e
force-pushed
the
feature/product-template
branch
from
b8e233a to
3543386
Compare
|
@djkazunoko Fjord Choice にPRがございましたが、該当のPRがcloseになっておりましたため、レビュー依頼を送らせていただきました。 すでに他レビューを依頼されているなど、 |
|
@koguchi-e |
komagata
force-pushed
the
main
branch
2 times, most recently
from
b420541 to
f31c31e
Compare
|
@koguchi-e |
koguchi-e
force-pushed
the
feature/product-template
branch
from
3543386 to
7fa8cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/practice.rb`:
- Line 30: The current reject_if proc on accepts_nested_attributes_for
:product_template blocks changes like clearing or destroying existing templates;
change the predicate so it only rejects truly empty new records — i.e., only
reject when the record has no id (new record) AND description is blank AND
_destroy is not set — so updates or _destroy operations on existing
product_template are not ignored; update the reject_if proc on
accepts_nested_attributes_for :product_template accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf0a77ca-ae41-4c50-af5b-65d73369613c
📒 Files selected for processing (9)
app/controllers/mentor/practices_controller.rbapp/models/practice.rbapp/models/product_template.rbapp/views/mentor/practices/_form.html.slimapp/views/products/_form.html.slimdb/migrate/20260520061059_create_product_templates.rbdb/schema.rbtest/fixtures/product_templates.ymltest/system/practices/mentor_test.rb
✅ Files skipped from review due to trivial changes (1)
- app/models/product_template.rb
🚧 Files skipped from review as they are similar to previous changes (7)
- test/fixtures/product_templates.yml
- app/controllers/mentor/practices_controller.rb
- app/views/mentor/practices/_form.html.slim
- db/schema.rb
- db/migrate/20260520061059_create_product_templates.rb
- test/system/practices/mentor_test.rb
- app/views/products/_form.html.slim
|
@djkazunoko |
|
@koguchi-e |
koguchi-e
force-pushed
the
feature/product-template
branch
from
7fa8cb7 to
dfb9e7b
Compare
|
@djkazunoko |
koguchi-e
force-pushed
the
feature/product-template
branch
from
dfb9e7b to
1a71ae5
Compare
djkazunoko
left a comment
djkazunoko
left a comment
There was a problem hiding this comment.
@koguchi-e
基本的な機能は問題なく動作していることを確認できました！
いくつかコメントしましたので、ご確認お願いします！
| t.text "description" | ||
| t.bigint "practice_id", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.index ["practice_id"], name: "index_product_templates_on_practice_id" |
There was a problem hiding this comment.
unique: trueが消えてしまっているように見えます。
マイグレーションファイルにはindex: { unique: true }の記述があるので、本来ならt.index ["practice_id"], name: "index_product_templates_on_practice_id", unique: trueのようになると思います。
|
|
||
| create_table "product_templates", force: :cascade do |t| | ||
| t.datetime "created_at", null: false | ||
| t.text "description" |
There was a problem hiding this comment.
app/models/product_template.rbにvalidates :description, presence: trueがあるので、DB側でもt.text :description, null: falseにしておくと、より安全かと思います。
| has_many :products, dependent: :destroy | ||
|
|
||
| has_one :product_template, dependent: :destroy | ||
| accepts_nested_attributes_for :product_template, update_only: true, allow_destroy: true, reject_if: proc { |attributes| attributes['description'].blank? } |
There was a problem hiding this comment.
There was a problem hiding this comment.
@djkazunoko
こちらご指摘ありがとうございます。
見逃しておりました、大変助かります。
@komagata @machida
こちらの件について、テンプレートを削除できるようにする必要はありますでしょうか？
削除できるようにする場合は、フォームに削除用のチェックボックスを追加し、チェックした状態で更新するとテンプレートが削除される形で実装しようと考えています。
削除が不要の場合は、allow_destroy: true を削除する予定です。
There was a problem hiding this comment.
@komagata
ご回答ありがとうございました。こちら承知しました。
@djkazunoko
こちら47bd2a7にて削除機能を追加いたしました。
以下のように、テンプレートがある場合、削除用のチェックボックスが表示され
チェックした状態で更新すると、テンプレートが削除されるようにしています。
また、削除機能のシステムテストを追加しました。
加えて、提出物テンプレート機能に関するモデルテストが不足していたため、あわせて追加しております。
koguchi-e
force-pushed
the
feature/product-template
branch
5 times, most recently
from
d11c672 to
4cdd755
Compare
koguchi-e
force-pushed
the
feature/product-template
branch
from
4cdd755 to
47bd2a7
Compare
|
@koguchi-e |
|
@djkazunoko 確かに、今回の要件だけであれば、 一方で、今回は以下の2点を重視し、提出物テンプレートを別モデル・別テーブルとして実装しました。 1. 既存の日報テンプレート機能との一貫性既存の日報テンプレート機能が、テンプレートを独立したモデルとして扱う構成になっていたため、今回の提出物テンプレート機能についても同様の構成にしました。 実装方針をそろえることで、コードを読む際の理解や今後の保守をしやすくしたいと考えました。 2. 責務の分離提出物テンプレートは、Practiceそのものの属性ではなく、Practiceに付随する独立した情報として扱いたいと考えました。 別モデルにすることで、今後テンプレート固有の属性やバリデーションが追加された場合にも、それらをPracticeモデルへ持たせず、テンプレート側の責務として管理できます。 また、Practiceモデルには現時点で 提出物テンプレートのカラム追加ですぐにモデルが大きくなってしまうわけはありませんが、今後提出物テンプレートに新たな処理が追加された際もそれらの責務もPracticeモデルに持たせることは避けたいと考え、別モデルとして分離する設計にしました。 ご指定の通り、実装は複雑になってしまっているのですが、 |
|
@koguchi-e 既存の日報テンプレート機能との一貫性についてですが、日報テンプレートはユーザーに紐づく情報である一方、今回の提出物テンプレートはプラクティスに紐づく情報なので、事情は少し異なるのかなと感じました。 提出物テンプレートは「このプラクティスではこのテンプレートを使う」というプラクティス固有の情報だと思うので、
また「今後テンプレート固有の属性やバリデーションが増える可能性」については、その時点で切り出す判断でもよいのではないかと思っています。（YAGNIの考え方に近いイメージです。） 既存実装との一貫性や将来の拡張性を重視する考え方も理解できますので、このあたりは設計方針に関わる部分だと思います。 |
|
@koguchi-e @djkazunoko @machida わかりやすくするために箇条書きで書かせていただきました〜 komagataの意見
気にしている観点
|
4 participants
Issue
概要
受講生が提出物の形式を間違えてしまうのを防ぐよう、提出物のテンプレートを作成しました。
メンターのみ、プラクティスごとに提出物のテンプレートを作成・編集ができます。
テンプレートを設定すると、提出物作成ページで設定した文字がtextareaに入力されます。
変更確認方法
テンプレートを新規に作成
feature/product-templateをローカルに取り込むmentormentaro等メンターでログインテンプレートの未設定時の動作確認
Screenshot
変更前
変更後
Flakewatch Report
Summary by CodeRabbit
Review App