[PM-9401] Server feature flags manager #3656

andrebispo5 · 2024-07-31T14:18:03Z

🎟️ Tracking

Android - Implement Feature Flags Support

📔 Objective

Add support for hardcoded feature flags.
Add support for server-related feature flags. Be able to use them as boolean, int or string types.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

…ts to be String - String

github-actions · 2024-07-31T14:23:42Z

Checkmarx One – Scan Summary & Details – cd343ff3-3915-4bfb-8beb-8d31dea2d543

No New Or Fixed Issues Found

david-livefront · 2024-07-31T14:40:11Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+    override val featureFlagsLocal: Map<String, Boolean>
+        get() = mapOf(CIPHER_KEY_ENCRYPTION_KEY to true)
+
+    override val featureFlagsServerStateFlow: StateFlow<Map<String, String>?>


I don't think we want to expose feature flags as strings, we want them to be type safe, right?

david-livefront · 2024-07-31T14:48:00Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+
+    override val featureFlagsServerStateFlow: StateFlow<Map<String, String>?>
+        get() = configDiskSource
+            .serverConfigFlow


Can we just subscribe to the serverConfigRepository.serverConfigStateFlow?

david-livefront · 2024-07-31T14:55:05Z

app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/BitwardenFeatureFlagManager.kt

-/**
- * Manages the available feature flags for the Bitwarden application.
- */
-interface BitwardenFeatureFlagManager {


Any reason why you renamed this?

Since the manager can have server-side flags, which may come from a self-hosted environments, I think removing the Bitwarden word from the class makes sense. What do you think?

david-livefront · 2024-07-31T14:56:19Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt

@@ -136,10 +139,23 @@ object PlatformManagerModule {
        dispatcherManager = dispatcherManager,
    )

+    @Provides
+    @Singleton
+    fun providesBitwardenFeatureFlagManager(


Let's rename this function since the class was renamed.

fun providesFeatureFlagManager(

david-livefront · 2024-07-31T14:58:12Z

app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/ConfigDiskSourceTest.kt

@@ -106,6 +106,6 @@ private val SERVER_CONFIG = ServerConfig(
            notificationsUrl = "http://localhost:61840",
            ssoUrl = "http://localhost:51822",
        ),
-        featureStates = mapOf("duo-redirect" to true, "flexible-collections-v-1" to false),
+        featureStates = mapOf("duo-redirect" to "true", "flexible-collections-v-1" to "false"),


Looks like the sample JSON above returns true, not "true"? Does the API return a string or boolean?

ConfigDiskSource will the values as strings in order for them to be casted into the 3 possible types.

david-livefront · 2024-07-31T14:59:36Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+
+/**
+ * Enum to hold feature flag keys.
+ * [stringValue] corresponds to the string value of a give key


Can we prefix this with @property:

* @property stringValue corresponds to the string value of a give key.

david-livefront · 2024-07-31T15:01:56Z

...rc/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt

+import java.time.Instant
+
+class FakeServerConfigRepository(
+    private val configDiskSource: ConfigDiskSource,


You should not need this dependency in a fake

david-livefront · 2024-07-31T15:18:59Z

app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt

+    @Test
+    fun `getFeatureFlag Boolean should return value if exists`() = runTest {
+        val flagValue = manager.getFeatureFlag(
+            FlagKey.EmailVerification,


Can you add the name for this param.

shannon-livefront · 2024-08-01T19:22:30Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManager.kt

+    /**
+     * Returns a map of constant feature flags that are only used locally.
+     */
+    val featureFlagsLocal: Map<String, Boolean>


I'm curious why the interfaces exposes both this map and the flow of a map from the server. When the rest of the app interacts with the feature flags, it shouldn't care whether the flag is local or from the server, right?

Maybe it's a naming confusion. Local is not meant as cached, the local flags are just constants that live on the device and are not affected by server/region changes.
Server flags are provided by the server, are dynamic and can change over time.
For now locals are only used to initialise the SDK.

These flags are currently just used for the SDK. So we could call it sdkFeatureFlags or something like that?

For the time being I think we can. If, down the line, we need constant flags locally we can add them as such.

shannon-livefront · 2024-08-01T19:25:03Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+    private suspend fun getFlagStringValueOrNull(key: FlagKey, forceRefresh: Boolean): String? {
+        val configuration = serverConfigRepository.getServerConfig(forceRefresh)
+        return configuration?.serverData?.featureStates?.get(key.stringValue)
+    }
+}


I think this can be condensed a bit:

Suggested change

private suspend fun getFlagStringValueOrNull(key: FlagKey, forceRefresh: Boolean): String? {

val configuration = serverConfigRepository.getServerConfig(forceRefresh)

return configuration?.serverData?.featureStates?.get(key.stringValue)

}

}

private suspend fun getFlagStringValueOrNull(key: FlagKey, forceRefresh: Boolean): String? =

serverConfigRepository

.getServerConfig(forceRefresh)

?.serverData

?.featureStates

?.get(key.stringValue)

shannon-livefront · 2024-08-01T19:27:47Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+        return getFlagStringValueOrNull(key, forceRefresh) ?: defaultValue
+    }
+
+    private suspend fun getFlagStringValueOrNull(key: FlagKey, forceRefresh: Boolean): String? {


I'm a little confused - this means that all three getFeatureFlag methods only check for remote feature flags? Is there a reason we can't check both remote and local sources, so that callers of the methods don't have to know where the flag is coming from? If not, I think it would be better to document in the comments for these methods that they only check remote flags.

codecov · 2024-08-02T20:31:38Z

Codecov Report

Attention: Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.79%. Comparing base (f110687) to head (7d928cd).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...en/data/platform/manager/FeatureFlagManagerImpl.kt	78.26%	1 Missing and 4 partials ⚠️
...rden/data/platform/manager/SdkClientManagerImpl.kt	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3656      +/-   ##
==========================================
- Coverage   87.80%   87.79%   -0.01%     
==========================================
  Files         368      368              
  Lines       30482    30503      +21     
  Branches     4557     4564       +7     
==========================================
+ Hits        26765    26781      +16     
- Misses       2128     2129       +1     
- Partials     1589     1593       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

david-livefront · 2024-08-02T20:35:31Z

...c/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt

@@ -102,5 +102,8 @@ object PlatformNetworkModule {

        // Respect model default property values.
        coerceInputValues = true
+
+        // Allow unquoted json keys and values to be parsed
+        isLenient = true


This is a global flag, I have some concerns about making everything lenient.

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

…efault

david-livefront · 2024-08-05T13:59:19Z

...c/main/java/com/x8bit/bitwarden/data/platform/datasource/network/model/ConfigResponseJson.kt

@@ -31,7 +31,7 @@ data class ConfigResponseJson(
    val environment: EnvironmentJson?,

    @SerialName("featureStates")
-    val featureStates: Map<String, Boolean>?,
+    val featureStates: Map<String, String>?,


I think it might make more sense to parse this as a JsonElement or JsonPrimitive, I don't think we would need to make the Json instance isLenient and it would imply the unsafe nature of this property.

let me try that out.

It worked out.

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/SdkClientManagerImpl.kt

david-livefront · 2024-08-05T14:00:37Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+ * @property [stringValue] corresponds to the string value of a give key
+ * @property [defaultValue] corresponds to default value of the flag of type [T]
+ */
+sealed class FlagKey<out T : Any> {


Can we move this into the model package

david-livefront · 2024-08-05T14:44:53Z

app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerTest.kt

+
+    @Test
+    fun `ConfigDiskSource flow with null should trigger default flag value value`() = runTest {
+        fakeServerConfigRepository.mutableServerConfigFlow.tryEmit(null)


Can this just be

fakeServerConfigRepository.serverConfigValue = null

david-livefront · 2024-08-05T14:45:05Z

...rc/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt

+             mutableServerConfigFlow.value = value
+         }
+
+    val mutableServerConfigFlow = MutableStateFlow<ServerConfig?>(SERVER_CONFIG)


Can we make this private

dseverns-livefront · 2024-08-05T21:06:26Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+
+/**
+ * Class to hold feature flag keys.
+ * @property [stringValue] corresponds to the string value of a give key


Suggested change

* @property [stringValue] corresponds to the string value of a give key

* @property [stringValue] corresponds to the string value of a given key

dseverns-livefront · 2024-08-05T21:15:20Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+ * @property [defaultValue] corresponds to default value of the flag of type [T]
+ */
+sealed class FlagKey<out T : Any> {
+    abstract val stringValue: String


💭 would it maybe be more clear to name this property keyName or just name

…ive.

…warden/android into pm-9401/server-feature-flags

david-livefront · 2024-08-06T13:49:30Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/FlagKey.kt

+ * @property [keyName] corresponds to the string value of a given key
+ * @property [defaultValue] corresponds to default value of the flag of type [T]
+ */
+sealed class FlagKey<out T : Any> {


david-livefront · 2024-08-06T13:50:02Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/model/FlagKey.kt

+     * Data object holding the key for an Int flag to be used in tests
+     */
+    data object DummyInt : FlagKey<Int>() {
+        override val keyName: String = "email-verification"


Will duplicating the keyName cause any issues?

david-livefront · 2024-08-06T14:09:25Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt

+                when (defaultValue::class) {
+                    Boolean::class -> it.content.toBoolean() as? T
+                    String::class -> it.content as? T
+                    Int::class -> it.content.toInt() as? T


I don't think you need the ? on the as's

david-livefront · 2024-08-06T14:19:51Z

...rc/test/java/com/x8bit/bitwarden/data/platform/repository/util/FakeServerConfigRepository.kt

+             mutableServerConfigFlow.value = value
+         }
+
+    private val mutableServerConfigFlow = MutableStateFlow<ServerConfig?>(SERVER_CONFIG)


andrebispo5 added 5 commits July 31, 2024 15:09

[PM-9401] Change feature flags object type from Boolean to String

9a93a9e

[PM-9401] Add FakeServerConfigRepository. Change feature flags in tes…

b3534b7

…ts to be String - String

[PM-9401] Remove old BitwardenFeatureFlagManager and tests

1e573db

[PM-9401] Add new FeatureFlagManager into platform

e93ac55

[PM-9401] Add FeatureFlagManager tests

628a09f

andrebispo5 requested review from david-livefront and SaintPatrck and removed request for david-livefront July 31, 2024 14:18

david-livefront reviewed Jul 31, 2024

View reviewed changes

shannon-livefront reviewed Aug 1, 2024

View reviewed changes

andrebispo5 added 4 commits August 2, 2024 20:52

[PM-9401] Refactor FeatureFlagManager and corresponding tests.

5b151d8

[PM-9401] Enable lenient json deserialising

6146255

[PM-9401] Fix ConfigDiskSourceTest data sample

41decbe

[PM-9401] Add parameter name

88ee02c

andrebispo5 requested review from david-livefront and shannon-livefront August 2, 2024 20:17

david-livefront reviewed Aug 2, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt Outdated Show resolved Hide resolved

david-livefront reviewed Aug 2, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FeatureFlagManagerImpl.kt Outdated Show resolved Hide resolved

[PM-9401] Rename featureFlagsLocal to sdkFeatureFlags

06ee894

andrebispo5 added 2 commits August 5, 2024 14:54

[PM-9401] Create extension method for ServerConfig to getFlagValueOrD…

456fe8a

…efault

Merge branch 'main' into pm-9401/server-feature-flags

bcf5058

david-livefront reviewed Aug 5, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/SdkClientManagerImpl.kt Show resolved Hide resolved

david-livefront reviewed Aug 5, 2024

View reviewed changes

dseverns-livefront reviewed Aug 5, 2024

View reviewed changes

andrebispo5 added 6 commits August 6, 2024 12:35

[PM-9401] Remove Json lenient and parse server response as JsonPrimit…

28c6a25

…ive.

Merge branch 'pm-9401/server-feature-flags' of https://github.com/bit…

107b992

…warden/android into pm-9401/server-feature-flags

[PM-9401] Move FlagKey into model folder.

88ca99c

[PM-9401] Fix FeatureFlagManager tests

b1187c1

[PM-9401] Rename KeyFlag stringValue to keyName

6f1696f

Merge branch 'main' into pm-9401/server-feature-flags

9f79b19

andrebispo5 requested review from dseverns-livefront and david-livefront August 6, 2024 11:59

david-livefront reviewed Aug 6, 2024

View reviewed changes

andrebispo5 added 2 commits August 6, 2024 15:39

[PM-9401] Remove null check.

d5f7ee9

[PM-9401] Change dummy FlagKey keyName values

7d928cd

andrebispo5 requested a review from david-livefront August 6, 2024 14:41

david-livefront approved these changes Aug 6, 2024

View reviewed changes

andrebispo5 merged commit 994a577 into main Aug 6, 2024
9 checks passed

andrebispo5 deleted the pm-9401/server-feature-flags branch August 6, 2024 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-9401] Server feature flags manager #3656

[PM-9401] Server feature flags manager #3656

andrebispo5 commented Jul 31, 2024

github-actions bot commented Jul 31, 2024 •

edited

Loading

david-livefront Jul 31, 2024

david-livefront Jul 31, 2024

david-livefront Jul 31, 2024

andrebispo5 Jul 31, 2024

david-livefront Jul 31, 2024

david-livefront Jul 31, 2024

andrebispo5 Aug 2, 2024

david-livefront Jul 31, 2024

david-livefront Jul 31, 2024

david-livefront Jul 31, 2024

shannon-livefront Aug 1, 2024

andrebispo5 Aug 2, 2024

david-livefront Aug 2, 2024

andrebispo5 Aug 5, 2024

shannon-livefront Aug 1, 2024

shannon-livefront Aug 1, 2024

codecov bot commented Aug 2, 2024 •

edited

Loading

david-livefront Aug 2, 2024

david-livefront Aug 5, 2024

andrebispo5 Aug 5, 2024

andrebispo5 Aug 6, 2024

david-livefront Aug 6, 2024

david-livefront Aug 5, 2024

david-livefront Aug 5, 2024

david-livefront Aug 5, 2024

dseverns-livefront Aug 5, 2024

dseverns-livefront Aug 5, 2024

david-livefront Aug 6, 2024

david-livefront Aug 6, 2024

david-livefront Aug 6, 2024

david-livefront Aug 6, 2024

	* @property [stringValue] corresponds to the string value of a give key
	* @property [stringValue] corresponds to the string value of a given key

[PM-9401] Server feature flags manager #3656

[PM-9401] Server feature flags manager #3656

Conversation

andrebispo5 commented Jul 31, 2024

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Jul 31, 2024 • edited Loading

No New Or Fixed Issues Found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2024 •

edited

Loading

codecov bot commented Aug 2, 2024 •

edited

Loading