Add EmailTemplate API functionality #47
Conversation
…d response error handling Add Email Templates API functionality
WalkthroughThis update introduces a comprehensive Email Templates API feature to the Mailtrap Ruby client. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmailTemplatesAPI
participant Client
participant MailtrapAPI
User->>EmailTemplatesAPI: initialize(account_id, client)
User->>EmailTemplatesAPI: list()
EmailTemplatesAPI->>Client: get("/api/accounts/:id/email_templates")
Client->>MailtrapAPI: GET /api/accounts/:id/email_templates
MailtrapAPI-->>Client: 200 OK (JSON array)
Client-->>EmailTemplatesAPI: parsed data
EmailTemplatesAPI-->>User: array of EmailTemplate
User->>EmailTemplatesAPI: create(options)
EmailTemplatesAPI->>Client: post("/api/accounts/:id/email_templates", options)
Client->>MailtrapAPI: POST /api/accounts/:id/email_templates
MailtrapAPI-->>Client: 201 Created (JSON)
Client-->>EmailTemplatesAPI: parsed data
EmailTemplatesAPI-->>User: EmailTemplate
User->>EmailTemplatesAPI: get(template_id)
EmailTemplatesAPI->>Client: get("/api/accounts/:id/email_templates/:template_id")
Client->>MailtrapAPI: GET /api/accounts/:id/email_templates/:template_id
MailtrapAPI-->>Client: 200 OK (JSON)
Client-->>EmailTemplatesAPI: parsed data
EmailTemplatesAPI-->>User: EmailTemplate
User->>EmailTemplatesAPI: update(template_id, options)
EmailTemplatesAPI->>Client: patch("/api/accounts/:id/email_templates/:template_id", options)
Client->>MailtrapAPI: PATCH /api/accounts/:id/email_templates/:template_id
MailtrapAPI-->>Client: 200 OK (JSON)
Client-->>EmailTemplatesAPI: parsed data
EmailTemplatesAPI-->>User: EmailTemplate
User->>EmailTemplatesAPI: delete(template_id)
EmailTemplatesAPI->>Client: delete("/api/accounts/:id/email_templates/:template_id")
Client->>MailtrapAPI: DELETE /api/accounts/:id/email_templates/:template_id
MailtrapAPI-->>Client: 204 No Content
Client-->>EmailTemplatesAPI: nil
EmailTemplatesAPI-->>User: nil
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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)
spec/spec_helper.rb (1)
14-24: Token masking no-longer returned →<BEARER_TOKEN>filtering silently disabled
config.filter_sensitive_dataexpects the block to return the secret to be substituted.
By appending thegsub!mutations after thematchbranch, the last evaluated expression is now the result ofgsub!, which isnilwhen no substitution occurs. Consequently VCR receivesnil, the bearer token stays in the cassette, and real credentials leak.- if auth_header && (match = auth_header.match(/^Bearer\s+([^,\s]+)/)) - match.captures.first - end - interaction.request.uri.gsub!(%r{/accounts/\d+/}, '/accounts/1111111/') - interaction.response.body.gsub!(/"account_id":\d+/, '"account_id": 1111111') + token = if auth_header && (match = auth_header.match(/^Bearer\s+([^,\s]+)/)) + match.captures.first + end + + # normalise account id + interaction.request.uri.gsub!(%r{/accounts/\d+/}, '/accounts/1111111/') + interaction.response.body.gsub!(/"account_id":\d+/, '"account_id": 1111111') + + tokenEven cleaner: move the URI/body sanitisation into a
before_recordhook so responsibilities are separated.
Please patch ASAP to avoid committing real tokens to VCS.
♻️ Duplicate comments (3)
spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml (1)
16-18: Duplicate of the leaked-token comment above – the same bearer token appears here and should be filtered out.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml (1)
16-18: Duplicate of the leaked-token comment above – the same bearer token appears here and should be filtered out.spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/returns_an_array_of_EmailTemplate_objects.yml (1)
16-18: Duplicate of the leaked-token comment above – the same bearer token appears here and should be filtered out.
🧹 Nitpick comments (10)
examples/email_template.rb (2)
18-19: Avoid hard-coding API keys in source-controlled examplesHard-coding the API key string increases the odds someone copies this verbatim and accidentally pushes a real key later. Prefer environment variables so the example is still copy-paste-able but safe.
-client = Mailtrap::Client.new(api_key: 'your-api-key') +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY'))
21-22:Template.newargument order is unclearYou pass
1_111_111without a named keyword – is that an account id or a project id?
Consider switching to a keyword arg (e.g.account_id:) to make the call self-describing and future-proof if the ctor gains additional positional parameters.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/returns_an_EmailTemplate_object.yml (2)
1-332: Add recorded_with metadata for consistency
This cassette is missing arecorded_with: VCR <version>entry at the end, while other fixtures include it. Please appendrecorded_with: VCR 6.2.0(or the appropriate version) to maintain uniformity.
1-332: Trim dynamic headers to reduce fixture noise
The file contains many ephemeral headers (Date, Cf-Ray, ETag, X-Ratelimit-*), which can make tests brittle. Consider filtering out or truncating irrelevant headers via VCR configuration to improve maintainability.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/returns_true.yml (2)
1-321: Add recorded_with metadata for consistency
This delete cassette lacks arecorded_withentry at the end, which other fixtures include. Please appendrecorded_with: VCR 6.2.0(or current version).
1-321: Filter out ephemeral headers
As with other cassettes, this file lists numerous dynamic headers. Consider leveraging VCR’s header filtering to remove Date, Cf-Ray, and other non-essential fields for cleaner fixtures.spec/mailtrap/template_spec.rb (1)
1-24: Consolidate top-level example groupsRuboCop warns about “multiple top-level example groups”. Wrapping the three
RSpec.describeblocks in a single top-levelRSpec.describe 'EmailTemplate API'(or nesting withcontext) will silence the cop and keep specs organised.lib/mailtrap/client.rb (2)
55-56: Use HTTPS URI builder for clarityAll requests go over TLS (
use_ssl = true) butURI::HTTP.buildimplies plain HTTP. Switching toURI::HTTPS.builddocuments intent and avoids accidental scheme mismatches:-uri = URI::HTTP.build(host: api_host, port: api_port, path: request_url) +uri = URI::HTTPS.build(host: api_host, port: api_port, path: request_url)…and similarly for the helper methods.
Also applies to: 63-64, 72-73, 81-82, 89-90
158-160: Provide structured error details for 4xx responsesFor
Net::HTTPClientErroryou currently return a raw body string; parsing JSON (if possible) keeps error handling consistent with other branches:- raise Mailtrap::Error, ['client error:', response.body] + payload = json_response(response.body) rescue response.body + raise Mailtrap::Error, Array(payload)CHANGELOG.md (1)
1-3: Normalize and enrich changelog entry. The new entry- Add EmailTemplates API functionalitycould be revised to use past tense (“Added”) and include concise details of what was introduced (e.g.,Mailtrap::Template,EmailTemplateRequest,EmailTemplateDTOs, CRUD operations) to maintain consistency and improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
CHANGELOG.md(1 hunks)examples/email_template.rb(2 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(5 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/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(1 hunks)spec/mailtrap/template_spec.rb(1 hunks)spec/spec_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
examples/email_template.rb (2)
lib/mailtrap/template.rb (5)
create(106-112)list(89-92)get(97-100)update(119-125)delete(130-132)lib/mailtrap/client.rb (2)
get(62-65)delete(88-91)
spec/mailtrap/template_spec.rb (3)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/template.rb (7)
list(89-92)get(97-100)create(106-112)update(119-125)delete(130-132)to_h(18-20)to_h(71-73)lib/mailtrap/client.rb (2)
get(62-65)delete(88-91)
lib/mailtrap/template.rb (2)
lib/mailtrap/client.rb (5)
initialize(28-50)get(62-65)post(71-74)patch(80-83)delete(88-91)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
lib/mailtrap/client.rb (2)
lib/mailtrap/template.rb (2)
get(97-100)delete(130-132)lib/mailtrap/mail/base.rb (1)
to_json(57-62)
🪛 RuboCop (1.75.5)
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] 256-264: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 324-332: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 372-384: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 404-416: Example has too many lines. [11/5]
(RSpec/ExampleLength)
🔇 Additional comments (14)
spec/mailtrap/client_spec.rb (1)
216-219: Possible typo in expected error string – stray commaThe spec now expects
'client error:, 🫖'(note comma after the colon).
Unless the implementation really emits that comma, this test will fail and mask genuine regressions.Double-check
Mailtrap::Client#handle_responseformatting; adjust either the code or expectation accordingly.lib/mailtrap.rb (1)
7-7: Template module successfully wired in
require_relative 'mailtrap/template'correctly exposes the new API without altering existing load order. 👍spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml (2)
3-166: Approve: Complete POST create + GET retrieval VCR fixture.
This cassette accurately records the initial POST to create a template and the following GET for the same template, including full headers and JSON bodies.
167-329: Approve: Recorded GET/email_templates/39573with 200 OK.
The second interaction captures the retrieval request and a valid JSON response, ensuring DTO mapping tests have the correct data.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/when_template_does_not_exist/raises_not_found_error.yml (2)
3-167: Approve: POST creation interaction is correctly captured.
Includes the proper JSON payload and a 201 Created response for the template setup.
167-326: Approve: GET for non-existent template yields 404 Not Found.
Captures the expected error response, enabling negative-path tests to verify proper exception handling.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/returns_an_EmailTemplate_object.yml (1)
3-167: Approve: Hash-based create EmailTemplate VCR fixture.
The POST interaction records the hash payload and 201 Created response with all template attributes.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml (2)
3-166: Approve: Create EmailTemplate fixture for POST.
Accurately captures the POST request and 201 Created response with full template data.
167-330: Approve: Subsequent GET/email_templates/:idinteraction.
Records the GET request and 200 OK JSON response, supporting end-to-end create + fetch tests.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/maps_response_data_to_EmailTemplate_object.yml (1)
3-66: Inconsistent summary: GET mapping missing in fixture.
The AI-generated summary mentions a subsequent GET for mapping response data, but this cassette only contains the POST interaction.Likely an incorrect or invalid review comment.
spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml (1)
1-168: LGTM
The request/response mapping is correct, account ID is normalized, andrecorded_withmetadata is present. No changes needed.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml (1)
1-332: LGTM
The cassette accurately captures the POST and PATCH interactions, includes normalized account ID andrecorded_withmetadata.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml (1)
1-168: LGTM
Fixtures properly map response data to the DTO, account ID is scrubbed, andrecorded_withis set.spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/when_api_key_is_incorrect/raises_authorization_error.yml (1)
1-165: VCR fixture for authorization error is valid. The cassette accurately captures the 401 Unauthorized response for an incorrect API token and includes the necessary request and response details for testing.
..._cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml
Outdated
Show resolved
Hide resolved
| Authorization: | ||
| - Bearer 793a32549bc04f67fcd4f800009781f2 |
There was a problem hiding this comment.
Filter the bearer token in fixtures
The cassette still contains a real-looking Authorization: Bearer ... value. Please mask it via VCR.configure { filter_sensitive_data('<API_KEY>') { ENV['MAILTRAP_API_KEY'] } } (or similar) to avoid accidental credential leakage.
Also applies to: 181-182
🤖 Prompt for AI Agents
In
spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/when_template_does_not_exist/raises_not_found_error.yml
around lines 17-18 and 181-182, the Authorization header contains a real bearer
token that should be masked to prevent credential leakage. Update the VCR
configuration to filter this sensitive data by replacing the actual token with a
placeholder like '<API_KEY>' using VCR.configure {
filter_sensitive_data('<API_KEY>') { ENV['MAILTRAP_API_KEY'] } } or a similar
approach.
...s/Mailtrap_Template/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
Outdated
Show resolved
Hide resolved
lib/mailtrap/template.rb
Outdated
| def create(request) | ||
| response = @client.post(base_path, | ||
| { | ||
| email_template: request.to_h | ||
| }) | ||
| EmailTemplate.new(response) | ||
| end | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unvalidated hash requests bypass safeguards
create/update accept a raw Hash; this skips length & presence validation performed by EmailTemplateRequest. Convert hashes to a request object internally:
- response = @client.post(base_path,
- { email_template: request.to_h })
+ normalised = request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request)
+ response = @client.post(base_path,
+ { email_template: normalised.to_h })Apply the same pattern in update.
Also applies to: 119-125
🤖 Prompt for AI Agents
In lib/mailtrap/template.rb around lines 106 to 113 and 119 to 125, the create
and update methods accept raw Hash inputs, bypassing the length and presence
validations done by EmailTemplateRequest. To fix this, convert the incoming hash
to an EmailTemplateRequest object inside these methods before proceeding,
ensuring validations are applied consistently.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
spec/mailtrap/template_spec.rb (1)
3-241: Consider nesting example groups to avoid RuboCop RSpec/MultipleDescribes offenceThe file defines three top-level
RSpec.describeblocks (Mailtrap::Template,Mailtrap::EmailTemplateRequest,Mailtrap::EmailTemplate). RuboCop flags this; nesting them under a single top-level describe keeps the suite organised and silences the cop without losing clarity.Example:
-RSpec.describe Mailtrap::Template do +RSpec.describe Mailtrap do + describe Mailtrap::Template do ... - end + end + + describe Mailtrap::EmailTemplateRequest do + ... + end + + describe Mailtrap::EmailTemplate do + ... + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
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/template_spec.rb(1 hunks)spec/spec_helper.rb(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/returns_an_array_of_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/returns_true.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml
🚧 Files skipped from review as they are similar to previous changes (15)
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/when_api_key_is_incorrect/raises_authorization_error.yml
- spec/spec_helper.rb
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/maps_response_data_to_EmailTemplate_object.yml
🧰 Additional context used
🧬 Code Graph Analysis (2)
spec/mailtrap/template_spec.rb (3)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/template.rb (7)
list(58-61)get(66-69)create(75-82)update(89-96)delete(101-103)to_h(12-14)to_h(40-42)lib/mailtrap/client.rb (2)
get(62-65)delete(88-91)
lib/mailtrap/template.rb (2)
lib/mailtrap/client.rb (5)
initialize(28-50)get(62-65)post(71-74)patch(80-83)delete(88-91)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
🪛 RuboCop (1.75.5)
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)
🔇 Additional comments (1)
lib/mailtrap/template.rb (1)
101-103: Return value of#deleteis ambiguous
@client.deletelikely returns the raw Net::HTTP response ortrue; specs expecttrue. To make the contract explicit and future-proof:def delete(template_id) - @client.delete("#{base_path}/#{template_id}") + response = @client.delete("#{base_path}/#{template_id}") + response.is_a?(TrueClass) || response == '' || response.code.to_i == 204 endDocumenting this behaviour clarifies intent for downstream callers.
spec/mailtrap/template_spec.rb
Outdated
| let!(:created_template) do | ||
| template.create( | ||
| name: 'Test Template', | ||
| subject: 'Test Subject', | ||
| category: 'Test Category', | ||
| body_html: '<div>Test HTML</div>', | ||
| body_text: 'Test Text' | ||
| ) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated live-creation of templates inflates cassette size & test time
Each of the #get, #update, and #delete contexts performs a real template.create via VCR. This triplicates traffic and cassettes for essentially the same fixture data.
Extract one shared let_it_be (or before(:all)) that creates the template once, and reuse its ID across the three groups. This shrinks specs and speeds execution while keeping test intent intact.
Also applies to: 146-153, 214-221
🤖 Prompt for AI Agents
In spec/mailtrap/template_spec.rb around lines 40 to 48, the template is being
created multiple times in different contexts, causing repeated live API calls
that increase cassette size and test duration. Refactor by extracting the
template creation into a shared let_it_be or a before(:all) block that runs once
before all tests, then reuse the created template's ID in the get, update, and
delete contexts. Apply the same refactoring to lines 146-153 and 214-221 to
reduce redundant template creations and improve test efficiency.
lib/mailtrap/template.rb
Outdated
| 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
DRY: identical request-normalisation duplicated in create and update
The two methods share the same three-line snippet. Introduce a private helper to centralise this logic:
- 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
+ def create(request)
+ response = @client.post(base_path, { email_template: normalise_request(request).to_h })
+ EmailTemplate.new(response)
+ end
@@
- def update(template_id, request)
- normalised = request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request)
- response = @client.patch("#{base_path}/#{template_id}", { email_template: normalised.to_h })
- EmailTemplate.new(response)
- end
+ def update(template_id, request)
+ response = @client.patch("#{base_path}/#{template_id}",
+ { email_template: normalise_request(request).to_h })
+ EmailTemplate.new(response)
+ end
+
+ private
+
+ def normalise_request(request)
+ request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request)
+ endAlso applies to: 89-96
🤖 Prompt for AI Agents
In lib/mailtrap/template.rb around lines 75 to 83 and 89 to 96, the request
normalization logic is duplicated in both create and update methods. Extract the
normalization code into a private helper method that takes the request, checks
if it is an EmailTemplateRequest instance, and returns a normalized
EmailTemplateRequest object. Replace the duplicated code in create and update by
calling this new helper method to centralize and DRY the normalization logic.
CHANGELOG.md
Outdated
| @@ -1,3 +1,6 @@ | |||
| ## [2.3.1] - 2025-06-17 | |||
There was a problem hiding this comment.
patch version is for bug fixes. minor version will be bumped but not now. please remove those changes from the PR.
lib/mailtrap/client.rb
Outdated
| @@ -10,6 +10,7 @@ class Client | |||
| BULK_SENDING_API_HOST = 'bulk.api.mailtrap.io' | |||
| SANDBOX_API_HOST = 'sandbox.api.mailtrap.io' | |||
| API_PORT = 443 | |||
| API_HOST = 'mailtrap.io' | |||
There was a problem hiding this comment.
please use the name that would be consistent with the parameter GENERAL_API_HOST
lib/mailtrap/client.rb
Outdated
| @@ -30,7 +31,8 @@ def initialize( # rubocop:disable Metrics/ParameterLists | |||
| api_port: API_PORT, | |||
| bulk: false, | |||
| sandbox: false, | |||
| inbox_id: nil | |||
| inbox_id: nil, | |||
| general_api_host: API_HOST | |||
There was a problem hiding this comment.
please update yard annotations
lib/mailtrap/client.rb
Outdated
| when Net::HTTPBadRequest | ||
| raise Mailtrap::Error, json_response(response.body)[:errors] | ||
| when Net::HTTPUnauthorized | ||
| raise Mailtrap::AuthorizationError, json_response(response.body)[:errors] | ||
| body = json_response(response.body) | ||
| raise Mailtrap::AuthorizationError, [body[:errors] || body[:error]].flatten |
There was a problem hiding this comment.
Please see Array.wrap. Should the same strategy be applied to HTTPBadRequest and HTTPForbidden?
lib/mailtrap/client.rb
Outdated
| json_response(response.body) | ||
| when Net::HTTPNoContent | ||
| true |
There was a problem hiding this comment.
maybe nil would be more inline with "no content" 🤔 thought?
There was a problem hiding this comment.
I would leave true for next reason:
updated = templates.update(created[:id], name: 'Welcome Updated')
if updated
# do something
end
but it fail, if it's might be considered as success
result = templates.delete(created[:id])
if result # won't go there actually because of nil
# do something
end
So for me having nil does create some inconsistancy and unpredictability, in any other case I would agree with use
There was a problem hiding this comment.
in case of error we throw exception. if there is no content / return value, nil seems more semantically correct.
lib/mailtrap/client.rb
Outdated
| def http_client | ||
| @http_client ||= Net::HTTP.new(api_host, api_port).tap { |client| client.use_ssl = true } | ||
| def perform_request(method, uri, body = nil) | ||
| http_client = Net::HTTP.new(uri.host, @api_port) |
There was a problem hiding this comment.
The HTTP client will be instantiated on every request. Not sure how big of a deal it is. It shouldn't be too hard to lazily initialize and cache two clients.
http_clients = Hash.new do |h, host|
h[host] = Net::HTTP.new(host, api_port).tap { |client| client.use_ssl = true }
end
There was a problem hiding this comment.
In general case auth might be more complex, clients might need to refresh remember tokens and refresh them. Or store some rate limiting state. Although it's not the case right now, Id agree with Ivan
lib/mailtrap/template.rb
Outdated
| # @attr_reader body_text [String] The plain text content | ||
| # @attr_reader created_at [String] The creation timestamp | ||
| # @attr_reader updated_at [String] The last update timestamp | ||
| EmailTemplate = Struct.new( |
There was a problem hiding this comment.
We wont be able to modify the API without breaking our clients. If a new field is added to the response, the code that uses it will fail with unknown keywords: foo (ArgumentError). You could of course update library every time a new property is added or you could simply return hash.
I'd appreciate more opinions on that matter 🙏 .
There was a problem hiding this comment.
It's always are tradeoff between having types and structures which allows you have more understanding and predictability or having flexible structure.
another mailtrap SDK's like mailtrap-nodejs or mailtrap-php where I took an inspiration, have same issues.
So I would suggest here to use own structs which will be able to ignore unknown fields to handle you concern, but still have data types
class OwnStruct
attr_accessor :name, :age
def initialize(**attributes)
attributes.each do |k, v|
self.send("#{k}=", v) if respond_to?("#{k}=")
end
end
end
There was a problem hiding this comment.
It would be great to have a strict OpenStruct that accepts dynamic input but raises an error for unknown properties. Stripe uses monkey patching in form of define_method to archive this. But I'd love to avoid monkey patching.
Let's keep your approach with Struct (Data is not supported in ruby 3.1) and pair it with slice to whitelist keys:
EmailTemplate = Struct.new(..., keyword_init: true)
def build_email_template(options)
EmailTemplate.new(options.slice(*EmailTemplate.members))
endThere was a problem hiding this comment.
could you please elaborate on why you decided to use vcr here instead of web mock? vcr has some benefits of course. but the ruby wrapper around api is pretty thin and webmock would be easier to maintain without obvious sacrifices.
There was a problem hiding this comment.
I'm also for using webmock here, just was following vcr practice which was already here
client_spec.rb and other specs.
There was a problem hiding this comment.
We use both. Feel free to use webmock if you gravitate towards it as well.
mailtrap-ruby/spec/mailtrap/client_spec.rb
Line 181 in fee2a44
lib/mailtrap/template.rb
Outdated
| end | ||
| end | ||
|
|
||
| class Template |
There was a problem hiding this comment.
the name should be aligned with out api docs:
EmailTemplate
Also I would like to discuss alternative options that read more naturally, more explicit about what the class does:
EmailTemplatesAPIEmailTemplateAPI
UPD more options:
EmailTemplatesClientEmailTemplatesRepository
There was a problem hiding this comment.
I would go for EmailTemplates because in EmailTemplatesApi API part seems redundant, as it's API client and with EmailTemplate we might have a collision with DTO EmailTemplate if we going to keep one
There was a problem hiding this comment.
When you instantiate Mailtrap::EmailTemplates.new it is not clear what this object represents. Is it collection or email template or something else? API postfix makes it very clear what this thing is.
The naming was discussed internally and EmailTemplatesAPI got more votes.
Feel free to suggest alternatives.
lib/mailtrap/template.rb
Outdated
| # @attr_reader subject [String] The email subject (required, <= 255 chars) | ||
| # @attr_reader body_text [String] The plain text content (<= 10000000 chars) | ||
| # @attr_reader body_html [String] The HTML content (<= 10000000 chars) | ||
| EmailTemplateRequest = Struct.new(:name, :category, :subject, :body_text, :body_html, keyword_init: true) do |
There was a problem hiding this comment.
not sure if this class is needed. I'd probably prefer a simpler approach with params or key args. Especially when EmailTemplateRequest is optional. Would appreciate more opinions.
There was a problem hiding this comment.
Me personally find it useful to understand the data that you work with, or have helpers for it like SendGrid does Data type example, Hash example
and we already enforcing to use ruby objects with email sending with no hash option, so it's also a consistency
mail = Mailtrap::Mail::Base.new(
from: { email: 'mailtrap@example.com', name: 'Mailtrap Test' },
...
)
client.send(mail)
There was a problem hiding this comment.
I generally agree with the point about consistency that you made. But I'm not sure if the reasons behind Mailtrap::Mail::Base should be applied in this case. I'd prioritize ergonomics over consistency here.
understand the data that you work with
Keyword args or yard annotations should do the trick.
examples/email_template.rb
Outdated
| @@ -1,3 +1,4 @@ | |||
| require 'bundler/setup' | |||
There was a problem hiding this comment.
could you please explain the need for this require? require 'mailtrap' should be enough to load email templates api.
examples/email_template.rb
Outdated
| body_html: '<h1>Hello</h1>', | ||
| body_text: 'Hello', | ||
| category: 'welcome' | ||
| ) # or Mailtrap::EmailTemplateRequest |
There was a problem hiding this comment.
What does or mean here? Does this mean that #create accepts different params? - if so, it's better to show it in a separate example
lib/mailtrap/client.rb
Outdated
| end | ||
|
|
||
| def send(mail) | ||
| raise ArgumentError, 'should be Mailtrap::Mail::Base object' unless mail.is_a? Mail::Base | ||
|
|
||
| request = post_request(request_url, mail.to_json) | ||
| response = http_client.request(request) | ||
| uri = URI::HTTP.build(host: api_host, port: api_port, path: request_url) |
There was a problem hiding this comment.
Also, request_url which is actually a path creates some more confusion
lib/mailtrap/client.rb
Outdated
| # Performs a POST request to the specified path | ||
| # @param path [String] The request path | ||
| # @param body [Hash] The request body | ||
| # @return [Hash] The JSON response |
There was a problem hiding this comment.
Not really just a a Hash, according to handle_response it can be a boolean. Also it can raise an error
lib/mailtrap/client.rb
Outdated
| # @param path [String] The request path | ||
| # @return [Hash] The JSON response | ||
| def get(path) | ||
| uri = URI::HTTP.build(host: @general_api_host, port: @api_port, path:) |
There was a problem hiding this comment.
Same question, why not to use attr readers?
lib/mailtrap/client.rb
Outdated
| def http_client | ||
| @http_client ||= Net::HTTP.new(api_host, api_port).tap { |client| client.use_ssl = true } | ||
| def perform_request(method, uri, body = nil) | ||
| http_client = Net::HTTP.new(uri.host, @api_port) |
There was a problem hiding this comment.
In general case auth might be more complex, clients might need to refresh remember tokens and refresh them. Or store some rate limiting state. Although it's not the case right now, Id agree with Ivan
lib/mailtrap/template.rb
Outdated
| normalised = request.is_a?(EmailTemplateRequest) ? request : EmailTemplateRequest.new(**request) | ||
| response = @client.post(base_path, | ||
| { | ||
| email_template: normalised.to_h | ||
| }) |
There was a problem hiding this comment.
So, keyword arguments can replace EmailTemplateRequest completely?
There was a problem hiding this comment.
yes, you can see my motivation to have it like that here #47 (comment)
lib/mailtrap/template.rb
Outdated
| # @attr_reader name [String] The template name (required, <= 255 chars) | ||
| # @attr_reader category [String] The template category (required, <= 255 chars) | ||
| # @attr_reader subject [String] The email subject (required, <= 255 chars) | ||
| # @attr_reader body_text [String] The plain text content (<= 10000000 chars) | ||
| # @attr_reader body_html [String] The HTML content (<= 10000000 chars) |
There was a problem hiding this comment.
Are validations enforced? How are we going to support this logic?
There was a problem hiding this comment.
no, it just hints, like other parts of annotations, enforcing validations on client side would take responsibility from API and break flexibility of it,
There was a problem hiding this comment.
If somebody changes API validation he will likely forget to look into SDK docs. Im suggesting to leave a ref to docs
lib/mailtrap/template.rb
Outdated
| # @param template_id [Integer] The template ID | ||
| # @param request [EmailTemplateRequest, Hash] The template request object or a hash with the same attributes | ||
| # @return [EmailTemplate] Updated template object | ||
| # @raise [ArgumentError] If the request is invalid |
There was a problem hiding this comment.
I guess it raises in other places too, but class docs do not mention that
| raise ArgumentError, "Unsupported HTTP method: #{method}" | ||
| end | ||
|
|
||
| request.body = body.to_json if body |
There was a problem hiding this comment.
Do we expect to set body for get?
There was a problem hiding this comment.
No, but we might not have body for delete, and sometimes even for post too, so don't see much sense to have different request setup methods for get, if you mean that.
…ns, added http clients memoization
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
spec/mailtrap/email_template_spec.rb (2)
3-241: Comprehensive test coverage with minor structural improvements needed.The test suite provides excellent coverage of the EmailTemplatesAPI functionality, including CRUD operations, error scenarios, and DTO validation. The use of VCR cassettes ensures reliable test execution.
Consider nesting the multiple describe blocks to improve organization:
RSpec.describe Mailtrap::EmailTemplatesAPI do # ... existing EmailTemplatesAPI tests + + describe 'DTOs' do + describe Mailtrap::EmailTemplateRequest do + # ... EmailTemplateRequest tests + end + + describe Mailtrap::EmailTemplate do + # ... EmailTemplate tests + end + end end -RSpec.describe Mailtrap::EmailTemplateRequest do - # ... move to nested structure -end - -RSpec.describe Mailtrap::EmailTemplate do - # ... move to nested structure -end
55-62: Consider extracting expected attributes to shared examples.The attribute verification pattern is repeated throughout the tests. Consider using shared examples to reduce duplication and improve maintainability.
lib/mailtrap/email_template.rb (2)
119-119: Incorrect return type documentation for delete method.The documentation indicates the delete method returns
Boolean, but the implementation returns the result of@client.deletewhich should benilfor successful deletion.- # @return [Boolean] true if successful + # @return [nil] nil if successful
135-137: Unused method should be removed.The
build_email_templatemethod is defined but never used in the codebase.- def build_email_template(options) - EmailTemplate.new(options.slice(*EmailTemplate.members)) - endlib/mailtrap/client.rb (1)
108-108: Inconsistent instance variable access.Line 108 uses
@general_api_hostwhile other methods use thegeneral_api_hostreader method for consistency.- uri = URI::HTTP.build(host: @general_api_host, path:) + uri = URI::HTTP.build(host: general_api_host, path:)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
examples/email_template.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/email_template.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/email_template_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/mailtrap.rb
- examples/email_template.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/mailtrap/email_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)
🔇 Additional comments (9)
spec/mailtrap/email_template_spec.rb (1)
24-34: Excellent error handling verification.The authorization error testing properly verifies both the exception type and message content, ensuring robust error handling.
lib/mailtrap/email_template.rb (3)
14-19: Clean DTO implementation with compact hash conversion.The EmailTemplateRequest struct is well-designed with keyword initialization and a compact hash conversion method that filters out nil values.
35-51: Comprehensive EmailTemplate DTO with proper documentation.The EmailTemplate struct includes all necessary fields with clear documentation references to the official API docs.
92-98: Verify request body structure matches API expectations.Ensure the nested
email_templatekey structure aligns with the Mailtrap API specification.Please verify that the API expects the request body in the format
{ email_template: { ... } }:What is the expected request body format for creating email templates in the Mailtrap API?lib/mailtrap/client.rb (5)
13-13: Good addition of GENERAL_API_HOST constant.The new constant follows the established naming pattern and supports the email template functionality.
114-116: Improved HTTP client caching strategy.The caching approach addresses the performance concern raised in past reviews while maintaining SSL configuration.
170-177: Enhanced error handling with response body inclusion.The improved error handling now includes response body content, providing better debugging information for API failures.
141-161: Well-structured request setup method.The method properly handles different HTTP verbs and sets up common headers consistently.
167-168: [web_search]What does the Mailtrap API v1 DELETE /email_templates/{id} endpoint return?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/mailtrap/client.rb (3)
28-29: Update YARD documentation to reflect past review feedback.The documentation needs to be updated to include the new
general_api_hostparameter properly and address past review comments about YARD annotations.-# @param [String] general_api_host The general API hostname for non-sending operations. Default: mailtrap.io. -# @raise [ArgumentError] If api_key or api_port is nil, or if sandbox is true but inbox_id is nil +# @param [String] general_api_host The general API hostname for non-sending operations. Default: GENERAL_API_HOST. +# @raise [ArgumentError] If api_key or api_port is nil, or if sandbox is true but inbox_id is nil, or if incompatible options are provided
109-111: Consider renaming method to align with constant naming.Based on past review feedback, the method name should be consistent with the
GENERAL_API_HOSTparameter naming convention.-def http_clients(host) +def http_client_for(host) @clients[host] ||= Net::HTTP.new(host, api_port).tap { |client| client.use_ssl = true } end
129-134: Simplify perform_request method signature based on past feedback.Past review comments suggest that building a URI is unnecessary since you only use the host and path.
-def perform_request(method, host, path, body = nil) - http_client = http_clients(host) - request = setup_request(method, path, body) - response = http_client.request(request) - handle_response(response) +def perform_request(method, host, path, body = nil) + http_client = http_client_for(host) + request = setup_request(method, path, body) + response = http_client.request(request) + handle_response(response) end
🧹 Nitpick comments (2)
spec/mailtrap/email_template_spec.rb (2)
3-243: Consider nesting the EmailTemplate tests under the main describe block.The static analysis correctly identifies that having multiple top-level describe blocks reduces test organization clarity.
RSpec.describe Mailtrap::EmailTemplatesAPI do # ... existing tests ... + + describe 'Mailtrap::EmailTemplate' do + describe '#initialize' do + # ... existing tests ... + end + + describe '#to_h' do + # ... existing tests ... + end + end end - -RSpec.describe Mailtrap::EmailTemplate do - # ... move tests here ... -end
55-62: Consider extracting attribute expectations to reduce example length.This example could be more concise by using shared examples or helper methods for common attribute checks.
+let(:expected_template_attributes) do + { + id: template_id, + name: 'Test Template', + subject: 'Test Subject', + category: 'Test Category' + } +end it 'maps response data to EmailTemplate object' do - expect(get).to have_attributes( - id: template_id, - name: 'Test Template', - subject: 'Test Subject', - category: 'Test Category' - ) + expect(get).to have_attributes(expected_template_attributes) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
examples/email_template.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/email_template.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/email_template_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml
🚧 Files skipped from review as they are similar to previous changes (15)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml
- examples/email_template.rb
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- lib/mailtrap/email_template.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/mailtrap/email_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] 263-275: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 295-307: Example has too many lines. [11/5]
(RSpec/ExampleLength)
🔇 Additional comments (7)
spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml (1)
1-321: VCR cassette properly records template lifecycle with good data sanitization.The fixture correctly captures the create-then-delete flow for email templates. Sensitive data like bearer tokens and account IDs are properly sanitized, and the HTTP interactions follow the expected pattern (POST → 201 Created, DELETE → 204 No Content).
spec/mailtrap/email_template_spec.rb (1)
9-242: Excellent comprehensive test coverage for the EmailTemplatesAPI.The test suite thoroughly covers all CRUD operations, error scenarios, and edge cases. The use of VCR cassettes ensures consistent and reliable test execution.
lib/mailtrap/client.rb (5)
13-13: Good addition of the GENERAL_API_HOST constant.This constant follows the existing naming pattern and provides a clear default for general API operations.
52-53: Good implementation of HTTP client caching.The client caching strategy addresses the past review feedback about instantiating HTTP clients on every request. This approach efficiently manages multiple host connections.
58-59: Excellent refactoring to use the unified perform_request method.This change centralizes HTTP request handling and eliminates code duplication, following DRY principles.
165-172: Good implementation of Array.wrap strategy for error handling.The error handling correctly implements the Array.wrap pattern suggested in past reviews, ensuring consistent error message handling across different response types.
178-178: Improved client error handling with response body.The addition of response body to client error messages provides better debugging information, addressing past feedback about error context.
examples/email_template.rb
Outdated
| category: 'welcome' | ||
| ) | ||
|
|
||
| puts "Created Template: #{created[:id]}" |
There was a problem hiding this comment.
- Response is not a hash
- minor. The variable names could be refined.
created_template = templates_api.create(...)
puts "Created Template: #{created_template.id}"
lib/mailtrap/client.rb
Outdated
| @@ -44,19 +48,68 @@ def initialize( # rubocop:disable Metrics/ParameterLists | |||
| @bulk = bulk | |||
| @sandbox = sandbox | |||
| @inbox_id = inbox_id | |||
| @general_api_host = general_api_host | |||
| @clients = {} | |||
There was a problem hiding this comment.
please rename to http_clients to make it more consistent with the original name which was http_client.
lib/mailtrap/client.rb
Outdated
| when Net::HTTPPayloadTooLarge | ||
| raise Mailtrap::MailSizeError, ['message too large'] | ||
| when Net::HTTPTooManyRequests | ||
| raise Mailtrap::RateLimitError, ['too many requests'] | ||
| when Net::HTTPClientError | ||
| raise Mailtrap::Error, ['client error'] | ||
| raise Mailtrap::Error, ['client error:', response.body] |
There was a problem hiding this comment.
Each element of the array should be an error.
raise Mailtrap::Error, ["client error '#{response.body}'"]There was a problem hiding this comment.
- file name should match the class name.
- extract
EmailTemplateinto a separate fileemail_template.rb - extract
EmailTempatesAPIinto a separate fileemail_templates_api.rb
- extract
lib/mailtrap/email_template.rb
Outdated
| # @param client [Mailtrap::Client] The client instance | ||
| def initialize(account_id, client = Mailtrap::Client.new) | ||
| @client = client | ||
| @account_id = account_id |
There was a problem hiding this comment.
- minor. i wonder if you can instantiate the default value for client without module?
- minor. please set instance variables in the same order as the parameters.
- minor. please use attr readers for consistency.
lib/mailtrap/email_template.rb
Outdated
| # @raise [Mailtrap::AuthorizationError] If the API key is invalid | ||
| # @raise [Mailtrap::RejectionError] If the server refuses to process the request | ||
| # @raise [Mailtrap::RateLimitError] If too many requests are made | ||
| def create(name:, subject:, category:, body_html:, body_text:) # rubocop:disable Metrics/MethodLength |
There was a problem hiding this comment.
not every parameter is required. please double check and add nil as the default. body_* should be optional.
lib/mailtrap/email_template.rb
Outdated
|
|
||
| # Lists all email templates for the account | ||
| # @return [Array<EmailTemplate>] Array of template objects | ||
| # @raise [Mailtrap::Error] If the API request fails with a client or server error |
There was a problem hiding this comment.
please replace the repeating @raises with a macro
# @!macro api_errors
# @raise [Mailtrap::Error] ...
# @raise [Mailtrap::AuthorizationError] ...
# ...
# @!macro api_errors
def foo
...
end
lib/mailtrap/email_template.rb
Outdated
| category:, | ||
| body_html:, | ||
| body_text: | ||
| }.compact |
There was a problem hiding this comment.
it should be possible to nullify body_* but you filter all the nulls out
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/mailtrap/email_templates_api.rb (1)
45-57: Consider refactoring to reduce method complexity.The create method is functionally correct but has been flagged by rubocop for method length. Consider extracting the payload construction to a private method for better readability.
def create(name:, subject:, category:, body_html: nil, body_text: nil) - response = client.post(base_path, - { - email_template: { - name:, - subject:, - category:, - body_html:, - body_text: - } - }) + payload = build_create_payload(name:, subject:, category:, body_html:, body_text:) + response = client.post(base_path, payload) build_email_template(response) end +private + +def build_create_payload(name:, subject:, category:, body_html:, body_text:) + { + email_template: { + name:, + subject:, + category:, + body_html:, + body_text: + } + } +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
examples/email_template.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/email_template.rb(1 hunks)lib/mailtrap/email_templates_api.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/client_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml
🚧 Files skipped from review as they are similar to previous changes (21)
- lib/mailtrap.rb
- spec/mailtrap/client_spec.rb
- examples/email_template.rb
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml
- lib/mailtrap/email_template.rb
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml
🔇 Additional comments (14)
lib/mailtrap/email_templates_api.rb (5)
15-18: LGTM! Clean initialization pattern.The initialization properly sets up the account ID and client dependency injection pattern, making the class testable and flexible.
23-26: LGTM! Simple and effective list implementation.The method correctly fetches all templates and maps them to domain objects using the helper method.
32-35: LGTM! Consistent get pattern.The implementation follows the same pattern as the list method and properly constructs the resource URL.
86-88: LGTM! Clean delete implementation.The delete method correctly makes the DELETE request and implicitly returns nil, which is appropriate for deletion operations.
92-98: LGTM! Well-designed helper methods.Both helper methods are clean and serve their purposes well. The
build_email_templatemethod safely extracts only the expected attributes, andbase_pathcentralizes URL construction.lib/mailtrap/client.rb (9)
13-15: LGTM! Good addition of general API host support.The new
GENERAL_API_HOSTconstant and its inclusion inattr_readerprovide clean separation between different API endpoints. This addresses the past review comment about naming consistency.
35-45: LGTM! Well-documented parameter addition.The addition of
general_api_hostparameter with proper documentation maintains backward compatibility while extending functionality.
58-60: LGTM! Proper initialization of new features.The initialization of
@general_api_hostand@http_clientshash sets up the infrastructure for the new functionality correctly.
65-66: LGTM! Clean refactoring of send method.The refactored send method now uses the unified
perform_requestmethod, making the codebase more consistent and maintainable.
68-100: LGTM! Excellent addition of RESTful HTTP methods.The new HTTP methods (
get,post,patch,delete) provide a clean, consistent API for interacting with the general API host. The documentation is comprehensive and the implementation follows a uniform pattern.
104-106: LGTM! Efficient HTTP client caching implementation.The
http_clients_formethod provides proper client caching per host, which addresses the past review comment about client instantiation performance. The naming is consistent with the past review feedback.
124-129: LGTM! Well-designed unified request handling.The
perform_requestmethod successfully centralizes HTTP request handling, making the code more maintainable and consistent across different HTTP methods.
131-151: LGTM! Comprehensive request setup method.The
setup_requestmethod handles all supported HTTP methods with proper error handling for unsupported methods. The conditional body assignment (line 145) correctly handles cases where body might be nil, addressing the past review comment about GET requests.
155-174: LGTM! Enhanced response handling with better error context.The improvements to
handle_responseare excellent:
- Added
Net::HTTPCreatedto success cases (line 155)- Added proper
Net::HTTPNoContenthandling returningnil(lines 157-158)- Enhanced error messages include response bodies for client errors (lines 160-167, 173)
- Consistent error handling pattern across different error types
These changes provide much better debugging information and align with REST API best practices.
lib/mailtrap/email_templates_api.rb
Outdated
| category:, | ||
| body_html:, | ||
| body_text: | ||
| }.compact.merge(body_html:, body_text:) |
There was a problem hiding this comment.
example. I want to update only name. wont that nullify body_html and body_text?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
spec/mailtrap/email_template_spec.rb (4)
3-3: Consider consolidating the test structure to improve organization.Having multiple top-level describe blocks can make the test suite harder to navigate. Consider nesting the
EmailTemplatemodel tests under a shared context or organizing them in a separate file.RSpec.describe Mailtrap::EmailTemplatesAPI do # ... existing tests ... + + describe 'EmailTemplate model' do + describe '#initialize' do + # ... move EmailTemplate tests here ... + end + end end - -RSpec.describe Mailtrap::EmailTemplate do - # ... move these tests above ... -endAlso applies to: 319-319
28-33: Enhance error message assertions for better test reliability.The current error assertions check for generic string inclusion, which might be fragile. Consider using more specific matchers or constants for expected error messages.
it 'raises authorization error' do 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 + expect(error.message).to match(/incorrect api token/i) + expect(error.messages).to include(match(/incorrect api token/i)) end end
102-124: Consider removing duplicate test scenarios.The "with hash request" context appears to test identical functionality to the main create test, as both use hash parameters. This duplication doesn't add value and increases maintenance overhead.
- context 'with hash request' do - let(:request) do - { - name: 'New Template', - subject: 'New Subject', - category: 'New Category', - body_html: '<div>New HTML</div>', - body_text: 'New Text' - } - end - - it 'returns an EmailTemplate object' do - expect(create).to be_a(Mailtrap::EmailTemplate) - end - - it 'maps response data to EmailTemplate object' do - expect(create).to have_attributes( - name: 'New Template', - subject: 'New Subject', - category: 'New Category' - ) - end - end
200-221: Extract helper method for partial update tests.The partial update tests have repetitive structure. Consider extracting a shared helper method to reduce duplication and improve maintainability.
Add a helper method:
def expect_partial_update(updated_attrs, preserved_attrs) expect(update).to be_a(Mailtrap::EmailTemplate) expect(update).to have_attributes(updated_attrs) expect(update).to have_attributes(preserved_attrs) endThen use it in the tests:
context 'when updating only category' do let(:request) { { category: 'New Category Only' } } - it 'returns an EmailTemplate object' do - expect(update).to be_a(Mailtrap::EmailTemplate) - end - - it 'updates only the category field' do - expect(update).to have_attributes( - category: 'New Category Only' - ) - end - - it 'preserves other fields unchanged' do - expect(update).to have_attributes( - name: 'Original Template', - subject: 'Original Subject', - body_html: '<div>Original HTML</div>', - body_text: 'Original Text' - ) - end + it 'updates only the category field and preserves others' do + expect_partial_update( + { category: 'New Category Only' }, + { + name: 'Original Template', + subject: 'Original Subject', + body_html: '<div>Original HTML</div>', + body_text: 'Original Text' + } + ) + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/preserves_other_fields_unchanged.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/updates_only_the_specified_fields.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/preserves_other_fields_unchanged.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/updates_only_the_body_html_field.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/preserves_other_fields_unchanged.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/updates_only_the_category_field.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/email_template_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/updates_only_the_category_field.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/preserves_other_fields_unchanged.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/updates_only_the_body_html_field.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/preserves_other_fields_unchanged.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/updates_only_the_specified_fields.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/preserves_other_fields_unchanged.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml
🧰 Additional context used
🪛 Gitleaks (8.26.0)
spec/mailtrap/email_template_spec.rb
7-7: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 RuboCop (1.75.5)
spec/mailtrap/email_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] 213-220: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 236-243: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 337-349: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 369-381: Example has too many lines. [11/5]
(RSpec/ExampleLength)
🔇 Additional comments (3)
spec/mailtrap/email_template_spec.rb (3)
40-49: Excellent use of test setup with created template.The use of
let!to create a template before testing thegetmethod is a good pattern that ensures test isolation and proper setup.
337-349: Comprehensive attribute testing is excellent.The initialization test thoroughly validates all attributes of the EmailTemplate model, ensuring proper data mapping and integrity.
369-381: Thorough hash conversion testing.The
#to_hmethod test ensures proper serialization of the model, which is important for data interchange and debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/mailtrap/email_template_spec.rb (1)
6-7: Hardcoded credentials remain a security concern.The API key and account ID are still hardcoded. This was previously flagged and should be addressed by using environment variables or test fixtures.
🧹 Nitpick comments (2)
spec/mailtrap/email_template_spec.rb (2)
3-3: Consider nesting related specs to follow RSpec conventions.RuboCop suggests avoiding multiple top-level describe blocks. Consider nesting under a shared module context:
+RSpec.describe 'Mailtrap Email Templates' do -RSpec.describe Mailtrap::EmailTemplatesAPI do + describe Mailtrap::EmailTemplatesAPI do # existing tests... + end -RSpec.describe Mailtrap::EmailTemplate do + describe Mailtrap::EmailTemplate do # existing tests... + end +endAlso applies to: 319-319
55-62: Consider breaking down long examples for better readability.Several test examples exceed the recommended length. Consider splitting them or extracting shared expectations:
# Example refactor for lines 337-349 +def expect_template_attributes(template) + expect(template).to have_attributes( + id: 26_730, + uuid: '018dd5e3-f6d2-7c00-8f9b-e5c3f2d8a132', + name: 'My Template', + subject: 'My Subject', + category: 'My Category', + body_html: '<div>HTML</div>', + body_text: 'Text', + created_at: '2021-01-01T00:00:00Z', + updated_at: '2021-01-01T00:00:00Z' + ) +end it 'creates a template with all attributes' do - expect(template).to have_attributes( - id: 26_730, - uuid: '018dd5e3-f6d2-7c00-8f9b-e5c3f2d8a132', - name: 'My Template', - subject: 'My Subject', - category: 'My Category', - body_html: '<div>HTML</div>', - body_text: 'Text', - created_at: '2021-01-01T00:00:00Z', - updated_at: '2021-01-01T00:00:00Z' - ) + expect_template_attributes(template) endAlso applies to: 213-220, 236-243, 337-349, 369-381
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/preserves_other_fields_unchanged.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/updates_only_the_specified_fields.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/preserves_other_fields_unchanged.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/updates_only_the_body_html_field.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/preserves_other_fields_unchanged.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/updates_only_the_category_field.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/email_template_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/updates_only_the_category_field.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/returns_an_EmailTemplate_object.yml
🚧 Files skipped from review as they are similar to previous changes (24)
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/maps_response_data_to_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/preserves_other_fields_unchanged.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/returns_no_content.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/with_hash_request/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_create/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/updates_only_the_specified_fields.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/maps_response_data_to_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_category/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_multiple_specific_fields/preserves_other_fields_unchanged.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_list/returns_an_array_of_EmailTemplate_objects.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_get/returns_an_EmailTemplate_object.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/preserves_other_fields_unchanged.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_delete/when_template_does_not_exist/raises_not_found_error.yml
- spec/fixtures/vcr_cassettes/Mailtrap_EmailTemplatesAPI/_update/when_updating_only_body_html/updates_only_the_body_html_field.yml
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/mailtrap/email_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] 213-220: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 236-243: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 337-349: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 369-381: Example has too many lines. [11/5]
(RSpec/ExampleLength)
🔇 Additional comments (3)
spec/mailtrap/email_template_spec.rb (3)
9-316: Excellent comprehensive test coverage for the EmailTemplatesAPI.The test suite provides thorough coverage of:
- All CRUD operations (list, get, create, update, delete)
- Error scenarios (authorization errors, not found errors)
- Partial update functionality
- Both hash and object-based request handling
- VCR integration for consistent HTTP mocking
The test logic is sound and well-structured.
319-383: Good model test coverage for EmailTemplate.The tests properly validate object initialization and serialization functionality, ensuring the EmailTemplate struct works correctly as a data container with all expected attributes.
9-9: Well-implemented VCR integration for test isolation.The VCR usage follows best practices with proper tagging, test isolation through dedicated setup, and comprehensive recording of both success and error scenarios.
Also applies to: 37-37, 77-77, 143-143, 287-287
There was a problem hiding this comment.
@IgorDobryn could you please review the current state of the PR?
lib/mailtrap/email_templates_api.rb
Outdated
| name:, | ||
| subject:, | ||
| category: | ||
| }.compact.merge(extra_params.slice(:body_html, :body_text)) |
There was a problem hiding this comment.
compact hides / ignores updates with name: nil. If user wants to make an update let them execute the original request and get an exception.
UPD tried to run invalid update and got unexpected exception.
templates_api.update template.id, name: nil
# => unexpected token at '' (JSON::ParserError)
lib/mailtrap/client.rb
Outdated
| attr_reader :api_key, :api_host, :api_port, :bulk, :sandbox, :inbox_id | ||
| attr_reader :api_key, :api_host, :api_port, :bulk, :sandbox, :inbox_id, :general_api_host | ||
|
|
||
| # @!macro api_errors |
There was a problem hiding this comment.
those annotations do not work. you probably missed the indentation.
lib/mailtrap/email_templates_api.rb
Outdated
| class EmailTemplatesAPI | ||
| attr_reader :client, :account_id | ||
|
|
||
| # @!macro api_errors |
There was a problem hiding this comment.
the idea was to reuse definitions.
…y response, updated signature of EmailTemplatesAPI#update
…lTemplatesAPI#updated
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
spec/mailtrap/email_templates_api_spec.rb (3)
1-1: Add frozen string literal comment for consistency.The file is missing the frozen string literal comment that's present in other Ruby files in the codebase.
+# frozen_string_literal: true + RSpec.describe Mailtrap::EmailTemplatesAPI, :vcr do
10-17: Consider extracting assertion logic to reduce example length.This example exceeds the recommended 5-line limit. Consider extracting the attribute verification to a shared example or helper method.
it 'maps response data to EmailTemplate objects' do expect(list).to all(be_a(Mailtrap::EmailTemplate)) - expect(list.first).to have_attributes( - name: be_a(String), - subject: be_a(String), - category: be_a(String) - ) + expect(list.first).to have_valid_email_template_attributes endThen define a custom matcher in
spec_helper.rb:RSpec::Matchers.define :have_valid_email_template_attributes do match do |template| template.name.is_a?(String) && template.subject.is_a?(String) && template.category.is_a?(String) end end
91-110: Consider consolidating duplicate test cases.This test case is nearly identical to the main create test. Consider using shared examples or parameterized tests to reduce duplication.
-context 'with hash request' do - let(:request) do - { - name: 'New Template', - subject: 'New Subject', - category: 'New Category', - body_html: '<div>New HTML</div>', - body_text: 'New Text' - } - end - - it 'maps response data to EmailTemplate object' do - expect(create).to be_a(Mailtrap::EmailTemplate) - expect(create).to have_attributes( - name: 'New Template', - subject: 'New Subject', - category: 'New Category' - ) - end -end +shared_examples 'creates email template successfully' do + it 'maps response data to EmailTemplate object' do + expect(create).to be_a(Mailtrap::EmailTemplate) + expect(create).to have_attributes( + name: 'New Template', + subject: 'New Subject', + category: 'New Category' + ) + end +end + +context 'with hash request' do + let(:request) { { name: 'New Template', subject: 'New Subject', category: 'New Category', body_html: '<div>New HTML</div>', body_text: 'New Text' } } + include_examples 'creates email template successfully' +endspec/mailtrap/email_template_spec.rb (1)
21-33: Consider using a more concise assertion approach.While comprehensive, this test could be simplified by leveraging the fact that the struct should preserve all input attributes.
it 'creates a template with all attributes' do - expect(template).to have_attributes( - id: 26_730, - uuid: '018dd5e3-f6d2-7c00-8f9b-e5c3f2d8a132', - name: 'My Template', - subject: 'My Subject', - category: 'My Category', - body_html: '<div>HTML</div>', - body_text: 'Text', - created_at: '2021-01-01T00:00:00Z', - updated_at: '2021-01-01T00:00:00Z' - ) + expect(template.to_h).to eq(attributes) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/mailtrap/client.rb(5 hunks)lib/mailtrap/email_templates_api.rb(1 hunks)spec/mailtrap/email_template_spec.rb(1 hunks)spec/mailtrap/email_templates_api_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap/email_templates_api.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/mailtrap/email_templates_api_spec.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 10-17: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 46-54: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 82-89: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 102-109: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 155-162: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 173-180: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 193-200: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 213-220: Example has too many lines. [6/5]
(RSpec/ExampleLength)
spec/mailtrap/email_template_spec.rb
[convention] 21-33: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 53-65: Example has too many lines. [11/5]
(RSpec/ExampleLength)
🔇 Additional comments (9)
spec/mailtrap/email_templates_api_spec.rb (2)
22-28: Excellent error handling verification.The test properly verifies both the exception type and message content, ensuring robust error handling for authorization failures.
183-201: Excellent partial update testing.The test properly verifies that partial updates work correctly and preserve unchanged fields, which is crucial for PATCH operations.
spec/mailtrap/email_template_spec.rb (1)
36-66: Well-structured hash conversion test.The test properly verifies that the
to_hmethod returns the correct hash representation of the EmailTemplate struct.lib/mailtrap/client.rb (6)
13-13: Good addition of general API host constant.The new constant follows the existing naming pattern and supports the email templates API functionality.
61-94: Excellent implementation of generic HTTP methods.The new HTTP methods (get, post, patch, delete) provide a clean, consistent interface for the email templates API. The YARD documentation is comprehensive and follows established patterns.
98-100: Smart HTTP client caching implementation.The caching mechanism prevents unnecessary HTTP client instantiation while maintaining thread safety through the hash-based approach.
118-123: Well-structured request orchestration.The
perform_requestmethod provides good separation of concerns between request setup, execution, and response handling.
154-159: Robust error handling for empty response bodies.The enhancement properly handles cases where the API returns empty response bodies for bad requests, providing meaningful error messages.
171-171: Verify error message format consistency.Ensure that the error message format for client errors is consistent with other error types. Consider using an array format for consistency with other error handlers.
#!/bin/bash # Description: Check error message formats across the codebase to ensure consistency echo "=== Checking error message patterns in the codebase ===" rg "raise.*Error.*\[" --type ruby -A 1 -B 1 echo -e "\n=== Checking if client error format is used elsewhere ===" rg "client error" --type ruby -A 1 -B 1
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/mailtrap/email_templates_api_spec.rb (1)
124-130: Assertion on generic error message is brittle
expect(error.message).to include('client error')tightly couples the spec to the wording of the HTTP client. Consider asserting on HTTP status or error class only, or match the specific validation detail you care about.- expect(error.message).to include('client error') + expect(error.code).to eq(422) # or whatever the API returnsKeeps the test stable if the library changes phrasing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/mailtrap/email_templates_api_spec.rb(1 hunks)
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/mailtrap/email_templates_api_spec.rb
[convention] 12-19: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 48-56: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 84-91: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 104-111: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 157-164: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 175-182: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 195-202: Example has too many lines. [6/5]
(RSpec/ExampleLength)
[convention] 215-222: Example has too many lines. [6/5]
(RSpec/ExampleLength)
🔇 Additional comments (1)
spec/mailtrap/email_templates_api_spec.rb (1)
6-8: ```shell
#!/bin/bash
set -eLocate the spec file
spec_file=$(fd email_templates_api_spec.rb)
echo "Found spec file at: $spec_file"
echo "----- First 50 lines of the spec -----"
sed -n '1,50p' "$spec_file"echo "----- Searching for VCR usage in the spec -----"
grep -R "VCR" -n "$spec_file" || echo "No VCR references found in this file."</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| let!(:created_template) do | ||
| template.create( | ||
| name: 'Original Template', | ||
| subject: 'Original Subject', | ||
| category: 'Original Category', | ||
| body_html: '<div>Original HTML</div>', | ||
| body_text: 'Original Text' | ||
| ) | ||
| end | ||
| let(:template_id) { created_template.id } | ||
| let(:request) do | ||
| { | ||
| name: 'Updated Template', | ||
| subject: 'Updated Subject', | ||
| category: 'Updated Category', | ||
| body_html: '<div>Updated HTML</div>', | ||
| body_text: 'Updated Text' | ||
| } | ||
| end | ||
|
|
There was a problem hiding this comment.
State-leakage between #update examples introduces order dependency
All #update contexts share the same created_template.
Each example mutates the remote record, so subsequent examples no longer start from the “Original Template” baseline; failures will surface once the spec order changes or when cassettes are re-recorded.
Refactor each context to create its own template or use around to reset state:
-describe '#update' do
- subject(:update) { template.update(template_id, **request) }
-
- let!(:created_template) do
- template.create(
- name: 'Original Template',
- ...
- )
- end
+describe '#update' do
+ subject(:update) { template.update(template_id, **request) }
+
+ let(:created_template) do
+ template.create(
+ name: 'Original Template',
+ ...
+ )
+ end
+ let(:template_id) { created_template.id }
+
+ around do |example|
+ created_template # triggers creation
+ example.run
+ template.delete(template_id) rescue nil
+ endThis isolates each example and keeps cassettes deterministic.
📝 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.
| let!(:created_template) do | |
| template.create( | |
| name: 'Original Template', | |
| subject: 'Original Subject', | |
| category: 'Original Category', | |
| body_html: '<div>Original HTML</div>', | |
| body_text: 'Original Text' | |
| ) | |
| end | |
| let(:template_id) { created_template.id } | |
| let(:request) do | |
| { | |
| name: 'Updated Template', | |
| subject: 'Updated Subject', | |
| category: 'Updated Category', | |
| body_html: '<div>Updated HTML</div>', | |
| body_text: 'Updated Text' | |
| } | |
| end | |
| describe '#update' do | |
| subject(:update) { template.update(template_id, **request) } | |
| let(:created_template) do | |
| template.create( | |
| name: 'Original Template', | |
| subject: 'Original Subject', | |
| category: 'Original Category', | |
| body_html: '<div>Original HTML</div>', | |
| body_text: 'Original Text' | |
| ) | |
| end | |
| let(:template_id) { created_template.id } | |
| around do |example| | |
| created_template # triggers creation before each example | |
| example.run | |
| template.delete(template_id) rescue nil | |
| end | |
| let(:request) do | |
| { | |
| name: 'Updated Template', | |
| subject: 'Updated Subject', | |
| category: 'Updated Category', | |
| body_html: '<div>Updated HTML</div>', | |
| body_text: 'Updated Text' | |
| } | |
| end | |
| # ... your individual `it` examples here ... | |
| end |
🤖 Prompt for AI Agents
In spec/mailtrap/email_templates_api_spec.rb around lines 137 to 156, the shared
use of created_template in multiple #update examples causes state leakage and
order dependency. To fix this, refactor each example or context to create its
own independent template instance or use an around hook to reset the template
state before each example runs. This ensures each test starts with a fresh
"Original Template" baseline, preventing mutations from affecting other tests
and keeping test cassettes deterministic.
| it 'maps response data to EmailTemplate objects' do | ||
| expect(list).to all(be_a(Mailtrap::EmailTemplate)) | ||
| expect(list.first).to have_attributes( | ||
| name: be_a(String), | ||
| subject: be_a(String), | ||
| category: be_a(String) | ||
| ) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
#list example can silently pass on empty collections
expect(list).to all(be_a(...)) and list.first assertions succeed even when list is [], so the test would give a false sense of security if the API ever starts returning no templates.
+expect(list).not_to be_empty
expect(list).to all(be_a(Mailtrap::EmailTemplate))Adding the emptiness expectation guarantees the API actually responded with data.
📝 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.
| it 'maps response data to EmailTemplate objects' do | |
| expect(list).to all(be_a(Mailtrap::EmailTemplate)) | |
| expect(list.first).to have_attributes( | |
| name: be_a(String), | |
| subject: be_a(String), | |
| category: be_a(String) | |
| ) | |
| end | |
| it 'maps response data to EmailTemplate objects' do | |
| expect(list).not_to be_empty | |
| expect(list).to all(be_a(Mailtrap::EmailTemplate)) | |
| expect(list.first).to have_attributes( | |
| name: be_a(String), | |
| subject: be_a(String), | |
| category: be_a(String) | |
| ) | |
| end |
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 12-19: Example has too many lines. [6/5]
(RSpec/ExampleLength)
🤖 Prompt for AI Agents
In spec/mailtrap/email_templates_api_spec.rb around lines 12 to 19, the test
does not verify that the list is not empty, which can cause it to pass silently
if the API returns an empty array. Add an expectation to check that the list is
not empty before asserting the types of its elements to ensure the API actually
returned data.
mailtrap.gemspec
Outdated
| @@ -29,4 +29,6 @@ Gem::Specification.new do |spec| | |||
| end | |||
| end | |||
| spec.require_paths = ['lib'] | |||
|
|
|||
| spec.add_development_dependency 'yard', '~> 0.9' | |||
examples/email_template.rb
Outdated
| client.send(mail) | ||
|
|
||
| templates = Mailtrap::EmailTemplatesAPI.new(ENV.fetch('MAILTRAP_ACCOUNT_ID', 1_111_111), client) |
There was a problem hiding this comment.
- Please move it to a separate example file. This file is about sending using templates, its not about templates API.
- how about this:
class EmailTemplatesAPI
def initialize(account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Client.new)
# ...
end
end| # @raise [Mailtrap::Error] If the API request fails with a client or server error | ||
| # @raise [Mailtrap::AuthorizationError] If the API key is invalid | ||
| # @raise [Mailtrap::RejectionError] If the server refuses to process the request | ||
| # @raise [Mailtrap::RateLimitError] If too many requests are made |
There was a problem hiding this comment.
I'm not sure we need it here, as this will be used for get, create and etc. for EmailTemplates which is likely won't have such error, this seems like send email specific message.
lib/mailtrap/client.rb
Outdated
| # @param path [String] The request path | ||
| # @return [Hash, nil] The JSON response | ||
| # @!macro api_errors | ||
| # @raise [Mailtrap::MailSizeError] If the message is too large |
There was a problem hiding this comment.
I dont think get can produce such error. Its raised when the payload is too large.
| private | ||
|
|
||
| def build_email_template(options) | ||
| EmailTemplate.new(options.slice(*EmailTemplate.members)) |
There was a problem hiding this comment.
As discussed before, OpenStruct won't work for us:
OpenStruct.new(foo: 1).bar # => nilThe reason we wanted to return an email template class instances is strictness.
lib/mailtrap/email_templates_api.rb
Outdated
| "/api/accounts/#{account_id}/email_templates" | ||
| end | ||
|
|
||
| def validate_options(options, supported_options = SUPPORTED_OPTIONS) |
There was a problem hiding this comment.
minor. is there a reason for supported_options parameter?
There was a problem hiding this comment.
it might be useful when new api will be added, it's more like a mock for now
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/email_templates_api.rb (4)
6-6: Clarify the alternative constructor usage.The comment mentions an alternative constructor but doesn't provide clear guidance on how to use it. Consider showing a more complete example:
-templates = Mailtrap::EmailTemplatesAPI.new # or Mailtrap::EmailTemplatesAPI.new(account_id, client) +# Using default initialization (reads from environment variables) +templates = Mailtrap::EmailTemplatesAPI.new + +# Alternative initialization with explicit parameters: +# client = Mailtrap::Client.new(api_key: 'your-api-key') +# templates = Mailtrap::EmailTemplatesAPI.new('your-account-id', client)
18-19: Consider limiting verbose output in examples.The list operation might produce verbose output if there are many templates. Consider showing just the count or first few templates:
-list = templates.list -puts "Templates: #{list}" +list = templates.list +puts "Found #{list.length} templates" +# puts "Templates: #{list}" # Uncomment to see full list
1-32: Consider adding basic error handling to the example.While the example demonstrates the API operations clearly, adding basic error handling would make it more educational and robust:
begin created_email_template = templates.create( name: 'Welcome Email', subject: 'Welcome to Mailtrap!', body_html: '<h1>Hello</h1>', body_text: 'Hello', category: 'welcome' ) puts "Created Template: #{created_email_template.id}" rescue => e puts "Error creating template: #{e.message}" exit 1 endThis would help users understand how to handle potential API errors in their own implementations.
27-28: ```shell
#!/bin/bashLocate the EmailTemplatesAPI implementation
rg -n "class EmailTemplatesAPI" -A 2
Find the update method within the API client
rg -n "def update" -A 4
Search for any handling or validation of body_html
rg -n "body_html" -A 4
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 235c3326134864f8d3826ac71ee617ac74b265b4 and 789f855399b2a2689af8434775ca26313d14f333. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `Gemfile` (1 hunks) * `examples/email_template.rb` (1 hunks) * `examples/email_templates_api.rb` (1 hunks) * `lib/mailtrap/client.rb` (5 hunks) * `lib/mailtrap/email_templates_api.rb` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * Gemfile </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * examples/email_template.rb * lib/mailtrap/email_templates_api.rb * lib/mailtrap/client.rb </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>examples/email_templates_api.rb (2)</summary><blockquote> <details> <summary>lib/mailtrap/email_templates_api.rb (5)</summary> * `create` (46-51) * `list` (22-25) * `get` (31-34) * `update` (64-69) * `delete` (75-77) </details> <details> <summary>lib/mailtrap/client.rb (2)</summary> * `get` (71-73) * `delete` (97-99) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>examples/email_templates_api.rb (3)</summary> `8-14`: **LGTM! Template creation follows correct API pattern.** The template creation correctly uses all the expected fields (name, subject, body_html, body_text, category) as defined in the EmailTemplatesAPI. --- `24-25`: **LGTM! Update operation demonstrates partial updates correctly.** The update operation correctly shows how to modify specific fields while preserving others. --- `30-32`: **LGTM! Clean deletion operation with helpful confirmation.** The delete operation correctly demonstrates the API usage with appropriate user feedback. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
examples/email_templates_api.rb
Outdated
| ENV['MAILTRAP_API_KEY'] ||= 'your-api-key' | ||
| ENV['MAILTRAP_ACCOUNT_ID'] ||= '1111111' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve credential handling in example code.
The current approach using ||= with hardcoded fallback values could be confusing. If environment variables are not set, the example will use placeholder values that won't work, potentially causing confusion for users.
Consider a more explicit approach that clearly indicates when credentials are missing:
-ENV['MAILTRAP_API_KEY'] ||= 'your-api-key'
-ENV['MAILTRAP_ACCOUNT_ID'] ||= '1111111'
+# Set your API credentials as environment variables:
+# export MAILTRAP_API_KEY='your-actual-api-key'
+# export MAILTRAP_ACCOUNT_ID='your-actual-account-id'
+
+unless ENV['MAILTRAP_API_KEY'] && ENV['MAILTRAP_ACCOUNT_ID']
+ puts "Please set MAILTRAP_API_KEY and MAILTRAP_ACCOUNT_ID environment variables"
+ exit 1
+end🤖 Prompt for AI Agents
In examples/email_templates_api.rb around lines 3 to 4, replace the use of ||=
with hardcoded placeholder values for MAILTRAP_API_KEY and MAILTRAP_ACCOUNT_ID.
Instead, explicitly check if these environment variables are set and raise a
clear error or warning if they are missing, to avoid confusion and ensure users
know they must provide valid credentials.
lib/mailtrap/client.rb
Outdated
| when Net::HTTPBadRequest | ||
| raise Mailtrap::Error, json_response(response.body)[:errors] | ||
| body = if response.body.empty? |
There was a problem hiding this comment.
could you please give an example of the api call that produces such response?
There was a problem hiding this comment.
Unfortnunately no, I got it for a once while one of test runs, and fixed it immediately, it might not even appear anymore
Templates API tweaks
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
lib/mailtrap/mail/from_template.rb (1)
8-32: Method length exceeds the new 20-line limit.The
initializemethod is 25 lines long, which violates the newMetrics/MethodLengthlimit of 20 lines set in.rubocop.yml. Removing the disable comment will cause RuboCop failures.Consider refactoring the method to reduce its length, possibly by extracting parameter assignment logic:
def initialize( # rubocop:disable Metrics/ParameterLists from: nil, to: [], reply_to: nil, cc: [], bcc: [], attachments: [], headers: {}, custom_variables: {}, template_uuid: nil, template_variables: {} ) - super( - from:, - to:, - reply_to:, - cc:, - bcc:, - attachments:, - headers:, - custom_variables: - ) - @template_uuid = template_uuid - @template_variables = template_variables + initialize_base_attributes(from, to, reply_to, cc, bcc, attachments, headers, custom_variables) + initialize_template_attributes(template_uuid, template_variables) end +private + +def initialize_base_attributes(from, to, reply_to, cc, bcc, attachments, headers, custom_variables) + super(from:, to:, reply_to:, cc:, bcc:, attachments:, headers:, custom_variables:) +end + +def initialize_template_attributes(template_uuid, template_variables) + @template_uuid = template_uuid + @template_variables = template_variables +endlib/mailtrap/mail/base.rb (1)
11-37: Method length exceeds the new 20-line limit.The
initializemethod is 27 lines long, which violates the newMetrics/MethodLengthlimit of 20 lines set in.rubocop.yml. Removing the disable comment will cause RuboCop failures.Consider refactoring to reduce method length by extracting assignment logic:
def initialize( # rubocop:disable Metrics/ParameterLists from: nil, to: [], reply_to: nil, cc: [], bcc: [], subject: nil, text: nil, html: nil, attachments: [], headers: {}, custom_variables: {}, category: nil ) - @from = from - @to = to - @reply_to = reply_to - @cc = cc - @bcc = bcc - @subject = subject - @text = text - @html = html - self.attachments = attachments - @headers = headers - @custom_variables = custom_variables - @category = category + assign_basic_attributes(from, to, reply_to, cc, bcc, subject, text, html, category) + assign_complex_attributes(attachments, headers, custom_variables) end +private +def assign_basic_attributes(from, to, reply_to, cc, bcc, subject, text, html, category) + @from = from + @to = to + @reply_to = reply_to + @cc = cc + @bcc = bcc + @subject = subject + @text = text + @html = html + @category = category +end +def assign_complex_attributes(attachments, headers, custom_variables) + self.attachments = attachments + @headers = headers + @custom_variables = custom_variables +end
🧹 Nitpick comments (1)
examples/full.rb (1)
34-34: Inconsistent base64 encoding methods.Line 34 still uses the old
Base64.encode64with manual newline removal, while line 21 was updated to useBase64.strict_encode64. Consider using the same method for consistency.-encoded = Base64.encode64(data).gsub("\n", '') +encoded = Base64.strict_encode64(data)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.rubocop.yml(2 hunks)Gemfile(1 hunks)README.md(2 hunks)examples/email_templates_api.rb(1 hunks)examples/full.rb(2 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(3 hunks)lib/mailtrap/email_templates_api.rb(1 hunks)lib/mailtrap/mail.rb(1 hunks)lib/mailtrap/mail/base.rb(2 hunks)lib/mailtrap/mail/from_template.rb(1 hunks)mailtrap.gemspec(1 hunks)spec/mailtrap/client_spec.rb(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- lib/mailtrap/email_templates_api.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- Gemfile
- lib/mailtrap.rb
- examples/email_templates_api.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/mailtrap/client_spec.rb (1)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
🔇 Additional comments (21)
mailtrap.gemspec (1)
22-22: Specify a version constraint for the base64 dependency.Adding dependencies without version constraints can lead to security vulnerabilities and compatibility issues. Consider specifying a minimum version.
- spec.add_dependency 'base64' + spec.add_dependency 'base64', '~> 0.1'Likely an incorrect or invalid review comment.
.rubocop.yml (2)
18-26: Good addition of code quality metrics.These metric thresholds are reasonable and will help maintain code quality across the codebase. The values align with common Ruby best practices.
39-41: Appropriate to disable RSpec/ExampleLength for integration tests.Disabling this rule makes sense given that integration tests and API interaction tests often require longer examples.
examples/full.rb (2)
21-21: Good improvement using strict_encode64.Using
Base64.strict_encode64is better for attachment content as it doesn't add newline characters, eliminating the need for manual stripping.
40-44: Helpful addition of environment variable example.The commented example showing how to use environment variables for API credentials improves the documentation and follows security best practices.
lib/mailtrap/mail/base.rb (1)
39-55: Method length is acceptable.The
as_jsonmethod at 17 lines is within the new 20-line limit. Good to see this method can comply with the stricter metrics.lib/mailtrap/mail.rb (1)
14-14: Good code quality improvement!Removing the
Metrics/MethodLengthdisable while keepingMetrics/AbcSizesuggests the method length is now within acceptable limits, which improves code quality compliance.spec/mailtrap/client_spec.rb (4)
169-169: Good refactoring to useletbinding.Converting
send_mailfrom a subject to aletbinding improves test clarity and reusability.
179-187: Excellent test helper refactoring.The new
stub_postmethod with assertion provides better test organization and ensures requests are actually made during testing.
219-233: Good test coverage for edge cases.These new test cases effectively cover the enhanced error handling scenarios, including empty response bodies and general API 403 responses.
237-237: Improved error message validation.The updated expectation to include the response body in the error message provides better error transparency for debugging.
lib/mailtrap/client.rb (10)
12-12: Well-named constant following existing pattern.The
GENERAL_API_HOSTconstant follows the same naming convention as other API host constants in the class.
15-15: Good addition to attr_reader for consistency.Including
general_api_hostin the attr_reader maintains consistency with other instance variables and addresses past review feedback.
20-31: Comprehensive documentation updates.The YARD documentation clearly explains the new parameter and its relationship to existing options, including important compatibility notes.
53-53: Excellent HTTP client caching implementation.The
@http_clientshash addresses the past review concern about HTTP client instantiation on every request, providing performance optimization.
72-100: Well-designed HTTP method interfaces.The new methods (
get,post,patch,delete) provide a clean, consistent interface for the Email Templates API with proper documentation and parameter handling.
104-106: Efficient HTTP client management.The
http_client_formethod implements the caching strategy suggested in past reviews, optimizing connection reuse while maintaining SSL configuration.
124-129: Clean request orchestration.The
perform_requestmethod provides excellent separation of concerns, handling the request lifecycle in a maintainable way.
131-151: Robust request setup with proper error handling.The
setup_requestmethod handles all HTTP methods consistently and includes proper error handling for unsupported methods.
153-178: Comprehensive response handling improvements.The enhanced
handle_responsemethod addresses multiple past review concerns:
- Supports additional success status codes (201, 204)
- Handles empty response bodies properly
- Includes response body in client error messages
- Maintains backward compatibility
180-183: Flexible error message extraction.The
response_errorsmethod handles both singular and plural error response formats, making it resilient to API response variations.
| # @param [Hash] options The parameters to update | ||
| # @option options [String] :name The template name | ||
| # @option options [String] :subject The email subject | ||
| # @option options [String] :category The template category | ||
| # @option options [String, nil] :body_html The HTML content | ||
| # @option options [String, nil] :body_text The plain text content |
There was a problem hiding this comment.
Could be another @macro
Motivation
Support new functionality (Email Templates API)
https://help.mailtrap.io/article/105-email-templates
Changes
Mailtrap::Clientwith new HTTP methodsmailtrap.ioAPI hostAuthorizationError,HTTPBadRequest,HTTPForbiddenandHTTPClientErrorto work withmailtrap.ioerror responsesMailtrap::EmailTemplatesAPIentity for interactions withEmailTemplateAPIMailtrap::EmailTemplateDTOHow to test
or set yout api key and account id
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores