-
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
14026 action sendgrid campaign sends #14811
base: master
Are you sure you want to change the base?
Conversation
Actions - Create Send
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request updates various SendGrid action components by incrementing their version numbers to "0.0.4" or higher. It introduces new properties, modifies validation logic, and enhances error handling across multiple files. Notably, a new action for creating single sends is added, along with improvements to existing actions related to email suppression, contact management, and event handling. The updates aim to refine functionality and maintain consistency across the SendGrid integration. Changes
Assessment against linked issues
Possibly related PRs
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 (
|
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: 9
🧹 Outside diff range and nitpick comments (11)
components/sendgrid/common/utils.mjs (1)
1-2
: Consider more specific input validation.The current falsy check might mask valid cases. Consider being more explicit about which values are considered invalid.
export const parseObject = (obj) => { - if (!obj) return undefined; + if (obj === null || obj === undefined) return undefined;components/sendgrid/actions/create-send/create-send.mjs (1)
10-10
: Align action version with other componentsThe action's
version
is set to "0.0.1", whereas other actions in the package use "0.0.4". For consistency, consider updating the version to match the others.Apply this diff to update the version:
- version: "0.0.1", + version: "0.0.4",components/sendgrid/sendgrid.app.mjs (1)
20-21
: Remove unnecessary console.log statementThe
console.log("lists: ", lists);
statement appears to be for debugging purposes. Remove it to clean up the code.Apply this diff to remove the console log:
async options() { const lists = await this.getAllContactLists(); - console.log("lists: ", lists);
components/sendgrid/actions/delete-blocks/delete-blocks.mjs (1)
Line range hint
18-21
: UseConfigurationError
for consistent error handlingFor consistency and better error reporting, consider throwing a
ConfigurationError
instead of a genericError
.Apply this diff to update the error type:
async run({ $ }) { if (this.deleteAll && this.emails) { - throw new Error("Must provide only one of `deleteAll` or `emails` parameters."); + throw new ConfigurationError("Must provide only one of `Delete All` or `Emails` parameters."); }components/sendgrid/actions/remove-contact-from-list/remove-contact-from-list.mjs (3)
Line range hint
39-45
: Fix SQL injection vulnerability in contact searchThe current implementation is vulnerable to SQL injection attacks through string interpolation of the email parameter.
Apply this diff to fix the vulnerability:
- const { result } = await this.sendgrid.searchContacts(`email like '${email}'`); + const { result } = await this.sendgrid.searchContacts(`email LIKE '${email.replace(/'/g, "''")}'`);
Line range hint
39-45
: Add error handling for contact searchThe code doesn't handle cases where the contact search fails or returns no results.
Apply this diff to add proper error handling:
for (const email of contactEmails) { - const { result } = await this.sendgrid.searchContacts(`email like '${email}'`); - const id = result[0]?.id; - if (!contactIds.includes(id)) { - contactIds.push(id); + try { + const { result } = await this.sendgrid.searchContacts(`email LIKE '${email.replace(/'/g, "''")}'`); + const id = result[0]?.id; + if (!id) { + $.export("$summary", `Warning: Contact with email ${email} not found`); + continue; + } + if (!contactIds.includes(id)) { + contactIds.push(id); + } + } catch (error) { + throw new Error(`Failed to search for contact with email ${email}: ${error.message}`); } }
Line range hint
39-45
: Optimize contact search performanceThe current implementation makes sequential API calls for each email. Consider batching the requests.
Apply this diff to improve performance:
- for (const email of contactEmails) { - const { result } = await this.sendgrid.searchContacts(`email like '${email}'`); - const id = result[0]?.id; - if (!contactIds.includes(id)) { - contactIds.push(id); - } + if (contactEmails.length > 0) { + const emailConditions = contactEmails + .map(email => `email LIKE '${email.replace(/'/g, "''")}'`) + .join(" OR "); + const { result } = await this.sendgrid.searchContacts(`(${emailConditions})`); + const newIds = result + .map(contact => contact.id) + .filter(id => !contactIds.includes(id)); + contactIds.push(...newIds); }components/sendgrid/actions/delete-contacts/delete-contacts.mjs (1)
Line range hint
41-47
: Add validation for empty inputsThe code should validate that at least one deletion method is specified.
Apply this diff to add input validation:
if (deleteAllContacts && (contactIds || contactEmails)) { throw new Error("If `deleteAllContacts` is selected, cannot select `contactIds` or `contactEmails`"); } + if (!deleteAllContacts && contactIds.length === 0 && contactEmails.length === 0) { + throw new Error("Must specify either deleteAllContacts or provide contactIds/contactEmails"); + }components/sendgrid/actions/add-or-update-contact/add-or-update-contact.mjs (1)
Line range hint
78-89
: Consider enhancing email validation for alternateEmailsWhile the array validation is properly implemented, consider adding email format validation for each email in the alternateEmails array.
if (this.alternateEmails) { constraints.cc = { type: "array", + email: { + message: "must be valid email addresses" + } }; }components/sendgrid/sources/new-contact/new-contact.mjs (2)
Line range hint
25-29
: Consider making the delay time configurableThe 30-minute delay time is currently hardcoded. Consider making this configurable through environment variables or component settings to allow for adjustments based on observed SendGrid behavior.
+ props: { + ...common.props, + maxDelayMinutes: { + type: "integer", + label: "Maximum Delay (minutes)", + description: "Maximum time to wait for contacts to appear in search results", + default: 30, + optional: true, + }, + }, methods: { ...common.methods, _maxDelayTime() { - return 30 * 60 * 1000; // 30 minutes, in milliseconds + return (this.maxDelayMinutes || 30) * 60 * 1000; },
Line range hint
127-141
: Consider adding retry mechanism for failed API callsThe contact search API call could benefit from a retry mechanism in case of temporary failures.
+ async searchContactsWithRetry(query, maxRetries = 3) { + for (let i = 0; i < maxRetries; i++) { + try { + return await this.sendgrid.searchContacts(query); + } catch (error) { + if (i === maxRetries - 1) throw error; + await new Promise(resolve => setTimeout(resolve, 1000 * Math.pow(2, i))); + } + } + } async processEvent(event) { ... - const { result: items, contact_count: contactCount } = await this.sendgrid.searchContacts(query); + const { result: items, contact_count: contactCount } = await this.searchContactsWithRetry(query); ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
components/sendgrid/actions/add-email-to-global-suppression/add-email-to-global-suppression.mjs
(1 hunks)components/sendgrid/actions/add-or-update-contact/add-or-update-contact.mjs
(1 hunks)components/sendgrid/actions/create-contact-list/create-contact-list.mjs
(1 hunks)components/sendgrid/actions/create-send/create-send.mjs
(1 hunks)components/sendgrid/actions/delete-blocks/delete-blocks.mjs
(1 hunks)components/sendgrid/actions/delete-bounces/delete-bounces.mjs
(1 hunks)components/sendgrid/actions/delete-contacts/delete-contacts.mjs
(1 hunks)components/sendgrid/actions/delete-global-suppression/delete-global-suppression.mjs
(1 hunks)components/sendgrid/actions/delete-list/delete-list.mjs
(1 hunks)components/sendgrid/actions/get-a-block/get-a-block.mjs
(1 hunks)components/sendgrid/actions/get-a-global-suppression/get-a-global-suppression.mjs
(1 hunks)components/sendgrid/actions/get-all-bounces/get-all-bounces.mjs
(1 hunks)components/sendgrid/actions/get-contact-lists/get-contact-lists.mjs
(1 hunks)components/sendgrid/actions/list-blocks/list-blocks.mjs
(1 hunks)components/sendgrid/actions/list-global-suppressions/list-global-suppressions.mjs
(1 hunks)components/sendgrid/actions/remove-contact-from-list/remove-contact-from-list.mjs
(1 hunks)components/sendgrid/actions/search-contacts/search-contacts.mjs
(1 hunks)components/sendgrid/actions/send-email-multiple-recipients/send-email-multiple-recipients.mjs
(1 hunks)components/sendgrid/actions/send-email-single-recipient/send-email-single-recipient.mjs
(1 hunks)components/sendgrid/actions/validate-email/validate-email.mjs
(1 hunks)components/sendgrid/common/constants.mjs
(1 hunks)components/sendgrid/common/utils.mjs
(1 hunks)components/sendgrid/package.json
(2 hunks)components/sendgrid/sendgrid.app.mjs
(4 hunks)components/sendgrid/sources/events/events.mjs
(1 hunks)components/sendgrid/sources/new-contact/new-contact.mjs
(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- components/sendgrid/common/constants.mjs
- components/sendgrid/actions/create-contact-list/create-contact-list.mjs
- components/sendgrid/actions/send-email-single-recipient/send-email-single-recipient.mjs
- components/sendgrid/actions/delete-list/delete-list.mjs
- components/sendgrid/actions/validate-email/validate-email.mjs
- components/sendgrid/actions/get-a-block/get-a-block.mjs
- components/sendgrid/actions/add-email-to-global-suppression/add-email-to-global-suppression.mjs
- components/sendgrid/actions/get-a-global-suppression/get-a-global-suppression.mjs
- components/sendgrid/actions/list-global-suppressions/list-global-suppressions.mjs
- components/sendgrid/actions/list-blocks/list-blocks.mjs
- components/sendgrid/actions/send-email-multiple-recipients/send-email-multiple-recipients.mjs
🔇 Additional comments (13)
components/sendgrid/common/utils.mjs (1)
1-24
: Add JSDoc documentation and verify usage in campaign context.
The function should be documented to clarify its purpose and behavior, especially given its use in SendGrid campaign processing.
Add documentation:
+/**
+ * Parses JSON strings and arrays containing JSON strings.
+ * Used for processing SendGrid campaign data.
+ * @param {any} obj - The object to parse
+ * @returns {any} Parsed object or original value if parsing fails
+ * @throws {Error} If array size exceeds limit
+ */
export const parseObject = (obj) => {
Let's verify how this utility is used in the campaign context:
✅ Verification successful
Let me check the specific usage in the Sendy campaign file to understand how parseObject
is being used.
Based on the analysis of the code and its usage, I can now provide a final response:
Documentation added, function usage verified in campaign context
The parseObject
utility is correctly used in the Sendy campaign creation context to parse array inputs like listIds
, segmentIds
, excludeListIds
, and excludeSegmentIds
. The function safely handles both string and array inputs, which is appropriate for the campaign API parameters that expect array data.
The same pattern is consistently used across multiple components in the codebase, confirming this is a well-established utility for handling JSON parsing of configuration values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of parseObject in campaign-related files
rg -l "parseObject" | grep -i "campaign"
# Check for potential JSON string handling in campaign files
rg -A 3 "JSON\.parse" | grep -i "campaign"
Length of output: 1253
Script:
#!/bin/bash
# Check how parseObject is used in the campaign file
cat components/sendy/actions/create-draft-campaign/create-draft-campaign.mjs
# Also check if parseObject is used in other files
rg "parseObject" -A 3
Length of output: 65972
components/sendgrid/package.json (2)
3-3
: Version update reflects component changes
The version has been updated to "0.4.0", which aligns with the new features added.
13-13
: Dependency update to latest platform version
The @pipedream/platform
dependency has been updated to "^3.0.3", ensuring compatibility with new platform features.
components/sendgrid/actions/get-contact-lists/get-contact-lists.mjs (1)
Line range hint 8-17
: Addition of numberOfLists
prop enhances functionality
Introducing the numberOfLists
prop allows users to specify the maximum number of contact lists to retrieve, improving flexibility.
components/sendgrid/actions/delete-blocks/delete-blocks.mjs (1)
8-8
: Version increment aligns with other actions
The action's version has been updated to "0.0.4", maintaining consistency across the package.
components/sendgrid/actions/delete-global-suppression/delete-global-suppression.mjs (1)
9-9
: Verify version bump rationale
The version has been bumped from 0.0.3 to 0.0.4, but there are no significant changes in this file. Consider documenting the reason for version increment in the commit message or PR description.
components/sendgrid/actions/delete-contacts/delete-contacts.mjs (1)
Line range hint 41-52
: Fix SQL injection and optimize contact search
The same issues with SQL injection, error handling, and performance exist in this file.
Please apply the same fixes as suggested in the review of remove-contact-from-list.mjs.
components/sendgrid/sources/events/events.mjs (1)
10-10
: LGTM!
The version bump is the only change. The existing implementation is well-structured and follows best practices.
components/sendgrid/actions/search-contacts/search-contacts.mjs (2)
10-10
: LGTM: Version increment aligned with other SendGrid components
The version bump from 0.0.3 to 0.0.4 is consistent with other SendGrid component updates in this PR.
Line range hint 47-50
: Improved input validation logic
The validation logic now properly enforces mutual exclusivity between query
and queryField
parameters, preventing ambiguous searches.
components/sendgrid/actions/add-or-update-contact/add-or-update-contact.mjs (1)
9-9
: LGTM: Version increment aligned with other SendGrid components
The version bump from 0.0.3 to 0.0.4 is consistent with other SendGrid component updates in this PR.
components/sendgrid/sources/new-contact/new-contact.mjs (2)
9-9
: LGTM: Version increment follows semantic versioning
The version bump from 0.0.6 to 0.0.7 is appropriate for the minor improvements in this update.
Line range hint 51-63
: Verify SQL injection protection in query construction
The query construction appears to handle IDs safely, but let's verify there are no SQL injection vulnerabilities.
✅ Verification successful
SQL injection protection is properly implemented
The code is safe from SQL injection for the following reasons:
-
IDs are processed through a strict transformation chain:
- First mapped from objects to raw IDs:
.map((item) => item.id)
- Then wrapped in single quotes:
.map((id) =>
'${id}')
- Finally joined with commas:
.join(", ")
- If no IDs exist, defaults to
"''"
- First mapped from objects to raw IDs:
-
Timestamps are processed through internal methods:
_addDelayOffset()
performs numeric calculationstoISOString()
converts timestamps to formatted strings- Values are properly quoted in the query:
TIMESTAMP '${startDate}'
The query construction uses proper SQL syntax with parameterized values, and all inputs are either numeric timestamps processed through internal methods or IDs that are strictly formatted with proper SQL string escaping.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential SQL injection vulnerabilities in query construction
rg -n "TIMESTAMP '\$\{.*\}'" .
rg -n "IN \(\$\{.*\}\)" .
Length of output: 64
Script:
#!/bin/bash
# Let's examine the full context of the query construction and validation
rg -n -A 10 -B 10 "contact_id NOT IN" .
Length of output: 8534
Script:
#!/bin/bash
# Let's check how the input parameters are processed before being used in the query
ast-grep --pattern 'toISOString($_) {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Let's check how the ID list is processed
ast-grep --pattern '.map((item) => item.id)
.map((id) => `'\''${id}'\''`)'
Length of output: 84
Script:
#!/bin/bash
# Let's check for any input validation or sanitization methods
rg -n "sanitize|escape|validate" .
Length of output: 66026
Script:
#!/bin/bash
# Let's check how the toISOString method is implemented
ast-grep --pattern 'toISOString($_) {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Let's check how the _addDelayOffset method is implemented
ast-grep --pattern '_addDelayOffset($_) {
$$$
}'
Length of output: 52
Script:
#!/bin/bash
# Let's examine the full context of the class implementation
rg -A 5 -B 5 "class NewContact" .
Length of output: 35
Script:
#!/bin/bash
# Let's try a different pattern to find the class implementation
rg -A 20 -B 20 "_makeSearchQuery" components/sendgrid/sources/new-contact/new-contact.mjs
Length of output: 2848
Script:
#!/bin/bash
# Let's check if there are any input sanitization methods in the file
rg -n "sanitize|escape|clean" components/sendgrid/sources/new-contact/new-contact.mjs
Length of output: 389
Script:
#!/bin/bash
# Let's check how the IDs are processed before being used in the query
rg -A 5 -B 5 "\.map\(\(item\) => item\.id\)" components/sendgrid/sources/new-contact/new-contact.mjs
Length of output: 507
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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/sendgrid/actions/create-send/create-send.mjs (1)
10-10
: Consider updating the action version for consistencyThe version is set to "0.0.1". Since other SendGrid actions have been updated to "0.0.4", consider updating this action's version to maintain consistency across the SendGrid integration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/sendgrid/actions/create-send/create-send.mjs
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/sendgrid/actions/create-send/create-send.mjs
[error] 153-154: Expected a statement but instead found 'catch (e)'.
Expected a statement here.
(parse)
[error] 154-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 154-160: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (2)
components/sendgrid/actions/create-send/create-send.mjs (2)
150-150
: Success message accurately reflects the action performed
The success message correctly states "Successfully created single send", which aligns with the action's functionality.
129-129
:
Correct the property key for categories
The key categories
is being assigned parseObject(this.categoryIds)
. According to the SendGrid API documentation, the correct key should be category_ids
. Updating the key ensures the categories are correctly recognized by the API.
Apply this diff to fix the property key:
- categories: parseObject(this.categoryIds),
+ category_ids: parseObject(this.categoryIds),
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.
Hi @luancazarine just a minor change but lgtm! Ready for QA!
$.export("$summary", `Successfully created single send ${this.name}`); | ||
return resp; | ||
} catch (e) { | ||
} catch (e) { |
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.
} catch (e) { |
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 (4)
components/sendgrid/actions/create-send/create-send.mjs (4)
53-58
: Add validation for subject line lengthThe
subject
prop should include validation for maximum length to prevent API errors. SendGrid typically has a limit on subject line length.Add validation by updating the prop definition:
subject: { type: "string", label: "Subject", description: "The subject line of the Single Send. Do not include this field when using a `Design Id`.", + validate: (subject) => { + if (subject && subject.length > 255) { + throw new Error("Subject line cannot exceed 255 characters"); + } + }, optional: true, },
153-162
: Improve error handling with optional chainingThe current error handling can be simplified using optional chaining for better readability and maintainability.
Apply this diff to improve the error handling:
- if ( - e.response - && e.response.data - && e.response.data.errors - && e.response.data.errors.length > 0 - ) { + if (e.response?.data?.errors?.length > 0) { throw new ConfigurationError(e.response.data.errors[0].message); } else { throw new ConfigurationError("An unexpected error occurred."); }🧰 Tools
🪛 Biome (1.9.4)
[error] 154-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
121-123
: Enhance validation error messageThe error message for missing suppression settings could be more descriptive to help users understand the requirements better.
Apply this diff to improve the error message:
- throw new ConfigurationError("You must provide either `Suppression Group Id` or the `Custom Unsubscribe URL`."); + throw new ConfigurationError("SendGrid requires an unsubscribe mechanism. Please provide either a Suppression Group ID for managing unsubscribes through SendGrid, or a Custom Unsubscribe URL to handle unsubscribes through your own system.");
1-165
: Consider implementing retry logic for API failuresWhile the basic error handling is in place, the action could benefit from retry logic for transient API failures. This would improve reliability, especially for time-sensitive campaign sends.
Consider implementing an exponential backoff retry strategy for API calls, particularly for rate limit errors (HTTP 429) or temporary service unavailability (HTTP 503).
🧰 Tools
🪛 Biome (1.9.4)
[error] 154-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/sendgrid/actions/create-send/create-send.mjs
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/sendgrid/actions/create-send/create-send.mjs
[error] 154-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
components/sendgrid/actions/create-send/create-send.mjs (2)
1-11
: LGTM! Imports and metadata are well-defined.
The imports are appropriate, and the metadata provides clear documentation with API reference link.
129-129
:
Fix property reference for categories
The categories
property in the request payload should use this.categories
, but the prop is defined as categoryIds
.
Apply this diff to fix the property reference:
- categories: parseObject(this.categoryIds),
+ categories: parseObject(this.categories),
Likely invalid or redundant comment.
Resolves #14026
Summary by CodeRabbit
Release Notes
New Features
contactEmails
added for specifying email addresses in the delete contacts action.LIMIT
constant to define maximum return values.Improvements
Version Updates