Add a UI for toggling developer feature flags #18602
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
J-Son89
changed the title
Jenkins BuildsClick to see older builds (37)
|
J-Son89
commented
Jan 23, 2024
| status-im-preview/main-screens))) | ||
| status-im-preview/main-screens) | ||
|
|
||
| (when config/quo-preview-enabled? |
There was a problem hiding this comment.
we should rename this var quo-preview-enabled? to something more generic
J-Son89
commented
Jan 23, 2024
| @@ -0,0 +1,30 @@ | |||
| (ns status-im.contexts.preview.feature-flags.view | |||
There was a problem hiding this comment.
we need to rename preview to something like developer or something to that effect
There was a problem hiding this comment.
Or just drop it all together?
feature-flags in itself is a good context name.
There was a problem hiding this comment.
ah, I believe there is some aim to not overload the context folder with too many direct folders as these are mainly higher concepts (related to designs files/functionality) mostly.
e.g
contexts/chat
contexts/shell
contexts/communities
and the problem becomes that the codebase is a bit more confusing to navigate if we start adding small sets of functionality within contexts.
but maybe feature flags can live somewhere better anyway 🤔
I'll leave for now but happy to have it moved going forward
J-Son89
commented
Jan 23, 2024
src/status_im/feature_flags.cljs
Outdated
| (fn [a] | ||
| (update-in a [section flag] not)))) | ||
|
|
||
| (defn alert [context flag action] |
There was a problem hiding this comment.
a simple abstraction idea, we can have others depending on the use case 👍
J-Son89
force-pushed
the
jc/feature-flags
branch
from
1c43b02 to
2269587
Compare
J-Son89
commented
Jan 23, 2024
src/status_im/feature_flags.cljs
Outdated
| (def ^:private feature-flags-config | ||
| (reagent/atom | ||
| {:wallet | ||
| {:edit-default-keypair (check-env :DEV_FF_EDIT_DEFAULT_KEYPAIR) |
There was a problem hiding this comment.
should allow for defaults to be set by an env var so devs can have a better dev experience - didn't quite get this working for new vars, perhaps dependent on the app being built for it to load?
@siddarthkay do you know?
better again would be that we can use env.dev or something hidden by gitignore 👍
There was a problem hiding this comment.
looks good to me overall,
should allow for defaults to be set by an env var so devs can have a better dev experience - didn't quite get this working for new vars, perhaps dependent on the app being built for it to load?
I think that's desireable,
if you need to pull from env, you will have to rebuild the code with the new env variable set, since they are build time flags, I think.
We have already a bunch of feature flags in config maybe worth moving those as well at some point?
the only thing that is missing that I can see, is that some feature flags are not UI only (anything that is also toggling status-go code will want for example to be making potentially an RPC and store it in settings, so you have it on when you login/logout/restart etc), maybe we should accommodate for that use case (some hooks or something)? (@alwx is working on adding a feature flag for some status-go code).
Also worth considering if you want to store in async storage, just to make it a bit more permanent, it might be a bit unexpected for a developer that make run.. clears your feature flags (unless you set them in the env file), but either is fine I think, this is simpler.
There was a problem hiding this comment.
We have already a bunch of feature flags in config maybe worth moving those as well at some point?
👍🏼
Also worth considering if you want to store in async storage, just to make it a bit more permanent
Nice 👍🏼! Good idea for a follow-up
J-Son89
commented
Jan 23, 2024
src/status_im/feature_flags.cljs
Outdated
|
|
||
| (def ^:private feature-flags-config | ||
| (reagent/atom | ||
| {:wallet |
There was a problem hiding this comment.
seperated by context mostly for the ui but this can also be one keyword.
e.g :wallet.edit-default-keypair
There was a problem hiding this comment.
I personally prefer without nesting, so that the function get-flag and others receive only one keyword argument. It simplifies tremendously grepping in the project. We would then qualify the flag, like :wallet/edit-default-keypair.
Perhaps even better is if we namespace the key since we have already required the namespace anyway to call functions like ff/alert. Namespacing would make them even more unique in the system. clojure-lsp can find references to namespaced keys very precisely. Given that feature flag keywords will be concentrated in one namespace, we wouldn't have to worry about updating them in the code because we wouldn't be moving them out of status-im.feature-flags.
;; In status-im.feature-flags
(def ^:private feature-flags-config
(reagent/atom
{::wallet.edit-default-keypair ...
::wallet.bridge-token ...}))
;; ::wallet.edit-default-keypair will be expanded behind the
;; scenes to status-im.feature-flags/wallet.edit-default-keypair
;; Anywhere else, e.g. status-im.contexts.wallet.create-account.view
(ff/alert ::ff/wallet.edit-default-keypair
(fn []
(rf/dispatch [:navigate-to :wallet-select-keypair])))
J-Son89
requested review from
pavloburykh,
OmarBasem,
smohamedjavid,
clauxx and
ulisesmac
J-Son89
changed the title
|
I totally like the idea! 💯 |
|
➕ 1️⃣ |
|
Looks great 💯 |
|
gave it a smoke test and looks fine @VolodLytvynenko 👍 |
69% of end-end tests have passedFailed tests (14)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
@J-Son89 Apologies, I was on day off yesterday. I'm now addressing this PR. Thank you for smoke |
|
hi @J-Son89 one question. Should the account be able to be removed if 'remove-account' toggle is activated? For not it does not work so remove_acc.mp4 |
J-Son89
force-pushed
the
jc/feature-flags
branch
from
1c6e74f to
506331e
Compare
J-Son89
force-pushed
the
jc/feature-flags
branch
from
506331e to
0ac4e02
Compare
|
apologies, yes the remove account feature should be available. I have fixed that now and the remove account feature flag should be correctly hooked up. |
|
hi @J-Son89 Thank you for PR> These are issues that can be addressed in future tasks. Please take a look at them if they should be fixed as part of this PR. If not, the PR can be merged and I will create them in a separate follow ups. ISSUE 1: Unable to remove an account with available tokens/collectiblesSteps to reproduce:
Actual result:The account with available tokens/collectibles cannot be removed. remove.mp4Expected result:
|
ISSUE 2: Watch-Only Addresses Available for BridgingSteps:
Actual Result:Watch-only accounts are shown and can be selected for bridging. bridge.mp4Expected Result:Watch-only accounts should not be displayed as options for bridging |
|
Great finds @VolodLytvynenko! 🙏 Let's handle these in separate issues as their scope is beyond to this pr |
65% of end-end tests have passedFailed tests (16)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (31)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
81% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (13)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr: #18530 and some recent discussions in the Wallet Team to start using Feature Epics (e.g #18568) encouraged the need for small improvements in how we use feature-flags.
Note - Please review by commit
chore: add feature flags poconlyCreated a simple UI page for a devs, QA, designer to toggle on/off these feature flags - intended for development purposes only
I wanted to bring this discussion so this pr is completely open to suggestion and hapopy to scrap the whle thing and take a new approach if needs be.
quick demo:
Screen.Recording.2024-01-23.at.21.03.48.mov
I didn't pull other features into this screen just yet as this can be done in a follow up.
Testing notes:
two features are feature flag.
The bridging - go to a wallet account, press the "Bridge" button.
The second is edit keypair, go to "Add account", press "edit" button beside " default keypair"