Contact fields #89
Contact fields #89
Conversation
…pdate, get, and delete operations
WalkthroughAdds Contact Fields CRUD support: new API resource, base wrapper, and client getter; introduces TypeScript types; provides example script; updates README; and adds unit/integration tests for all CRUD operations and error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Client as MailtrapClient
participant CFBase as ContactFieldsBaseAPI
participant CFAPI as ContactFieldsApi
participant HTTP as Axios/HTTP
participant Svc as Contact Fields Service
Dev->>Client: new MailtrapClient({ token, accountId })
Dev->>Client: client.contactFields
Client->>CFBase: construct (validates accountId)
CFBase->>CFAPI: init with Axios + accountId
rect rgba(200,230,255,0.3)
note right of Dev: Example: getList
Dev->>CFBase: getList()
CFBase->>CFAPI: getList()
CFAPI->>HTTP: GET /api/accounts/{accountId}/contact-fields
HTTP->>Svc: request
Svc-->>HTTP: 200 { data: [...] }
HTTP-->>CFAPI: response.data
CFAPI-->>CFBase: ContactFieldsResponse
CFBase-->>Dev: ContactFieldsResponse
end
rect rgba(220,255,220,0.3)
note right of Dev: Other ops
Dev->>CFBase: create/get/update/delete(...)
CFBase->>CFAPI: delegate(...)
CFAPI->>HTTP: POST/GET/PATCH/DELETE endpoints
HTTP-->>CFAPI: data or error
alt success
CFAPI-->>Dev: Response
else error
CFAPI-->>Dev: throws MailtrapError
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
examples/contact-fields/everything.ts (2)
18-18: Improve type safety.The type assertion
(fields as any)bypasses TypeScript's type checking. Consider using proper typing instead.Apply this diff to improve type safety:
- const firstField = (fields as any)[0]; + const firstField = Array.isArray(fields.data) ? fields.data[0] : null; if (!firstField) {Or add a type guard if you want to be more explicit:
const fieldsData = fields as ContactFieldsResponse; const firstField = fieldsData.data[0];
11-40: Consider adding a create() example.The example demonstrates getList, get, update, and delete operations but doesn't show how to create a new contact field. Adding a create() example would make this a complete CRUD demonstration.
Consider adding a create example before the getList call:
// Create a new contact field const newField = await client.contactFields.create({ name: "Phone Number", merge_tag: "phone", data_type: "text", }); console.log("Created field:", newField);src/lib/api/resources/ContactFields.ts (1)
26-30: Simplify URL usage.Line 27 uses a redundant template literal. The variable
this.contactFieldsURLalready contains the full URL.Apply this diff to simplify:
public async getList() { - const url = `${this.contactFieldsURL}`; - - return this.client.get<ContactFieldsResponse, ContactFieldsResponse>(url); + return this.client.get<ContactFieldsResponse, ContactFieldsResponse>( + this.contactFieldsURL + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)examples/contact-fields/everything.ts(1 hunks)src/__tests__/lib/api/ContactFields.test.ts(1 hunks)src/__tests__/lib/api/resources/ContactFields.test.ts(1 hunks)src/lib/MailtrapClient.ts(2 hunks)src/lib/api/ContactFields.ts(1 hunks)src/lib/api/resources/ContactFields.ts(1 hunks)src/types/api/contact-fields.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/lib/api/ContactFields.ts (2)
src/lib/api/resources/ContactFields.ts (1)
ContactFieldsApi(13-71)src/lib/MailtrapClient.ts (1)
contactFields(146-150)
src/lib/api/resources/ContactFields.ts (1)
src/types/api/contact-fields.ts (3)
ContactFieldsResponse(20-22)ContactFieldResponse(16-18)ContactFieldOptions(10-14)
src/lib/MailtrapClient.ts (1)
src/lib/api/ContactFields.ts (1)
ContactFieldsBaseAPI(5-30)
src/__tests__/lib/api/resources/ContactFields.test.ts (4)
src/lib/api/resources/ContactFields.ts (1)
ContactFieldsApi(13-71)src/types/api/contact-fields.ts (1)
ContactFieldOptions(10-14)src/lib/axios-logger.ts (1)
handleSendingError(20-69)src/lib/MailtrapError.ts (1)
MailtrapError(1-1)
🔇 Additional comments (9)
src/__tests__/lib/api/ContactFields.test.ts (1)
1-20: LGTM!The test appropriately validates that the ContactFields wrapper API exposes all necessary CRUD methods.
src/lib/MailtrapClient.ts (2)
14-14: LGTM!Import follows the established pattern for other API wrappers.
143-150: LGTM!The contactFields getter follows the same pattern as other API getters (contacts, contactLists, templates), validating accountId before instantiation.
src/__tests__/lib/api/resources/ContactFields.test.ts (1)
1-315: LGTM!Comprehensive test suite with excellent coverage:
- All CRUD operations tested with success and failure paths
- Proper use of axios-mock-adapter
- Request URL validation via mock.history
- Error handling validates MailtrapError instances and messages
- Includes partial update test case
src/types/api/contact-fields.ts (1)
1-22: LGTM!Clean and well-structured type definitions that properly describe the Contact Fields API surface.
src/lib/api/resources/ContactFields.ts (3)
44-49: LGTM!The create method correctly uses POST with the provided data.
54-61: LGTM!The update method correctly uses PATCH with the field ID and partial data.
66-70: LGTM!The delete method correctly uses DELETE with the field ID.
src/lib/api/ContactFields.ts (1)
1-30: LGTM!The wrapper class correctly binds methods from ContactFieldsApi to ensure proper
thiscontext when methods are used as standalone functions or destructured. This follows the same pattern used by other API wrappers in the codebase.
| constructor(client: AxiosInstance, accountId?: number) { | ||
| this.client = client; | ||
| this.contactFieldsURL = `${GENERAL_ENDPOINT}/api/accounts/${accountId}/contacts/fields`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Validate accountId in constructor.
The URL is constructed with accountId which may be undefined, resulting in URLs like /api/accounts/undefined/contacts/fields. Consider adding validation or making accountId required.
Apply this diff to validate accountId:
constructor(client: AxiosInstance, accountId?: number) {
+ if (!accountId) {
+ throw new Error("accountId is required for ContactFields API");
+ }
this.client = client;
this.contactFieldsURL = `${GENERAL_ENDPOINT}/api/accounts/${accountId}/contacts/fields`;
}Or verify if this pattern is intentional by checking other similar APIs:
🏁 Script executed:
#!/bin/bash
# Check if other API resources handle accountId similarly
rg -n -A5 'constructor.*accountId\?' src/lib/api/resources/ | head -50Length of output: 3113
Make accountId a required constructor parameter.
The accountId is currently optional but used directly in URL templates, leading to paths like /undefined. Remove the ? on accountId in the constructor (e.g., accountId: number) for ContactFields and all other resources in src/lib/api/resources to enforce compile-time safety.
🤖 Prompt for AI Agents
In src/lib/api/resources/ContactFields.ts around lines 18 to 21, the constructor
currently accepts an optional accountId but interpolates it into URLs causing
`/undefined` at runtime; change the constructor signature to require accountId
(remove the `?`), update the internal assignment to use the required parameter,
and propagate the same change to all other resource classes in
src/lib/api/resources (make accountId required in their constructors and update
any call sites/instantiations to pass a numeric accountId) so compilation
enforces the presence of accountId and eliminates URL construction with
undefined.
There was a problem hiding this comment.
since this issue is present across the entire codebase, it's preferable to introduce separate PR for it.
There was a problem hiding this comment.
@narekhovhannisyan, that makes sense! Handling this consistently across all resource classes in a dedicated PR will ensure a cleaner separation of concerns.
Would you like me to create a GitHub issue to track this enhancement? I can include details about making accountId required in all resource constructors to prevent the /undefined URL issue.
There was a problem hiding this comment.
good suggestion, do it
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track making accountId required across all resource classes
gh issue create \
--title "Make accountId required in all API resource constructors" \
--body "## Problem
Currently, the \`accountId\` parameter is optional in resource class constructors (e.g., ContactFields, Contacts, etc.), but it's used directly in URL template construction. This can lead to malformed URLs like \`/api/accounts/undefined/contacts/fields\` when \`accountId\` is not provided.
## Proposed Solution
Make \`accountId\` a required parameter (remove the \`?\`) in all resource class constructors under \`src/lib/api/resources/\` to enforce compile-time safety and prevent runtime URL construction errors.
## Affected Files
All resource classes in:
- \`src/lib/api/resources/\`
This change should be applied consistently across the codebase to ensure all resources validate the presence of \`accountId\` at compile time.
## Context
This issue was identified during the review of PR #89.
**Related PR:** #89
**Related Comment:** https://github.com/railsware/mailtrap-nodejs/pull/89#discussion_r2407281020
**Reported by:** @narekhovhannisyan" \
--assignee narekhovhannisyan
Length of output: 54
@narekhovhannisyan, I've created issue #90 to track making accountId required across all API resource constructors. The issue includes details about the problem, proposed solution, and links back to this PR and comment.
You can view it here: #90
narekhovhannisyan
requested review from
VladimirTaytor,
leonid-shevtsov and
mklocek
3 participants
Motivation
Add first-class support for Contact Fields management to the Node SDK so users can create, update, list, and delete custom fields without making raw HTTP calls. This closes the gap between available Mailtrap API endpoints and the SDK, improves developer ergonomics, and provides end‑to‑end examples and tests.
fixes #72
Changes
src/types/api/contact-fields.ts.src/index.ts) and API index.src/__tests__/lib/api/ContactFields.test.tssrc/__tests__/lib/api/resources/ContactFields.test.tsexamples/contact-fields/everything.ts.examples.ts) to include Contact Fields demo.dist/for new modules.How to test
yarn testyarn test ContactFieldsMAILTRAP_API_TOKENwith a valid API token.examples/contact-fields/everything.tsand ensureaccountIdis correct.ts-node examples/contact-fields/everything.ts(oryarn ts-node examples/contact-fields/everything.ts)yarn buildcompletes without errors.yarn lintpasses cleanly.Summary by CodeRabbit
New Features
Documentation
Examples
Tests