3box Replacement #15243
Merged
3box Replacement #15243
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
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
hilvmason
added
PR - P1
darkwing
requested changes
Jul 15, 2022
app/scripts/controllers/backup.js
Outdated
| async restoreUserData(jsonString) { | ||
| console.log('jsonString: ', jsonString); | ||
| const { preferences, addressBook } = JSON.parse(jsonString); | ||
| preferences && this.preferencesController.store.updateState(preferences); |
There was a problem hiding this comment.
Nit but I think the following looks a little bit cleaner:
if (preferences) {
this.preferencesController....
}
| const selectedAddress = backupController.preferencesController.getSelectedAddress(); | ||
| assert.equal(selectedAddress, '0x01'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
We should add restoreUserData and backupUserData tests to ensure that we get expected outcomes in each case, since those are the bulk of the work with this PR.
There was a problem hiding this comment.
Hi David,
I can't seem to find a way to mock window functions in sinon for backup so I spoke with @PeterYinusa and we agreed to do an e2e test instead.
I've however added a unit test for restore.
There was a problem hiding this comment.
The e2e suggestion was made to ensure we have some coverage, but is not a replacement for a unit test.
| <span className="settings-page__error-text"> | ||
| {t('restoreFailed')} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
This feels a bit cleaner:
{showResultMessage && restoreSuccessful && (
<span className="settings-page__content-description">
{t('restoreSuccessful')}
</span>
)}
{showResultMessage && !restoreSuccessful && (
<span className="settings-page__error-text">
{t('restoreFailed')}
</span>
)}
{showResultMessage && (
<span className={classnames({
'settings-page__content-description': restoreSuccessful,
'settings-page__error-text': !restoreSuccessful,
})}>
{restoreSuccessful ? t('restoreSuccessful') : t('restoreFailed')}
</span>
)}
| @@ -724,6 +833,8 @@ export default class AdvancedTab extends PureComponent { | |||
| return ( | |||
| <div className="settings-page__body"> | |||
| {warning ? <div className="settings-tab__error">{warning}</div> : null} | |||
| {this.renderUserDataBackup()} | |||
| {this.renderRestoreUserData()} | |||
There was a problem hiding this comment.
I am not sure whether renderUserDataBackup and renderRestoreUserData should be the first entry on the Advanced Tab since the 3box toggle is the 4th entry from the end. Does it make sense to move them closer to the 3box toggle itself?
There was a problem hiding this comment.
@danjm suggested we put it at the top.
There was a problem hiding this comment.
We can ignore my previous suggestion and follow @NiranjanaBinoy's suggestion here
app/_locales/en/messages.json
Outdated
| "message": "Backup your data" | ||
| }, | ||
| "backupUserDataDescription": { | ||
| "message": "Your data contains preferences and account addresses" |
There was a problem hiding this comment.
I think something like this might be more transparent for the user?
You can backup user settings containing preferences and account addresses into a JSON file.
app/_locales/en/messages.json
Outdated
| "message": "Restore user data" | ||
| }, | ||
| "restoreUserDataDescription": { | ||
| "message": "Restore user data from a previously backed up file" |
There was a problem hiding this comment.
Can we update this message to You can restore user settings containing preferences and account addresses from a previously backed up JSON file. or something similar.
app/scripts/controllers/backup.js
Outdated
| } | ||
|
|
||
| async restoreUserData(jsonString) { | ||
| console.log('jsonString: ', jsonString); |
There was a problem hiding this comment.
console.log needs to be removed.
app/scripts/controllers/backup.js
Outdated
|
|
||
| const date = new Date(); | ||
|
|
||
| const pz = (num) => prependZero(num, 2); |
There was a problem hiding this comment.
I know it wouldn't make much difference, but can we update the constant name to prefixZero or something similar instead of pz.
| @@ -51,6 +51,7 @@ export default class ComposableObservableStore extends ObservableStore { | |||
| updateStructure(config) { | |||
| this.config = config; | |||
| this.removeAllListeners(); | |||
| console.log('Config: ', config); | |||
There was a problem hiding this comment.
console.log needs to be removed.
|
The entries for |
|
|
||
| return ( | ||
| <div | ||
| ref={this.settingsRefs[0]} |
There was a problem hiding this comment.
Suggested change
| ref={this.settingsRefs[0]} | |
| ref={this.settingsRefs[15]} |
The index added currently is a duplicate value as 0 is used for the state log entry
| const { t } = this.context; | ||
| return ( | ||
| <div | ||
| ref={this.settingsRefs[0]} |
There was a problem hiding this comment.
Suggested change
| ref={this.settingsRefs[0]} | |
| ref={this.settingsRefs[14]} |
The index added currently is a duplicate value as 0 is used for the state log entry
|
The approach looks good 👍 |
Builds ready [4e0c64d]Page Load Metrics (1813 ± 47 ms)
highlights:storybook
|
PeterYinusa
reviewed
Jul 19, 2022
test/e2e/tests/backup.spec.js
Outdated
|
|
||
| const downloadsFolder = `${process.cwd()}/test-artifacts/downloads`; | ||
|
|
||
| const createDownloadFolder = async () => { |
There was a problem hiding this comment.
Can we reuse this method as we now have this in both the state-logs.spec.js and now backup.spec.js
danjm
reviewed
Jul 19, 2022
| date.getMinutes(), | ||
| )}_${prefixZero(date.getDay())}.json`; | ||
|
|
||
| exportAsFile(userDataFileName, result); |
There was a problem hiding this comment.
We should move the exportAsFile to a utility file in the shared/ directory
There was a problem hiding this comment.
Alternatively, this could be called from the UI. This utility uses the DOM, so it won't work in the MV3 service worker anyway
segun
force-pushed
the
dev-olu-mv3-3box-replacement
branch
from
4e0c64d to
08c590c
Compare
|
I think the ActionableMessage of type |
6 tasks
Builds ready [914f517]
Page Load Metrics (1819 ± 73 ms)
highlights:storybook
|
worldlyjohn
reviewed
Aug 2, 2022
app/scripts/controllers/backup.js
Outdated
|
|
||
| if (preferences && addressBook) { | ||
| this._trackMetaMetricsEvent({ | ||
| event: 'User Restored Data From Backup', |
There was a problem hiding this comment.
please change this to User Data Imported so it pairs well with the export event
darkwing
requested changes
Aug 3, 2022
| // })} | ||
| // > | ||
| // {restoreSuccessful ? t('restoreSuccessful') : t('restoreFailed')} | ||
| // </span> |
There was a problem hiding this comment.
All of this needs to be removed
segun
force-pushed
the
dev-olu-mv3-3box-replacement
branch
from
968b34e to
8b99cb1
Compare
Signed-off-by: Akintayo A. Olusegun <[email protected]> Tests for prependZero (utils.js) Signed-off-by: Akintayo A. Olusegun <[email protected]> Fix advancedtab test Signed-off-by: Akintayo A. Olusegun <[email protected]> backup controller tests Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes Signed-off-by: Akintayo A. Olusegun <[email protected]> Backup controller don't have a store. Signed-off-by: Akintayo A. Olusegun <[email protected]> Restore from file. Signed-off-by: Akintayo A. Olusegun <[email protected]> Advanced Tab tests Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fix Signed-off-by: Akintayo A. Olusegun <[email protected]> e2e tests for backup unit tests for restore. Signed-off-by: Akintayo A. Olusegun <[email protected]> Fix comments on PR. Signed-off-by: Akintayo A. Olusegun <[email protected]> restore style Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes Signed-off-by: Akintayo A. Olusegun <[email protected]> We should move the exportAsFile to a utility file in the shared/ directory Signed-off-by: Akintayo A. Olusegun <[email protected]> Move export as file to shared folder Signed-off-by: Akintayo A. Olusegun <[email protected]> Refactor create download folder methods Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes. Signed-off-by: Akintayo A. Olusegun <[email protected]> Move the backup/restore buttons closer to 3box Signed-off-by: Akintayo A. Olusegun <[email protected]> Change descriptions Add to search Signed-off-by: Akintayo A. Olusegun <[email protected]> refactor code to use if instead of && Signed-off-by: Akintayo A. Olusegun <[email protected]> Lint fixes Signed-off-by: Akintayo A. Olusegun <[email protected]> Restore button should change cursor to pointer. Signed-off-by: Akintayo A. Olusegun <[email protected]> Fix restore not uploading same file twice. Signed-off-by: Akintayo A. Olusegun <[email protected]> Do not backup these items in preferences identities lostIdentities selectedAddress Signed-off-by: Akintayo A. Olusegun <[email protected]> lint fixes. Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
rename event as per product suggestion. Signed-off-by: Akintayo A. Olusegun <[email protected]>
message Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
Signed-off-by: Akintayo A. Olusegun <[email protected]>
segun
force-pushed
the
dev-olu-mv3-3box-replacement
branch
from
8b99cb1 to
f50126f
Compare
Signed-off-by: Akintayo A. Olusegun <[email protected]>
brad-decker
approved these changes
Aug 9, 2022
darkwing
approved these changes
Aug 9, 2022
Builds ready [73bdbdd]
Page Load Metrics (1697 ± 34 ms)
highlights:storybook
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Explanation
Provides a button on top of the Advanced Settings for users to backup and restore the same settings that used to be backed up on 3box.
This is because 3box is being removed from the extension #14571
More Information
Fixes #14888
Screenshots/Screencaps
Pre-Merge Checklist
+ If there are functional changes: