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

Split sign in and sign up

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)

app/controllers/sessions/magic_links_controller.rb

@@ -9,9 +9,9 @@ class Sessions::MagicLinksController < ApplicationController
   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 @@ class Sessions::MagicLinksController < ApplicationController
     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

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 @@ class SessionsController < ApplicationController
 
   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 @@ class SessionsController < ApplicationController
     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

app/controllers/signups/completions_controller.rb

@@ -1,4 +1,4 @@
-class Signup::CompletionsController < ApplicationController
+class Signups::CompletionsController < ApplicationController
   layout "public"
 
   disallow_account_scope

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

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

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 @@ class MagicLink < ApplicationRecord
 
   def consume
     destroy
-    identity
+    self
   end
 
   private

app/models/signup.rb

@@ -18,7 +18,7 @@ class Signup
 
   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

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 %>

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:

app/views/sessions/menus/show.html+menu_section.erb

@@ -1,9 +0,0 @@
-<%= turbo_frame_tag Current.identity, :account_menu do %>
-  <% cache [ Current.identity, @accounts ] do %>
-    <%= collapsible_nav_section "Accounts" do %>
-      <% @accounts.each do |account| %>
-        <%= filter_place_menu_item landing_url(script_name: account.slug, account.name, "marker", new_window: true %>
-      <% end %>
-    <% end %>
-  <% end %>
-<% end %>

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>

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>

app/views/signup/new.html.erb

@@ -1,28 +0,0 @@
-<% @page_title = "Sign up for Fizzy" %>
-
-<div class="panel panel--centered flex flex-column gap-half <%= "shake" if flash[:alert] %>">
-  <h1 class="txt-xx-large margin-block-end-double">Sign up</h1>
-
-  <%= form_with model: @signup, url: signup_path, scope: "signup", class: "flex flex-column gap", data: { turbo: false, controller: "form" } do |form| %>
-    <%= form.email_field :email_address, class: "input", autocomplete: "username", placeholder: "Email address", required: true %>
-
-    <% if @signup.errors.any? %>
-      <div class="margin-block-half">
-        <ul class="margin-block-none txt-negative txt-small">
-          <% @signup.errors.full_messages.each do |message| %>
-            <li><%= message %></li>
-          <% end %>
-        </ul>
-      </div>
-    <% end %>
-
-    <button type="submit" class="btn btn--link center" data-form-target="submit">
-      <span>Continue</span>
-      <%= icon_tag "arrow-right" %>
-    </button>
-  <% end %>
-</div>
-
-<% content_for :footer do %>
-  <%= render "sessions/footer" %>
-<% end %>

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 %>

config/routes.rb

@@ -155,10 +155,14 @@ Rails.application.routes.draw do
     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

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

db/schema.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, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
@@ -315,6 +315,7 @@ ActiveRecord::Schema[8.2].define(version: 2025_11_27_000001) do
     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"

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 @@ ActiveRecord::Schema[8.2].define(version: 2025_11_27_000001) do
     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"

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 }
-    end
 
-    assert_response :redirect
-    assert cookies[:session_token].present?
-    assert_not MagicLink.exists?(magic_link.id), "The magic link should be consumed"
+      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)
+
+    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

test/controllers/sessions_controller_test.rb

@@ -23,11 +23,9 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
 
   test "create for a new user" do
     untenanted do
-      assert_difference -> { Identity.count }, +1 do
-        assert_difference -> { MagicLink.count }, +1 do
-          post session_path,
-            params: { email_address: "nonexistent-#{SecureRandom.hex(6)}@example.com" }
-        end
+      assert_no_difference -> { MagicLink.count } do
+        post session_path,
+          params: { email_address: "nonexistent-#{SecureRandom.hex(6)}@example.com" }
       end
 
       assert_redirected_to session_magic_link_path

test/controllers/signups_controller_test.rb

@@ -0,0 +1,52 @@
+require "test_helper"
+
+class SignupsControllerTest < ActionDispatch::IntegrationTest
+  test "new" do
+    untenanted do
+      get new_signup_path
+
+      assert_response :success
+    end
+  end
+
+  test "new for an authenticated user" do
+    identity = identities(:kevin)
+    sign_in_as identity
+
+    untenanted do
+      get new_signup_path
+
+      assert_redirected_to new_signup_completion_path
+    end
+  end
+
+  test "create" do
+    email_address = "newuser-#{SecureRandom.hex(6)}@example.com"
+
+    untenanted do
+      assert_difference -> { Identity.count }, +1 do
+        assert_difference -> { MagicLink.count }, +1 do
+          post signup_path, params: { signup: { email_address: email_address } }
+        end
+      end
+
+      assert_redirected_to session_magic_link_path
+    end
+  end
+
+  test "create for an authenticated user" do
+    identity = identities(:kevin)
+    sign_in_as identity
+
+    untenanted do
+      assert_no_difference -> { Identity.count } do
+        assert_no_difference -> { MagicLink.count } do
+          post signup_path,
+            params: { signup: { email_address: identity.email_address } }
+        end
+      end
+
+      assert_redirected_to new_signup_completion_path
+    end
+  end
+end

test/models/magic_link_test.rb

@@ -32,8 +32,8 @@ class MagicLinkTest < ActiveSupport::TestCase
     magic_link = MagicLink.create!(identity: identities(:kevin))
     code_with_spaces = magic_link.code.downcase.chars.join(" ")
 
-    identity = MagicLink.consume(code_with_spaces)
-    assert_equal identities(:kevin), identity
+    consumed_magic_link = MagicLink.consume(code_with_spaces)
+    assert_equal magic_link, consumed_magic_link
     assert_not MagicLink.exists?(magic_link.id)
 
     expired_link = MagicLink.create!(identity: identities(:kevin))