Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces Email Logs API support to the Mailtrap Ruby library, adding a new API client for listing and retrieving email logs with filtering and cursor-based pagination, along with comprehensive data transfer objects and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant EmailLogsAPI
participant HTTPClient as HTTP Client
participant APIServer as API Server
participant Mapper as Response Mapper
Client->>EmailLogsAPI: list(filters:, search_after:)
EmailLogsAPI->>EmailLogsAPI: build_list_query_params(filters, search_after)
EmailLogsAPI->>HTTPClient: GET /api/accounts/{id}/email_logs?filters[...]...
HTTPClient->>APIServer: HTTP GET request
APIServer-->>HTTPClient: 200 OK with messages array
HTTPClient-->>EmailLogsAPI: Response body
EmailLogsAPI->>Mapper: Map to EmailLogsListResponse
Mapper->>Mapper: Build EmailLogMessage objects
Mapper->>Mapper: Build EmailLogEvent & EmailLogEventDetails
Mapper-->>EmailLogsAPI: EmailLogsListResponse instance
EmailLogsAPI-->>Client: EmailLogsListResponse with messages, total_count, next_page_cursor
sequenceDiagram
actor Client
participant EmailLogsAPI
participant HTTPClient as HTTP Client
participant APIServer as API Server
participant Mapper as Response Mapper
Client->>EmailLogsAPI: get(sending_message_id)
EmailLogsAPI->>HTTPClient: GET /api/accounts/{id}/email_logs/{message_id}
HTTPClient->>APIServer: HTTP GET request
alt Message Found
APIServer-->>HTTPClient: 200 OK with message details
HTTPClient-->>EmailLogsAPI: Response body
EmailLogsAPI->>Mapper: Map to EmailLogMessage
Mapper->>Mapper: Build EmailLogEvent array
Mapper->>Mapper: Map event details to EmailLogEventDetails
Mapper-->>EmailLogsAPI: EmailLogMessage with events
EmailLogsAPI-->>Client: EmailLogMessage instance
else Message Not Found
APIServer-->>HTTPClient: 404 Not Found
HTTPClient-->>EmailLogsAPI: Error response
EmailLogsAPI-->>Client: Raise Mailtrap::Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/mailtrap/email_log_message_spec.rb (1)
27-29: Test intent says “all attributes” but excludes optional fields.Consider asserting
raw_message_urlandeventsexplicitly (even asnil) so the claim and coverage match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/email_log_message_spec.rb` around lines 27 - 29, The spec "it 'creates a message with all attributes'" currently uses expect(message).to have_attributes(attributes) but the attributes hash omits optional fields; update the test so the claim matches reality by adding raw_message_url and events to the asserted attributes (set them to nil if that's the expected value) or add explicit expectations like expect(message.raw_message_url).to be_nil and expect(message.events).to be_nil; refer to the test subject `message`, the example name, and the `attributes` hash when making the change.examples/email_logs_api.rb (1)
3-11: Consider reordering to favor environment variables as the primary approach.The example hardcodes credentials first, then shows the environment variable approach as a comment. For better security practices in example code, consider swapping these:
♻️ Suggested reordering
-account_id = 3229 -client = Mailtrap::Client.new(api_key: 'your-api-key') -email_logs = Mailtrap::EmailLogsAPI.new(account_id, client) - # Set your API credentials as environment variables # export MAILTRAP_API_KEY='your-api-key' # export MAILTRAP_ACCOUNT_ID=your-account-id -# -# email_logs = Mailtrap::EmailLogsAPI.new +email_logs = Mailtrap::EmailLogsAPI.new + +# Or manually pass credentials: +# account_id = 3229 +# client = Mailtrap::Client.new(api_key: 'your-api-key') +# email_logs = Mailtrap::EmailLogsAPI.new(account_id, client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/email_logs_api.rb` around lines 3 - 11, Swap the example to use environment variables as the primary credential approach: instantiate Mailtrap::Client and Mailtrap::EmailLogsAPI using ENV['MAILTRAP_API_KEY'] and ENV['MAILTRAP_ACCOUNT_ID'] (or call Mailtrap::EmailLogsAPI.new with no args if that constructor reads ENV) at the top, and move the hardcoded account_id and Mailtrap::Client.new(api_key: 'your-api-key') example into a commented “alternative” section; update the adjacent comments to show export instructions and mention the hardcoded snippet as an optional fallback.
🤖 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/email_log_event_details.rb`:
- Around line 52-56: The build method in EmailLogEventDetails currently assumes
DETAIL_STRUCTS[event_type.to_s] exists and will raise NoMethodError when
event_type is nil or unknown; update EmailLogEventDetails.build to handle a
missing struct_class by checking the result of DETAIL_STRUCTS[event_type.to_s]
(and event_type.nil?) and either (a) return a sensible default/ fallback struct
(or a generic OpenStruct) populated with the sliced attrs, or (b) raise a clear
ArgumentError/RuntimeError with a descriptive message including the unknown
event_type; ensure you reference DETAIL_STRUCTS and struct_class in the change
so callers get a graceful/failing-but-descriptive behavior from
EmailLogEventDetails.build.
---
Nitpick comments:
In `@examples/email_logs_api.rb`:
- Around line 3-11: Swap the example to use environment variables as the primary
credential approach: instantiate Mailtrap::Client and Mailtrap::EmailLogsAPI
using ENV['MAILTRAP_API_KEY'] and ENV['MAILTRAP_ACCOUNT_ID'] (or call
Mailtrap::EmailLogsAPI.new with no args if that constructor reads ENV) at the
top, and move the hardcoded account_id and Mailtrap::Client.new(api_key:
'your-api-key') example into a commented “alternative” section; update the
adjacent comments to show export instructions and mention the hardcoded snippet
as an optional fallback.
In `@spec/mailtrap/email_log_message_spec.rb`:
- Around line 27-29: The spec "it 'creates a message with all attributes'"
currently uses expect(message).to have_attributes(attributes) but the attributes
hash omits optional fields; update the test so the claim matches reality by
adding raw_message_url and events to the asserted attributes (set them to nil if
that's the expected value) or add explicit expectations like
expect(message.raw_message_url).to be_nil and expect(message.events).to be_nil;
refer to the test subject `message`, the example name, and the `attributes` hash
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c132c193-ccee-4f99-a377-9cc692da37f6
📒 Files selected for processing (16)
README.mdexamples/email_logs_api.rblib/mailtrap.rblib/mailtrap/email_log_event.rblib/mailtrap/email_log_event_details.rblib/mailtrap/email_log_message.rblib/mailtrap/email_logs_api.rblib/mailtrap/email_logs_list_response.rbspec/fixtures/vcr_cassettes/Mailtrap_EmailLogsAPI/_get/maps_response_data_to_EmailLogMessage_with_events_and_raw_message_url.ymlspec/fixtures/vcr_cassettes/Mailtrap_EmailLogsAPI/_get/when_message_not_found/raises_error.ymlspec/fixtures/vcr_cassettes/Mailtrap_EmailLogsAPI/_list/maps_response_data_to_EmailLogsListResponse_with_messages.ymlspec/fixtures/vcr_cassettes/Mailtrap_EmailLogsAPI/_list/with_filters_and_search_after/sends_correct_query_params_and_maps_response_data_to_EmailLogsListResponse_with_messages.ymlspec/mailtrap/email_log_event_details_spec.rbspec/mailtrap/email_log_event_spec.rbspec/mailtrap/email_log_message_spec.rbspec/mailtrap/email_logs_api_spec.rb
Comment on lines
+52
to
+56
| def self.build(event_type, hash) | ||
| struct_class = DETAIL_STRUCTS[event_type.to_s] | ||
| attrs = hash.slice(*struct_class.members) | ||
| struct_class.new(**attrs) | ||
| end |
There was a problem hiding this comment.
Handle unknown event types gracefully.
If event_type is nil or an unrecognized value (e.g., a new event type added by the API), struct_class will be nil, causing nil.members to raise NoMethodError.
🛡️ Proposed fix
def self.build(event_type, hash)
struct_class = DETAIL_STRUCTS[event_type.to_s]
+ return nil unless struct_class
+
attrs = hash.slice(*struct_class.members)
struct_class.new(**attrs)
endAlternatively, raise a descriptive error if failing loudly is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mailtrap/email_log_event_details.rb` around lines 52 - 56, The build
method in EmailLogEventDetails currently assumes DETAIL_STRUCTS[event_type.to_s]
exists and will raise NoMethodError when event_type is nil or unknown; update
EmailLogEventDetails.build to handle a missing struct_class by checking the
result of DETAIL_STRUCTS[event_type.to_s] (and event_type.nil?) and either (a)
return a sensible default/ fallback struct (or a generic OpenStruct) populated
with the sliced attrs, or (b) raise a clear ArgumentError/RuntimeError with a
descriptive message including the unknown event_type; ensure you reference
DETAIL_STRUCTS and struct_class in the change so callers get a
graceful/failing-but-descriptive behavior from EmailLogEventDetails.build.
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
1 participant
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
Add support for Email Logs API
Summary by CodeRabbit
New Features
Documentation
Tests