-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Gong - Get Extensive Data Action #16106
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request adds a new module to the Gong application that retrieves extensive call data. It introduces an action with properties such as Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as Gong Action
participant P as app.paginate
participant G as Gong API
U->>A: Trigger getExtensiveData with parameters
A->>P: Call app.paginate with filters
P->>G: Send POST request for extensive call data
G-->>P: Return paginated call data
P-->>A: Provide results
A->>U: Return summary message (number of calls retrieved)
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
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: 0
🧹 Nitpick comments (4)
components/gong/actions/get-extensive-data/get-extensive-data.mjs (4)
57-64
: Consider adding error handling to the API method.The
getExtensiveData
method is a simple wrapper around the app's POST method, but it lacks explicit error handling.getExtensiveData(args = {}) { - return this.app.post({ + return this.app.post({ path: "/calls/extensive", ...args, }); },You might want to wrap this with try/catch in the calling code to handle potential API errors.
65-84
: Consider validating input parameters.The run method doesn't validate the relationship between time-related parameters. For example, there's no check to ensure that
fromDateTime
is beforetoDateTime
.async run({ $ }) { const { app, getExtensiveData, maxResults, + fromDateTime, + toDateTime, ...filter } = this; + // Validate date parameters if provided + if (fromDateTime && toDateTime && new Date(fromDateTime) > new Date(toDateTime)) { + throw new Error("fromDateTime must be before toDateTime"); + } const calls = await app.paginate({ resourceFn: getExtensiveData, resourceFnArgs: { $, data: { filter, }, }, resourceName: "calls", max: maxResults, });
85-89
: Add explicit error handling.The current implementation checks if calls exist but doesn't explicitly handle potential errors from the API call.
if (calls?.length) { $.export("$summary", `Successfully retrieved data for ${calls.length} calls`); + } else { + $.export("$summary", "No calls found or no data returned"); } return calls;
1-91
: Add a note about testing limitations.The PR description mentions that this implementation hasn't been tested due to the absence of required scope. Consider adding a code comment acknowledging this limitation.
import app from "../../gong.app.mjs"; import constants from "../../common/constants.mjs"; +// Note: This action requires the 'api:calls:read:extensive' scope to function correctly. +// As of implementation, testing has been limited due to scope availability constraints. export default { key: "gong-get-extensive-data",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
components/gong/actions/get-extensive-data/get-extensive-data.mjs
(1 hunks)components/gong/package.json
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (4)
components/gong/package.json (1)
3-3
: Version bump is appropriate for new feature addition.The version change from 0.1.2 to 0.2.0 correctly follows semantic versioning principles, reflecting the addition of a new feature (the extensive data retrieval action).
components/gong/actions/get-extensive-data/get-extensive-data.mjs (3)
4-9
: Action metadata looks good.The metadata properly defines the action with appropriate key, name, description, version, and type. The documentation link helps users understand the API endpoint.
10-56
: Props structure is well-defined.The props are properly structured with appropriate types, labels, descriptions, and optional flags. The reuse of existing propDefinitions helps maintain consistency across the application.
38-48
: Verify the retrieval of Primary User IDs.The
primaryUserIds
prop uses the app'suserId
propDefinition, but it's unclear if this will correctly map to what the API expects as "primary user identifiers". Consider verifying this with the API documentation.Could you confirm that the
userId
propDefinition returns values that match what the Gong API expects for theprimaryUserIds
parameter? The description mentions these should match theprimaryUserId
field of calls.
Resolves #16062
Note: This hasn't been tested because we're lacking the scope:
api:calls:read:extensive
.Summary by CodeRabbit
New Features
Chores