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/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 7e038a7cae..58b4bbd7ce 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) - 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 e74dd57b40..35401d594d 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: @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 ) @@ -134,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/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/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/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/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 fc7381cac4..adb0a26744 100644 --- a/app/views/notifications/show.html.erb +++ b/app/views/notifications/show.html.erb @@ -104,6 +104,13 @@
+
+
From
+
+ <%= @notification.decorate.sender_name %> +
+
+
Subject
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 new file mode 100644 index 0000000000..d488d3cf98 --- /dev/null +++ b/spec/jobs/bulk_invite_email_job_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe BulkInviteEmailJob do + it "sends confirmation instructions to the user" do + user = create(:user, :unconfirmed) + + expect_any_instance_of(User).to receive(:send_confirmation_instructions).with(sender: nil) + + described_class.perform_now(user.id) + end + + it "passes the sender through so the invite is attributed to them" do + user = create(:user, :unconfirmed) + sender = create(:user) + + 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 "sends with no sender when the sender no longer exists" do + user = create(:user, :unconfirmed) + + expect_any_instance_of(User).to receive(:send_confirmation_instructions).with(sender: nil) + + 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/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..3e3fdbe8e7 100644 --- a/spec/requests/notifications_spec.rb +++ b/spec/requests/notifications_spec.rb @@ -249,6 +249,29 @@ 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(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 + automated = create(:notification, kind: "account_confirmation", sender: nil) + + get notification_path(automated) + + expect(from_row(response.body)).to have_text("AWBW Portal") + end end context "as a non-admin owner" do @@ -329,6 +352,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 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