Fixes add funds dialog #12154

NejcZdovc · 2017-12-01T07:54:28Z

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.

Resolves #12078

Auditors:

also added unit test for it

Test Plan:

steps based on comment: #12078 (comment)

Clean profile
start browser with staging server

LEDGER_ENVIRONMENT=staging npm run start

Enable payments
Visit some sites so that you have a few ledger entries showing on the Payments tab
Fund wallet (ex: claim UGP grant)
Close Brave
Update reconcileStamp in ledger-state.json to the epoch for now (or a time in the past). (This site is handy for conversions)
Launch Brave again using

LEDGER_NO_FUZZING=true LEDGER_VERBOSE=true LEDGER_ENVIRONMENT=staging npm run start

Wait for contribution to be made
"Needs deposit" notice should NOT show

Reviewer Checklist:

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

codecov-io · 2017-12-01T08:13:06Z

Codecov Report

Merging #12154 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #12154      +/-   ##
==========================================
- Coverage      55%   54.99%   -0.01%     
==========================================
  Files         275      275              
  Lines       26569    26563       -6     
  Branches     4273     4277       +4     
==========================================
- Hits        14613    14608       -5     
+ Misses      11956    11955       -1

Flag	Coverage Δ
#unittest	`54.99% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
app/browser/api/ledgerNotifications.js	`74.56% <100%> (+1.63%)`	⬆️
app/common/lib/ledgerUtil.js	`97.1% <100%> (+2.76%)`	⬆️
js/stores/appStoreRenderer.js	`91.17% <0%> (-8.83%)`	⬇️
app/renderer/components/reduxComponent.js	`84.37% <0%> (-6.25%)`	⬇️
js/stores/windowStore.js	`26.71% <0%> (-0.3%)`	⬇️

bsclifton · 2017-12-02T06:44:42Z

app/browser/api/ledgerNotifications.js

-  const bat = ledgerState.getInfoProp(state, 'bat')
-  return bat && (balance + unconfirmed > 0.9 * Number(bat))
+  const budget = parseInt(getSetting(settings.PAYMENTS_CONTRIBUTION_AMOUNT), 10) || 25
+  return balance + unconfirmed >= budget


The prior code that does getInfoProp(state, 'bat')... but I'm not seeing anything which sets this. I think the field name must have been updated from bat to converted at some point.

Why this is important: because ledgerUtil does something similar (in fact, maybe this code can be moved to ledgerUtil?):

browser-laptop/app/common/lib/ledgerUtil.js

Lines 99 to 101 in 5677468

if (pendingFunds + Number(ledgerData.get('balance') || 0) <

0.9 * Number(ledgerData.get('bat') || 0)) {

status.id = 'insufficientFundsStatus'

The code in ledgerUtil may have a problem too if nothing is setting this value

we need to fix this line as well, we don't have bat anymore

@NejcZdovc OK great- did you want to do that here also?

yeah will fix it in this PR

fixed, @bsclifton please re-review

bsclifton

comment left - tests all look good; just want to make sure similar code in ledgerUtil is handled the same

Resolves brave#12078 Auditors: Test Plan:

@NejcZdovc

…le testing) Manually tested (and updated test plan in PR). Works great! :) Auditors: @NejcZdovc

bsclifton

Thanks for updating the 2nd case! I just finished manually testing and it works great 😄 👍

Please note: I did bump the version of bat-client so that we can use the new LEDGER_NO_FUZZING parameter for testing.

Fixes add funds dialog

bsclifton · 2017-12-05T07:15:14Z

master 8c3ef5e
0.21.x 9a3ced4
0.20.x 893bfa4
0.19.x 7394478

srirambv · 2017-12-05T13:24:35Z

Works on Windows. No Waiting for a deposit notification shown.

NejcZdovc added the feature/rewards label Dec 1, 2017

NejcZdovc self-assigned this Dec 1, 2017

NejcZdovc requested a review from bsclifton December 1, 2017 07:54

NejcZdovc force-pushed the hotfix/#12078-funds branch 2 times, most recently from 02fd0e6 to 5d306aa Compare December 1, 2017 08:26

NejcZdovc added this to the 0.19.x Hotfix 7 (Release channel) milestone Dec 1, 2017

bsclifton changed the title ~~Fixes add founds dialog~~ Fixes add funds dialog Dec 1, 2017

bsclifton reviewed Dec 2, 2017

View reviewed changes

Alino mentioned this pull request Dec 2, 2017

add funds dialog shows up even if I have BAT already #12078

Closed

NejcZdovc force-pushed the hotfix/#12078-funds branch from 5d306aa to 10f4d8d Compare December 4, 2017 07:04

NejcZdovc requested a review from bsclifton December 4, 2017 07:07

NejcZdovc and others added 2 commits December 4, 2017 23:47

Fixes add founds dialog

0d3d087

Resolves brave#12078 Auditors: Test Plan:

Updated bat-client library with LEDGER_NO_FUZZING fix (to help enab…

c1ea5ba

…le testing) Manually tested (and updated test plan in PR). Works great! :) Auditors: @NejcZdovc

bsclifton force-pushed the hotfix/#12078-funds branch from 10f4d8d to c1ea5ba Compare December 5, 2017 06:48

bsclifton approved these changes Dec 5, 2017

View reviewed changes

bsclifton merged commit 8c3ef5e into brave:master Dec 5, 2017

bsclifton added a commit that referenced this pull request Dec 5, 2017

Merge pull request #12154 from NejcZdovc/hotfix/#12078-funds

9a3ced4

Fixes add funds dialog

bsclifton added a commit that referenced this pull request Dec 5, 2017

Merge pull request #12154 from NejcZdovc/hotfix/#12078-funds

893bfa4

Fixes add funds dialog

bsclifton added a commit that referenced this pull request Dec 5, 2017

Merge pull request #12154 from NejcZdovc/hotfix/#12078-funds

7394478

Fixes add funds dialog

srirambv mentioned this pull request Dec 5, 2017

Incorrect amount deducted from wallet for payment processing #12183

Closed

bsclifton modified the milestones: 0.19.x Hotfix 7 (Release channel), 0.19.x + C63 (Release Channel) Dec 7, 2017

bsclifton modified the milestones: 0.19.x + C63 (Release Channel), 0.19.x Private Search Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes add funds dialog #12154

Fixes add funds dialog #12154

NejcZdovc commented Dec 1, 2017 •

edited by bsclifton

Loading

codecov-io commented Dec 1, 2017 •

edited

Loading

bsclifton Dec 2, 2017

NejcZdovc Dec 2, 2017

bsclifton Dec 4, 2017

NejcZdovc Dec 4, 2017

NejcZdovc Dec 4, 2017

bsclifton left a comment

bsclifton left a comment

bsclifton commented Dec 5, 2017

srirambv commented Dec 5, 2017

	if (pendingFunds + Number(ledgerData.get('balance') \|\| 0) <
	0.9 * Number(ledgerData.get('bat') \|\| 0)) {
	status.id = 'insufficientFundsStatus'

Fixes add funds dialog #12154

Fixes add funds dialog #12154

Conversation

NejcZdovc commented Dec 1, 2017 • edited by bsclifton Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

codecov-io commented Dec 1, 2017 • edited Loading

Codecov Report

bsclifton Dec 2, 2017

Choose a reason for hiding this comment

NejcZdovc Dec 2, 2017

Choose a reason for hiding this comment

bsclifton Dec 4, 2017

Choose a reason for hiding this comment

NejcZdovc Dec 4, 2017

Choose a reason for hiding this comment

NejcZdovc Dec 4, 2017

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Dec 5, 2017

srirambv commented Dec 5, 2017

NejcZdovc commented Dec 1, 2017 •

edited by bsclifton

Loading

codecov-io commented Dec 1, 2017 •

edited

Loading