feat: reply-all recipients for outgoing mail

Author: dutow

app/assets/stylesheets/components/confirm-modal.css

@@ -24,6 +24,8 @@
   max-width: 32rem;
   width: 100%;
   padding: var(--spacing-6) var(--spacing-8);
+  max-height: calc(100vh - 2 * var(--spacing-6));
+  overflow-y: auto;
   animation: confirm-pop 140ms ease-out;
 }
 

app/controllers/drafts_controller.rb

@@ -83,7 +83,7 @@ def destroy
   end
 
   def confirm
-    @recipient = Outgoing::RecipientResolver.for(@draft.topic)
+    @recipients = Outgoing::RecipientResolver.for(@draft)
     render layout: false
   rescue Outgoing::RecipientResolver::MissingPostAddressError
     render plain: "This mailing list isn't configured for sending. An admin must set its post_address.",

app/services/outgoing/message_builder.rb

@@ -9,24 +9,25 @@ class MessageBuilder
     DEFAULT_DOMAIN = "hackorum.dev"
 
     def self.build(draft)
-      recipient = RecipientResolver.for(draft.topic)
-      domain    = ENV.fetch("HACKORUM_OUTGOING_DOMAIN", DEFAULT_DOMAIN)
-      msg_id    = "<#{SecureRandom.uuid}@#{domain}>"
+      recipients = RecipientResolver.for(draft)
+      domain     = ENV.fetch("HACKORUM_OUTGOING_DOMAIN", DEFAULT_DOMAIN)
+      msg_id     = "<#{SecureRandom.uuid}@#{domain}>"
 
       from_addr = draft.sender_alias
       mail = Mail.new do
         from       "#{from_addr.name} <#{from_addr.email}>"
-        to         recipient
+        to         recipients.to
         subject    draft.subject
         message_id msg_id
         body       draft.body
       end
+      mail.cc = recipients.cc if recipients.cc.any?
       mail.content_type "text/plain; charset=UTF-8"
       mail.in_reply_to = draft.reply_to_message.message_id.to_s.gsub(/[<>]/, "")
       mail.references  = build_references(draft.reply_to_message)
 
       Result.new(encoded: mail.encoded, message_id: msg_id,
-                 subject: draft.subject, recipient: recipient)
+                 subject: draft.subject, recipient: recipients.all.join(", "))
     end
 
     def self.build_references(parent)

app/services/outgoing/recipient_resolver.rb

@@ -1,22 +1,65 @@
+require "mail"
+
 module Outgoing
   class RecipientResolver
     class MissingPostAddressError   < StandardError; end
     class MissingDevOverrideError   < StandardError; end
     class RealListAddressInDevError < StandardError; end
 
-    def self.for(topic)
-      if Rails.env.production?
-        list = topic.mailing_lists.first
-        raise MissingPostAddressError if list.nil? || list.post_address.blank?
-        list.post_address
-      else
-        override = ENV["HACKORUM_DEV_REPLY_TO"]
-        raise MissingDevOverrideError if override.blank?
-        if MailingList.where("lower(post_address) = lower(?)", override).exists?
-          raise RealListAddressInDevError
-        end
-        override
+    Recipients = Struct.new(:to, :cc, keyword_init: true) do
+      def all = to + cc
+    end
+
+    FAKE_ADDRESS_SUFFIX = "@unknown.user"
+
+    def self.for(draft)
+      return Recipients.new(to: [ dev_override ], cc: []) unless Rails.env.production?
+
+      list = draft.topic.mailing_lists.first
+      raise MissingPostAddressError if list.nil? || list.post_address.blank?
+
+      parent = draft.reply_to_message
+      seen   = Set.new([ draft.sender_alias.email.to_s.strip.downcase ])
+
+      # both picks share seen, so cc never repeats to
+      to = pick(seen, [ parent.sender ])
+      cc = pick(seen, [ list.post_address ] + parent.mentioned_aliases.order("mentions.id").to_a)
+      to = cc.shift(1) if to.empty?
+
+      Recipients.new(to: to, cc: cc)
+    end
+
+    def self.pick(seen, entries)
+      entries.filter_map do |entry|
+        email = (entry.respond_to?(:email) ? entry.email : entry).to_s.strip
+        next if email.blank?
+        next if email.downcase.end_with?(FAKE_ADDRESS_SUFFIX)
+        next unless seen.add?(email.downcase)
+        format_address(entry, email)
+      end
+    end
+
+    def self.format_address(entry, email)
+      name = entry.respond_to?(:name) ? entry.name.to_s.strip : ""
+      return email if name.blank? || (entry.respond_to?(:noname?) ? entry.noname? : name == "Noname")
+      addr = Mail::Address.new
+      addr.address = email
+      addr.display_name = name
+      # scraped data holds garbage addresses; keep raw email if mail gem mangles it
+      addr.address == email ? addr.format : email
+    rescue Mail::Field::ParseError
+      email
+    end
+
+    def self.dev_override
+      override = ENV["HACKORUM_DEV_REPLY_TO"]
+      raise MissingDevOverrideError if override.blank?
+      if MailingList.where("lower(post_address) = lower(?)", override).exists?
+        raise RealListAddressInDevError
       end
+      override
     end
+
+    private_class_method :pick, :format_address, :dev_override
   end
 end

app/views/drafts/confirm.html.slim

@@ -8,7 +8,11 @@
         .confirm-value = "#{@draft.sender_alias.name} <#{@draft.sender_alias.email}>"
       .confirm-row
         strong To
-        .confirm-value = @recipient
+        .confirm-value = @recipients.to.join(", ")
+      - if @recipients.cc.any?
+        .confirm-row
+          strong Cc
+          .confirm-value = @recipients.cc.join(", ")
       .confirm-row
         strong Subject
         .confirm-value = @draft.subject
@@ -24,6 +28,6 @@
       .confirm-actions
         = button_tag "Cancel", type: "button", class: "button-secondary",
                      data: { action: "click->send-confirmation#cancel" }
-        = button_to "Send to #{@recipient}", send_now_draft_path(@draft),
+        = button_to "Send to #{pluralize(@recipients.all.size, 'recipient')}", send_now_draft_path(@draft),
                     method: :post, class: "button-primary",
                     data: { send_confirmation_target: "submit", disabled: true }

spec/requests/drafts_controller_spec.rb

@@ -159,6 +159,7 @@
         get confirm_draft_path(draft)
         expect(response).to be_successful
         expect(response.body).to include('test@example.com')
+        expect(response.body).not_to include('<strong>Cc</strong>')
       end
     end
 
@@ -168,6 +169,19 @@
         expect(response).to have_http_status(:unprocessable_entity)
       end
     end
+
+    it 'renders To and Cc rows when the resolver returns cc recipients' do
+      allow(Outgoing::RecipientResolver).to receive(:for).and_return(
+        Outgoing::RecipientResolver::Recipients.new(
+          to: [ 'Bob <bob@x>' ], cc: [ 'list@x', 'Carol <carol@x>' ]))
+      get confirm_draft_path(draft)
+      expect(response).to be_successful
+      expect(response.body).to include('bob@x')
+      expect(response.body).to include('list@x')
+      expect(response.body).to include('<strong>Cc</strong>')
+      expect(response.body).to include('Send to 3 recipients')
+      expect(response.body).to include('Bob &lt;bob@x&gt;')
+    end
   end
 
   describe 'POST /drafts/:id/send_now' do

spec/services/outgoing/message_builder_spec.rb

@@ -11,9 +11,14 @@
            mailing_lists: [ list ])
   }
 
+  def recipients(to:, cc: [])
+    Outgoing::RecipientResolver::Recipients.new(to: to, cc: cc)
+  end
+
   before do
     allow(Outgoing::RecipientResolver).to receive(:for)
-      .with(topic).and_return('test@example.com')
+      .with(kind_of(OutgoingDraft))
+      .and_return(recipients(to: [ 'test@example.com' ]))
   end
 
   def build_draft(overrides = {})
@@ -80,10 +85,31 @@ def build_draft(overrides = {})
     end
   end
 
-  it 'returns the resolver result as recipient' do
+  it 'joins all recipients into Result#recipient' do
+    allow(Outgoing::RecipientResolver).to receive(:for)
+      .with(kind_of(OutgoingDraft))
+      .and_return(recipients(to: [ 'a@x' ], cc: [ 'list@x', 'c@x' ]))
     with_env('HACKORUM_OUTGOING_DOMAIN' => 'hackorum.local') do
       result = described_class.build(build_draft)
-      expect(result.recipient).to eq('test@example.com')
+      expect(result.recipient).to eq('a@x, list@x, c@x')
+    end
+  end
+
+  it 'sets To and Cc headers from the resolver' do
+    allow(Outgoing::RecipientResolver).to receive(:for)
+      .with(kind_of(OutgoingDraft))
+      .and_return(recipients(to: [ 'a@x' ], cc: [ 'list@x', 'c@x' ]))
+    with_env('HACKORUM_OUTGOING_DOMAIN' => 'hackorum.local') do
+      mail = Mail.new(described_class.build(build_draft).encoded)
+      expect(mail.to).to eq([ 'a@x' ])
+      expect(mail.cc).to eq([ 'list@x', 'c@x' ])
+    end
+  end
+
+  it 'omits the Cc header when there are no cc recipients' do
+    with_env('HACKORUM_OUTGOING_DOMAIN' => 'hackorum.local') do
+      mail = Mail.new(described_class.build(build_draft).encoded)
+      expect(mail.cc).to be_nil
     end
   end
 

spec/services/outgoing/recipient_resolver_spec.rb

@@ -1,68 +1,125 @@
 require 'rails_helper'
 
 RSpec.describe Outgoing::RecipientResolver do
-  let(:list)  { create(:mailing_list, post_address: "real@list.example") }
-  let(:topic) { create(:topic, mailing_lists: [ list ]) }
+  let(:list)   { create(:mailing_list, post_address: "real@list.example") }
+  let(:topic)  { create(:topic, mailing_lists: [ list ]) }
+  let(:author) { create(:alias, name: "Bob", email: "bob@x") }
+  let(:parent) { create(:message, topic: topic, sender: author) }
+  let(:sender) { create(:alias, name: "Alice", email: "alice@x") }
+  let(:draft)  {
+    create(:outgoing_draft, topic: topic, reply_to_message: parent, sender_alias: sender)
+  }
+
+  def mention(email, name = "Noname")
+    create(:mention, message: parent, alias: create(:alias, email: email, name: name))
+  end
 
   context 'in production' do
     before { allow(Rails.env).to receive(:production?).and_return(true) }
 
-    it 'returns the post_address' do
-      expect(described_class.for(topic)).to eq("real@list.example")
+    it 'puts the parent author in To and the list in Cc' do
+      result = described_class.for(draft)
+      expect(result.to).to eq([ 'Bob <bob@x>' ])
+      expect(result.cc).to eq([ 'real@list.example' ])
+    end
+
+    it 'adds parent participants to Cc after the list' do
+      mention('carol@x', 'Carol')
+      result = described_class.for(draft)
+      expect(result.cc).to eq([ 'real@list.example', 'Carol <carol@x>' ])
+    end
+
+    it 'uses the plain email when the participant name is Noname' do
+      mention('dave@x')
+      expect(described_class.for(draft).cc).to include('dave@x')
+    end
+
+    it 'falls back to the raw email when the address does not parse with a display name' do
+      mention('bob at example dot com', 'Weird')
+      expect(described_class.for(draft).cc).to include('bob at example dot com')
+    end
+
+    it 'excludes the sending alias from recipients' do
+      create(:mention, message: parent, alias: sender)
+      result = described_class.for(draft)
+      expect(result.all.join).not_to include('alice@x')
+    end
+
+    it 'dedups addresses case-insensitively' do
+      mention('BOB@X', 'Bobby')
+      mention('carol@x', 'Carol')
+      mention('CAROL@x', 'Caroline')
+      result = described_class.for(draft)
+      expect(result.to).to eq([ 'Bob <bob@x>' ])
+      expect(result.cc).to eq([ 'real@list.example', 'Carol <carol@x>' ])
+    end
+
+    it 'drops fabricated unknown.user addresses' do
+      mention('ghost@unknown.user', 'Ghost')
+      expect(described_class.for(draft).all.join).not_to include('unknown.user')
     end
 
-    it 'raises MissingPostAddressError when blank' do
+    it 'moves the list into To when replying to your own message' do
+      parent.update!(sender: sender)
+      mention('carol@x', 'Carol')
+      result = described_class.for(draft)
+      expect(result.to).to eq([ 'real@list.example' ])
+      expect(result.cc).to eq([ 'Carol <carol@x>' ])
+    end
+
+    it 'moves the list into To when the parent sender address is fabricated' do
+      parent.update!(sender: create(:alias, name: 'Ghost', email: 'ghost@unknown.user'))
+      result = described_class.for(draft)
+      expect(result.to).to eq([ 'real@list.example' ])
+    end
+
+    it 'raises MissingPostAddressError when post_address is blank' do
       list.update!(post_address: nil)
-      expect { described_class.for(topic) }
+      expect { described_class.for(draft) }
         .to raise_error(Outgoing::RecipientResolver::MissingPostAddressError)
     end
 
     it 'raises MissingPostAddressError when topic has no mailing list' do
       topic.mailing_lists.clear
-      expect { described_class.for(topic) }
+      expect { described_class.for(draft) }
         .to raise_error(Outgoing::RecipientResolver::MissingPostAddressError)
     end
   end
 
   context 'in development' do
-    it 'returns the override' do
+    it 'returns only the override in To with empty Cc' do
       with_env('HACKORUM_DEV_REPLY_TO' => 'test@example.com') do
-        expect(described_class.for(topic)).to eq('test@example.com')
+        result = described_class.for(draft)
+        expect(result.to).to eq([ 'test@example.com' ])
+        expect(result.cc).to eq([])
       end
     end
 
     it 'raises MissingDevOverrideError when override blank' do
       with_env('HACKORUM_DEV_REPLY_TO' => nil) do
-        expect { described_class.for(topic) }
+        expect { described_class.for(draft) }
           .to raise_error(Outgoing::RecipientResolver::MissingDevOverrideError)
       end
     end
 
     it 'raises MissingDevOverrideError when override is empty string' do
       with_env('HACKORUM_DEV_REPLY_TO' => '') do
-        expect { described_class.for(topic) }
+        expect { described_class.for(draft) }
           .to raise_error(Outgoing::RecipientResolver::MissingDevOverrideError)
       end
     end
 
     it 'raises RealListAddressInDevError when override matches a real list (case-insensitive)' do
       with_env('HACKORUM_DEV_REPLY_TO' => 'REAL@list.example') do
-        expect { described_class.for(topic) }
+        expect { described_class.for(draft) }
           .to raise_error(Outgoing::RecipientResolver::RealListAddressInDevError)
       end
     end
 
-    it 'returns the override even when the topic has no mailing list with post_address' do
+    it 'returns the override even when the list has no post_address' do
       list.update!(post_address: nil)
       with_env('HACKORUM_DEV_REPLY_TO' => 'test@example.com') do
-        expect(described_class.for(topic)).to eq('test@example.com')
-      end
-    end
-
-    it 'returns the override for topics with no mailing list at all' do
-      orphan_topic = create(:topic)
-      with_env('HACKORUM_DEV_REPLY_TO' => 'test@example.com') do
-        expect(described_class.for(orphan_topic)).to eq('test@example.com')
+        expect(described_class.for(draft).to).to eq([ 'test@example.com' ])
       end
     end
   end