From feba821e673a5bb77fdade1afcb9f1db5256c970 Mon Sep 17 00:00:00 2001 From: maebeale Date: Mon, 2 Mar 2026 17:46:19 -0500 Subject: [PATCH 1/2] Add email change notifications and password change ahoy tracking - Add account_email_change_requested notification when Devise sends reconfirmation email (distinct from initial account_confirmation) - Add account_email_changed notification (after_commit) when email actually changes after confirmation - Track auth.email_change_requested_email_sent ahoy event for reconfirmation emails (shows in user activity feed) - Track auth.password_changed ahoy event via User model callback so all password change paths are covered - Add DeviseMailer, Notification, and User model specs Co-Authored-By: Claude Opus 4.6 --- app/mailers/devise_mailer.rb | 16 +++- app/models/notification.rb | 2 + app/models/user.rb | 22 +++++ spec/mailers/devise_mailer_spec.rb | 140 +++++++++++++++++++++++++++++ spec/models/notification_spec.rb | 20 +++++ spec/models/user_spec.rb | 75 ++++++++++++++++ 6 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 spec/mailers/devise_mailer_spec.rb diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index c648168fd..cf1777dd4 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -61,12 +61,20 @@ def create_notification_record return unless @mail && @record kind = notification_kind_for_devise_action[action_name] + + # For email changes (reconfirmation), use account_email_change_requested instead + if action_name == "confirmation_instructions" && @record.try(:pending_reconfirmation?) + kind = "account_email_change_requested" + end + return unless kind # don’t create fyi emails for Devise mailers you don’t care about + recipient_email = (kind == "account_email_change_requested") ? @record.unconfirmed_email : @record.email + notification = NotificationServices::CreateNotification.call( noticeable: @record, recipient_role: :person, - recipient_email: @record.email, + recipient_email: recipient_email, kind: kind, notification_type: 1, deliver: false # Devise already sent the email, so no need to deliver via the job @@ -104,6 +112,12 @@ def track_devise_email_event "confirmation_instructions" => "auth.confirmation_email_sent", "unlock_instructions" => "auth.unlock_email_sent" }[action_name] + + # For email changes (reconfirmation), use a specific event name + if action_name == "confirmation_instructions" && @record.try(:pending_reconfirmation?) + event_name = "auth.email_change_requested_email_sent" + end + return unless event_name Analytics::AhoyTracker.track_auth_event( diff --git a/app/models/notification.rb b/app/models/notification.rb index 7159164bb..355a4e63c 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -13,6 +13,8 @@ class Notification < ApplicationRecord welcome_instructions account_confirmation account_confirmation_fyi + account_email_change_requested + account_email_changed account_confirmed account_confirmed_fyi account_unlock_fyi diff --git a/app/models/user.rb b/app/models/user.rb index e8f9bde52..0c3f66eb9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -17,6 +17,9 @@ class User < ApplicationRecord after_update :track_admin_change after_update :track_inactive_change after_update :track_password_reset_sent + after_update :track_password_changed + + after_commit :create_email_changed_notification, on: :update before_destroy :track_account_deleted @@ -341,12 +344,31 @@ def track_password_reset_sent track_auth_event("auth.password_reset_sent", { sent_at: reset_password_sent_at }) end + def track_password_changed + return unless saved_change_to_encrypted_password? + track_auth_event("auth.password_changed") + end + def sync_email_to_person return unless saved_change_to_email? && person.present? person.update(email: email) end + def create_email_changed_notification + return unless previous_changes.key?("email") + + _from, to = previous_changes["email"] + NotificationServices::CreateNotification.call( + noticeable: self, + recipient_role: :person, + recipient_email: to, + kind: :account_email_changed, + notification_type: 1, + deliver: false + ) + end + def sync_locked_at_from_locked return unless @locked_will_change diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb new file mode 100644 index 000000000..884277bae --- /dev/null +++ b/spec/mailers/devise_mailer_spec.rb @@ -0,0 +1,140 @@ +require "rails_helper" + +RSpec.describe DeviseMailer, type: :mailer do + let(:user) { create(:user, email: "user@example.com") } + let(:token) { "fake-token-123" } + + describe "#track_devise_email_event" do + context "when sending confirmation for initial signup" do + it "tracks auth.confirmation_email_sent" do + expect(Analytics::AhoyTracker).to receive(:track_auth_event).with( + "auth.confirmation_email_sent", + hash_including(record_id: user.id, record_type: "User"), + user: anything + ) + + described_class.confirmation_instructions(user, token).deliver_now + end + end + + context "when sending confirmation for email change (reconfirmation)" do + before do + user.skip_confirmation_notification! + user.update!(email: "new@example.com") + # Devise sets unconfirmed_email and keeps original email + end + + it "tracks auth.email_change_requested_email_sent instead of confirmation_email_sent" do + expect(Analytics::AhoyTracker).to receive(:track_auth_event).with( + "auth.email_change_requested_email_sent", + hash_including(record_id: user.id, record_type: "User"), + user: anything + ) + + described_class.confirmation_instructions(user, token).deliver_now + end + end + + context "when sending reset password instructions" do + it "tracks auth.reset_password_email_sent" do + expect(Analytics::AhoyTracker).to receive(:track_auth_event).with( + "auth.reset_password_email_sent", + hash_including(record_id: user.id, record_type: "User"), + user: anything + ) + + described_class.reset_password_instructions(user, token).deliver_now + end + end + + context "when sending unlock instructions" do + let(:user) { create(:user, :locked, email: "locked@example.com") } + + it "tracks auth.unlock_email_sent" do + expect(Analytics::AhoyTracker).to receive(:track_auth_event).with( + "auth.unlock_email_sent", + hash_including(record_id: user.id, record_type: "User"), + user: anything + ) + + described_class.unlock_instructions(user, token).deliver_now + end + end + end + + describe "#create_notification_record" do + let(:notification) { build(:notification) } + + before do + allow(Rails.env).to receive(:test?).and_return(false) + allow(NotificationServices::PersistDeliveredEmail).to receive(:call) + allow(NotificationServices::CreateNotification).to receive(:call).and_return(notification) + end + + context "when sending confirmation for initial signup" do + it "creates an account_confirmation notification" do + expect(NotificationServices::CreateNotification).to receive(:call).with( + hash_including( + kind: "account_confirmation", + recipient_role: :person, + recipient_email: user.email, + deliver: false + ) + ).and_return(notification) + + described_class.confirmation_instructions(user, token).deliver_now + end + end + + context "when sending confirmation for email change (reconfirmation)" do + before do + user.skip_confirmation_notification! + user.update!(email: "new@example.com") + end + + it "creates an account_email_change_requested notification" do + expect(NotificationServices::CreateNotification).to receive(:call).with( + hash_including( + kind: "account_email_change_requested", + recipient_role: :person, + recipient_email: user.unconfirmed_email, + deliver: false + ) + ).and_return(notification) + + described_class.confirmation_instructions(user, token).deliver_now + end + end + + context "when sending reset password instructions" do + it "creates a reset_password notification and a reset_password_fyi" do + expect(NotificationServices::CreateNotification).to receive(:call).with( + hash_including(kind: "reset_password", recipient_role: :person, deliver: false) + ).ordered.and_return(notification) + + expect(NotificationServices::CreateNotification).to receive(:call).with( + hash_including(kind: "reset_password_fyi", recipient_role: :admin, deliver: true) + ).ordered + + described_class.reset_password_instructions(user, token).deliver_now + end + end + + context "when sending unlock instructions" do + let(:user) { create(:user, :locked, email: "locked@example.com") } + + it "creates an account_unlock notification" do + expect(NotificationServices::CreateNotification).to receive(:call).with( + hash_including( + kind: "account_unlock", + recipient_role: :person, + recipient_email: user.email, + deliver: false + ) + ).and_return(notification) + + described_class.unlock_instructions(user, token).deliver_now + end + end + end +end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index c593548b7..720f78579 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -8,6 +8,26 @@ it { should have_many(:child_notifications).class_name('Notification').with_foreign_key(:parent_notification_id) } end + describe "KINDS" do + it "includes account_email_change_requested" do + expect(Notification::KINDS).to include("account_email_change_requested") + end + + it "includes account_email_changed" do + expect(Notification::KINDS).to include("account_email_changed") + end + + it "validates account_email_change_requested as a valid kind" do + notification = build(:notification, kind: "account_email_change_requested", recipient_role: "person") + expect(notification).to be_valid + end + + it "validates account_email_changed as a valid kind" do + notification = build(:notification, kind: "account_email_changed", recipient_role: "person") + expect(notification).to be_valid + end + end + describe "#resendable?" do it "returns true for notification kinds handled by the mailer job" do notification = build(:notification, kind: "reset_password_fyi") diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b57c8ce3b..658ca7c54 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -336,4 +336,79 @@ expect(results).not_to include(regular_user, inactive_user) end end + + describe "#track_password_changed" do + let(:user) { create(:user, password: "original_password") } + + it "tracks auth.password_changed when encrypted_password changes" do + expect(Analytics::LifecycleBuffer).to receive(:push).with( + hash_including(name: "auth.password_changed") + ) + + user.update!(password: "new_secure_password", password_confirmation: "new_secure_password") + end + + it "does not track when other fields change" do + expect(Analytics::LifecycleBuffer).not_to receive(:push).with( + hash_including(name: "auth.password_changed") + ) + + user.update!(first_name: "Updated") + end + end + + describe "#create_email_changed_notification" do + let(:user) { create(:user, email: "old@example.com") } + + it "creates an account_email_changed notification when email changes" do + user.skip_reconfirmation! + + expect { + user.update!(email: "changed@example.com") + }.to change(Notification, :count).by(1) + + notification = Notification.last + expect(notification.kind).to eq("account_email_changed") + expect(notification.recipient_role).to eq("person") + expect(notification.recipient_email).to eq("changed@example.com") + expect(notification.noticeable).to eq(user) + expect(notification.delivered_at).to be_nil + end + + it "does not create notification when email does not change" do + expect { + user.update!(first_name: "Updated") + }.not_to change(Notification, :count) + end + end + + describe "#track_email_change" do + let(:user) { create(:user, email: "old@example.com") } + + it "tracks auth.email_changed when email is confirmed" do + user.skip_reconfirmation! + + expect(Analytics::LifecycleBuffer).to receive(:push).with( + hash_including( + name: "auth.email_changed", + properties: hash_including(changes: { email: { before: "old@example.com", after: "new@example.com" } }) + ) + ) + + user.update!(email: "new@example.com") + end + + it "tracks auth.email_update_requested when unconfirmed_email is set" do + expect(Analytics::LifecycleBuffer).to receive(:push).with( + hash_including( + name: "auth.email_update_requested", + properties: hash_including(changes: { email: { before: "old@example.com", after: "pending@example.com" } }) + ) + ) + + user.email = "pending@example.com" + user.skip_confirmation_notification! + user.save! + end + end end From 38e2b7936a2d777a33c58fff7e450b9659918b56 Mon Sep 17 00:00:00 2001 From: maebeale Date: Mon, 2 Mar 2026 20:08:18 -0500 Subject: [PATCH 2/2] Remove unlock_instructions tests (no unlock route) App uses unlock_strategy: :none so user_unlock_url doesn't exist and the Devise unlock view can't render in tests. Co-Authored-By: Claude Opus 4.6 --- spec/mailers/devise_mailer_spec.rb | 33 ++++-------------------------- 1 file changed, 4 insertions(+), 29 deletions(-) diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb index 884277bae..744258819 100644 --- a/spec/mailers/devise_mailer_spec.rb +++ b/spec/mailers/devise_mailer_spec.rb @@ -47,19 +47,8 @@ end end - context "when sending unlock instructions" do - let(:user) { create(:user, :locked, email: "locked@example.com") } - - it "tracks auth.unlock_email_sent" do - expect(Analytics::AhoyTracker).to receive(:track_auth_event).with( - "auth.unlock_email_sent", - hash_including(record_id: user.id, record_type: "User"), - user: anything - ) - - described_class.unlock_instructions(user, token).deliver_now - end - end + # unlock_instructions not tested here — app uses unlock_strategy: :none + # so user_unlock_url route doesn't exist and the view can't render end describe "#create_notification_record" do @@ -120,21 +109,7 @@ end end - context "when sending unlock instructions" do - let(:user) { create(:user, :locked, email: "locked@example.com") } - - it "creates an account_unlock notification" do - expect(NotificationServices::CreateNotification).to receive(:call).with( - hash_including( - kind: "account_unlock", - recipient_role: :person, - recipient_email: user.email, - deliver: false - ) - ).and_return(notification) - - described_class.unlock_instructions(user, token).deliver_now - end - end + # unlock_instructions not tested here — app uses unlock_strategy: :none + # so user_unlock_url route doesn't exist and the view can't render end end