Improve remote and local feature flag handling#15299
Conversation
Generated by 🚫 Danger |
| suspend fun isRemoteFeatureFlagEnabled(key: String): Boolean = withContext(Dispatchers.IO) { | ||
| featureFlagsStore.getFeatureFlagsByKey(key).firstOrNull()?.value ?: false | ||
| suspend fun isRemoteFeatureFlagEnabled(key: String): Boolean? = withContext(Dispatchers.IO) { | ||
| featureFlagsStore.getFeatureFlagsByKey(key).firstOrNull()?.value |
There was a problem hiding this comment.
This function will now return null if the key can't be found in remote flags.
| operator fun invoke(): Flow<Boolean> = selectedSite.observe() | ||
| .flatMapLatest { site -> | ||
| flow { | ||
| if (!featureFlagRepository.isEnabled(FeatureFlag.WOO_PUSH_NOTIFICATIONS_SYSTEM_M2)) { |
There was a problem hiding this comment.
I made some changes in this file to be able to suspend isEnabled function in the flow.
| FeatureFlag.BETTER_CUSTOMER_SEARCH_M2.isEnabled() | ||
| addressViewModel.viewStateData.liveData.value?.isBetterCustomerSearchEnabled == true |
There was a problem hiding this comment.
I moved this flag to the view model
| banner.isVisible = isVisible && FeatureFlag.WC_SHIPPING_BANNER.isEnabled() | ||
| banner.isVisible = isVisible && isWcShippingBannerEnabled |
There was a problem hiding this comment.
I moved this flag to the view model.
| val initialValues = remember { | ||
| allFeatureFlags.associateWith { it.isEnabled() }.toMutableMap() | ||
| } | ||
| var hasChanges by remember { mutableStateOf(false) } | ||
|
|
||
| fun updateHasChanges(flag: FeatureFlag, newValue: Boolean) { | ||
| initialValues[flag]?.let { _ -> | ||
| hasChanges = allFeatureFlags.any { featureFlag -> | ||
| val currentValue = if (featureFlag == flag) newValue else featureFlag.isEnabled() | ||
| currentValue != initialValues[featureFlag] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Since the logic in this screen became larger, I moved this logic to a new ViewModel.
| */ | ||
| override fun onDowngrade(db: SQLiteDatabase?, helper: WellTableManager?, oldVersion: Int, newVersion: Int) { | ||
| if (FeatureFlag.DB_DOWNGRADE.isEnabled(context)) { | ||
| if (PackageUtils.isDebugBuild() && context != null && PackageUtils.isBetaBuild(context)) { |
There was a problem hiding this comment.
I removed DB_DOWNGRADE flag since it was not actually a feature flag. We'll never switch this boolean here. So I moved the same logic from the old FeatureFlag class to here.
Also since this class runs before Hilt, we can't inject FeatureFlagRepository here.
|
|
||
| @Test | ||
| fun `Handles database downgrade correctly`() = testBlocking { | ||
| if (FeatureFlag.DB_DOWNGRADE.isEnabled()) { |
There was a problem hiding this comment.
FeatureFlag.DB_DOWNGRADE.isEnabled() was redundant here because this logic was always true in unit tests.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
…ure-flag # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/notifications/push/ShouldShowEnablePushNotificationsUi.kt
|
Hey @malinajirka just caught up with you comment:
I think @irfano replied really well to what I meant. I think the key is this:
And that's why the About using option B out of this 3:
Imho I'd stick with A. I think B introduces harder mental model and unnecessary complexity. In any case, I don't have a strong opinion on it. So either options sound good to me.
I'd say, now that you have started to work on this here. I'd wrap this up in this same PR. However, one thing I'd suggest doing is writting a P2 recap in Woomobile P2 to inform all Android devs about the new FF system, how it works and what benefits are we aiming for with these changes. |
irfano
force-pushed
the
issue/WOOMOB-2129-improve-remote-local-feature-flag
branch
from
fc0e375 to
e519e27
Compare
|
@JorgeMucientes , after reading Andrei's comment I realized that Consider this scenario:
To avoid that, I updated the resolution logic. For our use case, Jirka’s suggestion makes more sense: the local flag should gate feature readiness, and the remote flag should only control rollout once the local flag is enabled. I pushed the change here: cf1b6ae. |
Hey @irfano good point. Might be missing something, but I'm not sure how that would ever happen given remote FF are enabled on app version basis. For example, a recent remote flag that was added: So in the above scenario the remote FF would be enabled starting on version 23 onwards.
I'm ok with following that approach but that doesn't have the benefit you stated here:
|
|
Good point, @JorgeMucientes. You're right: if we use rules in the remote flag configuration, then my latest change is less necessary.
|
…ure-flag # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrderActionsProvider.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/util/FeatureFlag.kt # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersViewModelTest.kt
malinajirka
left a comment
malinajirka
left a comment
There was a problem hiding this comment.
Thanks again for working on this @irfano!
I'm at a meetup this week, and then another event for 2 weeks. I did my best to review the latest changes myself as well as with the help of AI and it all looks good to me. Having said that, I couldn't find time to do a proper testing of the latest changes. Might be worth asking someone to do a manual test just to be sure.
There was a problem hiding this comment.
Actually, I think it doesn't work as expected.
Remote-only flags like WOO_POS default to localValue = isDebugBuild(), which is false in release. Since the formula is local && remote, the remote value is ignored and the feature is always off in production.
Fix: set localValue = true for flags that previously existed only as RemoteFeatureFlag.
Wdyt?
|
Good catch, @malinajirka! My initial logic was |
|
Thanks for the fix @irfano ! LGTM. A couple more behavioral changes I noticed (with the help of AI). I believe it would be good to double check these are expected and intentional. I think we discussed some of them, I'm not entirely sure if all of them. Following better safe than sorry principle, let's just double check. 1. Missing remote value: pessimistic → optimistic default
"Absent" happens on fresh install before the first remote fetch completes (Room DB is empty). awaitRemoteFlagsLoaded() doesn't help — Room emits an empty list immediately, which satisfies the await, but all keys resolve to null. 2. Debug builds no longer force-enable remote flags Old: IsRemoteFeatureFlagEnabled returned isDebugBuild() || remoteValue, so debug builds always got true regardless of remote. New: effectiveValue = localValue && (remoteValue ?: true). If remote explicitly returns false, debug builds now see false. 3. WOO_POS_CLIENT_SIDE_BANNER change Old: two separate flags — WOO_POS_CLIENT_SIDE_BANNER (local, isDebugBuild()) acted as a gate. Release always returned false before checking remote. New: single flag WOO_POS_TABLET_PROMO_BANNER with localValue = true. In release, the banner can appear if remote is true or absent. It enables the banner in production. Sorry for not sharing everything in one go, but this is quite complex to reason about. |
| fun observeIsEnabled( | ||
| flag: FeatureFlag | ||
| ): Flow<Boolean> = remoteFlagValues.map { isEnabled(flag) }.distinctUntilChanged() |
There was a problem hiding this comment.
I'm worried this could lead to weird bugs for the users with hard-to-reproduce steps (FF changing while executing steps). I would prefer to have a "known" FF state for a given app launch. So, unless the app is restarted, FF values are constant.
There was a problem hiding this comment.
This PR doesn't introduce active feature flag observation. Before, local flags were synchronous and remote flags were asynchronous. When I unified them, I didn’t want to keep both isEnabled() and suspend isRemoteEnabled(), because that would make the unification pointless.
As solution, I introduced in-memory remoteFlagValues into FeatureFlagRepository. These in-memory remoteFlagValues are simply the in-memory equivalent of the remote flag values stored in the DB. Reading from the DB requires suspend functions, while reading the in-memory remoteFlagValues is synchronous. I keep the in-memory values updated via a DB observer.
init {
appCoroutineScope.launch {
featureFlagsStore.observeFeatureFlags().collect { remoteFlags ->
remoteFlagValues.value = remoteFlags.associate { remoteFlag -> remoteFlag.key to remoteFlag.value }
}
}
}
However, this introduced another risk: the in-memory values can be stale after the DB is updated, until the observer receives the change. That is why I originally added observeIsEnabled().
But this is confusing. Having both observeIsEnabled() and isEnabled() goes against the purpose of the unification goal. After reconsidering it, I concluded that observeIsEnabled() is not really addressing a meaningful risk. Let me elaborate:
Old way
- When the site is switched, fetching remote flags is triggered without waiting for the result.
- After the remote flags are received, they are saved to the DB.
- When the app needs to read a remote flag, it reads it from the DB.
⚠️ There is a risk between steps 1 and 3. If step 2 takes too long, we may read stale data.
New way
- When the site is switched, fetching remote flags is triggered without waiting for the result.
- After the remote flags are received, they are saved to the DB.
- After they are saved to the DB, the in-memory
FeatureFlagRepository.remoteFlagValuesis updated. - When the app needs to read a remote flag, it reads it from the in-memory
FeatureFlagRepository.remoteFlagValues. ⚠️ There is a risk between steps 1 and 4. If steps 2 and 3 take too long, we may read stale data.
observeIsEnabled() was intended to solve the new risk in step 3. But in practice, the risk introduced by step 3 is negligible compared to the existing risk in step 2. So observeIsEnabled() does not really solve anything meaningful and instead adds extra confusion. For that reason, I changed the implementation and removed observeIsEnabled().
I’d appreciate it if you could read my reasoning above and confirm whether it makes sense.
I would prefer to have a "known" FF state for a given app launch. So, unless the app is restarted, FF values are constant.
This is already the current behavior on trunk, and also in this PR, with one slight difference: feature flags are fetched not only on app startup, but also when switching sites.
Yes, this is a behavioral change. When the remote flag is absent, we use localValue. A remote flag can be absent for two reasons:
The new system does not support the old behavior described in 2. But do we really need it? Consider WOO_POS: if the remote WOO_POS is absent for any reason, such as an error, I think it’s better to fall back to true because the feature is complete. Do you think we should preserve the old behavior? If we want to keep it, we should change this to Remote > Local and rely on remote Rules for feature-ready versions, or give up on unifying the flags.
I think this is better. There may be multiple features that are remotely disabled, and developers do not need to see them: 59df59f
There were both a local flag and a remote flag used here, and we intentionally unified them into a single flag, but the behavior remains the same.
No worries! Thank you for looking into this while you were at the meet-up. You shared valuable feedback. |
|
Version |
|
I believe this is not entirely precise: Thanks again for working on all this! |
Description
Fixes WOOMOB-2129
This PR introduces an injectable
FeatureFlagRepositoryand routes runtime feature flag reads through it instead of staticFeatureFlag.isEnabled()calls and flag-specific wrappers. The goal is to make feature flag resolution testable and consistent while keeping reads synchronous at call sites.Feature Flag Resolution Model
Each flag can now be resolved from three inputs:
Override: developer-set override from the Feature Flags screenLocal: app-defined readiness gate inFeatureFlagRemote: backend value fetched from WPCom and cached locallyflowchart TD A["Override set?"] -->|Yes| B["Use Override"] A -->|No| C["Local flag enabled?"] C -->|No| D["Feature Disabled"] C -->|Yes| E["Remote flag exists?"] E -->|No| F["Feature Enabled"] E -->|Yes, true| F E -->|Yes, false| DEquivalent formula:
This means:
Overridestill has the highest priority.Localacts as a readiness gate.Remoteonly controls rollout for features that are already locally enabled.Developer Overrides UI
The Developer Options → Feature Flags screen now:
FeatureFlagRepositoryDefault,Disabled,Enabled)LocalorRemoteImportant implementation decisions
isEnabled()remains synchronous.Instead of making feature flag reads
suspendeverywhere, remote values are observed into an in-memory snapshot and runtime reads stay synchronous.FeatureFlagRepositoryis now the single runtime access point.This keeps call sites simple and makes tests easier to write.
isEnabledLocally()/isEnabledRemotely().That would leak flag source details into consumers and make future migrations easier to miss.
Review focus
The most important places to review are:
Core feature flag model
This is the new resolution model (
Override ?: (Local && (Remote ?: true))) and the persistence path behind it.FeatureFlagFeatureFlagRepositoryAppPrefsWPComRemoteFeatureFlagRepositoryFeatureFlagsStore/FeatureFlagConfigDaoDeveloper overrides UI
This is where the new model is exposed to developers and where the tri-state override behavior can be validated end to end.
DevFeatureFlagsScreenDevFeatureFlagsViewModelConsumers where timing matters
These are the places where the switch to the in-memory snapshot model matters most, because they either observe flag changes or wait for the initial cached snapshot before evaluating.
Observation was not a hard requirement, and the previous synchronous access would still have worked, since flags are not refreshed continuously while the app is running anyway. I still added observation in the areas listed below to better cover possible edge cases and leave room for future improvements.
ShouldShowEnablePushNotificationsUi: This path already returns aFlow, so it now observes the flag reactively instead of taking a one-time snapshot that could stay stale if the cache hydrates later.JetpackBenefitsViewModel: The visibility of the push-notifications benefit is now updated from observed flag state instead of being fixed at ViewModel creation time.ObserveBookingsVisibility: This now combines selected-site observation with reactive flag observation, so the visibility decision can recover correctly after cache hydration.OrderDetailViewModel/OrderDetailFragment: The flag is now resolved in the ViewModel and carried through view state instead of being checked directly in the Fragment.AddressViewModel/OrderCreateEditCustomerAddFragmentTaxRateSelectorViewModel/TaxRateSelectorScreen: The bottom bar visibility is now driven by ViewModel state backed by observed flag updates.WooApplicationPasswordsConfiguration: This is a one-shot gate, so it now waits for the initial cached remote flag snapshot before deciding whether App Passwords can be used.WooPosTabShouldBeVisible: This is another one-shot startup decision, so it now waits for the initial cached remote flag snapshot before evaluating visibility.ClientSidePosBanner: This now uses the realwoo_pos_tablet_promo_bannerremote key together with a local readiness gate throughFeatureFlagRepository.Test Steps
I tested some local and remote flags (
woo_pos_local_catalog_m1,bookings_mvp,woo_app_passwords_for_jetpack_sites). You can also choose some features, disable them from "Developer Options → Feature Flags", restart the app, and verify that the feature is disabled as expected.Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.