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

feat: Added support for custom domain #184

Merged

Conversation

ngarg-mparticle
Copy link
Contributor

Summary

@@ -67,6 +68,7 @@ class UrbanAirshipConfiguration(settings: Map<String, String>) {
private const val KEY_APP_KEY = "applicationKey"
private const val KEY_APP_SECRET = "applicationSecret"
private const val KEY_DOMAIN = "domain"
private const val KEY_INITIAL_CONFIG_URL = "initialConfigUrl"

Choose a reason for hiding this comment

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

Configuration setting key will be added to align with UI description.

Suggested change
private const val KEY_INITIAL_CONFIG_URL = "initialConfigUrl"
private const val KEY_CUSTOM_DOMAIN_PROXY_URL = "customDomainProxyUrl"

Comment on lines 100 to 102
if (settings.containsKey(KEY_INITIAL_CONFIG_URL)) {
initialConfigUrl = settings[KEY_INITIAL_CONFIG_URL]
}

Choose a reason for hiding this comment

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

Based on the configuration setting key change..

Suggested change
if (settings.containsKey(KEY_INITIAL_CONFIG_URL)) {
initialConfigUrl = settings[KEY_INITIAL_CONFIG_URL]
}
if (settings.containsKey(KEY_CUSTOM_DOMAIN_PROXY_URL)) {
initialConfigUrl = settings[KEY_CUSTOM_DOMAIN_PROXY_URL]
}

@@ -10,6 +10,7 @@ class UrbanAirshipConfiguration(settings: Map<String, String>) {
val applicationKey: String?
val applicationSecret: String?
val domain: String?
val initialConfigUrl: String?

Choose a reason for hiding this comment

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

Probably want to rename this property as well.

Suggested change
val initialConfigUrl: String?
val customDomainProxyUrl: String?

Inspection of MParticleAutopilot.kt seems to imply that the this class is not intended to map directly to the Airship API.
val custom_domain = preferences.getString(INITIAL_CONFIG_URL, null)

If this is intended to map to the API, val custom_domain should probably be renamed and this works.

@@ -34,6 +34,11 @@ class MParticleAutopilot : Autopilot() {
if ("EU".equals(preferences.getString(DOMAIN, null), true)) {
optionsBuilder.setSite(AirshipConfigOptions.SITE_EU)
}
val custom_domain = preferences.getString(INITIAL_CONFIG_URL, null)

Choose a reason for hiding this comment

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

Prefer camel case for Android

Suggested change
val custom_domain = preferences.getString(INITIAL_CONFIG_URL, null)
val customDomain = preferences.getString(INITIAL_CONFIG_URL, null)

@rmi22186 rmi22186 requested a review from markvdouw January 18, 2024 16:26
@glyn-leigh
Copy link

This is Glynis from the Sam's Club Platform Team! An overview on the testing that we conducted:
We made sure it would compile, and we changed the gradle file a bit to be able to use it as a library for our project.
We used the mParticle dashboard to put in our custom domain, and then debugged the kit code to ensure that:

  1. The custom domain was being passed through
  2. We checked the network traffic to ensure it was being passed through for push and in-app notifications
  3. Airship was still functioning normally
    Things look good from our end, so we're eagerly awaiting the official SDK update.

@rmi22186 rmi22186 merged commit a1ba7a4 into mparticle-integrations:development Apr 18, 2024
@rmi22186
Copy link

@glyn-leigh - thanks for your support with this! i've merged and this will go out with our next android release

@glyn-leigh
Copy link

@rmi22186
Do you know what date that release is slated for?

@Mansi-mParticle
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants