Restructure SDK #51
Restructure SDK #51
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| it("rejects with Mailtrap error, bulk and sanbox modes are incompatible.", async () => { | ||
| const client = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
There was a problem hiding this comment.
Please use the recommended ways to disable the Code Scanning checks in tests
|
|
||
| it("successfully sends testing email.", async () => { | ||
| const testingClient = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
|
|
||
| it("successfully sends bulk email.", async () => { | ||
| const bulkClient = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
|
|
||
| it("handles an API error.", async () => { | ||
| const testingClient = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
|
|
||
| it("handles an HTTP transport error.", async () => { | ||
| const testingClient = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
| expect.assertions(3); | ||
| it("rejects with Mailtrap error, when `testInboxId` is missing.", () => { | ||
| const client = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
|
|
||
| it("returns testing API object, console warn is called twice.", () => { | ||
| const client = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
| describe("get general(): ", () => { | ||
| it("returns testing API object, console warn is called twice.", () => { | ||
| const client = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical test
|
|
||
| it("rejects with Mailtrap error, bulk and sanbox modes are incompatible.", async () => { | ||
| const client = new MailtrapClient({ | ||
| token: "MY_API_TOKEN", |
There was a problem hiding this comment.
Please use the recommended ways to disable the Code Scanning checks in tests
| } | ||
| } | ||
| }); | ||
|
|
||
| it("sends request to mailtrap api", async () => { |
There was a problem hiding this comment.
Since we have a bunch of tests dedicated to sending emails using Sending API's send method, I suggest grouping them under a describe. Probably, even like this:
describe("configuration errors", () => {
it("rejects with Mailtrap error, bulk and sanbox modes are incompatible.", ...)
})
describe("using Testing API", () => {
it("successfully sends testing email.", ...)
it("handles an API error.", ...)
})
describe("using Sending API", () => {
// all Sending API examples
})
describe("using Bulk API", () => {
it("successfully sends email using the Bulk API.", ...)
it("handles an API error.", ...)
})
describe("API endpoint objects", () => {
// getXXXApi examples
})There was a problem hiding this comment.
they are under send method's describe section. At the moment we don't have multiple test cases for each API method. I guess we can introduce it later when we will deeply cover all the methods.
vittorius
force-pushed
the
restructure-sdk
branch
3 times, most recently
from
7e1b802
to
52fcd1a
Compare
vittorius
force-pushed
the
restructure-sdk
branch
from
52fcd1a
to
f764542
Compare
| @@ -6,6 +6,9 @@ jobs: | |||
| lint: | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| - uses: github/codeql-action/init@v3 | |||
There was a problem hiding this comment.
I guess since codeql is not triggered from the workflow, we don't have to mention it here.
|
@coderabbitai review |
Actions performedReview triggered.
|
WalkthroughThe changes encompass the introduction and modification of various configuration files, examples, and tests within the project. A new CodeQL configuration file was added to streamline code analysis, while updates were made to the GitHub Actions workflow to integrate CodeQL analysis. The Changes
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
Outside diff range and nitpick comments (3)
examples/bulk/send-mail.ts (1)
17-17: Consider adding a similar example for thesandbox: trueflag usage.As suggested in the past review comments, it would be great to add a similar example under the
/examples/testingdirectory to illustrate thesandbox: trueflag usage. This will provide a comprehensive set of examples for different use cases.examples/testing/send-mail.ts (1)
1-25: LGTM!The code provides a clear and concise example of how to send emails using the Mailtrap client. The comments and constants make it easy for users to understand and modify the code to suit their needs. The promise handling ensures that the success or error of the email sending operation is logged.
Here are a few suggestions to further improve the code:
- Consider adding error handling for the case where the required constants are not provided.
- Consider adding a comment to explain the purpose of the
sandboxparameter in theMailtrapClientconstructor.- Consider adding a comment to explain the structure of the object passed to the
sendmethod.examples/testing/transport.ts (1)
47-51: Consider removing the intentionally invalid HTML/CSS if not needed.The comment suggests that the HTML/CSS in this segment is intentionally invalid for demonstration purposes. If this is not needed, consider removing it to avoid confusion.
Apply this diff to remove the invalid HTML/CSS:
- <!-- Example of invalid for email html/css, will be detected by Mailtrap: --> - <style> - .main { background-color: white; } - a:hover { border-left-width: 1em; min-height: 2em; } - </style>
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (26)
- .github/codeql/codeql-config.yml (1 hunks)
- .github/workflows/test.yml (1 hunks)
- README.md (1 hunks)
- examples/bulk/send-mail.ts (1 hunks)
- examples/bulk/transport.ts (1 hunks)
- examples/general/account-accesses.ts (1 hunks)
- examples/general/accounts.ts (1 hunks)
- examples/general/permissions.ts (1 hunks)
- examples/sending/transport.ts (1 hunks)
- examples/testing/attachments.ts (1 hunks)
- examples/testing/inboxes.ts (1 hunks)
- examples/testing/messages.ts (1 hunks)
- examples/testing/send-mail.ts (1 hunks)
- examples/testing/template.ts (1 hunks)
- examples/testing/transport.ts (1 hunks)
- src/tests/lib/api/Bulk.test.ts (0 hunks)
- src/tests/lib/api/Testing.test.ts (0 hunks)
- src/tests/lib/mailtrap-client.test.ts (4 hunks)
- src/tests/lib/normalizer.test.ts (0 hunks)
- src/config/index.ts (1 hunks)
- src/lib/MailtrapClient.ts (3 hunks)
- src/lib/api/Bulk.ts (0 hunks)
- src/lib/api/Testing.ts (1 hunks)
- src/lib/normalizer.ts (1 hunks)
- src/types/mailtrap.ts (1 hunks)
- src/types/transport.ts (0 hunks)
Files not reviewed due to no reviewable changes (5)
- src/tests/lib/api/Bulk.test.ts
- src/tests/lib/api/Testing.test.ts
- src/tests/lib/normalizer.test.ts
- src/lib/api/Bulk.ts
- src/types/transport.ts
Files skipped from review due to trivial changes (9)
- .github/codeql/codeql-config.yml
- examples/general/account-accesses.ts
- examples/general/accounts.ts
- examples/general/permissions.ts
- examples/sending/transport.ts
- examples/testing/attachments.ts
- examples/testing/inboxes.ts
- examples/testing/messages.ts
- examples/testing/template.ts
Additional context used
Biome
examples/testing/transport.ts
[error] 19-19: expected
,but instead foundsandboxRemove sandbox
(parse)
LanguageTool
README.md
[style] ~124-~124: Consider using a different adverb to strengthen your wording.
Context: ...github.com/vchin). Package version 3 is a completely new package.(COMPLETELY_ENTIRELY)
Additional comments not posted (29)
.github/workflows/test.yml (1)
9-11: LGTM! The CodeQL analysis step is correctly configured.The addition of the CodeQL analysis step enhances the workflow by integrating static code analysis capabilities, allowing for improved code quality checks during the CI/CD process.
The configuration file located at
./.github/codeql/codeql-config.ymlis correctly referenced to customize the CodeQL analysis process for the repository.The past review comment suggesting that mentioning CodeQL may not be necessary is no longer valid. The current code segment is triggering CodeQL analysis from the workflow, so it is appropriate to include this step.
examples/bulk/send-mail.ts (2)
15-15: LGTM!The addition of the
bulk: trueoption to theMailtrapClientconstructor aligns with the PR objective to update the client to accept new parameters for bulk and sandbox modes.
17-17: LGTM!The change in the method call from
client.bulk.sendtoclient.sendaligns with the PR objective to consolidate thesendfunctionality into a single method within theMailtrapClient.src/lib/api/Testing.ts (1)
21-21: Approve the constructor simplification.The removal of the
testInboxIdparameter simplifies the instantiation of theTestingAPIclass, which is a positive change. This aligns with the removal of thesendmethod, as indicated in the AI-generated summary, which likely used thetestInboxId.Verify that any code that previously passed the
testInboxIdargument when creating an instance ofTestingAPIhas been updated to remove this parameter.Run the following script to find any remaining usages of
TestingAPIconstructor withtestInboxIdargument:Verification successful
Verification successful:
TestingAPIconstructor change correctly implementedThe removal of the
testInboxIdparameter from theTestingAPIconstructor has been successfully implemented across the codebase. Our verification process found:
- No instances of
TestingAPIconstructor being called with three or more arguments.- The only usage of the constructor (in
src/lib/MailtrapClient.ts) uses two arguments, consistent with the updated signature.This confirms that the simplification of the
TestingAPIclass instantiation has been properly applied, and no outdated usages remain.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `TestingAPI` constructor is not being called with `testInboxId` argument. # Test: Search for `TestingAPI` constructor usage with `testInboxId` argument. Expect: No results. rg --type typescript $'new TestingAPI\(.*,.*,.*\)'Length of output: 86
Script:
#!/bin/bash # Description: Verify that `TestingAPI` constructor is not being called with `testInboxId` argument. # Test 1: Search for `TestingAPI` constructor usage with three or more arguments using "ts" file type echo "Test 1: Searching for TestingAPI constructor with 3+ arguments (ts file type)" rg --type ts $'new TestingAPI\(.*,.*,.*\)' # Test 2: Search for `TestingAPI` constructor usage with three or more arguments without specifying file type echo "Test 2: Searching for TestingAPI constructor with 3+ arguments (no file type specified)" rg $'new TestingAPI\(.*,.*,.*\)' # Test 3: Search for general `TestingAPI` usage echo "Test 3: Searching for general TestingAPI usage" rg 'TestingAPI' # Test 4: List all TypeScript files in the repository echo "Test 4: Listing all TypeScript files" fd -e ts -e tsxLength of output: 3228
src/config/index.ts (1)
12-12: LGTM!The addition of the
BULK_SANDBOX_INCOMPATIBLEerror message is a valuable improvement to the error handling and user guidance in the application. The message clearly communicates the incompatibility between bulk mode and the sandbox API, helping developers identify and handle such scenarios appropriately.The message is concise, consistent with the existing naming convention, and does not introduce any breaking changes or compatibility issues.
src/types/mailtrap.ts (1)
67-68: LGTM! The addition ofbulkandsandboxproperties enhances the Mailtrap client's configuration options.The inclusion of these optional boolean properties in the
MailtrapClientConfigtype provides more flexibility and control over the Mailtrap client's behavior:
- The
bulkproperty likely indicates whether bulk operations are enabled.- The
sandboxproperty likely indicates whether the client should operate in a sandbox environment.These changes may impact how users interact with the Mailtrap API, as they can now specify bulk and sandbox modes when configuring the client. The changes are backward compatible, as the properties are optional.
src/lib/normalizer.ts (1)
Line range hint
40-48: LGTM! The changes simplify the code and improve type safety.The removal of the
mailtrapClientvariable and the direct usage ofclientfor sending emails streamline the function's flow by eliminating unnecessary conditional logic. This simplification improves code readability and maintainability.Additionally, the explicit typing of the
errorparameter asanyin the.catchblock enhances type safety in TypeScript, helping catch potential type-related issues.examples/bulk/transport.ts (4)
1-13: LGTM!The import statements and constants are correctly defined and used in the file.
15-18: LGTM!The transport object is correctly created with the necessary options for bulk sending.
20-58: LGTM!The email sending logic is correctly implemented with all the necessary components such as text, HTML, and an attachment. The HTML body is well-structured, and the attachment is correctly read from the file system.
59-60: LGTM!The promise handling is correctly implemented using
.thenand.catchmethods. Logging the success and error is useful for debugging and monitoring.examples/testing/transport.ts (2)
22-61: LGTM!The email sending logic is implemented correctly, and the code is well-structured.
61-62: LGTM!The promise handling logic is implemented correctly.
src/lib/MailtrapClient.ts (6)
15-15: LGTM!The import statement for
MailtrapErroris added correctly. It will likely be used for improved error handling in the code changes below.
18-27: LGTM!The constants are added correctly by destructuring the
CLIENT_SETTINGSandERRORSobjects. They will likely be used in the code changes below.
39-41: LGTM!The
bulkandsandboxproperties are added correctly to theMailtrapClientclass. They will likely be used to configure the behavior of theMailtrapClientinstance.
Line range hint
46-97: LGTM!The constructor is updated correctly to accept the new
bulkandsandboxparameters, and the corresponding properties are initialized correctly.The error handling in the
testinggetter is improved by throwingMailtrapErrorinstead of logging warnings, using the correct error messages from theERRORSobject.
99-117: LGTM!The
determineHostmethod is implemented correctly:
- It checks for the incompatible combination of
bulkandsandboxmodes and throws aMailtrapErrorwith the correct error message.- It determines the correct host URL based on the
bulkandsandboxproperties.- It's marked as private, which is appropriate since it's an internal implementation detail.
124-127: LGTM!The
sendmethod is updated correctly to use the newdetermineHostmethod to determine the host URL, which is then correctly interpolated into theurlvariable.README.md (1)
121-124: The existing comment by leonid-shevtsov is still valid and applicable to the current changes. Moving this section to the end of the README would be more appropriate as it is no longer news.Tools
LanguageTool
[style] ~124-~124: Consider using a different adverb to strengthen your wording.
Context: ...github.com/vchin). Package version 3 is a completely new package.(COMPLETELY_ENTIRELY)
src/__tests__/lib/mailtrap-client.test.ts (9)
44-56: LGTM!The test case correctly verifies that the
sendmethod rejects with aMailtrapErrorwhen bothbulkandsandboxmodes are set totrue.
59-93: LGTM!The test case correctly verifies that the
sendmethod successfully sends a testing email whensandboxmode is set totrueand a validtestInboxIdis provided.
95-128: LGTM!The test case correctly verifies that the
sendmethod successfully sends a bulk email whenbulkmode is set totrue.
130-175: LGTM!The test case correctly verifies that the
sendmethod handles an API error correctly when the API returns a 400 response with an error message.
177-212: LGTM!The test case correctly verifies that the
sendmethod handles an HTTP transport error correctly when the API returns a 404 response.
333-344: LGTM!The test case correctly verifies that the
testinggetter rejects with aMailtrapErrorwhentestInboxIdis missing.
347-360: LGTM!The test case correctly verifies that the
testinggetter rejects with aMailtrapErrorwhenaccountIdis missing.
362-373: LGTM!The test case correctly verifies that the
testinggetter returns aTestingAPIinstance when bothtestInboxIdandaccountIdare provided.
376-384: LGTM!The test case correctly verifies that the
generalgetter returns aGeneralAPIinstance.
| const transport = Nodemailer.createTransport(MailtrapTransport({ | ||
| token: TOKEN, | ||
| testInboxId: TEST_INBOX_ID | ||
| sandbox: true |
There was a problem hiding this comment.
Fix the syntax error.
There is a missing comma after the testInboxId property.
Apply this diff to fix the syntax error:
- testInboxId: TEST_INBOX_ID
+ testInboxId: TEST_INBOX_ID,
sandbox: trueCommittable 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.
| sandbox: true | |
| testInboxId: TEST_INBOX_ID, | |
| sandbox: true |
Tools
Biome
[error] 19-19: expected
,but instead foundsandboxRemove sandbox
(parse)
Motivation
MailtrapClientto accept two more params:bulk: Booleanandsandbox: Boolean. They are changing the behavior of the send method of the client in the following way:In the sandbox mode, the
client.sendsends the email to thesandbox, in the bulk mode - to the Bulk API.BulkSendingAPIandTestingAPIclasses. There should be only one send method on theMailtrapClientthat will behave as described above.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests