-
Notifications
You must be signed in to change notification settings - Fork 272
STRATCONN-5849 - braze_api_campaign #2947
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
base: main
Are you sure you want to change the base?
Conversation
… into STRATCONN-5849_braze_api_campaign
New required fields detectedWarning Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production. The following required fields were added in this PR:
Add these new fields as optional instead and assume default values in |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2947 +/- ##
==========================================
+ Coverage 77.83% 77.88% +0.04%
==========================================
Files 1077 1079 +2
Lines 19810 19851 +41
Branches 3800 3810 +10
==========================================
+ Hits 15420 15461 +41
Misses 3100 3100
Partials 1290 1290 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… into STRATCONN-5849_braze_api_campaign
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.
Great work! Left a few questions.
Specifically I wonder if object within object and array within objects are rendered correctly in the UI. If not, we should consider extracting them out of recipients
if (!payload.broadcast && !payload.recipients?.length && !payload.audience) { | ||
throw new IntegrationError( | ||
'One of "recipients", "broadcast" or "audience", must be provided.', | ||
'Missing required fields', | ||
400 | ||
) | ||
} |
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.
could we try using conditionally required fields for this?
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've added the conditionally required fields, however this check is still required in case the recipients are dynamically picked from the event and turns out to be an empty array (not undefined/null).
} | ||
|
||
// Prepare the request body with only defined fields | ||
const requestBody: Record<string, unknown> = { |
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.
All undefined fields will get removed by the request client when it does JSON.stringify. So, it is not required for us to do this check everytime. If a field doesn't support null, we can mark allowNull as false on the field definition
'@path': '$.userId' | ||
} | ||
}, | ||
user_alias: { |
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.
does this work? I wonder how object within object gets rendered in UI
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.
packages/destination-actions/src/destinations/braze/triggerCampaign/index.ts
Show resolved
Hide resolved
}) | ||
|
||
// Test with recipients array | ||
it('with recipients array', async () => { |
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.
could we extract these tests to a separate file like we do with all other destinations?
Added a new action Trigger Campaign in Braze destination. Refer this doc for more info.
Testing
Testing Doc