-
Notifications
You must be signed in to change notification settings - Fork 6
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
Refactor retrieving advertisement id to Coroutines #93
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
d90b0d6
refactor: create `deviceInfo` for every `buildEvent`
wzieba 9a420f0
refactor: extract creating "device info" to a separate class
wzieba 17c5e82
Rename .java to .kt
wzieba 65e25dd
refactor: move `DeviceInfoRepository` to Kotlin
wzieba ce1d56c
refactor: make `EventsBuilderTest` not test `DeviceInfoRepository`
wzieba 96acab0
refactor: move `GetAdKey` to Coroutines
wzieba 2c0cc68
refactor: simplify AdKey retrieval
wzieba 25d06f1
fix: remove `appname` from device info
wzieba c5962bc
style: remove unused imports
wzieba 4c91494
tests: make `EventsBuilderTest` not Robolectric test
wzieba 9cd6cce
refactor: extract getting ad key to `AdvertisementIdProvider`
wzieba 6ff145a
refactor: extract getting uuid to `UuidProvider`
wzieba 59a83d8
tests: add unit tests for `UuidProvider`
wzieba 3bcbb38
refactor: do not query `ANDROID_ID` from `SharedPreferences`.
wzieba c6f68bf
tests: add unit tests for `AndroidDeviceInfoRepository`
wzieba 30eb83f
Merge branch 'coroutines' into get_add_key_coroutines
wzieba 9d99164
Merge branch 'engagement_manager_coroutines' into get_add_key_coroutines
wzieba 988ff4b
Merge branch 'coroutines' into get_add_key_coroutines
wzieba 7533f97
fix: assign advertisement id in AdvertisementIdProvider
wzieba 473dc7e
tests: make SUT test-scoped in AndroidDeviceInfoRepositoryTest
wzieba 0c3102d
chore: add a comment for AdvertisementIdProvider#provide
wzieba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
50 changes: 50 additions & 0 deletions
50
parsely/src/main/java/com/parsely/parselyandroid/AdvertisementIdProvider.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package com.parsely.parselyandroid | ||
|
||
import android.content.Context | ||
import android.provider.Settings | ||
import com.google.android.gms.ads.identifier.AdvertisingIdClient | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.launch | ||
|
||
internal class AdvertisementIdProvider( | ||
private val context: Context, | ||
coroutineScope: CoroutineScope | ||
) : IdProvider { | ||
|
||
private var adKey: String? = null | ||
|
||
init { | ||
coroutineScope.launch { | ||
try { | ||
adKey = AdvertisingIdClient.getAdvertisingIdInfo(context).id | ||
} catch (e: Exception) { | ||
ParselyTracker.PLog("No Google play services or error!") | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* @return advertisement id if the coroutine in the constructor finished executing AdvertisingIdClient#getAdvertisingIdInfo | ||
* null otherwise | ||
*/ | ||
override fun provide(): String? = adKey | ||
} | ||
|
||
internal class AndroidIdProvider(private val context: Context) : IdProvider { | ||
override fun provide(): String? { | ||
val uuid = try { | ||
Settings.Secure.getString( | ||
context.applicationContext.contentResolver, | ||
Settings.Secure.ANDROID_ID | ||
) | ||
} catch (ex: Exception) { | ||
null | ||
} | ||
ParselyTracker.PLog(String.format("Android ID: %s", uuid)) | ||
return uuid | ||
} | ||
} | ||
|
||
internal fun interface IdProvider { | ||
oguzkocer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fun provide(): String? | ||
} |
46 changes: 46 additions & 0 deletions
46
parsely/src/main/java/com/parsely/parselyandroid/DeviceInfoRepository.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
package com.parsely.parselyandroid | ||
|
||
import android.os.Build | ||
|
||
internal interface DeviceInfoRepository{ | ||
fun collectDeviceInfo(): Map<String, String> | ||
} | ||
|
||
internal open class AndroidDeviceInfoRepository( | ||
private val advertisementIdProvider: IdProvider, | ||
private val androidIdProvider: IdProvider, | ||
): DeviceInfoRepository { | ||
|
||
/** | ||
* Collect device-specific info. | ||
* | ||
* | ||
* Collects info about the device and user to use in Parsely events. | ||
*/ | ||
override fun collectDeviceInfo(): Map<String, String> { | ||
val dInfo: MutableMap<String, String> = HashMap() | ||
|
||
// TODO: screen dimensions (maybe?) | ||
dInfo["parsely_site_uuid"] = parselySiteUuid | ||
dInfo["manufacturer"] = Build.MANUFACTURER | ||
dInfo["os"] = "android" | ||
dInfo["os_version"] = String.format("%d", Build.VERSION.SDK_INT) | ||
|
||
return dInfo | ||
} | ||
|
||
private val parselySiteUuid: String | ||
get() { | ||
val adKey = advertisementIdProvider.provide() | ||
val androidId = androidIdProvider.provide() | ||
|
||
ParselyTracker.PLog("adkey is: %s, uuid is %s", adKey, androidId) | ||
|
||
return if (adKey != null) { | ||
adKey | ||
} else { | ||
ParselyTracker.PLog("falling back to device uuid") | ||
androidId .orEmpty() | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
parsely/src/test/java/com/parsely/parselyandroid/AndroidDeviceInfoRepositoryTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
package com.parsely.parselyandroid | ||
|
||
import org.assertj.core.api.Assertions.assertThat | ||
import org.junit.Before | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.robolectric.RobolectricTestRunner | ||
import org.robolectric.annotation.Config | ||
import org.robolectric.shadows.ShadowBuild | ||
|
||
private const val SDK_VERSION = 33 | ||
private const val MANUFACTURER = "test manufacturer" | ||
|
||
@RunWith(RobolectricTestRunner::class) | ||
@Config(sdk = [SDK_VERSION]) | ||
internal class AndroidDeviceInfoRepositoryTest { | ||
|
||
@Before | ||
fun setUp() { | ||
ShadowBuild.setManufacturer(MANUFACTURER) | ||
} | ||
|
||
@Test | ||
fun `given the advertisement id exists, when collecting device info, then parsely site uuid is advertisement id`() { | ||
// given | ||
val advertisementId = "ad id" | ||
val sut = AndroidDeviceInfoRepository( | ||
advertisementIdProvider = { advertisementId }, | ||
androidIdProvider = { "android id" }) | ||
|
||
// when | ||
val result = sut.collectDeviceInfo() | ||
|
||
// then | ||
assertThat(result).isEqualTo(expectedConstantDeviceInfo + ("parsely_site_uuid" to advertisementId)) | ||
} | ||
|
||
@Test | ||
fun `given the advertisement is null and android id is not, when collecting device info, then parsely id is android id`() { | ||
// given | ||
val androidId = "android id" | ||
val sut = AndroidDeviceInfoRepository( | ||
advertisementIdProvider = { null }, | ||
androidIdProvider = { androidId } | ||
) | ||
|
||
// when | ||
val result = sut.collectDeviceInfo() | ||
|
||
// then | ||
assertThat(result).isEqualTo(expectedConstantDeviceInfo + ("parsely_site_uuid" to androidId)) | ||
} | ||
|
||
@Test | ||
fun `given both advertisement id and android id are null, when collecting device info, then parsely id is empty`() { | ||
// given | ||
val sut = AndroidDeviceInfoRepository( | ||
advertisementIdProvider = { null }, | ||
androidIdProvider = { null } | ||
) | ||
|
||
// when | ||
val result = sut.collectDeviceInfo() | ||
|
||
// then | ||
assertThat(result).isEqualTo(expectedConstantDeviceInfo + ("parsely_site_uuid" to "")) | ||
} | ||
|
||
private companion object { | ||
val expectedConstantDeviceInfo = mapOf( | ||
"manufacturer" to MANUFACTURER, | ||
"os" to "android", | ||
"os_version" to "$SDK_VERSION" | ||
) | ||
} | ||
} |
56 changes: 56 additions & 0 deletions
56
parsely/src/test/java/com/parsely/parselyandroid/AndroidIdProviderTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
package com.parsely.parselyandroid | ||
|
||
import android.app.Application | ||
import android.provider.Settings | ||
import androidx.test.core.app.ApplicationProvider | ||
import org.assertj.core.api.Assertions.assertThat | ||
import org.junit.Before | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.robolectric.RobolectricTestRunner | ||
|
||
@RunWith(RobolectricTestRunner::class) | ||
internal class AndroidIdProviderTest { | ||
|
||
lateinit var sut: AndroidIdProvider | ||
|
||
@Before | ||
fun setUp() { | ||
sut = AndroidIdProvider(ApplicationProvider.getApplicationContext()) | ||
} | ||
|
||
@Test | ||
fun `given no site uuid is stored, when requesting uuid, then return ANDROID_ID value`() { | ||
// given | ||
val fakeAndroidId = "test id" | ||
Settings.Secure.putString( | ||
ApplicationProvider.getApplicationContext<Application>().contentResolver, | ||
Settings.Secure.ANDROID_ID, | ||
fakeAndroidId | ||
) | ||
|
||
// when | ||
val result= sut.provide() | ||
|
||
// then | ||
assertThat(result).isEqualTo(fakeAndroidId) | ||
} | ||
|
||
@Test | ||
fun `given site uuid already requested, when requesting uuid, then return same uuid`() { | ||
// given | ||
val fakeAndroidId = "test id" | ||
Settings.Secure.putString( | ||
ApplicationProvider.getApplicationContext<Application>().contentResolver, | ||
Settings.Secure.ANDROID_ID, | ||
fakeAndroidId | ||
) | ||
val storedValue = sut.provide() | ||
|
||
// when | ||
val result = sut.provide() | ||
|
||
// then | ||
assertThat(result).isEqualTo(storedValue) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I am curious about the general design choice for this class.
If I am understanding it correctly, when an object of this class is instantiated, if we were to read the
adKey
immediately, it'll returnnull
. Looking at the constructor signature, there is a hint that we are using coroutines for something but there isn't much context about what it's used for unless we look into the implementation. So, I think a developer may not realize that gotcha and end up requesting theadKey
before it's ready. Also, I am not even sure if there is a straightforward way to tell when theadKey
is ready to be retrieved.So, I was wondering if we should use a
suspend
function forprovide
so that it can be used in a proper coroutine scope without worrying about the internal details. What do you think?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.
That's correct and intended 👍
Yes, I don't see a way in current implementation. One suggestion would be to maybe use a
(State)Flow
? But I'm not sure if it wouldn't be an overkill for this case.We could do this, I was also experimenting with such design. My decision to go with not using
suspend
is the actual usage ofAdvertisementIdProvider
in the codebase. If we madesuspend provide
method, we'd have to move executing coroutine invocation higher in a chain of execution - maybe run coroutine inAndroidDeviceInfoRepository#collectDeviceInfo
orEventsBuilder#buildEvent
? It got all more complex than what I intended with such PR, so I decided to run coroutine in constructor ofAdvertisementIdProvider
and returnnull
if the coroutine did not yet finish.WDYT? I understand the possibly misleading design, but having the context above - would it make sense to stick to such design, or we should iterate? If we should iterate and make
suspend provide
- where would you suggest running actual coroutine?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.
I think this one is important to get right. We don't have to use a suspended function if it's going to be more trouble than it's worth - but at the very least, I think it's worth having a big disclaimer in both the class definition and where it's consumed. The reason I strongly feel this way is because if we have any issues related to this, it can be hard to debug if the developer doesn't have any prior context. To be fair - the fact that it's marked as
nullable
may give enough context, so maybe it's not as big of a deal as I am making it out to be 🤷♂️I don't think we should make any major changes in this PR, because it'll get hard to review it. However, if I was working on this, I'd give using suspended function a chance as a separate PR. Assuming we keep the current architecture, this will cause a chain reaction and the following methods may also need to become suspended functions:
AndroidDeviceInfoRepository.collectDeviceInfo
,EventsBuilder.buildEvent
&ParselyTracker.trackPlay
.ParselyTracker.trackPlay
: Looking at the name, I assumed that this function would eventually become a suspended function regardless of the decision we'll make here, but looking at the implementation, I am not so sure. I'd love to hear your thoughts!EventsBuilder.buildEvent
: This probably shouldn't be a suspended function, but instead take the device info as a parameter. This is used inParselyTracker.trackPlay
&ParselyTracker.trackPageview
both of which could be suspended functions, so I think that'll work out.AndroidDeviceInfoRepository.collectDeviceInfo
: Making this a suspended function makes sense to me. It communicates that collecting device info is not an immediate thing which could be for many reasons. We could be reading from file, checking a DB etc to collect this info. Admittedly some of the info is available immediately, so I could see an argument for keeping it a regular function as well. 🤷♂️I think the decision will heavily depend on your vision for the SDK in general and I don't want to influence that vision too much. If you think it's better to leave the current design as is, I can live with that as long as we document it.
Hope you find my thoughts useful - let me know what you think!
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.
Gotcha 👍 I've created an issue to track it: #97
This function is one of the few main exposed to the consumers of the SDK. I'd argue to not make it, or rest of the tracking methods (
startEngagement
,trackPageview
)suspend
, as it would force clients to use coroutines when using our API. It'd be problematic for them, and sometimes very complex, if the client's codebase is in Java.True, that'd be a good place. Still, we would have to find a way to wait for
collectDeviceInfo
to finish without blocking the SDK from accepting new events - I'd prefer to never allow user to wait for accepting a new event.That's true - I think it'd be best to combine this improvement with changes described in #94 ! Thank you for sharing your thoughts and suggestions - I really appreciate it as, even if we won't apply them right away, they'll be certainly useful during later phases.
As that's the case - would you mind accepting this PR, or do you think we should iterate on something more here?
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.
I just wanted this conversation to resolve before approving the PR - in case you wanted to add a comment about this issue. Approved now - thanks for the discussion!
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.
Ah right, thanks! Added a comment: 0c3102d. Merging 👍