[ITE-146] Update Segment Engage Destination to handle onDelete events #2479
Conversation
[ITE-146] Update Segment Engage Destination to handle onDelete events
| @@ -12,6 +12,7 @@ paths = [ | |||
| regexes = ['''219-09-9999''', '''078-05-1120''', '''(9[0-9]{2}|666)-\d{2}-\d{4}'''] | |||
|
|
|||
| [[rules]] | |||
| id = "facebook_access_token" | |||
There was a problem hiding this comment.
hi @illumin04 - qq, why did you add the id for this Gitleaks rule?
There was a problem hiding this comment.
I added this when running into a local test error, and later found it is not necessary, I will remove this in the next commit.
There was a problem hiding this comment.
Hi @joe-ayoub-segment , if this is not added then I will keep getting this when trying to commit, Am I missing something, please let me know, thank you!
| userId: { | ||
| label: 'User ID', | ||
| description: 'The user ID to delete.', | ||
| type: 'string', | ||
| required: false | ||
| }, | ||
| anonymousId: { | ||
| label: 'Anonymous ID', | ||
| description: 'The anonymous ID to delete.', | ||
| type: 'string', | ||
| required: false | ||
| }, |
There was a problem hiding this comment.
hi @illumin04 ,
I noticed that the forwardProfile Action only has a single field named 'Segment User ID'. Should the same convention be followed by the new Delete Action?
| }, | ||
| advertiserId: { | ||
| label: 'Advertiser IDs', | ||
| description: 'Comma-separated list of advertiser IDs. If not provided, it will query the token info.', |
There was a problem hiding this comment.
hi @illumin04 I'm wondering if the description will be understood by customers. What does 'If not provided, it will query the token info.' mean?
| @@ -0,0 +1,73 @@ | |||
| import { GQL_ENDPOINT, EXTERNAL_PROVIDER } from '../functions' | |||
|
|
|||
| export async function onDelete(request: any, payload: any[]) { | |||
There was a problem hiding this comment.
The linter will probably complain about the use of 'any' here. Can we switch to payload: Payload[] ?
|
|
||
| const res_token = await request(GQL_ENDPOINT, { | ||
| body: JSON.stringify({ query: TokenQuery }), | ||
| throwHttpErrors: false |
There was a problem hiding this comment.
hi @illumin04 why was throwHttpErrors: false included here?
| } | ||
| }` | ||
|
|
||
| const res_token = await request(GQL_ENDPOINT, { |
There was a problem hiding this comment.
Hi @illumin04 It looks like this request provides a static value as TokenQuery. What is the need for this? Is the response identifying the customer account based on the Bearer in the header, and then providing some data?
There was a problem hiding this comment.
From what I was told we need to get the advertiser's ID for the deletion API, which needs to be fetched through this token query.
| }) | ||
|
|
||
| if (res_token.status !== 200) { | ||
| throw new Error('Failed to fetch advertiser information: ' + res_token.statusText) |
There was a problem hiding this comment.
Can we throw a different type of error (called IntegrationError) please?
| const advertiserNode = result_token.data?.tokenInfo?.scopesByAdvertiser?.nodes | ||
|
|
||
| if (!advertiserNode || advertiserNode.length === 0) { | ||
| throw new Error('No advertiser ID found.') |
There was a problem hiding this comment.
Same here. please throw an IntegrationError.
| } | ||
| }` | ||
|
|
||
| const res_token = await request(GQL_ENDPOINT, { |
There was a problem hiding this comment.
hi @illumin04 would you be OK defining a response type for this request please?
|
|
||
| export async function onDelete(request: any, payload: any[]) { | ||
| return (async () => { | ||
| const userId = payload[0].userId ?? payload[0].anonymousId |
There was a problem hiding this comment.
hi @illumin04 why are you only using one userId from the Payload array? The Payload array could contain userIds for different users.
If you prefer you could simply not implement the performBatch() function, and then you will be guaranteed to only receive a single Paryload per request. In which case you should redefine the onDelete function to handle a single Payload instead of Payload[]
There was a problem hiding this comment.
Hi @joe-ayoub-segment , sorry for the late respond, I was working on something else last two days. I will take a look at the review and follow your suggestions, might take some time, thanks for the help!
There was a problem hiding this comment.
Hi Joe, it is because this function is meant to be used for multiple userID and advertiser'd ID for future uses, but for now there can only be one userID, the userID and advertiser's ID in my understanding is one to many. I will figure a way to make this more clear.
|
hi @illumin04 thanks for raising this PR. I left a bunch of comments for you to review. Also the CI checks are failing. You can run the tests locally with the following command:
Also I'm wondering if you could add a description to the PR which includes the following please:
Kind regards, |
|
hi @illumin04 - just following up on this. Do you still want to progress this PR? Is there anything I can help with? |
Hi @joe-ayoub-segment , I am currently working on the stackadapt side of the feature, I will update you with these proof of it working once I finished that side. Once it is ready I will update this PR with all the recommended changes you left as well. Thanks again for your review. |
|
hi @illumin04 - just a friendly reminder about this ticket. Do you have a rough estimate for when you intend to get back to it? |
Hi @joe-ayoub-segment , sorry for not keeping you updated, I was working on other tasks and the stackadapt side of the feature last month, I hope to get back to you later this week after the testing on stackadapt side is done. Thanks for reaching out! |
A summary of your pull request, including the what change you're making and why.
Update Segment Engage Destination to handle onDelete events
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.
No additional testing is done due to API not fully ready.