Skip to content

[PINT-2637] - Several new features#3826

Open
jumpingGrendel wants to merge 5 commits into
segmentio:mainfrom
urbanairship:PINT-2637
Open

[PINT-2637] - Several new features#3826
jumpingGrendel wants to merge 5 commits into
segmentio:mainfrom
urbanairship:PINT-2637

Conversation

@jumpingGrendel

Copy link
Copy Markdown
Contributor
  • Add channel_id + channel_type as optional audience selectors across customEvents, setAttributes, and manageTags; channel_type is optional and defaults to the generic channel key (Airship resolves the type server-side); also fixes endpoint routing so both selector types correctly use /api/channels/attributes and /api/channels/tags
  • Support objects and arrays as attribute values in setAttributes; JSON attribute keys are automatically suffixed with #default as the instance ID when one isn't already supplied (Airship's required key format for JSON attributes)
  • Customize User-Agent header to outgoing requests for attribution in Airship-side logging
  • Add BROWSER env var support to ./bin/run serve so developers can override which browser opens the actions tester (e.g. BROWSER=firefox)

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.

Security Review

Please ensure sensitive data is properly protected in your integration.

  • Reviewed all field definitions for sensitive data (API keys, tokens, passwords, client secrets) and confirmed they use type: 'password'

New Destination Checklist

  • Extracted all action API versions to verioning-info.ts file. example

this.log(
chalk.greenBright`Visit https://app.segment.com/dev-center/actions-tester to preview your integration.`
)
void open('https://app.segment.com/dev-center/actions-tester')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hi @jumpingGrendel - please remove the change to this file, as it's not part of the airship integration.

description: 'Create in the Airship Go dashboard in Settings->Partner Integrations->Segment',
type: 'password',
// default: process.env.DEFAULT_ACCESS_TOKEN,
default: process.env.DEFAULT_ACCESS_TOKEN,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jumpingGrendel I think you left this in by mistake. Can you remove please?

description: 'The App Key identifies the Airship Project to which API requests are made.',
type: 'password',
// default: process.env.DEFAULT_APP_KEY,
default: process.env.DEFAULT_APP_KEY,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jumpingGrendel I think you left this in by mistake. Can you remove please?

},
channel_type: {
label: 'Channel Type',
description: 'The Airship audience key for the channel type (e.g. android_channel, ios_channel, amazon_channel, web_channel). If omitted, the generic channel key is used and Airship will resolve the type, which may introduce a slight delay.',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jumpingGrendel we can turn this into an enum if you like?

Just add:

choices: [
  { label: 'Android Channel', value: 'android_channel' },
  { label: 'iOS Channel', value: 'ios_channel' },
  { label: 'Amazon Channel', value: 'amazon_channel' },
  { label: 'Web Channel', value: 'web_channel' }
]

This will only allow one of those 4 values to be passed. If the user passes something else it will throw a PayloadValidationError.

Comment on lines +25 to +30
channel_type: {
label: 'Channel Type',
description: 'The Airship audience key for the channel type (e.g. android_channel, ios_channel, amazon_channel). If omitted, the generic channel key is used and Airship will resolve the type, which may introduce a slight delay.',
type: 'string',
required: false
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jumpingGrendel we can turn this into an enum if you like?

Just add:

choices: [
  { label: 'Android Channel', value: 'android_channel' },
  { label: 'iOS Channel', value: 'ios_channel' },
  { label: 'Amazon Channel', value: 'amazon_channel' }
]

This will only allow one of those 4 values to be passed. If the user passes something else it will throw a PayloadValidationError.

Comment on lines +26 to +31
channel_type: {
label: 'Channel Type',
description: 'The Airship audience key for the channel type (e.g. android_channel, ios_channel, amazon_channel, web_channel). If omitted, the generic channel key is used and Airship will resolve the type, which may introduce a slight delay.',
type: 'string',
required: false
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jumpingGrendel we can turn this into an enum if you like?

Just add:

choices: [
{ label: 'Android Channel', value: 'android_channel' },
{ label: 'iOS Channel', value: 'ios_channel' },
{ label: 'Amazon Channel', value: 'amazon_channel' },
{ label: 'Web Channel', value: 'web_channel' }
]

This will only allow one of those 4 values to be passed. If the user passes something else it will throw a PayloadValidationError.

Accept: 'application/vnd.urbanairship+json; version=3',
'Content-Type': 'application/json'
'Content-Type': 'application/json',
'User-Agent': `PartnerIntegrations/Segment (${settings.app_key})`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jumpingGrendel can you include this new header in a unit test please? Just want to make sure it's completely deliberate.

@joe-ayoub-segment

Copy link
Copy Markdown
Contributor

Hi @jumpingGrendel
I reviewed the PR and added some comments and a couple of minor requests. But overall it looks good to me!
Are you able to attach proof of testing (a couple of screenshots) to the PR description please?
Best regards,
Joe

…tently — maps ios/android/amazon/web → *_channel, falls back to generic channel when omitted/unrecognized

* gave channel_type a default: $.context.device.type and kept it a free string
* added a User-Agent header unit test
* added batch test for customEvents (was missing)
* dropped the BROWSER/serve.ts commit
* commented index.ts env-var defaults
@jumpingGrendel

Copy link
Copy Markdown
Contributor Author

Hi @jumpingGrendel I reviewed the PR and added some comments and a couple of minor requests. But overall it looks good to me! Are you able to attach proof of testing (a couple of screenshots) to the PR description please? Best regards, Joe

Hello @joe-ayoub-segment thanks for the review. I removed the change to serve (I hope Segment will consider adding this!) as well as the default env-vars as per your requests. I don't want to make channel_type an enum because

  • the assumption is, if a customer has access to the channel id, chances are they have access to the type in the precise format needed (via the source integration)
  • it will fall back to generic channel if it is missing or doesn't match
  • I gave it a default mapping

I added a unit test for the userAgent change, as well as a couple other test updates, as well as a bit of refactoring to make it make a bit more sense. Also, bunch of screenshots of the testing. I hope that covers it!

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