-
Notifications
You must be signed in to change notification settings - Fork 10
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 SDK config #1
Changes from 3 commits
a53ff8b
c0b7e3f
15841c4
8ff88ca
2589675
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1 @@ | ||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
package="io.customer.customer_io"> | ||
</manifest> | ||
<manifest package="io.customer.customer_io"></manifest> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,35 +1,111 @@ | ||||||
package io.customer.customer_io | ||||||
|
||||||
import android.app.Application | ||||||
import android.content.Context | ||||||
import androidx.annotation.NonNull | ||||||
|
||||||
import io.customer.customer_io.constant.Keys | ||||||
import io.customer.customer_io.extension.* | ||||||
import io.customer.sdk.CustomerIO | ||||||
import io.customer.sdk.CustomerIOShared | ||||||
import io.customer.sdk.data.store.Client | ||||||
import io.customer.sdk.util.Logger | ||||||
import io.flutter.embedding.engine.plugins.FlutterPlugin | ||||||
import io.flutter.plugin.common.MethodCall | ||||||
import io.flutter.plugin.common.MethodChannel | ||||||
import io.flutter.plugin.common.MethodChannel.MethodCallHandler | ||||||
import io.flutter.plugin.common.MethodChannel.Result | ||||||
|
||||||
/** CustomerIoPlugin */ | ||||||
class CustomerIoPlugin: FlutterPlugin, MethodCallHandler { | ||||||
/// The MethodChannel that will the communication between Flutter and native Android | ||||||
/// | ||||||
/// This local reference serves to register the plugin with the Flutter Engine and unregister it | ||||||
/// when the Flutter Engine is detached from the Activity | ||||||
private lateinit var channel : MethodChannel | ||||||
|
||||||
override fun onAttachedToEngine(@NonNull flutterPluginBinding: FlutterPlugin.FlutterPluginBinding) { | ||||||
channel = MethodChannel(flutterPluginBinding.binaryMessenger, "customer_io") | ||||||
channel.setMethodCallHandler(this) | ||||||
} | ||||||
|
||||||
override fun onMethodCall(@NonNull call: MethodCall, @NonNull result: Result) { | ||||||
if (call.method == "getPlatformVersion") { | ||||||
result.success("Android ${android.os.Build.VERSION.RELEASE}") | ||||||
} else { | ||||||
result.notImplemented() | ||||||
class CustomerIoPlugin : FlutterPlugin, MethodCallHandler { | ||||||
/// The MethodChannel that will the communication between Flutter and native Android | ||||||
/// | ||||||
/// This local reference serves to register the plugin with the Flutter Engine and unregister it | ||||||
/// when the Flutter Engine is detached from the Activity | ||||||
Comment on lines
+23
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great comment! I appreciate that since our team doesn't know Flutter. |
||||||
private lateinit var channel: MethodChannel | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I like the idea of having a more descriptive variable name so it's easy to understand that |
||||||
private lateinit var context: Context | ||||||
|
||||||
private val logger: Logger | ||||||
get() = CustomerIOShared.instance().diGraph.logger | ||||||
|
||||||
override fun onAttachedToEngine(@NonNull flutterPluginBinding: FlutterPlugin.FlutterPluginBinding) { | ||||||
context = flutterPluginBinding.applicationContext | ||||||
channel = MethodChannel(flutterPluginBinding.binaryMessenger, "customer_io") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of If so, should we use package name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its the channel name, we can have multiple channels to communicate with the native platform and flutter, also the convention in dart is to use "_" rather than "." |
||||||
channel.setMethodCallHandler(this) | ||||||
} | ||||||
|
||||||
override fun onMethodCall(@NonNull call: MethodCall, @NonNull result: Result) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, wow. If it's true that Flutter plugins use strings for calling methods of our native SDK, I can see us eventually wanting automated tests written. I don't know Flutter well enough to see if we create integration tests that:
|
||||||
when (call.method) { | ||||||
"getPlatformVersion" -> { | ||||||
result.success("Android-${android.os.Build.VERSION.RELEASE}") | ||||||
} | ||||||
Comment on lines
+41
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this function just for your own testing or is this something we need to implement for a flutter plugin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just for internal testing, will remove it afterwards. |
||||||
"initialize" -> { | ||||||
initialize(call, result) | ||||||
} | ||||||
else -> { | ||||||
result.notImplemented() | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
|
||||||
private fun initialize(call: MethodCall, result: Result) { | ||||||
try { | ||||||
val application: Application = context.applicationContext as Application | ||||||
val configData = call.arguments as? Map<String, Any> ?: emptyMap() | ||||||
val siteId = configData.getString(Keys.Environment.SITE_ID) | ||||||
val apiKey = configData.getString(Keys.Environment.API_KEY) | ||||||
val region = configData.getProperty<String>( | ||||||
Keys.Environment.REGION | ||||||
)?.takeIfNotBlank().toRegion() | ||||||
|
||||||
CustomerIO.Builder( | ||||||
siteId = siteId, | ||||||
apiKey = apiKey, | ||||||
region = region, | ||||||
appContext = application, | ||||||
).apply { | ||||||
setClient(client = getUserAgentClient(packageConfig = configData)) | ||||||
setupConfig(configData) | ||||||
}.build() | ||||||
logger.info("Customer.io instance initialized successfully") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this log statement need to be here since the native SDK makes logs similar to this? If there is value in adding logs here, should we make the logs Flutter specific?
Suggested change
|
||||||
result.success(true) | ||||||
} catch (e: Exception) { | ||||||
logger.error("Failed to initialize Customer.io instance from app, ${e.message}") | ||||||
result.error("FlutterSegmentException", e.localizedMessage, null); | ||||||
} | ||||||
} | ||||||
|
||||||
private fun getUserAgentClient(packageConfig: Map<String, Any?>?): Client { | ||||||
val sourceSDKVersion = packageConfig?.getProperty<String>( | ||||||
Keys.PackageConfig.SOURCE_SDK_VERSION | ||||||
)?.takeIfNotBlank() ?: packageConfig?.getProperty<String>( | ||||||
Keys.PackageConfig.SOURCE_SDK_VERSION_COMPAT | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure what Is this code saying that Flutter customers will set in their apps the value for
mrehan27 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
)?.takeIfNotBlank() ?: "n/a" | ||||||
// TODO: change it to flutter | ||||||
return Client.ReactNative(sdkVersion = sourceSDKVersion) | ||||||
} | ||||||
} | ||||||
|
||||||
override fun onDetachedFromEngine(@NonNull binding: FlutterPlugin.FlutterPluginBinding) { | ||||||
channel.setMethodCallHandler(null) | ||||||
} | ||||||
private fun CustomerIO.Builder.setupConfig(config: Map<String, Any?>?): CustomerIO.Builder { | ||||||
if (config == null) return this | ||||||
|
||||||
val logLevel = config.getProperty<String>(Keys.Config.LOG_LEVEL).toCIOLogLevel() | ||||||
setLogLevel(level = logLevel) | ||||||
config.getProperty<String>(Keys.Config.TRACKING_API_URL)?.takeIfNotBlank()?.let { value -> | ||||||
setTrackingApiURL(value) | ||||||
} | ||||||
config.getProperty<Boolean>(Keys.Config.AUTO_TRACK_DEVICE_ATTRIBUTES)?.let { value -> | ||||||
autoTrackDeviceAttributes(shouldTrackDeviceAttributes = value) | ||||||
} | ||||||
config.getProperty<Double>(Keys.Config.BACKGROUND_QUEUE_MIN_NUMBER_OF_TASKS)?.let { value -> | ||||||
setBackgroundQueueMinNumberOfTasks(backgroundQueueMinNumberOfTasks = value.toInt()) | ||||||
} | ||||||
config.getProperty<Double>(Keys.Config.BACKGROUND_QUEUE_SECONDS_DELAY)?.let { value -> | ||||||
setBackgroundQueueSecondsDelay(backgroundQueueSecondsDelay = value) | ||||||
} | ||||||
Comment on lines
+91
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a function that we can add to the native SDK or something to do this work for us? I feel this code is error-prone as we would need to remember to update this code each time that we update the SDK configuration object in the native SDKs. By housing the code in the native SDK, we have a higher chance of remembering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can support initialization through maps in Native SDKs to help in this for both React Native and Flutter. But we need to be careful and not rename any key used by these wrappers or change its type to avoid breaking this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Agreed, something that we should definitely focus later on. |
||||||
return this | ||||||
} | ||||||
|
||||||
override fun onDetachedFromEngine(@NonNull binding: FlutterPlugin.FlutterPluginBinding) { | ||||||
channel.setMethodCallHandler(null) | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package io.customer.customer_io.constant | ||
|
||
internal object Keys { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See PR comment in regards to iOS |
||
object Environment { | ||
const val SITE_ID = "siteId" | ||
const val API_KEY = "apiKey" | ||
const val REGION = "region" | ||
const val ORGANIZATION_ID = "organizationId" | ||
} | ||
|
||
object Config { | ||
const val TRACKING_API_URL = "trackingApiUrl" | ||
const val AUTO_TRACK_DEVICE_ATTRIBUTES = "autoTrackDeviceAttributes" | ||
const val LOG_LEVEL = "logLevel" | ||
const val AUTO_TRACK_PUSH_EVENTS = "autoTrackPushEvents" | ||
const val BACKGROUND_QUEUE_MIN_NUMBER_OF_TASKS = "backgroundQueueMinNumberOfTasks" | ||
const val BACKGROUND_QUEUE_SECONDS_DELAY = "backgroundQueueSecondsDelay" | ||
} | ||
|
||
object PackageConfig { | ||
const val SOURCE_SDK_VERSION = "version" | ||
const val SOURCE_SDK_VERSION_COMPAT = "sdkVersion" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||
package io.customer.customer_io.extension | ||||||
|
||||||
import io.customer.sdk.CustomerIOShared | ||||||
|
||||||
@Throws(IllegalArgumentException::class) | ||||||
internal inline fun <reified T> Map<String, Any?>.getPropertyUnsafe(key: String): T { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
IMO, |
||||||
val property = get(key) | ||||||
|
||||||
if (property !is T) { | ||||||
throw IllegalArgumentException( | ||||||
"Invalid value provided for key: $key, value $property must be of type ${T::class.java.simpleName}" | ||||||
) | ||||||
} | ||||||
return property | ||||||
} | ||||||
|
||||||
internal inline fun <reified T> Map<String, Any?>.getProperty(key: String): T? = try { | ||||||
getPropertyUnsafe(key) | ||||||
} catch (ex: IllegalArgumentException) { | ||||||
CustomerIOShared.instance().diGraph.logger.error( | ||||||
ex.message ?: "getProperty($key) -> IllegalArgumentException" | ||||||
) | ||||||
null | ||||||
} | ||||||
|
||||||
@Throws(IllegalArgumentException::class) | ||||||
internal fun Map<String, Any?>.getString(key: String): String = try { | ||||||
getPropertyUnsafe<String>(key).takeIfNotBlank() ?: throw IllegalArgumentException( | ||||||
"Invalid value provided for $key, must not be blank" | ||||||
) | ||||||
} catch (ex: IllegalArgumentException) { | ||||||
CustomerIOShared.instance().diGraph.logger.error( | ||||||
ex.message ?: "getString($key) -> IllegalArgumentException" | ||||||
) | ||||||
throw ex | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package io.customer.customer_io.extension | ||
|
||
internal fun String.takeIfNotBlank(): String? = takeIf { it.isNotBlank() } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package io.customer.customer_io.extension | ||
|
||
import io.customer.sdk.data.model.Region | ||
import io.customer.sdk.util.CioLogLevel | ||
|
||
internal fun String?.toRegion(fallback: Region = Region.US): Region { | ||
return if (this.isNullOrBlank()) fallback | ||
else listOf( | ||
Region.US, | ||
Region.EU, | ||
).find { value -> value.code.equals(this, ignoreCase = true) } ?: fallback | ||
} | ||
|
||
internal fun String?.toCIOLogLevel(fallback: CioLogLevel = CioLogLevel.NONE): CioLogLevel { | ||
return CioLogLevel.values().find { value -> value.name.equals(this, ignoreCase = true) } | ||
?: fallback | ||
} | ||
Comment on lines
+6
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See PR comment in regards to housing code like this in the native SDKs instead of in wrapper SDK (in this case, Flutter). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, thats the plan eventually, I think we even added that to |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
PODS: | ||
- customer_io (0.0.1): | ||
- CustomerIO/Tracking | ||
- Flutter | ||
- CustomerIO/Tracking (1.2.2): | ||
- CustomerIOTracking (= 1.2.2) | ||
- CustomerIOCommon (1.2.2) | ||
- CustomerIOTracking (1.2.2): | ||
- CustomerIOCommon (= 1.2.2) | ||
- Flutter (1.0.0) | ||
|
||
DEPENDENCIES: | ||
- customer_io (from `.symlinks/plugins/customer_io/ios`) | ||
- Flutter (from `Flutter`) | ||
|
||
SPEC REPOS: | ||
trunk: | ||
- CustomerIO | ||
- CustomerIOCommon | ||
- CustomerIOTracking | ||
|
||
EXTERNAL SOURCES: | ||
customer_io: | ||
:path: ".symlinks/plugins/customer_io/ios" | ||
Flutter: | ||
:path: Flutter | ||
|
||
SPEC CHECKSUMS: | ||
customer_io: 99ae280180a2fe4c0514f28a8da5e7a66734a612 | ||
CustomerIO: 0a31ad1baed6cd952469b9098e2e73fa96cec70e | ||
CustomerIOCommon: 3bf82c3574205819a69baa1f0bc5b47671dd2d19 | ||
CustomerIOTracking: 101dc8c25eff807eadc8fbcf83b76f98fb5832a5 | ||
Flutter: 50d75fe2f02b26cc09d224853bb45737f8b3214a | ||
|
||
PODFILE CHECKSUM: aafe91acc616949ddb318b77800a7f51bffa2a4c | ||
|
||
COCOAPODS: 1.11.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually when referring to CIO, we use
CustomerIO
instead ofCustomerIo
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since our team is not knowledgable with flutter, classes like this could benefit from more comment explaining what a Flutter plugin is?