Retrieve credentials for a single account #14657
Conversation
Accept the `include_credentials` parameter in the `getAccountById` method so that clients can retrieve the credentials of a single account.
jverce
added
bug
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@jverce is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces several changes to the SDK, including a new version entry (1.0.3) in the changelog. Key modifications include an updated method signature for Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (2)
packages/sdk/CHANGELOG.md (1)
8-9: Enhance the changelog entry with more specific details.The changelog entry could be more specific about the new parameter. Consider revising to include:
- The parameter name (
include_credentials)- That it's an optional parameter
- The parameter type (boolean)
- Added a new argument to the `getAccountById` method in the backend client to - allow the client to retrieve the credentials of the corresponding account. + Added an optional boolean parameter `include_credentials` to the `getAccountById` + method in the backend client to allow retrieving account credentials when needed.packages/sdk/src/server/__tests__/server.test.ts (1)
356-383: Test implementation looks good, but coverage could be improved.The test case effectively verifies the basic functionality of the
include_credentialsflag. However, consider adding the following test cases for better coverage:
- Edge cases for credentials field (null/undefined)
- Error scenarios (e.g., unauthorized access to credentials)
Here's a suggested implementation of additional test cases:
it("should handle null credentials in response", async () => { fetchMock.mockResponseOnce( JSON.stringify({ id: "account-1", name: "Test Account", credentials: null, }), { headers: { "Content-Type": "application/json", }, }, ); const result = await client.getAccountById("account-1", { include_credentials: true, }); expect(result.credentials).toBeNull(); }); it("should handle unauthorized access to credentials", async () => { fetchMock.mockResponseOnce( JSON.stringify({ error: "Unauthorized access to credentials", }), { status: 403, headers: { "Content-Type": "application/json", }, }, ); await expect( client.getAccountById("account-1", { include_credentials: true, }) ).rejects.toThrow("HTTP error! status: 403"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/server/__tests__/server.test.ts(1 hunks)packages/sdk/src/server/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/package.json
🔇 Additional comments (2)
packages/sdk/CHANGELOG.md (1)
4-4: Verify version release dates.
Both versions 1.0.2 and 1.0.3 are dated 2024-11-14. Please verify if these versions should be released on the same date or if one of the dates needs to be adjusted.
Also applies to: 10-10
✅ Verification successful
Based on the git history output, I can now generate a final response:
Both version dates are correct - these are separate releases on the same day
The git history confirms that both version 1.0.2 and 1.0.3 were legitimately released on 2024-11-14:
- Version 1.0.2 introduced changes related to deprecating
environment_nameand addingfullResponseflag - Version 1.0.3 added new functionality to
getAccountByIdmethod
Having multiple releases on the same day is valid when different features or changes need to be published separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check git history for version bumps to verify dates
# Look for version number changes in package.json
git log -p -- "packages/sdk/package.json" | grep -A 5 -B 5 "version"
# Look for previous changelog entries
git log -p -- "packages/sdk/CHANGELOG.md" | grep -A 5 -B 5 "\[1\.0\."
Length of output: 1848
packages/sdk/src/server/index.ts (1)
207-215: LGTM! Clean type definition with clear documentation.
The new GetAccountByIdOpts type is well-structured and consistent with the codebase's patterns.
| public getAccountById( | ||
| accountId: string, | ||
| params: GetAccountByIdOpts = {}, | ||
| ): Promise<Account> { | ||
| return this.makeConnectRequest(`/accounts/${accountId}`, { | ||
| method: "GET", | ||
| params, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update method documentation to reflect the new parameter.
The method implementation looks good, but the JSDoc comments need to be updated to:
- Document the new
paramsparameter - Add an example showing how to use
include_credentials
Here's the suggested documentation update:
/**
* Retrieves a specific account by ID.
*
* @param accountId - The ID of the account to retrieve.
+ * @param params - Options for retrieving the account.
+ * @param params.include_credentials - Whether to include the account's credentials in the response.
* @returns A promise resolving to the account.
*
* @example
* ```typescript
* const account = await client.getAccountById("account-id");
* console.log(account);
* ```
+ *
+ * @example
+ * ```typescript
+ * // Retrieve account with credentials
+ * const account = await client.getAccountById("account-id", { include_credentials: true });
+ * console.log(account.credentials);
+ * ```
*/
Accept the `include_credentials` parameter in the `getAccountById` method so that clients can retrieve the credentials of a single account.
Description
Accept the
include_credentialsparameter in thegetAccountByIdmethod so that clients can retrieve the credentials of a single account.Summary by CodeRabbit
New Features
getAccountByIdmethod to include associated credentials in the response.fullResponse, toRequestOptionsfor receiving complete HTTP response details.Bug Fixes
environment_namefield, streamlining environment determination.Documentation
Tests
getAccountByIdmethod, including scenarios for credential retrieval.