Merge pull request #1759 from basecamp/split-signin-and-signup

monorkin · web-flow · commit cb3ca8fa3023 · 2025-11-29T15:43:19.000+01:00
Split sign in and sign up
diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock
@@ -1,6 +1,6 @@
 GIT
   remote: https://github.com/basecamp/fizzy-saas
-  revision: 7f392bbbf9f5170d334b6ee2f6d240569bd157ed
+  revision: f80da3c2faf34b94d65a41a501f19e8dba379012
   specs:
     fizzy-saas (0.1.0)
       prometheus-client-mmap
@@ -52,7 +52,7 @@ GIT
 
 GIT
   remote: https://github.com/rails/rails.git
-  revision: 4f7ab01bb5d6be78c7447dbb230c55027d08ae34
+  revision: 690ec8898318b8f50714e86676353ebe1551261e
   branch: main
   specs:
     actioncable (8.2.0.alpha)
@@ -423,7 +423,7 @@ GEM
     rake-compiler-dock (1.9.1)
     rb_sys (0.9.117)
       rake-compiler-dock (= 1.9.1)
-    rdoc (6.15.1)
+    rdoc (6.16.1)
       erb
       psych (>= 4.0.0)
       tsort
@@ -479,10 +479,10 @@ GEM
       rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2, < 4.0)
       websocket (~> 1.0)
-    sentry-rails (6.1.1)
+    sentry-rails (6.2.0)
       railties (>= 5.2.0)
-      sentry-ruby (~> 6.1.1)
-    sentry-ruby (6.1.1)
+      sentry-ruby (~> 6.2.0)
+    sentry-ruby (6.2.0)
       bigdecimal
       concurrent-ruby (~> 1.0, >= 1.0.2)
     sniffer (0.5.0)
diff --git a/app/controllers/sessions/magic_links_controller.rb b/app/controllers/sessions/magic_links_controller.rb
@@ -9,9 +9,9 @@ def show
   end
 
   def create
-    if identity = MagicLink.consume(code)
-      start_new_session_for identity
-      redirect_to after_authentication_url
+    if magic_link = MagicLink.consume(code)
+      start_new_session_for magic_link.identity
+      redirect_to after_sign_in_url(magic_link)
     else
       redirect_to session_magic_link_path, alert: "Try another code."
     end
@@ -21,4 +21,12 @@ def create
     def code
       params.expect(:code)
     end
+
+    def after_sign_in_url(magic_link)
+      if magic_link.for_sign_up?
+        new_signup_completion_path
+      else
+        after_authentication_url
+      end
+    end
 end
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
@@ -1,13 +1,4 @@
 class SessionsController < ApplicationController
-  # FIXME: Remove this before launch!
-  unless Rails.env.local?
-    http_basic_authenticate_with \
-      name: Rails.application.credentials.account_signup_http_basic_auth.name,
-      password: Rails.application.credentials.account_signup_http_basic_auth.password,
-      realm: "Fizzy Signup",
-      only: :create, unless: -> { Identity.exists?(email_address: email_address) }
-  end
-
   disallow_account_scope
   require_unauthenticated_access except: :destroy
   rate_limit to: 10, within: 3.minutes, only: :create, with: -> { redirect_to new_session_path, alert: "Try again later." }
@@ -19,10 +10,11 @@ def new
 
   def create
     if identity = Identity.find_by_email_address(email_address)
-      handle_existing_user(identity)
-    elsif
-      handle_new_signup
+      magic_link = identity.send_magic_link
+      flash[:magic_link_code] = magic_link&.code if Rails.env.development?
     end
+
+    redirect_to session_magic_link_path
   end
 
   def destroy
@@ -34,16 +26,4 @@ def destroy
     def email_address
       params.expect(:email_address)
     end
-
-    def handle_existing_user(identity)
-      magic_link = identity.send_magic_link
-      flash[:magic_link_code] = magic_link&.code if Rails.env.development?
-      redirect_to session_magic_link_path
-    end
-
-    def handle_new_signup
-      Signup.new(email_address: email_address).create_identity
-      session[:return_to_after_authenticating] = new_signup_completion_path
-      redirect_to session_magic_link_path
-    end
 end
diff --git a/app/controllers/signups/completions_controller.rb b/app/controllers/signups/completions_controller.rb
@@ -1,4 +1,4 @@
-class Signup::CompletionsController < ApplicationController
+class Signups::CompletionsController < ApplicationController
   layout "public"
 
   disallow_account_scope
diff --git a/app/controllers/signups_controller.rb b/app/controllers/signups_controller.rb
@@ -0,0 +1,34 @@
+class SignupsController < ApplicationController
+  # FIXME: Remove this before launch!
+  unless Rails.env.local?
+    http_basic_authenticate_with \
+      name: Rails.application.credentials.account_signup_http_basic_auth.name,
+      password: Rails.application.credentials.account_signup_http_basic_auth.password,
+      realm: "Fizzy Signup"
+  end
+
+  disallow_account_scope
+  allow_unauthenticated_access
+  rate_limit to: 10, within: 3.minutes, only: :create, with: -> { redirect_to new_signup_path, alert: "Try again later." }
+  before_action :redirect_authenticated_user
+
+  layout "public"
+
+  def new
+    @signup = Signup.new
+  end
+
+  def create
+    Signup.new(signup_params).create_identity
+    redirect_to session_magic_link_path
+  end
+
+  private
+    def redirect_authenticated_user
+      redirect_to new_signup_completion_path if authenticated?
+    end
+
+    def signup_params
+      params.expect signup: :email_address
+    end
+end
diff --git a/app/models/identity.rb b/app/models/identity.rb
@@ -12,8 +12,10 @@ class Identity < ApplicationRecord
 
   normalizes :email_address, with: ->(value) { value.strip.downcase.presence }
 
-  def send_magic_link
-    magic_links.create!.tap do |magic_link|
+  def send_magic_link(**attributes)
+    attributes[:purpose] = attributes.delete(:for) if attributes.key?(:for)
+
+    magic_links.create!(attributes).tap do |magic_link|
       MagicLinkMailer.sign_in_instructions(magic_link).deliver_later
     end
   end
diff --git a/app/models/magic_link.rb b/app/models/magic_link.rb
@@ -4,6 +4,8 @@ class MagicLink < ApplicationRecord
 
   belongs_to :identity
 
+  enum :purpose, %w[ sign_in sign_up ], prefix: :for, default: :sign_in
+
   scope :active, -> { where(expires_at: Time.current...) }
   scope :stale, -> { where(expires_at: ..Time.current) }
 
@@ -24,7 +26,7 @@ def cleanup
 
   def consume
     destroy
-    identity
+    self
   end
 
   private
diff --git a/app/models/signup.rb b/app/models/signup.rb
@@ -18,7 +18,7 @@ def initialize(...)
 
   def create_identity
     @identity = Identity.find_or_create_by!(email_address: email_address)
-    @identity.send_magic_link
+    @identity.send_magic_link for: :sign_up
   end
 
   def complete
diff --git a/app/views/mailers/magic_link_mailer/sign_in_instructions.html.erb b/app/views/mailers/magic_link_mailer/sign_in_instructions.html.erb
@@ -1,5 +1,5 @@
 
-<% if @identity.users.any? %>
+<% if @magic_link.for_sign_in? %>
   <h1 class="title">Fizzy verification code</h1>
   <p class="subtitle">Please enter this 6-character verification code on the Fizzy sign-in page:</p>
 <% else %>
diff --git a/app/views/mailers/magic_link_mailer/sign_in_instructions.text.erb b/app/views/mailers/magic_link_mailer/sign_in_instructions.text.erb
@@ -1,4 +1,4 @@
-<% if @identity.users.any? %>
+<% if @magic_link.for_sign_in? %>
 Please enter this 6-character verification code on the Fizzy sign-in page:
 <% else %>
 Please enter this 6-character verification code on the Fizzy sign-up page to create your new account:
diff --git a/app/views/sessions/menus/show.html+menu_section.erb b/app/views/sessions/menus/show.html+menu_section.erb
diff --git a/app/views/sessions/menus/show.html.erb b/app/views/sessions/menus/show.html.erb
@@ -25,7 +25,7 @@
     <% end %>
 
     <div class="margin-block-start">
-      <%= link_to new_signup_completion_path, class: "btn btn--plain txt-link center txt-small", data: { turbo_prefetch: false } do %>
+      <%= link_to new_signup_path, class: "btn btn--plain txt-link center txt-small", data: { turbo_prefetch: false } do %>
         <span>Sign up for a new Fizzy account</span>
       <% end %>
     </div>
diff --git a/app/views/sessions/new.html.erb b/app/views/sessions/new.html.erb
@@ -10,7 +10,7 @@
       </label>
     </div>
 
-    <p><strong>New here?</strong> Enter your email to create an account. <strong>Already have an account?</strong> Enter your email and we’ll get you signed in.</p>
+    <p><strong>New here?</strong> <%= link_to "sign up", new_signup_path %> to create an account. <strong>Already have an account?</strong> Enter your email and we’ll get you signed in.</p>
 
     <button type="submit" id="log_in" class="btn btn--link center txt-medium">
       <span>Let’s go</span>
