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.

Sign up for GitHub

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

Added getIdentityFlags method #51

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Added getIdentityFlags method #51

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
811bcc3
Implemented getIdentity flags for getting flags by an identity and se…
jackforesightmobile May 31, 2024
2c1496a
Merge pull request #7 from foresightmobile/feature/get-identity-flags
jackforesightmobile May 31, 2024
0955e05
Removed getIdentityFlags, added traits argument to getFeatureFlags
jackforesightmobile Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ class Flagsmith constructor(
retrofit.getIdentityFlagsAndTraits(identity).enqueueWithResult(defaults = null, result = result)
.also { lastUsedIdentity = identity }

fun getIdentityFlags(identity: String, traits: List<Trait>, result: (Result<ArrayList<Flag>>) -> Unit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think better rename getIdentityFlags to setAndGetIdentityFlags

Copy link
Contributor

Choose a reason for hiding this comment

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

@devapro the idea here was to keep things consistent with a lot of our other SDKs which use the getIdentityFlags method name, however, I have some other feedback which I'm compiling the related PR in the ios client here.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, the function name getIdentityFlags is a bit confusing, because it doesn't match what this method does.

Get flags for an identity and set traits in the same call
Even in the code of this method, we can see the call of postTraits

An option that you propose in iOS PR (Add an optional traits argument to the existing getFeatureFlags method ) looks good to me. However, I think we don't need to raise an exception if the identity is null because there already is a condition if (identity != null), maybe we can use it and postTraits only if the identity is not null.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, I think we don't need to raise an exception if the identity is null because there already is a condition if (identity != null), maybe we can use it and postTraits only if the identity is not null.

In my opinion it makes sense to raise an exception here because it is an invalid set of arguments. I'm not sure what the benefit is of not raising an exception in this case? I think that, without raising the exception, we may end up with confusing behaviour (since the traits argument would be ignored in situations where the identity is not provided).

retrofit.postTraits(IdentityAndTraits(identity, traits)).enqueueWithResult(result = {
result(it.map { response -> response.flags })
}).also { lastUsedIdentity = identity }
}

fun clearCache() {
try {
cache?.evictAll()
Expand Down
13 changes: 13 additions & 0 deletions FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,17 @@ class FeatureFlagTests {
}
assertEquals("Flagsmith requires a context to use the analytics feature", exception.message)
}

@Test
fun testGetFeatureFlagsWithIdentityAndTraits() {
mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES)
runBlocking {
val result = flagsmith.getIdentityFlagsSync(identity = "person", traits = listOf())
assertTrue(result.isSuccess)

val found = result.getOrThrow().find { flag -> flag.feature.name == "with-value" }
assertNotNull(found)
assertEquals(756.0, found?.featureStateValue)
}
}
}
3 changes: 3 additions & 0 deletions FlagsmithClient/src/test/java/com/flagsmith/SynchronousFlagsmith.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,6 @@ suspend fun Flagsmith.setTraitsSync(traits: List<Trait>, identity: String) : Res
suspend fun Flagsmith.getIdentitySync(identity: String): Result<IdentityFlagsAndTraits>
= suspendCoroutine { cont -> this.getIdentity(identity) { cont.resume(it) } }

suspend fun Flagsmith.getIdentityFlagsSync(identity: String, traits: List<Trait>): Result<ArrayList<Flag>>
= suspendCoroutine { cont -> this.getIdentityFlags(identity, traits) { cont.resume(it) } }

Loading