Allow starting over from verification page - #9445
Conversation
MobileInstall the Dev Tool (iOS via TestFlight, iOS Simulator, or Android .apk) here. Scan the QR with your phone camera, or tap Open in Dev Tool on the device, to open this branch in the installed Dev Tool dev client.
Deep linksiOS Android Web (Vercel)View the Vercel web preview for this branch. Backend
Other
|
![couchersbot[bot]](https://avatars.githubusercontent.com/in/983009?s=60&v=4){kind=small}
|
Claude finished @nc225's task in 3m 17s —— View job Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: HIGH RISK — the migration file has unresolved merge conflict markers committed in it, so nothing runs at all.
Files Reviewed
Findings
Confirmed safe against existing data ✅
Outside migration safety
SummaryThe upgrade path is now genuinely safe against existing production data — the |
| ).scalar_one_or_none() | ||
| if not flow: | ||
| context.abort_with_error_code(grpc.StatusCode.NOT_FOUND, "invalid_token") | ||
| if request.cancel_signup: |
There was a problem hiding this comment.
Here I am adding the signup_cancelled flag when a signup is cancelled. Another option would be to use session.delete() but it seems we generally prefer adding info to destroying it - happy to go with session.delete() though since adding the flag this requires adding a few lines elsewhere too. Also, I thought it was a bit confusing to use the same name for the proto and the postgres variables so I called one signup_cancelled and the other cancel_signup but if this, too, is confusing, I will standardize.
| def downgrade() -> None: | ||
| op.drop_index( | ||
| "uq_signup_flows_username", | ||
| table_name="signup_flows", | ||
| postgresql_where=sa.text("username IS NOT NULL AND signup_cancelled = false"), | ||
| ) | ||
| op.create_unique_constraint( | ||
| op.f("uq_signup_flows_username"), "signup_flows", ["username"], postgresql_nulls_not_distinct=False | ||
| ) | ||
| <<<<<<< HEAD | ||
| op.drop_column("signup_flows", "signup_cancelled") | ||
| ======= | ||
| op.execute("DELETE FROM signup_flows WHERE signup_cancelled") | ||
| op.drop_column("signup_flows", "signup_cancelled") | ||
| >>>>>>> fa81c79b8 (update upgrade and downgrade to add signup_cancelled) |
There was a problem hiding this comment.
HIGH — blocker: unresolved merge conflict markers are committed in this file.
Lines 60–65 still contain <<<<<<< HEAD / ======= / >>>>>>> fa81c79b8. The module is not valid Python, so it raises SyntaxError the moment Alembic imports the versions directory — every alembic command fails (including upgrade head on deploy) and the backend won't start.
Also, whichever side you keep, the DELETE needs to run before create_unique_constraint, not after: re-adding the full UNIQUE (username) fails on duplicate usernames, and duplicates are exactly the state this feature creates (a cancelled flow plus a new flow reusing the same username). The current placement means the constraint blows up before the cleanup ever runs.
This resolves both:
| def downgrade() -> None: | |
| op.drop_index( | |
| "uq_signup_flows_username", | |
| table_name="signup_flows", | |
| postgresql_where=sa.text("username IS NOT NULL AND signup_cancelled = false"), | |
| ) | |
| op.create_unique_constraint( | |
| op.f("uq_signup_flows_username"), "signup_flows", ["username"], postgresql_nulls_not_distinct=False | |
| ) | |
| <<<<<<< HEAD | |
| op.drop_column("signup_flows", "signup_cancelled") | |
| ======= | |
| op.execute("DELETE FROM signup_flows WHERE signup_cancelled") | |
| op.drop_column("signup_flows", "signup_cancelled") | |
| >>>>>>> fa81c79b8 (update upgrade and downgrade to add signup_cancelled) | |
| def downgrade() -> None: | |
| op.drop_index( | |
| "uq_signup_flows_username", | |
| table_name="signup_flows", | |
| postgresql_where=sa.text("username IS NOT NULL AND signup_cancelled = false"), | |
| ) | |
| op.execute("DELETE FROM signup_flows WHERE signup_cancelled") | |
| op.create_unique_constraint( | |
| op.f("uq_signup_flows_username"), "signup_flows", ["username"], postgresql_nulls_not_distinct=False | |
| ) | |
| op.drop_column("signup_flows", "signup_cancelled") |
| context.abort_with_error_code(grpc.StatusCode.FAILED_PRECONDITION, "signup_flow_email_taken") | ||
| existing_flow = session.execute( | ||
| select(SignupFlow).where(SignupFlow.email == request.basic.email) | ||
| select(SignupFlow).where(SignupFlow.email == request.basic.email).where(SignupFlow.signup_cancelled == False) |
There was a problem hiding this comment.
MEDIUM — new regression on this push: cancel + re-signup with the same email is now a 500.
Filtering signup_cancelled here closes the resurrection hole (good), but signup_flows.email still carries a full UNIQUE constraint (schema.sql:2672 — only username was made partial). So:
- User signs up with
a@b.com, clicks "start over" → the row stays withsignup_cancelled = true, email = 'a@b.com'. - They re-submit
a@b.com(they mistyped their name, or realised the address was fine, or just retype it out of habit). - This lookup no longer matches the cancelled row, so we fall through to
SignupFlow(... email=request.basic.email ...)at L268 andsession.flush()at L275 →IntegrityErroronuq_signup_flows_email.
There's no IntegrityError handling anywhere in auth.py, so the user gets an unhandled internal error instead of the clean signup_flow_email_started_signup abort they'd have got before this PR. The FE makes this very reachable: restartSignup() clears flowState and signupEmail and drops the user straight back on BasicForm, so nothing stops them re-entering the same address.
Two ways out:
session.delete(flow)in the cancel branch instead of the soft flag. Cancellation becomes genuinely terminal, both the email and the username are freed, all four.where(SignupFlow.signup_cancelled == False)filters become unnecessary, and the migration (new column + partial index swap) isn't needed at all.- Keep the flag, but make
uq_signup_flows_emailpartial too (samesignup_cancelled = falsepredicate), so the insert succeeds.
Either way this deserves a regression test — "cancel, then re-signup with the same email" and "cancel, then a second flow takes the freed username" are exactly the cases that would have caught it. Nothing in src/tests/ touches cancel_signup yet.
Two smaller things on this hunk: this line and L221, L281, L523 are all >120 chars, so ruff format will rewrap them and CI's format check fails until make format is run. And cancel_signup is silently ignored on the email_token branch (L201–224) — it's only honoured in the else at L280; an explicit abort would beat a no-op.
|
(I am deleting a previous comment bc I figured out the problem -- I somehow ended up with duplicate migration files and that made the backend unhappy.) I've got this working in some form but I'd like to add more tests to the BE section before restoring from drafts. One BE question that I marked in the code too: when a signup is cancelled due to bad email, should we
I've gone with the first one because it seems a bit clearer, but it also means checking for The PR is a bit complicated right now, and I may explore whether a different path (say, letting the user change their email at/after the verification screen instead of restarting) may work better. |
| <<<<<<< HEAD | ||
| op.drop_column("signup_flows", "signup_cancelled") | ||
| ======= | ||
| op.execute("DELETE FROM signup_flows WHERE signup_cancelled") |
There was a problem hiding this comment.
Note that this will drop any signup_cancelled rows from the database upon downgrade so that username remains unique
|
@nc225 I think it's fine to continue the work here as a draft, but I would recommend closing it and splitting the BE and FE work before you ask for a new round of reviews so people can take a specialized look. |
|
Since we have to make BE changes anyhow to prevent mistyped emails from blocking usernames in SignupFlow, I am reworking this to use option 3 from this comment instead of option 2 as I had prior. Here are a couple screenshots in development of what the front end might look like on the email verification screen - open to any design tips. (Also forgive the still-misspelled Mistyped) I will also split this into FE and BE PRs. Separate convo: do we want to limit the number of times a Signup can change its email address? 
|
|
hey @nc225 just popping on here as these go in my email - FYI we normally try to break frontend and backend changes into separate PRs for easier review. |
5 participants
Closes #9092
A few notes:
1. No email address shown
The linked issue suggests that we
However, here we do not display the email address, because email is not currently available via auto.proto file. If we make email available from auth_pb.js alongside flowToken, then we can eventually incorporate email address here as well.
2. Prior verification link
If someone clicks the link to start over, their prior verification token/email is still valid so they could end up with two distinct accounts if they somehow have access to both emails. LMK if we want to invalidate the token as well, though I don't see it posing a major issue.
3. Handling incomplete signups
Keeping the current behavior here: if a new signup tries to use the email address of an existing incomplete signup, there is a message to "Please check your email for a link to continue signing up" and an email is sent with a link that picks up to wherever they left off in the signup process.
4. Suggestion for revised SignupFlow
Finally, I do think signup flow could benefit from a "back" button (What if you decide you want to change your Hosting Status before submitting? This is easy to do once your account is active, but still.)
Testing
Tested locally in a few different scenarios, seems to work fine. I wasn't sure which automated tests to add so I haven't made any but happy to talk about those.
Backend checklist
developif necessary for linear migration historyWeb frontend checklist
For maintainers