Add "Import recovery keys" button to Payments->Advanced->Recover #5809

willy-b · 2016-11-23T05:58:27Z

Add "Import recovery keys" button that opens a file chooser dialog

Submitted a ticket for my issue if one did not already exist. (issue Import recovery keys feature #4806)
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Test Plan

Automated test plan

# window 1
npm run watch-test
#window 2
npm run uitest -- --grep="Payments Panel -> Advanced Panel"

willy-b · 2016-11-23T05:58:53Z

cc/ @luixxiul

bbondy · 2016-11-28T04:14:21Z

Would you mind rebasing this on master? Thanks!

willy-b · 2016-11-28T06:26:57Z

@bbondy: no problem, rebased against latest

bsclifton · 2016-12-02T17:52:04Z

@willy-b doh- needs another rebase. If you can get that done, I'll make sure to review in a timely manner

willy-b · 2016-12-02T19:55:00Z

@bsclifton , rebasing and adjusting for the upstream changes in the Recovery Key File format now

willy-b · 2016-12-02T22:16:51Z

OK @bsclifton , rebase is done and force-pushed here.

(If needed to check something, the old branch was copied to https://github.com/willy-b/browser-laptop/tree/add-import-recovery-keys-button-4806-rebase-old )

Thanks!

bsclifton · 2016-12-04T08:31:44Z

Very cool feature, @willy-b! 😄

Some notes after reviewing:

When choosing a recovery file and you hit cancel, it shows the error screen. Could we change this to not show an error and expect the user to click the "import recovery keys" button again?
When importing is a success, it shows a nicely formatted message showing the balance, etc. However, it stays on the Advanced Settings screen. When user clicks the "OK" button (and it's a success), can we close all the modals so that they're returned to the regular Brave Payments screen?
The format of the recovery keys file which is read doesn't appear to be checked which can lead to folks falsely thinking they've restored properly. Can we validate the contents of the file or give a better response? For example, I backed up my wallet and then edited the text file; these scenarios all give a success message saying 0 BTC was imported:
- removing parts of one or both recovery keys
- removing the 2nd recovery key
- removing both of the recovery
- deleting all the contents of the file

willy-b · 2016-12-14T00:20:52Z

Don't necessarily re-review yet, one more commit coming! This commit just contains tests for saving and recovering the key file which I wanted to keep distinct. Thanks.

willy-b · 2016-12-15T22:00:37Z

OK @bsclifton, this should be ready for re-review. Everything in your previous comment should be fixed now. After review I'll do a rebase and force-push if it's OK with you.

posix4e · 2016-12-21T00:54:02Z

Any reason why we can't clear this out? I thin kthis already works?

bsclifton · 2016-12-21T00:56:42Z

@posix4e I haven't had time to review ☹️ If you have some time and can try this out (needs a rebase), I'd say go ahead and try it 😄

NOTE: if it looks good, let's just tag with ready for merge, since it would be nice to avoid pulling more features into 0.13.0.

posix4e · 2016-12-21T01:13:01Z

@bsclifton roger that

posix4e · 2016-12-21T01:15:08Z

Actually @bsclifton not sure what this commit does that we don't already @willy-b

willy-b · 2016-12-21T01:37:17Z

Hey @posix4e, thanks for taking a look.

Re your question, there's a few commits here:

If you're coming from Wallet Recovery success dialog should close parent dialog when hitting OK #6263 , there's a single commit that can be cherry-picked to fix that (close the Advanced and Recover popups when importing keys successfully) -> ~~~e01ad04~~~ https://github.com/willy-b/browser-laptop/commit/f1d9e52265bd6cffd83af81d6ee69ef281ead9b2 (rebased)
The main feature of this PR is adding a button to allow import of the recovery file, so the user doesn't have to parse it and enter the keys manually (originally requested in Import recovery keys feature #4806).
This PR also adds the first tests for the existing recovery keys backup feature, afaik

posix4e · 2016-12-21T02:11:43Z

@willy-b wanna rebase then?

willy-b · 2016-12-21T02:57:59Z

Sure, rebasing now (just pushed a rebase but it was to a somewhat out of date master, and it turns out there have been changes to the brave test library since then, so resolving those conflicts now).

The command for the new tests for the Payments -> Advanced modals is:
npm run test -- --grep="Payments Panel -> Advanced Panel"

willy-b · 2016-12-21T03:28:58Z

Rebased against latest master, thanks.

bsclifton · 2016-12-26T04:55:37Z

@willy-b can you update the commit that fixes #6263 to have the "Fixes #6263" keyword in the commit? 😄

Sorry for the extremely long turn-around here. I know all of us here at Brave are hustling on the open items for 0.13.0, which will be our first release with Chromium 54 (which has caused us to have major architectural changes). This is definitely an important fix and I'm excited about pulling it in (along with the other PRs for 0.13.1).

willy-b · 2016-12-28T20:37:18Z

hey @bsclifton

added the ref to Wallet Recovery success dialog should close parent dialog when hitting OK #6263 in commit https://github.com/willy-b/browser-laptop/commit/f1d9e52265bd6cffd83af81d6ee69ef281ead9b2 as requested
rebased / tested this whole PR against latest master

recommend merging this PR (w/ tests) rather than cherry-picking for #6263, just imho.

willy-b · 2016-12-29T00:21:33Z

note: just saw that #6422 is about to come in; on review I see that this PR will need an update after a6cc6a5 is merged.

so if #6422 comes in before this, then after final version of #6422 is merged let me update this again, thanks.

willy-b · 2016-12-30T19:00:29Z

heads up, 2 more pending PRs which may cause silent conflict when merged, bringing total to 3:

(if these are merged before this is, hold this until I or someone else tests it again. otherwise I think this is clear)

luixxiul · 2017-01-02T08:36:53Z

Is this ready-for-merge, or do we need another review?

willy-b · 2017-01-02T17:21:08Z

This is ready-to-merge, unless any of #6455, #6456, or #6422 are merged first. (update 1/4/2017: also #6529)
cc @bsclifton

willy-b · 2017-01-09T08:46:25Z

looks like you just rebased it and resolved conflicts yourself @bsclifton. thanks!
beat me to it -- was still testing locally, have some test issues with latest master.

I'll pull and test what you've got here then.

bsclifton

(finally had time to review)
Great work, @willy-b! 😄

I went through this pretty thoroughly and had a few updates which I made directly:

rebased against master
reverted changes from return => yield in ledgerPanelTest.js. These changes broke the tests for me. I verified reverting this works.
Removed unneeded .bind() in ledgerPanelAdvancedPanelTest.js (lint error was thrown; you may have pre-commit hooks disabled)
ran docs update task to properly update action descriptions (usually ran in post-commit hook)
squashed it all down to 1 commit

I verified that all "Payments Panel" tests pass (ran multiple times) and updated the PR to show the other issue this resolves

willy-b · 2017-01-09T18:17:35Z

thanks @bsclifton!
I'm trying to test this but as was the case yesterday I continue to have build / run issues with latest master (e.g. webpack server turns off after I run the app, so end up with a blank window and localhost:8080 refused errors in console). I'll update you once that is resolved and I can test your changes.

willy-b · 2017-01-09T18:40:02Z

FYI, #6455 just came in so may be silent conflict with that.

bsclifton · 2017-01-09T18:41:38Z

@willy-b yup- understood, as you mentioned here #6447 (comment) 😄

willy-b · 2017-01-09T23:46:23Z

hey @bsclifton, I couldn't get that branch to build for whatever reason -- probably a local issue.
Unfortunately I can't commit to review this until this weekend. Feel free to re-assign or do what you have to do.

bsclifton · 2017-01-10T06:18:43Z

@willy-b no review needed 😄 I spent a good amount of time with it and feel confident enough to merge

We've changed a few things about our PRs which may be confusing (especially since I am not sure we documented it!)

Assignee for the PR will be the folks who are doing the change OR own the change
Reviewers would be the folks who do the actual review in GitHub and approve/deny

I'll make sure our docs get updated. Thanks again; great work on this 😄

luixxiul · 2017-01-11T17:23:12Z

Another rebase is needed and 7 failing tests need passing to be merged in 0.13.1-branch.

(opens a file chooser dialog) fixes #4806 Includes tests for Ledger backup and recovery: - add advanced ledger panel tests file `test/components/ledgerPanelAdvancedPanelTest.js` - add tests for backup and recovery of wallet - add commands to Brave test client (`ipcOnce`, `ipcSendRendererSync`, and `translations`) client.translations: returns a map of all existing translations (current locale) to test client Import recovery keys success closes modals - successful import closes modals - and closing file chooser dialog does not trigger error screen fixes #6263 Import recovery keys shows error popover if keys are invalid or missing - error popover is displayed if paymentId / passphrase are missing or not UUIDs (ledger-client#recoverWallet should probably do validation too) - added tests for cases: one or both recovery keys missing from file a recovery key is not a UUID an empty recovery file

bsclifton · 2017-01-12T09:01:04Z

CI build had 3 errors; I ran each locally (one being a group of 6) and they each passed great. I removed the commit I had done to increase the timeout (it didn't improve things).

Changes are good to go; merging to 0.13.1-branch 😄

luixxiul · 2017-01-12T09:06:48Z

Nice work @willy-b 👍

luixxiul added the feature/rewards label Nov 28, 2016

bsclifton added this to the 0.13.1 milestone Dec 2, 2016

bsclifton self-assigned this Dec 2, 2016

willy-b mentioned this pull request Dec 8, 2016

Remove a dark background to the bottom layer if the modal contains .coinbaseOverlay #6093

Closed

willy-b mentioned this pull request Dec 16, 2016

Wallet Recovery success dialog should close parent dialog when hitting OK #6263

Closed

willy-b mentioned this pull request Dec 21, 2016

npm registry has 'brave' taken by another user, should use 'brave-browser' instead #6378

Closed

willy-b mentioned this pull request Dec 29, 2016

Ledger recovery error if fields aren't set #6447

Closed

bsclifton assigned willy-b and unassigned bsclifton Jan 9, 2017

bsclifton self-requested a review January 9, 2017 07:45

bsclifton added the QA/test-plan-specified label Jan 9, 2017

bsclifton added PR/ready-for-merge and removed PR/needs-QA-attention ☕ labels Jan 9, 2017

bsclifton approved these changes Jan 9, 2017

View reviewed changes

bsclifton changed the base branch from master to 0.13.1-branch January 10, 2017 17:03

luixxiul removed the PR/ready-for-merge label Jan 11, 2017

bsclifton merged commit 485309f into brave:0.13.1-branch Jan 12, 2017

bsclifton mentioned this pull request Jan 12, 2017

Import recovery keys feature #4806

Closed

luixxiul removed the QA/test-plan-specified label Jan 18, 2017

willy-b mentioned this pull request Jan 30, 2017

Fix disabled ledger / payments panel tests #6911

Closed

This was referenced Feb 3, 2017

Manual Tests for Windows x64 0.13.2 RC2 #7030

Closed

Manual Tests for Windows ia-32 0.13.2 RC3 #7029

Closed

Manual Tests for Linux 0.13.2 RC3 #7028

Closed

Manual Tests for OS X 0.13.2 RC2 #7027

Closed

alexwykoff added QA/checked-macOS QA/checked-Win32 QA/checked-Win64 labels Feb 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Import recovery keys" button to Payments->Advanced->Recover #5809

Add "Import recovery keys" button to Payments->Advanced->Recover #5809

willy-b commented Nov 23, 2016 •

edited by luixxiul

Loading

willy-b commented Nov 23, 2016

bbondy commented Nov 28, 2016

willy-b commented Nov 28, 2016

bsclifton commented Dec 2, 2016

willy-b commented Dec 2, 2016

willy-b commented Dec 2, 2016 •

edited

Loading

bsclifton commented Dec 4, 2016 •

edited

Loading

willy-b commented Dec 14, 2016

willy-b commented Dec 15, 2016

posix4e commented Dec 21, 2016

bsclifton commented Dec 21, 2016 •

edited

Loading

posix4e commented Dec 21, 2016

posix4e commented Dec 21, 2016

willy-b commented Dec 21, 2016 •

edited

Loading

posix4e commented Dec 21, 2016

willy-b commented Dec 21, 2016 •

edited

Loading

willy-b commented Dec 21, 2016

bsclifton commented Dec 26, 2016 •

edited

Loading

willy-b commented Dec 28, 2016 •

edited

Loading

willy-b commented Dec 29, 2016 •

edited

Loading

willy-b commented Dec 30, 2016

luixxiul commented Jan 2, 2017

willy-b commented Jan 2, 2017 •

edited

Loading

willy-b commented Jan 9, 2017 •

edited

Loading

bsclifton left a comment

willy-b commented Jan 9, 2017

willy-b commented Jan 9, 2017

bsclifton commented Jan 9, 2017 •

edited

Loading

willy-b commented Jan 9, 2017

bsclifton commented Jan 10, 2017 •

edited

Loading

luixxiul commented Jan 11, 2017 •

edited

Loading

bsclifton commented Jan 12, 2017

luixxiul commented Jan 12, 2017

Add "Import recovery keys" button to Payments->Advanced->Recover #5809

Add "Import recovery keys" button to Payments->Advanced->Recover #5809

Conversation

willy-b commented Nov 23, 2016 • edited by luixxiul Loading

Test Plan

Automated test plan

willy-b commented Nov 23, 2016

bbondy commented Nov 28, 2016

willy-b commented Nov 28, 2016

bsclifton commented Dec 2, 2016

willy-b commented Dec 2, 2016

willy-b commented Dec 2, 2016 • edited Loading

bsclifton commented Dec 4, 2016 • edited Loading

willy-b commented Dec 14, 2016

willy-b commented Dec 15, 2016

posix4e commented Dec 21, 2016

bsclifton commented Dec 21, 2016 • edited Loading

posix4e commented Dec 21, 2016

posix4e commented Dec 21, 2016

willy-b commented Dec 21, 2016 • edited Loading

posix4e commented Dec 21, 2016

willy-b commented Dec 21, 2016 • edited Loading

willy-b commented Dec 21, 2016

bsclifton commented Dec 26, 2016 • edited Loading

willy-b commented Dec 28, 2016 • edited Loading

willy-b commented Dec 29, 2016 • edited Loading

willy-b commented Dec 30, 2016

luixxiul commented Jan 2, 2017

willy-b commented Jan 2, 2017 • edited Loading

willy-b commented Jan 9, 2017 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

willy-b commented Jan 9, 2017

willy-b commented Jan 9, 2017

bsclifton commented Jan 9, 2017 • edited Loading

willy-b commented Jan 9, 2017

bsclifton commented Jan 10, 2017 • edited Loading

luixxiul commented Jan 11, 2017 • edited Loading

bsclifton commented Jan 12, 2017

luixxiul commented Jan 12, 2017

willy-b commented Nov 23, 2016 •

edited by luixxiul

Loading

willy-b commented Dec 2, 2016 •

edited

Loading

bsclifton commented Dec 4, 2016 •

edited

Loading

bsclifton commented Dec 21, 2016 •

edited

Loading

willy-b commented Dec 21, 2016 •

edited

Loading

willy-b commented Dec 21, 2016 •

edited

Loading

bsclifton commented Dec 26, 2016 •

edited

Loading

willy-b commented Dec 28, 2016 •

edited

Loading

willy-b commented Dec 29, 2016 •

edited

Loading

willy-b commented Jan 2, 2017 •

edited

Loading

willy-b commented Jan 9, 2017 •

edited

Loading

bsclifton commented Jan 9, 2017 •

edited

Loading

bsclifton commented Jan 10, 2017 •

edited

Loading

luixxiul commented Jan 11, 2017 •

edited

Loading