Batch sending api #48
Conversation
…d response error handling Add Email Templates API functionality
WalkthroughThis update introduces batch email sending support and a new EmailTemplates API to the Mailtrap Ruby client. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant API
User->>Client: send_batch(base_email, requests)
Client->>API: POST /api/batch (base_email + requests)
API-->>Client: Batch send response (success/failure per email)
Client-->>User: Return batch send result
sequenceDiagram
participant User
participant TemplateService
participant API
User->>TemplateService: create(request)
TemplateService->>API: POST /email_templates (request)
API-->>TemplateService: Created template
TemplateService-->>User: Return EmailTemplate
User->>TemplateService: list()
TemplateService->>API: GET /email_templates
API-->>TemplateService: List of templates
TemplateService-->>User: Return array of EmailTemplate
User->>TemplateService: get(template_id)
TemplateService->>API: GET /email_templates/:id
API-->>TemplateService: EmailTemplate data
TemplateService-->>User: Return EmailTemplate
User->>TemplateService: update(template_id, request)
TemplateService->>API: PATCH /email_templates/:id (request)
API-->>TemplateService: Updated template
TemplateService-->>User: Return EmailTemplate
User->>TemplateService: delete(template_id)
TemplateService->>API: DELETE /email_templates/:id
API-->>TemplateService: 204 No Content
TemplateService-->>User: Return true
Possibly related issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
lib/mailtrap/mail/base.rb (1)
64-66:attachments=silently fails for already-built Attachment objects.
attachments.map { |attachment| Mailtrap::Attachment.new(**attachment) }assumes every element is a Hash.
If callers passMailtrap::Attachmentinstances (quite likely insidesend_batch), this raisesTypeError.def attachments=(attachments) - @attachments = attachments.map { |attachment| Mailtrap::Attachment.new(**attachment) } + @attachments = attachments.map do |attachment| + case attachment + when Mailtrap::Attachment + attachment + when Hash + Mailtrap::Attachment.new(**attachment) + else + raise ArgumentError, 'attachments must be Hashes or Mailtrap::Attachment objects' + end + end end
♻️ Duplicate comments (7)
spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml (1)
24-70: Same fixture-size concern as previous cassettespec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml (1)
24-70: Same fixture-size concern as previous cassettespec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml (1)
24-70: Same fixture-size concern as previous cassettespec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml (1)
1-331: Filter out dynamic headers to slim fixtures
As above, this cassette includes many transient headers. Apply the same VCR configuration to remove them for consistency and maintainability.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/returns_an_EmailTemplate_object.yml (1)
1-168: Filter out dynamic headers to slim fixtures
Multiple volatile headers make the fixture overly large. Consolidate header filtering across all cassettes to keep them concise.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/returns_an_EmailTemplate_object.yml (1)
1-333: Filter out dynamic headers to slim fixtures
This cassette also records the same dynamic headers—centralize header filtering in your VCR config instead of duplicating cleanup logic.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml (1)
1-168: Filter out dynamic headers to slim fixtures
Consistent with other fixtures, strip Date, Etag, Cf-Ray, X-Ratelimit-* headers to focus on request/response payloads.
🧹 Nitpick comments (21)
lib/mailtrap/client.rb (1)
60-63: Minor: tighten type guard forrequests
Array#all?(Mail::Base)works but is obscure. An explicit block is clearer:unless requests.all? { |r| r.is_a?(Mail::Base) }lib/mailtrap/mail/base.rb (2)
40-55: Drop the redundant.compactcall aftercompact_with_empty_arrays.
compact_with_empty_arraysalready rejectsnilvalues and empty arrays, so the additional.compactprovides no benefit and costs an extra hash traversal.- }).compact + })
50-53: Finish the TODO for nested objects.
headersandcustom_variablesmay contain nested structures that themselves need cleaning. A recursivecompact_with_empty_arrays(or callingtransform_values) would keep the payload minimal and symmetric with attachments serialization.examples/batch.rb (2)
12-14: UseBase64.strict_encode64to avoid embedded newlines in attachment content.Some APIs reject base-64 strings containing line feeds produced by
Base64.encode64.- content: Base64.encode64('Attachment content'), # base64 encoded content or IO string + content: Base64.strict_encode64('Attachment content'),
24-34: Pull sensitive credentials from environment variables in examples.Hard-coding placeholders encourages copy-paste of real keys into source. Reading from
ENV['MAILTRAP_API_KEY']is safer and showcases best practice.-client = Mailtrap::Client.new(api_key: 'your-api-key') +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY'))examples/email_template.rb (2)
18-19: Prefer env-driven credentials & avoid hard-coding the API keyHard-coding any credential (even a placeholder) in example code encourages copy/paste of insecure patterns. Recommend reading it from an env var, e.g.
ENV.fetch("MAILTRAP_API_KEY"), so the snippet promotes production-ready practices.-client = Mailtrap::Client.new(api_key: 'your-api-key') +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY'))
22-23: Account id should be configurableAs with the API key, lifting the hard-coded
1_111_111into an env var (or CLI arg) makes the example immediately usable without edits.spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/returns_an_array_of_EmailTemplate_objects.yml (1)
24-70: Fixture size can be drastically reducedThe cassette stores every response header (e.g. ~50 lines of CSP) and the full list of 70-plus template objects. Tests usually only need a couple of representative objects and minimal headers. Trimming via VCR
before_recordfilters (orrecord: :once+ selectiveallow_headers) will:• shrink repo size
• speed up diff reviews
• avoid brittle tests when the header ordering changes.Example filter:
VCR.configure do |c| c.before_record do |interaction| interaction.response.headers.slice!('Content-Type') interaction.response.body = interaction.response.body[0, 1_000] # keep first 1 kB end endspec/mailtrap/client_spec.rb (3)
248-266: Recipient objects omit required fields – test may mask real failuresEach element in
recipientsonly setsto, leavingfrom,subject, etc. blank.
send_batchpresumably merges the base-level data, but the spec never asserts this.
If the implementation changes, these silent assumptions will break production before tests.Recommendation:
• Assert that every request built for a recipient inheritsfrom,subject, andtextfrombase_mail.
• Alternatively, pass plain recipient hashes and keep one canonicalMail::Base.
268-282: Context wording violates project RuboCop rules
context 'when bulk and sandbox modes are used together'passes, but the next two are'in bulk mode'and'in sandbox mode', triggeringRSpec/ContextWordingoffences.Consider renaming to keep the
when / with / withoutconvention, e.g.:-context 'in bulk mode' do +context 'when bulk mode is enabled' doKeeps lint green and documentation consistent.
285-371: Spec examples exceed agreed length – extract shared matcherSeveral examples (lines 287-298, 304-315, 333-344, 355-369) breach
RSpec/ExampleLength [10/5].
They all duplicate the same “successful batch send” assertions.Refactor into a shared expectation:
shared_examples 'successful batch response' do it 'returns success with message_ids' do expect(response).to include( success: true, responses: array_including( hash_including(success: true, message_ids: array_including(kind_of(String))) ) ) end endThen
it_behaves_likein each context → shorter, clearer tests and no RuboCop noise.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml (1)
1-333: Filter out dynamic headers to slim fixtures
This cassette captures many dynamic headers (Date, Etag, Cf-Ray, X-Ratelimit-*) that bloat the file and rarely add value to tests. Use VCR’sbefore_recordhook to strip or ignore irrelevant headers.lib/mailtrap/template.rb (2)
51-55: ImplicitMailtrap::Clientinstantiation may explode in unsuspecting environments.Calling
Template.new(account_id)will instantiate a freshMailtrap::ClientwithENV['MAILTRAP_API_KEY'].
If the env var is missing the constructor raises, producing a confusing error far from the call-site.
Prefer makingclienta required argument or raising here with a clearer message.
90-96: Duplicated normalisation logic betweencreateandupdate.Extract into a small private helper to keep responsibilities single and remove repetition.
- normalised = request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request) + normalised = normalize_request(request)…and add
def normalize_request(request) request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request) end private :normalize_requestspec/mailtrap/template_spec.rb (3)
3-8: Multiple top-leveldescribeblocks violateRSpec/MultipleDescribes.Nest the secondary groups (
Mailtrap::EmailTemplateRequest,Mailtrap::EmailTemplate) inside a primarydescribe Mailtrapor split them into separate spec files for clearer organisation and to silence RuboCop.
24-34: Long example can be compacted.The custom matcher chain inside the authorization-error expectation spans 8 lines.
Consider asserting withexpect { list }.to raise_error(Mailtrap::AuthorizationError, /Incorrect API token/)to keep examples concise and comply withRSpec/ExampleLength.
146-163: Repeated remote setup inflates cassette size and spec runtime.Each context recreates a template via the API. Extract shared setup into a
before(:all)(or a fixture cassette) so the create call is made once per cassette instead of once per example group.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml (2)
1-3: Keep fixtures focused and clear.
This cassette records both a template creation and a failed deletion, but the scenario name implies only delete. Consider splitting the POST creation into its own cassette and isolating the delete-error case for clarity and maintainability.
8-10: Preserve JSON integrity with block scalars.
The request JSON is split across lines, which may introduce unintended whitespace. Switch to a YAML block scalar (|) for thestringfield to maintain the exact payload.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml (2)
1-1: Enforce lowercase for cassette filenames.
The filenamereturns_an_EmailTemplate_object.ymlmixes CamelCase. Rename toreturns_an_email_template_object.ymlto adhere to standard snake_case conventions.
8-10: Avoid inline JSON line wrapping.
Thestringpayload for the POST is wrapped across multiple lines. Use a YAML block scalar to preserve the exact JSON and prevent accidental whitespace or newline injection:body: encoding: UTF-8 string: | {"email_template":{"name":"Original Template",...}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
CHANGELOG.md(1 hunks)examples/batch.rb(1 hunks)examples/email_template.rb(2 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/mail/base.rb(2 hunks)lib/mailtrap/template.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/returns_true.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/client_spec.rb(2 hunks)spec/mailtrap/template_spec.rb(1 hunks)spec/spec_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
examples/batch.rb (2)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/client.rb (1)
send_batch(57-75)
lib/mailtrap/template.rb (2)
lib/mailtrap/client.rb (5)
initialize(28-51)get(87-90)post(96-99)patch(105-108)delete(113-116)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
lib/mailtrap/client.rb (2)
lib/mailtrap/template.rb (2)
get(66-69)delete(101-103)lib/mailtrap/mail/base.rb (1)
to_json(57-62)
🪛 RuboCop (1.75.5)
spec/mailtrap/client_spec.rb
[convention] 284-284: Context description should match /^when\b/, /^with\b/, or /^without\b/.
(RSpec/ContextWording)
[convention] 287-298: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 301-301: Context description should match /^when\b/, /^with\b/, or /^without\b/.
(RSpec/ContextWording)
[convention] 304-315: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 333-344: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 355-369: Example has too many lines. [13/5]
(RSpec/ExampleLength)
spec/mailtrap/template_spec.rb
[convention] 3-3: Do not use multiple top-level example groups - try to nest them.
(RSpec/MultipleDescribes)
[convention] 55-62: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 258-266: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 283-291: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 331-343: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 363-375: Example has too many lines. [11/5]
(RSpec/ExampleLength)
lib/mailtrap/client.rb
[convention] 57-75: Method has too many lines. [14/10]
(Metrics/MethodLength)
🔇 Additional comments (14)
lib/mailtrap/client.rb (1)
65-69: Use HTTPS helper to avoid mismatched scheme/SSLYou build the URI with
URI::HTTP.buildbut later forceuse_ssl = true.
Simpler & less error-prone:- uri = URI::HTTP.build( + uri = URI::HTTPS.build(Same applies to
send,get,post,patch,delete.
[ suggest_optional_refactor ]CHANGELOG.md (1)
1-3: Changelog entry LGTMlib/mailtrap.rb (1)
7-7: No issues – new require integrates template APIspec/fixtures/vcr_cassettes/Mailtrap_Template/_list/when_api_key_is_incorrect/raises_authorization_error.yml (1)
1-165: Skipping – fixture content is fine.examples/email_template.rb (1)
19-19: ```shell
#!/bin/bashSearch for all
.send(call-sites in Ruby files without specifying typerg -n '.send(' -g '*.rb' | head
</details> <details> <summary>spec/mailtrap/client_spec.rb (1)</summary> `216-219`: ```shell #!/bin/bash set -e echo "== Showing lines 160-200 from lib/mailtrap/client.rb ==" sed -n '160,200p' lib/mailtrap/client.rbspec/fixtures/vcr_cassettes/Mailtrap_Template/_update/when_template_does_not_exist/raises_not_found_error.yml (1)
17-21: Sensitive headers look redacted – good job.
Authorizationheader is masked with<BEARER_TOKEN>. ✅spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml (1)
1-164: Skip auto-generated VCR cassette fixture
This YAML file is generated by VCR to record HTTP interactions for error scenarios and does not require manual review.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml (1)
1-168: Skip auto-generated VCR cassette fixture
This YAML file is generated by VCR to record the successful creation of an EmailTemplate and does not require manual review.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/returns_true.yml (1)
1-321: Skip auto-generated VCR cassette fixture
This YAML file captures delete interactions via VCR and is auto-generated; no manual review is needed.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/when_template_does_not_exist/raises_not_found_error.yml (1)
1-327: Skip auto-generated VCR cassette fixture
This YAML file records a not-found scenario and is auto-generated by VCR; no manual review is necessary.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/maps_response_data_to_EmailTemplate_object.yml (1)
1-331: Skip auto-generated VCR cassette fixture
This YAML file captures the mapping of response data to an EmailTemplate object and is auto-generated; it does not require manual review.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml (1)
5-6: Use dynamic placeholders for account IDs.
Hard-coding1111111in the URI can leak sensitive data. Ensurespec/spec_helper.rbfilters this viaconfig.filter_sensitive_dataand replace it with a placeholder like<ACCOUNT_ID>in the fixture.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml (1)
5-6: Mask sensitive data in fixtures.
Confirm that the account ID (1111111) and token (<BEARER_TOKEN>) are filtered viaconfig.filter_sensitive_datainspec/spec_helper.rb, and consider using<ACCOUNT_ID>placeholders in all URIs.
| interaction.request.uri.gsub!(%r{/accounts/\d+/}, '/accounts/1111111/') | ||
| interaction.response.body.gsub!(/"account_id":\d+/, '"account_id": 1111111') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating a potentially frozen response body
interaction.response.body may be frozen by VCR; gsub! would then raise FrozenError.
Prefer a non-destructive variant or operate on dup:
interaction.response.body = interaction.response.body.gsub(/"account_id":\d+/, '"account_id": 1111111')🤖 Prompt for AI Agents
In spec/spec_helper.rb around lines 17 to 19, avoid mutating the potentially
frozen interaction.response.body with gsub! as it may raise a FrozenError.
Instead, use the non-destructive gsub method and assign the result back to
interaction.response.body to prevent mutation errors.
lib/mailtrap/client.rb
Outdated
| def send_batch(base, requests) | ||
| raise ArgumentError, 'base should be Mailtrap::Mail::Base object' unless base.is_a?(Mail::Base) | ||
|
|
||
| unless requests.all?(Mail::Base) | ||
| raise ArgumentError, | ||
| 'requests should be an array of Mailtrap::Mail::Base objects' | ||
| end | ||
|
|
||
| uri = URI::HTTP.build( | ||
| host: api_host, | ||
| port: api_port, | ||
| path: batch_request_url | ||
| ) | ||
|
|
||
| perform_request(:post, uri, { | ||
| base:, | ||
| requests: | ||
| }) | ||
| end |
There was a problem hiding this comment.
Fix double-encoding of payload sent to /api/batch
body.to_json is called on a hash whose values are Mailtrap::Mail::Base objects.
Because Mail::Base#to_json already returns a JSON string, the resulting payload nests those strings and the API receives invalid JSON.
- perform_request(:post, uri, {
- base:,
- requests:
- })
+ payload = {
+ base: base.as_json,
+ requests: requests.map(&:as_json)
+ }
+
+ perform_request(:post, uri, payload)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def send_batch(base, requests) | |
| raise ArgumentError, 'base should be Mailtrap::Mail::Base object' unless base.is_a?(Mail::Base) | |
| unless requests.all?(Mail::Base) | |
| raise ArgumentError, | |
| 'requests should be an array of Mailtrap::Mail::Base objects' | |
| end | |
| uri = URI::HTTP.build( | |
| host: api_host, | |
| port: api_port, | |
| path: batch_request_url | |
| ) | |
| perform_request(:post, uri, { | |
| base:, | |
| requests: | |
| }) | |
| end | |
| def send_batch(base, requests) | |
| raise ArgumentError, 'base should be Mailtrap::Mail::Base object' unless base.is_a?(Mail::Base) | |
| unless requests.all?(Mail::Base) | |
| raise ArgumentError, | |
| 'requests should be an array of Mailtrap::Mail::Base objects' | |
| end | |
| uri = URI::HTTP.build( | |
| host: api_host, | |
| port: api_port, | |
| path: batch_request_url | |
| ) | |
| payload = { | |
| base: base.as_json, | |
| requests: requests.map(&:as_json) | |
| } | |
| perform_request(:post, uri, payload) | |
| end |
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 57-75: Method has too many lines. [14/10]
(Metrics/MethodLength)
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 57 to 75, the send_batch method is
double-encoding JSON because it calls to_json on a hash containing
Mailtrap::Mail::Base objects that already return JSON strings. To fix this,
convert each Mail::Base object to a hash or raw data structure before calling
to_json on the entire payload, ensuring the final JSON is correctly structured
without nested JSON strings.
spec/mailtrap/client_spec.rb
Outdated
| describe '#send_batch' do | ||
| let(:api_key) { '793a32549bc04f67fcd4f800009781f2' } | ||
| let(:base_mail) do |
There was a problem hiding this comment.
Do not commit real-looking API keys – treat them as secrets
let(:api_key) { '793a32549bc04f67fcd4f800009781f2' } looks like a live key.
Even if it’s only a test token, publishing it normalises unsafe behaviour and forces re-rolls if it is real.
-let(:api_key) { '793a32549bc04f67fcd4f800009781f2' }
+let(:api_key) { ENV.fetch('MAILTRAP_TEST_API_KEY') }Configure VCR to filter the env value so nothing leaks to the cassette.
[security/PII]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe '#send_batch' do | |
| let(:api_key) { '793a32549bc04f67fcd4f800009781f2' } | |
| let(:base_mail) do | |
| describe '#send_batch' do | |
| - let(:api_key) { '793a32549bc04f67fcd4f800009781f2' } | |
| + let(:api_key) { ENV.fetch('MAILTRAP_TEST_API_KEY') } | |
| let(:base_mail) do | |
| # ... | |
| end | |
| # ... | |
| end |
🤖 Prompt for AI Agents
In spec/mailtrap/client_spec.rb around lines 235 to 237, the API key is
hardcoded as a real-looking string, which risks exposing sensitive information.
Replace the hardcoded API key with a reference to an environment variable or a
test-specific placeholder. Additionally, configure VCR to filter out the API key
from recorded cassettes by masking or replacing it with a placeholder to prevent
leaking secrets in test artifacts.
| - mailtrap-ruby (https://github.com/railsware/mailtrap-ruby) | ||
| Authorization: | ||
| - Bearer <BEARER_TOKEN> | ||
| Content-Type: | ||
| - application/json |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mask sensitive authorization tokens
The Authorization: Bearer <BEARER_TOKEN> header should be filtered via VCR’s filter_sensitive_data to avoid exposing real credentials in fixtures.
🤖 Prompt for AI Agents
In
spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml
around lines 17 to 21, the Authorization header contains a real bearer token
that should be masked. Use VCR's filter_sensitive_data configuration to replace
the actual token with a placeholder like <BEARER_TOKEN> in the recorded fixture
to prevent exposing sensitive credentials.
| class Template | ||
| MAX_LENGTH = 255 | ||
| MAX_BODY_LENGTH = 10_000_000 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
MAX_LENGTH / MAX_BODY_LENGTH are currently dead code.
The constants are declared but never referenced. Either enforce the limits (e.g. in EmailTemplateRequest#initialize) or remove the constants to avoid misleading future maintainers.
🤖 Prompt for AI Agents
In lib/mailtrap/template.rb around lines 45 to 48, the constants MAX_LENGTH and
MAX_BODY_LENGTH are declared but never used, making them dead code. To fix this,
either remove these constants if they are unnecessary or enforce these limits by
adding validation logic in the EmailTemplateRequest#initialize method to check
that input lengths do not exceed these constants, raising errors or handling
violations appropriately.
| def create(request) | ||
| normalised = request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request) | ||
| response = @client.post(base_path, | ||
| { | ||
| email_template: normalised.to_h | ||
| }) | ||
| EmailTemplate.new(response) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
create performs zero validation – required attributes or length limits can slip through.
You already define MAX_LENGTH / MAX_BODY_LENGTH; validate normalised before issuing the HTTP request so that we fail fast locally instead of receiving a 422/400 from the API.
🤖 Prompt for AI Agents
In lib/mailtrap/template.rb around lines 75 to 82, the create method lacks
validation for required attributes and length limits before making the HTTP
request. Add validation logic to check that the normalized EmailTemplateRequest
meets required attribute presence and does not exceed MAX_LENGTH or
MAX_BODY_LENGTH constraints. Raise an error or return early if validation fails
to ensure the method fails fast locally instead of relying on the API to reject
invalid data.
sarco3t
force-pushed
the
batch-sending-api
branch
from
4a2f808 to
a90b21c
Compare
sarco3t
force-pushed
the
batch-sending-api
branch
from
a90b21c to
3c95335
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
spec/mailtrap/client_spec.rb (1)
235-237: Avoid hard-coding “real-looking” API keys in specsPrevious review already asked to rely on an env var – the new
#send_batch
examples re-introduce a literal token.-let(:api_key) { 'correct-api-key' } +let(:api_key) { ENV.fetch('MAILTRAP_TEST_API_KEY') }Remember to mask it in VCR cassettes (
config.filter_sensitive_data).
🧹 Nitpick comments (1)
lib/mailtrap/client.rb (1)
60-63: Type check is correct but brittle
requests.all?(Mail::Base)works becauseModule === objis kind-of?
But the intent is clearer (and Ruby-version-agnostic) with an explicit block:-unless requests.all?(Mail::Base) +unless requests.all? { |r| r.is_a?(Mail::Base) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/batch.rb(1 hunks)lib/mailtrap/client.rb(6 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/in_bulk_mode/successfully_sends_a_batch_of_emails.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/in_sandbox_mode/successfully_sends_a_batch_of_emails.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_API_errors/handles_API_errors.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_template/successfully_sends_a_batch_of_emails_with_template.yml(1 hunks)spec/mailtrap/client_spec.rb(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/in_sandbox_mode/successfully_sends_a_batch_of_emails.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_API_errors/handles_API_errors.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/in_bulk_mode/successfully_sends_a_batch_of_emails.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_template/successfully_sends_a_batch_of_emails_with_template.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/batch.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/mailtrap/client.rb (4)
lib/mailtrap/mail/base.rb (2)
as_json(39-55)to_json(57-62)lib/mailtrap/mail/from_template.rb (1)
as_json(34-41)lib/mailtrap/attachment.rb (1)
as_json(19-27)lib/mailtrap/template.rb (2)
get(66-69)delete(101-103)
🪛 RuboCop (1.75.5)
lib/mailtrap/client.rb
[convention] 57-75: Method has too many lines. [14/10]
(Metrics/MethodLength)
spec/mailtrap/client_spec.rb
[convention] 284-284: Context description should match /^when\b/, /^with\b/, or /^without\b/.
(RSpec/ContextWording)
[convention] 287-298: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 301-301: Context description should match /^when\b/, /^with\b/, or /^without\b/.
(RSpec/ContextWording)
[convention] 304-315: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 333-344: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 355-369: Example has too many lines. [13/5]
(RSpec/ExampleLength)
| it 'handles generic client errors' do | ||
| stub_api_send 418, '🫖' do | ||
| expect { send_mail }.to raise_error(Mailtrap::Error, 'client error') | ||
| expect { send_mail }.to raise_error(Mailtrap::Error, 'client error:, 🫖') | ||
| end | ||
| end |
There was a problem hiding this comment.
Expectation will never match the raised message
handle_response now raises the client-error branch with
raise Mailtrap::Error, ['client error:', response.body] – Ruby turns that
array into the string ["client error:", "🫖"].
The spec expects 'client error:, 🫖', so the example will fail once it is
executed.
- expect { send_mail }.to raise_error(Mailtrap::Error, 'client error:, 🫖')
+ expect { send_mail }.to raise_error(Mailtrap::Error, '["client error:", "🫖"]')(Alternatively – and preferably – fix the implementation to raise a clean
string; see client.rb comment.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In spec/mailtrap/client_spec.rb around lines 216 to 220, the test expects the
error message to be a single string 'client error:, 🫖', but the implementation
raises an error with an array converted to a string, resulting in '["client
error:", "🫖"]'. To fix this, update the test expectation to match the actual
raised message format or preferably modify the implementation in client.rb to
raise a clean concatenated string error message instead of an array, ensuring
the raised error message matches the test expectation.
| raise Mailtrap::Error, ['client error:', response.body] | ||
| when Net::HTTPServerError |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error message formatting – use a string, not an Array
Raising with an Array produces a cryptic ["client error:", "…"] message and
breaks the spec. Emit a plain string instead.
- when Net::HTTPClientError
- raise Mailtrap::Error, ['client error:', response.body]
+ when Net::HTTPClientError
+ raise Mailtrap::Error, "client error: #{response.body}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise Mailtrap::Error, ['client error:', response.body] | |
| when Net::HTTPServerError | |
| when Net::HTTPClientError | |
| raise Mailtrap::Error, "client error: #{response.body}" | |
| when Net::HTTPServerError |
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 188 to 189, the raise statement uses an
Array to format the error message, which results in a cryptic output and breaks
the spec. Change the Array to a single string by concatenating or interpolating
the message and response body so that the error message is a plain, readable
string.
| perform_request(:post, uri, { | ||
| base: compact_with_empty_arrays(base.as_json), | ||
| requests: | ||
| }) |
There was a problem hiding this comment.
Syntax error & double-encoding in payload construction
requests: is missing a value, which makes the file uncompilable.
Even once a value is supplied, passing raw Mail::Base objects will JSON-encode
twice (object → JSON string, then JSON string re-encoded).
- perform_request(:post, uri, {
- base: compact_with_empty_arrays(base.as_json),
- requests:
- })
+ payload = {
+ base: compact_with_empty_arrays(base.as_json),
+ requests: requests.map { |r| compact_with_empty_arrays(r.as_json) }
+ }
+ perform_request(:post, uri, payload)This produces valid JSON and eliminates the previously reported double-encoding
issue.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| perform_request(:post, uri, { | |
| base: compact_with_empty_arrays(base.as_json), | |
| requests: | |
| }) | |
| payload = { | |
| base: compact_with_empty_arrays(base.as_json), | |
| requests: requests.map { |r| compact_with_empty_arrays(r.as_json) } | |
| } | |
| perform_request(:post, uri, payload) |
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 71 to 74, the payload construction has a
syntax error because the `requests:` key is missing a value, causing the code to
be uncompilable. To fix this, assign the correct value to `requests:` by
converting the `Mail::Base` objects to hashes or JSON-compatible structures
before passing them, avoiding double JSON encoding. Ensure that the objects are
serialized only once by using a method like `as_json` or equivalent to produce a
JSON-ready hash instead of raw objects or JSON strings.
Motivation
Integrate Batch Sending API to support sending multiple emails in a single request, improving efficiency and reducing API calls.
Changes
send_batchtoMailtrap::ClientHow to test
or set yout api key
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests