From 89b9986a570c9253603cf7127b6ff4fcf7677c4f Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Mon, 31 Aug 2026 12:54:53 +0200 Subject: [PATCH 1/7] refactor: make member_email_deliveries a typed log Rows recorded which member received an email but not which email it was, and the chaser's "already emailed" exclusion only works because it is the sole writer of the table. Type each row with the mailer action that sent it (email_type; 'chaser' for all existing rows), enforce one row per (member, email_type) with a unique index, and scope the chaser's exclusion to its own rows so its behavior is unchanged regardless of what else gets logged. Log writes are replay-tolerant: a retried delivery job no longer raises after the email has already gone out. --- app/mailers/concerns/email_delivery.rb | 18 ++++++----- app/services/three_month_email_service.rb | 3 +- ...d_email_type_to_member_email_deliveries.rb | 8 +++++ ...member_email_deliveries_member_and_type.rb | 7 +++++ db/schema.rb | 4 ++- .../member_email_delivery_fabricator.rb | 1 + spec/mailers/member_mailer_spec.rb | 8 +++++ .../three_month_email_service_spec.rb | 30 ++++++++++++++++++- 8 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 db/migrate/20260831100000_add_email_type_to_member_email_deliveries.rb create mode 100644 db/migrate/20260831100100_add_unique_index_on_member_email_deliveries_member_and_type.rb diff --git a/app/mailers/concerns/email_delivery.rb b/app/mailers/concerns/email_delivery.rb index 01aa97035..fbae296cc 100644 --- a/app/mailers/concerns/email_delivery.rb +++ b/app/mailers/concerns/email_delivery.rb @@ -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: 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 diff --git a/app/services/three_month_email_service.rb b/app/services/three_month_email_service.rb index fab281c60..e46631271 100644 --- a/app/services/three_month_email_service.rb +++ b/app/services/three_month_email_service.rb @@ -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 diff --git a/db/migrate/20260831100000_add_email_type_to_member_email_deliveries.rb b/db/migrate/20260831100000_add_email_type_to_member_email_deliveries.rb new file mode 100644 index 000000000..809d8e9e0 --- /dev/null +++ b/db/migrate/20260831100000_add_email_type_to_member_email_deliveries.rb @@ -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 diff --git a/db/migrate/20260831100100_add_unique_index_on_member_email_deliveries_member_and_type.rb b/db/migrate/20260831100100_add_unique_index_on_member_email_deliveries_member_and_type.rb new file mode 100644 index 000000000..51c1a0410 --- /dev/null +++ b/db/migrate/20260831100100_add_unique_index_on_member_email_deliveries_member_and_type.rb @@ -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 diff --git a/db/schema.rb b/db/schema.rb index 112e8f584..b10ba22d8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -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" @@ -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 diff --git a/spec/fabricators/member_email_delivery_fabricator.rb b/spec/fabricators/member_email_delivery_fabricator.rb index 2b473815e..ba1f62108 100644 --- a/spec/fabricators/member_email_delivery_fabricator.rb +++ b/spec/fabricators/member_email_delivery_fabricator.rb @@ -1,5 +1,6 @@ Fabricator(:member_email_delivery) do member(fabricator: :member) + email_type('chaser') subject('Chaser') body('Lorem ipsum') to(['test_email@address']) diff --git a/spec/mailers/member_mailer_spec.rb b/spec/mailers/member_mailer_spec.rb index fe71c319e..5fcaaf436 100644 --- a/spec/mailers/member_mailer_spec.rb +++ b/spec/mailers/member_mailer_spec.rb @@ -180,6 +180,7 @@ 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 @@ -187,5 +188,12 @@ 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 + described_class.with(member: member).chaser.deliver_now + end.to change(MemberEmailDelivery, :count).by(1) + end end end diff --git a/spec/services/three_month_email_service_spec.rb b/spec/services/three_month_email_service_spec.rb index 19308784b..5908bd481 100644 --- a/spec/services/three_month_email_service_spec.rb +++ b/spec/services/three_month_email_service_spec.rb @@ -34,6 +34,20 @@ member end + let!(:student_with_other_email_logged) do + member = Fabricate(:member) + Fabricate(:subscription, member: member, group: students_group) + Fabricate( + :workshop_invitation, + member: member, + workshop: Fabricate(:workshop, chapter: chapter, date_and_time: 6.months.ago), + role: 'Student', + attended: true + ) + Fabricate(:member_email_delivery, member: member, email_type: 'welcome_email') + member + end + let!(:student_with_recent_attendance) do member = Fabricate(:member) Fabricate(:subscription, member: member, group: students_group) @@ -92,10 +106,11 @@ 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 @@ -103,6 +118,12 @@ .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 }) @@ -185,6 +206,13 @@ role: 'Student', attended: true ) + Fabricate( + :workshop_invitation, + member: student_with_other_email_logged, + workshop: Fabricate(:workshop, chapter: chapter, date_and_time: 1.month.ago), + role: 'Student', + attended: true + ) expect { perform_enqueued_jobs { call } }.not_to change(MemberEmailDelivery, :count) end From a340ed05757cddd38f290208ac7adeffa1dd076c Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:31:41 +0200 Subject: [PATCH 2/7] Update app/mailers/concerns/email_delivery.rb Co-authored-by: Olle Jonsson --- app/mailers/concerns/email_delivery.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/mailers/concerns/email_delivery.rb b/app/mailers/concerns/email_delivery.rb index fbae296cc..048916fd3 100644 --- a/app/mailers/concerns/email_delivery.rb +++ b/app/mailers/concerns/email_delivery.rb @@ -11,7 +11,7 @@ def log_sent_email return unless member return unless @_mail_was_called - MemberEmailDelivery.find_or_create_by!(member: member, email_type: action_name) do |delivery| + 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) From efb3c13deb5c11a6ebf7feb0d3b32ecc346126d4 Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:31:53 +0200 Subject: [PATCH 3/7] Update spec/mailers/member_mailer_spec.rb Co-authored-by: Olle Jonsson --- spec/mailers/member_mailer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mailers/member_mailer_spec.rb b/spec/mailers/member_mailer_spec.rb index 5fcaaf436..e89b33237 100644 --- a/spec/mailers/member_mailer_spec.rb +++ b/spec/mailers/member_mailer_spec.rb @@ -192,7 +192,7 @@ it 'logs one row per member even if the delivery is performed twice' do expect do described_class.with(member: member).chaser.deliver_now - described_class.with(member: member).chaser.deliver_now + described_class.with(member:).chaser.deliver_now end.to change(MemberEmailDelivery, :count).by(1) end end From 4e34ecb6e0d586d8f58b61881bdd1c6648175645 Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:32:06 +0200 Subject: [PATCH 4/7] Update spec/services/three_month_email_service_spec.rb Co-authored-by: Olle Jonsson --- spec/services/three_month_email_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/three_month_email_service_spec.rb b/spec/services/three_month_email_service_spec.rb index 5908bd481..69814854c 100644 --- a/spec/services/three_month_email_service_spec.rb +++ b/spec/services/three_month_email_service_spec.rb @@ -36,7 +36,7 @@ let!(:student_with_other_email_logged) do member = Fabricate(:member) - Fabricate(:subscription, member: member, group: students_group) + Fabricate(:subscription, member:, group: students_group) Fabricate( :workshop_invitation, member: member, From 1e02c61ab217f0601b150703283bda50ee29cef9 Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:40:09 +0200 Subject: [PATCH 5/7] Update spec/services/three_month_email_service_spec.rb Co-authored-by: Olle Jonsson --- spec/services/three_month_email_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/three_month_email_service_spec.rb b/spec/services/three_month_email_service_spec.rb index 69814854c..553d970b8 100644 --- a/spec/services/three_month_email_service_spec.rb +++ b/spec/services/three_month_email_service_spec.rb @@ -40,7 +40,7 @@ Fabricate( :workshop_invitation, member: member, - workshop: Fabricate(:workshop, chapter: chapter, date_and_time: 6.months.ago), + workshop: Fabricate(:workshop, chapter:, date_and_time: 6.months.ago), role: 'Student', attended: true ) From 2f383d8e765ea5012ba62e91b1ee40d29a14e7f7 Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:40:20 +0200 Subject: [PATCH 6/7] Update spec/services/three_month_email_service_spec.rb Co-authored-by: Olle Jonsson --- spec/services/three_month_email_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/three_month_email_service_spec.rb b/spec/services/three_month_email_service_spec.rb index 553d970b8..2a2ec4aba 100644 --- a/spec/services/three_month_email_service_spec.rb +++ b/spec/services/three_month_email_service_spec.rb @@ -209,7 +209,7 @@ Fabricate( :workshop_invitation, member: student_with_other_email_logged, - workshop: Fabricate(:workshop, chapter: chapter, date_and_time: 1.month.ago), + workshop: Fabricate(:workshop, chapter:, date_and_time: 1.month.ago), role: 'Student', attended: true ) From b6ab4c3ef825d067a8269895ce36251caf3e5f9c Mon Sep 17 00:00:00 2001 From: Morgan Roderick <20321+mroderick@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:55:29 +0200 Subject: [PATCH 7/7] Update spec/services/three_month_email_service_spec.rb Co-authored-by: Olle Jonsson --- spec/services/three_month_email_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/three_month_email_service_spec.rb b/spec/services/three_month_email_service_spec.rb index 2a2ec4aba..71192be6c 100644 --- a/spec/services/three_month_email_service_spec.rb +++ b/spec/services/three_month_email_service_spec.rb @@ -44,7 +44,7 @@ role: 'Student', attended: true ) - Fabricate(:member_email_delivery, member: member, email_type: 'welcome_email') + Fabricate(:member_email_delivery, member:, email_type: 'welcome_email') member end