Added Inleads Action Destination #1989
Conversation
github-actions
bot
added
deploy:push
deploy:registration
mode:cloud
team:external
labels
|
Thanks for the PR @nagendra-d - I'll schedule for review. |
|
hi @nagendra-d there are some failing CI checks / tests. Could you take a look please?
|
fixed inleads integration tests
Thanks @joe-ayoub-segment, I have fixed the failing test case |
| first_name: { | ||
| type: 'string', | ||
| required: false, | ||
| allowNull: true, |
There was a problem hiding this comment.
allowNull should allow the customer to null out the first_name. Is this what will happen?
There was a problem hiding this comment.
Yes, our API can handle even if this field is null
| type: 'string', | ||
| required: false, | ||
| description: "The user's name", | ||
| allowNull: true, |
There was a problem hiding this comment.
allowNull should allow the customer to null out the name. Is this what will happen?
There was a problem hiding this comment.
Yes, our API can handle even if this field is null
| required: true, | ||
| description: 'The External ID of the user', | ||
| label: 'User ID', | ||
| default: { '@path': '$.userId' } |
There was a problem hiding this comment.
Do you intend to allow anonymous user profile data to be collected? Or should all user data be for 'known' users?
There was a problem hiding this comment.
we do allow anonymous user data
| last_name: { | ||
| type: 'string', | ||
| required: false, | ||
| allowNull: true, |
There was a problem hiding this comment.
allowNull should allow the customer to null out the last_name. Is this what will happen?
There was a problem hiding this comment.
Yes, our API can handle even if this field is null
| '@if': { | ||
| exists: { '@path': '$.context.group_id' }, | ||
| then: { '@path': '$.context.group_id' }, | ||
| else: { '@path': '$.groupId' } |
There was a problem hiding this comment.
in a group() call, the groupId will always be at location $.groupId
| }, | ||
| created_at: { | ||
| type: 'string', | ||
| required: false, |
There was a problem hiding this comment.
You can add this if you like?
format:'date-time'
| } | ||
| }, | ||
| created_at: { | ||
| type: 'string', |
There was a problem hiding this comment.
consider adding this to ensure the value is an actual ISO8601 timestamp.
format:'date-time'
| }) | ||
|
|
||
| afterAll(() => { | ||
| nock.enableNetConnect() |
There was a problem hiding this comment.
no harm to remove this. nock should never be able to make real HTTPS requests.
| required: true, | ||
| description: 'The user id, to uniquely identify the user associated with the event', | ||
| label: 'User id', | ||
| default: { '@path': '$.userId' } |
There was a problem hiding this comment.
Do you intend to allow events for anonymous users to be collected?
| required: false, | ||
| description: 'The Traits of the track call', | ||
| label: 'Event Traits', | ||
| default: { '@path': '$.traits' } |
There was a problem hiding this comment.
traits in a track() call are usually located in $context.traits
| default: { '@path': '$.traits' } | |
| default: { '@path': '$.context.traits' } |
| timeZone: { '@path': '$.context.timezone' }, | ||
| }, | ||
| }, | ||
| anonymous_id: { |
There was a problem hiding this comment.
Ah I see you are allowing customer to collect anonymousId. You can ignore my comments about anonymous data then :)
| default: { '@path': '$.context.locale' } | ||
| }, | ||
| utc_time: { | ||
| type: 'string', |
There was a problem hiding this comment.
suggest adding:
format: 'date-time'
| }, | ||
| utc_time: { | ||
| type: 'string', | ||
| required: false, |
There was a problem hiding this comment.
you can make this required. It will always be there.
| userMeta: { | ||
| label: 'User Metadata', | ||
| type: 'object', | ||
| description: 'User metadata including IP, Location, etc.', |
There was a problem hiding this comment.
Just wondering why you didn't define properties for all of the sub fields such as latitude, country, etc?
Also, it may be worth adding this line to ensure the UI displays this field as a list of key:value pairs instead of an object.
defaultObjectUI: 'keyvalue',
There was a problem hiding this comment.
thanks for the suggestion to add defaultObjectUI, we require these fields as part of userMeta, so that it matches with our API
There was a problem hiding this comment.
hi @nagendra-d , regarding my first part of the comment:
Just wondering why you didn't define properties for all of the sub fields such as latitude, country, etc?
What I meant was, should you not be defining these sub fields with types?
properties: {
ip: {
label: 'IP Address',
description: 'The user's IP address,
type: 'string'
},
latitude: {
label: 'Latitude',
description: 'The latitude coordinates for the user. For example 123.455',
type: 'string'
}
etc...
},
You did this with the utm object field below.
There was a problem hiding this comment.
oh, I got it now, The sub fields are from different context ip, location, os, etc... , I wasn't sure having the approach as utm would work here, and was having trouble with accessing values that way
There was a problem hiding this comment.
added properties to userMeta object
|
hi @nagendra-d - nicely done with this PR! I provided some feedback. I noticed that the CI checks are failing. I don't think it is because of your code. Could you rebase from main branch please? I think it should fix the issue. |
fix [Inleads-3266] fixed test case and update date time formats
|
Hi @joe-ayoub-segment , Thanks for the suggestions, I have updated the code as per your suggestions for date and triats Regarding Thanks |
| export interface Payload { | ||
| /** | ||
| * Segment Event Payload | ||
| */ | ||
| segmentEventData: { | ||
| [k: string]: unknown | ||
| } | ||
| } |
There was a problem hiding this comment.
Hi again @nagendra-d ,
One thing I missed from my initial review.
Usually a separate generated-types.ts file is generated for each Action, as well as the main index.ts file.
How was this generated-types.ts file created? Did you run the ./bin/run generate:types command? Did you edit the file in any way manually?
There was a problem hiding this comment.
Hi @joe-ayoub-segment ,
I have manually created this file, is it mandatory to use the command to generate types, if so I will do and update,
when I checked some other destinations, I saw this type of implementation
There was a problem hiding this comment.
Ah OK - yes please use the command to generate these files.
./bin/run generate:types
|
Hi @nagendra-d thanks for applying those changes. Just 2 more comments to respond to and we should be good. |
fix [inleads-3267] Updated generated types
Hi @joe-ayoub-segment , Thanks,
Thanks, |
fix [inleads-3268] Added poprerties to define userMeta
|
Hi @joe-ayoub-segment , Thanks for merging the changes, when can we access / start testing private beta url |
|
hi @nagendra-d this PR has been deployed. I'll email you next steps. |
A new action destination for Inleads. It provides two actions:
Send Identify which maps Identify events by default
Send Track which maps TRACK events by default.
The destination introduces actions for Segment events.
Testing
Added unit tests for new functionality
Tested end-to-end using the local server
[Segmenters] Tested in the staging environment