Merge pull request #1968 from basecamp/flavorjones/only-email-confirmed-users

flavorjones · web-flow · commit 00db048f9ebf · 2025-12-05T22:15:13.000-05:00
Mark users as "verified" and only send notifications to verified users
diff --git a/app/controllers/join_codes_controller.rb b/app/controllers/join_codes_controller.rb
@@ -18,7 +18,7 @@ def create
     if identity == Current.identity && user.setup?
       redirect_to landing_url(script_name: @join_code.account.slug)
     elsif identity == Current.identity
-      redirect_to new_users_join_url(script_name: @join_code.account.slug)
+      redirect_to new_users_verification_url(script_name: @join_code.account.slug)
     else
       logout_and_send_new_magic_link(identity)
       redirect_to session_magic_link_url(script_name: nil)
@@ -44,6 +44,6 @@ def logout_and_send_new_magic_link(identity)
       magic_link = identity.send_magic_link
       serve_development_magic_link(magic_link)
 
-      session[:return_to_after_authenticating] = new_users_join_url(script_name: @join_code.account.slug)
+      session[:return_to_after_authenticating] = new_users_verification_url(script_name: @join_code.account.slug)
     end
 end
diff --git a/app/controllers/users/verifications_controller.rb b/app/controllers/users/verifications_controller.rb
@@ -0,0 +1,11 @@
+class Users::VerificationsController < ApplicationController
+  layout "public"
+
+  def new
+  end
+
+  def create
+    Current.user.verify
+    redirect_to new_users_join_path
+  end
+end
diff --git a/app/javascript/controllers/magic_link_controller.js b/app/javascript/controllers/magic_link_controller.js
@@ -4,12 +4,18 @@ import { onNextEventLoopTick } from "helpers/timing_helpers"
 export default class extends Controller {
   static targets = [ "input" ]
 
+  submitOnEnter(event) {
+    event.preventDefault()
+    this.submit()
+  }
+
+  submitOnPaste() {
+    onNextEventLoopTick(() => this.submit())
+  }
+
   submit() {
-    onNextEventLoopTick(() => {
-      if (!this.inputTarget.disabled) {
-        this.element.submit()
-        this.inputTarget.disabled = true
-      }
-    })
+    if (this.inputTarget.disabled) return
+    this.element.submit()
+    this.inputTarget.disabled = true
   }
 }
diff --git a/app/models/account.rb b/app/models/account.rb
@@ -21,7 +21,7 @@ class << self
     def create_with_owner(account:, owner:)
       create!(**account).tap do |account|
         account.users.create!(role: :system, name: "System")
-        account.users.create!(**owner.reverse_merge(role: "owner"))
+        account.users.create!(**owner.reverse_merge(role: "owner", verified_at: Time.current))
       end
     end
   end
diff --git a/app/models/user.rb b/app/models/user.rb
@@ -28,6 +28,14 @@ def setup?
     name != identity.email_address
   end
 
+  def verified?
+    verified_at.present?
+  end
+
+  def verify
+    update!(verified_at: Time.current) unless verified?
+  end
+
   private
     def close_remote_connections
       ActionCable.server.remote_connections.where(current_user: self).disconnect(reconnect: false)
diff --git a/app/models/user/settings.rb b/app/models/user/settings.rb
@@ -21,7 +21,7 @@ def bundle_aggregation_period
   end
 
   def bundling_emails?
-    !bundle_email_never? && !user.system? && user.active?
+    !bundle_email_never? && !user.system? && user.active? && user.verified?
   end
 
   def timezone
diff --git a/app/views/sessions/magic_links/show.html.erb b/app/views/sessions/magic_links/show.html.erb
@@ -10,7 +10,7 @@
     <%= form.text_field :code, required: true, class: "input center txt-align-enter txt-large",
         autofocus: true, autocorrect: "off", autocapitalize: "off", spellcheck: "false", "data-1p-ignore": true,
         autocomplete: "one-time-code", maxlength: "6", placeholder: "••••••", value: params[:code],
-        data: { magic_link_target: "input", action: "keydown.enter->magic-link#submit paste->magic-link#submit" } %>
+        data: { magic_link_target: "input", action: "keydown.enter->magic-link#submitOnEnter paste->magic-link#submitOnPaste" } %>
   <% end %>
 
   <p class="txt-small">The code you receive will work for <%= distance_of_time_in_words(MagicLink::EXPIRATION_TIME) %>.</p>
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
@@ -18,20 +18,25 @@
       <div class="flex flex-column gap-half margin-block-end">
         <h1 class="txt-x-large margin-none"><%= @user.name %></h1>
         <div class="txt-medium">
-          <% if @user.active? %>
-            <%= mail_to @user.identity.email_address %>
-          <% else %>
+          <% if !@user.active? %>
             <%= @user.name %> is no longer on this account
+          <% elsif !@user.verified? %>
+            Unverified
+            <div class="txt-small txt-tinted">A sign-in code has been sent to this email address, but the user has not yet logged in to confirm their identity.</div>
+          <% else %>
+            <%= mail_to @user.identity.email_address %>
           <% end %>
         </div>
       </div>
 
-      <div class="flex-inline center justify-center flex-wrap gap">
-        <%= link_to "Which cards are assigned to #{me_or_you}?",
-              cards_path(assignee_ids: [ @user.id ], sorted_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
-        <%= link_to "Which cards were added by #{me_or_you}?",
-              cards_path(creator_ids: [ @user.id ], sorted_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
-      </div>
+      <% if @user.verified? %>
+        <div class="flex-inline center justify-center flex-wrap gap">
+          <%= link_to "Which cards are assigned to #{me_or_you}?",
+                cards_path(assignee_ids: [ @user.id ], sorted_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
+          <%= link_to "Which cards were added by #{me_or_you}?",
+                cards_path(creator_ids: [ @user.id ], sorted_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
+        </div>
+      <% end %>
     </div>
   </section>
 
@@ -48,4 +53,6 @@
   <% end %>
 </div>
 
-<%= turbo_frame_tag "user_events", src: user_events_path(@user) %>
+<% if @user.verified? %>
+  <%= turbo_frame_tag "user_events", src: user_events_path(@user) %>
+<% end %>
diff --git a/app/views/users/verifications/new.html.erb b/app/views/users/verifications/new.html.erb
@@ -0,0 +1 @@
+<%= auto_submit_form_with url: users_verifications_path, method: :post %>
diff --git a/config/routes.rb b/config/routes.rb
@@ -138,6 +138,7 @@
 
   namespace :users do
     resources :joins
+    resources :verifications, only: %i[ new create ]
   end
 
   resource :session do
diff --git a/db/migrate/20251205010536_add_verified_at_to_users.rb b/db/migrate/20251205010536_add_verified_at_to_users.rb
@@ -0,0 +1,5 @@
+class AddVerifiedAtToUsers < ActiveRecord::Migration[8.2]
+  def change
+    add_column :users, :verified_at, :datetime
+  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_12_01_100607) do
+ActiveRecord::Schema[8.2].define(version: 2025_12_05_010536) do
   create_table "accesses", id: :uuid, force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
@@ -499,6 +499,7 @@
     t.string "name", limit: 255, null: false
     t.string "role", limit: 255, default: "member", null: false
     t.datetime "updated_at", null: false
+    t.datetime "verified_at"
     t.index ["account_id", "identity_id"], name: "index_users_on_account_id_and_identity_id", unique: true
     t.index ["account_id", "role"], name: "index_users_on_account_id_and_role"
     t.index ["identity_id"], name: "index_users_on_identity_id"
diff --git a/db/seeds.rb b/db/seeds.rb
@@ -36,7 +36,7 @@ def find_or_create_user(full_name, email_address)
     if user = identity.users.find_by(account: Current.account)
       user
     else
-      User.create!(name: full_name, identity: identity, account: Current.account)
+      User.create!(name: full_name, identity: identity, account: Current.account, verified_at: Time.current)
     end
   end
 
diff --git a/script/migrations/20251205-backfill-verified-at.rb b/script/migrations/20251205-backfill-verified-at.rb
@@ -0,0 +1,112 @@
+#!/usr/bin/env ruby
+
+require_relative "../../config/environment"
+
+BACKFILL_TIMESTAMP = Time.parse("2025-12-02 12:00:00 UTC")
+
+def collect_verified_user_ids
+  verified_ids = Set.new
+
+  # Owners (they created the account)
+  verified_ids.merge(User.where(role: :owner).pluck(:id))
+  puts "After owners: #{verified_ids.size} users"
+
+  # Card creators
+  verified_ids.merge(Card.distinct.pluck(:creator_id).compact)
+  puts "After card creators: #{verified_ids.size} users"
+
+  # Comment creators
+  verified_ids.merge(Comment.distinct.pluck(:creator_id).compact)
+  puts "After comment creators: #{verified_ids.size} users"
+
+  # Board creators
+  verified_ids.merge(Board.distinct.pluck(:creator_id).compact)
+  puts "After board creators: #{verified_ids.size} users"
+
+  # Event creators
+  verified_ids.merge(Event.distinct.pluck(:creator_id).compact)
+  puts "After event creators: #{verified_ids.size} users"
+
+  # Assigners (not assignees - they could be assigned without logging in)
+  verified_ids.merge(Assignment.distinct.pluck(:assigner_id).compact)
+  puts "After assigners: #{verified_ids.size} users"
+
+  # Manual closers (user_id is nil for automatic closures)
+  verified_ids.merge(Closure.where.not(user_id: nil).distinct.pluck(:user_id).compact)
+  puts "After closers: #{verified_ids.size} users"
+
+  # Manual postponers (user_id is nil for automatic entropy postponements)
+  verified_ids.merge(Card::NotNow.where.not(user_id: nil).distinct.pluck(:user_id).compact)
+  puts "After postponers: #{verified_ids.size} users"
+
+  # Reactors
+  verified_ids.merge(Reaction.distinct.pluck(:reacter_id).compact)
+  puts "After reactors: #{verified_ids.size} users"
+
+  # Filter creators
+  verified_ids.merge(Filter.distinct.pluck(:creator_id).compact)
+  puts "After filter creators: #{verified_ids.size} users"
+
+  # Pinners
+  verified_ids.merge(Pin.distinct.pluck(:user_id).compact)
+  puts "After pinners: #{verified_ids.size} users"
+
+  # Board accessors (accessed_at is touched when viewing boards)
+  verified_ids.merge(Access.where.not(accessed_at: nil).distinct.pluck(:user_id).compact)
+  puts "After board accessors: #{verified_ids.size} users"
+
+  # Export requesters
+  verified_ids.merge(Account::Export.distinct.pluck(:user_id).compact)
+  puts "After export requesters: #{verified_ids.size} users"
+
+  # Push subscribers
+  verified_ids.merge(Push::Subscription.distinct.pluck(:user_id).compact)
+  puts "After push subscribers: #{verified_ids.size} users"
+
+  # Users who completed setup (name != email)
+  verified_ids.merge(
+    User.joins(:identity)
+        .where.not("users.name = identities.email_address")
+        .pluck(:id)
+  )
+  puts "After setup completers: #{verified_ids.size} users"
+
+  # Users whose identity has at least one session
+  verified_ids.merge(
+    User.where(identity_id: Session.distinct.select(:identity_id)).pluck(:id)
+  )
+  puts "After identity sessions: #{verified_ids.size} users"
+
+  verified_ids
+end
+
+puts "Collecting verified user IDs..."
+verified_user_ids = collect_verified_user_ids
+
+puts "\nFiltering to unverified users only..."
+users_to_update = User.where(id: verified_user_ids.to_a)
+                      .where(verified_at: nil)
+                      .where(active: true)
+                      .where.not(identity_id: nil)
+                      .where.not(role: :system)
+
+update_count = users_to_update.count
+puts "Found #{update_count} users to backfill"
+
+# Report remaining unverified users (before update)
+remaining_before = User.where(verified_at: nil, active: true)
+                       .where.not(identity_id: nil)
+                       .where.not(role: :system)
+                       .count
+remaining_after = remaining_before - update_count
+puts "\nCurrently unverified active users: #{remaining_before}"
+puts "After backfill, remaining unverified: #{remaining_after}"
+puts "These users will need to verify on next login."
+
+if update_count > 0
+  puts "\nBackfilling verified_at..."
+  updated = users_to_update.update_all(verified_at: BACKFILL_TIMESTAMP)
+  puts "Updated #{updated} users"
+end
+
+puts "\nDone!"
diff --git a/test/controllers/join_codes_controller_test.rb b/test/controllers/join_codes_controller_test.rb
@@ -36,7 +36,7 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest
     end
 
     assert_redirected_to session_magic_link_url(script_name: nil)
-    assert_equal new_users_join_url(script_name: @account.slug), session[:return_to_after_authenticating]
+    assert_equal new_users_verification_url(script_name: @account.slug), session[:return_to_after_authenticating]
   end
 
   test "create for existing identity" do
@@ -55,7 +55,7 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to landing_url(script_name: @account.slug)
   end
 
-  test "create for signed-in identity without a user in the account redirects to user setup" do
+  test "create for signed-in identity without a user in the account redirects to verification" do
     identity = identities(:mike)
     sign_in_as :mike
 
@@ -67,6 +67,6 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest
       end
     end
 
-    assert_redirected_to new_users_join_url(script_name: @account.slug)
+    assert_redirected_to new_users_verification_url(script_name: @account.slug)
   end
 end
diff --git a/test/controllers/users/verifications_controller_test.rb b/test/controllers/users/verifications_controller_test.rb
@@ -0,0 +1,24 @@
+require "test_helper"
+
+class Users::VerificationsControllerTest < ActionDispatch::IntegrationTest
+  test "new renders the auto-submit form" do
+    sign_in_as :david
+
+    get new_users_verification_path
+
+    assert_response :ok
+  end
+
+  test "create verifies the user and redirects to join" do
+    sign_in_as :david
+
+    user = users(:david)
+    user.update_column(:verified_at, nil)
+    assert_not user.verified?
+
+    post users_verifications_path
+
+    assert_redirected_to new_users_join_path
+    assert user.reload.verified?
+  end
+end
diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml
diff --git a/test/models/account_test.rb b/test/models/account_test.rb
diff --git a/test/models/user/settings_test.rb b/test/models/user/settings_test.rb
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
diff --git a/test/system/smoke_test.rb b/test/system/smoke_test.rb

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+<%= auto_submit_form_with url: users_verifications_path, method: :post %>`