feat: Support transient identities and traits #54
Conversation
There was a problem hiding this comment.
@novakzaballa quite a lot of changes needed here. I appreciate that Kotlin isn't a language you are familiar with, but I would certainly expect a bit more thought and care over some of the items I've highlighted in my review here.
FlagsmithClient/src/main/java/com/flagsmith/entities/IdentityAndTraits.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/entities/IdentityFlagsAndTraits.kt
Outdated
Show resolved
Hide resolved
| mockServer.mockResponseFor(MockEndpoint.GET_TRANSIENT_IDENTITIES) | ||
| runBlocking { | ||
| val result = flagsmith.getIdentitySync("transient-identity") |
There was a problem hiding this comment.
This isn't testing transiency at all! It's not setting the transient attribute?
There was a problem hiding this comment.
I see that it's relying on the mocked server response, but really this isn't good enough.
| result.getOrThrow().traits.find { trait -> trait.key == "favourite-colour" }?.stringValue | ||
| ) | ||
| assertTrue(result.getOrThrow().traits.find { trait -> trait.transient == true }?.transient == true) |
There was a problem hiding this comment.
What is this? 2 things:
- I have no idea how this is even supposed to work since we're not setting any transient traits as part of this request.
- This test is literally just searching for a trait that has
transient = true... then asserting thattransient = true
There was a problem hiding this comment.
Ah, I think (1) can be answer by the use of MockEndpoint.GET_TRANSIENT_IDENTITIES, but (2) still stands.
| @@ -77,7 +77,7 @@ class TraitEntityTests { | |||
|
|
|||
| @Test | |||
| fun testTraitConstructorStringType() { | |||
| val trait = Trait( "string-key", "string-value") | |||
| val trait = Trait( "string-key", "string-value", false) | |||
There was a problem hiding this comment.
As I mentioned above, this shouldn't be necessary. This shouldn't need to be a breaking change.
Closes #52