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: 4278beec1b
ℹ️ 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".
| @@ -0,0 +1,11 @@ | |||
| class BackfillUsernames < ActiveRecord::Migration[8.1] | |||
| def up | |||
| CoPlan::User.where(username: nil).find_each do |user| | |||
There was a problem hiding this comment.
This backfill references username in host-app migrations, but there is no db/migrate/*add_username_to_coplan_users.co_plan.rb committed alongside it. In this repo, db:migrate is wired to run co_plan:install:migrations first, and existing .co_plan files show copied engine migrations get new timestamps; that means a fresh environment can copy the add-column migration with a timestamp later than 20260406200000, causing this migration to run first and fail with an unknown username column.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b5b8b1c — committed the copied migration file so fresh environments always run it before the backfill.
engine/app/models/coplan/user.rb
Outdated
| base = external_id.to_s.split("@").first.downcase.gsub(/[^a-z0-9._-]/, "") | ||
| base = base.presence || external_id.to_s.downcase.gsub(/[^a-z0-9._-]/, "") |
There was a problem hiding this comment.
The derivation keeps leading ./_/- from external_id (split("@").first + gsub), but the validation requires the first character to be [a-z0-9]. For valid existing IDs like _alice@example.com or -svc@idp, before_validation assigns an invalid username and save! fails, which can break sign-in and also abort the backfill on those rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — added .sub(/\\A[^a-z0-9]+/, "") to strip leading punctuation in both the host callback and the backfill migration. Fixed in b5b8b1c.
- Add username column (unique, indexed) to coplan_users - Engine validates format and uniqueness but does NOT auto-derive - Host app derives username from external_id in its authenticate callback - Engine accepts username (and avatar_url, title, team) from auth attrs - Backfill migration derives usernames for existing users - Add model specs for persistence, format validation, uniqueness Amp-Thread-ID: https://ampcode.com/threads/T-019d6373-a857-75b8-b976-dc9a989c53e0 Co-authored-by: Amp <amp@ampcode.com>
HamptonMakes
force-pushed
the
add-username-to-users
branch
from
4278bee to
5142a0b
Compare
- Commit the copied engine migration so fresh environments run it before the backfill - Strip leading dots/hyphens/underscores from derived usernames so they pass the [a-z0-9] first-character validation Amp-Thread-ID: https://ampcode.com/threads/T-019d6373-a857-75b8-b976-dc9a989c53e0 Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
Adds a
usernamefield to users — a clean, human-readable handle likehamptonfor use in @-mentions (coming next).How it works
external_id— strips the@domain.compart and lowercases (e.g.,hampton@squareup.com→hampton)aliceis taken, the next user getsalice1usernamedirectly and it won't be overwrittenFields
nameusernameexternal_id