Replace CoPlan::UserModel concern with engine-owned CoPlan::User model#32
Replace CoPlan::UserModel concern with engine-owned CoPlan::User model#32HamptonMakes merged 3 commits intomainfrom
Conversation
The engine now owns its own User model (coplan_users table) instead of requiring host apps to provide one and include CoPlan::UserModel. Engine changes: - Add CoPlan::User model with external_id, name, admin, metadata - Add config.authenticate callback (receives request, returns user attrs) - ApplicationController handles auth internally via find-or-create flow - All belongs_to associations reference CoPlan::User directly - Remove CoPlan::UserModel concern, config.user_class, CoPlan.user_class_name - Replace can_admin_coplan? with CoPlan::User#admin - CommentsHelper uses CoPlan::User and :api_tokens association - TokensController uses current_user.api_tokens (no coplan_ prefix) - Layout conditionally renders sign-out link Host app changes: - Remove include CoPlan::UserModel from User model - Replace config.user_class with config.authenticate callback - SlackNotificationJob bridges to host User via external_id for email - Migration creates coplan_users, migrates data, updates foreign keys Specs: - Add :coplan_user factory for CoPlan::User - Update sign_in_as to bridge CoPlan::User -> host User for session auth - All CoPlan-context specs use :coplan_user instead of :user Docs: - Add docs/HOST_APP_GUIDE.md with full integration guide Amp-Thread-ID: https://ampcode.com/threads/T-019c9ba6-3a56-7499-90bf-78e84f8c18cd Co-authored-by: Amp <amp@ampcode.com>
![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: 4028b12aae
ℹ️ 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".
- Skip host's authenticate_user! in CoPlan::ApplicationController so the overridden current_user doesn't break the parent's auth check - Use explicit column: args for all remove_foreign_key calls to avoid ambiguity when a table has multiple FKs to the same target - Update api_tokens_spec to expect 401 (CoPlan returns unauthorized directly instead of redirecting to host sign_in)
If two concurrent requests try to create the same CoPlan::User (by external_id), the unique index causes a RecordNotUnique on the loser. Rescue and re-find the existing record, then sync attributes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 740b790e71
ℹ️ 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".
| remove_foreign_key :coplan_plan_collaborators, column: :added_by_user_id | ||
| remove_foreign_key :coplan_plans, column: :created_by_user_id | ||
|
|
||
| add_foreign_key :coplan_api_tokens, :users, column: :user_id |
There was a problem hiding this comment.
The down path re-adds FKs to users immediately, but after up runs, newly provisioned coplan_users no longer share IDs with host users (only external_id matches). If any post-migration data references those new coplan_users.id values (e.g., coplan_plans.created_by_user_id), add_foreign_key ... :users fails with FK violations and blocks rollback during an incident. The rollback needs a data remap from CoPlan IDs back to host user IDs (via external_id) or should be marked irreversible.
Useful? React with 👍 / 👎.
1 participant
Summary
Refactors the CoPlan engine to own its own
CoPlan::Usermodel instead of requiring host apps to provide a User model and includeCoPlan::UserModel.Motivation
The old approach (
CoPlan::UserModelconcern included in the host's User model) coupled the engine to the host's schema, forced string PKs on the host, and required implementing 3 controller methods + aNotAuthorizedError. This replaces all of that with a singleconfig.authenticatecallback.What changed
Engine
CoPlan::Usermodel —coplan_userstable withexternal_id,name,admin,metadataconfig.authenticatecallback — receives request, returns user attributes hash (or nil)ApplicationControllercalls the callback and does find-or-create on every requestbelongs_toassociations reference"CoPlan::User"directly (no moreCoPlan.user_class_name)CoPlan::UserModel,config.user_class,CoPlan.user_class,CoPlan.user_class_name,can_admin_coplan?ApplicationPolicy#admin?now readsuser.admin?directlyHost app
include CoPlan::UserModelandcan_admin_coplan?from User modelconfig.user_class = "User"with session-basedconfig.authenticatecallbackSlackNotificationJobbridges to hostUserviaexternal_idfor email lookupcoplan_users, copies existing users (reusing IDs so FK columns stay valid), and updates foreign keysSpecs
:coplan_userfactorysign_in_asbridgesCoPlan::User→ hostUserfor session auth:coplan_userDocs
docs/HOST_APP_GUIDE.mdwith full integration guideBreaking change
This is a breaking change for any host app using
CoPlan::UserModel. Since we're pre-1.0 and haven't deployed to other hosts, this is acceptable.