New Components - campaign_monitor #14635
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request introduces several new modules to the Campaign Monitor integration, including actions for adding subscribers, sending smart transactional emails, and unsubscribing users. It also enhances the main application by adding various properties and methods for better interaction with the Campaign Monitor API. New sources are included for handling bounce events, tracking email opens, and managing new subscribers. The overall structure improves the capability to manage subscriber data and campaign events effectively. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (10)
components/campaign_monitor/sources/new-subscriber/new-subscriber.mjs (1)
1-10: LGTM! Consider documenting version strategy.The component setup follows best practices with clear naming and appropriate deduplication strategy. Starting with version 0.0.1 is good for a new component.
Consider adding a comment explaining the versioning strategy and what changes would trigger version increments.
components/campaign_monitor/sources/new-bounce/new-bounce.mjs (2)
6-8: Enhance the component description for better clarity.The current description is concise but could be more informative. Consider adding details about when bounces occur and what information is included in the event.
- description: "Emit new event when a campaign email bounces", + description: "Emit new event when a campaign email bounces. This includes hard bounces (permanent failures) and soft bounces (temporary failures) with the recipient's email address and bounce details.",
34-42: Consider adding pagination parameters.The
getArgsmethod sets up ordering but doesn't handle pagination, which might be necessary for handling large volumes of bounce data.Consider adding pagination parameters:
getArgs() { return { campaignId: this.campaignId, params: { orderfield: "date", orderdirection: "desc", + page: this.page || 1, + pagesize: 100, // Adjust based on API limits }, }; },components/campaign_monitor/actions/unsubscribe/unsubscribe.mjs (2)
1-8: LGTM! Consider starting with version 1.0.0The module definition and imports look good. The description is well-documented with a link to the API reference. However, since this is a new component that will be published to users, consider starting with version 1.0.0 instead of 0.0.1 to indicate it's ready for production use.
- version: "0.0.1", + version: "1.0.0",
36-46: Add error handling and improve the success messageWhile the implementation is functional, consider these improvements:
- Add error handling for API failures
- Include the list name in the success message for better context
async run({ $ }) { + const list = await this.campaignMonitor.getList({ + $, + listId: this.listId, + }); + const response = await this.campaignMonitor.unsubscribeSubscriber({ $, listId: this.listId, data: { EmailAddress: this.subscriber, }, }); - $.export("$summary", `Successfully unsubscribed ${this.subscriber}`); + $.export("$summary", `Successfully unsubscribed ${this.subscriber} from list "${list.Title}"`); return response; },components/campaign_monitor/sources/new-email-open/new-email-open.mjs (2)
7-7: Enhance the component description for clarity.The current description could be more informative about when events are emitted and what data is included.
- description: "Emit new event when an email from a campaign is opened", + description: "Emit new event when an email from a campaign is opened. Each event includes the subscriber's email address and timestamp of the open.",
13-28: Add required field validation to props.Both
clientIdandcampaignIdappear to be required fields but this isn't explicitly specified in the prop definitions.clientId: { propDefinition: [ common.props.campaignMonitor, "clientId", ], + required: true, }, campaignId: { propDefinition: [ common.props.campaignMonitor, "campaignId", (c) => ({ clientId: c.clientId, }), ], + required: true, },components/campaign_monitor/actions/add-subscriber/add-subscriber.mjs (1)
37-42: Add phone number format validationThe phone prop accepts any string format. Consider adding format validation to ensure valid international phone numbers.
phone: { type: "string", label: "Phone", description: "The phone number of the subscriber. Example: `+5012398752`", + pattern: "^\\+[1-9]\\d{1,14}$", + pattern_description: "Must be in E.164 format starting with + followed by country code and number", optional: true, },components/campaign_monitor/sources/common/base.mjs (2)
28-28: Consider initializinglastTsto current time to prevent processing old eventsCurrently,
_getLastTs()returns0iflastTsis not set, which will cause the first run to process all historical events since the Unix epoch. If the intention is to process only new events from the time the source is deployed, consider initializinglastTsto the current timestamp during deployment.Modify the
_getLastTsmethod:_getLastTs() { - return this.db.get("lastTs") || 0; + return this.db.get("lastTs") || Date.now(); }
79-81: Pass themaxparameter when callingprocessEventinrunmethodIn the
runmethod,processEventis called without themaxparameter. Ifmaxis an important parameter for limiting the number of events processed, consider passing it explicitly or setting a default value withinprocessEvent.Set a default value for
maxin theprocessEventmethod:async processEvent(max = 100) { // Set a default value for max const lastTs = this._getLastTs(); // ...Or pass the
maxparameter when callingprocessEventinrun:async run() { - await this.processEvent(); + await this.processEvent(100); // Specify the desired max value },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
components/campaign_monitor/actions/add-subscriber/add-subscriber.mjs(1 hunks)components/campaign_monitor/actions/send-smart-transactional-email/send-smart-transactional-email.mjs(1 hunks)components/campaign_monitor/actions/unsubscribe/unsubscribe.mjs(1 hunks)components/campaign_monitor/campaign_monitor.app.mjs(1 hunks)components/campaign_monitor/package.json(2 hunks)components/campaign_monitor/sources/common/base.mjs(1 hunks)components/campaign_monitor/sources/new-bounce/new-bounce.mjs(1 hunks)components/campaign_monitor/sources/new-email-open/new-email-open.mjs(1 hunks)components/campaign_monitor/sources/new-subscriber/new-subscriber.mjs(1 hunks)
🔇 Additional comments (17)
components/campaign_monitor/package.json (2)
3-3: LGTM: Version bump follows semantic versioning
The version increment from 0.0.1 to 0.1.0 appropriately reflects the addition of new features (polling sources and actions) while maintaining semantic versioning principles.
15-17: Verify @pipedream/platform version compatibility
The dependency on @pipedream/platform is correctly specified, but let's verify the compatibility of version ^3.0.3 with the new Campaign Monitor components.
✅ Verification successful
@pipedream/platform version is compatible
No breaking changes found for version 3.x in CHANGELOG.md.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for latest @pipedream/platform version and its compatibility
# Check the latest version available
npm view @pipedream/platform version
# Look for any breaking changes in the codebase that might affect platform version 3.x
rg -l "breaking.*change" --glob "CHANGELOG.md"
Length of output: 91
components/campaign_monitor/sources/new-subscriber/new-subscriber.mjs (2)
28-36: Verify the ordering field name with Campaign Monitor API.
Please verify that "date" is the correct field name for ordering in the Campaign Monitor API. Consider making this a constant if it's used elsewhere.
11-22: Verify the listId prop dependencies and validation.
The prop definition looks correct, but let's verify:
- That the common.props.campaignMonitor reference is properly defined in the base module
- That clientId is always available when needed
components/campaign_monitor/sources/new-bounce/new-bounce.mjs (3)
31-33: Verify the listBounces method implementation.
The method references this.campaignMonitor.listBounces. Let's verify its implementation.
#!/bin/bash
# Description: Verify the listBounces method implementation
# Expected: Find the method definition in the campaign_monitor app
rg -A 10 "listBounces.*\{" components/campaign_monitor/3-52: Verify integration with other Campaign Monitor components.
Let's ensure this component aligns with other Campaign Monitor components and follows the same patterns.
#!/bin/bash
# Description: Verify consistency across Campaign Monitor components
# Expected: Find similar patterns in other components
# Check for consistent component structure
fd -e mjs . components/campaign_monitor/sources/ -x echo "=== {} ===" \; cat {}
# Check for consistent prop usage
rg -l "propDefinition.*clientId|propDefinition.*campaignId" components/campaign_monitor/sources/11-28: Verify prop definitions in the common base module.
The props structure looks good, with proper handling of the clientId-campaignId dependency. Let's verify the prop definitions in the base module.
components/campaign_monitor/actions/unsubscribe/unsubscribe.mjs (2)
9-35: LGTM! Props configuration is well-structured
The props configuration implements a clean dependency chain (clientId → listId → subscriber) and correctly uses propDefinitions from the Campaign Monitor app. All required fields mentioned in the PR objectives are present.
40-42: Verify the API contract for EmailAddress field
The EmailAddress field is capitalized. Let's verify this matches the Campaign Monitor API contract.
components/campaign_monitor/sources/new-email-open/new-email-open.mjs (1)
31-33: Verify Campaign Monitor API integration.
Ensure that the listOpens method exists in the Campaign Monitor API client and handles the provided parameters correctly.
components/campaign_monitor/actions/send-smart-transactional-email/send-smart-transactional-email.mjs (1)
1-8: LGTM! Component metadata is well-structured.
The component configuration follows best practices with clear naming, proper versioning, and helpful documentation links.
components/campaign_monitor/actions/add-subscriber/add-subscriber.mjs (2)
1-8: LGTM! Component metadata is well-structured.
The component metadata follows best practices with clear naming, proper versioning, and helpful documentation links.
72-79: Verify API field mappings
Let's verify that the field names match the Campaign Monitor API requirements.
components/campaign_monitor/campaign_monitor.app.mjs (2)
73-90: Ensure proper handling of clientId in smartEmailId options
The options function for smartEmailId expects clientId in params. Verify that clientId is correctly passed and handled in all scenarios to fetch smart emails appropriately.
Run the following script to confirm usage:
#!/bin/bash
# Description: Check that `clientId` is correctly utilized when fetching smart emails.
# Search for usages of `listSmartEmails` and ensure `clientId` is included in parameters.
rg 'listSmartEmails\(\{' -A 5 | rg 'clientId'37-55: Validate presence of listId in subscriber options
In the subscriber property definition, the options function relies on listId. Ensure that listId is provided to avoid issues when fetching subscribers.
Run the following script to check if listId is always passed correctly:
✅ Verification successful
Verified: listId is consistently provided in all subscriber options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `listId` is used wherever `subscriber` options are fetched.
# Find occurrences where `subscriber` property is defined without `listId` context.
rg 'subscriber: \{' -A 10 | rg -L 'listId'
Length of output: 120
components/campaign_monitor/sources/common/base.mjs (2)
39-44: Ensure derived components implement getResourceFn and generateMeta methods
The methods getResourceFn() and generateMeta() currently throw errors indicating they are not implemented. This base class expects these methods to be overridden. Please ensure that all components inheriting from this base class provide concrete implementations of these methods to avoid runtime errors.
Run the following script to verify that derived components implement these methods:
51-65: Verify that paginate returns results sorted by date in descending order
The processEvent method assumes that the results returned by this.campaignMonitor.paginate() are sorted in descending order based on the timestamp field (tsField). This is important because the loop breaks when it encounters an item with a timestamp less than lastTs, under the assumption that no subsequent items will have newer timestamps. If the data isn't sorted in descending order, some events may be missed.
To confirm that the paginate method returns results in the correct order, run the following script:
...s/campaign_monitor/actions/send-smart-transactional-email/send-smart-transactional-email.mjs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
components/campaign_monitor/campaign_monitor.app.mjs (3)
40-55: Simplify pagination in subscriber optionsThe page increment
page + 1is unnecessary as the pagination should be handled by the API method itself.async options({ listId, page, }) { const { Results: subscribers } = await this.listSubscribers({ listId, params: { - page: page + 1, + page, }, });
91-102: Enhance consentToTrack descriptionThe description could be more specific about the implications of each option and legal requirements.
- description: "Whether the subscriber has given permission to have their email opens and clicks tracked", + description: "Specify tracking consent status: 'Yes' to enable tracking, 'No' to disable, or 'Unchanged' to maintain current preference. This affects email opens and click tracking in compliance with privacy regulations.",
224-249: Enhance pagination robustnessThe pagination implementation could be improved:
- Initialize page parameter
- Add type validation for max parameter
async *paginate({ fn, args, max, }) { + if (max !== undefined && (!Number.isInteger(max) || max < 0)) { + throw new Error('max must be a non-negative integer'); + } args = { ...args, params: { ...args?.params, + page: args?.params?.page || 1, pagesize: DEFAULT_PAGE_SIZE, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/campaign_monitor/campaign_monitor.app.mjs(1 hunks)
🔇 Additional comments (2)
components/campaign_monitor/campaign_monitor.app.mjs (2)
108-121: Add error handling to _makeRequest method
The previous review comment about adding error handling is still valid. This is critical for proper error management.
1-2: Verify Campaign Monitor's API pagination limits
The DEFAULT_PAGE_SIZE of 1000 seems high. Please verify this against Campaign Monitor's API documentation to ensure it doesn't exceed their rate limits or recommended page sizes.
There was a problem hiding this comment.
Hi @michelle0927 , I left a comment regarding a possible duplicate prop, which can impact in testing.
components/campaign_monitor/sources/new-email-open/new-email-open.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
components/campaign_monitor/campaign_monitor.app.mjs (3)
1-2: Consider reducing the DEFAULT_PAGE_SIZE valueThe current page size of 1000 might lead to high memory usage when dealing with large datasets. Consider reducing this value to a more moderate size (e.g., 100-250) to balance between network requests and memory consumption.
91-102: Consider using an enum for consentToTrack optionsThe consentToTrack options are hardcoded strings. Consider defining these as constants to prevent typos and improve maintainability.
Add this at the top of the file:
const CONSENT_OPTIONS = { YES: "Yes", NO: "No", UNCHANGED: "Unchanged", };Then update the options:
-options: [ - "Yes", - "No", - "Unchanged", -], +options: Object.values(CONSENT_OPTIONS),
1-270: Consider adding runtime type checking for API responsesThe app makes extensive use of destructuring API responses without validating their structure. Consider adding a runtime type checking library like
zodorjoito validate API responses and prevent runtime errors.This would help catch API changes early and provide better error messages. Example implementation:
import { z } from 'zod'; const ClientSchema = z.object({ ClientID: z.string(), Name: z.string(), }); const listClientsResponseSchema = z.array(ClientSchema); async listClients(opts = {}) { const response = await this._makeRequest({ path: "/clients.json", ...opts, }); return listClientsResponseSchema.parse(response); }
|
/approve |
Resolves #14557
Summary by CodeRabbit
Release Notes
New Features
Version Update
Documentation