Conversation
📝 WalkthroughWalkthroughAdds Sending Domains feature: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Mailtrap::SendingDomainsAPI
participant Server as mailtrap.io API
Client->>API: create(sending_domain)
API->>Server: POST /api/accounts/{account_id}/sending_domains
Server-->>API: 200 OK (sending_domain JSON)
API-->>Client: SendingDomain
Client->>API: list()
API->>Server: GET /api/accounts/{account_id}/sending_domains
Server-->>API: 200 OK [sending_domain...]
API-->>Client: [SendingDomain]
Client->>API: get(id)
API->>Server: GET /api/accounts/{account_id}/sending_domains/{id}
Server-->>API: 200 OK (sending_domain JSON)
API-->>Client: SendingDomain
Client->>API: send_setup_instructions(id, email)
API->>Server: POST /api/accounts/{account_id}/sending_domains/{id}/send_setup_instructions
Server-->>API: 204 No Content
API-->>Client: nil
Client->>API: delete(id)
API->>Server: DELETE /api/accounts/{account_id}/sending_domains/{id}
Server-->>API: 204 No Content
API-->>Client: nil
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
i7an
reviewed
Jan 7, 2026
lib/mailtrap/sending_domains_api.rb
Outdated
| # @return nil | ||
| # @!macro api_errors | ||
| def send_setup_instructions(domain_id, email) | ||
| client.post("#{base_path}/#{domain_id}/send_setup_instructions", { email: email }) |
lib/mailtrap/sending_domains_api.rb
Outdated
| # @param email [String] The email for instructions | ||
| # @return nil | ||
| # @!macro api_errors | ||
| def send_setup_instructions(domain_id, email) |
Contributor
There was a problem hiding this comment.
lets use kwargs for email for consistency. see list(email: nil) in suppressions api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@examples/sending_domains_api.rb`:
- Around line 23-25: The example calls the wrong method: replace the call to
sending_domains.clean with sending_domains.send_setup_instructions so it matches
the API spec; update the invocation that currently passes sending_domain.id and
'jonathan@mail.com' to call send_setup_instructions(sending_domain.id,
'jonathan@mail.com') and ensure the example comment remains accurate.
- Line 5: The example instantiates the wrong API class: replace the
instantiation of Mailtrap::InboxesAPI with Mailtrap::SendingDomainsAPI for the
sending_domains variable so it uses the correct client; update the line that
creates sending_domains (currently referencing Mailtrap::InboxesAPI) to use
Mailtrap::SendingDomainsAPI while keeping the account_id and client arguments
unchanged.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_send_setup_instructions/returns_nil.yml`:
- Around line 6-8: The cassette contains a real developer email in the recorded
request body; replace the literal string "yahor.vaitsiakhouski@railsware.com" in
the fixture content
(spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_send_setup_instructions/returns_nil.yml
— the body.string value) with an anonymized test address like "test@example.com"
to remove PII while preserving the fixture structure.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_send_setup_instructions/when_sending_domain_does_not_exist/raises_not_found_error.yml`:
- Around line 6-8: The fixture contains a real email in the VCR cassette
body.string; update the value '{"email":"yahor.vaitsiakhouski@railsware.com"}'
to use a placeholder/test address (e.g. test@example.com) so the body.string
entry in the Mailtrap_SendingDomainsAPI cassette no longer exposes a real
developer email.
🧹 Nitpick comments (4)
lib/mailtrap/sending_domains_api.rb (1)
53-55: Consider validating the email parameter.The
♻️ Optional: Add email presence validation
def send_setup_instructions(domain_id, email) + raise ArgumentError, 'email is required' if email.nil? || email.to_s.strip.empty? + client.post("#{base_path}/#{domain_id}/send_setup_instructions", { email: email }) endspec/mailtrap/sending_domain_spec.rb (2)
33-42: Remove unusedprojectvariable.The
projectvariable is defined but never used in the test. The actual test usessending_domaindefined in the context block. This appears to be leftover code, possibly from copy-paste.🧹 Proposed fix to remove dead code
describe '#to_h' do subject(:hash) { sending_domain.to_h } - let(:project) do - described_class.new( - id: '123456', - domain_name: 'My Sending Domain', - dns_verified: false, - compliance_status: 'pending', - created_at: '2024-01-01T00:00:00Z', - updated_at: '2024-01-02T00:00:00Z' - ) - end - context 'when some attributes are nil' do
44-64: Consider adding test coverage forto_hwhen all attributes are present.The
#to_htests only cover the case when some attributes are nil. Consider adding a test case verifying that all attributes are included in the hash when none are nil.💡 Suggested additional test case
context 'when all attributes are present' do let(:sending_domain) do described_class.new( id: '123456', domain_name: 'My Sending Domain', dns_verified: false, compliance_status: 'pending', created_at: '2024-01-01T00:00:00Z', updated_at: '2024-01-02T00:00:00Z' ) end it 'returns a hash with all attributes' do expect(hash).to eq( id: '123456', domain_name: 'My Sending Domain', dns_verified: false, compliance_status: 'pending', created_at: '2024-01-01T00:00:00Z', updated_at: '2024-01-02T00:00:00Z' ) end endspec/mailtrap/sending_domains_api_spec.rb (1)
125-127: Test description doesn't match the expectation.The test is named
'returns deleted sending domain data'but it expectsnil. Consider renaming to accurately describe the behavior, such as'returns nil on successful deletion'.📝 Proposed fix
- it 'returns deleted sending domain data' do + it 'returns nil on successful deletion' do expect(delete).to be_nil end
Comment on lines
6
to
8
| body: | ||
| encoding: UTF-8 | ||
| string: '{"email":"yahor.vaitsiakhouski@railsware.com"}' |
There was a problem hiding this comment.
Same privacy concern: consider anonymizing the email address.
As noted in the other cassette, replace the developer email with a test email like test@example.com.
🤖 Prompt for AI Agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_send_setup_instructions/returns_nil.yml`
around lines 6 - 8, The cassette contains a real developer email in the recorded
request body; replace the literal string "yahor.vaitsiakhouski@railsware.com" in
the fixture content
(spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_send_setup_instructions/returns_nil.yml
— the body.string value) with an anonymized test address like "test@example.com"
to remove PII while preserving the fixture structure.
Comment on lines
6
to
8
| body: | ||
| encoding: UTF-8 | ||
| string: '{"email":"yahor.vaitsiakhouski@railsware.com"}' |
There was a problem hiding this comment.
Consider anonymizing the email address in test fixtures.
The request body contains what appears to be a real developer email address. For privacy and to avoid potential spam, consider replacing it with a test/placeholder email like test@example.com.
🤖 Prompt for AI Agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_send_setup_instructions/when_sending_domain_does_not_exist/raises_not_found_error.yml`
around lines 6 - 8, The fixture contains a real email in the VCR cassette
body.string; update the value '{"email":"yahor.vaitsiakhouski@railsware.com"}'
to use a placeholder/test address (e.g. test@example.com) so the body.string
entry in the Mailtrap_SendingDomainsAPI cassette no longer exposes a real
developer email.
DagonWat
force-pushed
the
MT-19854-sending-domains
branch
from
c5248e9 to
465d4f4
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@examples/sending_domains_api.rb`:
- Around line 23-25: Replace the real-looking example email passed to
sending_domains.send_setup_instructions (call site uses sending_domain.id and
'jonathan@mail.com') with a non-real placeholder such as 'user@example.com' or
'user@example.test' so the example stays safe and generic; update the argument
to the send_setup_instructions call accordingly.
- Around line 15-17: Add a public update wrapper to SendingDomainsAPI so
examples calling sending_domains.update(...) don't raise NoMethodError:
implement a method named update in the SendingDomainsAPI class that accepts
domain_id and options and delegates to the existing base_update(domain_id,
options), matching other API operation wrappers.
In `@lib/mailtrap/sending_domain.rb`:
- Around line 4-25: The SendingDomain DTO (Struct named SendingDomain) omits API
fields and silently drops data; add :dns_records, :dns_verified_at, and
:permissions to the Struct.new(...) declaration (keeping keyword_init: true) so
incoming API responses map fully to the DTO, and ensure the existing to_h method
continues to return the compacted hash of all attributes.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_get/when_sending_domain_does_not_exist/raises_not_found_error.yml`:
- Around line 5-70: The cassette contains sensitive account and trace/request
IDs (e.g. account "1111111" in the URI and X-Request-Id / X-Cloud-Trace-Context
/ Cf-Ray headers) — update your VCR configuration (e.g. in your VCR.configure
block in spec helper or spec/support/vcr.rb) to scrub these before recording:
add VCR.filter_sensitive_data('<ACCOUNT_ID>') { '1111111' } or a regex-based
replacement for account IDs in request URIs, and add filters or a before_record
hook to replace header values like X-Request-Id, X-Cloud-Trace-Context and
Cf-Ray with placeholders (e.g. '<X_REQUEST_ID>', '<TRACE_CONTEXT>', '<CF_RAY>');
re-run tests to regenerate the cassette with redacted values and commit the
updated fixture
(spec/fixtures/vcr_cassettes/Mailtrap_SendingDomainsAPI/_get/when_sending_domain_does_not_exist/raises_not_found_error.yml).
🧹 Nitpick comments (1)
spec/mailtrap/sending_domains_api_spec.rb (1)
9-27: Consider asserting additional mapped fields.Once the DTO includes
dns_recordsandpermissions, it would be valuable to assert these in the list mapping so regressions are caught early.
...ailtrap_SendingDomainsAPI/_get/when_sending_domain_does_not_exist/raises_not_found_error.yml
Show resolved
Hide resolved
Merged
IgorDobryn
approved these changes
Jan 28, 2026
i7an
force-pushed
the
coderabbit-config
branch
from
3b328ba to
aae72b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@examples/sending_domains_api.rb`:
- Around line 15-17: Update the stale example comment after the call to
sending_domains.get so it reflects the actual created domain; replace
domain_name="proper.com" with domain_name="example.com" (or remove the example
output line) to match the created resource and avoid confusion—look for the
sending_domain variable and the sending_domains.get(sending_domain.id) call to
locate the line to change.
i7an
reviewed
Feb 3, 2026
DagonWat
force-pushed
the
MT-19854-sending-domains
branch
from
7c9b7b8 to
eb251ad
Compare
i7an
approved these changes
Feb 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Adding Sending Domains API for Ruby SDK
Changes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores