diff --git a/app/mailers/concerns/email_delivery.rb b/app/mailers/concerns/email_delivery.rb index 01aa97035..048916fd3 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:, 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..e89b33237 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:).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..71192be6c 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:, 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) @@ -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:, date_and_time: 1.month.ago), + role: 'Student', + attended: true + ) expect { perform_enqueued_jobs { call } }.not_to change(MemberEmailDelivery, :count) end