-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Zoho CRM - support picklist fields #14974
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe pull request introduces several modifications across multiple files within the Zoho CRM component. Key changes include an updated method signature for Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)components/zoho_crm/actions/common/common-objects.mjs (1)
The implementation correctly maps picklist values to a structured format with value/label pairs, which is essential for proper dropdown rendering in the UI. components/zoho_crm/actions/update-object/update-object.mjs (2)
The version bump and ConfigurationError import are appropriate given the new functionality and error handling. Also applies to: 10-10
The error handling has been enhanced with specific error messages for different scenarios:
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 (
|
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
🧹 Outside diff range and nitpick comments (1)
components/zoho_crm/actions/common/common-objects.mjs (1)
166-183
: RefineparseFields
to avoid unintended parsing of string valuesThe
parseFields
method currently attempts to parse all string values infields
, which might unintentionally parse regular string fields that are not JSON. Consider modifying the method to only parse fields known to contain JSON strings to prevent potential data issues.Apply this diff to refine the
parseFields
method:def parseFields(fields) { if (!fields) { return; } for (const [ key, value, ] of Object.entries(fields)) { try { - if (typeof value === "string") { + if (this.isJsonField(key) && typeof value === "string") { fields[key] = JSON.parse(value); } } catch { fields[key] = value; } } return fields; } +// Helper method to determine if a field is expected to be a JSON string +isJsonField(fieldName) { + const jsonFields = [ + // Add field names that are expected to contain JSON strings + "jsonField1", + "jsonField2", + ]; + return jsonFields.includes(fieldName); +}You can define an
isJsonField
helper method to check if a field should be parsed, preventing unintended parsing of plain string fields.
📜 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 (4)
components/zoho_crm/actions/common/common-objects.mjs
(2 hunks)components/zoho_crm/actions/create-object/create-object.mjs
(2 hunks)components/zoho_crm/actions/update-object/update-object.mjs
(2 hunks)components/zoho_crm/package.json
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/zoho_crm/package.json
🔇 Additional comments (5)
components/zoho_crm/actions/common/common-objects.mjs (1)
155-162
: Properly map picklist values to options
The mapping of pick_list_values
to the options
property within getOptionalProps
is correctly implemented. This will effectively populate the options for fields with picklist values.
components/zoho_crm/actions/update-object/update-object.mjs (2)
9-9
: Version updated to "0.3.3"
The version bump from "0.3.2" to "0.3.3" is appropriate given the introduced changes.
64-64
: Ensure parseFields
usage aligns with data expectations
The object
is now processed through this.parseFields(object)
before being passed to zohoCrm.updateObject
. Verify that this transformation is compatible with the expected data format and that it does not introduce unintended consequences due to parsing, especially in light of the concerns raised about parseFields
.
Run the following script to test the integrity of the data after parsing:
Ensure that the parsed object maintains the correct data types and values.
✅ Verification successful
parseFields
implementation is safe and appropriate for this usage
The parseFields
method implementation in common-objects.mjs
is designed to handle string-to-JSON conversion for field values, with proper error handling. It:
- Only attempts to parse string values that might be JSON
- Preserves the original value if parsing fails
- Maintains non-string values as-is
- Returns undefined if no fields are provided
Its usage in update-object.mjs
is appropriate as it ensures any stringified JSON fields are properly converted before being sent to the Zoho CRM API, preventing potential data format issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the parsed object fields retain expected data types.
# Assuming you have test cases or sample data
# Output the object before and after parsing to compare
echo "Original object:"
cat sample_object.json
echo "Parsed object:"
node -e '
const fs = require("fs");
const common = require("./components/zoho_crm/actions/common/common-objects.mjs");
const object = JSON.parse(fs.readFileSync("sample_object.json", "utf8"));
console.log(common.parseFields(object));
'
Length of output: 959
Script:
#!/bin/bash
# Let's examine the parseFields implementation and its usage
ast-grep --pattern 'parseFields($$$)' -l javascript
# Also check the content of the common-objects.mjs file
cat components/zoho_crm/actions/common/common-objects.mjs
# And examine the update-object.mjs file for context
cat components/zoho_crm/actions/update-object/update-object.mjs
Length of output: 6803
components/zoho_crm/actions/create-object/create-object.mjs (2)
8-8
: Version updated to "0.3.3"
The version increment from "0.3.2" to "0.3.3" reflects the updates made in the action.
56-56
: Validate the use of parseFields
when creating objects
The object
is now being processed with this.parseFields(object)
before the API call in zohoCrm.createObject
. Please ensure that this change is appropriate and does not lead to unintended data parsing issues, considering the earlier suggestion to refine parseFields
.
Run the following script to confirm that object creation works as expected with the parsed fields:
Check that the parsedObject
contains the correct data and types before proceeding with the API call.
✅ Verification successful
parseFields
implementation is safe and appropriate for object creation
The implementation of parseFields
in common-objects.mjs
is straightforward and safe. It attempts to parse string values as JSON only if they are actually strings, preserving the original value if parsing fails. This is appropriate for the create object operation as it:
- Handles potential JSON strings that might be passed as field values
- Preserves non-string values as-is
- Maintains the original value if JSON parsing fails
- Does not modify the structure of the object, only attempts to parse string values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test the create object functionality with parsed fields.
# This is a placeholder for an API test; in practice, you would mock the API call
echo "Testing createObject with parsed fields"
node -e '
const common = require("./components/zoho_crm/actions/common/common-objects.mjs");
const createObject = require("./components/zoho_crm/actions/create-object/create-object.mjs");
const sampleObject = {/* provide sample data */};
const parsedObject = common.parseFields(sampleObject);
// Mock zohoCrm.createObject or log the parsedObject for inspection
console.log(parsedObject);
'
Length of output: 1048
Script:
#!/bin/bash
# Let's examine the implementation of parseFields and its usage
ast-grep --pattern 'parseFields($$$)'
# Also check the content of both files
echo "=== Content of create-object.mjs ==="
cat components/zoho_crm/actions/create-object/create-object.mjs
echo -e "\n=== Content of common-objects.mjs ==="
cat components/zoho_crm/actions/common/common-objects.mjs
Length of output: 6984
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
🧹 Outside diff range and nitpick comments (1)
components/zoho_crm/actions/common/common-objects.mjs (1)
36-57
: Simplify the type mapping implementation and remove unnecessary semicolonThe implementation can be improved in a few ways:
- Remove the unnecessary semicolon after the switch statement
- Consider using an object map for type mapping instead of switch-case
Here's a more concise implementation:
getType({ data_type: dataType, json_type: jsonType, }) { - let type; - switch (dataType) { - case "boolean": - type = "boolean"; - break; - case "integer": - type = "integer"; - break; - case "bigint": - type = "integer"; - break; - default: - type = "string"; - break; - }; + const typeMap = { + boolean: "boolean", + integer: "integer", + bigint: "integer" + }; + const type = typeMap[dataType] || "string"; if (jsonType === "jsonarray") { return `${type}[]`; } return type; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/zoho_crm/actions/common/common-objects.mjs
(2 hunks)
🔇 Additional comments (2)
components/zoho_crm/actions/common/common-objects.mjs (2)
151-162
: LGTM! Well-structured picklist handling
The changes to getOptionalProps
are well implemented:
- Correctly passes field object to the updated
getType
method - Good handling of picklist values with clear mapping to value/label pairs
36-38
: Update all getType method calls in the codebase
The getType
method signature has changed to accept an object with data_type
and json_type
properties. All calls to this method throughout the codebase need to be updated to match the new signature to prevent runtime errors.
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!
/approve |
Resolves #14939
Summary by CodeRabbit
New Features
Version Updates
Bug Fixes