Fix broadcast suppression wrongly blocking transactional emails

`check_email_suppressions` and `retry!` queried all active
suppressions regardless of `stream_id`, so a newsletter unsubscribe
(`broadcast` `ManualSuppression`) would prevent invoices and other
transactional emails from being delivered. Now only `outbound`
suppressions are checked for non-newsletter emails, matching the
existing `HasEmails#active_emails` behavior.

46 transactional emails wrongly suppressed across 12 tenants.

Author: thibaudgg

app/models/mail_delivery/email.rb

@@ -97,7 +97,9 @@ def suppress_invalid_address!
     end
 
     def check_email_suppressions
-      suppressions = EmailSuppression.active.where(email: email).select(:id, :reason)
+      scope = EmailSuppression.active.where(email: email)
+      scope = scope.outbound unless mail_delivery.newsletter?
+      suppressions = scope.select(:id, :reason)
 
       self.email_suppression_ids = suppressions.map(&:id)
       self.email_suppression_reasons = suppressions.map(&:reason).uniq

app/models/mail_delivery/email/retriable.rb

@@ -15,7 +15,9 @@ module MailDelivery::Email::Retriable
   def retry!
     invalid_transition(:retry) unless suppressed?
 
-    fresh_suppressions = EmailSuppression.active.where(email: email).select(:id, :reason)
+    scope = EmailSuppression.active.where(email: email)
+    scope = scope.outbound unless mail_delivery.newsletter?
+    fresh_suppressions = scope.select(:id, :reason)
 
     if fresh_suppressions.any?
       update!(

test/mailers/concerns/templatable_test.rb

@@ -245,26 +245,26 @@ class TemplatableTest < ActionMailer::TestCase
     assert delivery.emails.first.processing?
   end
 
-  test "broadcast-suppressed email is marked as suppressed via process!" do
+  test "broadcast-suppressed email is NOT blocked for transactional email" do
     template = mail_templates(:member_validated)
     template.update!(active: true)
     member = members(:john)
     member.update!(emails: "john@doe.com, extra@doe.com")
-    suppression = suppress_email("extra@doe.com", stream_id: "broadcast")
+    suppress_email("extra@doe.com", stream_id: "broadcast")
 
     perform_enqueued_jobs do
       MailTemplate.deliver(:member_validated, member: member)
     end
 
-    # broadcast suppressions are NOT filtered by active_emails (only outbound are),
-    # so both emails are in one delivery; the broadcast-suppressed one is marked suppressed
+    # broadcast suppressions should not block transactional (non-newsletter) emails;
+    # both emails are created and both are deliverable
     delivery = MailDelivery.where(member: member, mailable_type: "Member", action: "validated").last
     assert_equal 2, delivery.emails.count
 
-    suppressed_email = delivery.emails.find_by(email: "extra@doe.com")
-    assert suppressed_email.suppressed?
-    assert_equal [ suppression.id ], suppressed_email.email_suppression_ids
-    assert_equal %w[HardBounce], suppressed_email.email_suppression_reasons
+    extra_email = delivery.emails.find_by(email: "extra@doe.com")
+    assert extra_email.processing?
+    assert extra_email.deliverable?
+    assert_empty extra_email.email_suppression_ids
 
     clean_email = delivery.emails.find_by(email: "john@doe.com")
     assert clean_email.processing?

test/models/mail_delivery/email/retriable_test.rb

@@ -56,6 +56,37 @@ class MailDelivery::Email::RetriableTest < ActiveSupport::TestCase
     assert_equal %w[ManualSuppression], email.email_suppression_reasons
   end
 
+  test "retry! succeeds for transactional email with only broadcast suppression" do
+    member = members(:john)
+    suppress_email("john@doe.com", stream_id: "broadcast", reason: "ManualSuppression", origin: "Customer")
+
+    # Force-create a suppressed transactional email (simulating the old buggy behavior)
+    delivery = MailDelivery.deliver!(
+      member: member, mailable: invoices(:other_closed), action: "created",
+      recipients: [ "john@doe.com" ])
+
+    email = delivery.emails.first
+    suppression = EmailSuppression.active.broadcast.find_by(email: "john@doe.com")
+    email.update_columns(
+      state: "suppressed",
+      email_suppression_ids: [ suppression.id ],
+      email_suppression_reasons: [ "ManualSuppression" ])
+    delivery.recompute_state!
+
+    assert email.reload.suppressed?
+
+    # retry! should now ignore the broadcast suppression for transactional emails
+    assert_enqueued_with(job: MailDelivery::ProcessJob) do
+      assert email.retry!
+    end
+
+    email.reload
+    assert email.processing?
+    assert_empty email.email_suppression_ids
+    assert_empty email.email_suppression_reasons
+    assert_equal "processing", email.mail_delivery.reload.state
+  end
+
   test "retry! raises for non-suppressed email" do
     member = members(:john)
     delivery = MailDelivery.deliver!(

test/models/mail_delivery/email_test.rb

@@ -248,6 +248,79 @@ class MailDelivery::EmailTest < ActiveSupport::TestCase
     assert_not MailDelivery::Email.exists?(email.id)
   end
 
+  test "broadcast suppression does NOT block transactional email (invoice)" do
+    member = members(:john)
+    suppress_email("john@doe.com", stream_id: "broadcast", reason: "ManualSuppression", origin: "Customer")
+
+    delivery = MailDelivery.deliver!(
+      member: member, mailable: invoices(:other_closed), action: "created")
+
+    email = delivery.emails.find_by(email: "john@doe.com")
+    assert email.present?, "Email record should have been created"
+    assert email.deliverable?, "Broadcast suppression should not block transactional email"
+    assert_empty email.email_suppression_ids
+  end
+
+  test "broadcast suppression DOES block newsletter email" do
+    member = members(:john)
+    suppress_email("john@doe.com", stream_id: "broadcast", reason: "ManualSuppression", origin: "Customer")
+
+    delivery = MailDelivery.deliver!(
+      member: member, mailable: newsletters(:simple), action: "newsletter")
+
+    email = delivery.emails.find_by(email: "john@doe.com")
+    assert email.present?, "Email record should have been created"
+    assert_not email.deliverable?, "Broadcast suppression should block newsletter"
+    assert_not_empty email.email_suppression_ids
+  end
+
+  test "outbound suppression blocks transactional email" do
+    member = members(:john)
+    member.update!(emails: "john@doe.com, jojo@old.com")
+    suppress_email("jojo@old.com", stream_id: "outbound")
+
+    delivery = MailDelivery.deliver!(
+      member: member, mailable: invoices(:other_closed), action: "created")
+
+    # jojo@old.com should not even be a recipient (filtered by active_emails)
+    assert_nil delivery.emails.find_by(email: "jojo@old.com")
+    # john@doe.com should be deliverable
+    email = delivery.emails.find_by(email: "john@doe.com")
+    assert email.present?
+    assert email.deliverable?
+  end
+
+  test "outbound suppression DOES block transactional email deliverable?" do
+    member = members(:john)
+    suppress_email("john@doe.com", stream_id: "outbound")
+
+    delivery = MailDelivery.deliver!(
+      member: member, mailable: invoices(:other_closed), action: "created",
+      recipients: [ "john@doe.com" ])
+
+    email = delivery.emails.find_by(email: "john@doe.com")
+    assert email.present?
+    assert_not email.deliverable?, "Outbound suppression should block transactional email"
+    assert_not_empty email.email_suppression_ids
+  end
+
+  test "both broadcast and outbound suppressions block newsletter email" do
+    member = members(:john)
+    member.update!(emails: "john@doe.com, jojo@old.com")
+    suppress_email("john@doe.com", stream_id: "broadcast", reason: "ManualSuppression", origin: "Customer")
+    suppress_email("jojo@old.com", stream_id: "outbound")
+
+    delivery = MailDelivery.deliver!(
+      member: member, mailable: newsletters(:simple), action: "newsletter",
+      recipients: [ "john@doe.com", "jojo@old.com" ])
+
+    email_broadcast = delivery.emails.find_by(email: "john@doe.com")
+    assert_not email_broadcast.deliverable?, "Broadcast suppression should block newsletter"
+
+    email_outbound = delivery.emails.find_by(email: "jojo@old.com")
+    assert_not email_outbound.deliverable?, "Outbound suppression should block newsletter"
+  end
+
   test "process! handles InvalidEmailRequestError by creating InvalidAddress suppression" do
     member = members(:john)
     delivery = MailDelivery.deliver!(