diff --git a/app/views/signup/new.html.erb b/app/views/signup/new.html.erb
diff --git a/app/views/signups/completions/new.html.erb b/app/views/signups/completions/new.html.erb
diff --git a/app/views/signups/new.html.erb b/app/views/signups/new.html.erb
@@ -0,0 +1,24 @@
+<% @page_title = "Signup for Fizzy" %>
+
+<div class="panel panel--centered flex flex-column gap-half">
+  <h1 class="txt-x-large font-weight-black margin-block-end">Sign up</h1>
+
+  <%= form_with model: @signup, url: signup_path, scope: "signup", class: "flex flex-column gap", data: { turbo: false, controller: "form" } do |form| %>
+    <div class="flex align-center gap">
+      <label class="flex align-center gap input input--actor">
+        <%= form.email_field :email_address, required: true, class: "input txt-large full-width", autofocus: true, autocomplete: "username", placeholder: "Enter your email address…" %>
+      </label>
+    </div>
+
+    <p>Enter your email to create an account.</p>
+
+    <button type="submit" id="log_in" class="btn btn--link center txt-medium">
+      <span>Let’s go</span>
+      <%= icon_tag "arrow-right" %>
+    </button>
+  <% end %>
+</div>
+
+<% content_for :footer do %>
+  <%= render "sessions/footer" %>
+<% end %>
diff --git a/config/routes.rb b/config/routes.rb
@@ -155,10 +155,14 @@
     end
   end
 
-  get "/signup/new", to: redirect("/session/new")
+  get "/signup", to: redirect("/signup/new")
 
-  namespace :signup do
-    resource :completion, only: %i[ new create ]
+  resource :signup, only: %i[ new create ] do
+    collection do
+      scope module: :signups, as: :signup do
+        resource :completion, only: %i[ new create ]
+      end
+    end
   end
 
   resource :landing
diff --git a/db/migrate/20251129110120_add_purpose_to_magic_links.rb b/db/migrate/20251129110120_add_purpose_to_magic_links.rb
@@ -0,0 +1,11 @@
+class AddPurposeToMagicLinks < ActiveRecord::Migration[8.2]
+  def change
+    add_column :magic_links, :purpose, :integer, null: true
+
+    execute <<-SQL
+      UPDATE magic_links SET purpose = 0
+    SQL
+
+    change_column_null :magic_links, :purpose, false
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.2].define(version: 2025_11_27_000001) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_29_110120) do
   create_table "accesses", id: :uuid, force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
@@ -315,6 +315,7 @@
     t.datetime "created_at", null: false
     t.datetime "expires_at", null: false
     t.uuid "identity_id"
+    t.integer "purpose", null: false
     t.datetime "updated_at", null: false
     t.index ["code"], name: "index_magic_links_on_code", unique: true
     t.index ["expires_at"], name: "index_magic_links_on_expires_at"
diff --git a/test/controllers/sessions/magic_links_controller_test.rb b/test/controllers/sessions/magic_links_controller_test.rb
@@ -9,17 +9,32 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "create" do
+  test "create with sign in code" do
     identity = identities(:kevin)
     magic_link = MagicLink.create!(identity: identity)
 
     untenanted do
       post session_magic_link_url, params: { code: magic_link.code }
+
+      assert_response :redirect
+      assert cookies[:session_token].present?
+      assert_redirected_to landing_path, "Should redirect to after authentication path"
+      assert_not MagicLink.exists?(magic_link.id), "The magic link should be consumed"
     end
+  end
+
+  test "create with sign up code" do
+    identity = identities(:kevin)
+    magic_link = MagicLink.create!(identity: identity, purpose: :sign_up)
 
-    assert_response :redirect
-    assert cookies[:session_token].present?
-    assert_not MagicLink.exists?(magic_link.id), "The magic link should be consumed"
+    untenanted do
+      post session_magic_link_url, params: { code: magic_link.code }
+
+      assert_response :redirect
+      assert cookies[:session_token].present?
+      assert_redirected_to new_signup_completion_path, "Should redirect to signup completion"
+      assert_not MagicLink.exists?(magic_link.id), "The magic link should be consumed"
+    end
   end
 
   test "create with invalid code" do
diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb
diff --git a/test/controllers/signups_controller_test.rb b/test/controllers/signups_controller_test.rb
diff --git a/test/models/magic_link_test.rb b/test/models/magic_link_test.rb

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-class Signup::CompletionsController < ApplicationController`
	`1`	`+class Signups::CompletionsController < ApplicationController`
`2`	`2`	`layout "public"`
`3`	`3`
`4`	`4`	`disallow_account_scope`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-<% if @identity.users.any? %>`
	`1`	`+<% if @magic_link.for_sign_in? %>`
`2`	`2`	`Please enter this 6-character verification code on the Fizzy sign-in page:`
`3`	`3`	`<% else %>`
`4`	`4`	`Please enter this 6-character verification code on the Fizzy sign-up page to create your new account:`