Skip to content

New Dub Destination #2865

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 16 commits into from
Closed

New Dub Destination #2865

wants to merge 16 commits into from

Conversation

devkiran
Copy link

@devkiran devkiran commented Apr 9, 2025 •
edited
Loading

Added new Dub Destination

Testing

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

@devkiran devkiran marked this pull request as ready for review April 9, 2025 10:45
@devkiran devkiran requested a review from a team as a code owner April 9, 2025 10:45
@joe-ayoub-segment joe-ayoub-segment self-assigned this Apr 29, 2025
@joe-ayoub-segment
Copy link
Contributor

hi @devkiran thank you for submitting a PR for a new Integration!
I'll review your PR and will provide feedback in the next 24h.

joe-ayoub-segment
joe-ayoub-segment requested changes Apr 30, 2025
View reviewed changes
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.

Thanks for the PR @devkiran .
Most of my suggestions related to default mappings. Please take a look and get back to me as soon as you can.
Thank you!
Joe

@joe-ayoub-segment
Copy link
Contributor

hi @devkiran please let me know when you are finished your changes so I can re-review.

@devkiran
Copy link
Author

devkiran commented May 5, 2025

@joe-ayoub-segment sorry for the late reply — I got caught up with some other work. I’ll be pushing the changes soon and will let you know once it’s done so you can re-review. Thanks for your patience!

@devkiran
Copy link
Author

devkiran commented May 6, 2025

@joe-ayoub-segment I've updated the PR based on your feedback. Feel free to let me know if you have any more suggestions. I appreciate your patience.

joe-ayoub-segment
joe-ayoub-segment reviewed May 6, 2025
View reviewed changes
packages/destination-actions/src/destinations/dub/trackLead/index.ts
const action: ActionDefinition<Settings, Payload> = {
title: 'Track a Lead',
description: 'Track a Lead for a Short Link.',
defaultSubscription: 'type = "track" and event = "Sign Up"',
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
defaultSubscription: 'type = "track" and event = "Sign Up"',
defaultSubscription: 'type = "track" and event = "Signed Up"',

@joe-ayoub-segment
Copy link
Contributor

Thanks for updating the PR @devkiran .
As mentioned above it would be good to connect so we can get the cookie grabbing code added. It's a fairly common thing but is tricky if you haven't done it before, so best to do it on a call.

We can also do a quick walk through the rest of the code as well.

I look forward to catching up!
Thanks,
Joe

@devkiran
Copy link
Author

devkiran commented May 8, 2025

@joe-ayoub-segment Hey! Sorry for the late response — I’m traveling for the next few days but will try to find some time if possible.

I'm wondering if it would be okay to publish a version without the cookie-grabbing code and extend it in a follow-up after the release.

@joe-ayoub-segment
Copy link
Contributor

Hi @devkiran I needed to update the types files, so instead of asking you to do it I opened a new PR here. #2865

We'll get this deployed in the next few days. I'll email you next steps after this is done.

We can work on the client side code to get the cookie value later.

@devkiran
Copy link
Author

@joe-ayoub-segment No problem, thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-ayoub-segment joe-ayoub-segment joe-ayoub-segment approved these changes

Assignees

@joe-ayoub-segment joe-ayoub-segment

Labels
deploy:push deploy:registration mode:cloud partner-eng-review (cloud) team:external
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devkiran @joe-ayoub-segment