added ContactImportAPI #64
Conversation
WalkthroughThe changes introduce bulk contact import functionality to the Mailtrap Ruby SDK. This includes a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContactsAPI
participant ContactImportsAPI
participant MailtrapServer
User->>ContactsAPI: Create contact list
User->>ContactImportsAPI: Create contact import (contacts array or builder)
ContactImportsAPI->>MailtrapServer: POST /contact-imports
MailtrapServer-->>ContactImportsAPI: Response (ContactImport)
ContactImportsAPI-->>User: Return ContactImport object
User->>ContactImportsAPI: Get contact import (by ID)
ContactImportsAPI->>MailtrapServer: GET /contact-imports/:id
MailtrapServer-->>ContactImportsAPI: Response (ContactImport)
ContactImportsAPI-->>User: Return ContactImport object
User->>ContactsAPI: Delete contact list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 🪧 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 (
|
sarco3t
force-pushed
the
contact-imports-api
branch
from
8682373 to
0a21de3
Compare
| first_name: 'Jane', | ||
| }, | ||
| list_ids_included: [list.id], | ||
| list_ids_excluded: [] |
There was a problem hiding this comment.
I think this is not elegant and exposes the confusing underlying API. Instead I suggest this:
contacts = Mailtrap::ContactsAPI.new
contacts.import do |req|
req.upsert(...)
req.upsert(...)
req.add_to_lists(...)
req.add_to_lists(...)
req.remove_from_lists(...)
req.remove_from_lists(...)
end
req = ImportRequest.new
req.upsert(...)
req.upsert(...)
req.add_to_lists(...)
req.add_to_lists(...)
req.remove_from_lists(...)
req.remove_from_lists(...)
contacts.import req
constats.import([{...}, {...}, {...}])@IgorDobryn thoughts?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/mailtrap/contacts_import_request.rb (2)
15-24: Consider merging fields instead of replacing them.The
upsertmethod replaces all fields when called multiple times for the same email, which may not be the expected behavior. Consider merging fields instead.- @data[email][:fields] = fields + @data[email][:fields].merge!(fields)
30-39: Potential duplicate list IDs not handled.The
add_to_listsandremove_from_listsmethods useconcatwhich can create duplicate list IDs. Consider using array union to prevent duplicates.- @data[email][:list_ids_included].concat(list_ids) + @data[email][:list_ids_included] |= list_ids - @data[email][:list_ids_excluded].concat(list_ids) + @data[email][:list_ids_excluded] |= list_idsAlso applies to: 45-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/contacts_api.rb(3 hunks)lib/mailtrap/contact_imports_api.rb(1 hunks)lib/mailtrap/contacts_import_request.rb(1 hunks)spec/mailtrap/contact_imports_api_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mailtrap/contact_imports_api.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/mailtrap/contact_imports_api_spec.rb (4)
lib/mailtrap/mail/base.rb (1)
to_json(57-62)lib/mailtrap/action_mailer/delivery_method.rb (1)
client(22-24)lib/mailtrap/contact_imports_api.rb (2)
get(18-20)create(33-40)lib/mailtrap/contacts_import_request.rb (3)
upsert(15-24)add_to_lists(30-39)remove_from_lists(45-54)
🪛 ast-grep (0.38.6)
spec/mailtrap/contact_imports_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)
🪛 GitHub Actions: Ruby
spec/mailtrap/contact_imports_api_spec.rb
[error] 146-146: RSpec/MultipleMemoizedHelpers: Example group has too many memoized helpers [8/5]
🔇 Additional comments (6)
lib/mailtrap/contacts_import_request.rb (1)
58-62: LGTM! Clean JSON serialization implementation.The
as_jsonmethod with aliasto_aprovides a clean interface for converting the builder data to the expected array format for the API.examples/contacts_api.rb (2)
144-175: LGTM! Clear demonstration of direct contact import creation.The example clearly shows how to create a contact import using a hash structure and retrieve its status. The expected return values are well documented.
176-203: Excellent builder pattern demonstration.The chained method calls effectively demonstrate the fluent API design of the
ContactsImportRequestbuilder. The example shows both the power and readability of the builder pattern.spec/mailtrap/contact_imports_api_spec.rb (3)
75-92: LGTM! Comprehensive test for contact import creation.The test properly validates the API request body, response handling, and returned object attributes. The stubbing is correctly configured.
146-171: Excellent test coverage for builder pattern.The test effectively validates that the
ContactsImportRequestbuilder produces the correct request structure and integrates properly with the API. The shared examples provide good reusability.
173-185: Good validation testing with builder manipulation.The test correctly validates that invalid data in the builder raises appropriate errors. The direct manipulation of the internal data structure is acceptable for testing edge cases.
| # contacts_over_limit_count: 0 | ||
| # ) | ||
|
|
||
| sleep 3 # Wait for the import to complete (if needed) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hardcoded sleep from production example.
The sleep 3 statement is problematic in example code as it suggests users should hardcode delays in their applications. Consider replacing with a comment about polling or using a more robust waiting mechanism.
-sleep 3 # Wait for the import to complete (if needed)
+# In production, consider polling the import status instead of sleeping
+# loop do
+# import_status = contact_imports.get(contact_import_2.id)
+# break if %w[completed failed].include?(import_status.status)
+# sleep 1
+# end🤖 Prompt for AI Agents
In examples/contacts_api.rb at line 219, remove the hardcoded sleep 3 statement
as it encourages poor practice in production code. Replace it with a comment
suggesting users implement a polling mechanism or a more robust way to wait for
the import to complete instead of using fixed delays.
| let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key')) } | ||
| let(:base_url) { 'https://mailtrap.io/api/accounts/1111111' } |
There was a problem hiding this comment.
Reduce memoized helpers to fix RSpec violation.
The test has 8 memoized helpers but RSpec limit is 5. Consider consolidating some let blocks or converting them to regular variables within specific test contexts.
RSpec.describe Mailtrap::ContactImportsAPI do
let(:client) { described_class.new('1111111', Mailtrap::Client.new(api_key: 'correct-api-key')) }
let(:base_url) { 'https://mailtrap.io/api/accounts/1111111' }
describe '#get' do
- let(:import_id) { 'import-123' }
- let(:expected_response) do
- {
- 'id' => 'import-123',
- 'status' => 'finished',
- 'created_contacts_count' => 10,
- 'updated_contacts_count' => 2,
- 'contacts_over_limit_count' => 0
- }
- end
it 'returns a specific contact import' do
+ import_id = 'import-123'
+ expected_response = {
+ 'id' => 'import-123',
+ 'status' => 'finished',
+ 'created_contacts_count' => 10,
+ 'updated_contacts_count' => 2,
+ 'contacts_over_limit_count' => 0
+ }
+
stub_request(:get, "#{base_url}/contacts/imports/#{import_id}")Also applies to: 8-17, 50-73, 112-122
🤖 Prompt for AI Agents
In spec/mailtrap/contact_imports_api_spec.rb around lines 4 to 5, there are too
many memoized helpers defined with `let`, exceeding RSpec's recommended limit of
5. To fix this, consolidate some of the `let` blocks by combining related
variables or convert some of them into regular local variables inside specific
test contexts where they are used, reducing the total number of memoized
helpers. Apply the same approach to the other indicated line ranges (8-17,
50-73, 112-122).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/mailtrap/contact_imports_api_spec.rb (1)
4-5: Memoized helpers limit still exceeded.The issue with too many memoized helpers (exceeding RSpec's recommended limit of 5) hasn't been addressed from the previous review. There are still approximately 10
letblocks across the test file.
🧹 Nitpick comments (1)
spec/mailtrap/contact_imports_api_spec.rb (1)
173-185: Consider a more robust approach for testing validation.The test manually manipulates the internal
@datainstance variable to simulate invalid data. While this works, it's somewhat brittle as it depends on internal implementation details.Consider testing validation through the public interface instead:
- it 'validates request data and raises error when invalid options are provided' do - request = Mailtrap::ContactsImportRequest.new - # Manually add invalid data to simulate corrupted request - request.instance_variable_get(:@data)['[email protected]'] = { - email: '[email protected]', - fields: {}, - list_ids_included: [], - list_ids_excluded: [], - invalid_field: 'should not be here' - } - - expect { client.create(request) }.to raise_error(ArgumentError, /invalid options are given/) - end + it 'validates request data and raises error when invalid options are provided' do + # Test with array containing invalid data that bypasses builder validation + invalid_contacts = [{ + email: '[email protected]', + fields: {}, + list_ids_included: [], + list_ids_excluded: [], + invalid_field: 'should not be here' + }] + + expect { client.create(invalid_contacts) }.to raise_error(ArgumentError, /invalid options are given/) + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/mailtrap/contact_imports_api_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/mailtrap/contact_imports_api_spec.rb (2)
lib/mailtrap/contact_imports_api.rb (2)
get(18-20)create(33-40)lib/mailtrap/contacts_import_request.rb (3)
upsert(15-24)add_to_lists(30-39)remove_from_lists(45-54)
🪛 ast-grep (0.38.6)
spec/mailtrap/contact_imports_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)
🔇 Additional comments (5)
spec/mailtrap/contact_imports_api_spec.rb (5)
4-4: Hardcoded test credentials are acceptable.The static analysis tool flagged hardcoded credentials, but this is acceptable in test files where mock data is expected. Test files commonly use hardcoded values for predictable test scenarios.
19-46: Well-structured tests for the #get method.The tests provide good coverage with both success and error scenarios. The HTTP stubbing is properly configured and the response validation using
have_attributesis appropriate.
75-109: Comprehensive test coverage for the #create method.The tests thoroughly cover success scenarios, input validation, and API error handling. The request body validation in the stubs ensures the correct payload structure is being sent to the API.
124-144: Good use of helper methods and shared examples.The
expect_successful_importhelper and shared examples reduce code duplication and make the tests more maintainable.
146-171: Effective integration testing for the builder pattern.The step-by-step method calls test validates that the ContactsImportRequest builder correctly constructs the expected request body and integrates properly with the API client.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
spec/mailtrap/contact_imports_api_spec.rb (3)
7-47: Well-structured API testing with proper mocking and error handling.The tests effectively cover both success and error scenarios with appropriate HTTP stubbing and response verification. The error handling test is particularly valuable for ensuring robustness.
49-110: Comprehensive API creation testing with validation and error handling.The tests cover all critical scenarios including successful creation, input validation, and error handling. The validation test for invalid options is particularly important for maintaining data integrity.
4-235: Address the memoized helpers concern flagged in previous review.As noted in the previous review, this file exceeds RSpec's recommended limit of 5 memoized helpers. Consider consolidating related variables or converting single-use
letblocks to local variables within specific test contexts.
🧹 Nitpick comments (1)
spec/mailtrap/contact_imports_api_spec.rb (1)
111-186: Good integration testing with room for validation test improvement.The integration tests effectively verify the ContactsImportRequest builder works with the API. However, the validation test (lines 173-185) uses internal implementation details to corrupt data, which makes the test brittle.
Consider creating a more robust validation test:
- # Manually add invalid data to simulate corrupted request - request.instance_variable_get(:@data)['[email protected]'] = { - email: '[email protected]', - fields: {}, - list_ids_included: [], - list_ids_excluded: [], - invalid_field: 'should not be here' - } + # Create a mock request object that returns invalid data + invalid_request = double('ContactsImportRequest') + allow(invalid_request).to receive(:as_json).and_return([ + { + email: '[email protected]', + fields: {}, + list_ids_included: [], + list_ids_excluded: [], + invalid_field: 'should not be here' + } + ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/mailtrap/contacts_import_request.rb(1 hunks)spec/mailtrap/contact_imports_api_spec.rb(1 hunks)spec/mailtrap/contacts_import_request_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/mailtrap/contacts_import_request.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/mailtrap/contacts_import_request_spec.rb (1)
lib/mailtrap/contacts_import_request.rb (4)
as_json(58-60)upsert(15-24)add_to_lists(30-39)remove_from_lists(45-54)
🪛 ast-grep (0.38.6)
spec/mailtrap/contact_imports_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)
🔇 Additional comments (7)
spec/mailtrap/contacts_import_request_spec.rb (5)
1-30: Well-structured test setup with reusable patterns.The file organization is excellent with proper use of shared examples to reduce duplication and clear test data setup. The shared examples for method chaining and contact creation are particularly well-designed.
31-96: Comprehensive test coverage for upsert functionality.The tests thoroughly cover all upsert scenarios including new contact creation, field merging, overwriting, and preservation of list associations. The test cases are well-structured and verify the complete internal state after operations.
98-147: Solid test coverage for list inclusion functionality.The tests effectively cover both new contact creation and updates for existing contacts, with proper verification of duplicate prevention and method chaining. The specific test for chaining on the same email (lines 140-146) is a valuable edge case.
149-200: Consistent and thorough testing of list exclusion functionality.The tests maintain good consistency with the
add_to_liststests and provide complete coverage of the exclusion scenarios. The method chaining test on the same email is particularly valuable for ensuring the builder pattern works correctly.
202-248: Excellent serialization testing with integration verification.The serialization tests provide complete coverage including empty state, populated state with complex data, and alias method verification. The integration test effectively demonstrates the builder pattern working end-to-end with multiple operations.
spec/mailtrap/contact_imports_api_spec.rb (2)
1-6: Appropriate test setup with mock credentials.The hardcoded credentials are suitable for unit testing as they're clearly mock values. The static analysis warning about hardcoded credentials is a false positive in this context since these are test fixtures, not production secrets.
189-235: Appropriate testing of method alias functionality.The test correctly verifies that
#startworks identically to#create, which is essential for ensuring the alias behaves as expected.
Motivation
Support new functionality (Contact Imports API)
https://help.mailtrap.io/article/147-contacts and https://api-docs.mailtrap.io/docs/mailtrap-api-docs/b46b1fe35bdf1-import-contacts
Related to #57
Changes
Mailtrap::ContactImportsAPIentity for interactions withContactImportAPIcreatewhich accepts raw data or import requestMailtrap::ContactImportresponse objectMailtrap::ContactsImportRequestimport request builderHow to test
or set yout api key and account id
Summary by CodeRabbit