Contact Imports API #72
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Contact Imports feature: a ContactImportsAPI client with create/get (alias start), a ContactsImportRequest builder, a ContactImport DTO with to_h, example usage, and RSpec tests; wires the API into lib/mailtrap.rb and adjusts one contact spec. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Builder as ContactsImportRequest
participant API as ContactImportsAPI
participant HTTP as HTTP Client
participant Svc as Mailtrap Service
participant DTO as ContactImport
Dev->>Builder: upsert/add_to_lists/remove_from_lists...
Dev->>API: create(Builder.to_a or array)
API->>HTTP: POST /api/accounts/{account_id}/contacts/imports\nbody: { contacts: [...] }
HTTP->>Svc: request
Svc-->>HTTP: 201 Created { id, status, counts? }
HTTP-->>API: response
API-->>DTO: instantiate ContactImport
API-->>Dev: return ContactImport
Dev->>API: get(import_id)
API->>HTTP: GET /api/accounts/{account_id}/contacts/imports/{id}
HTTP->>Svc: request
Svc-->>HTTP: 200 OK { id, status, counts }
HTTP-->>API: response
API-->>DTO: instantiate ContactImport
API-->>Dev: return ContactImport
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
lib/mailtrap/contact_import.rb (1)
5-6: Fix type in YARD docs: id is Integer, not String.Specs and stubs use numeric IDs; align the doc to avoid confusion.
- # @attr_reader id [String] The contact import ID + # @attr_reader id [Integer] The contact import IDspec/mailtrap/contacts_import_request_spec.rb (2)
9-16: Fix typos in test data emails ("trhee" → "three").Minor, but reduces friction when scanning logs.
- .upsert(email: 'trhee@example.com', fields: { first_name: 'Jack' }) - .upsert(email: 'trhee@example.com', fields: { first_name: 'Joe', last_name: 'Blow', age: 33 }) + .upsert(email: 'three@example.com', fields: { first_name: 'Jack' }) + .upsert(email: 'three@example.com', fields: { first_name: 'Joe', last_name: 'Blow', age: 33 }) - .add_to_lists(email: 'trhee@example.com', list_ids: [1]) - .add_to_lists(email: 'trhee@example.com', list_ids: [1, 2]) + .add_to_lists(email: 'three@example.com', list_ids: [1]) + .add_to_lists(email: 'three@example.com', list_ids: [1, 2]) - .remove_from_lists(email: 'trhee@example.com', list_ids: [1]) - .remove_from_lists(email: 'trhee@example.com', list_ids: [1, 2]) + .remove_from_lists(email: 'three@example.com', list_ids: [1]) + .remove_from_lists(email: 'three@example.com', list_ids: [1, 2])Also applies to: 30-31, 53-54
65-66: Nit: rename example description.It returns a Ruby Array of Hashes, not JSON.
- it 'returns json array' do + it 'returns an array of hashes' dospec/mailtrap/contact_imports_api_spec.rb (2)
38-71: Exercise the start alias.Add a small check that start behaves like create to prevent regressions.
response = imports_api.create(request) expect(response).to have_attributes( id: 1, status: 'started', created_contacts_count: nil, updated_contacts_count: nil, contacts_over_limit_count: nil ) + + # Alias works the same as create + response2 = imports_api.start(request) + expect(response2).to have_attributes( + id: 1, + status: 'started', + created_contacts_count: nil, + updated_contacts_count: nil, + contacts_over_limit_count: nil + )
73-75: Optionally assert error message content.Helps ensure validate_options! bubble-up remains informative.
- expect { imports_api.create([{ email: 'john@example.com', foo: 1 }]) }.to raise_error(ArgumentError) + expect { imports_api.create([{ email: 'john@example.com', foo: 1 }]) } + .to raise_error(ArgumentError, /invalid options/i)examples/contacts_api.rb (2)
7-7: Prefer ENV for account_id to mirror the API key pattern.Keeps examples copy-pasteable without editing magic numbers.
-contact_imports = Mailtrap::ContactImportsAPI.new 3229, client +account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID').to_i +contact_imports = Mailtrap::ContactImportsAPI.new account_id, client
160-169: Consider adding a simple polling example for import completion.Many users will need a ready-to-use snippet.
-# Wait for the import to complete (if needed) +# Wait for the import to complete (if needed) +# loop do +# imp = contact_imports.get(contact_import.id) +# break if %w[finished failed].include?(imp.status) +# sleep 2 +# endlib/mailtrap/contact_imports_api.rb (2)
43-45: Doc fix: returned object is ContactImport.- # @return [ContactImport] Created contact list object + # @return [ContactImport] Created contact import object
46-52: Clarify create payload handling and (optionally) validate presence of email.Avoid relying on Array#each return value; make intent explicit. Optional: early check for missing email.
- def create(contacts) - contact_data = contacts.to_a.each do |contact| - validate_options!(contact, supported_options) - end - response = client.post(base_path, contacts: contact_data) - handle_response(response) - end + def create(contacts) + contact_data = contacts.is_a?(Array) ? contacts : contacts.to_a + contact_data.each do |contact| + validate_options!(contact, supported_options) + # Optional: ensure minimal payload correctness client-side + # raise ArgumentError, 'email is required' unless contact[:email].is_a?(String) && !contact[:email].empty? + end + response = client.post(base_path, contacts: contact_data) + handle_response(response) + endlib/mailtrap/contacts_import_request.rb (1)
51-55: Guard against nil or non-Array list_ids.Prevents NoMethodError on nil and clarifies contract.
- def append_list_ids(email:, list_ids:, key:) - raise ArgumentError, 'list_ids must not be empty' if list_ids.empty? - - @data[email][key] |= list_ids - end + def append_list_ids(email:, list_ids:, key:) + unless list_ids.is_a?(Array) && !list_ids.empty? + raise ArgumentError, 'list_ids must be a non-empty Array' + end + @data[email][key] |= list_ids + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/contacts_api.rb(3 hunks)lib/mailtrap.rb(1 hunks)lib/mailtrap/contact_import.rb(1 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)spec/mailtrap/contact_spec.rb(1 hunks)spec/mailtrap/contacts_import_request_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
spec/mailtrap/contacts_import_request_spec.rb (1)
lib/mailtrap/contacts_import_request.rb (4)
upsert(18-22)to_a(45-47)add_to_lists(28-32)remove_from_lists(38-42)
lib/mailtrap/contact_imports_api.rb (3)
lib/mailtrap/base_api.rb (4)
supported_options(27-29)response_class(31-33)base_get(46-49)validate_options!(35-40)lib/mailtrap/contacts_import_request.rb (1)
to_a(45-47)lib/mailtrap/client.rb (1)
post(81-83)
examples/contacts_api.rb (2)
lib/mailtrap/contact_imports_api.rb (2)
create(46-52)get(18-20)lib/mailtrap/contacts_import_request.rb (3)
upsert(18-22)remove_from_lists(38-42)add_to_lists(28-32)
spec/mailtrap/contact_imports_api_spec.rb (2)
lib/mailtrap/contact_imports_api.rb (2)
get(18-20)create(46-52)lib/mailtrap/contacts_import_request.rb (4)
upsert(18-22)add_to_lists(28-32)remove_from_lists(38-42)to_a(45-47)
🔇 Additional comments (9)
lib/mailtrap/contact_import.rb (2)
10-17: DTO structure is clean and fits the API use.Good field selection and keyword_init; compacted to_h is handy for nil counts.
18-21: Resolved — Hash#compact is safe (Ruby ≥ 3.1 declared)
mailtrap.gemspec sets spec.required_ruby_version = '>= 3.1.0' (line 15), so Hash#compact is available; no fallback required.spec/mailtrap/contact_spec.rb (2)
21-22: Behavior change: default is newly_created? => true.Verify Contact#newly_created? semantics when action is absent are intentional and documented.
36-56: to_h expectations look right.Matches removal of :action and omission of nils.
lib/mailtrap.rb (1)
11-11: Wire-up LGTM.Requiring contact_imports_api here keeps the top-level require cohesive.
spec/mailtrap/contacts_import_request_spec.rb (1)
21-23: Add a spec for nil list_ids.Once builder guards against nil (see suggested code), add a matching example raising ArgumentError on nil.
spec/mailtrap/contact_imports_api_spec.rb (1)
6-24: GET spec looks solid.Good attribute assertion against the DTO.
lib/mailtrap/contact_imports_api.rb (1)
57-59: Base path looks correct and consistent with other APIs.lib/mailtrap/contacts_import_request.rb (1)
18-22: Chainable builder API reads well.Merging fields on repeated upserts is the right choice.
| # @attr_reader created_contacts_count [Integer, nil] Number of contacts created in this import | ||
| # @attr_reader updated_contacts_count [Integer, nil] Number of contacts updated in this import | ||
| # @attr_reader contacts_over_limit_count [Integer, nil] Number of contacts over the allowed limit | ||
| ContactImport = Struct.new( |
There was a problem hiding this comment.
Or it could be Data.define
There was a problem hiding this comment.
We chose Struct for flexibility. Struct does not require all fields to be present. For example here contact import API does not return created_contacts_count when import is started. Another reason Data class raises an error when more arguments are passed to it. So we could not add more fields to the api without breaking the ruby sdk.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/mailtrap/contacts_import_request.rb (2)
57-59: Tighten email validation (trim) and basic format check.Current check accepts whitespace and strings without “@”.
Apply:
- def validate_email!(email) - raise ArgumentError, 'email must be present' if email.nil? || email.empty? - end + def validate_email!(email) + raise ArgumentError, 'email must be present' if email.nil? || email.strip.empty? + raise ArgumentError, 'email must contain @' unless email.include?('@') + end
55-66: Additional private helpers (to be added underprivate).Add:
# Normalizes emails for consistent keying def normalize_email(email) email.to_s.strip.downcase end # Validates and normalizes list_ids def normalize_list_ids(list_ids) raise ArgumentError, 'list_ids must be present' if list_ids.nil? || (list_ids.respond_to?(:empty?) && list_ids.empty?) raise ArgumentError, 'list_ids must be an Array' unless list_ids.is_a?(Array) unless list_ids.all? { |id| id.is_a?(Integer) && id.positive? } raise ArgumentError, 'list_ids must be positive Integers' end list_ids end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/mailtrap/contacts_import_request.rb(1 hunks)spec/mailtrap/contacts_import_request_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/mailtrap/contacts_import_request_spec.rb
🔇 Additional comments (3)
lib/mailtrap/contacts_import_request.rb (3)
7-11: Good use of a default proc to lazily seed per‑email state.Per‑key hashes/arrays aren’t shared; safe and clear.
1-68: API field names verified — use list_ids_included / list_ids_excluded.
lib/mailtrap/contact_imports_api.rb, lib/mailtrap/contacts_import_request.rb, lib/mailtrap/contacts_api.rb, examples and specs consistently use and test these fields in request bodies.
61-65: list_ids nil/type checks; make include/exclude last-write-winsNormalize/validate list_ids (handle nil/non-Array, coerce to Integer) and remove IDs from the opposite bucket so the last operation wins.
File: lib/mailtrap/contacts_import_request.rb (append_list_ids)
- def append_list_ids(email:, list_ids:, key:) - raise ArgumentError, 'list_ids must be present' if list_ids.empty? - - @data[email][key] |= list_ids + def append_list_ids(email:, list_ids:, key:) + list_ids = normalize_list_ids(list_ids) + @data[email][key] |= list_ids + opposite = key == :list_ids_included ? :list_ids_excluded : :list_ids_included + @data[email][opposite] -= list_idsAdd a helper
normalize_list_ids(list_ids)that coerces to Array, maps items to Integer (or raises on invalid types), and ensures the array is not empty.Confirm whether the API should enforce "last write wins" for conflicting include/exclude directives; if not, change to raise an error instead of reconciling.
| def upsert(email:, fields: {}) | ||
| validate_email!(email) | ||
|
|
||
| @data[email][:fields].merge!(fields) | ||
|
|
||
| self | ||
| end |
There was a problem hiding this comment.
Nil or non-Hash fields will raise; normalize email before indexing.
merge! will NoMethodError if fields: nil. Also normalize the email once (trim/downcase) before using it as a Hash key to avoid duplicate entries for the same logical address.
Apply:
- def upsert(email:, fields: {})
- validate_email!(email)
-
- @data[email][:fields].merge!(fields)
+ def upsert(email:, fields: {})
+ email = normalize_email(email)
+ validate_email!(email)
+ fields = {} if fields.nil?
+ raise ArgumentError, 'fields must be a Hash' unless fields.is_a?(Hash)
+ @data[email][:fields].merge!(fields.transform_keys(&:to_s))Add normalize_email in the private section (see helper snippet below).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/mailtrap/contacts_import_request.rb around lines 18 to 24, the upsert
method uses the raw email as a hash key and calls merge! on fields which will
raise if fields is nil or not a Hash; normalize the email once (trim and
downcase) before indexing and ensure fields is turned into a Hash (e.g. coerce
nil to {} or call to_h) before merging. Change upsert to compute
normalized_email = normalize_email(email), validate that normalized_email,
ensure safe_fields = (fields || {}).to_h, then use
@data[normalized_email][:fields].merge!(safe_fields) and return self; add a
private normalize_email method (trim and downcase) in the private section as
requested.
| def add_to_lists(email:, list_ids:) | ||
| validate_email!(email) | ||
|
|
||
| append_list_ids email:, list_ids:, key: :list_ids_included | ||
|
|
||
| self | ||
| end |
There was a problem hiding this comment.
Handle nil/invalid list_ids and normalize email before appending.
list_ids.empty? will blow up on nil. Also prefer validating content (positive Integers).
Apply:
- def add_to_lists(email:, list_ids:)
- validate_email!(email)
-
- append_list_ids email:, list_ids:, key: :list_ids_included
+ def add_to_lists(email:, list_ids:)
+ email = normalize_email(email)
+ validate_email!(email)
+ list_ids = normalize_list_ids(list_ids)
+ append_list_ids email:, list_ids:, key: :list_ids_included📝 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 add_to_lists(email:, list_ids:) | |
| validate_email!(email) | |
| append_list_ids email:, list_ids:, key: :list_ids_included | |
| self | |
| end | |
| def add_to_lists(email:, list_ids:) | |
| email = normalize_email(email) | |
| validate_email!(email) | |
| list_ids = normalize_list_ids(list_ids) | |
| append_list_ids email:, list_ids:, key: :list_ids_included | |
| self | |
| end |
🤖 Prompt for AI Agents
In lib/mailtrap/contacts_import_request.rb around lines 30 to 36, the method
add_to_lists currently assumes list_ids is a non-nil array and uses the raw
email; change it to first normalize the email (strip and downcase) before
calling validate_email!, coerce list_ids into an array if a single Integer was
passed, return early if list_ids is nil/empty, and validate each id is a
positive Integer (raise ArgumentError for invalid entries) before calling
append_list_ids with the normalized email and validated list IDs.
| def remove_from_lists(email:, list_ids:) | ||
| validate_email!(email) | ||
|
|
||
| append_list_ids email:, list_ids:, key: :list_ids_excluded | ||
|
|
||
| self | ||
| end |
There was a problem hiding this comment.
Mirror the same validation/normalization in remove_from_lists.
Apply:
- def remove_from_lists(email:, list_ids:)
- validate_email!(email)
-
- append_list_ids email:, list_ids:, key: :list_ids_excluded
+ def remove_from_lists(email:, list_ids:)
+ email = normalize_email(email)
+ validate_email!(email)
+ list_ids = normalize_list_ids(list_ids)
+ append_list_ids email:, list_ids:, key: :list_ids_excluded📝 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 remove_from_lists(email:, list_ids:) | |
| validate_email!(email) | |
| append_list_ids email:, list_ids:, key: :list_ids_excluded | |
| self | |
| end | |
| def remove_from_lists(email:, list_ids:) | |
| email = normalize_email(email) | |
| validate_email!(email) | |
| list_ids = normalize_list_ids(list_ids) | |
| append_list_ids email:, list_ids:, key: :list_ids_excluded | |
| self | |
| end |
🤖 Prompt for AI Agents
In lib/mailtrap/contacts_import_request.rb around lines 42 to 48,
remove_from_lists only calls validate_email! and append_list_ids but doesn't
apply the same list_ids validation/normalization used elsewhere; update
remove_from_lists to mirror add_to_lists by validating and normalizing list_ids
(e.g., call validate_list_ids! or normalize_list_ids! as used by add_to_lists),
ensure the email is normalized the same way if add_to_lists normalizes it, then
call append_list_ids with the normalized values and key: :list_ids_excluded, and
return self.
| def to_a | ||
| @data.values | ||
| end |
There was a problem hiding this comment.
to_a exposes live internal structures; deep copy to avoid accidental mutation.
Returning references allows callers/tests to mutate builder state unexpectedly.
Apply:
- def to_a
- @data.values
- end
+ def to_a
+ @data.values.map do |c|
+ {
+ email: c[:email],
+ fields: c[:fields].dup,
+ list_ids_included: c[:list_ids_included].dup,
+ list_ids_excluded: c[:list_ids_excluded].dup
+ }
+ end
+ 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.
| def to_a | |
| @data.values | |
| end | |
| def to_a | |
| @data.values.map do |c| | |
| { | |
| email: c[:email], | |
| fields: c[:fields].dup, | |
| list_ids_included: c[:list_ids_included].dup, | |
| list_ids_excluded: c[:list_ids_excluded].dup | |
| } | |
| end | |
| end |
🤖 Prompt for AI Agents
In lib/mailtrap/contacts_import_request.rb around lines 51 to 53, the to_a
method currently returns @data.values which exposes internal objects to callers;
change it to return a deep copy of the values array so callers cannot mutate
internal state. Specifically, produce and return a new array where each element
is deep-duplicated (recursively duplicate hashes/arrays/strings as needed)
rather than returning the raw references; ensure the deep-dup approach preserves
value types and structure but isolates mutations from @data.
4 participants
Based on closed PR #64
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
New Features
Documentation
Tests