Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixes disabled create wallet button #13867

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Apr 19, 2018

Resolves #13227

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • 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).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Sadly I couldn't break the wallet to get to this state. I fixed this based on the logic that is there. I need help with test plan 😃

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Resolves brave#13227

Auditors:

Test Plan:
@codecov-io
Copy link

codecov-io commented Apr 19, 2018

Codecov Report

Merging #13867 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #13867      +/-   ##
==========================================
- Coverage   56.54%   56.51%   -0.03%     
==========================================
  Files         283      283              
  Lines       28892    28895       +3     
  Branches     4788     4789       +1     
==========================================
- Hits        16336    16330       -6     
- Misses      12556    12565       +9
Flag Coverage Δ
#unittest 56.51% <100%> (-0.03%) ⬇️
Impacted Files Coverage Δ
...r/components/preferences/payment/enabledContent.js 67.55% <100%> (+0.37%) ⬆️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
js/stores/windowStore.js 28.46% <0%> (-0.3%) ⬇️

Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks sound, but I'm not able reproduce breaking my wallet to test

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too, am unable to break the wallet to reproduce and test.

However, could this PR introduce the possibility of triggering a wallet creation if one already exists? I'm concerned that without reproduction we only be able to confirm this is a visual fix. I'll keep trying to repro

@NejcZdovc
Copy link
Contributor Author

@ryanml I don't see how this could trigger creating new wallet, because we just enable button. But yeah it's hard to be sure where we can't break the wallet 😃

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code itself lgtm

@jasonrsadler
Copy link
Contributor

jasonrsadler commented Apr 20, 2018

This actually did happen to me about a month ago on my windows box where I already had wallet created, payments showed disabled, and clicking Enable Payments didn't do anything. No STR. Cleared out AppData parts and kept going. Didn't repeat after that.

@NejcZdovc NejcZdovc merged commit a970195 into brave:master Apr 23, 2018
NejcZdovc added a commit that referenced this pull request Apr 23, 2018
Fixes disabled create wallet button
NejcZdovc added a commit that referenced this pull request Apr 23, 2018
Fixes disabled create wallet button
@NejcZdovc
Copy link
Contributor Author

master a970195
0.23 7d27d4a
0.22 82f33de

@NejcZdovc NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After Browser restart, Payments Flag wont toggle
4 participants