Contacts API #49
Contacts API #49
Conversation
WalkthroughThis change introduces comprehensive CRUD support for Mailtrap contacts, contact lists, and contact fields within the Ruby SDK. It adds new API client classes, DTOs, and supporting modules, refactors the email templates API to use a shared base, and provides thorough RSpec test suites and usage examples for all new APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Client as Mailtrap::Client
participant API as ContactsAPI/ContactListsAPI/ContactFieldsAPI
participant Server as Mailtrap API Server
App->>Client: Initialize with API key
App->>API: Initialize with account_id, client
App->>API: Call CRUD method (e.g., create, get, update, delete)
API->>Client: Make HTTP request (GET/POST/PATCH/DELETE)
Client->>Server: Send HTTP request
Server-->>Client: Return HTTP response (JSON)
Client-->>API: Return parsed response
API-->>App: Return DTO (Contact, ContactList, ContactField)
Possibly related issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)lib/mailtrap/base_api.rb (7)
🪛 ast-grep (0.38.6)lib/mailtrap/base_api.rb[warning] 58-58: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file. (hardcoded-secret-rsa-passphrase-ruby) 🔇 Additional comments (4)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/when_template_does_not_exist/raises_not_found_error.yml (1)
1-327: Slim down the VCR fixture by filtering out dynamic and unnecessary headers
Duplicate of the comment for the_listcassette above.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml (1)
1-164: Slim down the VCR fixture by filtering out dynamic and unnecessary headers
Duplicate of the comment for the_listcassette above.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/returns_an_EmailTemplate_object.yml (1)
1-333: Slim down the VCR fixture by filtering out dynamic and unnecessary headers
Duplicate of the comment for the_listcassette above.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml (1)
1-168: Slim down the VCR fixture by filtering out dynamic and unnecessary headers
Duplicate of the comment for the_listcassette above.
🧹 Nitpick comments (29)
CHANGELOG.md (1)
1-3: Changelog entry omits major user-facing additionsThe 2.3.1 note only mentions EmailTemplates, but this PR also delivers the new Contacts and ContactLists APIs. Omitting them makes the changelog an unreliable source for upgrade information.
-## [2.3.1] - 2025-06-17 -- Add EmailTemplates API functionality +## [2.3.1] - 2025-06-17 +- Add EmailTemplates API functionality +- Add Contacts API (`Mailtrap::Contact`, DTOs) +- Add ContactLists API (`Mailtrap::ContactList`, DTOs)lib/mailtrap.rb (1)
7-9: Consider lazy-loading to keep boot-time lowUnconditionally requiring three new files forces all Contact / ContactList / Template code (and their dependencies) to be loaded even in apps that never use them. Using
autoload(or Zeitwerk if available) defers cost until first use.-module Mailtrap; end +module Mailtrap + autoload :Template, 'mailtrap/template' + autoload :Contact, 'mailtrap/contact' + autoload :ContactList, 'mailtrap/contact_list' +endspec/mailtrap/client_spec.rb (1)
217-219: Error message expectation contains a stray commaThe message being asserted (
'client error:, 🫖') has an extra comma after the colon, which looks accidental and makes the text clunky.- expect { send_mail }.to raise_error(Mailtrap::Error, 'client error:, 🫖') + expect { send_mail }.to raise_error(Mailtrap::Error, 'client error: 🫖')spec/spec_helper.rb (1)
17-19: Mask account_id in query-params as wellThe current substitutions only touch the path and JSON body. If the account_id ever appears in a query string (e.g.,
/accounts/123/emails?account_id=123) it will leak into the cassette.- interaction.request.uri.gsub!(%r{/accounts/\d+/}, '/accounts/1111111/') + interaction.request.uri.gsub!(%r{/accounts/\d+/}, '/accounts/1111111/') + interaction.request.uri.gsub!(/account_id=\d+/, 'account_id=1111111')examples/email_template.rb (2)
1-3: Prefer ENV variables over hard-coding secretsHard-coding an API key in example code makes it too easy for newcomers to accidentally commit real credentials.
Use an environment variable so that the snippet is copy-paste-safe:-client = Mailtrap::Client.new(api_key: 'your-api-key') +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY'))Likewise consider
ENV['MAILTRAP_ACCOUNT_ID'](or similar) for the numeric ID.
21-32: Inconsistent accessor style – pickobj.idover hash-styleobj[:id]
created,updated, etc. areEmailTemplateobjects. Switching betweencreated[:id]and
created.id(orfound[:name] vsfound.name) is confusing and leaks internal implementation
details ([]most likely delegates to@data). Sticking to reader methods keeps the public
surface minimal and discoverable.-puts "Created Template: #{created[:id]}" +puts "Created Template: #{created.id}" … -found = templates.get(created[:id]) +found = templates.get(created.id) … -puts "Updated Template Name: #{updated[:name]}" +puts "Updated Template Name: #{updated.name}"examples/contact.rb (2)
4-5: Same secret-handling remark as above-client = Mailtrap::Client.new(api_key: 'your-api-key') +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY'))
22-24: Largeputsdumps harm readability of example outputDumping whole arrays / objects via
putsproduces noisy, often unreadable JSON. Consider pretty
printing or at least outputting counts / ids to keep the example concise.spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml (1)
21-60: Filter out dynamic response headers
This cassette includes many volatile headers (Date,Cf-Ray,X-Request-Id,ETag,X-Runtime, etc.) that can break tests on reruns. Consider configuring VCR to strip or stub these headers to improve fixture stability.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml (1)
25-63: Remove or filter noisy response headers
The response contains a long list of transient security and tracing headers (e.g.,Cache-Control,Content-Security-Policy,Cf-Ray,X-Request-Id). Trimming these in fixtures or filtering via VCR will make your tests more maintainable.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml (1)
56-63: Strip volatile values from headers
Response headers likeEtag,Strict-Transport-Security, and dynamic CSP strings add noise. Filter or remove them for cleaner fixtures.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml (1)
185-193: Simplify error response headers
The 404 response body is concise, but you still include dynamic headers (Cf-Ray,Cache-Control, etc.). Consider filtering these out to avoid brittle tests.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml (1)
167-175: Filter dynamic values in GET response
TheGETresponse includes transient headers (ETag,Cache-Control,Cf-Ray, etc.). Removing or filtering them will help keep the fixture lean and stable.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml (1)
1-332: Consider filtering dynamic response headers to reduce fixture noise.The cassette correctly captures the
POSTandPATCHinteractions and uses placeholders for sensitive values. However, repeated dynamic headers (e.g.,Date,Etag,Cf-Ray,X-Request-Id) inflate the fixture and may change frequently, leading to brittle tests.
Recommend adding VCR header filtering or usingfilter_sensitive_datainspec_helper.rbto strip or normalize these headers.spec/mailtrap/contact_list_spec.rb (2)
1-1: Add the frozen string literal magic commentThe rest of the spec suite uses the Ruby 2.3+ immutable string convention. Adding the directive keeps style-checks green and avoids needless diff-noise.
+# frozen_string_literal: true RSpec.describe Mailtrap::ContactList do
17-29: Trim long examples or enableaggregate_failuresRuboCop flags these as exceeding the 5-line guideline. Either:
- Split assertions into separate
itblocks, or- Add
aggregate_failuresto keep them concise while silencing the cop.Low-priority, but keeping specs terse improves readability.
spec/mailtrap/contact_spec.rb (3)
215-217: Remove stray debugging output
pp responseleaks noisy output to the test run and CI logs.- pp responseDelete the line or replace it with a purposeful assertion if you need to inspect the object.
17-31: Avoid large duplicated JSON payloads – extract to a helperThe same
expected_responsehash (or very similar) is repeated in multiple contexts.
Extract to a helper/factory to DRY the spec and make future payload changes cheaper:def contact_payload(overrides = {}) { 'data' => { 'id' => contact_id, 'email' => email, 'created_at' => 1_748_163_401_202, 'updated_at' => 1_748_163_401_202, 'list_ids' => [1, 2], 'status' => 'subscribed', 'fields' => { 'first_name' => 'John', 'last_name' => nil } } }.deep_merge(overrides) endThen use
contact_payloadin every stub.Also applies to: 51-66, 129-144
6-7: Hard-coded API key/account ID in specs – prefer ENV placeholdersEven in test code, static secrets trigger scanners and may be copy-pasted elsewhere.
-let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key')) } +let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_TEST_API_KEY', 'dummy'))) }Keeps tooling quiet and avoids accidental leakage.
lib/mailtrap/template.rb (1)
46-48:MAX_LENGTHandMAX_BODY_LENGTHare dead codeThe constants are declared but never referenced.
Either implement input validation (raiseArgumentErrorwhen exceeded) or drop the constants to avoid false “intention” signals.- MAX_LENGTH = 255 - MAX_BODY_LENGTH = 10_000_000spec/mailtrap/template_spec.rb (1)
40-50: Creating real templates insidelet!couples tests and slows suites
let!executes before every example in the group, creating N identical templates (and recording N cassettes).
Preferbefore(:all)/around(:all)or a VCR cassette recorded once, then reuse the data to keep the suite fast and deterministic.before(:all) do @created_template = template.create(...) end let(:template_id) { @created_template.id }Remember to clean up in an
after(:all)block if the endpoint has side-effects.lib/mailtrap/client.rb (3)
34-36: Parameter naming driftThe rest of the client uses
api_host/api_port; introducing a second host field (general_api_host) complicates configuration and increases the chance of mismatching hosts. Consider re-using the existingapi_hostfor all non-send traffic or clearly documenting when each one is used.Also applies to: 49-50
55-57: Build URIs with HTTPS, not HTTPAll requests are forced to
use_ssl = true, but the URI objects are created withURI::HTTP.build. While Ruby will still perform TLS, logging, redirects, and HSTS logic will report the wrong scheme. Switch toURI::HTTPS.build(orURI("https://…")) for accuracy and clarity.Also applies to: 59-66, 67-75, 76-83, 85-91
111-117: Use the port from the URI
Net::HTTP.new(uri.host, @api_port)ignores the port embedded in the URI (whichURI::HTTPS.buildwould supply). Useuri.portto stay consistent with the constructed URI.-http_client = Net::HTTP.new(uri.host, @api_port) +http_client = Net::HTTP.new(uri.host, uri.port)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/returns_an_array_of_EmailTemplate_objects.yml (1)
1-319: Slim down the VCR fixture by filtering out dynamic and unnecessary headersThis cassette includes dozens of dynamic or security-related headers (e.g., Cf-Ray, ETag, Cache-Control, Content-Security-Policy) that bloat its size and make tests brittle. Please configure VCR to strip all but essential headers (for instance only
Content-TypeandX-Mailtrap-Version) and match requests on just method and URI. Consider adding tospec/spec_helper.rb:VCR.configure do |config| config.before_record do |interaction| # Keep only essential response headers interaction.response.headers.select! { |key, _| ['Content-Type', 'X-Mailtrap-Version'] } end config.default_cassette_options = { match_requests_on: [:method, :uri] } endspec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml (2)
1-167: Consolidate and trim verbose headers for the create interaction
Thehttp_interactions[0]entry includes a large set of low-value headers (e.g., Content-Security-Policy, X-Frame-Options, Cf-Ray, etc.) that bloat the fixture and make it brittle. Please filter out or remove non-essential headers and rely on VCR’s validation of core fields (method, URI, status code) to stabilize tests. Also ensure that the account ID (1111111) and the bearer token are replaced with VCR placeholders (<ACCOUNT_ID>,<BEARER_TOKEN>) to avoid leaking sensitive data.
168-331: Apply same header cleanup and placeholder normalization for the update interaction
Repeat the trimming of unnecessary headers in the second interaction (PATCH /accounts/1111111/email_templates/39715). Confirm that dynamic values—especially the account ID—are filtered to<ACCOUNT_ID>and that only stable headers are retained in the cassette.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/when_template_does_not_exist/raises_not_found_error.yml (2)
1-167: Normalize sensitive data and remove noise from the create step
The initial POST interaction carries the same verbose headers and a hard-coded account ID. Please consolidate the fixture by stripping non-critical headers and replacing1111111with<ACCOUNT_ID>, ensuring the bearer token remains a placeholder (<BEARER_TOKEN>).
168-328: Validate 404 scenario and clean up headers
The error case forPATCH /accounts/1111111/email_templates/999999correctly returns a 404 payload. To improve readability and maintainability, trim unnecessary headers, normalize the account ID to<ACCOUNT_ID>, and verify that your client’s test suite expects this exact{"error":"Not Found"}body.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
CHANGELOG.md(1 hunks)examples/contact.rb(1 hunks)examples/email_template.rb(2 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/contact.rb(1 hunks)lib/mailtrap/contact_list.rb(1 hunks)lib/mailtrap/template.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/when_API_returns_an_error/raises_a_Mailtrap_Error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/returns_true.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/returns_an_array_of_EmailTemplate_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/when_template_does_not_exist/raises_not_found_error.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/maps_response_data_to_EmailTemplate_object.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/with_hash_request/returns_an_EmailTemplate_object.yml(1 hunks)spec/mailtrap/client_spec.rb(1 hunks)spec/mailtrap/contact_list_spec.rb(1 hunks)spec/mailtrap/contact_spec.rb(1 hunks)spec/mailtrap/template_spec.rb(1 hunks)spec/spec_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
examples/email_template.rb (3)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/template.rb (5)
create(75-82)list(58-61)get(66-69)update(89-96)delete(101-103)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 (4)
lib/mailtrap/contact_list.rb (2)
get(33-36)delete(60-62)lib/mailtrap/contact.rb (2)
get(82-85)delete(109-111)lib/mailtrap/template.rb (2)
get(66-69)delete(101-103)lib/mailtrap/mail/base.rb (1)
to_json(57-62)
🪛 RuboCop (1.75.5)
spec/mailtrap/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)
spec/mailtrap/contact_spec.rb
[convention] 34-47: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 68-78: Example has too many lines. [8/5]
(RSpec/ExampleLength)
[convention] 80-91: Example has too many lines. [9/5]
(RSpec/ExampleLength)
[convention] 95-104: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 106-116: Example has too many lines. [8/5]
(RSpec/ExampleLength)
[convention] 146-159: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 161-174: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 205-227: Example has too many lines. [20/5]
(RSpec/ExampleLength)
[convention] 229-246: Example has too many lines. [15/5]
(RSpec/ExampleLength)
spec/mailtrap/contact_list_spec.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 17-29: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 38-48: Example has too many lines. [8/5]
(RSpec/ExampleLength)
[convention] 50-59: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 68-81: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 83-95: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 105-118: Example has too many 8000 lines. [11/5]
(RSpec/ExampleLength)
🪛 ast-grep (0.38.1)
spec/mailtrap/contact_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
spec/mailtrap/contact_list_spec.rb
[warning] 1-1: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (15)
spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/when_api_key_is_incorrect/raises_authorization_error.yml (1)
1-165: Cassette looks good – nothing to flagRecorded interaction is properly anonymised and aligned with the new filtering rules.
spec/fixtures/vcr_cassettes/Mailtrap_Template/_list/maps_response_data_to_EmailTemplate_objects.yml (1)
3-19: Verify placeholder and request structure
The request correctly uses a<BEARER_TOKEN>placeholder, and the HTTP method and URI match thelistendpoint. Ensure your VCR setup filters the real token when recording.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/maps_response_data_to_EmailTemplate_object.yml (1)
3-12: Validate create request payload
ThePOSTto/email_templatesincludes the expected JSON body and<BEARER_TOKEN>placeholder. This aligns with yourMailtrap::Template.createimplementation.spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/returns_an_EmailTemplate_object.yml (1)
3-12: Ensure consistency in create-and-return fixture
This cassette mirrors thecreateinteraction and returns the new template object. The structure and data align with your domain DTO.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/when_template_does_not_exist/raises_not_found_error.yml (2)
3-24: Validate setup request for delete scenario
The initialPOSTto set up the fixture is correct and matches yourMailtrap::Template.createcall.
167-182: Review delete request path for non-existent ID
TheDELETEis correctly pointed at/email_templates/999999, simulating a missing resource. This matches the intended error case.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/returns_an_EmailTemplate_object.yml (1)
3-24: Confirm create-and-fetch sequence
The firstPOSTfor creation and subsequentGETfor retrieval are correctly ordered and reflect yourTemplate.findworkflow.spec/fixtures/vcr_cassettes/Mailtrap_Template/_get/maps_response_data_to_EmailTemplate_object.yml (1)
1-331: VCR get-recording looks comprehensive and accurate.The fixture cleanly captures the create (
POST) and retrieval (GET) of an email template, with correct placeholders and normalized account ID. No issues found.spec/fixtures/vcr_cassettes/Mailtrap_Template/_delete/returns_true.yml (1)
1-321: Delete-recording fixture is properly structured.The cassette covers the full lifecycle of creating and deleting a template, using placeholders for tokens and IDs. Everything aligns with expectations.
spec/fixtures/vcr_cassettes/Mailtrap_Template/_create/with_hash_request/returns_an_EmailTemplate_object.yml (1)
1-168: Create-recording fixture is well-formed.This fixture accurately captures the
POSTcreation response and uses consistent placeholders for dynamic values. No concerns.spec/fixtures/vcr_cassettes/Mailtrap_Template/_update/returns_an_EmailTemplate_object.yml (1)
1-333: Update-recording fixture is correct and complete.The two-step interactions (create then patch) are clearly recorded, with placeholders in place. It matches the client’s expected behavior.
spec/mailtrap/contact_spec.rb (1)
11-13:around { VCR.turned_off … }disables all VCR recording in this fileEvery example in the spec relies solely on WebMock stubs, so disabling VCR is fine.
However, if another contributor later adds:vcrmetadata, this block will silently bypass cassette recording and the spec will behave differently from others.Consider scoping VCR off only where needed:
around(:example, :without_vcr) { |ex| VCR.turned_off { ex.run } }…and tag the relevant examples with
:without_vcr.lib/mailtrap/template.rb (1)
75-82: Inconsistent response wrapping vs. contacts API
Contact#createexpects{ data: … }whileTemplate#createexpects the object itself.
If the real Mailtrap API wraps template responses similarly (docs sometimes use"data"), this will raiseNoMethodErrorwhenresponseis aHashwith:data.Double-check the endpoint and, if wrapped, adjust:
- EmailTemplate.new(response) + EmailTemplate.new(response[:data])Aligning DTO handling across resources makes the client predictable.
lib/mailtrap/contact_list.rb (1)
41-45: Body wrapper may be required by the API
Contactuses{ contact: … }, butContactList#createposts raw attributes.
If the Mailtrap endpoint expects{ contact_list: {...} }, the request will get 422.Please confirm and wrap similarly for consistency:
- response = @client.post(base_path, normalised.to_h) + response = @client.post(base_path, { contact_list: normalised.to_h })lib/mailtrap/client.rb (1)
119-133: Risk of missingNet::HTTP::Patchconstant
Net::HTTP::Patchis only available on Ruby ≥ 2.6. If you support older Rubies, this will raiseNameError. Either enforce a minimum Ruby version or fall back toNet::HTTPGenericRequest.new('PATCH', …)when the constant is absent.
examples/contact.rb
Outdated
| contact = contacts.create(email: 'test@example.com', fields: { first_name: 'John Doe' }, list_ids: [list.id]) | ||
| puts "Created Contact: #{contact.id}" | ||
|
|
||
| contact = contacts.get(contact.id) | ||
| puts "Contact: #{contact.email}" | ||
|
|
||
| contact = contacts.update(contact.id, email: 'test2@example.com', fields: { first_name: 'Jane Doe' }, list_ids: [2]) | ||
| puts "Updated Contact: #{contact.data.email}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mis-matched attribute access hides real-world bugs
contact is first treated as having top-level readers (contact.id, contact.email) and then as
a wrapper exposing contact.data.email. Pick one. If the DTO exposes direct readers there is no
need to traverse data.
Also, the update uses a hard-coded list id of 2, ignoring the freshly created list – causing the
call to fail in real usage.
-contact = contacts.update(contact.id, email: 'test2@example.com', fields: { first_name: 'Jane Doe' }, list_ids: [2])
-puts "Updated Contact: #{contact.data.email}"
-
-contacts.delete(contact.data.id)
+contact = contacts.update(contact.id,
+ email: 'test2@example.com',
+ fields: { first_name: 'Jane Doe' },
+ list_ids: [list.id])
+puts "Updated Contact: #{contact.email}"
+
+contacts.delete(contact.id)📝 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.
| contact = contacts.create(email: 'test@example.com', fields: { first_name: 'John Doe' }, list_ids: [list.id]) | |
| puts "Created Contact: #{contact.id}" | |
| contact = contacts.get(contact.id) | |
| puts "Contact: #{contact.email}" | |
| contact = contacts.update(contact.id, email: 'test2@example.com', fields: { first_name: 'Jane Doe' }, list_ids: [2]) | |
| puts "Updated Contact: #{contact.data.email}" | |
| contact = contacts.create( | |
| email: 'test@example.com', | |
| fields: { first_name: 'John Doe' }, | |
| list_ids: [list.id] | |
| ) | |
| puts "Created Contact: #{contact.id}" | |
| contact = contacts.get(contact.id) | |
| puts "Contact: #{contact.email}" | |
| contact = contacts.update(contact.id, | |
| email: 'test2@example.com', | |
| fields: { first_name: 'Jane Doe' }, | |
| list_ids: [list.id]) | |
| puts "Updated Contact: #{contact.email}" | |
| contacts.delete(contact.id) |
🤖 Prompt for AI Agents
In examples/contact.rb around lines 10 to 18, the contact object is
inconsistently accessed with direct attributes (contact.id, contact.email) and
nested attributes (contact.data.email). Choose one consistent access pattern,
preferably direct attribute readers, and update all references accordingly.
Also, replace the hard-coded list ID '2' in the update call with the dynamically
created list's ID variable to ensure the update uses the correct list and does
not fail.
spec/mailtrap/contact_list_spec.rb
Outdated
| describe '#' do | ||
| let(:expected_response) do | ||
| [ | ||
| { 'id' => 1, 'name' => 'List 1' }, | ||
| { 'id' => 2, 'name' => 'List 2' } | ||
| ] | ||
| end |
There was a problem hiding this comment.
describe '#' loses the method context
RSpec outputs “Mailtrap::ContactList #” which conveys nothing.
You probably meant describe '#list' do.
-describe '#' do
+describe '#list' do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe '#' do | |
| let(:expected_response) do | |
| [ | |
| { 'id' => 1, 'name' => 'List 1' }, | |
| { 'id' => 2, 'name' => 'List 2' } | |
| ] | |
| end | |
| describe '#list' do | |
| let(:expected_response) do | |
| [ | |
| { 'id' => 1, 'name' => 'List 1' }, | |
| { 'id' => 2, 'name' => 'List 2' } | |
| ] | |
| end |
🤖 Prompt for AI Agents
In spec/mailtrap/contact_list_spec.rb around lines 9 to 15, the describe block
uses a generic '#' which loses the method context in RSpec output. Change the
describe string from '#' to the actual instance method name being tested, for
example '#list', to clearly indicate which method the tests are for.
| @@ -0,0 +1,132 @@ | |||
| RSpec.describe Mailtrap::ContactList do | |||
| let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key')) } | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coded credentials inside the repository
Even in unit specs, shipping real-looking secrets ('correct-api-key') violates CWE-798 and triggers secret-scanners.
Inject via ENV or let VCR/wire-mocking short-circuit the request instead:
-let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key')) }
+let(:client) do
+ described_class.new(
+ ENV.fetch('MAILTRAP_ACCOUNT_ID', '1111111'),
+ Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY', 'dummy-key'))
+ )
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key')) } | |
| let(:client) do | |
| described_class.new( | |
| ENV.fetch('MAILTRAP_ACCOUNT_ID', '1111111'), | |
| Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY', 'dummy-key')) | |
| ) | |
| end |
🤖 Prompt for AI Agents
In spec/mailtrap/contact_list_spec.rb at line 2, the API key is hard-coded as
'correct-api-key', which risks exposing sensitive information and triggers
secret scanners. Replace the hard-coded string with an environment variable
reference, such as ENV['MAILTRAP_API_KEY'], to inject the key securely.
Alternatively, configure VCR or a mocking library to intercept requests and
avoid using real credentials in tests.
lib/mailtrap/contact.rb
Outdated
| def get(contact_id) | ||
| response = @client.get("#{base_path}/#{contact_id}") | ||
| ContactDTO.new(response[:data]) | ||
| end |
There was a problem hiding this comment.
URL-encoding is missing – tests will fail and real requests with @, +, etc. break
contact_id can be an e-mail. Passing it raw into the path leaves @, +, ?, … unescaped, so:
- Specs that stub
.../contacts/#{CGI.escape(email)}won’t match the real call (/contacts/test@example.com→ mismatch ⇒ test failure). - Real API calls with special characters will be rejected (HTTP 400) because the server expects the component to be URL-encoded.
- response = @client.get("#{base_path}/#{contact_id}")
+ encoded_id = CGI.escape(contact_id.to_s)
+ response = @client.get("#{base_path}/#{encoded_id}")Apply the same CGI.escape (or ERB::Util.url_encode) in #update and #delete to stay consistent.
🤖 Prompt for AI Agents
In lib/mailtrap/contact.rb around lines 82 to 85, the contact_id used in the URL
path is not URL-encoded, causing test failures and API errors with special
characters like '@' or '+'. Fix this by applying CGI.escape (or
ERB::Util.url_encode) to contact_id before including it in the URL string in the
get method. Also, update the update and delete methods similarly to encode
contact_id consistently across all requests.
lib/mailtrap/contact.rb
Outdated
| def update(contact_id, request) | ||
| normalised = request.is_a?(ContactUpdateRequest) ? request : ContactUpdateRequest.new(**request) | ||
| response = @client.patch("#{base_path}/#{contact_id}", { contact: normalised.to_h }) | ||
| ContactUpdateResponse.new(response) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent encoding for update
Follow-up to the issue above – make sure contact_id is encoded before interpolating.
- response = @client.patch("#{base_path}/#{contact_id}", { contact: normalised.to_h })
+ encoded_id = CGI.escape(contact_id.to_s)
+ response = @client.patch("#{base_path}/#{encoded_id}", { contact: normalised.to_h })Without this, stubs in specs (lines 230-234) and real API calls diverge.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def update(contact_id, request) | |
| normalised = request.is_a?(ContactUpdateRequest) ? request : ContactUpdateRequest.new(**request) | |
| response = @client.patch("#{base_path}/#{contact_id}", { contact: normalised.to_h }) | |
| ContactUpdateResponse.new(response) | |
| end | |
| def update(contact_id, request) | |
| normalised = request.is_a?(ContactUpdateRequest) ? request : ContactUpdateRequest.new(**request) | |
| encoded_id = CGI.escape(contact_id.to_s) | |
| response = @client.patch("#{base_path}/#{encoded_id}", { contact: normalised.to_h }) | |
| ContactUpdateResponse.new(response) | |
| end |
🤖 Prompt for AI Agents
In lib/mailtrap/contact.rb around lines 100 to 104, the contact_id parameter is
used directly in the URL string interpolation without encoding, which causes
inconsistencies between test stubs and real API calls. To fix this, encode
contact_id using an appropriate URI encoding method before interpolating it into
the URL string to ensure consistent and safe URL construction.
lib/mailtrap/client.rb
Outdated
| API_HOST = 'mailtrap.io' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default host likely incorrect
Documentation shows the general REST endpoints are served from api.mailtrap.io, not the naked domain. Using mailtrap.io may 301/404 and break every Contacts/Lists call.
-API_HOST = 'mailtrap.io'
+API_HOST = 'api.mailtrap.io'📝 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.
| API_HOST = 'mailtrap.io' | |
| API_HOST = 'api.mailtrap.io' |
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 13 to 14, the API_HOST constant is set to
'mailtrap.io', which is incorrect as the API endpoints are served from
'api.mailtrap.io'. Update the API_HOST value to 'api.mailtrap.io' to ensure the
client correctly targets the API server and avoids potential 301 redirects or
404 errors.
lib/mailtrap/client.rb
Outdated
| body = json_response(response.body) | ||
| raise Mailtrap::AuthorizationError, [body[:errors] || body[:error]].flatten | ||
| when Net::HTTPForbidden | ||
| raise Mailtrap::RejectionError, json_response(response.body)[:errors] | ||
| body = json_response(response.body) | ||
| raise Mailtrap::RejectionError, [body[:errors] || body[:error]].flatten | ||
| when Net::HTTPPayloadTooLarge |
There was a problem hiding this comment.
JSON parse can fail on empty body
Some 401/403 responses come with an empty body. Calling json_response unguarded will raise JSON::ParserError, masking the real HTTP error.
- body = json_response(response.body)
- raise Mailtrap::AuthorizationError, [body[:errors] || body[:error]].flatten
+ body = response.body.to_s.empty? ? {} : json_response(response.body)
+ raise Mailtrap::AuthorizationError, Array(body[:errors] || body[:error])Same applies to the 403 branch.
🤖 Prompt for AI Agents
In lib/mailtrap/client.rb around lines 150 to 155, the calls to json_response on
the response body for 401 and 403 errors can raise JSON::ParserError if the body
is empty, masking the real HTTP error. To fix this, add a guard to check if the
response body is present and non-empty before calling json_response; if empty,
handle the error by raising the appropriate exception with a default or empty
error message instead of parsing JSON.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
lib/mailtrap/contact.rb (3)
87-89: URL encoding missing for contact identifier.The
contact_idparameter can be an email address containing special characters like@and+. Without URL encoding, this will cause test failures (stubs won't match) and API errors.def get(contact_id) - response = @client.get("#{base_path}/#{contact_id}") + encoded_id = CGI.escape(contact_id.to_s) + response = @client.get("#{base_path}/#{encoded_id}") build_entity(response[:data], Contact) endDon't forget to add
require 'cgi'at the top of the file.
112-118: Apply consistent URL encoding in update method.Same URL encoding issue applies to the update method.
def update(contact_id, request) + encoded_id = CGI.escape(contact_id.to_s) response = @client.patch( - "#{base_path}/#{contact_id}", + "#{base_path}/#{encoded_id}", { contact: prepare_request(request, ContactUpdateRequest) } ) build_entity(response, ContactUpdateResponse) end
127-129: Apply consistent URL encoding in delete method.Same URL encoding issue applies to the delete method.
def delete(contact_id) - @client.delete("#{base_path}/#{contact_id}") + encoded_id = CGI.escape(contact_id.to_s) + @client.delete("#{base_path}/#{encoded_id}") end
🧹 Nitpick comments (3)
lib/mailtrap/api.rb (1)
1-1: Add frozen string literal comment for performance.The static analysis correctly identifies the missing frozen string literal comment.
+# frozen_string_literal: true + module Mailtrapspec/mailtrap/contacts_api_spec.rb (1)
212-212: Remove debug statement.The
pp responsestatement appears to be leftover debug code and should be removed.response = client.update(contact_id, update_data) - pp responsespec/mailtrap/contact_lists_api_spec.rb (1)
1-1: Add frozen string literal comment.Missing frozen string literal comment for consistency with other test files.
+# frozen_string_literal: true + RSpec.describe Mailtrap::ContactListsAPI do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/contact.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/api.rb(1 hunks)lib/mailtrap/contact.rb(1 hunks)lib/mailtrap/contact_list.rb(1 hunks)spec/mailtrap/contact_lists_api_spec.rb(1 hunks)spec/mailtrap/contacts_api_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/contact.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
lib/mailtrap/api.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
spec/mailtrap/contacts_api_spec.rb
[convention] 30-43: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 64-74: Example has too many lines. [8/5]
(RSpec/ExampleLength)
[convention] 76-87: Example has too many lines. [9/5]
(RSpec/ExampleLength)
[convention] 91-100: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 102-112: Example has too many lines. [8/5]
(RSpec/ExampleLength)
[convention] 142-155: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 157-170: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 201-223: Example has too many lines. [20/5]
(RSpec/ExampleLength)
[convention] 225-242: Example has too many lines. [15/5]
(RSpec/ExampleLength)
spec/mailtrap/contact_lists_api_spec.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 13-25: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 34-44: Example has too many lines. [8/5]
(RSpec/ExampleLength)
[convention] 46-55: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 64-77: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 79-91: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 101-114: Example has too many lines. [11/5]
(RSpec/ExampleLength)
[convention] 116-128: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 130-142: Example has too many lines. [10/5]
(RSpec/ExampleLength)
[convention] 156-165: Example has too many lines. [7/5]
(RSpec/ExampleLength)
[convention] 167-176: Example has too many lines. [7/5]
(RSpec/ExampleLength)
🪛 ast-grep (0.38.1)
spec/mailtrap/contacts_api_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
spec/mailtrap/contact_lists_api_spec.rb
[warning] 1-1: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (8)
lib/mailtrap/api.rb (2)
5-8: LGTM! Clean request normalization logic.The
prepare_requestmethod provides a consistent way to handle both object and hash inputs by normalizing to the expected class before converting to hash.
10-12: LGTM! Efficient entity building with attribute filtering.The
build_entitymethod efficiently creates response objects by filtering only the relevant attributes usingslice(*response_class.members).spec/mailtrap/contacts_api_spec.rb (2)
64-87: LGTM! Proper handling of special characters in email.The tests correctly use
CGI.escapefor email addresses and include a dedicated test for special characters like+in email addresses.
142-170: LGTM! Comprehensive error handling coverage.The tests cover both successful creation and proper error handling for invalid data with appropriate HTTP status codes.
lib/mailtrap/contact.rb (1)
9-14: LGTM! Well-structured DTOs with proper documentation.The DTO structs are well-documented with clear attribute descriptions and proper
to_himplementations usingcompactto remove nil values.Also applies to: 23-29
lib/mailtrap/contact_list.rb (3)
7-12: LGTM! Clean DTO implementation.The DTO structs are well-designed with proper documentation and efficient
to_himplementations.Also applies to: 18-23
40-43: LGTM! Consistent CRUD operations.The CRUD methods follow a consistent pattern with proper error handling documentation and clean implementation. Since
list_idis an integer, no URL encoding is needed.Also applies to: 52-55, 65-70, 79-81
85-88: LGTM! Efficient list method implementation.The
listmethod efficiently maps the response array to ContactList objects using the sharedbuild_entityhelper.
| # @attr_reader status [String] The contact status (subscribed/unsubscribed) | ||
| # @attr_reader created_at [Integer] The creation timestamp | ||
| # @attr_reader updated_at [Integer] The last update timestamp | ||
| Contact = Struct.new( |
There was a problem hiding this comment.
@IgorDobryn if the only motivation for resource classes it to achieve strictness, then we can do this:
contact_hash = {id: 1}
contact_hash.default_proc = proc { |_, k| raise "unknown key '#{k}'" }Thoughts?
There was a problem hiding this comment.
It can work, but rather unexpected. So, I'd prefer to keep separate class
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/mailtrap/contacts_api.rb (1)
1-72: Good implementation with minor inconsistency in response handling.The
ContactsAPIclass is well-implemented with:
- Proper inheritance from
BaseAPI- Comprehensive CRUD operations
- Good parameter validation using predefined constants
- Excellent documentation with YARD syntax
- Support for both UUID and email identifiers
However, there's an inconsistency in response handling:
def get(contact_id) response = client.get("#{base_path}/#{contact_id}") - build_entity(response[:data], Contact) + build_entity(response, Contact) endThe
getmethod accessesresponse[:data]whilecreateandupdatemethods expect the response structure to match the entity. Consider standardizing the response handling pattern across all methods or document why different approaches are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/contacts_api.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/base_api.rb(1 hunks)lib/mailtrap/contact.rb(1 hunks)lib/mailtrap/contact_list.rb(1 hunks)lib/mailtrap/contact_lists_api.rb(1 hunks)lib/mailtrap/contacts_api.rb(1 hunks)spec/mailtrap/contact_lists_api_spec.rb(1 hunks)spec/mailtrap/contacts_api_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/contacts_api.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/mailtrap.rb
- lib/mailtrap/contact_list.rb
- lib/mailtrap/contact.rb
🧰 Additional context used
🧬 Code Graph Analysis (4)
lib/mailtrap/contacts_api.rb (4)
lib/mailtrap/contact_lists_api.rb (6)
freeze(6-69)get(14-17)base_path(66-68)create(25-30)update(39-46)delete(52-54)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/base_api.rb (2)
build_entity(26-28)validate_options!(19-24)lib/mailtrap/client.rb (2)
post(81-83)patch(90-92)
lib/mailtrap/contact_lists_api.rb (4)
lib/mailtrap/contacts_api.rb (4)
get(16-19)create(29-34)update(47-55)delete(61-63)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/base_api.rb (2)
build_entity(26-28)validate_options!(19-24)lib/mailtrap/client.rb (2)
post(81-83)patch(90-92)
lib/mailtrap/base_api.rb (2)
lib/mailtrap/contact.rb (1)
initialize(34-37)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
spec/mailtrap/contact_lists_api_spec.rb (2)
lib/mailtrap/contact_lists_api.rb (5)
list(59-62)get(14-17)create(25-30)update(39-46)delete(52-54)lib/mailtrap/contacts_api.rb (4)
get(16-19)create(29-34)update(47-55)delete(61-63)
🪛 ast-grep (0.38.1)
spec/mailtrap/contact_lists_api_spec.rb
[warning] 3-3: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
spec/mailtrap/contacts_api_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (4)
lib/mailtrap/base_api.rb (1)
1-31: LGTM! Well-designed base class with proper error handling.The
BaseAPIclass provides a solid foundation for API clients with good practices:
- Proper initialization with default values and validation
- Clear error messages for missing required parameters
- Reusable validation and entity building methods
- Clean separation of concerns
The implementation follows Ruby conventions and provides a good abstraction layer for the specific API classes.
lib/mailtrap/contact_lists_api.rb (1)
1-71: Excellent implementation following consistent patterns.The
ContactListsAPIclass demonstrates:
- Consistent inheritance from
BaseAPI- Complete CRUD operations plus listing functionality
- Proper validation using
SUPPORTED_OPTIONSconstant- Good documentation with YARD syntax
- Clean method implementations
The code follows the same patterns established in
ContactsAPIand maintains consistency across the API client library. Thelistmethod properly transforms the response array into entity objects.spec/mailtrap/contacts_api_spec.rb (1)
1-263: Comprehensive test suite with excellent coverage.The test suite demonstrates:
- Complete coverage of all CRUD operations
- Proper testing of both UUID and email identifiers
- Edge case handling (special characters in emails)
- Comprehensive error scenarios
- Clean test structure with proper mocking
The static analysis warning about hardcoded credentials is a false positive - these are test values used for mocking API responses, not production credentials. This is standard practice in test suites.
The tests properly validate response attributes and error handling, ensuring the API client behaves correctly under various conditions.
spec/mailtrap/contact_lists_api_spec.rb (1)
1-181: Well-structured test suite with comprehensive error handling.The test suite provides:
- Complete coverage of all API methods (
list,get,create,update,delete)- Comprehensive error scenario testing (404, 429, 422, 401)
- Proper distinction between different error types (
Mailtrap::ErrorvsMailtrap::AuthorizationError)- Clean test organization with appropriate mocking
The static analysis warning about hardcoded credentials is a false positive - these are test values for mocking, which is standard practice. The test suite effectively validates both successful operations and error conditions, ensuring robust API client behavior.
lib/mailtrap/contact_lists_api.rb
Outdated
| require_relative 'contact_list' | ||
|
|
||
| module Mailtrap | ||
| class ContactListsAPI < BaseAPI |
There was a problem hiding this comment.
do you plan to add Contact Fields API support?
lib/mailtrap/contacts_api.rb
Outdated
| module Mailtrap | ||
| class ContactsAPI < BaseAPI | ||
| CREATE_SUPPORTED_OPTIONS = %i[email fields list_ids].freeze | ||
| UPDATE_SUPPORTED_OPTIONS = %i[email fields list_ids_included list_ids_excluded unsubscribed].freeze |
There was a problem hiding this comment.
I believe list_ids_included and list_ids_excluded should not be available as options. Instead there should be dedicated methods to add or remove a contact to lists:
add_to_lists(contact_id, contact_list_ids)remove_from_lists(contact_id, contact_list_ids)
lib/mailtrap/base_api.rb
Outdated
| # frozen_string_literal: true | ||
|
|
||
| module Mailtrap | ||
| class BaseAPI |
There was a problem hiding this comment.
This class has a bunch of helper methods. I wonder if it should be a mixin instead.
As an alternative, you could add methods to the base class, making inheritance more meaningful:
def delete(id)
client.delete("#{base_path}/#{id}")
endSee also the abstract yard annotation tag.
There was a problem hiding this comment.
I wouldn't go with mixin, cause main goal was to share #initialize method, and having two entities(base class and mixin) for 3 method seem over-engineered to me.
There was a problem hiding this comment.
also I can advice to have base implementations for all methods here get, create, etc..
and to make that possible I would introduce
def create(options)
validate_options!(options, supported_options)
response = client.post(base_path, options)
build_entity(response, response_entity)
end
private
def response_entity
raise NotImplementedError
end
def supported_options
raise NotImplementedError
endand it will be implemented like
class ContactsApi < BaseApi
...
def supported_options
SUPPORTED_OPTIONS
end
...
endThere was a problem hiding this comment.
Yeah that what I meant. Though keep in mind that ruby is not java, thus you don't need to implement methods with raise NotImplementedError. Use abstract annotation instead.
Through there is one problem. Not all APIs support all CRUD methods. For example, there is no list in the Contacts API. In this case overriding with raise might be relevant. If you choose to go this route please mind respond_to?.
As a side note, you do can put initialize in a mixin.
UPD NotImplementedError is commonly misused. But this is so common that you can say its the standard.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/mailtrap/contacts_api.rb (1)
59-70: Well-implemented list management methodsThe dedicated
add_to_listsandremove_from_listsmethods follow the reviewer's previous suggestion and provide a clean API for managing contact lists.Also applies to: 72-83
🧹 Nitpick comments (2)
lib/mailtrap/contacts_api.rb (1)
49-57: Consider using base_update for consistencyThe
updatemethod manually implements the update logic instead of usingbase_updatelike other methods. This breaks the pattern established by the BaseAPI module.Since the method needs to return
ContactUpdateResponseinstead ofContact, you could override the response handling:def update(contact_id, options) - validate_options!(options, %i[email fields unsubscribed]) - - response = client.patch( - "#{base_path}/#{contact_id}", - { contact: options } - ) - build_entity(response, ContactUpdateResponse) + base_update(contact_id, options, %i[email fields unsubscribed]) end + +# Override to return ContactUpdateResponse for update +def handle_update_response(response) + build_entity(response, ContactUpdateResponse) +endThen modify BaseAPI to call
handle_update_responseif it exists.lib/mailtrap/base_api.rb (1)
65-69: Consider making response handling more flexibleThe current design assumes all responses use the same response_class, but as seen in ContactsAPI, the update method needs to return a different response type.
Consider adding method-specific response handling:
def base_update(id, options, supported_options_override = supported_options) validate_options!(options, supported_options_override) response = client.patch("#{base_path}/#{id}", wrap_request(options)) - handle_response(response) + handle_update_response(response) end +def handle_update_response(response) + handle_response(response) +endThis would allow subclasses to override response handling for specific operations while maintaining the default behavior.
Also applies to: 71-75, 86-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/contacts_api.rb(1 hunks)lib/mailtrap/base_api.rb(1 hunks)lib/mailtrap/contact_lists_api.rb(1 hunks)lib/mailtrap/contacts_api.rb(1 hunks)lib/mailtrap/email_templates_api.rb(3 hunks)spec/mailtrap/contacts_api_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/contacts_api.rb
- lib/mailtrap/contact_lists_api.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/mailtrap/base_api.rb (6)
lib/mailtrap/contact.rb (1)
initialize(34-37)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/contact_lists_api.rb (3)
get(17-19)base_path(59-61)delete(46-48)lib/mailtrap/email_templates_api.rb (4)
get(24-26)base_path(67-69)wrap_request(71-73)delete(61-63)lib/mailtrap/contacts_api.rb (5)
get(16-18)base_path(95-97)handle_response(91-93)wrap_request(87-89)delete(36-38)lib/mailtrap/client.rb (2)
post(81-83)patch(90-92)
🪛 ast-grep (0.38.1)
spec/mailtrap/contacts_api_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
lib/mailtrap/base_api.rb
[warning] 72-72: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: client.patch("#{base_path}/#{id}", wrap_request(options))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 RuboCop (1.75.5)
lib/mailtrap/base_api.rb
[warning] 16-16: Unnecessary disabling of Naming/AccessorMethodName.
(Lint/RedundantCopDisableDirective)
[warning] 24-24: Unnecessary disabling of Naming/AccessorMethodName.
(Lint/RedundantCopDisableDirective)
🔇 Additional comments (2)
spec/mailtrap/contacts_api_spec.rb (1)
1-478: Excellent test coverageThe test suite provides comprehensive coverage for all public methods, including:
- Testing with both UUID and email identifiers
- Special character handling in emails
- Error cases and edge cases
- Empty array handling for list operations
lib/mailtrap/email_templates_api.rb (1)
7-12: Clean refactoring to use BaseAPIThe refactoring successfully simplifies the class by:
- Removing duplicate code for client interactions
- Using declarative configuration for supported options and response class
- Maintaining API compatibility while reducing code complexity
Also applies to: 17-17, 25-25, 39-39, 54-54, 62-62, 71-73
| # @param account_id [Integer] The account ID | ||
| # @param client [Mailtrap::Client] The client instance | ||
| # @raise [ArgumentError] If account_id is nil | ||
| def initialize(account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) | ||
| raise ArgumentError, 'account_id is required' if account_id.nil? |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify account_id parameter type consistency
The comment indicates account_id should be an Integer, but it's being fetched from ENV which returns a String. This could lead to type mismatches in API calls.
🏁 Script executed:
#!/bin/bash
# Description: Check how account_id is used across the codebase
# Check if account_id is used in string interpolation (which would work with both types)
rg -A 2 'account_id' --type ruby
# Check if there are any type conversions for account_id
ast-grep --pattern 'account_id.to_$_'Length of output: 1980
Ensure consistent account_id typing
Currently, account_id is pulled directly from ENV.fetch, which always returns a String, even though the YARD comment says it’s an Integer—and the API client only uses it in URL interpolation ("/api/accounts/#{account_id}/…") so it never blows up at runtime. You should either:
-
Convert
account_idto an Integer up front, e.g.def initialize(account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) raise ArgumentError, 'account_id is required' if account_id.nil? @account_id = Integer(account_id) @client = client end
and keep the
@param account_id [Integer]doc. -
Or relax the doc to reflect that you accept both Strings and Integers, e.g.
# @param account_id [Integer] The account ID -# @param account_id [Integer] The account ID +# @param account_id [String, Integer] The account ID (numeric value)
Pick one approach and update lib/mailtrap/base_api.rb accordingly.
🤖 Prompt for AI Agents
In lib/mailtrap/base_api.rb around lines 29 to 33, the account_id parameter is
documented as an Integer but is assigned from ENV.fetch which returns a String,
causing a type inconsistency. To fix this, convert account_id to an Integer
immediately after fetching it by wrapping it with Integer(account_id) in the
initialize method, and keep the YARD doc param as Integer to maintain
consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/mailtrap/contacts_api.rb (1)
9-10: Address the inconsistent supported options configurationThis issue was previously identified but remains unresolved. The
supported_optionsdeclareslist_idsbut theupdatemethod validates against different options (fields,unsubscribed). Thelist_idsfunctionality is correctly handled by dedicated methodsadd_to_listsandremove_from_lists.Remove
list_idsfrom the supported options since list management is handled by dedicated methods:-supported_options %i[email fields list_ids] +supported_options %i[email fields]
🧹 Nitpick comments (1)
lib/mailtrap/contacts_api.rb (1)
73-78: Consider extracting the base_update overrideThe
base_updatemethod override withsupported_options_overrideparameter is a good approach for handling different validation requirements. However, this pattern might be useful in other API classes as well.Consider moving this pattern to the
BaseAPImodule to make it available for other API classes that need similar functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/contact_fields.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/contact_field.rb(1 hunks)lib/mailtrap/contact_fields_api.rb(1 hunks)lib/mailtrap/contacts_api.rb(1 hunks)spec/mailtrap/contact_fields_api_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/contact_fields.rb
- lib/mailtrap/contact_field.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/mailtrap/contact_fields_api.rb (2)
lib/mailtrap/contacts_api.rb (7)
include(6-95)get(16-18)create(28-30)update(49-51)base_update(73-78)delete(36-38)base_path(92-94)lib/mailtrap/base_api.rb (4)
base_get(60-63)base_create(65-69)base_delete(77-79)base_list(81-84)
🪛 ast-grep (0.38.1)
spec/mailtrap/contact_fields_api_spec.rb
[warning] 3-3: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
lib/mailtrap/contacts_api.rb
[warning] 75-75: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: client.patch("#{base_path}/#{id}", wrap_request(options))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (9)
lib/mailtrap/contacts_api.rb (1)
53-82: Well-implemented list management methodsThe
add_to_listsandremove_from_listsmethods are well-designed and address the previous feedback about having dedicated methods for list operations. The privateupdate_listsmethod correctly validates against the appropriate options for list operations.lib/mailtrap/contact_fields_api.rb (3)
9-11: Clean and consistent API configurationThe supported options and response class configuration is well-structured and consistent with the established pattern in the codebase.
41-43: Appropriate field restrictions for updatesThe update method correctly restricts updatable fields to only
nameandmerge_tag, which makes sense sincedata_typeshould not be changeable after creation. This is a good design decision that prevents potential data integrity issues.
62-64: Correct API endpoint path constructionThe base path construction follows the expected pattern and correctly includes the account ID in the URL structure.
spec/mailtrap/contact_fields_api_spec.rb (5)
1-6: Well-structured test setupThe test setup with client initialization and base URL configuration is clean and follows RSpec best practices.
15-27: Comprehensive list method testingThe test properly validates the response structure, array length, and individual object attributes, ensuring the API client correctly handles the list response.
87-99: Good error handling test coverageThe test suite includes appropriate error scenarios like rate limiting (429) and validation failures (422), ensuring robust error handling in the API client.
159-167: Excellent validation test for restricted fieldsThe test correctly verifies that attempting to update
data_typeraises anArgumentError, which validates the field restriction logic in the update method.
192-201: Complete error scenario coverageThe test suite covers different HTTP error codes (401, 404, 422, 429) and verifies the appropriate exception types are raised, ensuring comprehensive error handling validation.
There was a problem hiding this comment.
Very neat!
Let's figure out what to do with ContactUpdateResponse and merge!
cc @IgorDobryn
examples/contact_fields.rb
Outdated
| @@ -0,0 +1,25 @@ | |||
| require 'mailtrap' | |||
There was a problem hiding this comment.
Why did you decide to create a separate example file for fields but not for lists?
There was a problem hiding this comment.
Contacts and Contacs list would be hard to demonstrate an example without each other, as sometimes we need to have a contact_list
# Create new contact
contact = contacts.create(email: 'test@example.com', fields: { first_name: 'John Doe' }, list_ids: [list.id])while fields are not depends on it, so it can be as separate file.
lib/mailtrap/contact_fields_api.rb
Outdated
|
|
||
| supported_options %i[name data_type merge_tag] | ||
|
|
||
| response_class ContactField |
There was a problem hiding this comment.
how about:
self.response_class = ContactField
That way you can avoid get_ and call response_class.
There was a problem hiding this comment.
Same applies to supported_options.
lib/mailtrap/contact.rb
Outdated
| # @see https://api-docs.mailtrap.io/docs/mailtrap-api-docs/16eab4fff9740-contact-update-response | ||
| # @attr_reader action [String] The performed action (created/updated) | ||
| # @attr_reader data [Contact, Hash] The contact data | ||
| ContactUpdateResponse = Struct.new(:action, :data, keyword_init: true) do |
There was a problem hiding this comment.
In my opinion it creates inconsistency. How about adding previously_new_record?, newly_created? or similar method to Contact model to avoid ContactUpdateResponse? Please share your thoughts.
lib/mailtrap/contacts_api.rb
Outdated
| # @return [ContactUpdateResponse] Updated contact object | ||
| # @!macro api_errors | ||
| # @raise [ArgumentError] If invalid options are provided | ||
| def update(contact_id, options) |
There was a problem hiding this comment.
How about renaming it to upsert to highlight what it really does?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/mailtrap/contacts_api.rb (2)
16-18: URL encoding is still missing for contact_id parameterThe contact_id parameter can be an email address containing special characters like
@,+, etc. Without URL encoding, this will cause test failures and API errors.Apply URL encoding to the contact_id parameter:
def get(contact_id) - base_get(contact_id) + encoded_id = CGI.escape(contact_id.to_s) + base_get(encoded_id) endThe same encoding should be applied to the
deletemethod and any other methods that usecontact_idin URL paths.
73-78: Custom base_update method needs URL encodingThe custom
base_updatemethod directly interpolates theidparameter into the URL path without encoding, which will cause issues with email addresses containing special characters.Apply URL encoding to the id parameter:
def base_update(id, options, supported_options_override = supported_options) validate_options!(options, supported_options_override) - response = client.patch("#{base_path}/#{id}", wrap_request(options)) + encoded_id = CGI.escape(id.to_s) + response = client.patch("#{base_path}/#{encoded_id}", wrap_request(options)) build_entity(response[:data].merge(action: response[:action]), Contact) endlib/mailtrap/base_api.rb (1)
29-36: Address account_id type inconsistencyThe documentation states
account_idshould be an Integer, butENV.fetchreturns a String. This type inconsistency should be resolved.Either convert to Integer in the initializer:
def initialize(account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) raise ArgumentError, 'account_id is required' if account_id.nil? - @account_id = account_id + @account_id = Integer(account_id) @client = client endOr update the documentation to reflect that both types are accepted:
-# @param account_id [Integer] The account ID +# @param account_id [String, Integer] The account ID (numeric value)
🧹 Nitpick comments (4)
spec/mailtrap/contacts_api_spec.rb (4)
155-155: Fix RuboCop formatting: Use 'be' for boolean comparison-expect(response.newly_created?).to eq(true) +expect(response.newly_created?).to be(true)
212-219: Fix indentation and boolean comparisonexpect(response).to have_attributes( - id: contact_id, - fields: include( - last_name: 'Smith' - ), - action: 'updated' - ) - expect(response.newly_created?).to eq(false) + id: contact_id, + fields: include( + last_name: 'Smith' + ), + action: 'updated' + ) + expect(response.newly_created?).to be(false)
234-237: Fix indentation and boolean comparisonexpect(response).to have_attributes( - email: - ) - expect(response.newly_created?).to eq(false) + email: + ) + expect(response.newly_created?).to be(false)
292-296: Fix indentationexpect(response).to have_attributes( - id: contact_id, - list_ids: include(1, 2, 3, 4, 5), - action: 'updated' - ) + id: contact_id, + list_ids: include(1, 2, 3, 4, 5), + action: 'updated' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/contacts_api.rb(1 hunks)lib/mailtrap/base_api.rb(1 hunks)lib/mailtrap/contact.rb(1 hunks)lib/mailtrap/contact_fields_api.rb(1 hunks)lib/mailtrap/contact_lists_api.rb(1 hunks)lib/mailtrap/contacts_api.rb(1 hunks)lib/mailtrap/email_templates_api.rb(3 hunks)spec/mailtrap/contacts_api_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/mailtrap/contact_lists_api.rb
- lib/mailtrap/contact_fields_api.rb
- lib/mailtrap/email_templates_api.rb
🧰 Additional context used
🧬 Code Graph Analysis (5)
examples/contacts_api.rb (4)
lib/mailtrap/contact_lists_api.rb (5)
list(53-55)create(27-29)update(38-40)get(17-19)delete(46-48)lib/mailtrap/contact_fields_api.rb (5)
list(56-58)create(29-31)update(41-43)get(17-19)delete(49-51)lib/mailtrap/contacts_api.rb (6)
create(28-30)get(16-18)upsert(49-51)remove_from_lists(67-69)add_to_lists(58-60)delete(36-38)lib/mailtrap/contact.rb (1)
newly_created?(26-30)
lib/mailtrap/contact.rb (1)
lib/mailtrap/contact_list.rb (1)
to_h(10-12)
lib/mailtrap/contacts_api.rb (5)
lib/mailtrap/contact_lists_api.rb (5)
include(6-62)get(17-19)create(27-29)delete(46-48)base_path(59-61)lib/mailtrap/contact_fields_api.rb (5)
include(6-65)get(17-19)create(29-31)delete(49-51)base_path(62-64)lib/mailtrap/email_templates_api.rb (6)
include(6-74)get(24-26)create(38-40)delete(61-63)base_path(67-69)wrap_request(71-73)lib/mailtrap/base_api.rb (15)
supported_options(12-14)supported_options(16-18)supported_options(41-43)response_class(20-22)response_class(24-26)response_class(45-47)base_get(60-63)base_create(65-69)base_delete(77-79)base_update(71-75)validate_options!(49-54)base_path(94-96)wrap_request(90-92)build_entity(56-58)handle_response(86-88)lib/mailtrap/client.rb (1)
patch(90-92)
spec/mailtrap/contacts_api_spec.rb (2)
lib/mailtrap/contacts_api.rb (7)
get(16-18)create(28-30)upsert(49-51)include(6-95)delete(36-38)add_to_lists(58-60)remove_from_lists(67-69)lib/mailtrap/contact.rb (1)
newly_created?(26-30)
lib/mailtrap/base_api.rb (6)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/contact_lists_api.rb (3)
get(17-19)base_path(59-61)delete(46-48)lib/mailtrap/contact_fields_api.rb (3)
get(17-19)base_path(62-64)delete(49-51)lib/mailtrap/contacts_api.rb (6)
get(16-18)base_path(92-94)handle_response(88-90)wrap_request(84-86)base_update(73-78)delete(36-38)lib/mailtrap/email_templates_api.rb (4)
get(24-26)base_path(67-69)wrap_request(71-73)delete(61-63)lib/mailtrap/client.rb (2)
post(81-83)patch(90-92)
🪛 ast-grep (0.38.1)
lib/mailtrap/contacts_api.rb
[warning] 75-75: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: client.patch("#{base_path}/#{id}", wrap_request(options))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
spec/mailtrap/contacts_api_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
lib/mailtrap/base_api.rb
[warning] 72-72: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: client.patch("#{base_path}/#{id}", wrap_request(options))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 GitHub Actions: Ruby
spec/mailtrap/contacts_api_spec.rb
[error] 155-155: RSpec/BeEq: Prefer be over eq. (Correctable) - expect(response.newly_created?).to eq(true)
[error] 213-213: Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line. (Correctable)
[error] 219-219: RSpec/BeEq: Prefer be over eq. (Correctable) - expect(response.newly_created?).to eq(false)
[error] 235-235: Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line. (Correctable)
[error] 236-236: Layout/ClosingParenthesisIndentation: Indent ) to column 8 (not 6) (Correctable)
[error] 237-237: RSpec/BeEq: Prefer be over eq. (Correctable) - expect(response.newly_created?).to eq(false)
[error] 293-293: Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line. (Correctable)
[error] 296-296: Layout/ClosingParenthesisIndentation: Indent ) to column 8 (not 6) (Correctable)
🔇 Additional comments (5)
lib/mailtrap/contact.rb (1)
14-36: Well-structured Contact DTO with clear semanticsThe Contact struct is well-implemented with comprehensive attributes and useful helper methods. The
newly_created?method provides clear semantics for determining contact creation status, and theto_hmethod follows the established pattern from other DTOs.examples/contacts_api.rb (1)
1-69: Comprehensive example demonstrating all major API operationsExcellent example script that thoroughly demonstrates the Contacts API functionality. The script shows both hardcoded and environment variable approaches for credentials, and covers all major operations including CRUD operations, list management, and field management.
lib/mailtrap/contacts_api.rb (1)
84-92: Well-structured request handling methodsThe
wrap_requestandhandle_responsemethods provide clean abstractions for request/response handling, maintaining consistency with the BaseAPI pattern.spec/mailtrap/contacts_api_spec.rb (1)
64-87: Good test coverage for URL encoding with special charactersThe tests properly demonstrate URL encoding with
CGI.escape, which is essential for handling email addresses with special characters like+and@.lib/mailtrap/base_api.rb (1)
49-96: Well-structured base API abstractionThe BaseAPI module provides excellent abstraction for common CRUD operations with proper option validation, response handling, and extensibility through method overrides. The design promotes consistency across API clients while allowing customization where needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
spec/mailtrap/contacts_api_spec.rb (3)
200-200: Improve test description clarity.The test description "contact by id" is ambiguous. It should be more descriptive about what action is being performed.
- it 'contact by id' do + it 'updates contact by id' do
222-222: Improve test description clarity.Similar to the previous test, the description should be more specific about the action being performed.
- it 'contact by email' do + it 'updates contact by email' do
13-28: Consider extracting shared response fixtures.The test file contains multiple similar
expected_responsestructures that could be refactored into shared fixtures or factory methods to reduce duplication and improve maintainability.Consider creating a helper method or shared context for generating contact response fixtures:
def contact_response_fixture(overrides = {}) { 'data' => { 'id' => contact_id, 'email' => email, 'created_at' => 1_748_163_401_202, 'updated_at' => 1_748_163_401_202, 'list_ids' => [1, 2], 'status' => 'subscribed', 'fields' => { 'first_name' => 'John', 'last_name' => nil } }.deep_merge(overrides.fetch('data', {})) }.deep_merge(overrides.except('data')) endAlso applies to: 47-62, 125-140, 182-198, 260-276, 349-365
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/mailtrap/contacts_api_spec.rb(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.1)
spec/mailtrap/contacts_api_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (5)
spec/mailtrap/contacts_api_spec.rb (5)
6-6: Static analysis warning about hardcoded credentials is acceptable in test files.The static analysis tool flagged the hardcoded API key, but this is appropriate for test files where mock credentials are used for testing purposes.
64-74: Good email parameter handling with CGI.escape.The tests correctly use
CGI.escapewhen passing email addresses as URL parameters, which is essential for handling special characters in email addresses.Also applies to: 222-236, 297-310, 386-401
155-155: Good use of thenewly_created?method for distinguishing actions.The tests appropriately verify whether a contact was newly created or updated using the
newly_created?method, which provides clear semantics for the operation result.Also applies to: 219-219, 235-235
329-344: Good edge case coverage for optional parameters.The tests properly handle cases where
list_idsparameter is omitted, ensuring the default behavior (empty array) is correctly implemented.Also applies to: 420-435
142-156: Comprehensive HTTP status code coverage.The tests use appropriate HTTP status codes for different operations (201 for creation, 200 for updates, 204 for deletion, 404 for not found, 422 for validation errors), which aligns with REST API conventions.
Also applies to: 278-295, 367-384
lib/mailtrap/contact.rb
Outdated
| :status, | ||
| :created_at, | ||
| :updated_at, | ||
| :action, |
There was a problem hiding this comment.
action is metadata. thus it should not be among regular fields:
Struct.new(:id, :email, keyword_init: true) do
def initialize(options)
@action = options[:action]
super(options.except(:action)
end
def newly_created?
@action != 'updated'
end
def to_h
super.compact
end
endThere was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
spec/mailtrap/contacts_api_spec.rb (3)
200-200: Complete the test description.The test description is incomplete and should be more descriptive.
- it 'contact by id' do + it 'updates contact by id' do
221-221: Complete the test description.The test description is incomplete and should be more descriptive.
- it 'contact by email' do + it 'updates contact by email' do
118-140: Consider extracting common response structure.The expected response structure is repeated across multiple tests. Consider extracting it into a shared helper method or let block to reduce duplication.
Example approach:
let(:base_contact_response) do { 'id' => contact_id, 'email' => email, 'created_at' => 1_748_163_401_202, 'updated_at' => 1_748_163_401_202, 'status' => 'subscribed', 'fields' => { 'first_name' => 'John', 'last_name' => nil } } end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/mailtrap/contact.rb(1 hunks)spec/mailtrap/contacts_api_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap/contact.rb
🧰 Additional context used
🪛 ast-grep (0.38.1)
spec/mailtrap/contacts_api_spec.rb
[warning] 5-5: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key'))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (4)
spec/mailtrap/contacts_api_spec.rb (4)
6-6: Static analysis false positive for test credentials.The hardcoded API key and account ID are appropriate for test files and don't represent a security risk in this context.
76-87: Excellent edge case coverage for special characters.Good practice testing email addresses with special characters and proper URL encoding.
310-342: Good coverage of edge cases for empty arrays.The tests properly handle both empty arrays and default parameter behavior, which is important for API robustness.
1-453: Comprehensive test coverage with good practices.This test suite demonstrates excellent coverage of the ContactsAPI functionality including:
- CRUD operations with both UUID and email identifiers
- Error handling for various HTTP status codes
- Edge cases like special characters and empty arrays
- Proper use of WebMock for HTTP stubbing
- Clear assertions using RSpec matchers
The test structure follows Ruby and RSpec best practices effectively.
lib/mailtrap/contact.rb
Outdated
| :status, | ||
| :created_at, | ||
| :updated_at, | ||
| :action, |
There was a problem hiding this comment.
Please remove the field...
c = Contact.new id: 1
# => #<struct Contact id=1, email=nil, fields=nil, list_ids=nil, status=nil, created_at=nil, updated_at=nil, action=nil>
c.action = 'hi'
# => "hi"
c.action
# => "hi"
c.to_h
# => {id: 1, action: "hi"}
examples/contacts_api.rb
Outdated
| require 'mailtrap' | ||
|
|
||
| client = Mailtrap::Client.new(api_key: 'your-api-key') | ||
| contact_list = Mailtrap::ContactListsAPI.new 3229, client |
There was a problem hiding this comment.
rename the variable. use plural form for consistency.
examples/contacts_api.rb
Outdated
| contact_list.delete(list.id) | ||
|
|
||
| # Create new contact field | ||
| field = contact_fields.create(name: 'Updated name', data_type: 'text', merge_tag: 'updated_name') |
There was a problem hiding this comment.
It would be cool to demonstrate field usage after creation. for example create a contact with a custom field.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/contacts_api.rb (1)
4-4: Address the variable naming inconsistency.The variable naming inconsistency noted in previous reviews is still present. For consistency with
contactsandcontact_fields(plural forms), consider renamingcontact_listtocontact_lists.
🧹 Nitpick comments (2)
examples/contacts_api.rb (2)
3-6: Consider making the account ID configurable.The hardcoded account ID
3229is used throughout the initialization. For better flexibility and real-world usage, consider making it configurable via environment variables, similar to the commented alternative approach shown below.-client = Mailtrap::Client.new(api_key: 'your-api-key') -contact_list = Mailtrap::ContactListsAPI.new 3229, client -contacts = Mailtrap::ContactsAPI.new 3229, client -contact_fields = Mailtrap::ContactFieldsAPI.new 3229, client +client = Mailtrap::Client.new(api_key: 'your-api-key') +account_id = 3229 # or fetch from ENV['MAILTRAP_ACCOUNT_ID'] +contact_list = Mailtrap::ContactListsAPI.new account_id, client +contacts = Mailtrap::ContactsAPI.new account_id, client +contact_fields = Mailtrap::ContactFieldsAPI.new account_id, client
55-62: Potential logical flow issue with contact deletion.The contact is deleted on line 62, but the code structure suggests it might be used in subsequent operations. While this is acceptable for a demonstration script, consider adding comments to clarify the intended flow or ensure the deletion happens after all contact operations.
# Remove contact from lists contacts.remove_from_lists(contact.id, [list.id]) # Add contact to lists contacts.add_to_lists(contact.id, [list.id]) -# Delete contact +# Clean up: Delete contact contacts.delete(contact.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/contacts_api.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
examples/contacts_api.rb (4)
lib/mailtrap/contact_lists_api.rb (5)
list(53-55)create(27-29)update(38-40)get(17-19)delete(46-48)lib/mailtrap/contact_fields_api.rb (5)
list(56-58)create(29-31)update(41-43)get(17-19)delete(49-51)lib/mailtrap/contacts_api.rb (6)
create(28-30)get(16-18)upsert(49-51)remove_from_lists(67-69)add_to_lists(58-60)delete(36-38)lib/mailtrap/contact.rb (1)
newly_created?(30-32)
🔇 Additional comments (2)
examples/contacts_api.rb (2)
41-42: Excellent demonstration of custom field usage.The code effectively addresses the previous feedback by demonstrating field usage after creation. The example shows creating a contact with a custom field and demonstrates the
newly_created?method.
16-68: Comprehensive API demonstration with good coverage.The example script provides excellent coverage of all CRUD operations for contacts, contact lists, and contact fields. It demonstrates practical usage patterns including custom field creation, contact list management, and the upsert functionality. The cleanup section ensures resources are properly removed.
examples/contacts_api.rb
Outdated
| contacts.upsert(updated_contact.email, email: 'test3@example.com', fields: { field.merge_tag => 'Jane Doe' }) | ||
| updated_contact.newly_created? # => false |
There was a problem hiding this comment.
Fix variable reassignment for correct demonstration.
The upsert call on line 52 doesn't reassign the result to updated_contact, but line 53 calls newly_created? on the old variable. This creates incorrect expectations in the comment.
-contacts.upsert(updated_contact.email, email: 'test3@example.com', fields: { field.merge_tag => 'Jane Doe' })
-updated_contact.newly_created? # => false
+updated_contact = contacts.upsert(updated_contact.email, email: 'test3@example.com', fields: { field.merge_tag => 'Jane Doe' })
+updated_contact.newly_created? # => false📝 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.
| contacts.upsert(updated_contact.email, email: 'test3@example.com', fields: { field.merge_tag => 'Jane Doe' }) | |
| updated_contact.newly_created? # => false | |
| updated_contact = contacts.upsert( | |
| updated_contact.email, | |
| email: 'test3@example.com', | |
| fields: { field.merge_tag => 'Jane Doe' } | |
| ) | |
| updated_contact.newly_created? # => false |
🤖 Prompt for AI Agents
In examples/contacts_api.rb around lines 52 to 53, the result of the upsert call
is not reassigned to updated_contact, causing the subsequent call to
newly_created? to use the old object and produce incorrect behavior. Fix this by
assigning the result of contacts.upsert back to updated_contact so that the
variable reflects the updated state before calling newly_created?.
lib/mailtrap.rb
Outdated
| require_relative 'mailtrap/action_mailer' if defined? ActionMailer | ||
| require_relative 'mailtrap/mail' | ||
| require_relative 'mailtrap/errors' | ||
| require_relative 'mailtrap/base_api' |
There was a problem hiding this comment.
You dont need to require it here. Please require the base api only where it is needed, in api implementations.
There was a problem hiding this comment.
A few adjustments. Good in general
| # Set your API credentials as environment variables | ||
| # export MAILTRAP_API_KEY='your-api-key' | ||
| # export MAILTRAP_ACCOUNT_ID=your-account-id | ||
| # | ||
| # contact_list = Mailtrap::ContactListsAPI.new | ||
| # contacts = Mailtrap::ContactsAPI.new | ||
| # contact_fields = Mailtrap::ContactFieldsAPI.new |
There was a problem hiding this comment.
- Section with exports should go before initializing apis.
- Keeping initializing apis commented out while you have it above does not make sense
There was a problem hiding this comment.
@IgorDobryn It's done to demonstrate two possible ways of initalization API's, one is explicitly(the one which on top), and other way is using ENV , that was the intent.
examples/contacts_api.rb
Outdated
| # Create new contact list | ||
| list = contact_list.create(name: 'Test List') |
There was a problem hiding this comment.
idea, provide example response
| # Create new contact list | |
| list = contact_list.create(name: 'Test List') | |
| # Create new contact list | |
| list = contact_list.create(name: 'Test List') | |
| # => {...} |
There was a problem hiding this comment.
@IgorDobryn will this be enough, as we returns structs with defined fields in it,
list = contact_lists.create(name: 'Test List')
# => ContactList.newor more explicitly will be better?
list = contact_lists.create(name: 'Test List')
# => ContactList.new(id: 1, name: 'Test List')There was a problem hiding this comment.
I'd prefer more explicit version
lib/mailtrap/base_api.rb
Outdated
| def supported_options=(options) | ||
| @supported_options = options | ||
| end | ||
|
|
||
| def supported_options | ||
| @supported_options | ||
| end | ||
|
|
||
| def response_class=(response_class) | ||
| @response_class = response_class | ||
| end | ||
|
|
||
| def response_class | ||
| @response_class | ||
| end |
There was a problem hiding this comment.
| def supported_options=(options) | |
| @supported_options = options | |
| end | |
| def supported_options | |
| @supported_options | |
| end | |
| def response_class=(response_class) | |
| @response_class = response_class | |
| end | |
| F0FD | |
| def response_class | |
| @response_class | |
| end | |
| attr_accessor :response_class, :supported_options |
a3c2d9d to
bf27376
Compare
Motivation
Support new functionality (Contacts API)
https://help.mailtrap.io/article/147-contacts
Changes
Net::HTTPForbiddenandHTTPClientErrorto work withmailtrap.ioerror responsesMailtrap::ContactsAPIentity for interactions withContactAPIMailtrap::ContactCreateRequest,Mailtrap::ContactUpdateRequest,Mailtrap::ContactUpdateResponseandMailtrap::ContactDTOsMailtrap::ContactListsAPIentity for interactions withContactListAPIMailtrap::ContactandMailtrap::ContactListRequestDTOsHow to test
or set yout api key and account id
Summary by CodeRabbit
New Features
Refactor
Tests