From dcb648ae57d405f17df3d88b6c96856db666b879 Mon Sep 17 00:00:00 2001 From: maebeale Date: Tue, 4 Aug 2026 01:46:52 -0400 Subject: [PATCH 1/7] Attribute invite emails to the sending person MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Invitation emails (both the single Invite button and the bulk console tool) were recorded with no sender, so the notifications UI showed them as "From: AWBW Portal" — anonymous. Attribute them to a real person. DeviseMailer now records Current.user as the notification sender. The Invite button already runs in a request where Current.user is set, so it gets this for free. The bulk path runs in the console with no request, so BulkInviteService takes a sender: and threads it through the job, which sets Current.user before sending. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/jobs/bulk_invite_email_job.rb | 3 +- app/mailers/devise_mailer.rb | 1 + app/services/bulk_invite_service.rb | 17 +++++---- .../create_notification.rb | 4 ++- spec/jobs/bulk_invite_email_job_spec.rb | 36 +++++++++++++++++++ spec/services/bulk_invite_service_spec.rb | 17 +++++++++ 6 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 spec/jobs/bulk_invite_email_job_spec.rb diff --git a/app/jobs/bulk_invite_email_job.rb b/app/jobs/bulk_invite_email_job.rb index 7e038a7cae..f97b1f3c8e 100644 --- a/app/jobs/bulk_invite_email_job.rb +++ b/app/jobs/bulk_invite_email_job.rb @@ -1,8 +1,9 @@ class BulkInviteEmailJob < ApplicationJob queue_as :default - def perform(user_id) + def perform(user_id, sender_id: nil) user = User.find(user_id) + Current.user = User.find_by(id: sender_id) if sender_id user.send_confirmation_instructions end end diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index e74dd57b40..d3fc8758a9 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -82,6 +82,7 @@ def create_notification_record recipient_email: recipient_email, kind: kind, notification_type: 1, + sender: Current.user, # attribute to the operator when one is set (e.g. bulk invites) deliver: false # Devise already sent the email, so no need to deliver via the job ) diff --git a/app/services/bulk_invite_service.rb b/app/services/bulk_invite_service.rb index b15793fbd2..9280ce5ddb 100644 --- a/app/services/bulk_invite_service.rb +++ b/app/services/bulk_invite_service.rb @@ -1,14 +1,17 @@ class BulkInviteService - attr_reader :ids, :dry_run, :results + attr_reader :ids, :dry_run, :sender, :results - # BulkInviteService.call(ids: [1, 2, 3]) + # sender is the person running the invite; bulk invites are attributed to them + # on the notification (and the ahoy event). Pass a User, e.g. the operator: + # BulkInviteService.call(ids: [1, 2, 3], sender: User.find_by(email: "you@awbw.org")) # BulkInviteService.call(ids: [1, 2, 3], dry_run: true) - def self.call(ids:, dry_run: false) - new(ids: ids, dry_run: dry_run).call + def self.call(ids:, sender: nil, dry_run: false) + new(ids: ids, sender: sender, dry_run: dry_run).call end - def initialize(ids:, dry_run: false) + def initialize(ids:, sender: nil, dry_run: false) @ids = Array(ids).map(&:to_i) + @sender = sender @dry_run = dry_run @results = if dry_run { dry_run_would_send_ids: [], missing_ids: [], already_confirmed_ids: [] } @@ -40,6 +43,8 @@ def call return results end + log sender ? "Attributing invites to #{sender.name} <#{sender.email}>" : "Warning: no sender — invites will show as sent by AWBW Portal." + log "Found #{unconfirmed.size} unconfirmed users:" unconfirmed.each { |u| log " #{u.id}: #{u.name} <#{u.email}>" } @@ -68,7 +73,7 @@ def invite_user(user, index, total) user.update!(welcome_instructions_sent_at: Time.current, created_at: nil) end - BulkInviteEmailJob.perform_later(user.id) + BulkInviteEmailJob.perform_later(user.id, sender_id: sender&.id) results[:sent_ids] << user.id log " Invited #{user.email} (#{index}/#{total})" rescue => e diff --git a/app/services/notification_services/create_notification.rb b/app/services/notification_services/create_notification.rb index 614460e840..8b65cf4e4e 100644 --- a/app/services/notification_services/create_notification.rb +++ b/app/services/notification_services/create_notification.rb @@ -8,6 +8,7 @@ def self.call( notification_type:, custom_message: nil, custom_subject: nil, + sender: nil, deliver: true, persist_delivered_email: true ) @@ -19,7 +20,8 @@ def self.call( recipient_role: recipient_role.to_s, recipient_email: recipient_email, custom_message: custom_message, - custom_subject: custom_subject + custom_subject: custom_subject, + sender: sender ) Rails.logger.info({ event: "notification.created", diff --git a/spec/jobs/bulk_invite_email_job_spec.rb b/spec/jobs/bulk_invite_email_job_spec.rb new file mode 100644 index 0000000000..b05b6bb8a4 --- /dev/null +++ b/spec/jobs/bulk_invite_email_job_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe BulkInviteEmailJob do + after { Current.reset } + + it "sends confirmation instructions to the user" do + user = create(:user, :unconfirmed) + + expect_any_instance_of(User).to receive(:send_confirmation_instructions) + + described_class.perform_now(user.id) + end + + it "sets Current.user to the sender so the invite is attributed to them" do + user = create(:user, :unconfirmed) + sender = create(:user) + + allow_any_instance_of(User).to receive(:send_confirmation_instructions) do |record| + expect(Current.user).to eq(sender) if record == user + end + + described_class.perform_now(user.id, sender_id: sender.id) + end + + it "leaves Current.user unset when no sender is given" do + user = create(:user, :unconfirmed) + + allow_any_instance_of(User).to receive(:send_confirmation_instructions) do |record| + expect(Current.user).to be_nil if record == user + end + + described_class.perform_now(user.id) + end +end diff --git a/spec/services/bulk_invite_service_spec.rb b/spec/services/bulk_invite_service_spec.rb index 23045fa366..7ceb3b1456 100644 --- a/spec/services/bulk_invite_service_spec.rb +++ b/spec/services/bulk_invite_service_spec.rb @@ -80,6 +80,23 @@ expect(results[:sent_ids]).to eq([ user.id ]) end + + it "threads the sender through to the job for attribution" do + user = create(:user, :unconfirmed) + sender = create(:user) + + expect { + described_class.call(ids: [ user.id ], sender: sender) + }.to have_enqueued_job(BulkInviteEmailJob).with(user.id, sender_id: sender.id) + end + + it "enqueues with a nil sender_id when no sender is given" do + user = create(:user, :unconfirmed) + + expect { + described_class.call(ids: [ user.id ]) + }.to have_enqueued_job(BulkInviteEmailJob).with(user.id, sender_id: nil) + end end context "with already confirmed users" do From 287f1d03027eedd123f500ed46b6aa19b80a88aa Mon Sep 17 00:00:00 2001 From: maebeale Date: Tue, 4 Aug 2026 09:32:16 -0400 Subject: [PATCH 2/7] Attribute admin-sent reminders/resends to the sender; show From MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Event reminders sent by hand from the bulk reminders page, and resent notifications, were created with no sender — so the communications index and show page labeled them "AWBW Portal" as if the portal sent them automatically. Pass the acting admin as the sender on both paths, and add a From row to the notification show page (person's name when a staff member sent it, "AWBW Portal" only for truly automated messages). Co-Authored-By: Claude Opus 4.8 (1M context) --- app/controllers/events_controller.rb | 1 + app/controllers/notifications_controller.rb | 1 + app/views/notifications/show.html.erb | 8 +++++++ spec/requests/events/bulk_reminders_spec.rb | 11 +++++++++ spec/requests/notifications_spec.rb | 26 +++++++++++++++++++++ 5 files changed, 47 insertions(+) diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index f90026197a..f22033f3e1 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -322,6 +322,7 @@ def send_reminder recipient_role: :person, recipient_email: event_registration.registrant.preferred_email, notification_type: 0, + sender: current_user, # an admin sent these by hand from the reminders page custom_message: custom_message.presence, custom_subject: custom_subject.presence ) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 1d332c855b..20a968d1c8 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -71,6 +71,7 @@ def resend recipient_email: @notification.recipient_email, recipient_role: @notification.recipient_role, notification_type: @notification.notification_type, + sender: current_user, # a resend is an admin action — attribute it to them deliver: true, persist_delivered_email: true ) diff --git a/app/views/notifications/show.html.erb b/app/views/notifications/show.html.erb index fc7381cac4..057f590968 100644 --- a/app/views/notifications/show.html.erb +++ b/app/views/notifications/show.html.erb @@ -104,6 +104,14 @@ +
+
From
+
+ <%# A person only when a staff member sent it; otherwise the portal sent it automatically. %> + <%= @notification.sender&.full_name.presence || "AWBW Portal" %> +
+
+
Subject
diff --git a/spec/requests/events/bulk_reminders_spec.rb b/spec/requests/events/bulk_reminders_spec.rb index 37126a731b..887bfefab9 100644 --- a/spec/requests/events/bulk_reminders_spec.rb +++ b/spec/requests/events/bulk_reminders_spec.rb @@ -92,5 +92,16 @@ def checked?(body, registration) expect(response).to redirect_to(registrants_event_path(event)) end + + it "attributes each reminder to the admin who sent it, not the portal" do + post send_reminder_event_path(event), params: { registration_ids: [ jane.id, sam.id ] } + + reminders = Notification.where(kind: "event_registration_reminder") + expect(reminders.count).to eq(2) + expect(reminders.map(&:sender)).to all(eq(admin)) + # Guards the "FROM: AWBW Portal" regression — a sent-by-hand reminder must + # carry a sender so the index/show page name the admin. + expect(reminders.map(&:sender_id)).not_to include(nil) + end end end diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index 17b25362bf..5847174e6a 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -249,6 +249,26 @@ expect(response.body).not_to match(/]*name="notification\[responded\]"/) end + + it "names the sending person in the From row when a sender is set" do + sender = create(:user, :admin, first_name: "Dana", last_name: "Sender") + sent = create(:notification, kind: "event_registration_reminder", sender: sender) + + get notification_path(sent) + + expect(response.body).to include("From") + expect(response.body).to include("Dana Sender") + expect(response.body).not_to include("AWBW Portal") + end + + it "shows AWBW Portal in the From row for automated messages with no sender" do + automated = create(:notification, kind: "account_confirmation", sender: nil) + + get notification_path(automated) + + expect(response.body).to include("From") + expect(response.body).to include("AWBW Portal") + end end context "as a non-admin owner" do @@ -329,6 +349,12 @@ expect(new_notification.recipient_email).to eq(notification.recipient_email) end + it "attributes the resent copy to the admin who resent it" do + post resend_notification_path(notification.id) + + expect(Notification.last.sender).to eq(admin) + end + it "tracks resend chain correctly when resending a resent notification" do # Create first resend first_resend = nil From cd3869e689566a06a661ed3a3ce60c423abdde89 Mon Sep 17 00:00:00 2001 From: maebeale Date: Tue, 4 Aug 2026 10:09:47 -0400 Subject: [PATCH 3/7] Pass the invite sender explicitly instead of through Current.user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Threading the sender through the global Current.user meant a background job was mutating request-scoped state to talk to the mailer. It also silently un-gated AhoyTrackable's lifecycle tracking (which keys off Current.user): send_confirmation_instructions saves the record, so every invite pushed an event onto LifecycleBuffer, a thread-local that only ApplicationController ever flushes — in a job those piled up unflushed and undelivered. Devise sends with deliver_now, so the mailer holds the same User instance the caller does; the sender can just ride along on the record. The four admin-initiated call sites that were relying on ApplicationController setting Current.user now pass current_user explicitly, so they keep their attribution. Also collapses the "sender name, else AWBW Portal" fallback into NotificationDecorator#sender_name — the index was rendering a lowercase "AWBW portal" while the row partial and detail page said "AWBW Portal". Co-Authored-By: Claude --- app/controllers/users_controller.rb | 2 +- app/decorators/notification_decorator.rb | 7 ++++++ app/jobs/bulk_invite_email_job.rb | 4 ++-- app/mailers/devise_mailer.rb | 4 ++-- app/models/user.rb | 8 ++++++- .../process_confirmation.rb | 2 +- .../user_services/process_email_change.rb | 2 +- .../process_email_manual_confirm.rb | 2 +- app/views/notifications/_index.html.erb | 2 +- .../notifications/_notification_row.html.erb | 2 +- app/views/notifications/show.html.erb | 3 +-- .../decorators/notification_decorator_spec.rb | 12 ++++++++++ spec/jobs/bulk_invite_email_job_spec.rb | 18 +++++---------- spec/models/user_spec.rb | 22 +++++++++++++++++++ spec/requests/notifications_spec.rb | 13 ++++++----- 15 files changed, 73 insertions(+), 30 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 574ad17dc2..beb31a2175 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -305,7 +305,7 @@ def send_welcome_instructions @user.updated_by = current_user @user.set_welcome_instructions_token! @user.update(welcome_instructions_sent_at: Time.current, welcome_instructions_sent_by: current_user) - @user.send_confirmation_instructions + @user.send_confirmation_instructions(sender: current_user) redirect_to users_path(search: params[:search], super_user: params[:super_user], diff --git a/app/decorators/notification_decorator.rb b/app/decorators/notification_decorator.rb index 13ba456183..7e4029d768 100644 --- a/app/decorators/notification_decorator.rb +++ b/app/decorators/notification_decorator.rb @@ -10,6 +10,13 @@ class NotificationDecorator < ApplicationDecorator "video" => "fa-video" }.freeze + # Shown as the "From" on a communication that no staff member sent by hand. + PORTAL_SENDER_NAME = "AWBW Portal".freeze + + def sender_name + sender&.full_name.presence || PORTAL_SENDER_NAME + end + def title "Re #{noticeable_type} ##{noticeable_id}" end diff --git a/app/jobs/bulk_invite_email_job.rb b/app/jobs/bulk_invite_email_job.rb index f97b1f3c8e..58b4bbd7ce 100644 --- a/app/jobs/bulk_invite_email_job.rb +++ b/app/jobs/bulk_invite_email_job.rb @@ -3,7 +3,7 @@ class BulkInviteEmailJob < ApplicationJob def perform(user_id, sender_id: nil) user = User.find(user_id) - Current.user = User.find_by(id: sender_id) if sender_id - user.send_confirmation_instructions + sender = User.find_by(id: sender_id) if sender_id + user.send_confirmation_instructions(sender: sender) end end diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index d3fc8758a9..35401d594d 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -82,7 +82,7 @@ def create_notification_record recipient_email: recipient_email, kind: kind, notification_type: 1, - sender: Current.user, # attribute to the operator when one is set (e.g. bulk invites) + sender: @record.try(:confirmation_sender), # the staff member who triggered it, when one did deliver: false # Devise already sent the email, so no need to deliver via the job ) @@ -135,7 +135,7 @@ def track_devise_email_event Analytics::AhoyTracker.track_auth_event( event_name, properties, - user: Current.user + user: @record.confirmation_sender || Current.user ) end end diff --git a/app/models/user.rb b/app/models/user.rb index ff026317e7..21ed16edb0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -208,11 +208,17 @@ def gallery_assets # method needed for idea_submitted_fyi mailer [] end + # The staff member who triggered this confirmation email, when one did. Not + # persisted — DeviseMailer reads it off the record to attribute the + # notification it logs, since it has no request and no current_user. + attr_reader :confirmation_sender + # Override Devise to always send confirmation to the pending email when present. # Devise's default checks `pending_reconfirmation?` but can still route to the # current email in some flows. This ensures the confirmation always targets the # unconfirmed (new) email address. - def send_confirmation_instructions + def send_confirmation_instructions(sender: nil) + @confirmation_sender = sender generate_confirmation_token! unless @raw_confirmation_token target = unconfirmed_email.presence || email send_devise_notification(:confirmation_instructions, @raw_confirmation_token, to: target) diff --git a/app/services/event_registration_services/process_confirmation.rb b/app/services/event_registration_services/process_confirmation.rb index 0838c80941..533130b3ba 100644 --- a/app/services/event_registration_services/process_confirmation.rb +++ b/app/services/event_registration_services/process_confirmation.rb @@ -73,7 +73,7 @@ def send_welcome_instructions user.updated_by = @current_user user.set_welcome_instructions_token! user.update!(welcome_instructions_sent_at: Time.current, welcome_instructions_sent_by: @current_user) - user.send_confirmation_instructions + user.send_confirmation_instructions(sender: @current_user) @actions_taken << "System invite sent" end diff --git a/app/services/user_services/process_email_change.rb b/app/services/user_services/process_email_change.rb index 56297a20d6..fb43fa3b90 100644 --- a/app/services/user_services/process_email_change.rb +++ b/app/services/user_services/process_email_change.rb @@ -32,7 +32,7 @@ def send_confirmation_email # Credit the acting admin. Devise saves only when it regenerates the token, # so persist the attribution ourselves if it's left dirty. @user.updated_by = @current_user - @user.send_confirmation_instructions + @user.send_confirmation_instructions(sender: @current_user) @user.save(validate: false) if @user.changed? @actions_taken << "A confirmation email has been sent to #{@user.unconfirmed_email}" end diff --git a/app/services/user_services/process_email_manual_confirm.rb b/app/services/user_services/process_email_manual_confirm.rb index 9bf46b65e9..f078384bd1 100644 --- a/app/services/user_services/process_email_manual_confirm.rb +++ b/app/services/user_services/process_email_manual_confirm.rb @@ -35,7 +35,7 @@ def resend_confirmation # Credit the acting admin. Devise saves only when it regenerates the token, # so persist the attribution ourselves if it's left dirty. @user.updated_by = @current_user - @user.send_confirmation_instructions + @user.send_confirmation_instructions(sender: @current_user) @user.save(validate: false) if @user.changed? @actions_taken << "Confirmation email has been resent to #{target_email}" end diff --git a/app/views/notifications/_index.html.erb b/app/views/notifications/_index.html.erb index 8e91e71c6e..8bccac0a14 100644 --- a/app/views/notifications/_index.html.erb +++ b/app/views/notifications/_index.html.erb @@ -33,7 +33,7 @@
To: <%= notification.recipient_email %>
-
From: <%= notification.sender&.full_name.presence || "AWBW portal" %>
+
From: <%= notification.decorate.sender_name %>
diff --git a/app/views/notifications/_notification_row.html.erb b/app/views/notifications/_notification_row.html.erb index 8c7abb1dac..c15901099c 100644 --- a/app/views/notifications/_notification_row.html.erb +++ b/app/views/notifications/_notification_row.html.erb @@ -13,7 +13,7 @@ <% show_body = admin || !body_admin_only %> <% subject = notification.email_subject.presence || notification.kind.to_s.humanize %> <% body = notification.email_body_text.to_s if show_body %> -<% sender_name = notification.sender&.full_name.presence || "AWBW Portal" %> +<% sender_name = notification.decorate.sender_name %>
<%= notification.created_at.strftime("%-m/%-d/%Y") %> <%# Fixed, snug width (~"Umberto User") + truncate so the channel icons line up diff --git a/app/views/notifications/show.html.erb b/app/views/notifications/show.html.erb index 057f590968..adb0a26744 100644 --- a/app/views/notifications/show.html.erb +++ b/app/views/notifications/show.html.erb @@ -107,8 +107,7 @@
From
- <%# A person only when a staff member sent it; otherwise the portal sent it automatically. %> - <%= @notification.sender&.full_name.presence || "AWBW Portal" %> + <%= @notification.decorate.sender_name %>
diff --git a/spec/decorators/notification_decorator_spec.rb b/spec/decorators/notification_decorator_spec.rb index 973446588c..616e42408c 100644 --- a/spec/decorators/notification_decorator_spec.rb +++ b/spec/decorators/notification_decorator_spec.rb @@ -1,6 +1,18 @@ require "rails_helper" RSpec.describe NotificationDecorator, type: :decorator do + describe "#sender_name" do + it "names the staff member who sent it" do + sender = build_stubbed(:user, first_name: "Dana", last_name: "Sender", person: nil) + + expect(build_stubbed(:notification, sender: sender).decorate.sender_name).to eq("Dana Sender") + end + + it "falls back to the portal when nobody sent it by hand" do + expect(build_stubbed(:notification, sender: nil).decorate.sender_name).to eq("AWBW Portal") + end + end + describe "#channel_icon" do { "email" => "fa-envelope", diff --git a/spec/jobs/bulk_invite_email_job_spec.rb b/spec/jobs/bulk_invite_email_job_spec.rb index b05b6bb8a4..d488d3cf98 100644 --- a/spec/jobs/bulk_invite_email_job_spec.rb +++ b/spec/jobs/bulk_invite_email_job_spec.rb @@ -3,34 +3,28 @@ require "rails_helper" RSpec.describe BulkInviteEmailJob do - after { Current.reset } - it "sends confirmation instructions to the user" do user = create(:user, :unconfirmed) - expect_any_instance_of(User).to receive(:send_confirmation_instructions) + expect_any_instance_of(User).to receive(:send_confirmation_instructions).with(sender: nil) described_class.perform_now(user.id) end - it "sets Current.user to the sender so the invite is attributed to them" do + it "passes the sender through so the invite is attributed to them" do user = create(:user, :unconfirmed) sender = create(:user) - allow_any_instance_of(User).to receive(:send_confirmation_instructions) do |record| - expect(Current.user).to eq(sender) if record == user - end + expect_any_instance_of(User).to receive(:send_confirmation_instructions).with(sender: sender) described_class.perform_now(user.id, sender_id: sender.id) end - it "leaves Current.user unset when no sender is given" do + it "sends with no sender when the sender no longer exists" do user = create(:user, :unconfirmed) - allow_any_instance_of(User).to receive(:send_confirmation_instructions) do |record| - expect(Current.user).to be_nil if record == user - end + expect_any_instance_of(User).to receive(:send_confirmation_instructions).with(sender: nil) - described_class.perform_now(user.id) + described_class.perform_now(user.id, sender_id: -1) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5d85c58635..42a2fcdbca 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -432,5 +432,27 @@ .with(user, anything, hash_including(to: user.email)) end end + + context "when a sender is given" do + let(:user) { create(:user, confirmed_at: nil) } + let(:sender) { create(:user) } + + before do + user + allow(DeviseMailer).to receive(:confirmation_instructions).and_return(mock_mail) + end + + it "exposes it as confirmation_sender for the mailer to attribute" do + user.send_confirmation_instructions(sender: sender) + + expect(user.confirmation_sender).to eq(sender) + end + + it "leaves confirmation_sender unset when none is given" do + user.send_confirmation_instructions + + expect(user.confirmation_sender).to be_nil + end + end end end diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb index 5847174e6a..3e3fdbe8e7 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -250,15 +250,19 @@ expect(response.body).not_to match(/]*name="notification\[responded\]"/) end + # Scoped to the From row's
so an unrelated mention of the sender or of + # "AWBW Portal" elsewhere on the page can't satisfy (or break) the assertion. + def from_row(body) + Capybara.string(body).find(:xpath, "//dt[normalize-space()='From']/following-sibling::dd[1]") + end + it "names the sending person in the From row when a sender is set" do sender = create(:user, :admin, first_name: "Dana", last_name: "Sender") sent = create(:notification, kind: "event_registration_reminder", sender: sender) get notification_path(sent) - expect(response.body).to include("From") - expect(response.body).to include("Dana Sender") - expect(response.body).not_to include("AWBW Portal") + expect(from_row(response.body)).to have_text("Dana Sender") end it "shows AWBW Portal in the From row for automated messages with no sender" do @@ -266,8 +270,7 @@ get notification_path(automated) - expect(response.body).to include("From") - expect(response.body).to include("AWBW Portal") + expect(from_row(response.body)).to have_text("AWBW Portal") end end From 860212ba8af3db699617d71af10cfdff91a97e6a Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 9 Aug 2026 15:15:26 -0400 Subject: [PATCH 4/7] Cover the sender attribution that DeviseMailer skips in test env create_notification_record short-circuits on Rails.env.test?, so nothing was verifying that a hand-sent invite actually lands on the notification as its sender. Also pin the inverse: templated confirmations stay portal. --- spec/jobs/bulk_invite_email_job_spec.rb | 13 +++++ spec/mailers/devise_mailer_spec.rb | 48 +++++++++++++++++++ .../process_confirmation_spec.rb | 18 +++++++ 3 files changed, 79 insertions(+) diff --git a/spec/jobs/bulk_invite_email_job_spec.rb b/spec/jobs/bulk_invite_email_job_spec.rb index d488d3cf98..dceeddbad4 100644 --- a/spec/jobs/bulk_invite_email_job_spec.rb +++ b/spec/jobs/bulk_invite_email_job_spec.rb @@ -27,4 +27,17 @@ described_class.perform_now(user.id, sender_id: -1) end + + # End-to-end through DeviseMailer, which is where the attribution actually lands. + # It skips logging in the test env, so unstub that after the factories have run. + it "logs the invite notification as sent by the sender, not the portal" do + user = create(:user, :unconfirmed) + sender = create(:user, first_name: "Dana", last_name: "Sender") + allow(Rails.env).to receive(:test?).and_return(false) + + described_class.perform_now(user.id, sender_id: sender.id) + + expect(Notification.last.sender).to eq(sender) + expect(Notification.last.decorate.sender_name).to eq("Dana Sender") + end end diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb index c2c7ec83bd..cf4e829431 100644 --- a/spec/mailers/devise_mailer_spec.rb +++ b/spec/mailers/devise_mailer_spec.rb @@ -156,4 +156,52 @@ # 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 + + # Exercises the real CreateNotification (no stub) so these cover the persisted + # Notification and the "From" name the communications pages render. Rails.env.test? + # is stubbed per-example rather than in a before block so the user factories — + # which fire Devise's on-create confirmation — don't log notifications of their own. + describe "sender attribution" do + let(:admin) { create(:user, first_name: "Dana", last_name: "Sender") } + + def unstub_notification_logging + allow(Rails.env).to receive(:test?).and_return(false) + end + + it "attributes an admin-sent invite to the admin" do + invitee = create(:user, :unconfirmed) + admin + unstub_notification_logging + + expect { + invitee.send_confirmation_instructions(sender: admin) + }.to change(Notification, :count).by(1) + + expect(Notification.last.sender).to eq(admin) + expect(Notification.last.decorate.sender_name).to eq("Dana Sender") + end + + it "leaves an automated confirmation as the portal" do + signup = create(:user, :unconfirmed) + unstub_notification_logging + + expect { + signup.send_confirmation_instructions + }.to change(Notification, :count).by(1) + + expect(Notification.last.sender).to be_nil + expect(Notification.last.decorate.sender_name).to eq(NotificationDecorator::PORTAL_SENDER_NAME) + end + + it "leaves an automated password reset as the portal" do + user + unstub_notification_logging + + user.send_reset_password_instructions + + reset = Notification.where(kind: "reset_password").last + expect(reset.sender).to be_nil + expect(reset.decorate.sender_name).to eq(NotificationDecorator::PORTAL_SENDER_NAME) + end + end end diff --git a/spec/services/event_registration_services/process_confirmation_spec.rb b/spec/services/event_registration_services/process_confirmation_spec.rb index 4868322413..6ffb8772ff 100644 --- a/spec/services/event_registration_services/process_confirmation_spec.rb +++ b/spec/services/event_registration_services/process_confirmation_spec.rb @@ -152,6 +152,24 @@ current_user: admin ) end + + # Templated confirmations are automated even though an admin ticks the box — + # only hand-written sends (invites, bulk reminders, resends) name a person. + it "leaves the confirmation attributed to the portal, not the admin" do + described_class.call( + event_registration: registration, + person: person, + create_user: false, + send_invite: false, + send_confirmation_email: true, + send_admin_fyi: false, + current_user: admin + ) + + confirmation = Notification.where(kind: "event_registration_confirmation").last + expect(confirmation.sender).to be_nil + expect(confirmation.decorate.sender_name).to eq(NotificationDecorator::PORTAL_SENDER_NAME) + end end context "send_admin_fyi" do From 99740937e74cb8be4aa662b4f4986f2ee7b6b0e7 Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 9 Aug 2026 15:25:21 -0400 Subject: [PATCH 5/7] Show the sending admin's name in the From header, not their address SendGrid only authorizes our own domain, so attribution rides in the display name while the address and reply_to stay on the generic mailbox. Driven off notification.sender, so automated mail stays unnamed. --- AGENTS.md | 1 + app/jobs/notification_mailer_job.rb | 4 ++ app/mailers/devise_mailer.rb | 2 + app/services/attributed_from_address.rb | 14 +++++ spec/jobs/notification_mailer_job_spec.rb | 39 +++++++++++++ spec/mailers/devise_mailer_spec.rb | 23 ++++++++ spec/services/attributed_from_address_spec.rb | 56 +++++++++++++++++++ 7 files changed, 139 insertions(+) create mode 100644 app/services/attributed_from_address.rb create mode 100644 spec/services/attributed_from_address_spec.rb diff --git a/AGENTS.md b/AGENTS.md index 8cfd259da1..1fe9b9f2c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -205,6 +205,7 @@ action, or `authorize! :workshop, to: :summary?`). - `TaggingSearchService` — Search and filter tagging data - `PersonFromUserService` — Create Person from User account - `BulkInviteService` — Bulk send welcome instructions and reset created_at for users +- `AttributedFromAddress` — Builds the From header for admin-sent mail: the notification sender's name becomes the display name while the address stays on the SendGrid-authorized generic mailbox (used by `NotificationMailerJob` and `DeviseMailer`) - `FormBuilderService` — Builds configurable forms from composable sections with per-field visibility - `ModelDeduper` — Deduplication logic - `RichTextMigrator` — Rich text migration utility diff --git a/app/jobs/notification_mailer_job.rb b/app/jobs/notification_mailer_job.rb index 298afb5c2d..ab67581f32 100644 --- a/app/jobs/notification_mailer_job.rb +++ b/app/jobs/notification_mailer_job.rb @@ -23,6 +23,10 @@ def perform(notification_id, persist_delivered_email: true) mailer = mailer_map[notification.kind]&.call(notification) raise "Unknown notification kind: #{notification.kind}" unless mailer + # Name the admin who sent it in the From display name, keeping the generic + # address (and reply_to) each mailer already set. No-op when sender is nil. + mailer.message.from = AttributedFromAddress.call(notification.sender, mailer.message.from&.first) + Notification.transaction do notification.lock! return if notification.delivered_at.present? diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index 35401d594d..12d0a13554 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -29,6 +29,8 @@ def confirmation_instructions(record, token, opts = {}) else "AWBW Portal: Welcome instructions for #{record.full_name}" end + # Invites sent by hand carry the staff member's name; the address stays generic. + opts[:from] = AttributedFromAddress.call(record.try(:confirmation_sender), ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org")) @mail = super end diff --git a/app/services/attributed_from_address.rb b/app/services/attributed_from_address.rb new file mode 100644 index 0000000000..0f29772aae --- /dev/null +++ b/app/services/attributed_from_address.rb @@ -0,0 +1,14 @@ +class AttributedFromAddress + # Builds the From header for mail an admin sends by hand. SendGrid only authorizes + # our own domain, so the address stays generic and only the display name names the + # sender — "Dana Sender ". Returns the address untouched when + # there's no sender, so automated mail keeps going out as the portal. + def self.call(sender, address) + return address if address.blank? + + name = sender&.full_name.to_s.gsub(/[[:cntrl:]]/, " ").squish.presence + return address unless name + + Mail::Address.new(address).tap { |a| a.display_name = name }.format + end +end diff --git a/spec/jobs/notification_mailer_job_spec.rb b/spec/jobs/notification_mailer_job_spec.rb index 2f87bddbcd..d828efae78 100644 --- a/spec/jobs/notification_mailer_job_spec.rb +++ b/spec/jobs/notification_mailer_job_spec.rb @@ -73,5 +73,44 @@ expect(ActionMailer::Base.deliveries.last.subject).to eq("Don't forget us tomorrow!") end end + + # We only have SendGrid authorization for our own domain, so an attributed + # reminder may name the admin in the display name but never in the address. + context "sender attribution on the From header" do + let(:event_registration) { create(:event_registration) } + let(:admin) { create(:user, first_name: "Dana", last_name: "Sender") } + + def reminder_for(sender) + create(:notification, + kind: "event_registration_reminder", + noticeable: event_registration, + recipient_role: "person", + recipient_email: event_registration.registrant.preferred_email, + sender: sender) + end + + it "names the sending admin in the display name only" do + described_class.new.perform(reminder_for(admin).id) + + mail = ActionMailer::Base.deliveries.last + expect(mail[:from].display_names).to eq([ "Dana Sender" ]) + expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "no-reply@awbw.org") ]) + end + + it "leaves reply_to on the generic address so replies come back to us" do + described_class.new.perform(reminder_for(admin).id) + + expect(ActionMailer::Base.deliveries.last.reply_to) + .to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + end + + it "sends automated mail with no display name" do + described_class.new.perform(reminder_for(nil).id) + + mail = ActionMailer::Base.deliveries.last + expect(mail[:from].display_names.compact).to be_empty + expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "no-reply@awbw.org") ]) + end + end end end diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb index cf4e829431..18c9407da3 100644 --- a/spec/mailers/devise_mailer_spec.rb +++ b/spec/mailers/devise_mailer_spec.rb @@ -193,6 +193,29 @@ def unstub_notification_logging expect(Notification.last.decorate.sender_name).to eq(NotificationDecorator::PORTAL_SENDER_NAME) end + # We only have SendGrid authorization for our own domain, so the sender's name + # may appear in the From display name but never in the address. + it "names the sending admin in the invite's From display name only" do + invitee = create(:user, :unconfirmed) + + invitee.send_confirmation_instructions(sender: admin) + + mail = ActionMailer::Base.deliveries.last + expect(mail[:from].display_names).to eq([ "Dana Sender" ]) + expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + expect(mail.reply_to).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + end + + it "sends an automated confirmation with no display name" do + signup = create(:user, :unconfirmed) + + signup.send_confirmation_instructions + + mail = ActionMailer::Base.deliveries.last + expect(mail[:from].display_names.compact).to be_empty + expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + end + it "leaves an automated password reset as the portal" do user unstub_notification_logging diff --git a/spec/services/attributed_from_address_spec.rb b/spec/services/attributed_from_address_spec.rb new file mode 100644 index 0000000000..8f5ac4503f --- /dev/null +++ b/spec/services/attributed_from_address_spec.rb @@ -0,0 +1,56 @@ +require "rails_helper" + +RSpec.describe AttributedFromAddress do + let(:generic) { "programs@awbw.org" } + + describe ".call" do + it "keeps the generic address and only sets the display name" do + sender = build_stubbed(:user, first_name: "Dana", last_name: "Sender", person: nil) + + result = described_class.call(sender, generic) + + expect(result).to eq("Dana Sender ") + expect(Mail::Address.new(result).address).to eq(generic) + end + + it "returns the address untouched for automated mail with no sender" do + expect(described_class.call(nil, generic)).to eq(generic) + end + + it "replaces a display name the address already carried" do + sender = build_stubbed(:user, first_name: "Dana", last_name: "Sender", person: nil) + + result = described_class.call(sender, "AWBW Portal ") + + expect(Mail::Address.new(result).display_name).to eq("Dana Sender") + expect(Mail::Address.new(result).address).to eq(generic) + end + + it "quotes a name containing address special characters" do + sender = build_stubbed(:user, first_name: "Dana,", last_name: "Sender ", person: nil) + + result = described_class.call(sender, generic) + + expect(Mail::Address.new(result).address).to eq(generic) + end + + it "strips control characters so a name cannot inject a header" do + sender = build_stubbed(:user, first_name: "Dana\r\nBcc: x@evil.test", last_name: "Sender", person: nil) + + result = described_class.call(sender, generic) + + expect(result).not_to include("\r", "\n") + expect(Mail::Address.new(result).address).to eq(generic) + end + + it "falls back to the address when the sender has no usable name" do + sender = build_stubbed(:user, first_name: " ", last_name: " ", email: "", person: nil) + + expect(described_class.call(sender, generic)).to eq(generic) + end + + it "returns a blank address unchanged rather than building a nameless header" do + expect(described_class.call(nil, nil)).to be_nil + end + end +end From 91387ab448951702e1bb341302b437568e9a2aef Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 9 Aug 2026 15:44:06 -0400 Subject: [PATCH 6/7] Keep the sender out of the delivered email; attribution is internal only Backs out the From display name. Recipients keep seeing the generic mailbox unchanged; sender stays an internal field the notifications index and show page read. Tests now guard that direction instead. --- AGENTS.md | 1 - app/jobs/notification_mailer_job.rb | 4 -- app/mailers/devise_mailer.rb | 2 - app/services/attributed_from_address.rb | 14 ----- spec/jobs/notification_mailer_job_spec.rb | 37 ++++++------ spec/mailers/devise_mailer_spec.rb | 19 ++----- spec/services/attributed_from_address_spec.rb | 56 ------------------- 7 files changed, 24 insertions(+), 109 deletions(-) delete mode 100644 app/services/attributed_from_address.rb delete mode 100644 spec/services/attributed_from_address_spec.rb diff --git a/AGENTS.md b/AGENTS.md index 1fe9b9f2c0..8cfd259da1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -205,7 +205,6 @@ action, or `authorize! :workshop, to: :summary?`). - `TaggingSearchService` — Search and filter tagging data - `PersonFromUserService` — Create Person from User account - `BulkInviteService` — Bulk send welcome instructions and reset created_at for users -- `AttributedFromAddress` — Builds the From header for admin-sent mail: the notification sender's name becomes the display name while the address stays on the SendGrid-authorized generic mailbox (used by `NotificationMailerJob` and `DeviseMailer`) - `FormBuilderService` — Builds configurable forms from composable sections with per-field visibility - `ModelDeduper` — Deduplication logic - `RichTextMigrator` — Rich text migration utility diff --git a/app/jobs/notification_mailer_job.rb b/app/jobs/notification_mailer_job.rb index ab67581f32..298afb5c2d 100644 --- a/app/jobs/notification_mailer_job.rb +++ b/app/jobs/notification_mailer_job.rb @@ -23,10 +23,6 @@ def perform(notification_id, persist_delivered_email: true) mailer = mailer_map[notification.kind]&.call(notification) raise "Unknown notification kind: #{notification.kind}" unless mailer - # Name the admin who sent it in the From display name, keeping the generic - # address (and reply_to) each mailer already set. No-op when sender is nil. - mailer.message.from = AttributedFromAddress.call(notification.sender, mailer.message.from&.first) - Notification.transaction do notification.lock! return if notification.delivered_at.present? diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index 12d0a13554..35401d594d 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -29,8 +29,6 @@ def confirmation_instructions(record, token, opts = {}) else "AWBW Portal: Welcome instructions for #{record.full_name}" end - # Invites sent by hand carry the staff member's name; the address stays generic. - opts[:from] = AttributedFromAddress.call(record.try(:confirmation_sender), ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org")) @mail = super end diff --git a/app/services/attributed_from_address.rb b/app/services/attributed_from_address.rb deleted file mode 100644 index 0f29772aae..0000000000 --- a/app/services/attributed_from_address.rb +++ /dev/null @@ -1,14 +0,0 @@ -class AttributedFromAddress - # Builds the From header for mail an admin sends by hand. SendGrid only authorizes - # our own domain, so the address stays generic and only the display name names the - # sender — "Dana Sender ". Returns the address untouched when - # there's no sender, so automated mail keeps going out as the portal. - def self.call(sender, address) - return address if address.blank? - - name = sender&.full_name.to_s.gsub(/[[:cntrl:]]/, " ").squish.presence - return address unless name - - Mail::Address.new(address).tap { |a| a.display_name = name }.format - end -end diff --git a/spec/jobs/notification_mailer_job_spec.rb b/spec/jobs/notification_mailer_job_spec.rb index d828efae78..11d67484bf 100644 --- a/spec/jobs/notification_mailer_job_spec.rb +++ b/spec/jobs/notification_mailer_job_spec.rb @@ -74,42 +74,43 @@ end end - # We only have SendGrid authorization for our own domain, so an attributed - # reminder may name the admin in the display name but never in the address. - context "sender attribution on the From header" do + # notification.sender is an internal audit field only. Recipients must keep + # seeing the generic mailbox with no personal name attached, both because + # SendGrid only authorizes our own domain and because we're not ready to + # change what the public sees. + context "sender attribution stays out of the delivered email" do let(:event_registration) { create(:event_registration) } let(:admin) { create(:user, first_name: "Dana", last_name: "Sender") } - - def reminder_for(sender) + let(:notification) do create(:notification, kind: "event_registration_reminder", noticeable: event_registration, recipient_role: "person", recipient_email: event_registration.registrant.preferred_email, - sender: sender) + sender: admin) end - it "names the sending admin in the display name only" do - described_class.new.perform(reminder_for(admin).id) + it "sends an attributed reminder from the generic address with no display name" do + described_class.new.perform(notification.id) mail = ActionMailer::Base.deliveries.last - expect(mail[:from].display_names).to eq([ "Dana Sender" ]) expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "no-reply@awbw.org") ]) + expect(mail[:from].display_names.compact).to be_empty + expect(mail.reply_to).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) end - it "leaves reply_to on the generic address so replies come back to us" do - described_class.new.perform(reminder_for(admin).id) + it "never names the sender in any header" do + described_class.new.perform(notification.id) - expect(ActionMailer::Base.deliveries.last.reply_to) - .to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + headers = ActionMailer::Base.deliveries.last.header.fields.map(&:to_s).join("\n") + expect(headers).not_to include("Dana Sender") end - it "sends automated mail with no display name" do - described_class.new.perform(reminder_for(nil).id) + it "still records the sender for the communications pages" do + described_class.new.perform(notification.id) - mail = ActionMailer::Base.deliveries.last - expect(mail[:from].display_names.compact).to be_empty - expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "no-reply@awbw.org") ]) + expect(notification.reload.sender).to eq(admin) + expect(notification.decorate.sender_name).to eq("Dana Sender") end end end diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb index 18c9407da3..949726e4f6 100644 --- a/spec/mailers/devise_mailer_spec.rb +++ b/spec/mailers/devise_mailer_spec.rb @@ -193,27 +193,18 @@ def unstub_notification_logging expect(Notification.last.decorate.sender_name).to eq(NotificationDecorator::PORTAL_SENDER_NAME) end - # We only have SendGrid authorization for our own domain, so the sender's name - # may appear in the From display name but never in the address. - it "names the sending admin in the invite's From display name only" do + # The sender is an internal audit field only — an admin-sent invite must still + # reach the recipient from the generic mailbox with no personal name attached. + it "sends an attributed invite from the generic address with no display name" do invitee = create(:user, :unconfirmed) invitee.send_confirmation_instructions(sender: admin) mail = ActionMailer::Base.deliveries.last - expect(mail[:from].display_names).to eq([ "Dana Sender" ]) expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) - expect(mail.reply_to).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) - end - - it "sends an automated confirmation with no display name" do - signup = create(:user, :unconfirmed) - - signup.send_confirmation_instructions - - mail = ActionMailer::Base.deliveries.last expect(mail[:from].display_names.compact).to be_empty - expect(mail.from).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + expect(mail.reply_to).to eq([ ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") ]) + expect(mail.header.fields.map(&:to_s).join("\n")).not_to include("Dana Sender") end it "leaves an automated password reset as the portal" do diff --git a/spec/services/attributed_from_address_spec.rb b/spec/services/attributed_from_address_spec.rb deleted file mode 100644 index 8f5ac4503f..0000000000 --- a/spec/services/attributed_from_address_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -require "rails_helper" - -RSpec.describe AttributedFromAddress do - let(:generic) { "programs@awbw.org" } - - describe ".call" do - it "keeps the generic address and only sets the display name" do - sender = build_stubbed(:user, first_name: "Dana", last_name: "Sender", person: nil) - - result = described_class.call(sender, generic) - - expect(result).to eq("Dana Sender ") - expect(Mail::Address.new(result).address).to eq(generic) - end - - it "returns the address untouched for automated mail with no sender" do - expect(described_class.call(nil, generic)).to eq(generic) - end - - it "replaces a display name the address already carried" do - sender = build_stubbed(:user, first_name: "Dana", last_name: "Sender", person: nil) - - result = described_class.call(sender, "AWBW Portal ") - - expect(Mail::Address.new(result).display_name).to eq("Dana Sender") - expect(Mail::Address.new(result).address).to eq(generic) - end - - it "quotes a name containing address special characters" do - sender = build_stubbed(:user, first_name: "Dana,", last_name: "Sender ", person: nil) - - result = described_class.call(sender, generic) - - expect(Mail::Address.new(result).address).to eq(generic) - end - - it "strips control characters so a name cannot inject a header" do - sender = build_stubbed(:user, first_name: "Dana\r\nBcc: x@evil.test", last_name: "Sender", person: nil) - - result = described_class.call(sender, generic) - - expect(result).not_to include("\r", "\n") - expect(Mail::Address.new(result).address).to eq(generic) - end - - it "falls back to the address when the sender has no usable name" do - sender = build_stubbed(:user, first_name: " ", last_name: " ", email: "", person: nil) - - expect(described_class.call(sender, generic)).to eq(generic) - end - - it "returns a blank address unchanged rather than building a nameless header" do - expect(described_class.call(nil, nil)).to be_nil - end - end -end From 76c7986997d34f9a47de8ce2846089aed00aa07d Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 9 Aug 2026 15:59:12 -0400 Subject: [PATCH 7/7] Carry the invite sender as an id through mailer opts, not a record ivar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sender rode on an @confirmation_sender ivar of the User record, which only survives because Devise delivers confirmations with deliver_now (same in-memory object reaches DeviseMailer). If delivery ever became async the record would round-trip through GlobalID and the ivar — and the "From: " attribution — would be silently lost. Pass the sender as a plain id in the mailer opts instead; a hash survives GlobalID serialization, so the attribution holds regardless of delivery mode. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/mailers/devise_mailer.rb | 15 +++++++++++++-- app/models/user.rb | 16 +++++++++------- spec/models/user_spec.rb | 10 ++++++---- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index 35401d594d..55d765fbe3 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -19,6 +19,9 @@ def reset_password_instructions(record, token, opts = {}) end def confirmation_instructions(record, token, opts = {}) + # The invite sender arrives as a plain id in opts (GlobalID-safe for async + # delivery); pull it out before super so Devise doesn't fold it into the headers. + @confirmation_sender_id = opts.delete(:sender_id) @record = record @token = token @user = record @@ -82,7 +85,7 @@ def create_notification_record recipient_email: recipient_email, kind: kind, notification_type: 1, - sender: @record.try(:confirmation_sender), # the staff member who triggered it, when one did + sender: confirmation_sender, # the staff member who triggered it, when one did deliver: false # Devise already sent the email, so no need to deliver via the job ) @@ -135,7 +138,15 @@ def track_devise_email_event Analytics::AhoyTracker.track_auth_event( event_name, properties, - user: @record.confirmation_sender || Current.user + user: confirmation_sender || Current.user ) end + + # The staff member who triggered this confirmation, resolved from the id passed + # through the mailer opts. Memoized so create_notification_record and + # track_devise_email_event share one lookup. + def confirmation_sender + return @confirmation_sender if defined?(@confirmation_sender) + @confirmation_sender = @confirmation_sender_id ? User.find_by(id: @confirmation_sender_id) : nil + end end diff --git a/app/models/user.rb b/app/models/user.rb index 21ed16edb0..c8e14c6769 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -208,20 +208,22 @@ def gallery_assets # method needed for idea_submitted_fyi mailer [] end - # The staff member who triggered this confirmation email, when one did. Not - # persisted — DeviseMailer reads it off the record to attribute the - # notification it logs, since it has no request and no current_user. - attr_reader :confirmation_sender - # Override Devise to always send confirmation to the pending email when present. # Devise's default checks `pending_reconfirmation?` but can still route to the # current email in some flows. This ensures the confirmation always targets the # unconfirmed (new) email address. + # + # The invite sender (the staff member who triggered it, when one did) rides + # through the mailer opts as a plain id — DeviseMailer reads it to attribute the + # notification it logs, since it has no request and no current_user. Passing an + # id rather than stashing it on the record keeps it intact if the confirmation + # is ever delivered async (the record round-trips through GlobalID; the opts don't). def send_confirmation_instructions(sender: nil) - @confirmation_sender = sender generate_confirmation_token! unless @raw_confirmation_token target = unconfirmed_email.presence || email - send_devise_notification(:confirmation_instructions, @raw_confirmation_token, to: target) + opts = { to: target } + opts[:sender_id] = sender.id if sender + send_devise_notification(:confirmation_instructions, @raw_confirmation_token, opts) end def set_welcome_instructions_token! diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 42a2fcdbca..92dcf2d705 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -442,16 +442,18 @@ allow(DeviseMailer).to receive(:confirmation_instructions).and_return(mock_mail) end - it "exposes it as confirmation_sender for the mailer to attribute" do + it "passes the sender id through the mailer opts for attribution" do user.send_confirmation_instructions(sender: sender) - expect(user.confirmation_sender).to eq(sender) + expect(DeviseMailer).to have_received(:confirmation_instructions) + .with(user, anything, hash_including(sender_id: sender.id)) end - it "leaves confirmation_sender unset when none is given" do + it "omits the sender id when none is given" do user.send_confirmation_instructions - expect(user.confirmation_sender).to be_nil + expect(DeviseMailer).to have_received(:confirmation_instructions) + .with(user, anything, hash_excluding(:sender_id)) end end end