Add EmailTemplate API functionality #47
Conversation
…d response error handling Add Email Templates API functionality
WalkthroughThis update introduces a new object-oriented API for managing email templates, including full CRUD operations, DTOs, and error handling. The HTTP client is refactored to support generic REST methods and robust response handling. Comprehensive tests and example scripts are added, alongside VCR fixtures for reliable, repeatable test runs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmailTemplatesAPI
participant Client
participant MailtrapAPI
User->>EmailTemplatesAPI: create(request)
EmailTemplatesAPI->>Client: post("/api/accounts/:id/email_templates", request)
Client->>MailtrapAPI: POST /api/accounts/:id/email_templates
MailtrapAPI-->>Client: 201 Created + template data
Client-->>EmailTemplatesAPI: template data
EmailTemplatesAPI-->>User: EmailTemplate object
User->>EmailTemplatesAPI: list()
EmailTemplatesAPI->>Client: get("/api/accounts/:id/email_templates")
Client->>MailtrapAPI: GET /api/accounts/:id/email_templates
MailtrapAPI-->>Client: 200 OK + templates array
Client-->>EmailTemplatesAPI: templates array
EmailTemplatesAPI-->>User: [EmailTemplate, ...]
User->>EmailTemplatesAPI: update(template_id, request)
EmailTemplatesAPI->>Client: patch("/api/accounts/:id/email_templates/:tid", request)
Client->>MailtrapAPI: PATCH /api/accounts/:id/email_templates/:tid
MailtrapAPI-->>Client: 200 OK + updated data
Client-->>EmailTemplatesAPI: updated data
EmailTemplatesAPI-->>User: EmailTemplate object
User->>EmailTemplatesAPI: delete(template_id)
EmailTemplatesAPI->>Client: delete("/api/accounts/:id/email_templates/:tid")
Client->>MailtrapAPI: DELETE /api/accounts/:id/email_templates/:tid
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 as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/mailtrap/email_template_spec.rb (1)
6-7: Good use of environment variables for sensitive test data.The implementation properly uses environment variables with fallback values, addressing the security concerns from previous reviews about hardcoded API keys.
🧹 Nitpick comments (6)
spec/mailtrap/client_spec.rb (1)
192-200: Good test coverage for empty response body handling.The new test properly verifies that the client handles 400 responses with empty bodies gracefully, falling back to a generic 'bad request' message. This ensures robustness when the API returns empty error responses.
However, the static analysis tool flags this as an overly long example.
Consider extracting the error expectations into a shared example or helper method to reduce duplication and line count:
+shared_examples 'raises error with message' do |error_class, message| + expect { send_mail }.to raise_error do |error| + expect(error).to be_a(error_class) + expect(error.message).to eq(message) + expect(error.messages).to eq([message]) + end +end it 'handles 400 with empty response body' do stub_api_send 400, '' do - expect { send_mail }.to raise_error do |error| - expect(error).to be_a(Mailtrap::Error) - expect(error.message).to eq('bad request') - expect(error.messages).to eq(['bad request']) - end + include_examples 'raises error with message', Mailtrap::Error, 'bad request' end endspec/mailtrap/email_template_spec.rb (5)
3-3: Consider restructuring to use nested describe blocks.RuboCop flags the use of multiple top-level describe blocks. Consider nesting the
EmailTemplatetests within the mainEmailTemplatesAPIdescribe block or using a shared context.RSpec.describe Mailtrap::EmailTemplatesAPI do # ... existing tests ... + + describe Mailtrap::EmailTemplate do + # Move the EmailTemplate tests here + end end - -RSpec.describe Mailtrap::EmailTemplate do - # ... move these tests up -end
55-62: Comprehensive attribute validation but example length could be optimized.The test properly validates all expected attributes of the returned EmailTemplate object. However, RuboCop flags this as too long.
Consider using
have_attributeswith a hash matcher to make it more concise: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 + +def expected_template_attributes + { + id: template_id, + name: 'Test Template', + subject: 'Test Subject', + category: 'Test Category' + } +end
102-124: Redundant test case for hash request.The test case "with hash request" (lines 102-124) appears to be testing the same functionality as the main create test, since Ruby method calls with keyword arguments are equivalent to passing a hash.
Consider removing this redundant test case or clarifying what different behavior it's testing:
- 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
178-198: Similar redundancy in update test cases.The "with hash request" context appears redundant since the update method already accepts keyword arguments.
Consider consolidating the test cases or clarifying the distinction between different input formats.
337-349: Detailed struct initialization test but could be more concise.The test thoroughly validates struct initialization but is flagged as too long. Consider using a more concise approach:
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 have_attributes(attributes) end
📜 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 (7)
examples/email_template.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/email_templates_api.rb(1 hunks)mailtrap.gemspec(1 hunks)spec/mailtrap/client_spec.rb(2 hunks)spec/mailtrap/email_template_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- mailtrap.gemspec
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/mailtrap.rb
- examples/email_template.rb
- lib/mailtrap/email_templates_api.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/mailtrap/client_spec.rb
[convention] 192-200: Example has too many lines. [7/5]
(RSpec/ExampleLength)
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 (23)
spec/mailtrap/client_spec.rb (1)
228-228: Updated error message expectation aligns with improved client error handling.The change correctly reflects the enhanced error message format that now includes the response body content.
spec/mailtrap/email_template_spec.rb (1)
200-272: Excellent partial update test coverage.The comprehensive testing of partial updates (single field, multiple fields) ensures the PATCH functionality works correctly and preserves unchanged fields. This is crucial for API correctness.
lib/mailtrap/client.rb (21)
13-13: New constant follows existing naming convention.The
GENERAL_API_HOSTconstant is properly named and consistent with the existing API host constants.
15-15: Comprehensive attr_reader declarations.Good practice to expose all the instance variables through attr_readers for consistency and external access.
28-29: Well-documented YARD parameter documentation.The new
general_api_hostparameter is properly documented with clear description and default value.
36-37: Proper parameter handling in constructor.The new parameter is correctly added with appropriate default value from the constant.
51-52: Instance variable initialization and HTTP client caching setup.Good implementation of HTTP client caching to avoid creating new instances on every request.
58-59: Refactored send method uses new unified request handling.The refactoring to use
perform_requestcreates consistency across all HTTP operations.
61-68: Well-documented generic GET method.The new GET method is properly documented with YARD annotations and follows the established pattern.
70-77: Consistent POST method implementation.Follows the same pattern as GET with proper documentation and parameter handling.
79-86: PATCH method completes the CRUD operations.Good addition to support partial updates through the PATCH HTTP method.
88-94: DELETE method rounds out the HTTP verb support.Completes the full set of CRUD operations needed for the EmailTemplatesAPI.
98-100: Efficient HTTP client caching implementation.The
http_clients_formethod properly implements per-host caching with SSL configuration. The method name follows the past review suggestion to be consistent with the originalhttp_clientnaming.
114-114: Method renamed for clarity.Good rename from the generic name to
send_urlwhich better describes its purpose.
118-123: Centralized request handling improves maintainability.The
perform_requestmethod provides a unified approach for all HTTP operations, reducing code duplication and ensuring consistent behavior.
125-137: Comprehensive HTTP method support with proper request setup.The switch statement covers all necessary HTTP methods and properly creates the appropriate Net::HTTP request objects. Good error handling for unsupported methods.
139-139: Conditional JSON body serialization.Properly handles cases where body might be nil, avoiding unnecessary serialization.
149-149: Enhanced success response handling.Good addition of
Net::HTTPCreatedto handle 201 responses from CREATE operations.
151-152: Proper handling of no-content responses.Returns
nilfor 204 No Content responses, which is semantically correct as discussed in past reviews.
154-159: Improved 400 error handling with fallback.The enhanced handling for empty response bodies provides a better user experience by falling back to a generic "bad request" message when the API doesn't provide specific error details.
161-162: Consistent error extraction pattern.Uses the same pattern as other error handlers to extract error messages from the response body.
164-165: Matching error handling for forbidden responses.Consistent with the authorization error handling pattern.
171-171: Enhanced client error message includes response body.Good improvement to include the actual response body in the error message, providing more context for debugging. The error is properly wrapped in an array as required by the error handling pattern.
|
|
||
| module Mailtrap; end | ||
| module Mailtrap | ||
| # @!macro api_errors |
lib/mailtrap/client.rb
Outdated
| @@ -69,17 +111,32 @@ def select_api_host(bulk:, sandbox:) | |||
| end | |||
| end | |||
|
|
|||
| def request_url | |||
| def send_url | |||
There was a problem hiding this comment.
| def send_url | |
| def send_path |
As it's not really a url
lib/mailtrap/email_templates_api.rb
Outdated
| def update(template_id, **options) | ||
| response = client.patch("#{base_path}/#{template_id}", | ||
| { | ||
| email_template: options.slice(:name, :subject, :category, :body_html, :body_text) |
There was a problem hiding this comment.
Meaning we ignore if .update called with invalid params? - let's either pass all of them or be explicit.
There was a problem hiding this comment.
Like that?
email_template: options
using options.slice was needed to be able code like .update(id, name: 'name') work and do no override the rest of fields. and I think, having it email_template: options might mislead according to method annotations
There was a problem hiding this comment.
Like,
solution 1:
def update(template_id, **options)
response = client.patch("#{base_path}/#{template_id}", email_template: options)
end
solution 2:
def update(template_id, options)
supported_options = [:name, :subject, :category, :body_html, :body_text]
invalid_options = options.keys - supported_options
if invalid_options.any?
raise ArgumentError, "invalid options are given: #{invalid_options}, supported_options: #{supported_options}"
end
response = client.patch("#{base_path}/#{template_id}", email_template: options)
end
There was a problem hiding this comment.
I'd go with Solution 2 which is stricter. You could extract it into a method and reuse it for both create and update.
templates_api.update(1, subjekt: 'Bar') # => ArgumentError
templates_api.create(name: 'Foo', subjekt: 'Bar', category: 'Baz') # => ArgumentError| private | ||
|
|
||
| def build_email_template(options) | ||
| EmailTemplate.new(options.slice(*EmailTemplate.members)) |
There was a problem hiding this comment.
This will require an update if API gets new column. From other side we would like to enforce methods. How about
irb(main):001> response = { a: 1, b: 2, c: 3 }
=> {a: 1, b: 2, c: 3}
irb(main):002> Struct.new(*response.keys).new(*response.values)
=> #<struct a=1, b=2, c=3>
irb(main):003> Struct.new(*response.keys).new(*response.values).a
=> 1
irb(main):004> Struct.new(*response.keys).new(*response.values).b
=> 2
irb(main):005> Struct.new(*response.keys).new(*response.values).c
=> 3
irb(main):006> Struct.new(*response.keys).new(*response.values).d
(irb):6:in '<main>': undefined method 'd' for an instance of #<Class:0x0000000138152b40> (NoMethodError)
cc: @i7an
There was a problem hiding this comment.
Struct.new creates a new anonymous class which can be confusing.
templates
=> [#<struct name=Foo>, #<struct name=Bar>, #<struct name=Baz>]We either have to accept the lack of flexibility or implement a class using method_missing or define_method.
There was a problem hiding this comment.
or we can use OpenStruct.new to wrap response hash
spec/mailtrap/email_template_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
| describe '#delete', :vcr do |
There was a problem hiding this comment.
Do all tests in this file use vcr?
There was a problem hiding this comment.
yes, might have sense to add it like RSpec.describe Mailtrap::EmailTemplatesAPI, :vcr do
spec/mailtrap/email_template_spec.rb
Outdated
| expect(list.first).to have_attributes( | ||
| name: be_a(String), | ||
| subject: be_a(String), | ||
| category: be_a(String) | ||
| ) |
There was a problem hiding this comment.
If previous test ensures (be_a(Mailtrap::EmailTemplate)) do we need to test specific attributes?
There was a problem hiding this comment.
I guess, so, as being instance of Mailtrap::EmailTemplate does not necessarily guarantee that values are not empty, as Mailtrap::EmailTemplate.new({}) will also return a valid instance
spec/mailtrap/email_template_spec.rb
Outdated
| expect(get).to be_a(Mailtrap::EmailTemplate) | ||
| end | ||
|
|
||
| it 'maps response data to EmailTemplate object' do |
There was a problem hiding this comment.
Makes sense to merge with previous folllowing one
There was a problem hiding this comment.
but they are testing different methods and endpoint of api #list(previous one) and #get current one
There was a problem hiding this comment.
sorry for the confusion, following one
spec/mailtrap/email_template_spec.rb
Outdated
| expect(create).to be_a(Mailtrap::EmailTemplate) | ||
| end | ||
|
|
||
| it 'maps response data to EmailTemplate object' do |
There was a problem hiding this comment.
Can merge with previous, but not too critical
spec/mailtrap/email_template_spec.rb
Outdated
| end | ||
|
|
||
| it 'raises a Mailtrap::Error' do | ||
| expect { create }.to raise_error(Mailtrap::Error) |
spec/mailtrap/email_template_spec.rb
Outdated
| @@ -0,0 +1,383 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| RSpec.describe Mailtrap::EmailTemplatesAPI do | |||
There was a problem hiding this comment.
There should be 2 separate files one for EmailTemplatesAPI and another one for EmailTemplate
…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.
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:: EmailTemplateRequestandMailtrap::EmailTemplateDTOsHow to test
or set yout api key and account id
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores