Feature/bma/enable polls #1246
Feature/bma/enable polls #1246
Conversation
Fix typo, rename class and interface, add doc, do small refacto, to improve code clarity.
…ue, and `StaticFeatureFlagProvider` return the default value. This fixes #1241.
bmarty
force-pushed
the
feature/bma/enablePolls
branch
from
74e3cc9 to
aacfa0d
Compare
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #1246 +/- ##
========================================
Coverage 57.71% 57.71%
========================================
Files 1078 1078
Lines 28038 28039 +1
Branches 5777 5777
========================================
+ Hits 16181 16182 +1
Misses 9330 9330
Partials 2527 2527
☔ View full report in Codecov by Sentry. |
| enum class FeatureFlags( | ||
| override val key: String, | ||
| override val title: String, | ||
| override val description: String? = null, | ||
| override val defaultValue: Boolean = true | ||
| override val defaultValue: Boolean |
There was a problem hiding this comment.
I'd keep the defaultValue to true, so it's enabled by default for debug
There was a problem hiding this comment.
I do not agree, for debug, you can enable it in the developer option.
There was a problem hiding this comment.
But I admit it can be an issue if the flags is impacting the FTUE flow...
There was a problem hiding this comment.
Yes of course, but defaultValue was more intended to be use as a fallback value.
We should probably add more comment then, adding a reference to the StaticFeatureFlagProvider.
There was a problem hiding this comment.
Developers (or people enabling flags) do not have to know about StaticFeatureFlagProvider IMO.
There was a problem hiding this comment.
And IMO, it's the only file they have to know :D
We could add a ReleaseFeatureFlags file somewhere, and StaticFeatureFlagProvider would just read from it?
There was a problem hiding this comment.
Maybe it's a naming issue with defaultValue.
We can have a flag with defaultValue = false, and the feature enabled in StaticFeatureFlagProvider. Too confusing IMO.
There was a problem hiding this comment.
I can't really find a better name like this...default is the value if it's not overridden by any provider...
| FeatureFlags.Polls -> false | ||
| FeatureFlags.NotificationSettings -> false | ||
| } | ||
| feature.defaultValue |
There was a problem hiding this comment.
IMO StaticFeatureFlag should really be clear about which feature is enabled or not.
Using a when makes it exhaustive, so it's the responsability of the developer to enable it or not before the release. Maybe we should just add a comment at the beginning of the when to make it clear
There was a problem hiding this comment.
Developers forget to update the value here, they just do it in FeatureFlags, like for instance in this PR: #1196.
With this change, I think there is no more possible mistake 🤞
There was a problem hiding this comment.
We should just educate developers, it should be clear which feature or not is enabled in release, and it shouldn't affect other providers.
There was a problem hiding this comment.
I see. I will try to improve this then.
Ideally we would have a centralized configuration file, that can also be used for gradle Kotlin files.
There was a problem hiding this comment.
Yes but we should makes sure it's exhaustive...
… `StaticFeatureFlagProvider.isFeatureEnabled()`. Iterate after Ganfra's review.
|
Updated with 762a032. |
...featureflag/api/src/main/kotlin/io/element/android/libraries/featureflag/api/FeatureFlags.kt
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed! |
Type of change
Content
Small rework of the feature flag module, to improve code clarity and avoid mistake.
Enable polls in release build.
Motivation and context
Fixes #1241
Screenshots / GIFs
Tests
Tested devices
Checklist