-
-
Notifications
You must be signed in to change notification settings - Fork 74
Exclude invited users from User Joined #1602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
341654f
64f0061
b6bbbbd
6242daf
33195df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,16 @@ function isUserAlreadyExistsAuthError(err: unknown): boolean { | |
| ) | ||
| } | ||
|
|
||
| function isMissingCreatedViaInviteColumnError(err: unknown): boolean { | ||
| const anyErr = err as any | ||
| const code = String(anyErr?.code ?? '').toUpperCase() | ||
| const msg = String(anyErr?.message ?? '').toLowerCase() | ||
|
|
||
| // PostgREST returns schema cache errors as PGRST204. | ||
| // Some environments may surface a Postgres undefined_column code (42703). | ||
| return code === 'PGRST204' || code === '42703' || msg.includes('created_via_invite') | ||
| } | ||
|
|
||
| async function rollbackCreatedUser(c: Parameters<typeof useSupabaseAdmin>[0], userId: string) { | ||
| // Best-effort rollback so users can retry the invite flow if something fails mid-way. | ||
| const admin = useSupabaseAdmin(c) | ||
|
|
@@ -106,6 +116,7 @@ async function rollbackCreatedUser(c: Parameters<typeof useSupabaseAdmin>[0], us | |
| } | ||
|
|
||
| async function ensurePublicUserRowExists( | ||
| c: Parameters<typeof useSupabaseAdmin>[0], | ||
| supabaseAdmin: ReturnType<typeof useSupabaseAdmin>, | ||
| userId: string, | ||
| invitation: any, | ||
|
|
@@ -123,14 +134,36 @@ async function ensurePublicUserRowExists( | |
| if (existingRows && existingRows.length > 0) | ||
| return | ||
|
|
||
| const { error: insertError } = await supabaseAdmin.from('users').insert({ | ||
| const insertPayload = { | ||
| id: userId, | ||
| email: invitation.email, | ||
| first_name: invitation.first_name, | ||
| last_name: invitation.last_name, | ||
| enable_notifications: true, | ||
| opt_for_newsletters: optForNewsletters, | ||
| }) | ||
| created_via_invite: true, | ||
| } | ||
|
|
||
| let { error: insertError } = await supabaseAdmin.from('users').insert(insertPayload) | ||
|
|
||
| // Log any initial error for observability during rollout | ||
| if (insertError) { | ||
| cloudlog({ | ||
| requestId: c.get('requestId'), | ||
| message: 'ensurePublicUserRowExists: initial insert error', | ||
| error: insertError, | ||
| }) | ||
| } | ||
|
|
||
| // Backward compatible rollout: if the column doesn't exist yet, retry without it. | ||
| if (isMissingCreatedViaInviteColumnError(insertError)) { | ||
| cloudlog({ | ||
| requestId: c.get('requestId'), | ||
| message: 'ensurePublicUserRowExists: created_via_invite column missing, retrying without it', | ||
| }) | ||
| const { created_via_invite: _createdViaInvite, ...fallbackPayload } = insertPayload | ||
| ;({ error: insertError } = await supabaseAdmin.from('users').insert(fallbackPayload)) | ||
| } | ||
|
|
||
| if (insertError) { | ||
| return quickError(500, 'failed_to_accept_invitation', 'Failed to create user row', { error: insertError.message }) | ||
|
|
@@ -380,7 +413,7 @@ app.post('/', async (c) => { | |
| }) | ||
|
|
||
| if (!sessionError && session.user?.id) { | ||
| await ensurePublicUserRowExists(supabaseAdmin, session.user.id, invitation, body.opt_for_newsletters) | ||
| await ensurePublicUserRowExists(c, supabaseAdmin, session.user.id, invitation, body.opt_for_newsletters) | ||
| await ensureOrgMembership(supabaseAdmin, session.user.id, invitation, org) | ||
|
|
||
| const { error: tmpUserDeleteError } = await supabaseAdmin.from('tmp_users').delete().eq('invite_magic_string', body.magic_invite_string) | ||
|
|
@@ -404,14 +437,39 @@ app.post('/', async (c) => { | |
| let didRollback = false | ||
| try { | ||
| // TODO: improve error handling | ||
| const { error: userNormalTableError, data } = await supabaseAdmin.from('users').insert({ | ||
| const insertUserPayload = { | ||
| id: user.user.id, | ||
| email: invitation.email, | ||
| first_name: invitation.first_name, | ||
| last_name: invitation.last_name, | ||
| enable_notifications: true, | ||
| opt_for_newsletters: body.opt_for_newsletters, | ||
| }).select().single() | ||
| created_via_invite: true, | ||
| } | ||
|
|
||
| let { | ||
| error: userNormalTableError, | ||
| data, | ||
| } = await supabaseAdmin.from('users').insert(insertUserPayload).select().single() | ||
|
|
||
| // Log any initial error for observability during rollout | ||
| if (userNormalTableError) { | ||
| cloudlog({ | ||
| requestId: c.get('requestId'), | ||
| message: 'accept_invitation: initial user insert error', | ||
| error: userNormalTableError, | ||
| }) | ||
| } | ||
|
|
||
| // Backward compatible rollout: if the column doesn't exist yet, retry without it. | ||
| if (isMissingCreatedViaInviteColumnError(userNormalTableError)) { | ||
| cloudlog({ | ||
| requestId: c.get('requestId'), | ||
| message: 'accept_invitation: created_via_invite column missing, retrying without it', | ||
| }) | ||
| const { created_via_invite: _createdViaInvite, ...fallbackPayload } = insertUserPayload | ||
| ;({ error: userNormalTableError, data } = await supabaseAdmin.from('users').insert(fallbackPayload).select().single()) | ||
| } | ||
|
Comment on lines
453
to
472
|
||
|
|
||
| if (userNormalTableError) { | ||
| didRollback = true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| -- Track whether a user account was created via the invitation flow. | ||
| -- This is used for internal onboarding metrics ("User Joined") so we can exclude invited members. | ||
| ALTER TABLE "public"."users" | ||
| ADD COLUMN IF NOT EXISTS "created_via_invite" boolean NOT NULL DEFAULT false; | ||
|
|
||
| COMMENT ON COLUMN "public"."users"."created_via_invite" IS | ||
| 'True when the account was created through /private/accept_invitation (invited members), false for normal self-signups.'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { randomUUID } from 'node:crypto' | ||
| import { describe, expect, it } from 'vitest' | ||
| import { USER_PASSWORD_HASH, executeSQL } from './test-utils.ts' | ||
|
|
||
| describe('users.created_via_invite', () => { | ||
| it.concurrent('defaults to false for normal inserts (self-signup semantics)', async () => { | ||
| const userId = randomUUID() | ||
| const email = `user-created-via-invite-default-${randomUUID()}@test.com` | ||
|
|
||
| await executeSQL( | ||
| `INSERT INTO auth.users (id, email, encrypted_password, email_confirmed_at, created_at, updated_at, raw_user_meta_data) | ||
| VALUES ($1, $2, $3, NOW(), NOW(), NOW(), '{}'::jsonb) | ||
| ON CONFLICT (id) DO NOTHING`, | ||
| [userId, email, USER_PASSWORD_HASH], | ||
| ) | ||
|
|
||
| await executeSQL( | ||
| `INSERT INTO public.users (id, email) | ||
| VALUES ($1, $2) | ||
| ON CONFLICT (id) DO UPDATE SET email = EXCLUDED.email`, | ||
| [userId, email], | ||
| ) | ||
|
|
||
| const rows = await executeSQL( | ||
| 'SELECT created_via_invite FROM public.users WHERE id = $1', | ||
| [userId], | ||
| ) | ||
| expect(rows[0]?.created_via_invite).toBe(false) | ||
| }) | ||
|
|
||
| it.concurrent('can be explicitly set true for invite-created accounts', async () => { | ||
| const userId = randomUUID() | ||
| const email = `user-created-via-invite-true-${randomUUID()}@test.com` | ||
|
|
||
| await executeSQL( | ||
| `INSERT INTO auth.users (id, email, encrypted_password, email_confirmed_at, created_at, updated_at, raw_user_meta_data) | ||
| VALUES ($1, $2, $3, NOW(), NOW(), NOW(), '{}'::jsonb) | ||
| ON CONFLICT (id) DO NOTHING`, | ||
| [userId, email, USER_PASSWORD_HASH], | ||
| ) | ||
|
|
||
| await executeSQL( | ||
| `INSERT INTO public.users (id, email, created_via_invite) | ||
| VALUES ($1, $2, $3) | ||
| ON CONFLICT (id) DO UPDATE SET | ||
| email = EXCLUDED.email, | ||
| created_via_invite = EXCLUDED.created_via_invite`, | ||
| [userId, email, true], | ||
| ) | ||
|
|
||
| const rows = await executeSQL( | ||
| 'SELECT created_via_invite FROM public.users WHERE id = $1', | ||
| [userId], | ||
| ) | ||
| expect(rows[0]?.created_via_invite).toBe(true) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backward-compat fallback (retry insert without
created_via_invite) is silent. When this triggers, it changes metrics semantics and makes it hard to diagnose schema/cache mismatches; consider logging the originalinsertError(at least once / at debug level) before retrying.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback