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

[LAB-1682] Create presets for each standard audience action destination #2714

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

Conversation

drew-thompson
Copy link

This PR adds presets (of the specificEvent kind) for each of the standard (not full sync) audience action destinations in our catalog. These presets will be used in filtering out actions which are extraneous to event emitter creation, leaving only the ones used for syncing profiles.

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
  • [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.

SethNuteTwilio
SethNuteTwilio previously approved these changes Jan 30, 2025
Copy link

@SethNuteTwilio SethNuteTwilio 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
Contributor

@rvadera12 rvadera12 left a comment

Choose a reason for hiding this comment

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

From my understanding, the new actions will be visible to all users, regardless of whether they are using the entities flow or not. Given that adding these presets essentially creates new actions with predefined property values, it might lead to confusion among users who don’t need these actions or aren’t familiar with the concept.

Would it be possible to hide these new actions from the UI for all customers to avoid adding unnecessary complexity? Alternatively, perhaps we could include a more detailed explanation of these actions in the docs or setup flow, so users understand their purpose and context better.

This would help streamline the user experience and prevent any potential confusion when these actions appear in the UI.

Just considering the user flow, we've received numerous questions and feedback in the past regarding multiple actions that essentially perform the same function, which has led to confusion.

CC: @smultani

@drew-thompson
Copy link
Author

From my understanding, the new actions will be visible to all users, regardless of whether they are using the entities flow or not. Given that adding these presets essentially creates new actions with predefined property values, it might lead to confusion among users who don’t need these actions or aren’t familiar with the concept.

Would it be possible to hide these new actions from the UI for all customers to avoid adding unnecessary complexity? Alternatively, perhaps we could include a more detailed explanation of these actions in the docs or setup flow, so users understand their purpose and context better.

This would help streamline the user experience and prevent any potential confusion when these actions appear in the UI.

Just considering the user flow, we've received numerous questions and feedback in the past regarding multiple actions that essentially perform the same function, which has led to confusion.

CC: @smultani

Hi @rvadera12! We're not adding any new actions, just presets that have the field type: 'specificEvent', which is something recently built for this specific purpose. As a quick example, customer.io has many of these presets defined, all pointing to the trackEvent action, and each of those presets doesn't cause a new action to be presented to customers.

You can see the PR which added specificEvent (which makes it an internal mechanism) in #1450.

@rvadera12
Copy link
Contributor

From my understanding, the new actions will be visible to all users, regardless of whether they are using the entities flow or not. Given that adding these presets essentially creates new actions with predefined property values, it might lead to confusion among users who don’t need these actions or aren’t familiar with the concept.
Would it be possible to hide these new actions from the UI for all customers to avoid adding unnecessary complexity? Alternatively, perhaps we could include a more detailed explanation of these actions in the docs or setup flow, so users understand their purpose and context better.
This would help streamline the user experience and prevent any potential confusion when these actions appear in the UI.
Just considering the user flow, we've received numerous questions and feedback in the past regarding multiple actions that essentially perform the same function, which has led to confusion.
CC: @smultani

Hi @rvadera12! We're not adding any new actions, just presets that have the field type: 'specificEvent', which is something recently built for this specific purpose. As a quick example, customer.io has many of these presets defined, all pointing to the trackEvent action, and each of those presets doesn't cause a new action to be presented to customers.

You can see the PR which added specificEvent (which makes it an internal mechanism) in #1450.

Got it! Thanks for clarifying, I didn't know we had the capability to hide presets if it is an engage flow.

@drew-thompson drew-thompson marked this pull request as ready for review February 4, 2025 18:42
@drew-thompson drew-thompson requested a review from a team as a code owner February 4, 2025 18:42
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.50%. Comparing base (52537f3) to head (964c8b0).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
.../src/destinations/dynamic-yield-audiences/index.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2714      +/-   ##
==========================================
+ Coverage   78.46%   78.50%   +0.03%     
==========================================
  Files        1036     1036              
  Lines       18771    18801      +30     
  Branches     3561     3576      +15     
==========================================
+ Hits        14729    14760      +31     
+ Misses       2844     2836       -8     
- Partials     1198     1205       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marinhero
marinhero previously approved these changes Feb 6, 2025
@itsarijitray
Copy link
Contributor

@drew-thompson @SethNuteTwilio
Some validation steps are failing in the CI. Can you look into this?
Also, why are you adding properties node to presets?

Copy link
Contributor

@marinhero marinhero left a comment

Choose a reason for hiding this comment

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

We've reached internal consensus and you'll be deployed in 2/18. Thank you for your patience.

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