Skip to content
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

[Antavo ] New Antavo Destination #2619

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bbantavo
Copy link

Added new Antavo Destination

Testing

@bbantavo bbantavo requested a review from a team as a code owner November 29, 2024 15:58
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@joe-ayoub-segment
Copy link
Contributor

Hi @bbantavo thanks for raising this PR for a new Integration.

The code looks pretty straightforward.

Instead of pointing out all the issues I'm going to ask you some broad questions, and based on your answers I might end up making some code changes myself before merging and deploying.

  1. Do you want to capture user events and user profile data for 'anonymous' users, as well as 'known' users? Or do you just want to capture data for 'known' users. A 'known' user is a user who has a userId which has been assigned to them by the customer.
  2. Do you want the event and profile Mappings to be automatically enabled when the Customer creates an instance of the Destination on the Segment platform (and provides auth credentials)?
  3. Segment has recommended patterns for collecting user profile data. Customers tend to adhere to these patterns. Would you like me to update your code so that the 'Customer properties' field automatically maps to where the user profile data is likely to be?
  4. Segment has a specification for ecommerce events which customers tend to adhere to. Does your platform have something similar? If so, it might be worth updating your code so that things like 'purchase amount', 'price', 'quantity' are automatically mapped?

Finally - would you please run the yarn types command, and commit the result? This should stop the 'Validate' CI check from failing.

Thank you!
Joe

@bbantavo
Copy link
Author

bbantavo commented Dec 5, 2024

Hi @joe-ayoub-segment !

Here are our answers:

  1. We only want to capture events for known users
  2. Mappings should be added manually, in order to set the field mapping properly
  3. We'd like that, thanks
  4. Our customer have their own naming conventions for events. They can create custom events or redefine the built-in events, so we would like to keep the code as it is

We've also ran and pushed the results of the yarn types command

Comment on lines +29 to +31
// Return a request that tests/validates the user's credentials.
// If you do not have a way to validate the authentication fields safely,
// you can remove the `testAuthentication` function, though discouraged.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bbantavo is there a request you can make here to validate that the api_key is valid?

Comment on lines +35 to +40
onDelete: async () => {
return true
// Return a request that performs a GDPR delete for the provided Segment userId or anonymousId
// provided in the payload. If your destination does not support GDPR deletion you should not
// implement this function and should remove it completely.
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bbantavo
Please remove the onDelete code if you don't intend to use it.

fields: {
stack: {
label: 'Stack',
description: 'Antavo stack',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @bbantavo please indicate where the customer can get this value from.

},
api_key: {
label: 'API Key',
description: 'Antavo brand API key',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @bbantavo please indicate where the customer can get this value from.

Comment on lines +10 to +15
customer: {
label: 'Customer ID',
description: 'User ID, selected in Antavo as customer identifier',
type: 'string',
required: true
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a default mapping so that userId is automatically mapped.

Suggested change
customer: {
label: 'Customer ID',
description: 'User ID, selected in Antavo as customer identifier',
type: 'string',
required: true
},
customer: {
label: 'Customer ID',
description: 'User ID, selected in Antavo as customer identifier',
type: 'string',
required: true,
default: {
'@path': '$.userId'
}
},

Comment on lines +72 to +81
default: {
first_name: '',
last_name: '',
email: '',
birth_date: '',
gender: '',
language: '',
phone: '',
mobile_phone: '',
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding default mappings as per Segment identify spec https://segment.com/docs/connections/spec/identify/

Suggested change
default: {
first_name: '',
last_name: '',
email: '',
birth_date: '',
gender: '',
language: '',
phone: '',
mobile_phone: '',
}
default: {
first_name: {
'@path': '$.traits.first_name'
},
last_name: {
'@path': '$.traits.last_name'
},
email: {
'@path': '$.traits.email'
},
birth_date: {
'@path': '$.traits.birthday'
},
gender: {
'@path': '$.traits.gender'
},
language: {
'@path': '$.traits.language'
},
phone: {
'@path': '$.traits.phone'
},
mobile_phone: {
'@path': '$.traits.mobile_phone'
}
}

label: 'Customer ID',
description: 'User ID, selected in Antavo as customer identifier',
type: 'string',
required: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
required: true
required: true,
default: {
'@path': '$.userId'
}

Comment on lines +28 to +33
data: {
label: 'Event data',
description: 'Event data',
type: 'object',
required: false
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually event data would be contained in this location in a Segment track() event. Up to you if you want to collect and send it.

default: {
'@path': '$.properties'
}

@joe-ayoub-segment
Copy link
Contributor

hi @bbantavo I added some more comments - mostly default mappings for fields in your 2 Actions.
Please also take a look at the testAuthencication() function and let me know if you can implement it or not.
Finally, would you be able to write a couple of unit tests to validate that the payloads being sent to your platform are as expected please (for each Action)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants