-
Notifications
You must be signed in to change notification settings - Fork 13
Fix optional account #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… resource constructors
WalkthroughRefactors to require a concrete Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant MC as MailtrapClient
participant V as validateAccountIdPresence()
participant Base as BaseAPI (e.g., ContactsBaseAPI)
participant Res as ResourceApi (e.g., ContactsApi)
Dev->>MC: access client.contacts (getter)
MC->>V: validateAccountIdPresence()
V-->>MC: accountId (number)
MC->>Base: new ContactsBaseAPI(client, accountId)
activate Base
Base->>Res: new ContactsApi(client, accountId)
Res-->>Base: instance (methods bound)
Base-->>Dev: exposed methods (get/create/update/delete)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 check |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Unrecoverable error: Request timed out after 300000ms (requestId=a1774396-b9c8-4024-a18c-ba87fabb99ef) 🏁 Script executed:
Length of output: 605 🏁 Script executed:
Length of output: 91 🏁 Script executed:
Length of output: 6505 🏁 Script executed:
Length of output: 7805 🏁 Script executed:
Length of output: 7735 @narekhovhannisyan I've reviewed PR #91. Here's my analysis: SummaryThis PR fixes a type safety issue by making Key Changes1. Core Validation Enhancement (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/lib/api/resources/ContactLists.ts (1)
17-19
: Consistent with the required accountId pattern.Same pattern as other API resources—accountId is now mandatory and used directly for URL construction.
src/lib/api/resources/Attachments.ts (1)
15-17
: Consistent pattern applied.src/lib/api/resources/Contacts.ts (1)
18-20
: Consistent pattern applied.src/lib/api/resources/Messages.ts (1)
21-23
: Consistent pattern applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.md
(1 hunks)src/lib/MailtrapClient.ts
(2 hunks)src/lib/api/ContactFields.ts
(1 hunks)src/lib/api/ContactLists.ts
(1 hunks)src/lib/api/Contacts.ts
(1 hunks)src/lib/api/General.ts
(1 hunks)src/lib/api/Suppressions.ts
(1 hunks)src/lib/api/Templates.ts
(1 hunks)src/lib/api/Testing.ts
(1 hunks)src/lib/api/resources/AccountAccesses.ts
(1 hunks)src/lib/api/resources/Attachments.ts
(1 hunks)src/lib/api/resources/ContactFields.ts
(1 hunks)src/lib/api/resources/ContactLists.ts
(1 hunks)src/lib/api/resources/Contacts.ts
(1 hunks)src/lib/api/resources/Inboxes.ts
(1 hunks)src/lib/api/resources/Messages.ts
(1 hunks)src/lib/api/resources/Permissions.ts
(2 hunks)src/lib/api/resources/Projects.ts
(1 hunks)src/lib/api/resources/Suppressions.ts
(1 hunks)src/lib/api/resources/Templates.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/lib/api/Contacts.ts (2)
src/lib/MailtrapClient.ts (1)
contacts
(127-130)src/lib/api/resources/Contacts.ts (1)
ContactsApi
(13-59)
src/lib/api/Testing.ts (4)
src/lib/api/resources/Projects.ts (1)
ProjectsApi
(10-63)src/lib/api/resources/Inboxes.ts (1)
InboxesApi
(10-117)src/lib/api/resources/Messages.ts (1)
MessagesApi
(16-152)src/lib/api/resources/Attachments.ts (1)
AttachmentsApi
(10-37)
src/lib/MailtrapClient.ts (8)
src/lib/MailtrapError.ts (1)
MailtrapError
(1-1)src/lib/api/Testing.ts (1)
TestingAPI
(8-23)src/lib/api/General.ts (1)
GeneralAPI
(7-19)src/lib/api/Contacts.ts (1)
ContactsBaseAPI
(5-21)src/lib/api/ContactLists.ts (1)
ContactListsBaseAPI
(5-24)src/lib/api/ContactFields.ts (1)
ContactFieldsBaseAPI
(5-24)src/lib/api/Templates.ts (1)
TemplatesBaseAPI
(5-24)src/lib/api/Suppressions.ts (1)
SuppressionsBaseAPI
(5-15)
src/lib/api/General.ts (2)
src/lib/api/resources/Accounts.ts (1)
AccountsApi
(10-29)src/lib/api/resources/Permissions.ts (1)
PermissionsApi
(13-60)
src/lib/api/Suppressions.ts (2)
src/lib/MailtrapClient.ts (1)
suppressions
(159-162)src/lib/api/resources/Suppressions.ts (1)
SuppressionsApi
(9-41)
src/lib/api/Templates.ts (2)
src/lib/MailtrapClient.ts (1)
templates
(151-154)src/lib/api/resources/Templates.ts (1)
TemplatesApi
(13-69)
src/lib/api/ContactLists.ts (2)
src/lib/MailtrapClient.ts (1)
contactLists
(135-138)src/lib/api/resources/ContactLists.ts (1)
ContactListsApi
(12-67)
src/lib/api/ContactFields.ts (2)
src/lib/MailtrapClient.ts (1)
contactFields
(143-146)src/lib/api/resources/ContactFields.ts (1)
ContactFieldsApi
(13-71)
🔇 Additional comments (11)
src/lib/api/resources/AccountAccesses.ts (1)
19-21
: LGTM! Type-safe URL construction.The constructor now requires
accountId
, preventing runtime URL construction errors (e.g.,/api/accounts/undefined/account_accesses
). The URL is built directly from the constructor parameter, which is cleaner and enforces compile-time safety.src/lib/api/resources/Permissions.ts (2)
18-20
: LGTM! Required accountId prevents malformed URLs.Constructor now requires
accountId
, ensuring type-safe URL construction.
36-36
: Good refactor: reuses base URL.The method now reuses
this.permissionsURL
instead of reconstructing the base path inline, which reduces duplication and improves maintainability.src/lib/api/resources/ContactFields.ts (1)
18-20
: LGTM! Consistent with the required accountId pattern.The constructor changes across all API resource files are uniform and correct. Making
accountId
required prevents runtime URL construction errors and provides compile-time type safety.src/lib/api/Contacts.ts (1)
14-20
: LGTM! Clean refactor to require accountId.The constructor now requires a concrete
accountId
and passes it directly toContactsApi
instead of storing it internally. The method binding pattern is correct and maintains proper context.src/lib/MailtrapClient.ts (2)
92-96
: Excellent improvement to return the validated accountId.Changing the return type from
void
tonumber
makes the validation more functional and explicit. Callers can immediately use the validated value without re-readingthis.accountId
.
112-161
: LGTM! Consistent pattern across all API getters.All seven API getters (
testing
,general
,contacts
,contactLists
,contactFields
,templates
,suppressions
) follow the same pattern:
- Validate and capture the returned
accountId
- Pass it to the API constructor
This ensures type safety and runtime validation before API instantiation.
src/lib/api/resources/Inboxes.ts (1)
13-21
: LGTM! Correctly stores accountId for runtime usage.Unlike some other resources,
InboxesApi
storesaccountId
because thecreate
method (line 27) needs it for URL construction. Making it required (non-optional) prevents runtime errors from undefined values.src/lib/api/resources/Templates.ts (1)
18-21
: LGTM! Efficient approach for construction-only usage.The constructor correctly uses
accountId
directly for URL construction without storing it on the instance. This is appropriate sinceaccountId
is only needed once at initialization.src/lib/api/Suppressions.ts (1)
10-14
: LGTM! Consistent with the base API pattern.The constructor correctly requires
accountId
, instantiatesSuppressionsApi
directly, and binds its methods without storing internal state. This matches the pattern established in other base API classes.src/lib/api/Testing.ts (1)
17-22
: LGTM! Consistent initialization pattern.The constructor correctly requires
accountId
and passes it to all four resource API constructors (ProjectsApi
,InboxesApi
,MessagesApi
,AttachmentsApi
). This ensures all testing-related APIs have the requiredaccountId
at construction time.
00fb81c
to
60da81e
Compare
…cific APIs based on accountId presence
…stantiation scenarios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/__tests__/lib/api/General.test.ts (6)
42-49
: Simplify toThrow usage; drop ESLint disableYou can assert getter throws without wrapping a block and disabling ESLint.
Apply:
- expect(() => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - generalAPI.accountAccesses; - }).toThrow( + expect(() => generalAPI.accountAccesses).toThrow( "Account ID is required for this operation. Please provide accountId when creating GeneralAPI instance." );
51-58
: Same simplification for permissions getter- expect(() => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - generalAPI.permissions; - }).toThrow( + expect(() => generalAPI.permissions).toThrow( "Account ID is required for this operation. Please provide accountId when creating GeneralAPI instance." );
89-96
: Test title is misleading (not actually lazy with accountId)Constructor eagerly instantiates accountAccesses when accountId is present. Reword title.
- it("instantiates accountAccesses lazily when accountId is provided but not accessed during construction.", () => { + it("provides accountAccesses when accountId is provided.", () => {
97-104
: Same: clarify permissions test title- it("instantiates permissions lazily when accountId is provided but not accessed during construction.", () => { + it("provides permissions when accountId is provided.", () => {
135-142
: Simplify getter throw assertion (undefined accountId case)- expect(() => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - generalAPI.accountAccesses; - }).toThrow(); + expect(() => generalAPI.accountAccesses).toThrow();
144-151
: Simplify getter throw assertion (null accountId case)- expect(() => { - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - generalAPI.accountAccesses; - }).toThrow(); + expect(() => generalAPI.accountAccesses).toThrow();src/lib/api/General.ts (3)
23-31
: Consider fully lazy instantiation for account-scoped APIsEager creation increases work and contradicts the lazy accessors. Let getters instantiate on first access.
- // Only instantiate account-specific APIs if accountId is provided - if (this.accountId !== null) { - this.accountAccessesInstance = new AccountAccessesApi( - client, - this.accountId - ); - this.permissionsInstance = new PermissionsApi(client, this.accountId); - }Note: Adjust tests that assert “instantiates immediately” accordingly.
33-40
: Prefer a project-specific error type for consistencyThrow the same error class used elsewhere (e.g., MailtrapError) to keep error handling uniform across SDK.
Replace
new Error(...)
with the shared SDK error (same message). Based on PR objectives.
8-16
: Minor: mark immutable fields as readonlyClient and accounts aren’t reassigned; marking readonly helps prevent accidental mutation.
- public accounts: AccountsApi; + public readonly accounts: AccountsApi; - private client: AxiosInstance; + private readonly client: AxiosInstance;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/lib/api/General.test.ts
(1 hunks)src/lib/api/General.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/__tests__/lib/api/General.test.ts (1)
src/lib/api/General.ts (2)
accountAccesses
(42-52)permissions
(54-61)
src/lib/api/General.ts (2)
src/lib/api/resources/Accounts.ts (1)
AccountsApi
(10-29)src/lib/api/resources/Permissions.ts (1)
PermissionsApi
(13-60)
🔇 Additional comments (1)
src/lib/api/General.ts (1)
42-61
: LGTM: Accounts API remains accessible without accountIdThis addresses the earlier gating concern; per‑account APIs are guarded via presence check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/__tests__/lib/api/General.test.ts (1)
75-111
: Consider simplifying implementation-focused test descriptions.While these tests provide good coverage, several descriptions are overly specific about implementation details:
- Line 89: "instantiates accountAccesses lazily when accountId is provided but not accessed during construction"
- Line 97: "instantiates permissions lazily when accountId is provided but not accessed during construction"
- Line 105: "covers checkAccountIdPresence return path when accountId is valid"
These tests verify behavior (getters return working API instances) rather than implementation (lazy vs. eager instantiation). Consider focusing descriptions on observable behavior.
Example revision:
- it("instantiates accountAccesses lazily when accountId is provided but not accessed during construction.", () => { + it("provides working accountAccesses API when accountId is available.", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/lib/api/General.test.ts
(1 hunks)src/lib/api/General.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/api/General.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/lib/api/General.test.ts (1)
src/lib/api/General.ts (2)
accountAccesses
(42-52)permissions
(54-61)
🔇 Additional comments (3)
src/__tests__/lib/api/General.test.ts (3)
6-16
: LGTM!The test setup and initial property checks are well-structured and correctly verify that all necessary properties are present when
accountId
is provided.
25-73
: Excellent coverage of accountId scenarios.These test suites comprehensively verify:
- Constructor behavior without
accountId
- Error handling for account-specific APIs without
accountId
- Account discovery functionality that doesn't require
accountId
The coverage aligns well with the PR objectives to make
accountId
optional in the client while requiring it for account-specific operations.
134-162
: Excellent edge case coverage.These tests properly verify boundary conditions:
undefined
accountId → should throw when accessing account-specific APIsnull
accountId → should throw when accessing account-specific APIs0
accountId → should work (0 is a valid account ID despite being falsy)The test at line 153 is particularly valuable as it confirms the implementation doesn't conflate 0 with missing accountId.
…for account-specific APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/lib/api/General.test.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/lib/api/General.test.ts (1)
src/lib/api/General.ts (2)
accountAccesses
(42-52)permissions
(54-61)
🔇 Additional comments (4)
src/__tests__/lib/api/General.test.ts (4)
6-37
: LGTM! Clear separation of constructor scenarios.The tests properly validate both constructor paths—with and without
accountId
—and confirm that account discovery works independently of account-specific APIs.
39-73
: LGTM! Comprehensive error handling and account discovery coverage.The tests correctly verify that accessing account-specific APIs without
accountId
throws descriptive errors, while the accounts API remains accessible for account discovery.
75-86
: LGTM! Backward compatibility validated.This test confirms that the refactored lazy instantiation preserves the existing API surface for account-specific operations.
132-141
: LGTM! Critical edge case for accountId = 0.This test ensures that
0
is treated as a validaccountId
rather than a falsy value, which is important for correctness.
…APIs in GeneralAPI
Motivation
The Mailtrap client was inconsistently handling the
accountId
parameter across different API classes. While the main client class requiredaccountId
to be mandatory, all API resource classes and base classes were treating it as optional, creating a mismatch in the API design. Additionally, users should be able to use the client for basic email sending without needing to provide anaccountId
, as it's only required for accessing account-specific APIs like contacts, templates, etc.fixes #90
Changes
accountId
optional inMailtrapClientConfig
andMailtrapClient
to allow basic email sending without account IDContacts
,ContactLists
,ContactFields
,Templates
,Suppressions
,AccountAccesses
,Permissions
,Inboxes
,Projects
,Messages
,Attachments
,SendingDomains
) to requireaccountId
in constructorsContactsBaseAPI
,ContactListsBaseAPI
,ContactFieldsBaseAPI
,TemplatesBaseAPI
,SuppressionsBaseAPI
) to requireaccountId
in constructorsGeneralAPI
andTestingAPI
to requireaccountId
in constructorsvalidateAccountIdPresence()
method that throwsMailtrapError
whenaccountId
is missing, used by all API gettersthis.client
andthis.accountId
fields fromContactFieldsBaseAPI
to eliminate redundant state storagetesting
,general
,contacts
,contactLists
,contactFields
,templates
,suppressions
) to validateaccountId
presence before instantiating APIsHow to test
MailtrapClient
can be instantiated withoutaccountId
for basic email sendingclient.contacts
,client.templates
, etc.) throwsMailtrapError
whenaccountId
is missingaccountId
is providedaccountId
accountId
in their constructorsSummary by CodeRabbit
Documentation
Refactor
Tests
Impact