fix: safely skip emails to members with invalid addresses

app/helpers/email_header_helper.rb

@@ -1,14 +1,28 @@
 module EmailHeaderHelper
+  class SkippedEmail
+    def deliver_now; self; end
+    def deliver_later; self; end
+    def deliver; self; end
+    def deliver!; self; end
+  end
+
   private
 
-  def mail_args(member, subject, from_email = 'meetings@codebar.io', cc = '', bcc = '')
-    return nil if invalid_email?(member.email, member.id)
+  def mail_to_member(member, subject, from_email = 'meetings@codebar.io', cc = '', bcc = '', &block)
+    if invalid_email?(member.email, member.id)
+      Rails.logger.info(
+        "[#{self.class.name}] Skipped email to member #{member.id}: " \
+        "invalid email #{member.email.inspect}"
+      )
+      return SkippedEmail.new
+    end
 
-    { from: "codebar.io <#{from_email}>",
-      to: member.email,
-      cc: cc,
-      bcc: bcc,
-      subject: subject }
+    mail(from: "codebar.io <#{from_email}>",
+         to: member.email,
+         cc: cc,
+         bcc: bcc,
+         subject: subject,
+         &block)
   end
 
   def invalid_email?(email, member_id)

app/mailers/concerns/email_delivery.rb

@@ -6,6 +6,7 @@ module EmailDelivery
   def log_sent_email
     member = params[:member]
     return unless member
