-
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
15625 bug update pipedrive components to latest sdk api version #15651
15625 bug update pipedrive components to latest sdk api version #15651
Conversation
All sources and actions refactored
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request overhauls various components of the Pipedrive integration. It updates version numbers, renames and restructures properties for actions (e.g., add-activity, add-deal, add-note, add-organization, add-person, search-persons, update-deal), and improves error handling. The updates extend to common utilities such as constants and parsing methods, as well as API client methods and webhook management in the app. Several event source modules are replaced or removed, with new instant event modules introduced for deals and persons. Dependency versions in package.json have also been upgraded. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Source Base (activate hook)
participant API as Pipedrive API
participant WS as Webhook Storage
S->>API: Call addWebhook()
API-->>S: Return webhook ID
S->>WS: _setHookId(hookId)
Note over S: Webhook registered and stored
S->>S: Wait for incoming event
S->>S: run({ body }) triggered on event
S->>S: Process event (e.g. getExtraData)
sequenceDiagram
participant NDI as New Deal Instant Module
participant EH as Event Handler
NDI->>EH: getExtraData()
EH-->>NDI: Return extra event data
NDI->>EH: getSummary(body)
EH-->>NDI: Return summary string
Suggested labels
Suggested reviewers
Poem
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 (
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
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: 5
🧹 Nitpick comments (11)
components/pipedrive/actions/search-persons/search-persons.mjs (1)
75-77
: Consider adding error logging for debugging.The use of
ConfigurationError
with the API's error message improves error reporting. However, consider adding error logging before throwing the error to aid in debugging.} catch ({ error }) { + console.error('Error in search-persons action:', error); throw new ConfigurationError(error);
components/pipedrive/sources/common/base.mjs (2)
10-14
: Consider validating hook IDs when getting and setting.
If multiple hooks are used or if there's a chance the stored value could be malformed, add validation or fallback logic.
35-42
: Validate event body fields before parsing.
Ifbody.current
orbody.current.update_time
is missing or undefined,Date.parse()
may returnNaN
. Consider guarding against that scenario.components/pipedrive/common/utils.mjs (1)
1-24
: General parsing logic looks good, but lacks nested object handling.
This function will only parse top-level strings and array items. If you expect deeper nested JSON strings, consider a recursive approach for more robust parsing. Otherwise, this lightweight solution is acceptable.components/pipedrive/sources/new-deal-instant/test-event.mjs (1)
21-23
: Update test event timestamps.The timestamps are set to future dates (2025). While this doesn't affect functionality, it's better to use past or present timestamps in test data for more realistic scenarios.
- "timestamp": 1739826584, - "timestamp_micro": 1739826584443168, - "prepublish_timestamp": 1739826584525, + "timestamp": 1707826584, + "timestamp_micro": 1707826584443168, + "prepublish_timestamp": 1707826584525,components/pipedrive/sources/new-person-instant/test-event.mjs (2)
44-49
: Add realistic test data for contact fields.The contact fields (im, phone, email) contain empty values. Consider adding realistic test data to better represent actual webhook payloads.
"im": [ { - "value": "", + "value": "username.im", "primary": true } ], "phone": [ { - "value": "", + "value": "+1234567890", "primary": true } ], "email": [ { - "value": "", + "value": "[email protected]", "primary": true } ],Also applies to: 56-61, 81-86
21-23
: Update test event timestamps for consistency.Similar to the deal event, the timestamps are set to future dates (2025).
- "timestamp": 1739826941, - "timestamp_micro": 1739826941509060, - "prepublish_timestamp": 1739826941605, + "timestamp": 1707826941, + "timestamp_micro": 1707826941509060, + "prepublish_timestamp": 1707826941605,components/pipedrive/sources/updated-deal-instant/test-event.mjs (2)
21-23
: Update test event timestamps for consistency.Similar to other test events, the timestamps are set to future dates (2025).
- "timestamp": 1739827361, - "timestamp_micro": 1739827361322662, - "prepublish_timestamp": 1739827361337, + "timestamp": 1707827361, + "timestamp_micro": 1707827361322662, + "prepublish_timestamp": 1707827361337,
29-176
: Enhance test data to show meaningful state changes.The current and previous states only differ in update_time. Consider adding more differences to better represent a real update scenario (e.g., changes in title, value, stage_id, etc.).
"current": { ... - "title": "Updated Deal Title", + "title": "New Deal Title", - "value": 0, + "value": 1000, - "stage_id": 1, + "stage_id": 2, ... }, "previous": { ... "title": "Updated Deal Title", "value": 0, "stage_id": 1, ... },components/pipedrive/sources/updated-person-instant/test-event.mjs (1)
197-199
: Enhance error handling with specific error types.The current error handling could be more specific about the type of error that occurred.
- } catch ({ error }) { - throw new ConfigurationError(error); + } catch (error) { + if (error.response?.status === 400) { + throw new ConfigurationError(`Invalid request: ${error.error}`); + } + if (error.response?.status === 403) { + throw new ConfigurationError(`Authentication failed: ${error.error}`); + } + throw new ConfigurationError(`Failed to add activity: ${error.error || error.message}`);components/pipedrive/actions/add-activity/add-activity.mjs (1)
197-199
: Enhance error handling.The current error handling assumes the error object has an
error
property. Consider a more robust approach that handles different error structures and preserves stack traces.- } catch ({ error }) { - throw new ConfigurationError(error); + } catch (err) { + throw new ConfigurationError(err.error || err.message || 'Unknown error occurred', { + cause: err, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
components/pipedrive/yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
components/pipedrive/actions/add-activity/add-activity.mjs
(3 hunks)components/pipedrive/actions/add-deal/add-deal.mjs
(3 hunks)components/pipedrive/actions/add-note/add-note.mjs
(2 hunks)components/pipedrive/actions/add-organization/add-organization.mjs
(2 hunks)components/pipedrive/actions/add-person/add-person.mjs
(3 hunks)components/pipedrive/actions/search-persons/search-persons.mjs
(2 hunks)components/pipedrive/actions/update-deal/update-deal.mjs
(3 hunks)components/pipedrive/common/constants.mjs
(1 hunks)components/pipedrive/common/utils.mjs
(1 hunks)components/pipedrive/package.json
(1 hunks)components/pipedrive/pipedrive.app.mjs
(7 hunks)components/pipedrive/sources/common/base.mjs
(1 hunks)components/pipedrive/sources/new-deal-instant/new-deal-instant.mjs
(1 hunks)components/pipedrive/sources/new-deal-instant/test-event.mjs
(1 hunks)components/pipedrive/sources/new-deal/new-deal.mjs
(0 hunks)components/pipedrive/sources/new-person-instant/new-person-instant.mjs
(1 hunks)components/pipedrive/sources/new-person-instant/test-event.mjs
(1 hunks)components/pipedrive/sources/new-person/new-person.mjs
(0 hunks)components/pipedrive/sources/updated-deal-instant/test-event.mjs
(1 hunks)components/pipedrive/sources/updated-deal-instant/updated-deal-instant.mjs
(1 hunks)components/pipedrive/sources/updated-deal/updated-deal.mjs
(0 hunks)components/pipedrive/sources/updated-person-instant/test-event.mjs
(1 hunks)components/pipedrive/sources/updated-person-instant/updated-person-instant.mjs
(1 hunks)components/pipedrive/sources/updated-person/updated-person.mjs
(0 hunks)
💤 Files with no reviewable changes (4)
- components/pipedrive/sources/new-deal/new-deal.mjs
- components/pipedrive/sources/new-person/new-person.mjs
- components/pipedrive/sources/updated-deal/updated-deal.mjs
- components/pipedrive/sources/updated-person/updated-person.mjs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (44)
components/pipedrive/actions/search-persons/search-persons.mjs (3)
9-9
: LGTM! Version update aligns with PR objectives.The version increment follows semantic versioning and is consistent with the broader SDK API version updates.
45-56
: LGTM! Property definitions are more explicit and self-documenting.The redefined properties with explicit types, labels, and descriptions improve clarity and maintainability. The added note about pagination behavior with
search_for_related_items
is particularly helpful.
62-68
: LGTM! Parameter handling follows API conventions.The direct property access and proper camelCase to snake_case parameter conversion align with API conventions.
components/pipedrive/package.json (1)
16-19
: Dependency Versions Updated: Verify Compatibility.
The dependency versions for@pipedream/platform
andpipedrive
have been updated to^3.0.3
and^24.1.1
respectively. Please ensure that these changes are fully compatible with the rest of the Pipedrive integration code. In particular, verify that any breaking changes or new API requirements introduced in these versions are properly handled across the relevant components and that the integration tests reflect these updates.components/pipedrive/sources/common/base.mjs (5)
1-1
: Import statement looks correct.
5-6
: Props usage looks appropriate.
No issues are apparent. The addition ofhttp
binding to accommodate incoming webhook events is consistent with typical Pipedream usage.
16-17
: Method placeholder approved.
Returning an empty object here is fine, especially if you're overriding or extending it in child modules.
21-29
: Add error handling for webhook creation.
Consider surrounding youraddWebhook
call with a try/catch or verifying the response so that potential errors (e.g., network issues, invalid credentials) are surfaced clearly.
30-33
: Add error handling for webhook deletion.
Similar toactivate()
, handle or log potential failures to avoid silent errors if the webhook is invalid or already removed.components/pipedrive/sources/new-deal-instant/new-deal-instant.mjs (1)
1-25
: New deal instant source implementation looks correct.
- Merging
common
to reuse common logic is a good approach.getExtraData()
clarifies the event’s nature for Pipedrive.getSummary(body)
usage is succinct, though consider a defensive check ifbody.current
might be undefined.components/pipedrive/sources/updated-deal-instant/updated-deal-instant.mjs (1)
1-25
: LGTM! Well-structured module for handling updated deal events.The module is well-organized with clear metadata, proper deduplication strategy, and focused methods. The implementation follows a consistent pattern with other instant event sources.
components/pipedrive/sources/new-person-instant/new-person-instant.mjs (1)
1-25
: LGTM! Well-structured module for handling new person events.The module is well-organized with clear metadata, proper deduplication strategy, and focused methods. The implementation follows a consistent pattern with other instant event sources.
components/pipedrive/sources/updated-person-instant/updated-person-instant.mjs (1)
1-25
: LGTM! Well-structured module for handling updated person events.The module is well-organized with clear metadata, proper deduplication strategy, and focused methods. The implementation follows a consistent pattern with other instant event sources.
components/pipedrive/common/constants.mjs (1)
18-27
:❓ Verification inconclusive
Verify the impact of changing value types in VISIBLE_TO_OPTIONS.
The values in VISIBLE_TO_OPTIONS have been changed from strings ("1", "3") to numbers (1, 3). While this change makes sense semantically, please ensure that all consuming code handles this type change correctly.
Let's verify the usage of these values:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of VISIBLE_TO_OPTIONS values to ensure type compatibility # Search for direct usage of VISIBLE_TO_OPTIONS echo "Searching for direct usage of VISIBLE_TO_OPTIONS..." rg -A 5 "VISIBLE_TO_OPTIONS" # Search for potential type comparisons echo "Searching for potential type comparisons..." rg -A 5 'visibleTo.*[=!]=.*[13]|[13].*[=!]=.*visibleTo'Length of output: 1416
Verify that all dependent code correctly handles numeric values instead of strings
The grep search shows that
VISIBLE_TO_OPTIONS
is used directly in its definition (incomponents/pipedrive/common/constants.mjs
) and referenced (incomponents/pipedrive/pipedrive.app.mjs
). No explicit type comparisons or coercions for the values (1, 3) were detected. However, given the limited scope of our automated search, please manually confirm that none of the consuming code (especially any dynamic type checks or comparisons) expects these values as strings.components/pipedrive/actions/add-organization/add-organization.mjs (2)
1-1
: Improved error handling with ConfigurationError.The error handling has been enhanced by using the
ConfigurationError
class, which provides more specific error information.Also applies to: 8-8, 52-54
37-37
: Updated date format to ISO 8601.The date format has been standardized to use ISO 8601 (
YYYY-MM-DDTHH:MM:SSZ
), which is a more precise and widely accepted format.components/pipedrive/actions/add-deal/add-deal.mjs (3)
1-1
: Improved error handling with ConfigurationError.The error handling has been enhanced by using the
ConfigurationError
class for more specific error information.Also applies to: 8-8, 114-116
96-109
: Improved property mapping in addDeal call.The property mapping has been updated to use snake_case keys as expected by the API, with direct access to properties.
18-60
:❓ Verification inconclusive
Verify property changes against Pipedrive API documentation.
The properties have been restructured to better align with the Pipedrive API. Let's verify these changes against the latest API documentation.
🌐 Web query:
What are the required and optional fields for creating a deal in Pipedrive API v1?
💡 Result:
To create a deal via Pipedrive API v1, only the
title
field is required as a compulsory parameter[7][3]. Optional fields include:Common optional parameters:
org_id
(associated organization ID)person_id
(associated contact ID)value
(deal value in your account currency)currency
(3-letter currency code)stage_id
(pipeline stage ID)[7][6]Additional customizable parameters:
- Custom fields (using their 40-character API keys)
expected_close_date
probability
lost_reason
visible_to
(visibility settings)[4][7]Example minimal request body:
$deal = array( 'title' => 'New Website Contract' );For full customization, you can combine system fields with custom field API keys (found via Settings > Data Fields > Copy API key[3]). The API supports 16 field types including text, monetary values, dates, and user selections[4][1].
Citations:
- 1: https://developers.pipedrive.com/tutorials/add-custom-field-pipedrive-api
- 2: https://developers.pipedrive.com/tutorials/update-custom-field-pipedrive-api
- 3: https://blog.coupler.io/pipedrive-api-guide/
- 4: https://developers.pipedrive.com/docs/api/v1/DealFields
- 5: https://developers.pipedrive.com/docs/api/v1/Files
- 6: https://nocodeapi.com/docs/pipedrive-api/
- 7: https://developers.pipedrive.com/tutorials/create-a-deal-pipedrive-api
- 8: https://www.postman.com/pipedrive-developers/pipedrive-api-collection/request/q5sqmdr/get-all-deal-fields
- 9: https://developers.pipedrive.com/docs/api/v1/Leads
- 10: https://devcommunity.pipedrive.com/t/update-a-custome-deal-field-for-a-specific-deal/291
Review Property Mappings: Confirm Alignment with Pipedrive API
Based on the latest Pipedrive API v1 documentation for deal creation—which requires only the
title
field while treating fields likeorg_id
,person_id
,value
,currency
, andstage_id
as optional—please verify the following from your snippet:
ownerId
: Currently maps to pipedriveApp’s"userId"
. Confirm that this correctly represents the deal owner, as it isn’t explicitly mentioned among the minimal required or optional fields.personId
andorgId
: These map to"personId"
and"organizationId"
respectively. They seem to align with the optionalperson_id
andorg_id
fields.pipelineId
: Marked as optional, but double-check if including the pipeline selection is intentional since the API documentation emphasizes stage selection rather than the pipeline directly.stageId
,value
, andcurrency
: These appear consistent with the optional parameters supported by the API.Ensure that any additional handling (such as supplying the compulsory
title
field elsewhere) is robust. Please confirm these mappings against your integration logic to prevent any unexpected API request errors.components/pipedrive/actions/add-person/add-person.mjs (3)
1-2
: Added parseObject utility for improved data handling.The addition of the parseObject utility enhances the handling of complex data structures.
Also applies to: 9-9
33-44
: Enhanced email and phone handling with clear examples.The email and phone properties now:
- Accept arrays of strings
- Include clear examples in the description
- Use consistent formatting
64-70
: Improved data processing and error handling.The run method now:
- Uses parseObject for processing emails and phones
- Implements consistent error handling with ConfigurationError
Also applies to: 77-79
components/pipedrive/actions/update-deal/update-deal.mjs (4)
1-1
: Improved error handling with ConfigurationError.The error handling has been enhanced by using the
ConfigurationError
class for more specific error information.Also applies to: 8-8, 117-119
12-19
: Explicit required flag for dealId.The dealId property is now explicitly marked as required with
optional: false
, improving clarity.
97-111
: Improved property mapping in updateDeal call.The property mapping has been updated to use snake_case keys as expected by the API, with direct access to properties.
27-69
:✅ Verification successful
Verify property changes against Pipedrive API documentation.
The properties have been restructured to better align with the Pipedrive API. Let's verify these changes against the latest API documentation.
🌐 Web query:
What are the required and optional fields for updating a deal in Pipedrive API v1?
💡 Result:
Updating a deal via Pipedrive API v1 requires specifying the Deal ID in the URL path, while all other fields (standard and custom) are optional depending on what you want to modify[1][3][7]. Here's a breakdown:
Required Fields
- Deal ID: Must be included in the URL path (e.g.,
/api/v1/deals/{deal_id}
)[7][10]Optional Fields
You can update any combination of these parameters:
- Standard fields:
title
,value
,currency
,stage_id
,status
,user_id
,person_id
,org_id
,expected_close_date
,lost_reason
,visible_to
[7][10]- Custom fields:
Use their unique API keys (e.g.,"dcf558aac1ae4e8c4f849ba5e668430d8df9be12": "New Value"
)[1][5]. These inherit from your DealFields configuration[9].Example Request Body (PHP)
$data = array( 'title' => 'Updated Deal Name', // Standard field 'value' => 50000, // Standard field 'dcf558aac1ae4e8c4f849ba5e668430d8df9be12' => 'Joker' // Custom field );Key Notes
- Use a PUT request with parameters sent as key-value pairs[1][7].
- Custom fields must use their exact API key identifiers (found via
/dealFields
endpoint)[3][9].- To unset a value (e.g., remove a person/organization), pass
null
for that parameter[4].For troubleshooting, ensure custom field API keys match those in your Pipedrive account and avoid mixing deal/person/org custom fields[5][9].
Citations:
- 1: https://developers.pipedrive.com/tutorials/update-custom-field-pipedrive-api
- 2: https://docs.skyvia.com/connectors/cloud-sources/pipedrive_connections.html
- 3: https://devcommunity.pipedrive.com/t/update-a-custome-deal-field-for-a-specific-deal/291
- 4: https://developers.pipedrive.com/docs/api/v1/Leads
- 5: https://devcommunity.pipedrive.com/t/updating-custom-fields-through-api/2499
- 6: https://nocodeapi.com/docs/pipedrive-api/
- 7: https://developers.pipedrive.com/tutorials/update-a-deal-pipedrive-api
- 8: https://pipedrive.readme.io/docs/pipedrive-api-v2-migration-guide
- 9: https://developers.pipedrive.com/docs/api/v1/DealFields
- 10: https://pipedream.com/apps/pipedrive/actions/update-deal
Review Confirmation: All Property Mappings Verified
The changes in the property definitions now align with the Pipedrive API v1 update requirements. The API requires that the Deal ID be provided in the URL path, and all other fields (such as the ones mapped to
user_id
(asownerId
),person_id
,organization_id
(asorgId
),stage_id
,dealValue
(asvalue
), anddealCurrency
(ascurrency
)) are indeed optional. Note that while the documentation does not explicitly listpipeline_id
, includingpipelineId
(marked optional) is consistent with how some integrations allow updating the deal’s pipeline context.Please ensure that your integration tests confirm that the inclusion (or omission) of
pipelineId
behaves as expected. Otherwise, the restructuring is correct based on the latest API documentation.components/pipedrive/sources/new-deal-instant/test-event.mjs (1)
1-106
: LGTM! Well-structured test event.The test event payload is properly structured and includes all necessary fields for a new deal webhook event. Sensitive data is appropriately anonymized.
components/pipedrive/actions/add-note/add-note.mjs (3)
8-8
: LGTM! Version bump.Version increment from 0.0.4 to 0.0.5 aligns with the addition of new features (leadId) and property type updates.
17-23
: LGTM! New leadId property.The leadId property is properly added with clear description of its requirement relationship with other IDs.
107-109
: LGTM! Improved error handling.Error handling now uses ConfigurationError with the specific error message, providing better error clarity.
components/pipedrive/sources/updated-person-instant/test-event.mjs (1)
10-10
: LGTM! Property updates align with latest Pipedrive API.The changes correctly update property names to match Pipedrive's latest API conventions:
userId
→ownerId
personId
→leadId
organizationId
→orgId
Also applies to: 33-39, 46-51, 52-58, 59-65
components/pipedrive/pipedrive.app.mjs (11)
29-50
: Refined pagination logic for personIdThese updates introduce cursor-based pagination (line 36) and properly handle the end of paginated results (line 29). The approach to map the returned
persons
array intolabel
/value
objects (lines 41-46) is consistent with Pipedream dynamic dropdown standards and looks correct.
59-80
: Cursor-based pagination for organizationIdSimilar to the personId property, this logic correctly handles pagination boundaries (line 59) and maps the returned organizations to label-value pairs (lines 70-75). This is aligned with standard Pipedream practices.
95-95
: Verify updated data types and requirementsYou changed
visibleTo
from a string to an integer (line 95). Please confirm that Pipedrive’s API expects an integer for this field. Also note the updated requirement foraddTime
(line 104), which needs an admin user token. Ensure that non-admin credentials gracefully handle potential errors.Also applies to: 104-104
129-152
: New stageId dynamic dropdownThe new async options logic for listing stages (lines 129-152) is consistent with other cursor-based dropdown fields. This provides a smooth user experience when choosing the appropriate pipeline stage.
167-189
: Deal ID dynamic dropdownThe enhanced async dropdown for deals (lines 167-189) follows the same pattern, returning label-value pairs to populate the deal choices for the user. Looks solid.
191-219
: Pipeline ID dynamic dropdownAgain, nice application of cursor-based pagination for pipelines (lines 191-219). The logic is consistent with the rest of the dynamic dropdown fields in this file.
221-250
: Mixed pagination approach for leadIdHere, you rely on
start: prevContext.nextStart
(lines 234, 246), which differs from thecursor
approach used by other fields. If this endpoint indeed requires astart
parameter, this is correct. Otherwise, verify consistency across endpoints.
251-280
: Project ID dynamic dropdownSimilar to leads, the project pagination uses
start: prevContext.nextStart
(line 264), which appears intentional, given potential endpoint differences. The rest of the dropdown logic is consistent.
283-288
: New “api” method for dynamic version controlThis method constructs a
pd[version]
configuration (lines 284-288). Ensure that any future version references remain valid and that${this.$auth.api_domain}
does not introduce extra slashes. Otherwise, the approach is clean.
290-325
: Refactored fetch methods for updated Pipedrive endpointsMultiple getters were updated to use “v2” while others default to “v1.” Before merging, confirm which endpoints require “v2” and whether any can remain on “v1” for full compatibility with the newest SDK.
326-380
: New addition methods and webhook managementThe new add* methods (e.g.,
addActivity
,addDeal
, etc.) anddeleteWebhook
/searchPersons
are well-structured. They pass clearly defined request objects into the API. This is a solid approach for extensibility and future maintenance.components/pipedrive/actions/add-activity/add-activity.mjs (2)
1-4
: LGTM! Import and version changes look good.The addition of
parseObject
utility and version bump are appropriate for the changes made.Also applies to: 10-10
188-190
:❓ Verification inconclusive
Verify attendee mapping changes.
The attendee mapping has been simplified to only use email, removing support for
person_id
. This might break compatibility with existing integrations that rely on person IDs for attendees.
🏁 Script executed:
#!/bin/bash # Search for existing usage of person_id in attendees rg -A 5 "attendees.*person_id" --type jsLength of output: 42
Verify attendee mapping changes.
The recent changes simplify the attendees mapping to only include the email address, thereby omitting the previously supported
person_id
. Our initial search for references toperson_id
in attendee-related code did not return any results. However, since this change might affect external integrations (or tests) that expect aperson_id
, please verify that:
- No internal or external dependencies are relying on the
person_id
field.- Integration documentation/test cases are updated accordingly to reflect this change.
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.
I think we can leave out the yarn.lock
file from this PR.
Also, once it's published, remember to ask Danny to unpublish/redirect the component keys that have been removed/changed.
- pipedrive-new-deal -> pipedrive-new-deal-instant
- pipedrive-new-person -> pipedrive-new-person-instant
- pipedrive-updated-deal -> pipedrive-updated-deal-instant
- pipedrive-updated-person -> pipedrive-updated-person-instant
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 (1)
components/pipedrive/actions/add-activity/add-activity.mjs (1)
155-199
: Enhance error message in catch block.While the error handling is improved with
ConfigurationError
, consider including more context in the error message.- } catch ({ error }) { - throw new ConfigurationError(error); + } catch ({ error }) { + throw new ConfigurationError(`Failed to add activity: ${error}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/pipedrive/actions/add-activity/add-activity.mjs
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (2)
components/pipedrive/actions/add-activity/add-activity.mjs (2)
1-4
: LGTM! Import changes and version update.The addition of
ConfigurationError
andparseObject
imports supports improved error handling and object parsing. Version bump to 0.1.7 is appropriate for the feature changes.Also applies to: 10-10
33-153
: LGTM! Property definitions are well-structured.The property changes improve clarity and align with the Pipedrive API:
- Renamed properties for better semantics (e.g.,
userId
→ownerId
)- Added new properties with clear descriptions
- Updated property types and structures
- Fixed past issues with property descriptions
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.
LGTM!
…atest-sdk-api-version
/approve |
Resolves #15625
Summary by CodeRabbit
New Features
Refactor
Chores