Skip to content

Conversation

Vanessa-SSY
Copy link
Contributor

A summary of your pull request, including the what change you're making and why.

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.

label: "Advertiser ID",
description: "The StackAdapt advertiser ID to add the profile to. The value in this field field can also be overridden at the Action level via the Action field of the same name.",
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.

Should be required:false.

description: "The StackAdapt advertiser ID to add the profile to. The value in this field field can also be overridden at the Action level via the Action field of the same name.",
type: 'string',
required: true,
disabledInputMethods: ['literal', 'variable', 'function', 'freeform', 'enrichment'],
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this line. Customers can't map data from payloads anyway so you can delete this line.

const userId = payload.userId
const formattedExternalIds = `["${userId}"]`
const syncId = sha256hash(String(userId))
const advertiserId = settings.advertiser_id
if(!advertiserId) {
throw new InvalidAuthenticationError("Advertiser value must be provided in either the main Settings Advertiser field or at the Action level Advertiser field.")
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
throw new InvalidAuthenticationError("Advertiser value must be provided in either the main Settings Advertiser field or at the Action level Advertiser field.")
throw new InvalidAuthenticationError("To delete a user, the Advertiser field in Settings should be populated.")

@Vanessa-SSY
Copy link
Contributor Author

Hi @joe-ayoub-segment, thanks for your comments. Is there a way to use a dynamic dropdown instead of input box for advertiserID in under Settings?

@Vanessa-SSY
Copy link
Contributor Author

We will not enable onDelete if we can't have the dropdown for advertiserIds in a single place that can be referenced for both perform and onDelete. It's going to cause unnecessary confusion for our clients.

@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented Sep 1, 2025 via email

@Vanessa-SSY
Copy link
Contributor Author

Vanessa-SSY commented Sep 2, 2025

It's not in use by clients at the moment. The traffic is from our internal testing.
Could you help confirm if we can have the same dropdown for advertiserIds under Settings as the one we have now under Mappings UI? @joe-ayoub-segment

@joe-ayoub-segment
Copy link
Contributor

It's not in use by clients at the moment. The traffic is from our internal testing. Could you help confirm if we can have the same dropdown for advertiserIds under Settings as the one we have now under Mappings UI? @joe-ayoub-segment

Ah OK if no customers are using it, we can just keep the advertiserIds field in Settings / authentication, and then delete the advertiserIds in the Mappings UI.

@Vanessa-SSY
Copy link
Contributor Author

Vanessa-SSY commented Sep 2, 2025

It's not in use by clients at the moment. The traffic is from our internal testing. Could you help confirm if we can have the same dropdown for advertiserIds under Settings as the one we have now under Mappings UI? @joe-ayoub-segment

Ah OK if no customers are using it, we can just keep the advertiserIds field in Settings / authentication, and then delete the advertiserIds in the Mappings UI.

Yes, we can surely do that but more importantly we want to keep the UI intuitive for clients. I tried to have the dropdown for advertiser IDs in Settings / authentication but it doesn't seem to work. If we can get the dropdown working, we would still prefer having it in the Mappings UI instead of an input box in Settings

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