+    return unless @_mail_was_called
 
     MemberEmailDelivery.create!(
       member: member,

app/mailers/contact_mailer.rb

@@ -9,6 +9,6 @@ def subscription_notification(contact)
 
     subject = "You have been added to codebar's sponsors mailing list"
 
-    mail(mail_args(contact, subject, 'no-reply@codebar.io'), &:html)
+    mail_to_member(contact, subject, 'no-reply@codebar.io', &:html)
   end
 end

app/mailers/event_invitation_mailer.rb

@@ -12,7 +12,7 @@ def invite_student(event, member, invitation)
 
     subject = "Invitation: #{@event.name}"
 
-    mail(mail_args(member, subject), &:html)
+    mail_to_member(member, subject, &:html)
   end
 
   def invite_coach(event, member, invitation)
@@ -22,8 +22,8 @@ def invite_coach(event, member, invitation)
     @host_address = AddressPresenter.new(@event.venue.address) if @event.venue.present?
     @everyone_is_invited = !event.audience
 
-    mail(mail_args(member, @everyone_is_invited ? "Invitation: #{@event.name}" : "Coach Invitation: #{@event.name}"),
-         &:html)
+    mail_to_member(member, @everyone_is_invited ? "Invitation: #{@event.name}" : "Coach Invitation: #{@event.name}",
+                   &:html)
   end
 
   def attending(event, member, invitation)
@@ -38,7 +38,7 @@ def attending(event, member, invitation)
 
     subject = "Your spot to #{@event.name} has been confirmed."
 
-    mail(mail_args(member, subject), &:html)
+    mail_to_member(member, subject, &:html)
   end
 
   private

app/mailers/feedback_request_mailer.rb

@@ -10,7 +10,7 @@ def request_feedback(workshops, member, feedback_request)
 
     subject = "#{@workshop} Feedback for #{l(@workshop.date_and_time, format: :email_title)}"
 
-    mail(mail_args(member, subject), &:html)
+    mail_to_member(member, subject, &:html)
   end
 
   helper do

app/mailers/meeting_invitation_mailer.rb

@@ -12,7 +12,7 @@ def invite(meeting, member, invitation)
     @rsvp_url = meeting_url(@meeting, token: @invitation.token)
 
     subject = "You are invited to codebar's #{@meeting.name} on #{humanize_date(@meeting.date_and_time)}"
-    mail(mail_args(@member, subject), &:html)
+    mail_to_member(@member, subject, &:html)
   end
 
   def attending(meeting, member)
@@ -22,7 +22,7 @@ def attending(meeting, member)
     @cancellation_url = meeting_url(@meeting)
 
     subject = "See you at #{@meeting.name} on #{humanize_date(@meeting.date_and_time)}"
-    mail(mail_args(@member, subject), &:html)
+    mail_to_member(@member, subject, &:html)
   end
 
   def approve_from_waitlist(meeting, member)
@@ -32,7 +32,7 @@ def approve_from_waitlist(meeting, member)
     @cancellation_url = meeting_url(@meeting)
 
     subject = "A spot has opened up for #{@meeting.name} on #{humanize_date(@meeting.date_and_time)}"
-    mail(mail_args(@member, subject), &:html)
+    mail_to_member(@member, subject, &:html)
   end
 
   def attendance_reminder(meeting, member)
@@ -42,7 +42,7 @@ def attendance_reminder(meeting, member)
     @cancellation_url = meeting_url(@meeting)
 
     subject = "Reminder: You have a spot for #{@meeting.name} on #{humanize_date(@meeting.date_and_time)}"
-    mail(mail_args(@member, subject), &:html)
+    mail_to_member(@member, subject, &:html)
   end
 
   private

app/mailers/member_mailer.rb

@@ -7,7 +7,7 @@ class MemberMailer < ApplicationMailer
   def chaser
     @member = params[:member]
     subject = "It’s been a while, how are you doing? ♥️"
-    mail mail_args(@member, subject, 'hello@codebar.io', 'hello@codebar.io') do |format|
+    mail_to_member(@member, subject, 'hello@codebar.io', 'hello@codebar.io') do |format|
       format.html {render 'three_month_chaser'}
     end
   end
@@ -36,7 +36,7 @@ def welcome_student(member)
     @member = member
     subject = 'How codebar works'
 
-    mail(mail_args(member, subject, 'hello@codebar.io')) do |format|
+    mail_to_member(member, subject, 'hello@codebar.io') do |format|
       format.html { render 'welcome_student' }
     end
   end
@@ -45,7 +45,7 @@ def welcome_coach(member)
     @member = member
     subject = 'How codebar works'
 
-    mail(mail_args(member, subject, 'hello@codebar.io')) do |format|
+    mail_to_member(member, subject, 'hello@codebar.io') do |format|
       format.html { render 'welcome_coach' }
     end
   end
@@ -54,7 +54,7 @@ def eligibility_check(member, sender = 'hello@codebar.io')
     @member = member
     subject = 'Eligibility confirmation'
 
-    mail(mail_args(member, subject, 'hello@codebar.io', 'hello@codebar.io', sender)) do |format|
+    mail_to_member(member, subject, 'hello@codebar.io', 'hello@codebar.io', sender) do |format|
       format.html { render 'eligibility_check' }
     end
   end
@@ -63,7 +63,8 @@ def attendance_warning(member, sender = 'hello@codebar.io')
     @member = member
     subject = 'Attendance warning'
 
-    mail(mail_args(member, subject, 'hello@codebar.io', 'hello@codebar.io', sender)) do |format|
+    # TODO: .deliver here causes double-delivery because callers also call .deliver_now
+    mail_to_member(member, subject, 'hello@codebar.io', 'hello@codebar.io', sender) do |format|
       format.html { render 'attendance_warning' }
     end.deliver
   end
@@ -74,7 +75,8 @@ def ban(member, ban)
     @expiry_date = I18n.l(ban.expires_at, format: :default)
     @ban = ban
 
-    mail(mail_args(member, @reason, 'hello@codebar.io', 'hello@codebar.io')) do |format|
+    # TODO: .deliver here causes double-delivery because callers also call .deliver_now
+    mail_to_member(member, @reason, 'hello@codebar.io', 'hello@codebar.io') do |format|
       format.html { render 'ban' }
     end.deliver
   end

app/mailers/virtual_workshop_invitation_mailer.rb

@@ -17,39 +17,39 @@ def attending(workshop, member, invitation, waiting_list = false)
     attachments['codebar.ics'] = { mime_type: 'text/calendar',
                                    content: WorkshopCalendar.new(workshop, invitation_url(invitation)).ical }
 
-    mail(mail_args(member, subject, @workshop.chapter.email), &:html)
+    mail_to_member(member, subject, @workshop.chapter.email, &:html)
   end
 
   def attending_reminder(workshop, member, invitation)
     setup(workshop, invitation, member)
     subject = t('mailer.workshop_invitation.virtual.attending_reminder.subject',
                 date_time: humanize_date(workshop.date_and_time, with_time: true))
 
-    mail(mail_args(member, subject, @workshop.chapter.email), &:html)
+    mail_to_member(member, subject, @workshop.chapter.email, &:html)
   end
 
   def invite_coach(workshop, member, invitation)
     setup(workshop, invitation, member)
     subject = t('mailer.workshop_invitation.virtual.invite_coach.subject',
                 date_time: humanize_date(@workshop.date_and_time, with_time: true))
 
-    mail(mail_args(member, subject, 'no-reply@codebar.io'), &:html)
+    mail_to_member(member, subject, 'no-reply@codebar.io', &:html)
   end
 
   def invite_student(workshop, member, invitation)
     setup(workshop, invitation, member)
     subject = t('mailer.workshop_invitation.virtual.invite_student.subject',
                 date_time: humanize_date(@workshop.date_and_time, with_time: true))
 
-    mail(mail_args(member, subject, 'no-reply@codebar.io'), &:html)
+    mail_to_member(member, subject, 'no-reply@codebar.io', &:html)
   end
 
   def waiting_list_reminder(workshop, member, invitation)
     setup(workshop, invitation, member)
     subject = t('mailer.workshop_invitation.waiting_list_reminder.subject',
                 date_time: humanize_date(workshop.date_and_time, with_time: true))
 
-    mail(mail_args(member, subject, @workshop.chapter.email), &:html)
+    mail_to_member(member, subject, @workshop.chapter.email, &:html)
   end
 
   private

app/mailers/workshop_invitation_mailer.rb

@@ -18,7 +18,7 @@ def attending(workshop, member, invitation, waiting_list = false)
     attachments['codebar.ics'] = { mime_type: 'text/calendar',
                                    content: WorkshopCalendar.new(workshop, invitation_url(invitation)).ical }
 
-    mail(mail_args(member, subject, @workshop.chapter.email), &:html)
+    mail_to_member(member, subject, @workshop.chapter.email, &:html)
   end
 
   def attending_reminder(workshop, member, invitation)
@@ -35,7 +35,7 @@ def invite_coach(workshop, member, invitation)
     subject = t('mailer.workshop_invitation.invite_coach.subject',
                 date_time: humanize_date(@workshop.date_and_time, with_time: true))
 
-    mail(mail_args(member, subject, 'no-reply@codebar.io'), &:html)
+    mail_to_member(member, subject, 'no-reply@codebar.io', &:html)
   end
 
   def invite_student(workshop, member, invitation)
@@ -46,7 +46,7 @@ def invite_student(workshop, member, invitation)
     subject = t('mailer.workshop_invitation.invite_student.subject',
                 date_time: humanize_date(@workshop.date_and_time, with_time: true))
 
-    mail(mail_args(member, subject, 'no-reply@codebar.io'), &:html)
+    mail_to_member(member, subject, 'no-reply@codebar.io', &:html)
   end
 
   def notify_waiting_list(invitation)
@@ -56,7 +56,7 @@ def notify_waiting_list(invitation)
 
     subject = t('mailer.workshop_invitation.notify_waiting_list.subject')
 
-    mail(mail_args(@member, subject, @workshop.chapter.email), &:html)
+    mail_to_member(@member, subject, @workshop.chapter.email, &:html)
   end
 
   def waiting_list_reminder(workshop, member, invitation)
@@ -72,6 +72,6 @@ def reminder_setup(workshop, member, invitation, subject)
     @member = member
     @invitation = invitation
 
-    mail(mail_args(member, subject, @workshop.chapter.email), &:html)
+    mail_to_member(member, subject, @workshop.chapter.email, &:html)
   end
 end

spec/helpers/email_header_helper_spec.rb

@@ -1,60 +1,78 @@
 RSpec.describe EmailHeaderHelper, type: :helper do
-  before { EmailHeaderHelper.module_eval { public :mail_args } }
+  before do
+    EmailHeaderHelper.module_eval { public :mail_to_member }
+  end
 
-  describe '#mail_args' do
+  describe '#mail_to_member' do
     let(:member) { Struct.new(:id, :email).new(1, 'test@example.com') }
 
-    it 'returns mail arguments for valid email' do
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result[:to]).to eq('test@example.com')
-      expect(result[:subject]).to eq('Test Subject')
+    it 'calls mail with correct arguments for valid email' do
+      expect(helper).to receive(:mail).with(
+        from: 'codebar.io <meetings@codebar.io>',
+        to: 'test@example.com',
+        cc: '',
+        bcc: '',
+        subject: 'Test Subject'
+      )
+      helper.mail_to_member(member, 'Test Subject')
+    end
+
+    it 'forwards the block to mail' do
+      expect(helper).to receive(:mail) do |**kwargs, &block|
+        expect(kwargs).to include(from: 'codebar.io <meetings@codebar.io>')
+        expect(block).to be_a(Proc)
+      end
+      helper.mail_to_member(member, 'Test Subject') { |format| format.html }
     end
 
-    it 'returns nil for nil email' do
+    it 'returns SkippedEmail for nil email' do
       member = Struct.new(:id, :email).new(1, nil)
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result).to be_nil
+      result = helper.mail_to_member(member, 'Test Subject')
+      expect(result).to be_a(EmailHeaderHelper::SkippedEmail)
     end
 
-    it 'returns nil for blank email' do
+    it 'returns SkippedEmail for blank email' do
       member = Struct.new(:id, :email).new(1, '')
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result).to be_nil
+      result = helper.mail_to_member(member, 'Test Subject')
+      expect(result).to be_a(EmailHeaderHelper::SkippedEmail)
     end
 
-    it 'returns nil for invalid email format' do
+    it 'returns SkippedEmail for invalid email format' do
       member = Struct.new(:id, :email).new(1, 'invalid-email')
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result).to be_nil
+      result = helper.mail_to_member(member, 'Test Subject')
+      expect(result).to be_a(EmailHeaderHelper::SkippedEmail)
     end
 
-    it 'returns nil for email missing @ symbol' do
+    it 'returns SkippedEmail for email missing @ symbol' do
       member = Struct.new(:id, :email).new(1, 'invalidexample.com')
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result).to be_nil
+      result = helper.mail_to_member(member, 'Test Subject')
+      expect(result).to be_a(EmailHeaderHelper::SkippedEmail)
     end
 
-    it 'returns nil for email missing TLD' do
+    it 'returns SkippedEmail for email missing TLD' do
       member = Struct.new(:id, :email).new(1, 'invalid@example')
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result).to be_nil
+      result = helper.mail_to_member(member, 'Test Subject')
+      expect(result).to be_a(EmailHeaderHelper::SkippedEmail)
     end
 
-    it 'returns mail arguments for valid email with plus addressing' do
-      member = Struct.new(:id, :email).new(1, 'user+tag@example.com')
-      result = helper.mail_args(member, 'Test Subject')
-      expect(result[:to]).to eq('user+tag@example.com')
+    it 'logs the skip' do
+      member = Struct.new(:id, :email).new(1, 'bad-email')
+      expect(Rails.logger).to receive(:info).with(/Skipped email to member 1/)
+      helper.mail_to_member(member, 'Test Subject')
     end
 
-    it 'includes from email when provided' do
-      result = helper.mail_args(member, 'Test Subject', 'custom@codebar.io')
-      expect(result[:from]).to eq('codebar.io <custom@codebar.io>')
+    it 'includes custom from email when provided' do
+      expect(helper).to receive(:mail).with(hash_including(from: 'codebar.io <custom@codebar.io>'))
+      helper.mail_to_member(member, 'Test Subject', 'custom@codebar.io')
     end
 
     it 'includes cc and bcc when provided' do
-      result = helper.mail_args(member, 'Test Subject', 'from@codebar.io', 'cc@codebar.io', 'bcc@codebar.io')
-      expect(result[:cc]).to eq('cc@codebar.io')
-      expect(result[:bcc]).to eq('bcc@codebar.io')
+      expect(helper).to receive(:mail).with(
+        hash_including(cc: 'cc@codebar.io', bcc: 'bcc@codebar.io')
+      )
+      helper.mail_to_member(member, 'Test Subject', 'from@codebar.io', 'cc@codebar.io', 'bcc@codebar.io')
     end
   end
+
+
 end