Braze cloud mode -New merge users action#3725
Braze cloud mode -New merge users action#3725segment-voliveira wants to merge 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Braze Cloud Mode (Actions) action to merge two user profiles in Braze via the /users/merge endpoint, including request construction/validation in shared Braze utilities and accompanying tests.
Changes:
- Introduces a new
mergeUsersaction with configurable “identifier to merge” and “identifier to keep” inputs. - Adds a
mergeUsersutility inbraze/utils.tsto validate identifiers and build the Braze API request payload. - Registers the action in the Braze destination and adds unit/snapshot test coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/braze/utils.ts | Adds mergeUsers() helper that validates identifiers and posts merge_updates to Braze. |
| packages/destination-actions/src/destinations/braze/mergeUsers/index.ts | Defines the new Merge Users action fields and wiring to the utility. |
| packages/destination-actions/src/destinations/braze/mergeUsers/generated-types.ts | Generated payload types for the new action. |
| packages/destination-actions/src/destinations/braze/mergeUsers/tests/snapshot.test.ts | Snapshot coverage for required/all-fields mappings. |
| packages/destination-actions/src/destinations/braze/mergeUsers/tests/index.test.ts | Unit tests covering identifier variants and validation failures. |
| packages/destination-actions/src/destinations/braze/mergeUsers/tests/snapshots/snapshot.test.ts.snap | Stored snapshots for the new action. |
| packages/destination-actions/src/destinations/braze/index.ts | Registers the new mergeUsers action in the Braze destination. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3725 +/- ##
==========================================
- Coverage 80.91% 80.73% -0.19%
==========================================
Files 1386 1330 -56
Lines 27903 24958 -2945
Branches 6026 5201 -825
==========================================
- Hits 22577 20149 -2428
+ Misses 4350 3855 -495
+ Partials 976 954 -22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| try { | ||
| const json = JSON.parse(rawBody) | ||
| expect(json).toMatchSnapshot() | ||
| return | ||
| } catch (err) { | ||
| expect(rawBody).toMatchSnapshot() | ||
| } | ||
|
|
||
| expect(request.headers).toMatchSnapshot() | ||
| }) |
There was a problem hiding this comment.
In this snapshot test, the return inside the JSON-parse try block makes the expect(request.headers).toMatchSnapshot() assertion unreachable on the success path. Remove the early return (or move the headers snapshot above the return) so headers are always validated when the body is valid JSON.
| }, | ||
| identifier_to_keep: { | ||
| external_id: { | ||
| '@path': '$.userId' | ||
| } |
There was a problem hiding this comment.
This test claims to verify the action’s default mapping from userId, but it explicitly supplies an @path mapping for identifier_to_keep.external_id. Either rename the test to reflect what it actually covers, or omit identifier_to_keep from mapping and let the action’s field defaults populate it (then assert the resolved request body).
| }, | |
| identifier_to_keep: { | |
| external_id: { | |
| '@path': '$.userId' | |
| } |
| // Validate identifier_to_merge | ||
| const mergeUserAlias = getUserAlias(payload.identifier_to_merge?.user_alias) | ||
| const hasMergeIdentifier = | ||
| payload.identifier_to_merge?.external_id || | ||
| mergeUserAlias || | ||
| payload.identifier_to_merge?.braze_id || | ||
| payload.identifier_to_merge?.email || | ||
| payload.identifier_to_merge?.phone | ||
|
|
||
| if (!hasMergeIdentifier) { | ||
| throw new IntegrationError( | ||
| 'Identifier to Merge must specify one of: external_id, user_alias, braze_id, email, or phone.', | ||
| 'Missing required fields', | ||
| 400 | ||
| ) | ||
| } |
There was a problem hiding this comment.
When user_alias is present but incomplete (missing alias_name or alias_label), getUserAlias() returns undefined and this code throws the generic “must specify one of … user_alias …” error. Consider detecting the “user_alias provided but invalid” case and throwing a more specific message (or explicitly stating that user_alias must include both alias_name and alias_label) to make the failure actionable.
| user_alias: { | ||
| label: 'User Alias', | ||
| description: 'The user alias object identifying the user to merge', | ||
| type: 'object', | ||
| properties: { | ||
| alias_name: { | ||
| label: 'Alias Name', | ||
| type: 'string' | ||
| }, | ||
| alias_label: { | ||
| label: 'Alias Label', | ||
| type: 'string' | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
user_alias is treated as valid only when both alias_name and alias_label are present (via getUserAlias()), but the field definition doesn’t indicate that requirement. Update the field description and/or schema (e.g., mark alias_name/alias_label as required) so mappings fail validation with a clear message instead of being treated as “missing identifier” at runtime.
| external_id: { | ||
| label: 'External ID', | ||
| description: 'The external ID of the user to keep', | ||
| type: 'string', | ||
| default: { | ||
| '@path': '$.userId' | ||
| } | ||
| }, | ||
| user_alias: { | ||
| label: 'User Alias', | ||
| description: 'The user alias object identifying the user to keep', | ||
| type: 'object', | ||
| properties: { | ||
| alias_name: { | ||
| label: 'Alias Name', | ||
| type: 'string' | ||
| }, | ||
| alias_label: { | ||
| label: 'Alias Label', | ||
| type: 'string' | ||
| } | ||
| } | ||
| }, | ||
| braze_id: { | ||
| label: 'Braze ID', | ||
| description: 'The Braze ID of the user to keep', | ||
| type: 'string' | ||
| }, | ||
| email: { | ||
| label: 'Email', | ||
| description: 'The email address of the user to keep', | ||
| type: 'string', | ||
| format: 'email' | ||
| }, | ||
| phone: { | ||
| label: 'Phone', | ||
| description: 'The phone number of the user to keep in E.164 format (e.g., +14155552671)', | ||
| type: 'string' | ||
| } | ||
| }, | ||
| default: { | ||
| external_id: { | ||
| '@path': '$.userId' | ||
| }, | ||
| braze_id: { | ||
| '@if': { | ||
| exists: { '@path': '$.context.traits.brazeId' } | ||
| }, |
There was a problem hiding this comment.
identifier_to_keep.external_id defines a default at the property level and the parent identifier_to_keep object also defines a default that includes external_id. Keeping defaults in one place (preferably the parent object default here) avoids confusion about which default should win and reduces future maintenance overhead.
|
Comments: Main thing: If only 1 previous Id and one new ID can be passed at the same time, then we should change the field structure to this:
|
| // Generated file. DO NOT MODIFY IT BY HAND. | ||
|
|
||
| export interface Payload { | ||
| /** | ||
| * The type of identifier for the user to be merged. One of: external_id, user_alias, braze_id, email, or phone. | ||
| */ | ||
| previousIdType: string | ||
| /** | ||
| * The value of the identifier for the user to be merged. | ||
| */ | ||
| previousIdValue?: string | ||
| /** | ||
| * The value of the user alias identifier for the user to be merged. Required if the previous identifier type is user_alias. | ||
| */ | ||
| previousAliasIdValue?: { | ||
| /** | ||
| * The label of the user alias for the user to be merged. | ||
| */ | ||
| alias_label: string | ||
| /** | ||
| * The name of the user alias for the user to be merged. | ||
| */ | ||
| alias_name: string | ||
| } | ||
| /** | ||
| * The type of identifier for the user to be kept. One of: external_id, user_alias, braze_id, email, or phone. | ||
| */ | ||
| keepIdType: string | ||
| /** | ||
| * The value of the identifier for the user to be kept. | ||
| */ | ||
| keepIdValue?: string | ||
| /** | ||
| * The value of the user alias identifier for the user to be kept. Required if the keep identifier type is user_alias. | ||
| */ | ||
| keepAliasIdValue?: { | ||
| /** | ||
| * The label of the user alias for the user to be kept. | ||
| */ | ||
| alias_label: string | ||
| /** | ||
| * The name of the user alias for the user to be kept. | ||
| */ | ||
| alias_name: string | ||
| } | ||
| } |
There was a problem hiding this comment.
The action definition adds keepIdPrioritization and previousIdPrioritization, but these fields are not present in mergeUsers/generated-types.ts and are not used when building the /users/merge request. As-is, these UI fields won’t be type-safe and won’t have any effect. Either regenerate generated-types and implement sending the prioritization values in the request body, or remove the fields.
| // Provide custom event data with valid identifiers | ||
| const customEventData = { | ||
| identifier_to_merge: { | ||
| external_id: 'user-to-merge' | ||
| }, | ||
| identifier_to_keep: { | ||
| external_id: 'user-to-keep' | ||
| } | ||
| } | ||
|
|
||
| nock(/.*/).persist().get(/.*/).reply(200) | ||
| nock(/.*/).persist().post(/.*/).reply(200) | ||
| nock(/.*/).persist().put(/.*/).reply(200) | ||
|
|
||
| const event = createTestEvent({ | ||
| properties: customEventData | ||
| }) | ||
|
|
||
| const responses = await testDestination.testAction(actionSlug, { | ||
| event: event, | ||
| mapping: customEventData, | ||
| settings: settingsData, | ||
| auth: undefined | ||
| }) |
There was a problem hiding this comment.
The snapshot test builds a mapping with identifier_to_merge / identifier_to_keep, which don’t exist as fields on the action (it defines previousIdType, previousIdValue, etc.). This will cause the snapshot to drift from the actual request payload produced by the action. Align the snapshot mapping shape with the action’s field schema (or change the action schema to match the mapping).
| } | ||
|
|
||
| if (!value) { | ||
| throw new PayloadValidationError(`ID value to ${label} value must be provided.`) |
There was a problem hiding this comment.
getMergeIdentifier throws ID value to ${label} value must be provided. which reads as duplicated/awkward (“value” twice). Since this is a user-facing validation error, consider rephrasing it (e.g., “ID value to ${label} must be provided.”) to be clearer.
| throw new PayloadValidationError(`ID value to ${label} value must be provided.`) | |
| throw new PayloadValidationError(`ID value to ${label} must be provided.`) |
| it('should throw error when identifier_to_merge has no valid identifier', async () => { | ||
| const event = createTestEvent({ | ||
| type: 'track' | ||
| }) | ||
|
|
||
| await expect( | ||
| testDestination.testAction('mergeUsers', { | ||
| event, | ||
| settings, | ||
| mapping: { | ||
| identifier_to_merge: {}, | ||
| identifier_to_keep: { | ||
| external_id: 'user-to-keep-123' | ||
| } | ||
| } | ||
| }) | ||
| ).rejects.toThrowError( | ||
| 'Identifier to Merge must specify one of: external_id, user_alias, braze_id, email, or phone.' | ||
| ) | ||
| }) |
There was a problem hiding this comment.
These error expectations don’t match the current implementation. With the action schema/util as written, missing/invalid identifiers will surface as PayloadValidationError from getMergeIdentifier (e.g., “ID value to merge … must be provided”), not “Identifier to Merge must specify one of …”. Update the validation/error messages (or the tests) so they assert the actual behavior.
| default: { | ||
| '@path': 'external_id' | ||
| } |
There was a problem hiding this comment.
previousIdType.default is set to { '@path': 'external_id' }, but @path expects a JSONPath. This will not default the identifier type to the literal string external_id and may resolve to undefined at runtime. Use a literal default (e.g., default: 'external_id') instead.
| default: { | |
| '@path': 'external_id' | |
| } | |
| default: 'external_id' |
| keepIdPrioritization: { | ||
| label: 'Rule Prioritization', | ||
| description: 'Rule determining which user to merge if multiple users are found.', | ||
| type: 'string', | ||
| choices: prioritizationChoices, | ||
| required: { | ||
| match: 'all', | ||
| conditions: [ | ||
| { | ||
| fieldKey: 'keepIdType', | ||
| operator: 'is', | ||
| value: ['email', 'phone'] | ||
| } | ||
| ] | ||
| }, | ||
| depends_on: { | ||
| match: 'all', | ||
| conditions: [ | ||
| { | ||
| fieldKey: 'keepIdType', | ||
| operator: 'is', | ||
| value: ['email', 'phone'] | ||
| } | ||
| ] | ||
| }, | ||
| default: 'identified' | ||
| }, | ||
| previousIdPrioritization: { | ||
| label: 'Rule Prioritization', | ||
| description: 'Rule determining which user to merge if multiple users are found.', | ||
| type: 'string', | ||
| choices: prioritizationChoices, | ||
| required: { | ||
| match: 'all', | ||
| conditions: [ | ||
| { | ||
| fieldKey: 'previousIdType', | ||
| operator: 'is', | ||
| value: ['email', 'phone'] | ||
| } | ||
| ] | ||
| }, | ||
| depends_on: { | ||
| match: 'all', | ||
| conditions: [ | ||
| { | ||
| fieldKey: 'previousIdType', | ||
| operator: 'is', | ||
| value: ['email', 'phone'] | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
The keepIdPrioritization / previousIdPrioritization conditional required/depends_on uses operator: 'is' with an array value (['email','phone']). The condition system (fields-to-jsonschema) expects a single scalar value, so this will generate an invalid/ineffective JSON Schema condition. Split this into separate conditions with match: 'any' (one for email, one for phone) so the UI/validation behaves as intended.
| identifier_to_merge: { | ||
| external_id: 'user-to-merge-456' | ||
| }, | ||
| identifier_to_keep: { | ||
| external_id: 'user-to-keep-123' | ||
| } |
There was a problem hiding this comment.
The tests map identifier_to_merge / identifier_to_keep, but the action’s declared fields are previousIdType, previousIdValue, keepIdType, etc. With the current action schema, these mappings won’t populate the payload consumed by mergeUsers() and the request body assertions won’t match. Update the action fields to accept identifier_to_merge/identifier_to_keep objects, or update the tests/mappings to use the action’s actual field keys.
| identifier_to_merge: { | |
| external_id: 'user-to-merge-456' | |
| }, | |
| identifier_to_keep: { | |
| external_id: 'user-to-keep-123' | |
| } | |
| previousIdType: 'external_id', | |
| previousIdValue: 'user-to-merge-456', | |
| keepIdType: 'external_id', | |
| keepIdValue: 'user-to-keep-123' |
| } | ||
| ] | ||
| }, | ||
| default: 'external_id' |
There was a problem hiding this comment.
keepIdValue.default is currently the literal string 'external_id', which would send external_id: "external_id" if the user doesn’t override the mapping. Given the action’s defaultSubscription: 'type = "alias"', this likely should default from the alias event’s $.userId (and previousIdValue from $.previousId) so out-of-the-box mappings produce real IDs.
| default: 'external_id' | |
| default: { | |
| '@path': '$.userId' | |
| } |
| choices: [ | ||
| { label: 'External ID', value: 'external_id' }, | ||
| { label: 'User Alias', value: 'user_alias' }, | ||
| { label: 'Braze ID', value: 'braze_id' }, |
There was a problem hiding this comment.
I think there's a type in the types.ts file you should also update.
This PR introduces a new action within the Braze cloud mode destination to perform a user merge between two users which exists in Braze. This implements the endpoint : https://www.braze.com/docs/api/endpoints/user_data/post_users_merge/#request-body
Testing
Testing has been completed locally
Security Review
type: 'password'