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 2 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.

name: 'Entities Audience Membership Changed',
partnerAction: 'uploadConversionAdjustment',
mapping: {
// TODO: See if we use the original or the v2 action

Choose a reason for hiding this comment

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

Do we need to follow-up on this?

Copy link
Author

Choose a reason for hiding this comment

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

Yes we do 😄

@@ -77,7 +77,10 @@ const destination: AudienceDestinationDefinition<Settings, AudienceSettings> = {
full_audience_sync: false
},
async createAudience(request, createAudienceInput) {
const audienceName = createAudienceInput.settings?.audience_identifier === 'computation_key' ? createAudienceInput.personas?.computation_key : createAudienceInput.audienceName
const audienceName =
Copy link

@SethNuteTwilio SethNuteTwilio Jan 30, 2025

Choose a reason for hiding this comment

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

Nested ternary 😭 Nit, we could break this apart into two distinct ternaries to improve readability

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.57%. Comparing base (52537f3) to head (294588d).
Report is 15 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.57%   +0.10%     
==========================================
  Files        1036     1036              
  Lines       18771    18769       -2     
  Branches     3561     3562       +1     
==========================================
+ Hits        14729    14747      +18     
+ Misses       2844     2821      -23     
- Partials     1198     1201       +3     

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

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.

4 participants