Refactor consent #1
Conversation
| * trigger the Segment Consent Preference event to be fired. | ||
| */ | ||
| fun notifyConsentChanged() { | ||
| consentChange?.let { |
There was a problem hiding this comment.
| consentChange?.let { | |
| consentChange?.invoke() |
| analytics?.process(event) | ||
| } | ||
|
|
||
| queuedEvents?.clear() |
There was a problem hiding this comment.
not sure how we gonna use the start method, but it's better to make queuedEvents a queue instead of list, so we can do
while (queueEvents is not empty) {
poll from queue
analytics.process
}
avoiding to clear at the end, thus avoiding concurrent modification issues.
| @@ -0,0 +1,14 @@ | |||
| package com.segment.analytics.kotlin.destinations.consent | |||
|
|
|||
| class Constants { | |||
There was a problem hiding this comment.
maybe make it an object? so we don't need to define a companion object again.
| class Constants { | |
| object Constants { |
|
|
||
| // Look for a missing consent category | ||
| requiredConsentCategories.forEach { | ||
| if (!consentJsonArray.contains(it)) { |
There was a problem hiding this comment.
maybe make consentJsonArray a set instead of a list. reduce the complexity of contains from O(n) to O(1)
There was a problem hiding this comment.
Great suggestion!!
| val segmentDestination = it.find(Constants.SEGMENT_IO_KEY) | ||
| segmentDestination.let { | ||
| val existingBlocker = | ||
| segmentDestination?.analytics?.find(SegmentConsentBlocker::class) |
There was a problem hiding this comment.
why not find from analytics directly?
| segmentDestination?.analytics?.find(SegmentConsentBlocker::class) | |
| analytics.find(SegmentConsentBlocker::class) |
|
|
||
| // Add Segment Destination blocker | ||
| val segmentDestination = it.find(Constants.SEGMENT_IO_KEY) | ||
| segmentDestination.let { |
There was a problem hiding this comment.
| segmentDestination.let { | |
| segmentDestination?.let { |
| val existingBlocker = | ||
| segmentDestination?.analytics?.find(SegmentConsentBlocker::class) | ||
| if (existingBlocker == null) { | ||
| segmentDestination?.add(SegmentConsentBlocker(store)) |
There was a problem hiding this comment.
now you can use segmentDestination directly, since we did null check with let
| segmentDestination?.add(SegmentConsentBlocker(store)) | |
| it.add(SegmentConsentBlocker(store)) |
| val destinationKeys = state.destinationCategoryMap.keys | ||
| for (key in destinationKeys) { | ||
| val destination = analytics.find(key) | ||
| destination.let { |
There was a problem hiding this comment.
better to use the following
analytics.find(key)?.let { destination ->
// then you don't have to do null check for the usage of destination
}
| settings.toJsonElement().jsonObject.get(CONSENT_SETTINGS_KEY)?.let { | ||
| val jsonElement = it.jsonObject.get(HAS_UNMAPPED_DESTINATIONS_KEY) | ||
| println("hasUnmappedDestinations jsonElement: $jsonElement") | ||
| hasUnmappedDestinations = jsonElement.toString() == "true" |
There was a problem hiding this comment.
we have a set of utility method that can simplify operation like this. see here
for example:
- get object:
jsonObject.get("s")?.safeJsonObject - get string:
jsonObject.getString("s") - get boolean:
jsonObject.getBoolean("s") - etc
Update to make consent more like the updated Swift consent.