-
Notifications
You must be signed in to change notification settings - Fork 283
[Klaviyo] [Multistatus] Add Multistatus for Klaviyo upsertProfile action #3203
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3203 +/- ##
==========================================
+ Coverage 79.62% 79.91% +0.28%
==========================================
Files 1177 1184 +7
Lines 21703 21826 +123
Branches 4216 4251 +35
==========================================
+ Hits 17282 17442 +160
+ Misses 3679 3634 -45
- Partials 742 750 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
✨ - Looks great @harsh-joshi99 . Just one comment that I'd like to get answer to before approving.
packages/destination-actions/src/destinations/klaviyo/upsertProfile/index.ts
Outdated
Show resolved
Hide resolved
if (!payload.email && !payload.phone_number && !payload.external_id) { | ||
response.error = { | ||
status: 400, | ||
errortype: 'PAYLOAD_VALIDATION_FAILED', | ||
errormessage: 'One of External ID, Phone Number or Email is required.' | ||
} | ||
return response | ||
} |
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.
Note for later - we can move this to conditionally required field configuration and remove this code.
packages/destination-actions/src/destinations/klaviyo/functions.ts
Outdated
Show resolved
Hide resolved
* | ||
* Ensures that at least one of `email`, `phone_number`, or `external_id` is present. | ||
* If `phone_number` is provided, it validates and converts it to E.164 format using the `country_code`. | ||
* If `email` is provided, it validates the email format. |
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.
This comment needs to be updated?
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.
Pull Request Overview
This PR adds multistatus response support for the Klaviyo upsertProfile action, enabling individual error handling for each profile in a batch operation. The changes streamline the batching logic by removing the previous group-by-list-ID approach in favor of using batch keys, and add explicit email validation to support proper error reporting.
Key Changes
- Implements MultiStatusResponse for granular error handling in batch operations
- Removes complex list ID grouping logic in favor of simplified batch processing
- Adds comprehensive payload validation with specific error messages for email and phone number validation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/destination-actions/src/destinations/klaviyo/upsertProfile/index.ts |
Refactored performBatch method to use MultiStatusResponse with payload validation and simplified batch processing |
packages/destination-actions/src/destinations/klaviyo/functions.ts |
Added validateProfilePayload function, removed groupByListId and processProfilesByGroup functions, exported multistatus helper functions |
packages/destination-actions/src/destinations/klaviyo/types.ts |
Added validateProfilePayloadResult interface and removed GroupedProfiles interface |
packages/destination-actions/src/destinations/klaviyo/upsertProfile/__tests__/index.test.ts |
Removed outdated tests for grouping logic and error handling that no longer apply |
packages/destination-actions/src/destinations/klaviyo/__tests__/multistatus.test.ts |
Added comprehensive test suite for multistatus functionality with various validation scenarios |
packages/destination-actions/src/destinations/klaviyo/upsertProfile/index.ts
Show resolved
Hide resolved
packages/destination-actions/src/destinations/klaviyo/upsertProfile/index.ts
Show resolved
Hide resolved
* @param payload - The profile payload to validate. | ||
* @returns An object containing the validated payload or an error response if validation fails. | ||
*/ | ||
export function validateProfilePayload(payload: Payload): validateProfilePayloadResult { |
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.
The function documentation mentions email validation, but the actual implementation doesn't include email format validation. Either update the documentation to remove the email validation reference or implement the missing email validation logic.
Copilot uses AI. Check for mistakes.
Hi @harsh-joshi99 PR deployed |
Add multistatus for upsert profile.
Removed the group by list Id logic since we have batch keys now.
Added explicit email validation to support multistatus.
Testing
Stage Testing Doc.