-
Notifications
You must be signed in to change notification settings - Fork 224
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
feat: added Contentstack's destination-action. #2016
Conversation
Hi @hanoak20 thanks for raising this PR. I'm out of office until 16-May and will review the PR then. If you have an urgent query please email partner-support@segment.com Kind regards, |
packages/destination-actions/src/destinations/contentstack/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
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.
You'll need to correct the Custom Attributes Sync description. Adding the requested extendRequest
is optional, but I believe you'll find that it cleans up your code a good deal.
One thing you'll need to add eventually are some unit tests. But considering you primarily want this deployed to test in private, I'll go ahead and mark this ready for release so this will be included in the next deployment.
The next deploy is scheduled on Tuesday.
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/utils.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/utils.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/utils.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/contentstack/customAttributesSync/index.ts
Outdated
Show resolved
Hide resolved
const d = data as Data | ||
|
||
const { rawData } = d | ||
const customTraitsOfSegment = Object.keys(rawData?.traits || {}) | ||
|
||
const personalizeAttributesData = (await fetchAllAttributes(request)).map( | ||
(attribute: personalizeAttributes) => attribute?.key | ||
) |
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.
Can we do this in the standard way instead of accessing the rawData unnecessarily?
We only allow use of rawData as a workaround as it breaks a platform standard convention and builds up technical debt.
perform: async (request, {payoad}) => {
Simply add a field called traits.
{
traits: {
type: "object",
default: {"@path": "$.traits"},
label: "User traits",
description: "User Profile traits to send to Contentstack",
required: true
}
}
Then the code for customTraitsOfSegment will be a lot simpler:
const attributesToCreate = Object.keys(payload.traits).filter(
(trait: string) => !personalizeAttributesData.includes(trait)
)
method: 'patch', | ||
json: rawData?.traits, | ||
headers: { | ||
'x-cs-eclipse-user-uid': rawData?.userId || '' |
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.
userId should also be a field.
{
userId: {
type: "string",
default: {"@path": "$.userId"},
label: "User ID",
description: "ID for the user",
required: false
}
}
you will then access it like this:
'x-cs-eclipse-user-uid': payload.userId ?? ''
|
||
return request(`${PERSONALIZE_EDGE_API_URL}/user-attributes`, { | ||
method: 'patch', | ||
json: rawData?.traits, |
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.
json: rawData?.traits, | |
json: payload.traits, |
export interface Data { | ||
settings: Settings | ||
payload: Payload | ||
rawData?: { | ||
traits: Record<string, unknown> | ||
userId: string | ||
} | ||
auth: { | ||
accessToken: string | ||
} | ||
} |
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 shouldn't be needed if you use regular fields for accessing data.
const action: ActionDefinition<Settings, Payload> = { | ||
title: 'Custom Attributes Sync', | ||
description: | ||
'This action is used to sync custom attributes to your contentstack experience, and is only compatible with Identify events.', |
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 action is used to sync custom attributes to your contentstack experience, and is only compatible with Identify events.', | |
'Sync Custom Attributes to your Contentstack Experience.', |
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 @joe-ayoub-segment Just to let you know that, the description was like this only even previously. We updated as per Thomas's comments only.
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 @hanoak20 thanks again for raising this PR.
I'm just back from PTO and noticed that there are some things that we do need to change before we deploy this code.
I've provided suggestions. These changes are super simple but will reduce the potential for technical debt on our platform.
Kind regards,
Joe
cc @tcgilbert
hi @hanoak20 - PR deployed. Please confirm the change is good ! |
Added Contentstack's Destination Action.
Description:
Added an action called "customAttributesSync" to sync custom attributes, computed attributes, and audience into Contentstack's Personalization. All of these syncs work on 'identify' events.