Batch Sending API - #53
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces batch email sending capabilities to the Mailtrap Ruby client. It adds new methods for constructing batch emails, a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailtrapClient
participant MailtrapAPI
User->>MailtrapClient: Initialize with api_key (and options)
User->>MailtrapClient: send_batch(base_mail, recipient_mails)
MailtrapClient->>MailtrapAPI: POST /api/batch with base_mail and recipient_mails
MailtrapAPI-->>MailtrapClient: Batch send response (success/errors)
MailtrapClient-->>User: Returns API response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related issues
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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. 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: 3
🧹 Nitpick comments (7)
examples/batch.rb (2)
10-15: PreferBase64.strict_encode64to avoid soft line-breaks
Base64.encode64inserts\nevery 60 chars, which can mangle attachments or exceed Mailtrap’s size limits.- content: Base64.encode64('Attachment content'), # base64 encoded content or IO string + content: Base64.strict_encode64('Attachment content'), # single-line base64
24-24: Use ENV for secrets in examplesHard-coding even a placeholder key encourages bad habits.
-client = Mailtrap::Client.new(api_key: 'your-api-key') +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY'))lib/mailtrap/client.rb (2)
67-70: Consider optimizing the validation logic.The
all?method call could be more efficient by usingis_a?with the method argument.- unless requests.all?(Mail::Base) - raise ArgumentError, - 'requests should be an array of Mailtrap::Mail::Base objects' - end + unless requests.all? { |request| request.is_a?(Mail::Base) } + raise ArgumentError, + 'requests should be an array of Mailtrap::Mail::Base objects' + end
31-55: Note: Cyclomatic complexity increase due to additional validation.The pipeline failure indicates that the
initializemethod's cyclomatic complexity has exceeded the threshold (8/7). While the new validation on line 42 is necessary and follows the existing pattern, consider refactoring the initialization logic to reduce complexity.Consider extracting validation logic into a separate private method:
private def validate_configuration!(sandbox, bulk, inbox_id) raise ArgumentError, 'bulk mode is not applicable for sandbox API' if sandbox && bulk raise ArgumentError, 'inbox_id is required for sandbox API' if sandbox && inbox_id.nil? endWould you like me to generate a refactored version of the
initializemethod to address the complexity issue?spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/in_sandbox_mode/successfully_sends_a_batch_of_emails.yml (1)
8-10: Use reserved example domains to avoid accidental trafficThe cassette payload embeds
mailtrap@demomailtrap.coandto@mail.com. While harmless, RFC 2606 recommendsexample.com,example.org, orexample.netfor documentation & fixtures to guarantee they never resolve in production.
Switching keeps us 100 % safe from stray e-mails if the cassette is ever replayed against live endpoints.spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_template/successfully_sends_a_batch_of_emails_with_template.yml (1)
8-9: Consider RFC-2606 example domains in fixture addressesSame note as the sandbox cassette—replace real-looking domains (
mailtrap@demomailtrap.co,to@mail.com) withexample.comvariants to remove any chance of misrouting.spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/in_bulk_mode/successfully_sends_a_batch_of_emails.yml (1)
8-9: Prefer placeholder domains reserved for docsFor consistency and zero-risk, substitute
mailtrap@demomailtrap.co/to@mail.comwithexample.comaddresses in the recorded JSON string.
📜 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(4 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(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(64-76)
spec/mailtrap/client_spec.rb (2)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/client.rb (1)
send_batch(64-76)
lib/mailtrap/client.rb (3)
lib/mailtrap/mail/from_template.rb (1)
as_json(34-41)lib/mailtrap/mail/base.rb (1)
as_json(39-55)lib/mailtrap/attachment.rb (1)
as_json(19-27)
🪛 RuboCop (1.75.5)
spec/mailtrap/client_spec.rb
[convention] 303-303: Context description should match /^when\b/, /^with\b/, or /^without\b/.
(RSpec/ContextWording)
[convention] 320-320: Context description should match /^when\b/, /^with\b/, or /^without\b/.
(RSpec/ContextWording)
🪛 GitHub Actions: Ruby
spec/mailtrap/client_spec.rb
[error] 303-303: Rubocop RSpec/ContextWording: Context description should match /^when\b/, /^with\b/, or /^without\b/.
[error] 320-320: Rubocop RSpec/ContextWording: Context description should match /^when\b/, /^with\b/, or /^without\b/.
lib/mailtrap/client.rb
[error] 31-31: Rubocop Metrics/CyclomaticComplexity: Cyclomatic complexity for initialize is too high. [8/7]
🔇 Additional comments (6)
examples/batch.rb (1)
35-46: Confirm per-recipient minimal payload is acceptedEach
Mailtrap::Mail::Baseinrequestsonly setsto:. If the API does not auto-inheritfromandsubjectfrombase, the call will reproduce the 422 validation error captured in the cassette ('from' is required,'subject' is required). Please verify this with a live call or spec.spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_API_errors/handles_API_errors.yml (1)
45-47: Check double-quoted field names in error stringsThe response shows
''from'' is required(double single-quotes). If the client surfaces these messages directly, they’ll look odd to users. Consider normalising or stripping surplus quotes when presenting validation errors.spec/mailtrap/client_spec.rb (1)
254-390: Comprehensive test coverage for the batch sending feature.The test suite thoroughly covers various scenarios including bulk mode, sandbox mode, template usage, and error handling. The use of VCR cassettes for HTTP interaction recording is appropriate for integration testing.
lib/mailtrap/client.rb (3)
57-76: Well-implemented batch sending method with proper validation.The implementation follows established patterns from the existing
sendmethod and includes comprehensive parameter validation. The use ofcompact_with_empty_arraysfor data cleaning is appropriate for API payloads.
146-148: Consistent path generation following established patterns.The
batch_request_pathmethod properly mirrors the structure ofsend_pathwith appropriate sandbox inbox ID handling.
215-217: Useful utility method for API payload cleaning.The
compact_with_empty_arraysmethod provides more thorough data cleaning than the standardcompactmethod by also removing empty arrays, which is appropriate for API payloads.
| perform_request(:post, api_host, batch_request_path, { | ||
| base: compact_with_empty_arrays(base.as_json), | ||
| requests: | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the requests array transformation in the API payload.
The requests array is passed directly without applying compact_with_empty_arrays transformation. Ensure this is intentional and that individual request objects don't require the same cleaning as the base object.
🏁 Script executed:
#!/bin/bash
# Description: Check how requests array is handled in the codebase and verify the API expects raw request objects
# Search for batch API usage patterns and documentation
rg -A 10 -B 5 "batch.*api|api.*batch" --type md
# Look for any examples or documentation about batch request format
fd -e rb -e md | xargs rg -l "send_batch|batch.*request" | head -5 | xargs catLength of output: 20855
🏁 Script executed:
#!/bin/bash
# Inspect Mailtrap::Mail::Base class and its JSON serialization methods
rg -n "class Base" -A10 -B2 --type rbLength of output: 66
🏁 Script executed:
#!/bin/bash
# Locate and inspect Mailtrap::Mail::Base#as_json implementation
files=$(fd -e rb base.rb)
for f in $files; do
echo "=== Inspecting $f ==="
rg -n "def as_json" -A20 -B2 "$f"
doneLength of output: 887
🏁 Script executed:
#!/bin/bash
# Display Mailtrap::Mail::Base to inspect defaults, initialize, and to_json
sed -n '1,200p' lib/mailtrap/mail/base.rbLength of output: 2019
Clean request objects in batch payload
In send_batch (lib/mailtrap/client.rb), we apply compact_with_empty_arrays to the base JSON but pass each request raw. Since Mailtrap::Mail::Base#as_json only uses Hash#compact (removing nils but not empty arrays), any empty‐array fields (e.g. cc, bcc, attachments, headers, custom_variables) will still be sent. To ensure consistency and avoid sending unwanted empty arrays, wrap each request in the same cleanup:
• File: lib/mailtrap/client.rb
Method: send_batch
Proposed change:
- perform_request(:post, api_host, batch_request_path, {
- base: compact_with_empty_arrays(base.as_json),
- requests:
- })
+ perform_request(:post, api_host, batch_request_path, {
+ base: compact_with_empty_arrays(base.as_json),
+ requests: requests.map { |req|
+ compact_with_empty_arrays(req.as_json)
+ }
+ })If the API is fine with empty arrays in individual requests, please confirm this is intentional.
📝 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, api_host, batch_request_path, { | |
| base: compact_with_empty_arrays(base.as_json), | |
| requests: | |
| }) | |
| perform_request(:post, api_host, batch_request_path, { | |
| base: compact_with_empty_arrays(base.as_json), | |
| requests: requests.map { |req| | |
| compact_with_empty_arrays(req.as_json) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 72 to 75, the send_batch method applies
compact_with_empty_arrays to the base JSON but sends each request without
cleaning empty arrays, causing unwanted empty array fields to be included. To
fix this, apply compact_with_empty_arrays to each request's as_json output
before including it in the requests array, ensuring all empty arrays are removed
consistently from both base and individual requests.
| end | ||
|
|
||
| # Sends a batch of emails | ||
| # @param base [Mailtrap::Mail::Base] The base email configuration |
There was a problem hiding this comment.
According to API, base parameter has less fields than Mailtrap::Mail::Base. Please consider those options:
- Dedicated class for
basee.g.Mailtrap::Batch::Base - Raise
ArgumentErrorifbasehasto,cc,bcc
@IgorDobryn what do you think?
There was a problem hiding this comment.
I would go with dedicated class, so in batch_send method we won't need to do base.as_json.except('to', 'cc', 'bcc') and having dedicated class would be more descriptive and straitforward
There was a problem hiding this comment.
The problem with the dedicated class is that you'd need two of them: one for regular emails, another for template emails. This creates potentially confusing hierarchy. Having said that, I agree with your point.
There was a problem hiding this comment.
Valid point. In that case, what about having dedicated serializer for Mailtrap::Mail::Base handling batch case and raise errors there, like
perform_request(:post, api_host, batch_request_path, {
base: BaseBatchSerializer(base).to_h,
...,
})
class BaseBatchSerializer
def initialize(base)
raise ArgumentError unless base.to.empty?
end
def to_h
@base.as_json(%w[to cc, bcc])
end
ans as result we will have dedicated batch logic encapsulated somewhere
There was a problem hiding this comment.
Let's use separate class. Otherwise it will create misexpectations. I.e. when somebody constructs mail object with to/cc/bcc
There was a problem hiding this comment.
- please remove unnecessary requires
- update formatting making it similar to the existing example files
- make sure the example works
- the the example more unique by removing the comments that present in other files.
| Mailtrap::Mail::Base.new( | ||
| to: [ | ||
| { | ||
| email: ENV.fetch('MAILTRAP_TO_EMAIL', 'to@mail.com'), |
There was a problem hiding this comment.
Why do you need ENV.fetch here? Could you please explain the use case?
There was a problem hiding this comment.
Only to not expose own email address, as sender mailtrap@demomailtrap.co allows you to send only to account owner.
| end | ||
|
|
||
| perform_request(:post, api_host, batch_request_path, { | ||
| base: compact_with_empty_arrays(base.as_json), |
There was a problem hiding this comment.
- how about
base.as_json.except('to', 'cc', 'bcc')? - why do you want to
compactbase json?
There was a problem hiding this comment.
why do you want to compact base json?
only to remove 'to', 'cc', 'bcc', which in case of base has to be empty
There was a problem hiding this comment.
It wont be necessary once you add classes for base.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/mailtrap/client.rb (1)
72-75: Apply consistent payload cleaning to request objects.Based on previous review feedback, the
requestsarray should have the same empty array cleaning applied as the base object to ensure consistency and avoid sending unwanted empty fields.Apply the same
compact_with_empty_arraystreatment to each request:perform_request(:post, api_host, batch_request_path, { base: base.as_json.except('to', 'cc', 'bcc'), - requests: + requests: requests.map { |req| compact_with_empty_arrays(req.as_json) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/batch.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(4 hunks)lib/mailtrap/mail/base.rb(1 hunks)lib/mailtrap/mail/batch/base.rb(1 hunks)lib/mailtrap/mail/batch/from_template.rb(1 hunks)lib/mailtrap/mail/from_template.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_bulk_mode/successfully_sends_a_batch_of_emails.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_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(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/mailtrap.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_template/successfully_sends_a_batch_of_emails_with_template.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/with_API_errors/handles_API_errors.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_bulk_mode/successfully_sends_a_batch_of_emails.yml
- spec/mailtrap/client_spec.rb
- examples/batch.rb
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_sandbox_mode/successfully_sends_a_batch_of_emails.yml
🧰 Additional context used
🧬 Code Graph Analysis (3)
lib/mailtrap/mail/from_template.rb (1)
lib/mailtrap/mail/base.rb (1)
to_batch(84-96)
lib/mailtrap/mail/base.rb (1)
lib/mailtrap/mail/from_template.rb (1)
to_batch(46-56)
lib/mailtrap/mail/batch/base.rb (3)
lib/mailtrap/mail/batch/from_template.rb (3)
attr_accessor(13-51)initialize(21-39)as_json(43-50)lib/mailtrap/mail/base.rb (4)
attr_accessor(7-97)initialize(11-37)attachments(64-66)as_json(39-55)lib/mailtrap/client.rb (2)
attr_reader(8-221)initialize(31-52)
🔇 Additional comments (7)
lib/mailtrap/mail/base.rb (1)
81-96: Well-implemented conversion method.The
to_batchmethod correctly converts a single mail object to a batch mail object, preserving all relevant attributes while excluding recipient-specific fields that are handled separately in batch operations.lib/mailtrap/mail/from_template.rb (1)
43-56: Consistent template-to-batch conversion.The method follows the same pattern as the base class
to_batchmethod while correctly handling template-specific attributes. Good consistency across the codebase.lib/mailtrap/mail/batch/from_template.rb (1)
1-54: Well-structured template batch class.The
FromTemplateclass properly extends the batch base functionality with template-specific attributes and maintains clean inheritance patterns. The implementation correctly handles template UUID and variables while preserving base batch behavior.lib/mailtrap/client.rb (3)
40-40: Good refactoring of validation logic.Extracting argument validation into a dedicated method improves code organization and maintainability.
146-148: Correct batch endpoint path construction.The batch request path correctly mirrors the send path pattern, including sandbox inbox ID when needed.
215-220: Well-organized validation method.The extracted validation method consolidates all argument checks in a logical sequence and provides clear error messages.
lib/mailtrap/mail/batch/base.rb (1)
71-73: Correct JSON serialization for batch base.The
as_jsonmethod correctly excludes recipient fields (to,cc,bcc) that are not relevant for batch base objects, as these are handled in individual requests.
| def send_batch(base, requests) | ||
| unless base.is_a?(Mail::Batch::Base) | ||
| raise ArgumentError, | ||
| 'base should be Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate object' | ||
| end | ||
|
|
||
| unless requests.all?(Mail::Base) | ||
| raise ArgumentError, | ||
| 'requests should be an array of Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate objects' | ||
| end |
There was a problem hiding this comment.
Fix request validation logic.
The validation on line 68 checks requests.all?(Mail::Base) but should validate batch mail objects instead. Batch requests should be instances of Mail::Batch::Base or its subclasses.
- unless requests.all?(Mail::Base)
+ unless requests.all?(Mail::Batch::Base)
raise ArgumentError,
'requests should be an array of Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate objects'
end📝 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) | |
| unless base.is_a?(Mail::Batch::Base) | |
| raise ArgumentError, | |
| 'base should be Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate object' | |
| end | |
| unless requests.all?(Mail::Base) | |
| raise ArgumentError, | |
| 'requests should be an array of Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate objects' | |
| end | |
| def send_batch(base, requests) | |
| unless base.is_a?(Mail::Batch::Base) | |
| raise ArgumentError, | |
| 'base should be Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate object' | |
| end | |
| unless requests.all?(Mail::Batch::Base) | |
| raise ArgumentError, | |
| 'requests should be an array of Mailtrap::Mail::Batch::Base or Mailtrap::Mail::FromTemplate objects' | |
| end | |
| # ... rest of method ... | |
| end |
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 62 to 71, the validation for the requests
array incorrectly checks if all elements are instances of Mail::Base, but it
should verify that each element is an instance of Mail::Batch::Base or its
subclasses. Update the validation condition to check
requests.all?(Mail::Batch::Base) to correctly enforce the expected type for
batch mail objects.
| def initialize( # rubocop:disable Metrics/ParameterLists,Lint/MissingSuper | ||
| from: nil, | ||
| reply_to: nil, | ||
| subject: nil, | ||
| text: nil, | ||
| html: nil, | ||
| attachments: [], | ||
| headers: {}, | ||
| custom_variables: {}, | ||
| category: nil | ||
| ) | ||
| @from = from | ||
| @reply_to = reply_to | ||
| @subject = subject | ||
| @text = text | ||
| @html = html | ||
| self.attachments = attachments | ||
| @headers = headers | ||
| @custom_variables = custom_variables | ||
| @category = category | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix initialization to properly inherit from parent class.
The initializer disables Lint/MissingSuper but doesn't call the parent's initialize method. This could cause issues with inherited functionality, particularly attachment handling which relies on the parent's setter method.
- def initialize( # rubocop:disable Metrics/ParameterLists,Lint/MissingSuper
+ def initialize( # rubocop:disable Metrics/ParameterLists
from: nil,
reply_to: nil,
subject: nil,
text: nil,
html: nil,
attachments: [],
headers: {},
custom_variables: {},
category: nil
)
+ super(
+ from: from,
+ reply_to: reply_to,
+ subject: subject,
+ text: text,
+ html: html,
+ attachments: attachments,
+ headers: headers,
+ custom_variables: custom_variables,
+ category: category,
+ to: [], # Batch emails don't have direct recipients
+ cc: [],
+ bcc: []
+ )
- @from = from
- @reply_to = reply_to
- @subject = subject
- @text = text
- @html = html
- self.attachments = attachments
- @headers = headers
- @custom_variables = custom_variables
- @category = category
end🤖 Prompt for AI Agents
In lib/mailtrap/mail/batch/base.rb lines 47 to 67, the initialize method
disables the Lint/MissingSuper cop but does not call super, which can break
inherited behavior. Fix this by adding a call to super with the appropriate
arguments at the start of the initialize method to ensure the parent class's
initialization logic, especially for attachments, is executed properly.
| # @param attachments [Array<Hash>] The attachments for the email. | ||
| # @param headers [Hash] The custom headers for the email. | ||
| # @param custom_variables [Hash] The custom variables for the email. | ||
| class Base < Mailtrap::Mail::Base |
There was a problem hiding this comment.
The hierarchy should be reworked. I'll have to prepare a separate PR with the refactoring. Until that, this PR is on hold.
There was a problem hiding this comment.
@sarco3t I've merged hierarchy refactoring. There is no need to create a dedicated class for batch base. Feel free to use just Base. No need to exclude anything from the to_json output, simply rely on the server validations.
f93bbd3 to
7ecb29b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/batch.rb (2)
6-13: Simplify configuration comments to make the example more unique.The configuration comments appear to duplicate content from other example files. Per previous feedback, these should be more concise and unique to the batch sending example.
Consider simplifying to focus on batch-specific configuration:
-# Set your API credentials as environment variables -# export MAILTRAP_API_KEY='your-api-key' -# -# client = Mailtrap::Client.new -# Bulk sending (@see https://help.mailtrap.io/article/113-sending-streams) -# client = Mailtrap::Client.new(bulk: true) -# Sandbox sending (@see https://help.mailtrap.io/article/109-getting-started-with-mailtrap-email-testing) -# client = Mailtrap::Client.new(sandbox: true, inbox_id: 12) +# For bulk or sandbox mode: +# client = Mailtrap::Client.new(bulk: true) +# client = Mailtrap::Client.new(sandbox: true, inbox_id: 12)
1-74: Improve overall structure and formatting consistency.The example file should follow the formatting patterns of other example files to maintain consistency across the codebase. Consider adding a brief header comment and ensuring the structure aligns with existing examples.
Consider adding a header comment to explain the example:
+# frozen_string_literal: true + +# This example demonstrates batch email sending using the Mailtrap Ruby client. +# It shows two approaches: regular batch sending and template-based batch sending. + require 'mailtrap'
🧹 Nitpick comments (4)
examples/batch.rb (4)
30-32: Update the hardcoded year to be more current.The hardcoded year 2022 in the custom variables makes the example feel outdated.
custom_variables: { - year: 2022 + year: 2024 }
35-46: Use different email addresses to better demonstrate batch sending.Using the same email address for both recipients doesn't effectively showcase batch sending functionality. Different email addresses would provide a more realistic example.
Mailtrap::Mail::Base.new( to: [ - { email: 'your@email.com', name: 'recipient1' } + { email: 'recipient1@email.com', name: 'recipient1' } ] ), Mailtrap::Mail::Base.new( to: [ - { email: 'your@email.com', name: 'recipient2' } + { email: 'recipient2@email.com', name: 'recipient2' } ] )
48-48: Make the comment more specific to differentiate from the first example.The comment is identical to line 15, which doesn't help users understand the difference between the two approaches.
-# Batch sending with Mailtrap::Mail::Base +# Batch sending with templates using Mailtrap::Mail::Base
58-73: Improve recipient diversity and template variable clarity.Similar to the first example, using the same email address doesn't effectively demonstrate batch sending. Additionally, the template variables could be more meaningful.
Mailtrap::Mail::Base.new( to: [ - { email: 'your@email.com', name: 'recipient1' } + { email: 'recipient1@email.com', name: 'recipient1' } ] ), Mailtrap::Mail::Base.new( to: [ - { email: 'your@email.com', name: 'recipient2' } + { email: 'recipient2@email.com', name: 'recipient2' } ], template_variables: { - 'user_name' => 'John Doe 1', - 'user_name2' => 'John Doe 2' + 'user_name' => 'Jane Smith', + 'user_id' => '12345' } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/batch.rb(1 hunks)lib/mailtrap/client.rb(4 hunks)lib/mailtrap/mail/from_template.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_bulk_mode/successfully_sends_a_batch_of_emails.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/mailtrap/mail/from_template.rb
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_bulk_mode/successfully_sends_a_batch_of_emails.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_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/with_template/successfully_sends_a_batch_of_emails_with_template.yml
- spec/mailtrap/client_spec.rb
- lib/mailtrap/client.rb
🔇 Additional comments (1)
examples/batch.rb (1)
1-2: Keep explicitrequire 'base64'Our check confirmed that the Base64 module isn’t auto-loaded in Ruby, so the explicit
require 'base64'on line 2 is necessary. No changes needed.
| ) | ||
|
|
||
| client.send_batch(mail, [ | ||
| Mailtrap::Mail::Base.new( |
There was a problem hiding this comment.
minor. the excessive indentation does not look pretty. how about reformatting it
client.send_batch(
mail,
[
Mailtrap::Mail::Base.new(
to: [
{ email: 'your@email.com', name: 'recipient1' }
]
),
Mailtrap::Mail::Base.new(
to: [
{ email: 'your@email.com', name: 'recipient2' }
]
)
]
)Applies to other places as well.
| # client = Mailtrap::Client.new(sandbox: true, inbox_id: 12) | ||
|
|
||
| # Batch sending with Mailtrap::Mail::Base | ||
| mail = Mailtrap::Mail::Base.new( |
There was a problem hiding this comment.
what do you think about creating helper methods for base similar to from_template and from_content:
batch_base_from_templatebatch_base_from_content
def batch_base_from_content(
from: nil,
reply_to: nil,
attachments: [],
headers: {},
custom_variables: {},
subject: nil,
text: nil,
html: nil,
category: nil
)
Mailtrap::Mail::Base.new(
from:,
reply_to:,
attachments:,
headers:,
custom_variables:,
subject:,
text:,
html:,
category:
)
end| def validate_args!(api_key, api_port, bulk, sandbox, inbox_id) | ||
| raise ArgumentError, 'api_key is required' if api_key.nil? | ||
| raise ArgumentError, 'api_port is required' if api_port.nil? | ||
| raise ArgumentError, 'bulk mode is not applicable for sandbox API' if bulk && sandbox |
There was a problem hiding this comment.
We call it "bulk stream" in public articles.
https://help.mailtrap.io/article/113-sending-streams
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/mailtrap/mail.rb (3)
125-131: Fix the documentation example to reference the correct method name.The documentation example references
Mailtrap::Mail.from_templateinstead ofMailtrap::Mail.batch_from_template.Apply this diff to fix the documentation:
- # mail = Mailtrap::Mail.from_template( + # mail = Mailtrap::Mail.batch_from_template(
154-158: Fix the documentation example to reference the correct method name.The documentation example references
Mailtrap::Mail.from_contentinstead ofMailtrap::Mail.batch_from_content.Apply this diff to fix the documentation:
- # mail = Mailtrap::Mail.from_content( + # mail = Mailtrap::Mail.batch_from_content(
132-150: Consider refactoring to reduce code duplication.The batch methods have significant code duplication with their regular counterparts. Consider extracting the common logic into a private method or using a more DRY approach.
Here's a potential refactor approach:
def batch_from_template(**args) from_template(**args.except(:to, :cc, :bcc)) end def batch_from_content(**args) from_content(**args.except(:to, :cc, :bcc)) endOr create a private helper method:
private def build_mail_object(recipient_fields: true, **args) params = args.dup unless recipient_fields params.delete(:to) params.delete(:cc) params.delete(:bcc) end Mailtrap::Mail::Base.new(**params) endAlso applies to: 159-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/batch.rb(1 hunks)lib/mailtrap/client.rb(4 hunks)lib/mailtrap/mail.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_bulk_stream/successfully_sends_a_batch_of_emails.yml(1 hunks)spec/mailtrap/client_spec.rb(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/fixtures/vcr_cassettes/Mailtrap_Client/_send_batch/when_in_bulk_stream/successfully_sends_a_batch_of_emails.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/mailtrap/client.rb
- examples/batch.rb
- spec/mailtrap/client_spec.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/mailtrap/mail.rb (1)
lib/mailtrap/mail/base.rb (1)
attachments(71-73)
🔇 Additional comments (1)
lib/mailtrap/mail.rb (1)
120-150: Implementation looks correct for batch sending functionality.Both methods correctly omit recipient fields (
to,cc,bcc) and pass the appropriate parameters toMailtrap::Mail::Base.new, which aligns with the batch sending requirements described in the PR objectives.Also applies to: 152-181
…ing and error messages improvements
db29a61 to
6995844
Compare
| # 'user_name' => 'John Doe' | ||
| # } | ||
| # ) | ||
| def batch_from_template(**args) |
There was a problem hiding this comment.
- the idea here is to provide argument level validation. Dont worry about the verbosity.
baseword is valuable as it better describes what you are building.
| # } | ||
| # ) | ||
| # | ||
| # client.send_batch(mail, [ |
There was a problem hiding this comment.
- rewrite the example to use the helper methods
- change the formatting / indentation
| ) | ||
| end | ||
|
|
||
| # Builds a mail object that will be sent using a pre-defined email |
There was a problem hiding this comment.
please update the documentation putting more emphasis on batch sending and the concept of base. Applies to the second base builder as well.
eeff166 to
fe7b7b8
Compare
Update Batch Sending API Examples
Add batch sending to the main readme
3 participants
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
Tests