Suppressions Api #52
Suppressions Api #52
Conversation
|
Warning Rate limit exceeded@sarco3t has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces a new Suppressions API to the Mailtrap Ruby client. It adds the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SuppressionsAPI
participant Client
participant MailtrapServer
User->>SuppressionsAPI: list(email: nil)
SuppressionsAPI->>Client: get("/accounts/:account_id/suppressions", query_params)
Client->>MailtrapServer: HTTP GET /accounts/:account_id/suppressions
MailtrapServer-->>Client: JSON array of suppressions
Client-->>SuppressionsAPI: parsed response
SuppressionsAPI-->>User: Array of Suppression objects
User->>SuppressionsAPI: delete(suppression_id)
SuppressionsAPI->>Client: delete("/accounts/:account_id/suppressions/:suppression_id")
Client->>MailtrapServer: HTTP DELETE /accounts/:account_id/suppressions/:suppression_id
MailtrapServer-->>Client: HTTP 204 No Content
Client-->>SuppressionsAPI: nil
SuppressionsAPI-->>User: nil
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
spec/spec_helper.rb (1)
52-55: Consider using!equal?for object identity comparison.The static analysis tool correctly suggests using
!equal?instead of!=when comparing object identity. This is more semantically appropriate and potentially more performant.RSpec::Matchers.define :have_different_object_id_than do |expected| match do |actual| - actual.object_id != expected.object_id + !actual.equal?(expected) endlib/mailtrap/base_api.rb (1)
10-15: Handle missing environment variable gracefully.The current implementation will raise a
KeyErrorifMAILTRAP_ACCOUNT_IDis not set, which may not provide a clear error message to users.-def initialize(account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) +def initialize(account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID', nil), client = Mailtrap::Client.new)This change would make the error message from line 11 more informative than the default
KeyError.spec/mailtrap/suppressions_api_spec.rb (1)
12-54: Consider reducing test data complexity for maintainability.While the comprehensive test data coverage is excellent, the extensive attribute definitions (30+ attributes per suppression) might make the tests harder to maintain. Consider extracting common test data to shared examples or factories if this pattern repeats across other test files.
For better maintainability, consider creating a test factory:
+# In spec/support/factories.rb or similar +def build_suppression_attributes(overrides = {}) + { + 'id' => '123e4567-e89b-12d3-a456-426614174000', + 'type' => 'hard bounce', + 'created_at' => '2024-06-01T12:00:00Z', + 'email' => 'user1@example.com', + # ... other common attributes + }.merge(overrides) +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/suppressions_api.rb(1 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/base_api.rb(1 hunks)lib/mailtrap/client.rb(2 hunks)lib/mailtrap/email_templates_api.rb(4 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)spec/spec_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
lib/mailtrap/client.rb (1)
lib/mailtrap/email_templates_api.rb (1)
get(22-25)
lib/mailtrap/base_api.rb (2)
lib/mailtrap/client.rb (2)
attr_reader(8-190)initialize(31-54)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)
lib/mailtrap/suppressions_api.rb (4)
lib/mailtrap/email_templates_api.rb (4)
list(13-16)get(22-25)base_path(72-74)delete(66-68)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/client.rb (2)
get(73-75)delete(99-101)lib/mailtrap/base_api.rb (1)
build_entity(26-28)
spec/mailtrap/suppression_spec.rb (1)
lib/mailtrap/suppression.rb (1)
to_h(42-44)
🪛 RuboCop (1.75.5)
spec/spec_helper.rb
[warning] 54-54: Use !equal? instead of != when comparing object_id.
(Lint/IdentityComparison)
🔇 Additional comments (21)
lib/mailtrap/client.rb (2)
70-75: Excellent implementation of query parameter support.The addition of optional
query_paramsto thegetmethod is well-designed:
- Maintains backward compatibility with a sensible default
- Clear parameter documentation
- Follows existing method signature patterns
125-128: Clean query parameter handling implementation.The query parameter encoding logic is implemented correctly:
- Only processes parameters when present (
query_params.any?)- Uses standard
URI.encode_www_formfor proper URL encoding- Integrates seamlessly with existing request setup
lib/mailtrap/suppression.rb (2)
3-21: Excellent documentation and API reference.The comprehensive YARD documentation with proper attribute descriptions and the reference to the official API documentation makes this DTO very maintainable and developer-friendly.
22-45: Well-designed DTO implementation.Excellent use of Ruby Struct with several good practices:
keyword_init: truefor better readability and clarity- Comprehensive attribute set that matches the API structure
- Smart
to_hoverride usingsuper.compactto remove nil values, which is perfect for clean JSON serialization- Follows immutable data structure patterns
lib/mailtrap.rb (1)
7-9: Proper module dependency management.The new require statements are correctly placed and follow the logical dependency order (base_api before suppressions_api). This maintains consistency with the existing require pattern in the file.
spec/fixtures/vcr_cassettes/Mailtrap_SuppressionsAPI/vcr_list/maps_response_data_to_Suppression_objects.yml (1)
1-167: Well-structured VCR test fixture.This VCR cassette properly captures the Suppressions API interaction with:
- Correct HTTP method and endpoint structure
- Proper authorization and headers
- Realistic response data that matches the Suppression DTO structure
- Appropriate sensitive data filtering (bearer token masked)
- Comprehensive security headers that reflect production API behavior
The fixture will provide reliable test coverage for the suppressions API functionality.
spec/mailtrap/suppression_spec.rb (1)
1-46: Well-structured test suite with comprehensive coverage.The test specification effectively covers both initialization and hash conversion functionality. The comprehensive attribute set provides thorough testing of the Suppression DTO, and the use of the custom
have_different_object_id_thanmatcher appropriately verifies thatto_hreturns a new hash object rather than the original attributes.spec/fixtures/vcr_cassettes/Mailtrap_SuppressionsAPI/vcr_list/when_api_key_is_incorrect/raises_authorization_error.yml (1)
1-167: Proper VCR recording for authorization error testing.The VCR cassette correctly captures an HTTP 401 Unauthorized response scenario with appropriate token masking and realistic response structure. This will enable reliable testing of error handling in the API client.
lib/mailtrap/suppressions_api.rb (2)
11-17: Clean implementation of list method with proper parameter handling.The method correctly handles optional email filtering by building query parameters conditionally and maps the response to Suppression objects using the inherited
build_entitymethod. The implementation is consistent with other API classes in the codebase.
23-25: Simple and correct delete implementation.The delete method properly constructs the URL path and delegates to the client's delete method. The implementation follows the established pattern from other API classes.
lib/mailtrap/base_api.rb (2)
19-24: Robust options validation with clear error messaging.The validation method properly identifies invalid options and provides clear error messages including both the invalid options and supported options. This will help developers quickly identify and fix API usage issues.
26-28: Elegant entity building using response class members.The
build_entitymethod cleverly uses the response class'smembersto slice only relevant attributes from the options hash. This provides a clean and maintainable way to construct objects while preventing unexpected attributes from being passed through.spec/mailtrap/suppressions_api_spec.rb (5)
1-10: LGTM! Clean test setup with proper configuration.The test setup follows RSpec best practices with proper subject definition, environment variable handling with sensible defaults, and clear variable naming.
56-68: Excellent test coverage for the list method.The test properly verifies both the response structure (array of Suppression objects) and the attribute mapping. The use of
all(be_a(Mailtrap::Suppression))is a clean way to verify the collection type.
70-79: Good error handling test for unauthorized access.The test correctly verifies that HTTP 401 responses raise
Mailtrap::AuthorizationError. This ensures proper error propagation from the API client.
82-114: Comprehensive delete method test coverage.The tests cover all critical scenarios:
- Successful deletion (HTTP 204 → nil response)
- Not found error (HTTP 404 → Mailtrap::Error)
- Unauthorized error (HTTP 401 → Mailtrap::AuthorizationError)
The error handling distinctions are appropriate for different HTTP status codes.
116-145: Excellent integration testing with VCR.The VCR tests provide valuable integration coverage and properly test both success and authorization error scenarios. The dynamic attribute checking with
be_a(String)is appropriate for integration tests where exact values may vary.lib/mailtrap/email_templates_api.rb (4)
6-6: Good refactoring to inherit from BaseAPI.The inheritance change eliminates code duplication and provides a consistent base for API classes. This follows the DRY principle and improves maintainability across the SDK.
15-15: Consistent adoption of the base class build_entity method.The replacement of
build_email_template(template)withbuild_entity(template, EmailTemplate)is consistent across all methods and leverages the generalized approach from BaseAPI. This maintains the same functionality while using shared code.Also applies to: 24-24, 41-41, 59-59
38-38: Proper validation method usage with explicit options.The updated
validate_options!calls correctly pass theSUPPORTED_OPTIONSconstant to the base class method. This maintains the validation logic while using the generalized implementation.Also applies to: 56-56
72-74: Appropriate retention of API-specific logic.The
base_pathmethod remains in the class as it contains API-specific logic. This is the correct approach - inherit common functionality while keeping specialized behavior local.
| list = suppressions.list | ||
|
|
||
| # Delete a suppression | ||
| suppressions.delete(list.first.id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil check before accessing list.first.
The code assumes the suppressions list is not empty, which could cause a NoMethodError if no suppressions exist.
-suppressions.delete(list.first.id)
+suppressions.delete(list.first.id) if list.any?🤖 Prompt for AI Agents
In examples/suppressions_api.rb at line 16, add a nil check before accessing
list.first to prevent a NoMethodError when the suppressions list is empty.
Modify the code to verify that list.first is not nil before calling .id and
passing it to suppressions.delete, ensuring safe handling of empty lists.
| client = Mailtrap::Client.new(api_key: 'your-api-key') | ||
| suppressions = Mailtrap::SuppressionsAPI.new 3229, client |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hardcoded credentials from example.
The example contains hardcoded API credentials which could be accidentally committed or copied by users. Consider using placeholder text or environment variables by default.
-client = Mailtrap::Client.new(api_key: 'your-api-key')
-suppressions = Mailtrap::SuppressionsAPI.new 3229, client
+# Set your API credentials as environment variables
+# export MAILTRAP_API_KEY='your-api-key'
+# export MAILTRAP_ACCOUNT_ID=your-account-id
+suppressions = Mailtrap::SuppressionsAPI.new📝 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.
| client = Mailtrap::Client.new(api_key: 'your-api-key') | |
| suppressions = Mailtrap::SuppressionsAPI.new 3229, client | |
| # Set your API credentials as environment variables | |
| # export MAILTRAP_API_KEY='your-api-key' | |
| # export MAILTRAP_ACCOUNT_ID=your-account-id | |
| suppressions = Mailtrap::SuppressionsAPI.new |
🤖 Prompt for AI Agents
In examples/suppressions_api.rb around lines 3 to 4, the API key is hardcoded
directly in the code, which risks accidental exposure. Replace the hardcoded API
key string with a placeholder text like 'your-api-key' or fetch it from an
environment variable to avoid embedding sensitive credentials in the example.
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
New Features
Refactor
Bug Fixes
Tests