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

Adds balance breakdown #14251

Merged
merged 1 commit into from
May 28, 2018
Merged

Adds balance breakdown #14251

merged 1 commit into from
May 28, 2018

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented May 25, 2018 •
edited
Loading

Resolves #14229

Empty wallet
image

With user fund
image

With grant
image

With grant + user fund
image

Contribution in progress
image

Contribution done
image

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:

  • clean profile
  • enable wallet
  • add funds
  • claim promotion
  • do a contribution
    Make sure that in all this steps you are getting correct amounts listed in the account balance section

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

@NejcZdovc NejcZdovc self-assigned this May 25, 2018
@NejcZdovc NejcZdovc force-pushed the feature/#14229-breakdown branch 3 times, most recently from 6a80158 to 784e4b8 Compare May 25, 2018 14:14
@NejcZdovc NejcZdovc requested review from bsclifton, ryanml and jasonrsadler and removed request for bsclifton May 25, 2018 16:28
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented May 25, 2018 •
edited
Loading

To the reviewers: Please check only last commit, because this PR is pending on captcha PR. Will rebase as soon as captcha PR is merged

Rebased

@NejcZdovc NejcZdovc force-pushed the feature/#14229-breakdown branch 3 times, most recently from 451eb83 to b3d7dad Compare May 25, 2018 16:39
@NejcZdovc NejcZdovc force-pushed the feature/#14229-breakdown branch from b3d7dad to 0a08085 Compare May 25, 2018 16:54
@NejcZdovc NejcZdovc force-pushed the feature/#14229-breakdown branch from 0a08085 to 80d48ad Compare May 25, 2018 17:07
@jasonrsadler
Copy link
Contributor

Need 'clean profile' on test plan. (nit)

Do we have a way to test when a second promotion is received (before first expires)?

@NejcZdovc
Copy link
Contributor Author

@jasonrsadler sadly we only have one active promotion

ryanml
ryanml reviewed May 25, 2018
View reviewed changes
app/browser/api/ledger.js
@@ -1882,15 +1882,33 @@ const onWalletProperties = (state, body) => {
state = ledgerState.setInfoProp(state, 'currentRate', rate)
}

// Grants
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this block perhaps be extracted to a separate function and exported? It shouldn't change any of the existing tests you've put in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if needed

ryanml
ryanml reviewed May 25, 2018
View reviewed changes
app/renderer/components/preferences/payment/enabledContent.js
fundsAmount__item: {
marginBottom: '4px',
width: '215px',
fontSize: '14.5px'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this isn't just 15px? it looked fine with it for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use 14.5px in other places by default, so that's why I set it here as well

ryanml
ryanml reviewed May 25, 2018
View reviewed changes
app/renderer/components/preferences/payment/enabledContent.js
grants.map(grant => {
return <div className={css(styles.fundsAmount__item)}>
{formatCurrentBalance(ledgerData, grant.get('amount'), false)}
<span> (<span data-l10n-id='expires' /> {new Date(grant.get('expirationDate') * 1000).toLocaleDateString()})</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) could we maybe separate new Date(grant.get('expirationDate') * 1000).toLocaleDateString() in a to a formatting function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if needed

@jasonrsadler
Copy link
Contributor

When a contribution is made, do we have determination as to what funds are used first? (i.e. Is UGP used first or user funds)

@NejcZdovc
Copy link
Contributor Author

@jasonrsadler UGP funds should be used first

@NejcZdovc NejcZdovc requested a review from ryanml May 28, 2018 05:14
@NejcZdovc
Adds new balance breakdown
a0d55ef 
Resolves brave#14229

Auditors:

Test Plan:
@NejcZdovc NejcZdovc force-pushed the feature/#14229-breakdown branch from 80d48ad to a0d55ef Compare May 28, 2018 05:32
ryanml
ryanml approved these changes May 28, 2018
View reviewed changes
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.

One bug addressed over slack, test plan works well. Awesome work 👍

@NejcZdovc NejcZdovc merged commit 9713445 into brave:master May 28, 2018
NejcZdovc added a commit that referenced this pull request May 28, 2018
@NejcZdovc
Merge pull request #14251 from NejcZdovc/feature/#14229-breakdown
d037f07 
Adds balance breakdown
NejcZdovc added a commit that referenced this pull request May 28, 2018
@NejcZdovc
Merge pull request #14251 from NejcZdovc/feature/#14229-breakdown
50b39e0 
Adds balance breakdown
@NejcZdovc
Copy link
Contributor Author

master 9713445
0.23 d037f07
0.22 50b39e0

@mandar-brave mandar-brave mentioned this pull request May 29, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanml ryanml ryanml approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

@jasonrsadler jasonrsadler Awaiting requested review from jasonrsadler

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
0.22.x Release 5 (Release channel)
Development

Successfully merging this pull request may close these issues.
3 participants
@NejcZdovc @jasonrsadler @ryanml