Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Sending Stats API: DTOs for aggregated and grouped stats, a Mailtrap::StatsAPI client with aggregation and grouping endpoints (by domain/category/ESP/date), example usage, RSpec tests with VCR fixtures, and changelog/README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Mailtrap::StatsAPI
participant Remote as Mailtrap API (HTTP)
participant DTO as SendingStats / SendingStatGroup
Client->>API: call get(start_date,end_date, filters)
API->>Remote: GET /api/accounts/{id}/stats?start_date=...&end_date=...&filters...
Remote-->>API: 200 OK + JSON payload
API->>DTO: map JSON -> SendingStats / SendingStatGroup
API-->>Client: return DTO or Array[SendingStatGroup]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
c5a2318 to
bd40917
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/stats_api.rb (1)
3-5: Use ENV-based placeholders for runnable credentials.For executable examples, prefer
ENV.fetchforaccount_idandapi_keyto reduce accidental hardcoded-secret edits in local copies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/stats_api.rb` around lines 3 - 5, Replace hardcoded credentials with ENV-based placeholders: use ENV.fetch to obtain the API key and account id and convert the account id to an integer before constructing Mailtrap::StatsAPI. Update the account_id variable (currently "account_id = 3229"), the Mailtrap::Client.new call that passes api_key: 'your-api-key', and the Mailtrap::StatsAPI.new(account_id, client) usage to read ENV.fetch('MAILTRAP_ACCOUNT_ID') (to_i) and ENV.fetch('MAILTRAP_API_KEY') so the example is runnable without hardcoded secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/mailtrap/stats_api_spec.rb`:
- Around line 12-159: Add a cassette-backed spec that exercises the optional
array filters and [] serialization by calling the relevant API method (e.g.,
stats_api.get or a grouping method like stats_api.by_categories) with filter
params sending_domain_ids, sending_streams, categories, and
email_service_providers set to arrays; assert the request was recorded/played
(VCR cassette) and that the response mapping still succeeds, and additionally
verify the request query string uses the [] form (e.g.,
"sending_domain_ids[]=1") or that the client sent multiple param[] entries—use
the existing examples (subject(:stats) { ... }) and test helpers (match_struct)
to validate the returned stats while exercising these filters so query param
construction/regression is covered.
---
Nitpick comments:
In `@examples/stats_api.rb`:
- Around line 3-5: Replace hardcoded credentials with ENV-based placeholders:
use ENV.fetch to obtain the API key and account id and convert the account id to
an integer before constructing Mailtrap::StatsAPI. Update the account_id
variable (currently "account_id = 3229"), the Mailtrap::Client.new call that
passes api_key: 'your-api-key', and the Mailtrap::StatsAPI.new(account_id,
client) usage to read ENV.fetch('MAILTRAP_ACCOUNT_ID') (to_i) and
ENV.fetch('MAILTRAP_API_KEY') so the example is runnable without hardcoded
secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 123b0e56-2fa0-4011-b3ed-cec1ba30ca8d
📒 Files selected for processing (15)
CHANGELOG.mdREADME.mdexamples/stats_api.rblib/mailtrap.rblib/mailtrap/sending_stat_group.rblib/mailtrap/sending_stats.rblib/mailtrap/stats_api.rbspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_categories/returns_stats_grouped_by_categories.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_date/returns_stats_grouped_by_date.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_domains/returns_stats_grouped_by_domains.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_email_service_providers/returns_stats_grouped_by_email_service_providers.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/returns_aggregated_sending_stats.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/when_api_key_is_incorrect/raises_authorization_error.ymlspec/mailtrap/stats_api_spec.rbspec/spec_helper.rb
There was a problem hiding this comment.
♻️ Duplicate comments (1)
spec/mailtrap/stats_api_spec.rb (1)
27-49:⚠️ Potential issue | 🟡 MinorStill missing an assertion for
[]query serialization.This example exercises the filters, but it only checks response mapping. The PR’s main risk is the outgoing query shape (
sending_domain_ids[],sending_streams[], etc.), and that still is not asserted here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/stats_api_spec.rb` around lines 27 - 49, The test exercises stats_api.get with filter arrays but doesn't assert that array query params are serialized with trailing brackets (e.g. sending_domain_ids[], sending_streams[], categories[], email_service_providers[]); update the spec for stats_api.get to also assert the outgoing request/query string includes those bracketed keys — either by inspecting the stubbed HTTP request made by the client or by asserting the full generated query params on the request double used in the test so the spec ensures correct "[]"-style serialization for sending_domain_ids, sending_streams, categories and email_service_providers.
🧹 Nitpick comments (1)
spec/mailtrap/stats_api_spec.rb (1)
68-92: Avoid coupling grouped-spec assertions to response order.These examples all pin
first/last. If the API returns the same groups in a different order, the SDK is still correct but the suite fails. Prefer sorting byvaluefirst or using order-insensitive expectations.Also applies to: 98-123, 129-153, 159-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/stats_api_spec.rb` around lines 68 - 92, The test assumes a fixed response order by using stats.first/stats.last which makes it flaky; update the assertions to be order-insensitive by either sorting the stats array by the grouping key (e.g., sort by element[:value]) before asserting, or locate each group by its value and assert its contents (use stats.find { |s| s[:value] == 1 } and stats.find { |s| s[:value] == 2 }), and keep the existing match_struct checks (name: :sending_domain_id, value: ..., stats: ...) unchanged so the test validates group contents regardless of order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@spec/mailtrap/stats_api_spec.rb`:
- Around line 27-49: The test exercises stats_api.get with filter arrays but
doesn't assert that array query params are serialized with trailing brackets
(e.g. sending_domain_ids[], sending_streams[], categories[],
email_service_providers[]); update the spec for stats_api.get to also assert the
outgoing request/query string includes those bracketed keys — either by
inspecting the stubbed HTTP request made by the client or by asserting the full
generated query params on the request double used in the test so the spec
ensures correct "[]"-style serialization for sending_domain_ids,
sending_streams, categories and email_service_providers.
---
Nitpick comments:
In `@spec/mailtrap/stats_api_spec.rb`:
- Around line 68-92: The test assumes a fixed response order by using
stats.first/stats.last which makes it flaky; update the assertions to be
order-insensitive by either sorting the stats array by the grouping key (e.g.,
sort by element[:value]) before asserting, or locate each group by its value and
assert its contents (use stats.find { |s| s[:value] == 1 } and stats.find { |s|
s[:value] == 2 }), and keep the existing match_struct checks (name:
:sending_domain_id, value: ..., stats: ...) unchanged so the test validates
group contents regardless of order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ffca3dc-28fe-4244-b8d2-5be950b6e72f
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.mdlib/mailtrap/version.rbspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/with_optional_filters/returns_filtered_sending_stats.ymlspec/mailtrap/stats_api_spec.rb
✅ Files skipped from review due to trivial changes (1)
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/with_optional_filters/returns_filtered_sending_stats.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| require_relative 'mailtrap/inboxes_api' | ||
| require_relative 'mailtrap/sandbox_messages_api' | ||
| require_relative 'mailtrap/sandbox_attachments_api' | ||
| require_relative 'mailtrap/stats_api' |
There was a problem hiding this comment.
sending_stats and sending_stat_group are required in stats_api. thus no need to require them explicitly
| stats = Mailtrap::StatsAPI.new(account_id, client) | ||
|
|
||
| # Get aggregated sending stats | ||
| stats.get(start_date: '2026-01-01', end_date: '2026-01-31') |
There was a problem hiding this comment.
i believe the start and end dates should be of type #to_date:
stats.get(start_date: Date.today - 10, end_date: Date.today)In the code you can do date.to_date which should work for Date and Time. In RoR applications it should work with strings as well.
URI.encode_www_form({start_date: (Time.now - 10).to_date})
# => "start_date=2026-03-06"There was a problem hiding this comment.
In our client we are already doing
uri.query = URI.encode_www_form(query_params) if query_params.any?
so you can safely use Date in any query param
There was a problem hiding this comment.
require 'time'
account_id = 3229
client = Mailtrap::Client.new(api_key: 'your-api-key')
stats = Mailtrap::StatsAPI.new(account_id, client)
stats.get(start_date: Time.now, end_date: '2026-01-31')
# => https://mailtrap.io/api/accounts/3229/stats?start_date=2026-03-09+11%3A25%3A22+%2B0100&end_date=2026-01-31
lib/mailtrap/stats_api.rb
Outdated
| # @param email_service_providers [Array<String>] Filter by email service providers | ||
| # @return [Array<SendingStatGroup>] Array of SendingStatGroup structs with sending_domain_id and stats | ||
| # @!macro api_errors | ||
| def by_domains(start_date:, end_date:, sending_domain_ids: nil, sending_streams: nil, categories: nil, # rubocop:disable Metrics/ParameterLists |
There was a problem hiding this comment.
maybe it should be by_domain singular? you would normally say "group by domain"
applies to all by_ methods
There was a problem hiding this comment.
I'm not sure about this. I agree that with group by it sounds better, but in our API we have same naming convention - I think you should stick with it:
/api/accounts/{account_id}/stats
/api/accounts/{account_id}/stats/domains
/api/accounts/{account_id}/stats/categories
/api/accounts/{account_id}/stats/email_service_providers
/api/accounts/{account_id}/stats/date
There was a problem hiding this comment.
I agree that with group by it sounds better,
its sdk. it's a wrapper around our api. you have to look at the ruby code and make it "nice". the ruby code is the interface for other ruby devs.
| - h3=":443"; ma=86400 | ||
| body: | ||
| encoding: ASCII-8BIT | ||
| string: '[{"email_service_provider":"Gmail","stats":{"delivery_count":80,"delivery_rate":0.97,"bounce_count":2,"bounce_rate":0.03,"open_count":70,"open_rate":0.88,"click_count":35,"click_rate":0.5,"spam_count":1,"spam_rate":0.013}},{"email_service_provider":"Yahoo","stats":{"delivery_count":70,"delivery_rate":0.93,"bounce_count":6,"bounce_rate":0.07,"open_count":50,"open_rate":0.71,"click_count":25,"click_rate":0.5,"spam_count":1,"spam_rate":0.014}}]' |
6f96204 to
54c2421
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 1-3: The changelog currently marks version header "## [2.9.0] -
2026-03-06" as a dated release but this entry must remain unreleased; locate the
header "## [2.9.0] - 2026-03-06" (the section containing "Add Sending Stats
API") and either remove the date or move the bullet under an "## [Unreleased]"
section so it is not listed as a published release (for example change the
header to "## [2.9.0] - Unreleased" or place the "Add Sending Stats API" entry
under an existing Unreleased header).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c8382c7-60d3-4a0d-b1e0-b3561bd19688
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
CHANGELOG.mdlib/mailtrap.rblib/mailtrap/version.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap.rb
54c2421 to
430864f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/mailtrap/stats_api_spec.rb (1)
68-92: AssertSendingStatGroupexplicitly in the grouped examples.
match_structalso accepts hash-like objects (spec/spec_helper.rb:69-107), so these examples would still pass ifby_*regressed from returningMailtrap::SendingStatGroupinstances to plain hashes. Adding a type assertion keeps the new public DTO contract covered.💡 Suggested test hardening
it 'returns stats grouped by domain' do + expect(stats).to all(be_a(Mailtrap::SendingStatGroup)) expect(stats.size).to eq(2) @@ it 'returns stats grouped by category' do + expect(stats).to all(be_a(Mailtrap::SendingStatGroup)) expect(stats.size).to eq(2) @@ it 'returns stats grouped by email service provider' do + expect(stats).to all(be_a(Mailtrap::SendingStatGroup)) expect(stats.size).to eq(2) @@ it 'returns stats grouped by date' do + expect(stats).to all(be_a(Mailtrap::SendingStatGroup)) expect(stats.size).to eq(2)Also applies to: 98-123, 129-153, 159-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/stats_api_spec.rb` around lines 68 - 92, The grouped examples currently only use match_struct and can pass if the group objects degrade to plain hashes; add an explicit type assertion that the returned group objects are Mailtrap::SendingStatGroup (e.g. expect(stats.first).to be_a(Mailtrap::SendingStatGroup>) and same for stats.last and the other grouped examples referenced) so the spec verifies the DTO class as well as its shape; update the examples at the shown block and the other ranges (98-123, 129-153, 159-183) to include this type check alongside the existing match_struct assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/mailtrap/stats_api.rb`:
- Around line 3-5: Add a top-level require for Ruby's stdlib date by inserting
require 'date' alongside the existing requires, and update the normalize_date
method to use strict parsing instead of Date.parse: replace Date.parse(...) with
Date.strptime(date_string, '%Y-%m-%d') and still rescue Date::Error (and handle
nil/empty input the same way) so only exact YYYY-MM-DD strings are accepted;
reference the normalize_date method and Date::Error in your change to tighten
validation.
---
Nitpick comments:
In `@spec/mailtrap/stats_api_spec.rb`:
- Around line 68-92: The grouped examples currently only use match_struct and
can pass if the group objects degrade to plain hashes; add an explicit type
assertion that the returned group objects are Mailtrap::SendingStatGroup (e.g.
expect(stats.first).to be_a(Mailtrap::SendingStatGroup>) and same for stats.last
and the other grouped examples referenced) so the spec verifies the DTO class as
well as its shape; update the examples at the shown block and the other ranges
(98-123, 129-153, 159-183) to include this type check alongside the existing
match_struct assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17886a7c-083d-47a4-bbd0-99fb7b2850ce
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdexamples/stats_api.rblib/mailtrap.rblib/mailtrap/sending_stat_group.rblib/mailtrap/sending_stats.rblib/mailtrap/stats_api.rbspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_category/returns_stats_grouped_by_category.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_date/returns_stats_grouped_by_date.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_domain/returns_stats_grouped_by_domain.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_email_service_provider/returns_stats_grouped_by_email_service_provider.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/returns_aggregated_sending_stats.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/when_api_key_is_incorrect/raises_authorization_error.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/with_optional_filters/returns_filtered_sending_stats.ymlspec/mailtrap/stats_api_spec.rbspec/spec_helper.rb
✅ Files skipped from review due to trivial changes (2)
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_category/returns_stats_grouped_by_category.yml
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_domain/returns_stats_grouped_by_domain.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_date/returns_stats_grouped_by_date.yml
- spec/spec_helper.rb
- examples/stats_api.rb
- CHANGELOG.md
- README.md
- lib/mailtrap.rb
- lib/mailtrap/sending_stats.rb
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/returns_aggregated_sending_stats.yml
430864f to
3933b2d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/mailtrap/stats_api.rb (1)
130-131: Consider documenting timezone behavior for Time inputs.
strftime('%F')uses the local timezone of the Time object. If a user passes a Time near midnight in a non-UTC timezone, the resulting date string may differ from their expectation. This is fine behavior, but a brief note in the YARD docs forstart_date/end_dateparameters could help users understand this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mailtrap/stats_api.rb` around lines 130 - 131, The code path that handles Time inputs uses value.strftime('%F') which formats the date using the Time object's zone (local time), so update the YARD docs for the start_date and end_date parameters (and any related method in StatsApi) to state that Time inputs are interpreted in their own timezone and may produce a different ISO date near midnight in non-UTC zones; suggest callers pass a Date, a UTC Time (time.getutc), or convert via time.to_date if they want timezone-agnostic behavior, and mention the Time branch that calls strftime('%F') so readers can find the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/mailtrap/stats_api.rb`:
- Around line 130-131: The code path that handles Time inputs uses
value.strftime('%F') which formats the date using the Time object's zone (local
time), so update the YARD docs for the start_date and end_date parameters (and
any related method in StatsApi) to state that Time inputs are interpreted in
their own timezone and may produce a different ISO date near midnight in non-UTC
zones; suggest callers pass a Date, a UTC Time (time.getutc), or convert via
time.to_date if they want timezone-agnostic behavior, and mention the Time
branch that calls strftime('%F') so readers can find the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20586c3e-6ef5-4028-a4d6-48f20204414e
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdexamples/stats_api.rblib/mailtrap.rblib/mailtrap/sending_stat_group.rblib/mailtrap/sending_stats.rblib/mailtrap/stats_api.rbspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_category/returns_stats_grouped_by_category.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_date/returns_stats_grouped_by_date.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_domain/returns_stats_grouped_by_domain.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_email_service_provider/returns_stats_grouped_by_email_service_provider.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/returns_aggregated_sending_stats.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/when_api_key_is_incorrect/raises_authorization_error.ymlspec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/with_optional_filters/returns_filtered_sending_stats.ymlspec/mailtrap/stats_api_spec.rbspec/spec_helper.rb
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_get/with_optional_filters/returns_filtered_sending_stats.yml
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_domain/returns_stats_grouped_by_domain.yml
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_category/returns_stats_grouped_by_category.yml
- CHANGELOG.md
- lib/mailtrap/sending_stat_group.rb
- lib/mailtrap.rb
- spec/fixtures/vcr_cassettes/Mailtrap_StatsAPI/_by_email_service_provider/returns_stats_grouped_by_email_service_provider.yml

Motivation
Changes
How to test
Mailtrap::StatsAPI.new(account_id, client).getmethod with different parameters (start_date, end_date, sending_domain_ids[], sending_streams[], categories[], email_service_providers[])Examples
Summary by CodeRabbit
New Features
Documentation
Examples
Tests