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

fest(Segment): add all new endpoints to segment #1459

Conversation

rubyjohn93
Copy link
Contributor

@rubyjohn93 rubyjohn93 commented Aug 1, 2023

We have added 3 new endpoints to talon-integration. This PR aims at adding these 3 endpoints to segment.

The new endpoints mainly support a new query param skipNonExistingAttributes compared to their previous versions.

This flag indicates whether to skip non-existing attributes.
If true, the non-existing attributes are skipped and a 400 error is not returned.
If false, a 400 error is returned in case of non-existing attributes.

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.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
    Please find the attached screenshots for the same. I have added screenshots of Action Tester with and without the skipNonExistingAttributes query param and the talon-ui after the API succeeds.
  • Track Events Endpoint:
skipNonExistingAttributes=false skipNonExistingAttributes=true talon-ui
  • Update Customer Profile Endpoint:
skipNonExistingAttributes=false skipNonExistingAttributes=true Screenshot 2023-08-16 at 16 19 37 Screenshot 2023-08-16 at 16 19 46
  • Update Customer Session Endpoint:
skipNonExistingAttributes=false skipNonExistingAttributes=true Screenshot 2023-08-16 at 16 50 02
  • [Segmenters] Tested in the staging environment

@joe-ayoub-segment
Copy link
Contributor

Hi @rubyjohn93 thanks for raising this PR. I'll schedule for review.

@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented Aug 8, 2023

Thanks for raising this PR @rubyjohn93 ,

I'd like to have a little more context about these changes so I can be more informed when reviewing the PR.
I'm also interested in doing some testing so we can be more certain that the new Actions behave as expected. Since this Destination is already publicly available we won't be able to test these new Actions in Production prior to customers getting access to them.

Would you be OK scheduling a call with me via this Calendly link please?
http://calendly.com/joe_ayoub/integration-workshop

Best regads,
Joe

@joe-ayoub-segment
Copy link
Contributor

hi @rubyjohn93 , Alex and Kristina,

Thanks for taking the time to catch up just now.
During the call we discussed the following:

  1. Adding new versions of Actions is not the most desirable way to migrate to a new API. However after looking at the code in the new Actions it seems like we've no choice. It would not be possible to upgrade the existing Actions to new API versions without making the customer experience more complicated.
  2. We discussed adding Presets. However since the existing Actions don't have defaultSubscriptions configured, adding Presets at this time might confuse customers. For example: Multiple Actions in the Talon One Destination could map to Segment track() events. Because of this we decided against using Presets.
  3. We agreed that we should somehow inform customers that certain Actions are 'deprecated'. We agreed to do this by adding the work [Deprecated] to the start of the Action Description, and we'd also explain which Action to use instead.
  4. We agreed that we'd add a new paragraph to the Segment docs to explain which Actions are deprecated, and which ones should be used going forward.
  5. Finally, it would be great if you could include some more evidence of testing in the PR. Maybe some screenshots of payloads being sent to the "Action Tester" tool, and then the resulting impact in the Talon One UI.

Thanks again for your time,
Joe

@joe-ayoub-segment
Copy link
Contributor

Hi @rubyjohn93 , Alex and Krishna,

Please let me know once you've updated the PR based on our discussion, and that you are happy for me to rereview.

Best regards,
Joe

@rubyjohn93
Copy link
Contributor Author

Hi @rubyjohn93 , Alex and Krishna,

Please let me know once you've updated the PR based on our discussion, and that you are happy for me to rereview.

Best regards, Joe

Hi @joe-ayoub-segment
We will make these changes and get back to you.
Sorry about the delay.

Copy link
Contributor

@kkupreeva kkupreeva left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@vignesh-giridharan vignesh-giridharan left a comment

Choose a reason for hiding this comment

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

Just some minor tweaks to the phrasing....

@joe-ayoub-segment
Copy link
Contributor

Hi @vignesh-giridharan and @rubyjohn93 , I can see that you're still working on this so I'll hold off on approving it. Please message me directly here to let me know when you are done :)

@rubyjohn93
Copy link
Contributor Author

Hi @vignesh-giridharan and @rubyjohn93 , I can see that you're still working on this so I'll hold off on approving it. Please message me directly here to let me know when you are done :)

@joe-ayoub-segment Yes, @vignesh-giridharan is from our docs team and we are working on making the mappings name in segment and our docs the same, so that the customers can easily relate both. I'll make the proposed changes and once everything is finalised I'll ping you here.

Copy link

@vignesh-giridharan vignesh-giridharan left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, @rubyjohn93! Also, hi, Joe! 👋🏽

@rubyjohn93
Copy link
Contributor Author

@joe-ayoub-segment
A small update on this PR.

I have completed the changes you proposed, in the scope of this repo. I incorporated a few suggestion from our docs team as well.
But regarding your other comment for docs repo

We agreed that we'd add a new paragraph to the Segment docs to explain which Actions are deprecated, and which ones should be used going forward.

This change will be picked up by our docs team later in their pipeline and hopefully they will raise a PR by next week.

I would like to request you to wait till then so that we can make avail both PRs ready for your review.
Thank you!
cc: @vignesh-giridharan @kkupreeva @gotgelf

Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment left a comment

Choose a reason for hiding this comment

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

Hi @rubyjohn93 , few more minor changes.

Comment on lines 10 to 12
description: `This records a custom event in Talon.One.

**Important:** This endpoint is deprecated. Use the current **Track event** endpoint instead.`,
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
description: `This records a custom event in Talon.One.
**Important:** This endpoint is deprecated. Use the current **Track event** endpoint instead.`,
description: `This records a custom event in Talon.One. **Important:** This Action is deprecated. Use the **Track event** Action instead.`,

