Add Suppressions API #73
Conversation
WalkthroughAdds a Suppressions API: new Suppression DTO, SuppressionsAPI with list/delete (supports email filter), example script, specs and VCR fixtures; refactors Client to accept query params and updates BaseAPI to forward them; loader now requires suppressions API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant SA as SuppressionsAPI
participant CL as Client
participant API as Mailtrap API
Dev->>SA: list(email: optional)
SA->>CL: get(path: "/api/accounts/{account}/suppressions", query_params)
CL->>API: HTTPS GET /api/accounts/{account}/suppressions?email=...
API-->>CL: 200 JSON [suppressions]
CL-->>SA: parsed response
SA-->>Dev: [Suppression objects]
rect rgba(230,255,230,0.25)
note right of API: success path
end
alt Unauthorized
API-->>CL: 401 {"error":"Incorrect API token"}
CL-->>SA: raises AuthorizationError
SA-->>Dev: AuthorizationError
end
Dev->>SA: delete(suppression_id)
SA->>CL: delete(path: "/api/accounts/{account}/suppressions/{id}")
CL->>API: HTTPS DELETE /api/.../suppressions/{id}
API-->>CL: 204 / 404
CL-->>SA: nil / raises Error
SA-->>Dev: nil / Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
i7an
force-pushed
the
suppressions-api
branch
from
8579b71 to
c5b5a18
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
lib/mailtrap/base_api.rb (1)
67-69: Harden list mapping against non-array responsesDefensively coerce to an array to avoid errors if the client ever returns nil or a single object.
- def base_list(query_params = {}) - response = client.get(base_path, query_params) - response.map { |item| handle_response(item) } - end + def base_list(query_params = {}) + response = client.get(base_path, query_params) + Array(response).map { |item| handle_response(item) } + endPlease confirm client.get(base_path, query_params) always returns an Array for list endpoints today.
examples/suppressions_api.rb (1)
1-39: Prefer ENV-based config and print outputs in examplesUse ENV for credentials by default and print results to make the example runnable as-is.
-require 'mailtrap' - -client = Mailtrap::Client.new(api_key: 'your-api-key') -suppressions = Mailtrap::SuppressionsAPI.new 3229, client +require 'mailtrap' + +# Set your API credentials as environment variables: +# export MAILTRAP_API_KEY='your-api-key' +# export MAILTRAP_ACCOUNT_ID='your-account-id' + +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY')) +suppressions = Mailtrap::SuppressionsAPI.new(ENV.fetch('MAILTRAP_ACCOUNT_ID'), client) @@ -# Get all suppressions -suppressions.list +# Get all suppressions +all = suppressions.list +puts "Total suppressions: #{all.size}" +puts all.map(&:to_h) @@ -# Get suppressions for the email -list = suppressions.list(email: '[email protected]') +# Get suppressions for the email +list = suppressions.list(email: '[email protected]') +puts "Suppressions for [email protected]: #{list.size}" @@ -# Delete a suppression -suppressions.delete(list.first.id) +# Delete a suppression (ensure there is at least one) +if (first = list.first) + suppressions.delete(first.id) + puts "Deleted suppression #{first.id}" +endlib/mailtrap/suppressions_api.rb (1)
16-21: Optional: allow empty params with a leaner callTiny nit: you can avoid the intermediate hash.
- def list(email: nil) - query_params = {} - query_params[:email] = email if email - - base_list(query_params) - end + def list(email: nil) + base_list(email ? { email: } : {}) + endspec/mailtrap/suppressions_api_spec.rb (2)
70-78: Use WebMock query matcher to avoid percent-encoding mismatches.Matching the full URL with a raw
@in the query can fail since the client percent-encodes it (%40). Prefer.with(query: ...)for robust matching.Apply this diff:
- stub_request(:get, "#{base_url}/suppressions?email=#{email}") + stub_request(:get, "#{base_url}/suppressions") + .with(query: { email: email }) .to_return( status: 200, body: [expected_attributes].to_json, headers: { 'Content-Type' => 'application/json' } )
98-103: Use a realistic UUID in tests for clarity.Minor readability improvement; it better reflects production IDs and prevents subtle type assumptions.
Apply this diff:
- let(:suppression_id) { 1 } + let(:suppression_id) { '123e4567-e89b-12d3-a456-426614174000' }lib/mailtrap/client.rb (4)
122-130: Avoid Ruby 3.1+ shorthand in gem code unless minimum Ruby >= 3.1.
base:andrequests:shorthand requires Ruby 3.1. Use explicit values for broader compatibility.Please confirm
required_ruby_versionin the gemspec is >= 3.1. If not, apply:perform_request( method: :post, host: api_host, path: batch_request_path, body: { - base:, - requests: + base: base, + requests: requests } )
271-275: Set Content-Type only when a body is present; add Accept header.Avoid sending Content-Type on GET/DELETE and explicitly declare Accept.
Apply this diff:
- request.body = body.to_json if body - request['Authorization'] = "Bearer #{api_key}" - request['Content-Type'] = 'application/json' - request['User-Agent'] = 'mailtrap-ruby (https://github.com/railsware/mailtrap-ruby)' + request.body = body.to_json if body + request['Authorization'] = "Bearer #{api_key}" + request['Accept'] = 'application/json' + request['Content-Type'] = 'application/json' if body + request['User-Agent'] = 'mailtrap-ruby (https://github.com/railsware/mailtrap-ruby)'
306-309: Harden error parsing to handle non-JSON error bodies and “message” fields.Prevents JSON::ParserError on text responses and captures common
messagekeys.Apply this diff:
- def response_errors(body) - parsed_body = json_response(body) - Array(parsed_body[:errors] || parsed_body[:error]) - end + def response_errors(body) + parsed_body = json_response(body) + errors = parsed_body[:errors] || parsed_body[:error] || parsed_body[:message] + errors = errors.is_a?(Array) ? errors : [errors].compact + return errors unless errors.empty? + + [body.to_s] + rescue JSON::ParserError + [body.to_s] + end
168-176: Doc return type for GET should include Array.GET endpoints like suppressions return arrays.
Apply this diff:
- # @return [Hash, nil] The JSON response + # @return [Hash, Array, nil] The parsed JSON response
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/suppressions_api.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/base_api.rb(1 hunks)lib/mailtrap/client.rb(5 hunks)lib/mailtrap/suppression.rb(1 hunks)lib/mailtrap/suppressions_api.rb(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_SuppressionsAPI/vcr_list/maps_response_data_to_Suppression_objects.yml(1 hunks)spec/fixtures/vcr_cassettes/Mailtrap_SuppressionsAPI/vcr_list/when_api_key_is_incorrect/raises_authorization_error.yml(1 hunks)spec/mailtrap/suppression_spec.rb(1 hunks)spec/mailtrap/suppressions_api_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
lib/mailtrap/base_api.rb (1)
lib/mailtrap/client.rb (1)
get(173-180)
lib/mailtrap/suppressions_api.rb (2)
lib/mailtrap/base_api.rb (3)
response_class(31-33)base_list(67-70)base_path(80-82)lib/mailtrap/client.rb (1)
delete(214-220)
lib/mailtrap/client.rb (2)
lib/mailtrap/suppressions_api.rb (1)
delete(27-29)lib/mailtrap/base_api.rb (1)
handle_response(72-74)
examples/suppressions_api.rb (2)
lib/mailtrap/suppressions_api.rb (2)
list(16-21)delete(27-29)lib/mailtrap/client.rb (1)
delete(214-220)
spec/mailtrap/suppressions_api_spec.rb (2)
lib/mailtrap/suppressions_api.rb (3)
list(16-21)delete(27-29)include(7-36)lib/mailtrap/client.rb (1)
delete(214-220)
spec/mailtrap/suppression_spec.rb (1)
lib/mailtrap/suppression.rb (1)
to_h(42-44)
🔇 Additional comments (10)
lib/mailtrap.rb (1)
12-12: Require wiring looks goodRequiring suppressions_api here cleanly exposes the new API surface.
spec/mailtrap/suppression_spec.rb (1)
1-43: DTO spec coverage looks solidInitialization and to_h behavior are covered with realistic attributes.
spec/fixtures/vcr_cassettes/Mailtrap_SuppressionsAPI/vcr_list/maps_response_data_to_Suppression_objects.yml (1)
1-167: Fixture aligns with DTO mappingResponse shape matches Suppression fields; useful for deterministic tests.
lib/mailtrap/suppression.rb (1)
22-45: DTO implementation is clean; compact to_h is usefulStruct with keyword_init and compacted to_h fits the use case.
spec/fixtures/vcr_cassettes/Mailtrap_SuppressionsAPI/vcr_list/when_api_key_is_incorrect/raises_authorization_error.yml (1)
1-167: Global VCR token filtering verified spec_helper’s filter_sensitive_data is configured and all cassettes use the<BEARER_TOKEN>placeholder.spec/mailtrap/suppressions_api_spec.rb (3)
100-106: LGTM: delete happy path is asserted correctly.Asserting 204 → nil aligns with Client#handle_response.
108-117: LGTM: 404 maps to Mailtrap::Error.Covers the generic 4xx branch.
149-159: LGTM: VCR auth failure scenario is well covered.Validates both the error class and message contents.
lib/mailtrap/client.rb (2)
246-255: Nice: centralized URI building + query params support.Clean separation of host/path and proper encoding via
URI.encode_www_form.
173-179: Allclient.getcall sites conform to the new(path, query_params = {})signature. Single-argument calls use the default empty hash and two-argument calls pass parameters explicitly.
| def delete(suppression_id) | ||
| client.delete("#{base_path}/#{suppression_id}") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Return type mismatch in docs vs implementation; validate input
YARD says “return nil” but the method returns whatever client.delete returns. Either return nil explicitly or update the doc. Also guard against blank IDs.
- # @return nil
+ # @return [nil] Always nil on success
# @!macro api_errors
def delete(suppression_id)
- client.delete("#{base_path}/#{suppression_id}")
+ raise ArgumentError, 'suppression_id is required' if suppression_id.to_s.empty?
+ client.delete("#{base_path}/#{suppression_id}")
+ nil
endIf you prefer surfacing the HTTP response (e.g., status/boolean), update the YARD return type instead and keep the current behavior.
🏁 Script executed:
#!/bin/bash
echo "SuppressionsApi delete method docs and implementation:"
rg -nP -C2 "(#.*@return|def delete)" lib/mailtrap/suppressions_api.rb
echo -e "\nOther API delete methods:"
rg -nP -C3 "def delete" -g "lib/mailtrap/*_api.rb"
echo -e "\nClient.delete method:"
rg -nP -C2 "def delete" -g "lib/mailtrap/*client*.rb"Length of output: 2985
🏁 Script executed:
#!/bin/bash
# Locate base_delete implementation to verify its behavior
rg -nP "def base_delete" -g "lib/mailtrap/**/*.rb" -C5Length of output: 649
🏁 Script executed:
#!/bin/bash
# Show class definition of SuppressionsApi
rg -nC3 "class SuppressionsApi" -g "lib/mailtrap/suppressions_api.rb"Length of output: 0
Update YARD @return to match actual return value
- Change
# @return nilto# @return [Hash, nil]sinceclient.deletereturns parsed JSON ornil - (Optional) Guard against blank
suppression_idwithraise ArgumentError, 'suppression_id is required' if suppression_id.to_s.empty?
🤖 Prompt for AI Agents
In lib/mailtrap/suppressions_api.rb around lines 27 to 29, the YARD return tag
is incorrect and should reflect that client.delete returns parsed JSON or nil;
update the method documentation from `# @return nil` to `# @return [Hash, nil]`
(or a more specific type if known), and optionally add a guard at the top of the
method to raise ArgumentError when suppression_id is blank (e.g. raise
ArgumentError, 'suppression_id is required' if suppression_id.to_s.empty?)
before calling client.delete.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/email_templates_api.rb (1)
3-5: Use ENV for credentials and account_id in examplesAvoid hard-coded IDs/keys; read from MAILTRAP_ACCOUNT_ID/MAILTRAP_API_KEY for copy‑pasteability.
Apply:
-account_id = 3229 -client = Mailtrap::Client.new(api_key: 'your-api-key') -templates = Mailtrap::EmailTemplatesAPI.new(account_id, client) +account_id = Integer(ENV.fetch('MAILTRAP_ACCOUNT_ID')) +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY')) +templates = Mailtrap::EmailTemplatesAPI.new(account_id, client)Also update the commented initializer below to include required args to prevent confusion.
examples/suppressions_api.rb (2)
3-5: Read account_id and API key from ENVMake the example runnable without edits and avoid hard-coded secrets/IDs.
-account_id = 3229 -client = Mailtrap::Client.new(api_key: 'your-api-key') -suppressions = Mailtrap::SuppressionsAPI.new(account_id, client) +account_id = Integer(ENV.fetch('MAILTRAP_ACCOUNT_ID')) +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY')) +suppressions = Mailtrap::SuppressionsAPI.new(account_id, client)
7-12: Fix commented initializer to reflect required constructor argumentsThe no-arg example is misleading given the current signature.
-# suppressions = Mailtrap::SuppressionsAPI.new +# suppressions = Mailtrap::SuppressionsAPI.new(account_id, client)examples/contacts_api.rb (1)
3-8: Prefer ENV-driven initialization for examplesUse MAILTRAP_ACCOUNT_ID and MAILTRAP_API_KEY to make examples safer and ready to run.
-account_id = 3229 -client = Mailtrap::Client.new(api_key: 'your-api-key') +account_id = Integer(ENV.fetch('MAILTRAP_ACCOUNT_ID')) +client = Mailtrap::Client.new(api_key: ENV.fetch('MAILTRAP_API_KEY')) contact_lists = Mailtrap::ContactListsAPI.new(account_id, client) contacts = Mailtrap::ContactsAPI.new(account_id, client) contact_fields = Mailtrap::ContactFieldsAPI.new(account_id, client) contact_imports = Mailtrap::ContactImportsAPI.new(account_id, client)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/contacts_api.rb(1 hunks)examples/email_templates_api.rb(1 hunks)examples/suppressions_api.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
examples/email_templates_api.rb (1)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
examples/suppressions_api.rb (3)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/suppressions_api.rb (2)
list(16-21)delete(27-29)lib/mailtrap/client.rb (1)
delete(214-220)
examples/contacts_api.rb (1)
lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
| list = suppressions.list(email: '[email protected]') | ||
|
|
||
| # Delete a suppression | ||
| suppressions.delete(list.first.id) |
There was a problem hiding this comment.
Guard against empty list before delete
list.first can be nil and raise NoMethodError on .id.
-# Delete a suppression
-suppressions.delete(list.first.id)
+# Delete a suppression (if any)
+if list.any?
+ suppressions.delete(list.first.id)
+else
+ warn 'No suppressions found for the given email'
+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.
| list = suppressions.list(email: '[email protected]') | |
| # Delete a suppression | |
| suppressions.delete(list.first.id) | |
| list = suppressions.list(email: '[email protected]') | |
| # Delete a suppression (if any) | |
| if list.any? | |
| suppressions.delete(list.first.id) | |
| else | |
| warn 'No suppressions found for the given email' | |
| end |
🤖 Prompt for AI Agents
In examples/suppressions_api.rb around lines 37 to 40, calling list.first.id can
raise NoMethodError when the list is empty; update the code to check that the
returned list is non-empty (e.g., list.any? or !list.empty?) before attempting
to access first.id, and only call suppressions.delete when an item exists; if
the list is empty, either skip the delete or handle the case with a clear
message or error handling.
Re-created #52
Motivation
Support new functionality (Suppressions API)
https://help.mailtrap.io/article/95-suppressions-list
Changes
Mailtrap::SuppressionsAPIentity for interactions withSuppressionsAPIMailtrap::Client#getMailtrap::SuppressionDTOHow to test
or set yout api key and account id
Summary by CodeRabbit