Conversation
…bug-in-mailtrap-gem Map Reply-To into structured field and strip Reply-To header from payload
…bug-in-mailtrap-gem-itmsm1 test: cover folded line-break reply-to parsing error
📝 WalkthroughWalkthroughAdds Reply-To handling: includes 'replyto' in Mailtrap::Mail::SPECIAL_HEADERS and maps message['reply-to'] into the mail object's structured Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as Client (Message)
participant Mail as Mailtrap::Mail (Base)
participant API as Mailtrap API
Sender->>Mail: provide headers (including Reply-To)
Mail->>Mail: parse headers (from,to,cc,bcc, reply-to)
Note right of Mail: `'replyto'` in SPECIAL_HEADERS\nreply_to set to first parsed address
Mail->>API: POST /send with JSON body including reply_to: { email, name }
API-->>Mail: 200 OK
Mail-->>Sender: delivery result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/mailtrap/mail_spec.rb (1)
186-197: Duplicate test contexts for invalidreply-to.The test at lines 186-197 and lines 212-224 both test the same scenario: an invalid
'Reply-To': 'invalid email@example.com'raisingMailtrap::Errorwith the same message. Consider consolidating these into a single context block with the folded line break case nested within it.♻️ Suggested consolidation
Remove lines 186-197 and keep only the block at lines 212-241, which already covers both the basic invalid case and the folded line break edge case.
- context "when 'reply-to' is invalid" do - before do - message.header['Reply-To'] = 'invalid email@example.com' - end - - it 'raises an error' do - expect { mail }.to raise_error( - Mailtrap::Error, - "failed to parse 'Reply-To': 'invalid email@example.com'" - ) - end - end - %i[from to cc bcc].each do |header|Also applies to: 212-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/mail_spec.rb` around lines 186 - 197, There are duplicate RSpec contexts named "when 'reply-to' is invalid" testing the same invalid Reply-To value; remove the earlier duplicate context (the one that sets message.header['Reply-To'] = 'invalid email@example.com' and its expect) and keep the later context that already covers both the basic invalid case and the folded line-break edge case (the nested case that manipulates message.header['Reply-To'] and expects Mailtrap::Error with the failure message); alternatively, consolidate by merging the simple invalid-value example into the later context as a nested example so only one context named "when 'reply-to' is invalid" remains and both assertions (simple invalid and folded line break) live under it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/mailtrap/mail_spec.rb`:
- Around line 186-197: There are duplicate RSpec contexts named "when 'reply-to'
is invalid" testing the same invalid Reply-To value; remove the earlier
duplicate context (the one that sets message.header['Reply-To'] = 'invalid
email@example.com' and its expect) and keep the later context that already
covers both the basic invalid case and the folded line-break edge case (the
nested case that manipulates message.header['Reply-To'] and expects
Mailtrap::Error with the failure message); alternatively, consolidate by merging
the simple invalid-value example into the later context as a nested example so
only one context named "when 'reply-to' is invalid" remains and both assertions
(simple invalid and folded line break) live under it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 274dddc9-cd03-4644-9a18-84c358f9ebd6
📒 Files selected for processing (3)
lib/mailtrap/mail.rbspec/fixtures/vcr_cassettes/Mailtrap_ActionMailer_DeliveryMethod/_deliver_/converts_the_message_and_sends_via_API.ymlspec/mailtrap/mail_spec.rb
|
@ryush00 I see the problem. Will have to test the changes. Thanks. |
| body: | ||
| encoding: UTF-8 | ||
| string: '{"from":{"email":"mailtrap@mailtrap.io","name":"Mailtrap Test"},"to":[{"email":"to_1@railsware.com","name":"To 1"},{"email":"to_2@railsware.com"}],"cc":[{"email":"cc_1@railsware.com"},{"email":"cc_2@railsware.com","name":"Cc 2"}],"bcc":[{"email":"bcc_1@railsware.com"},{"email":"bcc_2@railsware.com"}],"subject":"You are awesome!","text":"Some text","html":"<div>HTML part</div>","attachments":[{"content":"VGhpcyBpcyBhIHRleHQgZmlsZQo=","type":"text/plain","filename":"file.txt","disposition":"attachment","content_id":"txt_content_id@test.mail"},{"content":"iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAABiElEQVRIid3VSW5UMRCA4U8d0gIx7AjhAghBTgFICAkxRFHukDDDIRiuwI7xPNAMISQRV4CABFmkWbie2nrt9muaFZTkxavhr7Jfucz/Ln2s4hU28B0/sBO6VczPCl/GNoYd6wsuZ3ELWKuBe3iSAQa4g7M4jEM4jRt4g5+4lMEHEbc+KUED/xVOvY5iThXgg/gek+UMfq4CbstU8L7RmU/c3qxwUkc0TnN/CT9Ycn4djrenhB/H24j5iMXQX8TTUsCncD6T6daUtzyp8hNSV30uJfgWAUfje70AqMFF7FC6kGOy20rQPoKTBd1ii3EsbF9LCUpH1K62q1uWwr5RSvAyjHdb+rzqSZU38iB8npWMTZu+M96mzU5qfT6H98FYKTn0sRUONwv2hQqcNK+G2FSZsNeNRsX5CqwtF7CHfVzpcn6cJbmlfqsPSJXvRczDaarpZUmaf3JP6pAjsZZw3+jM9/FIffKOyTXpRnY9OJu4+ifgXOaljnghtedurA94HraZn8x/Q34DYaON8Fk9Z1IAAAAASUVORK5CYII=","type":"image/png","filename":"file.png","disposition":"inline","content_id":"png_content_id@test.mail"}],"headers":{"Reply-To":"reply-to@railsware.com","X-Special-Domain-Specific-Header":"SecretValue","One-more-custom-header":"CustomValue"},"category":"Module Test"}' | ||
| string: '{"from":{"email":"mailtrap@mailtrap.io","name":"Mailtrap Test"},"to":[{"email":"to_1@railsware.com","name":"To 1"},{"email":"to_2@railsware.com"}],"reply_to":{"email":"reply-to@railsware.com"},"cc":[{"email":"cc_1@railsware.com"},{"email":"cc_2@railsware.com","name":"Cc 2"}],"bcc":[{"email":"bcc_1@railsware.com"},{"email":"bcc_2@railsware.com"}],"subject":"You are awesome!","text":"Some text","html":"<div>HTML part</div>","attachments":[{"content":"VGhpcyBpcyBhIHRleHQgZmlsZQo=","type":"text/plain","filename":"file.txt","disposition":"attachment","content_id":"txt_content_id@test.mail"},{"content":"iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABmJLR0QA/wD/AP+gvaeTAAABiElEQVRIid3VSW5UMRCA4U8d0gIx7AjhAghBTgFICAkxRFHukDDDIRiuwI7xPNAMISQRV4CABFmkWbie2nrt9muaFZTkxavhr7Jfucz/Ln2s4hU28B0/sBO6VczPCl/GNoYd6wsuZ3ELWKuBe3iSAQa4g7M4jEM4jRt4g5+4lMEHEbc+KUED/xVOvY5iThXgg/gek+UMfq4CbstU8L7RmU/c3qxwUkc0TnN/CT9Ycn4djrenhB/H24j5iMXQX8TTUsCncD6T6daUtzyp8hNSV30uJfgWAUfje70AqMFF7FC6kGOy20rQPoKTBd1ii3EsbF9LCUpH1K62q1uWwr5RSvAyjHdb+rzqSZU38iB8npWMTZu+M96mzU5qfT6H98FYKTn0sRUONwv2hQqcNK+G2FSZsNeNRsX5CqwtF7CHfVzpcn6cJbmlfqsPSJXvRczDaarpZUmaf3JP6pAjsZZw3+jM9/FIffKOyTXpRnY9OJu4+ifgXOaljnghtedurA94HraZn8x/Q34DYaON8Fk9Z1IAAAAASUVORK5CYII=","type":"image/png","filename":"file.png","disposition":"inline","content_id":"png_content_id@test.mail"}],"headers":{"X-Special-Domain-Specific-Header":"SecretValue","One-more-custom-header":"CustomValue"},"category":"Module Test"}' |
There was a problem hiding this comment.
reply-to address was moved from headers to the body
| to: prepare_addresses(message['to']), | ||
| cc: prepare_addresses(message['cc']), | ||
| bcc: prepare_addresses(message['bcc']), | ||
| reply_to: prepare_addresses(message['reply-to']).first, |
There was a problem hiding this comment.
Reply-to in the raw email.
Before:
>=?UTF-8?q?"=EB=A7=88=EC=9D=B8=EB=A6=AC=EC=8A=A4=ED=8A=B8"_<support@mailtr?==?UTF-8?q?ap.io>?=
After:
=?utf-8?q?=EB=A7=88=EC=9D=B8=EB=A6=AC=EC=8A=A4=ED=8A=B8?=<support@mailtrap.io>
spec/mailtrap/mail_spec.rb
Outdated
| its(:template_variables) { is_expected.to eq('first_name' => 'John') } | ||
| end | ||
|
|
||
| context "when 'reply-to' is invalid" do |
There was a problem hiding this comment.
the tests should be refactored. I can do that once merged.
- this should be merged with the next block.
spec/mailtrap/mail_spec.rb
Outdated
| end | ||
|
|
||
| context 'when address contains a folded line break' do | ||
| let(:invalid_reply_to) do |
There was a problem hiding this comment.
could you please explain this case?
There was a problem hiding this comment.
I misunderstood RFC 2047. It doesn’t seem necessary here. Instead, after reading the part that says the address part must not be RFC 2047-encoded, I’ll remove this test and update it to strengthen coverage around that rule.
|
I initially misunderstood the issue and assumed the folded / domain-splitting encoding was being generated in this gem. Because of that, I removed the folding test I had added earlier. If Reply-To was becoming malformed while going through the headers path, that seems more like a backend-side issue than |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/mailtrap/mail_spec.rb (2)
89-99: Consider clarifying test setup to avoid implicit header accumulation.In this nested context, the outer
beforeblock (line 82) already setsmessage.reply_to, so when the innerbeforeruns, there are actually four Reply-To headers total. The test passes because the Mail gem'smessage['reply-to']returns the last header added ('lower-reply-to@railsware.com').This behavior is valid to test, but the implicit header accumulation might confuse future readers. Consider either:
- Adding a comment explaining the test verifies "last header wins" behavior
- Or resetting reply_to in the inner before to make the test self-contained
Option: Add clarifying comment
context 'when reply-to header variants are present' do before do + # Note: outer before already set one reply-to; Mail gem returns last header added message.header['Reply-To'] = 'Reply To <reply-to@railsware.com>' message.header['REPLY-TO'] = 'Upper Case <upper-reply-to@railsware.com>' message.header['reply-to'] = 'Lower Case <lower-reply-to@railsware.com>' end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/mail_spec.rb` around lines 89 - 99, The test's inner before block adds three Reply-To headers on top of the outer before's message.reply_to, causing header accumulation; either make the nested context self-contained by clearing/resetting reply_to before adding the three headers (e.g., call message.header['Reply-To'] = nil or reset message.reply_to) or add a concise comment above the inner before explaining this test intentionally exercises "last header wins" behavior; update references in the spec around message.reply_to, message.header['Reply-To'], and mail.reply_to accordingly so future readers understand why multiple headers are present.
56-64: Minor: Test description could be more specific.The context description says "varying formats" but only tests a single format (quoted non-ASCII display name). Consider renaming to something more specific like
'when reply-to has non-ASCII display name', or expand to test multiple formats within this context if "varying formats" coverage is intended.That said, the test correctly verifies that Reply-To is stripped from custom headers regardless of encoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/mail_spec.rb` around lines 56 - 64, The context description is misleading—rename the RSpec context from 'when reply-to is added in varying formats' to a specific description like 'when reply-to has non-ASCII display name' (the surrounding context block and its before hook that sets message.reply_to = '"메일트랩" <support@mailtrap.io>') or alternatively add additional examples within the same context to cover other formats (e.g., plain address and encoded formats) so the description matches coverage; update only the context string (and/or add extra it blocks) to keep test intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/mailtrap/mail_spec.rb`:
- Around line 89-99: The test's inner before block adds three Reply-To headers
on top of the outer before's message.reply_to, causing header accumulation;
either make the nested context self-contained by clearing/resetting reply_to
before adding the three headers (e.g., call message.header['Reply-To'] = nil or
reset message.reply_to) or add a concise comment above the inner before
explaining this test intentionally exercises "last header wins" behavior; update
references in the spec around message.reply_to, message.header['Reply-To'], and
mail.reply_to accordingly so future readers understand why multiple headers are
present.
- Around line 56-64: The context description is misleading—rename the RSpec
context from 'when reply-to is added in varying formats' to a specific
description like 'when reply-to has non-ASCII display name' (the surrounding
context block and its before hook that sets message.reply_to = '"메일트랩"
<support@mailtrap.io>') or alternatively add additional examples within the same
context to cover other formats (e.g., plain address and encoded formats) so the
description matches coverage; update only the context string (and/or add extra
it blocks) to keep test intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9561ded7-6e00-4673-9c92-b32a8d8fe295
📒 Files selected for processing (1)
spec/mailtrap/mail_spec.rb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/mailtrap/mail_spec.rb (1)
89-99: Clarify intent of multi-header variant test.This test documents behavior when multiple Reply-To headers with different cases coexist. The expectation that the lowercase variant (
'reply-to') wins depends on how the Mail gem handles duplicate headers internally.Consider adding a brief comment explaining why this edge case matters (e.g., potential for malformed messages from external sources) or whether this is documenting a contract you intend to maintain vs. incidental Mail gem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/mail_spec.rb` around lines 89 - 99, Add a short clarifying comment inside the context 'when reply-to header variants are present' describing why the test asserts that the lowercase 'reply-to' header wins (e.g., whether this is a deliberate contract your code must preserve versus an incidental Mail gem behavior or to document handling of malformed external messages), placing the comment near the message.header assignments (the context block and the message.header[...] lines) and referencing the expected outcome asserted by expect(mail.reply_to) to make the intent explicit for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/mailtrap/mail_spec.rb`:
- Around line 89-99: Add a short clarifying comment inside the context 'when
reply-to header variants are present' describing why the test asserts that the
lowercase 'reply-to' header wins (e.g., whether this is a deliberate contract
your code must preserve versus an incidental Mail gem behavior or to document
handling of malformed external messages), placing the comment near the
message.header assignments (the context block and the message.header[...] lines)
and referencing the expected outcome asserted by expect(mail.reply_to) to make
the intent explicit for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4ec97a4-1fdd-4ff7-8998-296790bc7664
📒 Files selected for processing (1)
spec/mailtrap/mail_spec.rb
3 participants
Motivation
Some users sending to Yahoo reported bounces like:
In our ActionMailer conversion path,
Reply-Tocould remain inheadersinstead of being represented as structuredreply_to.This risks inconsistent header serialization/parsing for edge cases (including folded/encoded header forms).
What changed
1) Map
Reply-Tointo structured payload fieldMailtrap::Mail.from_messagenow parsesmessage['reply-to']and sets:reply_to: prepare_addresses(message['reply-to']).firstReply-Tohandling with other structured address fields at payload build time.2) Remove
Reply-Tofrom custom headersreplytoto the headers-removal list (SPECIAL_HEADERS→HEADERS_TO_REMOVEpath),so
Reply-Todoes not remain duplicated inheaders.3) Test coverage improvements
Updated
spec/mailtrap/mail_spec.rbto verify:reply_tomappingReply-Toheader removal (including case variants:Reply-To,REPLY-TO,reply-to)Reply-Toerror behaviorWhy this is safe
from/to/cc/bcc/subject/text/html/attachmentsis unchanged.reply_totype remains compatible with currentBasepayload shape (single object).How to test
Notes for reviewers
Summary by CodeRabbit
New Features
Tests