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

Added getIdentityFlags method #51

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Conversation

jackforesightmobile
Copy link
Contributor

No description provided.

jackforesightmobile and others added 2 commits May 31, 2024 11:46
Implemented getIdentity flags for getting flags by an identity and se…
@@ -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).

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

Approved with one minor comment.

Comment on lines +114 to +122
if (traits != null) {
retrofit.postTraits(IdentityAndTraits(identity, traits)).enqueueWithResult(result = {
result(it.map { response -> response.flags })
}).also { lastUsedIdentity = identity }
} else {
retrofit.getIdentityFlagsAndTraits(identity).enqueueWithResult { res ->
flagUpdateFlow.tryEmit(res.getOrNull()?.flags ?: emptyList())
result(res.map { it.flags })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my comment here, I think we can remove this conditional logic and always use the POST endpoint, but if it's non-trivial to achieve, we can ignore for the purpose of this PR.

@matthewelwell matthewelwell merged commit 3294590 into Flagsmith:main Oct 4, 2024
1 check passed
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.

3 participants