Comment on lines 8 to 10
description: `This updates attributes and audiences for a single customer profile.

**Important:** This endpoint is deprecated. Use the current **Update customer profile** endpoint instead.`,
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
description: `This updates attributes and audiences for a single customer profile.
**Important:** This endpoint is deprecated. Use the current **Update customer profile** endpoint instead.`,
description: `This updates attributes and audiences for a single customer profile. **Important:** This Action is deprecated. Use the **Update customer profile** Action instead.`,

Comment on lines 8 to 10
description: `You do not have to create attributes or audiences before using this endpoint.

**Important:** This endpoint is deprecated. Use the current **Update customer profile** endpoint instead.`,
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
description: `You do not have to create attributes or audiences before using this endpoint.
**Important:** This endpoint is deprecated. Use the current **Update customer profile** endpoint instead.`,
description: `You do not have to create attributes or audiences before using this Action. **Important:** This Action is deprecated. Use the **Update customer profile** Action instead.`,

Comment on lines 8 to 10
description: `This updates a customer session.

**Important:** This endpoint is deprecated. Use the current **Update customer session** endpoint instead.`,
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
description: `This updates a customer session.
**Important:** This endpoint is deprecated. Use the current **Update customer session** endpoint instead.`,
description: `This updates a customer session. **Important:** This Action is deprecated. Use the **Update customer session** Action instead.`,

const action: ActionDefinition<Settings, Payload> = {
title: 'Update customer profile',
description:
'This updates attributes and audiences for a single customer profile. Create all the required attributes and audiences before using this endpoint.',
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
'This updates attributes and audiences for a single customer profile. Create all the required attributes and audiences before using this endpoint.',
'This updates attributes and audiences for a single customer profile. Create all the required attributes and audiences before using this Action.',


const action: ActionDefinition<Settings, Payload> = {
title: 'Update customer session',
description: 'This updates a customer session. Create all the required attributes before using this endpoint.',
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
description: 'This updates a customer session. Create all the required attributes before using this endpoint.',
description: 'This updates a customer session. Create all the required attributes before using this Action.',

@joe-ayoub-segment
Copy link
Contributor

Internal note:
I verified that it's OK to rename the title and description of an Action. There was a suspicion that it might cause issues in Production. Test images from Staging attached.

talon one staging test 2
talon one staging test
talon one test 3
talon one test 3b
talon one test 3c

@rubyjohn93
Copy link
Contributor Author

Hi @rubyjohn93 , few more minor changes.

Hi @joe-ayoub-segment,

Thank you for the review. Made these changes.
Now all the descriptions are in single line and changed endpoint to Action.

@joe-ayoub-segment
Copy link
Contributor

hi @rubyjohn93 thanks for making the changes. I've approved the PR. Waiting on CI checks to pass. If they do, we plan to deploy Thursday 24th August.

@rubyjohn93
Copy link
Contributor Author

Hi @joe-ayoub-segment
Thank you!
Seems like one test failed due to some authentication issue. Can you please check when you have time.

@joe-ayoub-segment
Copy link
Contributor

hi @rubyjohn93 , nope we're good. The Synk test fails unless it's an internal PR. We're good to deploy Thursday.

@rubyjohn93
Copy link
Contributor Author

hi @rubyjohn93 , nope we're good. The Synk test fails unless it's an internal PR. We're good to deploy Thursday.

Thank you 👍
As I mentioned earlier, the docs PR will be raised by next week.
Thanks again for your support throughout the process 🙇‍♀️

@joe-ayoub-segment joe-ayoub-segment merged commit 7dd10cf into segmentio:main Aug 24, 2023
@joe-ayoub-segment
Copy link
Contributor

Hi @rubyjohn93 this PR has been deployed.

wpride pushed a commit to wpride/action-destinations that referenced this pull request Oct 12, 2023
* add create audience action

* try enable CI

* try fix

* return ci how it was

* add updateAudience, deleteAudience, trackEvent

* check workflow

* fix tests

* fix tests

* fix description

* improve test

* fix labels and descriptions

* generate types

* return workflows how it was before

* change destination name Talon One -> Talon.One

* review

* new files

* added 3 endpoints

* added generated files

* implemented 3 endppoints

* tests

* fixed tests, added right json tags

* updated models

* updated test models

* update models

* updated models

* tests generated

* updated models

* tests generated

* Revert "Merge pull request #2 from talon-one/sc-7503-add-destination-endpoints-to-work-with-customer"

This reverts commit d391a8e, reversing
changes made to 6871863.

* Revert "Revert "Merge pull request #2 from talon-one/sc-7503-add-destination-endpoints-to-work-with-customer""

This reverts commit b7e7d31.

* added more endpoints and fixed some minor things

* deleted an unnecessarily added file

* updated snapshots, thank you @jinapark202

* add updateCustomerProfileV2 action

* add default mapping

* integrationId -> integration ID

* new update customer session destination for talon.one

* fixed tests, review

* added choices for attributesInfo

* reused attributesInfo constant from t1-properties.ts

* updated test by cardItems array

* updated test by cardItems array

* generated types

* fixed unit tests

* fixed a typo

* review

* deleted destination to update multiple attributes fro multiple customer profiles

* Add new talon-integration endpoints to segment

* Mark old endpoints as deprecated in title and description

* Fix PR comments from docs team

* Fix casing

* Fix PR comments

---------

Co-authored-by: kkupreeva <kupreeva@talon.one>
Co-authored-by: Mikhail Gunkov <painter@3lines.club>
Co-authored-by: Michael Gunkoff <54396113+kaatinga@users.noreply.github.com>
Co-authored-by: Kristina Kupreeva <86834674+kkupreeva@users.noreply.github.com>
Co-authored-by: Kristina Kupreeva <kristine.kupreeva@gmail.com>
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.

5 participants