Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an Accounts API: new Account DTO and Mailtrap::AccountsAPI with a list method, VCR fixtures and RSpec tests, README and CHANGELOG updates, an examples usage file, and a new require_relative in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Dev
participant Client as Mailtrap::Client
participant API as Mailtrap::AccountsAPI
participant Server as Mailtrap API
Dev->>Client: initialize(api_key)
Dev->>API: AccountsAPI.new(client)
Dev->>API: call list
API->>Client: build GET /api/accounts request
Client->>Server: GET /api/accounts (Bearer token)
Server-->>Client: 200 OK + JSON [accounts] / 401 Unauthorized
Client-->>API: parsed response / raised error
API-->>Dev: Array<Account> (mapped DTOs) / AuthorizationError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
lib/mailtrap/account.rb (1)
6-8: Consider using@!attribute [r]instead of@attr_readerfor Struct fields.
@attr_readeris not a standard YARD tag;@!attribute [r]is the idiomatic YARD macro for documenting read-only Struct/class attributes and will be correctly picked up by YARD's parser.♻️ Proposed refactor
- # `@attr_reader` id [Integer] The account ID - # `@attr_reader` name [String] The account name - # `@attr_reader` access_levels [Array] The account access levels + # @!attribute [r] id + # `@return` [Integer] The account ID + # @!attribute [r] name + # `@return` [String] The account name + # @!attribute [r] access_levels + # `@return` [Array] The account access levels🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mailtrap/account.rb` around lines 6 - 8, Replace the nonstandard YARD tags `@attr_reader` with the idiomatic `@!attribute [r]` doc macro for the Struct fields (id, name, access_levels) in lib/mailtrap/account.rb so YARD properly recognizes them; update each docline to use `@!attribute [r] id`, `@!attribute [r] name`, and `@!attribute [r] access_levels` (preserving the existing type and description) and remove the old `@attr_reader` entries.spec/mailtrap/accounts_api_spec.rb (2)
12-15: Add field-level assertions to verify Account struct mapping.The test confirms the type and count of returned objects but never verifies that the individual fields (
id,name,access_levels) are actually populated from the API response. A bug in field mapping (e.g., wrong key name) would pass this test. Since the VCR cassette produces deterministic data, this is straightforward to add.♻️ Proposed addition
it 'maps response data to Account objects' do expect(list).to all(be_a(Mailtrap::Account)) expect(list.size).to eq(1) + + account = list.first + expect(account.id).to be_a(Integer) + expect(account.name).to be_a(String) + expect(account.access_levels).to be_an(Array) endAlternatively, pin the exact values from the VCR cassette fixture to tighten the contract:
+ expect(account.id).to eq(123456) + expect(account.name).to eq('My Account') + expect(account.access_levels).to eq([1000])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/accounts_api_spec.rb` around lines 12 - 15, The test only asserts types and count for 'list' but not field mapping; update the spec in spec/mailtrap/accounts_api_spec.rb to assert that each Mailtrap::Account in 'list' has correctly populated fields by checking account.id, account.name and account.access_levels against the expected values from the VCR cassette (or at minimum that they are not nil/empty); reference the Mailtrap::Account objects returned in 'list' and add expectations for id, name and access_levels to ensure the API response keys are mapped properly.
22-25: Remove redundant assertion on line 24.The
error.messagesarray is joined intoerror.message(messages.join(', ')), so checkingerror.message.include?(...)on line 23 anderror.messages.any? { |msg| msg.include?(...) }on line 24 tests the same data path twice. Keep line 23 and remove line 24.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/accounts_api_spec.rb` around lines 22 - 25, Remove the redundant assertion that checks error.messages (the expectation using error.messages.any? { |msg| msg.include?('Incorrect API token') }) in the spec; keep the existing check that inspects error.message.include?('Incorrect API token') and delete the second expectation so the spec only asserts the message once (refer to the expectations around Mailtrap::AuthorizationError and the lines that call error.message.include? and error.messages.any?).examples/accounts_api.rb (2)
14-14:accounts.listreturn value is silently discarded when run as a script.In an IRB/REPL session the result is printed automatically, but when the file is executed with
ruby examples/accounts_api.rbthe return value is thrown away and no output appears. Assign the result or usep/ppso the example is self-contained and demonstrable as a runnable script.♻️ Proposed fix
-accounts.list +result = accounts.list +pp result # => [#<struct Mailtrap::Account id=123456, name="My Account", access_levels=[1000]>]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/accounts_api.rb` at line 14, The script calls accounts.list but discards its return value, so running the file prints nothing; update the example to surface the result by assigning the return to a variable (e.g., result = accounts.list) and then print it (e.g., p result or puts result.inspect), or directly wrap the call with p/pp (p accounts.list) so the script is demonstrable when executed; modify the line containing accounts.list accordingly.
3-14: Consider restructuring the example so the env-var pattern is the active code.The current flow is logically inverted: hardcoded credentials are used to build the
client/accountsobjects (lines 3–5), and only afterward does the comment tell the reader to use environment variables (lines 7–9), with the preferred no-arg form commented out (line 11). A reader scanning top-to-bottom will use the hardcoded values without realising the env-var pattern even exists.Additionally,
account_id = 3229looks like a real account ID rather than a recognisable placeholder. For consistency with'your-api-key'it should be something like100_000or'your-account-id'.♻️ Proposed restructure
require 'mailtrap' +# Option 1: Provide credentials explicitly +client = Mailtrap::Client.new(api_key: 'your-api-key') +account_id = 100_000 +accounts = Mailtrap::AccountsAPI.new(account_id, client) + +# Option 2: Set credentials via environment variables and use defaults # export MAILTRAP_API_KEY='your-api-key' # export MAILTRAP_ACCOUNT_ID=your-account-id - -client = Mailtrap::Client.new(api_key: 'your-api-key') -account_id = 3229 -accounts = Mailtrap::AccountsAPI.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 - -# accounts = Mailtrap::AccountsAPI.new +# accounts = Mailtrap::AccountsAPI.new # Get all accounts -accounts.list +result = accounts.list # => [#<struct Mailtrap::Account id=123456, name="My Account", access_levels=[1000]>]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/accounts_api.rb` around lines 3 - 14, The example currently constructs Mailtrap::Client.new and Mailtrap::AccountsAPI.new using hardcoded values before showing the env-var pattern; reorder the snippet so the active code reads credentials from ENV (use ENV['MAILTRAP_API_KEY'] and ENV['MAILTRAP_ACCOUNT_ID'] to build Mailtrap::Client.new and Mailtrap::AccountsAPI.new) and use a clearly placeholder account_id like 100_000 or 'your-account-id'; move the hardcoded example into a commented block below as an alternative and keep the call to accounts.list as the final action so readers run the env-var flow by default (refer to Mailtrap::Client.new, Mailtrap::AccountsAPI.new, account_id, and accounts.list when making the change).
🤖 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/account.rb`:
- Line 4: The YARD comment contains a typo where the Cyrillic string "Фссщгте"
was typed instead of "Account"; update the comment in lib/mailtrap/account.rb
(the YARD module/class comment above the Account DTO) to read "Data Transfer
Object for Account" so the documentation correctly references the Account DTO
and remove the Cyrillic text.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_AccountsAPI/_list/maps_response_data_to_Account_objects.yml`:
- Line 77: Replace the real person's full name in the recorded response body by
editing the fixture entry that contains the JSON string with "name":"Yahor
Vaitsiakhouski" (the value inside the string field '[{"id":2326475,"name":"Yahor
Vaitsiakhouski","access_levels":[100]}]'). Substitute that value with an
anonymized placeholder (e.g., "name":"REDACTED_NAME" or "name":"Test User") and
save the YAML so the cassette no longer exposes PII while preserving the rest of
the record (id and access_levels).
In `@spec/mailtrap/account_spec.rb`:
- Line 17: The test description in spec/mailtrap/account_spec.rb is a copy-paste
error — change the example description in the it block that currently reads
"creates a project with all attributes" to correctly refer to an account (e.g.,
"creates an account with all attributes") so the test name matches the spec file
and the assertions in the example.
---
Nitpick comments:
In `@examples/accounts_api.rb`:
- Line 14: The script calls accounts.list but discards its return value, so
running the file prints nothing; update the example to surface the result by
assigning the return to a variable (e.g., result = accounts.list) and then print
it (e.g., p result or puts result.inspect), or directly wrap the call with p/pp
(p accounts.list) so the script is demonstrable when executed; modify the line
containing accounts.list accordingly.
- Around line 3-14: The example currently constructs Mailtrap::Client.new and
Mailtrap::AccountsAPI.new using hardcoded values before showing the env-var
pattern; reorder the snippet so the active code reads credentials from ENV (use
ENV['MAILTRAP_API_KEY'] and ENV['MAILTRAP_ACCOUNT_ID'] to build
Mailtrap::Client.new and Mailtrap::AccountsAPI.new) and use a clearly
placeholder account_id like 100_000 or 'your-account-id'; move the hardcoded
example into a commented block below as an alternative and keep the call to
accounts.list as the final action so readers run the env-var flow by default
(refer to Mailtrap::Client.new, Mailtrap::AccountsAPI.new, account_id, and
accounts.list when making the change).
In `@lib/mailtrap/account.rb`:
- Around line 6-8: Replace the nonstandard YARD tags `@attr_reader` with the
idiomatic `@!attribute [r]` doc macro for the Struct fields (id, name,
access_levels) in lib/mailtrap/account.rb so YARD properly recognizes them;
update each docline to use `@!attribute [r] id`, `@!attribute [r] name`, and
`@!attribute [r] access_levels` (preserving the existing type and description)
and remove the old `@attr_reader` entries.
In `@spec/mailtrap/accounts_api_spec.rb`:
- Around line 12-15: The test only asserts types and count for 'list' but not
field mapping; update the spec in spec/mailtrap/accounts_api_spec.rb to assert
that each Mailtrap::Account in 'list' has correctly populated fields by checking
account.id, account.name and account.access_levels against the expected values
from the VCR cassette (or at minimum that they are not nil/empty); reference the
Mailtrap::Account objects returned in 'list' and add expectations for id, name
and access_levels to ensure the API response keys are mapped properly.
- Around line 22-25: Remove the redundant assertion that checks error.messages
(the expectation using error.messages.any? { |msg| msg.include?('Incorrect API
token') }) in the spec; keep the existing check that inspects
error.message.include?('Incorrect API token') and delete the second expectation
so the spec only asserts the message once (refer to the expectations around
Mailtrap::AuthorizationError and the lines that call error.message.include? and
error.messages.any?).
.../fixtures/vcr_cassettes/Mailtrap_AccountsAPI/_list/maps_response_data_to_Account_objects.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/mailtrap/account.rb (2)
6-8:@attr_readeris deprecated in YARD since 0.8.0; prefer@!attribute [r].
@attrtags have been deprecated since YARD 0.8.0 in favour of the@!attributedirective. YARD's own tag docs recommend using "the more powerful@!attributedirective instead," noting that@attr_readeris "only applicable on class docstrings" and "declares a readonly attribute on a Struct or class." TheAccount = Struct.new(...)constant-assignment form is not a class docstring, so YARD'sStructHandlermay not pick up these tags correctly regardless.📝 Proposed fix
- # `@attr_reader` id [Integer] The account ID - # `@attr_reader` name [String] The account name - # `@attr_reader` access_levels [Array] The account access levels + # @!attribute [r] id + # `@return` [Integer] The account ID + # @!attribute [r] name + # `@return` [String] The account name + # @!attribute [r] access_levels + # `@return` [Array] The account access levels🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mailtrap/account.rb` around lines 6 - 8, Replace deprecated YARD `@attr_reader` tags for the Account Struct with @!attribute directives: add @!attribute [r] id, @!attribute [r] name and @!attribute [r] access_levels (including their types and short descriptions) near the Account = Struct.new(...) declaration so YARD's StructHandler will pick them up correctly; ensure the attributes are documented as readonly and that the text mirrors the existing comments (Integer for id, String for name, Array for access_levels).
6-8: Use@!attribute [r]instead of@attr_readerfor YARD documentation.The
@attr_readertag is not recognized by the YARD parser and will not render in API documentation. The correct YARD directive for documenting readable attributes is@!attribute [r].📝 Proposed fix
- # `@attr_reader` id [Integer] The account ID - # `@attr_reader` name [String] The account name - # `@attr_reader` access_levels [Array] The account access levels + # @!attribute [r] id + # `@return` [Integer] The account ID + # @!attribute [r] name + # `@return` [String] The account name + # @!attribute [r] access_levels + # `@return` [Array] The account access levels🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mailtrap/account.rb` around lines 6 - 8, Replace the non-standard YARD tags by changing each "@attr_reader" doc line for the Account attributes to the supported YARD directive "@!attribute [r]" and keep the type and description—i.e., update the doc comments referencing id, name, and access_levels in lib/mailtrap/account.rb so they become "@!attribute [r] id [Integer] The account ID", "@!attribute [r] name [String] The account name", and "@!attribute [r] access_levels [Array] The account access levels" (preserve wording/types for id, name, access_levels) so YARD will render them as readable attributes.
🤖 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/account.rb`:
- Around line 6-8: Replace deprecated YARD `@attr_reader` tags for the Account
Struct with @!attribute directives: add @!attribute [r] id, @!attribute [r] name
and @!attribute [r] access_levels (including their types and short descriptions)
near the Account = Struct.new(...) declaration so YARD's StructHandler will pick
them up correctly; ensure the attributes are documented as readonly and that the
text mirrors the existing comments (Integer for id, String for name, Array for
access_levels).
- Around line 6-8: Replace the non-standard YARD tags by changing each
"@attr_reader" doc line for the Account attributes to the supported YARD
directive "@!attribute [r]" and keep the type and description—i.e., update the
doc comments referencing id, name, and access_levels in lib/mailtrap/account.rb
so they become "@!attribute [r] id [Integer] The account ID", "@!attribute [r]
name [String] The account name", and "@!attribute [r] access_levels [Array] The
account access levels" (preserve wording/types for id, name, access_levels) so
YARD will render them as readable attributes.
examples/accounts_api.rb
Outdated
|
|
||
| client = Mailtrap::Client.new(api_key: 'your-api-key') | ||
| account_id = 3229 | ||
| accounts = Mailtrap::AccountsAPI.new(account_id, client) |
There was a problem hiding this comment.
why do you need an account id to list accounts?
lib/mailtrap.rb
Outdated
| require_relative 'mailtrap/projects_api' | ||
| require_relative 'mailtrap/sandbox_messages_api' | ||
| require_relative 'mailtrap/sandbox_attachments_api' | ||
| require_relative 'mailtrap/suppressions_api' |
There was a problem hiding this comment.
@DagonWat could you please minimise changes in this file?
| General: | ||
|
|
||
| - Templates CRUD – [`email_templates_api.rb`](examples/email_templates_api.rb) | ||
| - Action Mailer – [`action_mailer.rb`](examples/action_mailer.rb) |
There was a problem hiding this comment.
you wanted to fix "Action Mailer", didnt you?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/accounts_api.rb (1)
3-9: Credential guidance appears after the code that uses a hardcoded key.The env-variable comment (lines 6–7) is placed after the hardcoded-key initialization on line 3, inverting the intended setup order. Readers following top-to-bottom will copy the hardcoded path and miss the env-var guidance.
♻️ Suggested reordering
require 'mailtrap' +# Set your API credentials as environment variables +# export MAILTRAP_API_KEY='your-api-key' + +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY')) -client = Mailtrap::Client.new(api_key: 'your-api-key') accounts = Mailtrap::AccountsAPI.new(client) -# Set your API credentials as environment variables -# export MAILTRAP_API_KEY='your-api-key' - -# accounts = Mailtrap::AccountsAPI.new # Get all accounts accounts.list # => [#<struct Mailtrap::Account id=123456, name="My Account", access_levels=[1000]>]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/accounts_api.rb` around lines 3 - 9, Move the environment-variable guidance before the code that constructs the client so readers see the recommended setup before a hardcoded key; update examples/accounts_api.rb to place the comment lines about MAILTRAP_API_KEY and the alternative initialization (Mailtrap::Client.new using ENV['MAILTRAP_API_KEY'] or Mailtrap::AccountsAPI.new without args) above the current Mailtrap::Client.new(api_key: 'your-api-key') and Mailtrap::AccountsAPI.new(client) lines, and optionally replace the hardcoded example with the ENV-based example to encourage secure usage.
🤖 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/accounts_api_spec.rb`:
- Around line 20-25: The test's raise_error matcher is too permissive; change
the expectation to assert the specific error type and message by calling
raise_error with Mailtrap::AuthorizationError and a message pattern (e.g.,
/Incorrect API token/) for the main matcher, and keep only the custom assertion
about error.messages inside the block; update the spec around the call to list
so it uses raise_error(Mailtrap::AuthorizationError, /Incorrect API token/) and
then yields the error to verify error.messages contains the expected string.
---
Nitpick comments:
In `@examples/accounts_api.rb`:
- Around line 3-9: Move the environment-variable guidance before the code that
constructs the client so readers see the recommended setup before a hardcoded
key; update examples/accounts_api.rb to place the comment lines about
MAILTRAP_API_KEY and the alternative initialization (Mailtrap::Client.new using
ENV['MAILTRAP_API_KEY'] or Mailtrap::AccountsAPI.new without args) above the
current Mailtrap::Client.new(api_key: 'your-api-key') and
Mailtrap::AccountsAPI.new(client) lines, and optionally replace the hardcoded
example with the ENV-based example to encourage secure usage.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/accounts_api.rblib/mailtrap/accounts_api.rbspec/mailtrap/accounts_api_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap/accounts_api.rb
| expect { list }.to raise_error do |error| | ||
| expect(error).to be_a(Mailtrap::AuthorizationError) | ||
| expect(error.message).to include('Incorrect API token') | ||
| expect(error.messages.any? { |msg| msg.include?('Incorrect API token') }).to be true | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the AuthorizationError class definition
find . -name "*.rb" -type f | xargs rg -l "class AuthorizationError"Repository: mailtrap/mailtrap-ruby
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Look for error class definitions in the mailtrap directory structure
fd -e rb "error" --path "*mailtrap*" -type fRepository: mailtrap/mailtrap-ruby
Length of output: 299
🏁 Script executed:
#!/bin/bash
# Search for AuthorizationError class and its attributes
rg -n "class AuthorizationError" -A 30 --type rubyRepository: mailtrap/mailtrap-ruby
Length of output: 965
🏁 Script executed:
#!/bin/bash
# Also search for any definition of 'messages' or 'details' attributes in error classes
rg -n "(attr_reader|attr_accessor|def messages|def details)" --type ruby -B 3 -A 1 | head -100Repository: mailtrap/mailtrap-ruby
Length of output: 7635
🏁 Script executed:
#!/bin/bash
# View the entire errors.rb file to see the Error base class definition
cat -n lib/mailtrap/errors.rbRepository: mailtrap/mailtrap-ruby
Length of output: 1174
🏁 Script executed:
#!/bin/bash
# Search for Error class definition and attributes
rg -n "class Error" -A 25 --type ruby lib/mailtrap/errors.rbRepository: mailtrap/mailtrap-ruby
Length of output: 845
Specify error type in raise_error to avoid false positives.
The raise_error matcher without a specific error type argument can match unintended exceptions raised inside the block (e.g., NoMethodError), causing tests to pass silently when they shouldn't. While error.messages is a valid attribute on Mailtrap::AuthorizationError, the bare matcher remains a code smell.
Pass the expected error type and message pattern directly to raise_error, and move only the custom assertion inside the block:
♻️ Proposed fix
- expect { list }.to raise_error do |error|
- expect(error).to be_a(Mailtrap::AuthorizationError)
- expect(error.message).to include('Incorrect API token')
- expect(error.messages.any? { |msg| msg.include?('Incorrect API token') }).to be true
- end
+ expect { list }.to raise_error(Mailtrap::AuthorizationError, /Incorrect API token/) do |error|
+ expect(error.messages).to include(a_string_including('Incorrect API token'))
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/mailtrap/accounts_api_spec.rb` around lines 20 - 25, The test's
raise_error matcher is too permissive; change the expectation to assert the
specific error type and message by calling raise_error with
Mailtrap::AuthorizationError and a message pattern (e.g., /Incorrect API token/)
for the main matcher, and keep only the custom assertion about error.messages
inside the block; update the spec around the call to list so it uses
raise_error(Mailtrap::AuthorizationError, /Incorrect API token/) and then yields
the error to verify error.messages contains the expected string.
Motivation
Add Accounts API to Ruby SDK
Changes
Summary by CodeRabbit
New Features
Documentation
Tests