Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9634421a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content: params[:content] || "", | ||
| user: current_user | ||
| user: current_user, | ||
| plan_type_id: params[:plan_type_id] |
There was a problem hiding this comment.
params[:plan_type_id] is passed directly into Plans::Create, where Plan.create! writes the FK. If a client sends a nonexistent ID, MySQL raises ActiveRecord::InvalidForeignKey, but this action only rescues ActiveRecord::RecordInvalid, so the API returns a 500 instead of a client-error response. This is a new failure mode introduced by accepting plan_type_id and will affect any caller that sends stale/typoed IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58108af — added params[:plan_type_id].presence sanitization and rescue ActiveRecord::InvalidForeignKey returning 422. Also added API spec coverage for invalid plan_type_id.
|
|
||
| create_table "coplan_users", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| | ||
| t.boolean "admin", default: false, null: false | ||
| t.string "avatar_url" |
There was a problem hiding this comment.
This commit adds new coplan_users columns in db/schema.rb (avatar_url, notification_preferences, team, title), but there is no migration in db/migrate or engine/db/migrate that creates them (repo-wide search only finds the new plan-types migration). That makes schema-load and migration-based setups diverge, which can cause environment drift and hard-to-diagnose deploy/test inconsistencies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58108af — reverted the unrelated coplan_users column additions (avatar_url, notification_preferences, team, title) from schema.rb. Only the plan_types migration changes remain.
- Migration: coplan_plan_types table (UUID PK, name, description, default_tags, template_content, metadata) + plan_type_id FK on plans - PlanType model with validations, after_initialize defaults, ransackable - Plan belongs_to :plan_type (optional) - Plans::Create service accepts plan_type_id - API: plan create accepts plan_type_id, responses include plan_type fields - Configuration: seed_plan_types attribute - ActiveAdmin registration - Factory and model specs Amp-Thread-ID: https://ampcode.com/threads/T-019d54e9-adf1-746a-861d-2f2b14d1b882 Co-authored-by: Amp <amp@ampcode.com>
- Sanitize plan_type_id with .presence, rescue InvalidForeignKey → 422 - Add .includes(:plan_type, :created_by_user) to index to fix N+1 - Revert unrelated coplan_users schema.rb changes - Remove non-functional default_tags from AA permit_params - Add Create service specs for plan_type_id (with and without) - Add API specs for valid/invalid plan_type_id on create
- Data migration creates 'General' plan type and sets it on all existing plans with NULL plan_type_id - Seeds add General, RFC, Design Doc, ADR plan types (idempotent) - Seeds backfill any untyped plans to General
Amp-Thread-ID: https://ampcode.com/threads/T-019d54e9-adf1-746a-861d-2f2b14d1b882 Co-authored-by: Amp <amp@ampcode.com>
- Add current_plan_version_id to Plan ransackable_attributes - Add template_content to PlanType ransackable_attributes - Explicitly declare filters on Plans admin to avoid auto-filter issues Amp-Thread-ID: https://ampcode.com/threads/T-019d54e9-adf1-746a-861d-2f2b14d1b882 Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
Adds a new
CoPlan::PlanTypemodel to categorize plans by type (e.g., RFC, Design Doc, ADR).Changes
Model & Migration
coplan_plan_typestable: UUID PK,name(unique, not null),description,default_tags(JSON),template_content,metadata(JSON), timestamps.plan_type_idFK oncoplan_plans(indexed, optional).has_many :plans, dependent: :nullify,after_initializedefaults for JSON columns,ransackable_*methods.belongs_to :plan_type, optional: true,ransackable_attributes/ransackable_associations.API
plan_type_id(sanitized with.presence, invalid FK returns 422).plan_type_idandplan_type_name..includes(:plan_type, :created_by_user)to avoid N+1.Admin & Config
seed_plan_typesattribute (defaults to[]).Seeds
Tests
plan_type_id.Plans::Createwith and withoutplan_type_id.Verified
/admin/plansand/admin/plan_typestested in browser — both render correctly.