chore: remove mock data from wallet and feature flag incomplete features #18569
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
Jenkins BuildsClick to see older builds (12)
|
OmarBasem
approved these changes
Jan 19, 2024
Comment on lines
+165
to
+168
|
|
||
| (def wallet-feature-flags | ||
| {:edit-default-keypair false | ||
| :bridge-token false}) |
|
@briansztamfater, @vkjr can you check this pr please. It's brief but want to make sure you're both okay with this as it's "removing" work you did. |
vkjr
approved these changes
Jan 19, 2024
briansztamfater
approved these changes
Jan 19, 2024
| @@ -50,7 +51,9 @@ | |||
| :receive-action #(rf/dispatch [:open-modal :wallet-share-address {:status :receive}]) | |||
| :buy-action #(rf/dispatch [:show-bottom-sheet | |||
| {:content buy-drawer}]) | |||
| :bridge-action #(rf/dispatch [:open-modal :wallet-bridge])}]) | |||
| :bridge-action (if (:bridge-token config/wallet-feature-flags) | |||
There was a problem hiding this comment.
To improve readability, maybe would be nice to have some function like feature-enabled? that receives the feature key, located in a utils file. WDYT?
There was a problem hiding this comment.
yeah, I agree! There was discussion in another pr - #18530 (comment)
where we will put some effort into how we want to work with feature flags.
For now I will leave it this way but in an upcoming pr/issue will refactor this
| @@ -12,6 +11,6 @@ | |||
| (defn view | |||
| [] | |||
| [rn/flat-list | |||
| {:data temp/collectible-activities | |||
| {:data nil | |||
There was a problem hiding this comment.
yeah sure, I'll create the issues for the code removed so the data structure is easy to track 👍
J-Son89
force-pushed
the
jc/clean-mocks
branch
from
afa5059 to
e7ff56d
Compare
75% of end-end tests have passedFailed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (36)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
|
1 task
smohamedjavid
approved these changes
Jan 22, 2024
| :section (i18n/label :t/on-the-web)}] | ||
| [rn/view {:style style/link-cards-container} | ||
| (for [item (:cards collectible-about)] | ||
| ^{:key (:title item)} |
There was a problem hiding this comment.
Not a blocker: I know this is not from your PR. I have a feeling that using the name of the collectable as the key is not a good idea as it may have the same names.
ulisesmac
approved these changes
Jan 23, 2024
There was a problem hiding this comment.
This PR was really needed @J-Son89 👍
Just a comment, as @briansztamfater said, I'd like to improve the readability of the feature flags, but we can do it in a later PR.
pavloburykh
force-pushed
the
jc/clean-mocks
branch
from
e7ff56d to
1c6ec2f
Compare
67% of end-end tests have passedFailed tests (13)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (32)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
90% of end-end tests have passedFailed tests (2)Click to expandClass TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (43)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
Hey @J-Son89! Thanks for the PR. Please take a look at the issue. ISSUE 1 Suggested addresses are still shown while entering receiver's addressActual result: telegram-cloud-document-2-5298888018512330314.mp4Expected result: according to PR description
|
|
Thanks @pavloburykh - I missed that one! 🙏 |
ISSUE 2 Editing Derivation Path is not under feature flagActual result: 'Button pressed' alert instead of 'feature disabled in config file' alert when editing Derivation Path at the same time 'feature disabled in config file' alert appears when tapping Edit default key-pair - is it expected? telegram-cloud-document-2-5298888018512330819.mp4Expected result: Editing Derivation Path should be under feature flag |
|
Oh sorry issue 2 is sort of sloppy on my(/devs) part - it hadn't been started yet so there is no feature flag there. |
'coming soon' alert text would be okay IMHO. |
|
Ok thanks, I'll update with that 👌 |
J-Son89
force-pushed
the
jc/clean-mocks
branch
from
1c6ec2f to
94c0a6e
Compare
|
@pavloburykh - For issue 1 - this was an oversight on my part. the reason I assumed the suggestions would be empty is because I removed the mock data, however this is actually the resolved address of the ENS being looked up and so it is real data. So from the perspective of what this pr is achieving to do this is not an issue (as it's real data) However @briansztamfater did find some other issue related to that and has addressed in a small pr here: Let me know what you think to do from here? |
|
Thanx @J-Son89. No other issues from my side. But a small question: we also have 'to be implemented' alerts all over the app including wallet telegram-cloud-document-2-5303391618139701348.mp4Are we going to change those alert to a new one 'Coming soon'? Is there any difference between the purpose of those 2 different alerts? |
|
They're the exact same, we can look to standardise this in another issue. Probably a better approach and will be easier for us to track too |
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.
fixes: #18378
This pr removes mock data for the following features
Removed mock data from Collectibles Overview Page
Added Feature flag for Bridge page and for Edit Derivation Path
https://github.com/status-im/status-mobile/assets/22799766/eccd0fe1-8a8c-421b-a8a4-529cd0e27393
≈i.e on this page:
Note: I will create features to correctly restore all of these features with real data and with that I will link this pr so that the respective data structures are not lost 👍
Happy to rework this approach if there is a preferable alternative.
To test:
View a collectible page - you will see the activity is now not displaying
you will also see the socials links are now not showing - these were on the about tab of the collectibles view
Also on the Account input there is no suggested addresses showing now - these were mock previously.
i.e go to account page - click send button - start inputting an address
Added Feature flag for Bridge page
Added Feature flag for Edit Derivation Path