Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions app/mailers/concerns/email_delivery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,20 @@ module EmailDelivery

private

# The mailer action name distinguishes which email a logged row is for.
# find_or_create_by keeps a replayed delivery (e.g. a retried job) from
# raising on the unique index after the email has already gone out.
def log_sent_email
member = params[:member]
return unless member
return unless @_mail_was_called

MemberEmailDelivery.create!(
member: member,
subject: mail.subject,
body: mail.html_part ? mail.html_part.body.to_s : mail.body.to_s,
to: Array(mail.to),
cc: Array(mail.cc),
bcc: Array(mail.bcc)
)
MemberEmailDelivery.find_or_create_by!(member:, email_type: action_name) do |delivery|
delivery.subject = mail.subject
delivery.body = mail.html_part ? mail.html_part.body.to_s : mail.body.to_s
delivery.to = Array(mail.to)
delivery.cc = Array(mail.cc)
delivery.bcc = Array(mail.bcc)
end
end
end
3 changes: 1 addition & 2 deletions app/services/three_month_email_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ def self.send_chaser
.accepted_toc
.joins(:groups)
.merge(Group.students)
.left_joins(:member_email_deliveries)
.where(member_email_deliveries: { id: nil })
.where.not(id: MemberEmailDelivery.where(email_type: 'chaser').select(:member_id))
.where.not(id: recent_attendee_ids)
.where(id: past_year_attendee_ids)
.distinct
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class AddEmailTypeToMemberEmailDeliveries < ActiveRecord::Migration[8.1]
def change
# 'chaser' backfills existing rows (the only mailer action logging today);
# the default is then dropped so every writer must set the type explicitly.
add_column :member_email_deliveries, :email_type, :string, default: 'chaser', null: false
change_column_default :member_email_deliveries, :email_type, from: 'chaser', to: nil
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class AddUniqueIndexOnMemberEmailDeliveriesMemberAndType < ActiveRecord::Migration[8.1]
disable_ddl_transaction!

def change
add_index :member_email_deliveries, %i[member_id email_type], unique: true, algorithm: :concurrently
end
end
4 changes: 3 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[8.1].define(version: 2026_08_31_090000) do
ActiveRecord::Schema[8.1].define(version: 2026_08_31_100100) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_catalog.plpgsql"

Expand Down Expand Up @@ -420,10 +420,12 @@
t.text "body"
t.text "cc", default: [], array: true
t.datetime "created_at", null: false
t.string "email_type", null: false
t.bigint "member_id"
t.text "subject"
t.text "to", default: [], array: true
t.datetime "updated_at", null: false
t.index ["member_id", "email_type"], name: "index_member_email_deliveries_on_member_id_and_email_type", unique: true
t.index ["member_id"], name: "index_member_email_deliveries_on_member_id"
end

Expand Down
1 change: 1 addition & 0 deletions spec/fabricators/member_email_delivery_fabricator.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
Fabricator(:member_email_delivery) do
member(fabricator: :member)
email_type('chaser')
subject('Chaser')
body('Lorem ipsum')
to(['test_email@address'])
Expand Down
8 changes: 8 additions & 0 deletions spec/mailers/member_mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,20 @@
log = MemberEmailDelivery.last!

expect(log.member).to eq(member)
expect(log.email_type).to eq('chaser')
expect(log.subject).to eq('It’s been a while, how are you doing? ♥️')
expect(log.to).to eq([member.email])
# premailer-rails converts the message to multipart/alternative during
# delivery, so the logged body must come from the html part
expect(log.body).to be_present
expect(log.body).to include('codebar workshop')
end

it 'logs one row per member even if the delivery is performed twice' do
expect do
described_class.with(member: member).chaser.deliver_now

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
described_class.with(member: member).chaser.deliver_now
described_class.with(member:).chaser.deliver_now

described_class.with(member:).chaser.deliver_now
end.to change(MemberEmailDelivery, :count).by(1)
end
end
end
30 changes: 29 additions & 1 deletion spec/services/three_month_email_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@
member
end

let!(:student_with_other_email_logged) do
member = Fabricate(:member)
Fabricate(:subscription, member:, group: students_group)
Fabricate(
:workshop_invitation,
member: member,
workshop: Fabricate(:workshop, chapter:, date_and_time: 6.months.ago),
role: 'Student',
attended: true
)
Fabricate(:member_email_delivery, member:, email_type: 'welcome_email')
member
end

let!(:student_with_recent_attendance) do
member = Fabricate(:member)
Fabricate(:subscription, member: member, group: students_group)
Expand Down Expand Up @@ -92,17 +106,24 @@
end

it 'emails only students who have not attended in the last 3 months and were not emailed before' do
expect { perform_enqueued_jobs { call } }.to change(MemberEmailDelivery, :count).by(2)
expect { perform_enqueued_jobs { call } }.to change(MemberEmailDelivery, :count).by(3)

expect(MemberEmailDelivery.where(member: eligible_student)).to exist
expect(MemberEmailDelivery.where(member: student_with_old_attendance)).to exist
expect(MemberEmailDelivery.where(member: student_with_other_email_logged, email_type: 'chaser')).to exist
end

it 'does not email a member already present in member_email_deliveries' do
expect { perform_enqueued_jobs { call } }
.not_to(change { MemberEmailDelivery.where(member: already_emailed_student).count })
end

it 'emails a member whose only logged delivery is of another type' do
expect { perform_enqueued_jobs { call } }
.to change { MemberEmailDelivery.where(member: student_with_other_email_logged, email_type: 'chaser').count }
.by(1)
end

it 'does not email students with a recent attended workshop' do
expect { perform_enqueued_jobs { call } }
.not_to(change { MemberEmailDelivery.where(member: student_with_recent_attendance).count })
Expand Down Expand Up @@ -185,6 +206,13 @@
role: 'Student',
attended: true
)
Fabricate(
:workshop_invitation,
member: student_with_other_email_logged,
workshop: Fabricate(:workshop, chapter:, date_and_time: 1.month.ago),
role: 'Student',
attended: true
)

expect { perform_enqueued_jobs { call } }.not_to change(MemberEmailDelivery, :count)
end
Expand Down