Merged
Conversation
4 tasks
maebeale
force-pushed
the
maebeale/fix-auth-flow-bugs
branch
from
6ac8763 to
a13b2be
Compare
jmilljr24
reviewed
Mar 27, 2026
Collaborator
jmilljr24
left a comment
jmilljr24
left a comment
There was a problem hiding this comment.
One comment regarding the turbo stuff. There's probably other places where turbo can be re-enabled with my suggestion but doesn't have to be part of this PR.
| method: :put, | ||
| html: { class: "space-y-4" }) do |f| %> | ||
| html: { class: "space-y-4" }, | ||
| data: { turbo: false }) do |f| %> |
Collaborator
There was a problem hiding this comment.
I would really try to avoid doing this. While it technically works, it sets a bad pattern of just turning off turbo without finding the root cause and obviously removes all the benefits that turbo provides.
I did some digging and found this config setting we should have.
# config/initializers/devise.rb
Devise.setup do |config|
# ...
config.responder.error_status = :unprocessable_entity
config.responder.redirect_status = :see_other
# ...
end
I'm wondering if this was part of the issue with some of our flaky specs regarding devise flows.
When the password reset failed validation (short password..) the controller would responded with turbo but status was 200 so the validation errors wouldn't display.
Collaborator
Author
There was a problem hiding this comment.
ooooo, TYYY for finding that config setting! will submit a pr for that.
maebeale
changed the title
maebeale
changed the title
maebeale
force-pushed
the
maebeale/fix-auth-flow-bugs
branch
from
fa4f9d3 to
026762d
Compare
- Disable Turbo on password reset form to fix "button does nothing" bug - Redirect expired welcome token users to password reset instead of sign-in dead end (user has no known password) - Fix Font Awesome icon class on welcome page password toggles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a new user confirms their email after the welcome token expires, regenerate the token and send them to the welcome set-password page instead of the Devise password reset form. Distinguish email-change reconfirmation (existing users who have signed in) from initial account setup to avoid routing active users through the welcome flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Existing users confirming an email change are likely not signed in when clicking the link, so send them to sign-in with a clear message rather than bouncing through authenticate_user!. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Justin <16829344+jmilljr24@users.noreply.github.com>
PR #1443 removed token expiry, so the "expired token gets regenerated" test no longer applies. Updated to verify old tokens are kept as-is. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n_count
Checking `previous_changes.key?("email")` directly detects whether this
confirmation was an email change, rather than inferring from sign_in_count.
Handles edge cases like legacy ported users (no welcome token, no sign-ins)
and users who happen to have a welcome token while doing an email change.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
maebeale
force-pushed
the
maebeale/fix-auth-flow-bugs
branch
from
c1ecc8a to
2ede30b
Compare
maebeale
commented
Mar 28, 2026
| def after_confirmation_success(resource) | ||
| if resource.welcome_instructions_token.present? | ||
| if resource.previous_changes.key?("email") | ||
| redirect_to new_user_session_path, notice: "Your email has been confirmed. Please sign in." |
Collaborator
Author
There was a problem hiding this comment.
this is now true to what we're actually trying to separate -- the email change flow vs the initial welcome flow
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the goal of this PR and why is this important?
How did you approach the change?
sign_in_countto distinguish new users (send to welcome flow) from existing users changing their email (send to sign-in with "confirmed" message)fa fa-eye→fa-solid fa-eyeon welcome page password toggle buttonsUI Testing Checklist
🤖 Generated with Claude Code