Deprecate Mailtrap::Mail::FromTemplate #54
Conversation
WalkthroughThis update introduces new factory methods for constructing email objects with or without templates, enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailtrapClient
participant MailObject
participant MailtrapAPI
User->>MailObject: Create via from_content or from_template
User->>MailtrapClient: send(MailObject or Hash)
MailtrapClient->>MailObject: to_json (if needed)
MailtrapClient->>MailtrapAPI: POST /api/send (JSON body)
MailtrapAPI-->>MailtrapClient: Response
MailtrapClient-->>User: Response
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 (
|
i7an
force-pushed
the
deprecate-from-template
branch
from
44c15ef to
db0c765
Compare
i7an
force-pushed
the
deprecate-from-template
branch
from
db0c765 to
8a46ffb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
README.md (2)
49-58: Example now usesMail.from_content– good callUsing the builder clarifies intent and steers newcomers away from manual instantiation.
Minor nit: consider adding a short comment that the method returns aMailtrap::Mail::Baseobject to hint at the underlying type for readers skimming the docs.
64-72: Hash-basedclient.sendexample is valuableDemonstrates the relaxed parameter contract well.
You might mention that any object responding to#to_jsonis accepted, not only raw hashes, to avoid readers inferring hash-only support.lib/mailtrap/mail.rb (3)
16-27: Consider adding parameter validation for required fields.Both factory methods accept many optional parameters, but some combinations might be invalid (e.g., template emails without
template_uuid, or content emails without any content). Consider adding basic validation to provide better error messages.Example validation for
from_template:def from_template( # rubocop:disable Metrics/ParameterLists from: nil, to: [], reply_to: nil, cc: [], bcc: [], attachments: [], headers: {}, custom_variables: {}, template_uuid: nil, template_variables: {} ) + raise ArgumentError, 'template_uuid is required for template-based emails' if template_uuid.nil? + raise ArgumentError, 'from address is required' if from.nil? + raise ArgumentError, 'to addresses are required' if to.empty? Mailtrap::Mail::Base.new( from:, to:, # ... rest of parameters ) endAlso applies to: 43-56
12-15: Enhance documentation with usage examples.The documentation could be more comprehensive by including usage examples and clarifying when to use each method.
- # Builds a mail object that will be sent using a pre-defined email - # template. The template content (subject, text, html, category) is - # defined in the Mailtrap dashboard and referenced by the template_uuid. - # Template variables can be passed to customize the template content. + # Builds a mail object that will be sent using a pre-defined email + # template. The template content (subject, text, html, category) is + # defined in the Mailtrap dashboard and referenced by the template_uuid. + # Template variables can be passed to customize the template content. + # + # @param template_uuid [String] The UUID of the template defined in Mailtrap dashboard + # @param template_variables [Hash] Variables to customize the template content + # @param from [Hash] Sender information with :email and optional :name + # @param to [Array<Hash>] Array of recipient hashes with :email and optional :name + # @return [Mailtrap::Mail::Base] Mail object ready to be sent + # + # @example + # mail = Mailtrap::Mail.from_template( + # from: { email: '[email protected]', name: 'Sender' }, + # to: [{ email: '[email protected]', name: 'Recipient' }], + # template_uuid: 'template-uuid-here', + # template_variables: { name: 'John', product: 'Widget' } + # )Also applies to: 42-42
16-16: Consider refactoring to reduce parameter list complexity.The rubocop disable comments indicate these methods have complex parameter lists. Consider using a parameter object or builder pattern to improve maintainability.
Alternative approach using keyword arguments with a parameter object:
def from_template(**params) template_params = TemplateMailParams.new(**params) template_params.validate! Mailtrap::Mail::Base.new(**template_params.to_h) endThis would centralize parameter validation and make the methods more maintainable.
Also applies to: 43-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(2 hunks)examples/email_template.rb(2 hunks)examples/full.rb(2 hunks)lib/mailtrap/client.rb(1 hunks)lib/mailtrap/mail.rb(1 hunks)lib/mailtrap/mail/base.rb(5 hunks)lib/mailtrap/mail/from_template.rb(2 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/when_all_params_are_set/sending_is_successful.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/when_api_key_is_incorrect/raises_authorization_error_with_array_of_errors.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/when_no_subject_and_no_text_set/raises_sending_error_with_array_of_errors.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_an_alternative_host/sending_is_successful.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_bulk_flag/chooses_host_for_bulk_sending.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_bulk_flag_and_alternative_host/chooses_alternative_host.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_sandbox_flag/chooses_host_for_sandbox_sending.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail_is_hash/sends_an_email.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_template/when_all_params_are_set/sending_is_successful.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_template/when_api_key_is_incorrect/raises_authorization_error_with_array_of_errors.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_template/when_using_sandbox/sending_is_successful.yml(1 hunks)spec/mailtrap/client_spec.rb(1 hunks)spec/mailtrap/mail/base_spec.rb(1 hunks)spec/mailtrap/mail/from_template_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
examples/full.rb (2)
lib/mailtrap/mail.rb (1)
from_content(43-71)lib/mailtrap/client.rb (1)
send(61-63)
examples/email_template.rb (2)
lib/mailtrap/mail.rb (1)
from_template(16-40)lib/mailtrap/client.rb (1)
send(61-63)
spec/mailtrap/client_spec.rb (1)
lib/mailtrap/client.rb (1)
send(61-63)
lib/mailtrap/mail/base.rb (2)
lib/mailtrap/action_mailer/delivery_method.rb (1)
attr_accessor(5-25)lib/mailtrap/attachment.rb (1)
attr_accessor(7-49)
lib/mailtrap/mail.rb (1)
lib/mailtrap/mail/base.rb (1)
attachments(71-73)
🔇 Additional comments (28)
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_template/when_api_key_is_incorrect/raises_authorization_error_with_array_of_errors.yml (1)
8-8: Payload simplified – empty collections correctly removedThe cassette now mirrors the new
as_jsonbehaviour, avoiding noisy empty keys. 👍spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_template/when_using_sandbox/sending_is_successful.yml (1)
8-8: Consistent with updated serialization logicEmpty
cc,bcc,headers, andcustom_variablesare gone, keeping the request minimal and aligned with code changes.spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_bulk_flag_and_alternative_host/chooses_alternative_host.yml (1)
8-8: Fixture trimmed to essentialsThe removal of redundant empty fields keeps the cassette tidy and future-proof. Looks good.
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/when_api_key_is_incorrect/raises_authorization_error_with_array_of_errors.yml (1)
8-8: Updated body reflects newMail#as_jsoncontractCassette accurately captures the streamlined payload. No issues spotted.
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_sandbox_flag/chooses_host_for_sandbox_sending.yml (1)
8-8: Fixture follows serialization changeEmpty arrays/hashes removed – cassette remains valid and clearer.
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/when_all_params_are_set/sending_is_successful.yml (1)
6-9: Cassette payload trimmed – looks goodEmpty keys (
cc,bcc,headers,custom_variables) are now omitted, matching the newas_jsonbehaviour. This keeps cassettes smaller and avoids unnecessary diff-noise. 👍spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_template/when_all_params_are_set/sending_is_successful.yml (1)
6-9: Consistent removal of empty fieldsChange aligns template cassette with content cassette updates. All good.
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_an_alternative_host/sending_is_successful.yml (1)
6-9: Alternative-host cassette updated correctlyPayload pruning matches the unified serialization logic. ✔
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/with_bulk_flag/chooses_host_for_bulk_sending.yml (1)
6-9: Bulk-host cassette in syncNo issues spotted; fixture now reflects the compact JSON.
README.md (1)
60-63: Explicit send step kept conciseClear and idiomatic Ruby; no concerns.
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail/when_no_subject_and_no_text_set/raises_sending_error_with_array_of_errors.yml (1)
8-8: LGTM! JSON payload optimization.The removal of empty fields from the JSON request body is a good optimization that reduces payload size and aligns with the updated serialization behavior in the mail classes.
spec/fixtures/vcr_cassettes/Mailtrap_Client/_send/when_mail_is_hash/sends_an_email.yml (1)
1-38: LGTM! New test fixture supports hash-based mail sending.This VCR cassette properly records the HTTP interaction for sending emails using a hash instead of a mail object, supporting the enhanced client flexibility introduced in this PR.
spec/mailtrap/mail/from_template_spec.rb (1)
99-99: LGTM! Test expectations updated for empty field omission.The test correctly expects empty fields to be omitted from the JSON output, aligning with the updated serialization behavior. Since
FromTemplateis being deprecated per the PR objectives, consider whether these tests should be updated to reflect the new patterns or marked for future removal.spec/mailtrap/mail/base_spec.rb (1)
71-71: LGTM! Test expectations updated for cleaner JSON serialization.The test correctly expects empty fields to be omitted from the JSON output, which results in cleaner and more efficient serialization of mail objects.
examples/full.rb (2)
4-8: LGTM! Clear documentation of new factory methods.The comments effectively document the available options for creating mail objects, including the new factory methods that provide a cleaner API.
60-68: LGTM! Excellent demonstration of direct hash sending.This example clearly shows the new flexibility where raw email parameters can be passed directly to
client.send, eliminating the need to create mail objects in simple cases. This aligns perfectly with the PR objective of making the client accept any object responding to#to_json.spec/mailtrap/client_spec.rb (1)
117-136: LGTM! New test properly validates hash-based email sending.The test correctly validates that the client can now accept plain Ruby hashes for email sending, which aligns with the relaxed type constraint on the
sendmethod. The use ofBase64.strict_encode64for attachment content in the hash format is appropriate and consistent with how the Mail object would serialize attachments.examples/email_template.rb (2)
3-14: LGTM! Example properly demonstrates the new factory method.The change from
Mailtrap::Mail::FromTemplate.newtoMailtrap::Mail.from_templatecorrectly demonstrates the new recommended approach for creating templated emails, following the deprecation of theFromTemplateclass.
20-30: LGTM! Excellent demonstration of the new hash-based API.This second example effectively shows users how to send emails by passing parameters directly to the client without creating mail objects, which is now possible due to the relaxed type constraint on the
sendmethod.lib/mailtrap/client.rb (1)
57-58: LGTM! Documentation accurately reflects the relaxed type constraint.The updated documentation correctly indicates that the
sendmethod now accepts any object responding to#to_json, which enables the new hash-based email sending functionality while maintaining backwards compatibility with existing mail objects.lib/mailtrap/mail/from_template.rb (2)
5-5: LGTM! Proper deprecation marking.The
@deprecatedcomment clearly indicates that users should useMailtrap::Mail::Baseinstead, providing clear guidance for migration.
19-19: LGTM! Simplified constructor delegates to Base class.The constructor now simply calls
super, which is appropriate since all template functionality has been moved to theBaseclass. This maintains backwards compatibility while the class is being deprecated.lib/mailtrap/mail/base.rb (4)
8-9: LGTM! Template attributes properly added to Base class.The addition of
template_uuidandtemplate_variablesto theattr_accessordeclaration correctly consolidates template functionality into the Base class, supporting the deprecation of theFromTemplateclass.
25-26: LGTM! Constructor properly supports template parameters.The constructor correctly accepts and initializes the new template parameters, maintaining consistency with the existing parameter handling pattern.
Also applies to: 40-41
59-61: LGTM! JSON serialization enhanced with template fields and empty value filtering.The
as_jsonmethod now includes template fields and uses thepresencehelper withcompactto omit empty values from the JSON output, which improves API efficiency by reducing payload size.
88-90: LGTM! Well-implemented presence helper method.The
presencehelper method correctly identifies empty values by checking if the value responds toempty?and returnsnilfor empty collections, which works well with thecompactmethod to filter out empty fields from JSON serialization.lib/mailtrap/mail.rb (2)
12-40: LGTM! Clean factory method implementation for template-based emails.The
from_templatemethod provides a clear API for creating template-based emails and correctly separates template parameters from content parameters, addressing the issues mentioned in the PR objectives.
42-71: LGTM! Clean factory method implementation for content-based emails.The
from_contentmethod provides a clear API for creating content-based emails with explicit subject, text, html, and category parameters.
| def send(mail) | ||
| raise ArgumentError, 'should be Mailtrap::Mail::Base object' unless mail.is_a? Mail::Base |
| # create mail object | ||
| mail = Mailtrap::Mail::Base.new( | ||
| # Create mail object | ||
| mail = Mailtrap::Mail.from_content( |
There was a problem hiding this comment.
Other name could be from_hash
There was a problem hiding this comment.
I wanted to highlight the fact that mail object is instantiated with text and html in contrast with from_template where template_uuid is provided.
Motivation
The goal of this PR is to prepare the codebase for the Batch Sending API. The Batch Sending API has
BatchEmailBase. There are several ways how to implement it.Get rid of hierarchy of the mail classes
The existing hierarchy is broken:
The client relies solely on the
to_jsonmethod to build a request. The client depends more on the interface than on a base class.I suggest to get rid of
FromTemplateclass and avoid introducing new subclasses. TheBaseclass extended with templates would do the trick. Input validation will be handled by the server.Pros:
Cons:
Rework the hierarchy of the mail classes
Base
BatchContentBase
BatchTemplateBase
ContentMail
TemplateMail
Pros:
Cons:
to_jsonmethod.Baseclass should be renamed, which would constitute a breaking change, or a parallel hierarchy should be introduced instead.Changes
Mailtrap::Mail::FromTemplateHow to test
Mailtrap::Mail::FromTemplateMailtrap::Mail::BaseMailtrap::Mail.from_templateMailtrap::Mail.from_contentSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor