WIP - Add Payment History dialog to Payments Tab (#2994) #3391

willy-b · 2016-08-24T23:59:20Z

Used Github auto-closing keywords in the commit message.

This pull request adds a Payment History dialog to the Payments Tab.
It is a work in progress: The Receipt Link column links do not work yet (working on generating the receipt PDFs now). But the rest of the content in the Payment History dialog should be correct.

Detailed discussions and @bradleyrichter's original mockup can be found in issue #2994 (see mockup at #2994 (comment)).

- We now store the last BTC price with the ledgerInfo object in `ledger.js`, which becomes the `ledgerData` object in UI - Why? The BTC price was being computed in a couple of places from the current account balance (balance $ / balance USD). Recomputation from derived quantities can lead to error, this is simpler.

- Displays a table of user payment history (Each row represents a batched payout to multiple publishers sometimes called a `reconciliation` or `transaction` elsewhere) - 3 columns are shown for each payment: `Date`, `Total Amount`, & `Receipt Link` - Data comes from `transactions` array in `ledgerData` obj in UI (same as `ledgerInfo` in `browser-laptop/app/ledger.js`) - Next payment date in footer comes from `ledgerData.reconcileStamp` Loose ends / unfinished items: - `Receipt Link` is a dummy filename for now: PDF receipts are NOT impl yet - `Total Amount` USD value fluctuates because the stored value is in BTC and current exchange rate is used. This will be fixed in followup commit - The "Show Payment History" button is in an awkward spot impls issue #2994

- restyle "View Payment History" button per updated spec Simple orange text in Account Balance box #2994 (comment)

diracdeltas · 2016-08-25T17:22:50Z

app/ledger.js

-        if ((body.rates) && (body.rates[currency])) info.btc = (amount / body.rates[currency]).toFixed(8)
+        if ((body.rates) && (body.rates[currency])) {
+          info.btcPrice = body.rates[currency]
+          info.btc = (amount / body.rates[currency]).toFixed(8)


need @mrose17 to review this part

i guess this is obsolete now.

agreed. no longer needed.

diracdeltas · 2016-08-25T17:44:39Z

I don't think we will add this to 0.11.6 @bbondy but looking good so far. Thanks @willy-b

willy-b · 2016-08-25T20:03:30Z

Thanks for the code review @diracdeltas! I'll get those points addressed this afternoon.

fixes #3391 (comment)

mrose17 · 2016-08-25T21:01:05Z

@willy-b what's your thinking in terms of a timeframe?

willy-b · 2016-08-25T21:36:30Z

@mrose17, I will finish addressing the points @diracdeltas raised in her review by ~~3:30PM~~. (EDIT: merging ledger-client update from master dragged this out, but all done now)

Then I aim to post a proposal/breakdown for the Receipt PDF based on @bradleyrichter's gorgeous new Receipt PDF mockups (reference: #2994 (comment)) by ~~4:30PM~~. (EDIT: going to do in a separate PR and do CSV here to keep PRs small and avoid merge/rebase hell :-) )

For one I am going to suggest we omit absolute time figures for pages and only include contributions actually sent. This is the data available in the ledgerData.transactions array and also what I would argue is important to display (the probability of contributing to a site is less important after the fact than what was contributed).

We might want to do simple CSV receipts in this PR and implement @bradleyrichter's really nice mockups in a separate PR depending on schedule.

diracdeltas · 2016-08-25T21:37:56Z

(the probability of contributing to a site is less important after the fact than what was contributed

agree, and i think simple CSV receipts are fine for now

diracdeltas · 2016-08-25T21:44:38Z

app/ledger.js

@@ -695,6 +695,7 @@ var ledgerInfo = {

  // the desired contribution (the btc value approximates the amount/currency designation)
  btc: undefined,
+  btcPrice: undefined,


@willy-b not sure if this is obvious to you, but the BTC price should now be available as a property of transactions according to @mrose17. specifically ledgerInfo.transactions[i].contribution.rates is the BTC price in the local currency at the time of the transaction (i think).

Yes, @mrose17's updates in ledger-client v0.8.46 brought in by #3397 are really nice.
They save me from having to add a historical btcPrice to the transactions entries, and obsolete the btcPrice I added :-)

…ransactions fixes #3391 (comment)

… is undefined/null fixes #3391 (comment)

diracdeltas · 2016-08-25T23:02:51Z

js/about/preferences.js

+  }
+
+  get satoshis () {
+    return this.transaction.get('satoshis')


@mrose17 is this correct, or should it be this.transaction.get('contribution').get('satoshis') now?

Sorry @diracdeltas, hold on just a minute, I haven't pushed the merge of @mrose17's update which includes this.
When you see the merge commit with latest master then you can come in full guns blazing :-).

@diracdeltas, while @mrose17 updated the docs/state.md in 5040058 , he didn't update app/ledger.js to whitelist the contribution field to be picked from ledger-client transaction result. Thus after #3397 the contribution field still wasn't making it to UI.
I am updating the comments and code in ledger.js right now. Let me know if this is available in a different commit I can cherry-pick, because enabling transactions[n].contribution probably shouldn't be part of this PR.

will defer to @mrose17 on that

oops! i'll fix it in master momentarily... sorry!

use this.transaction.get('contribution').get('satoshis')

willy-b · 2016-08-25T23:59:04Z

Update: I merged latest master to get @mrose17's updates in ledger-client v0.8.46 brought in by #3397, specifically the awesome new transactions[n].contribution field.
But now I'm seeing some issues (ledger-client seems to be running over and over in a loop, and I'm trying to trigger a reconcile ahead of schedule to get real data with new format) so I'm running a bit behind schedule. (Only pushed 3 of the changes requested by @diracdeltas so far.)

mrose17 · 2016-08-26T00:02:42Z

@willy-b - sorry about that. do me a favor and to your ledger-state.json file, towards the top look for

  "options": {
    "verboseP": false,

and change false to true and then let me know what it's doing... thanks!

willy-b · 2016-08-26T00:11:37Z

@mrose17 no worries at all, thanks for the help.

It just appears to be logging the same line ledger client run over and over again:

ledger client run: clientP=true delayTime=0
ledger client run: clientP=true delayTime=0
ledger client run: clientP=true delayTime=0
...

- text should not wrap/overflow when window is narrow #3391 (comment) - "View Payment History" should be left-aligned now, since #3396

for #2994

diracdeltas · 2016-08-26T21:16:36Z

js/about/preferences.js

+  }
+
+  get numericDateStr () {
+    var date = new Date(this.timestamp)


why not just return date.toLocaleDateString() here?

So I did do that for the displayed date: 48b0171#diff-e3eeb751016b2ce9f8278efce585a461R1258

This is for a filename, so toLocaleDateString is a little awkward with the slashes and lack of zero-padding. But sure, I can use toLocaleDateString and then replace the '/' with '-'s and let the lack of zero-padding be.

gotcha, i missed the fact that this is for a filename. sorry

fix https://github.com/brave/browser-laptop/pull/3391/files/fe5ae395854edddfa79a61026ec3b16c35fe795b#r76489409 for #2994

bradleyrichter · 2016-08-26T22:54:11Z

@willy-b Heads up for you. We are discussing a change that would swap the percent column for dollars (or local currency) per site. We should have this ironed out soon.

willy-b · 2016-08-26T22:59:36Z

Thanks for heads up @bradleyrichter!
That's no problem. If I have the %, I can just multiply the % by the total in local currency. We do have the local currency value at historical exchange thanks to @mrose17's recent ledger-client update.

My semi-blocker right now is that the ballots field seems to have disappeared from the transactions entry -- that was what I used to get the %.

diracdeltas · 2016-08-26T23:15:10Z

js/about/preferences.js

+  }
+
+  get satoshis () {
+    return this.transaction.get('contribution').get('satoshis')


contribution and other child properties may not necessarily exist, so this would throw cannot read property 'get' of undefined errors. please use this.transaction.getIn(['contribution', ...]).

diracdeltas · 2016-08-26T23:38:54Z

@willy-b does the semi-blocker relating to the disappearance of ballots only affect the transaction CSV/PDF files? I am fine with merging this for now once the comments above are addressed, and the CSV/PDF part can be done in a follow-up PR.

Thanks for the work; please run git rebase -i to squash commits when you're done.

@diracdeltas

Fixes for some items from @diracdeltas code review (thanks!) - use .getIn instead of .get for nested props https://github.com/brave/browser-laptop/pull/3391/files/1eded36027dbe9bf21d1fdfeb8fdcec8a3ff9a61#r76501448 - use template strings https://github.com/brave/browser-laptop/pull/3391/files/1eded36027dbe9bf21d1fdfeb8fdcec8a3ff9a61#r76501619 - return null, not undefined in React render calls https://github.com/brave/browser-laptop/pull/3391/files/1eded36027dbe9bf21d1fdfeb8fdcec8a3ff9a61#r76501793 - check that typeof is Number before using toFixed https://github.com/brave/browser-laptop/pull/3391/files/1eded36027dbe9bf21d1fdfeb8fdcec8a3ff9a61#r76502209

willy-b · 2016-08-27T00:30:51Z

Thanks for all the testing and code reviewing @diracdeltas.

Yes, the ballots issue only affects the transaction-level CSV/PDF. If everyone is OK with it, I think it's a great idea to move the 'Receipt Link' to a separate PR.

bradleyrichter · 2016-08-27T00:48:25Z

@willy-b False alarm on the change to currency. We are staying with percentages for now.

- fix overflow in Payment History table https://github.com/brave/browser-laptop/pull/3391/files/1eded36027dbe9bf21d1fdfeb8fdcec8a3ff9a61#r76502448 - fix awkward alignment of `Account Balance`, `Monthly Budget` columns, and `View Payment History` button in Payments Tab https://github.com/brave/browser-laptop/pull/3391/files/bd84215c68d40093da63ed071347fb60617bb5f2#r76502032 - change "Add Funds" button text to "Add Funds..." and decrease font size https://github.com/brave/browser-laptop/pull/3391/files/bd84215c68d40093da63ed071347fb60617bb5f2#r76502449 - restore Buy With Coinbase modal broken by my ModalOverlay changes https://github.com/brave/browser-laptop/pull/3391/files/1eded36027dbe9bf21d1fdfeb8fdcec8a3ff9a61#r76502568

bbondy · 2016-08-27T05:31:29Z

Is this ready to merge? I won't include it in beta5 yet because I'm not sure.

mrose17 · 2016-08-27T05:32:22Z

@willy-b - what's your thinking on @bbondy's question?

willy-b · 2016-08-27T05:44:14Z

Hey! If we are OK with doing the 'Receipt Link' in a separate PR then I
just need to disable that column and squash the commits for this to undergo
a final review for merging.
On Aug 26, 2016 10:32 PM, "Marshall T. Rose" notifications@github.com
wrote:

@willy-b https://github.com/willy-b - what's your thinking on @bbondy
https://github.com/bbondy's question?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3391 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGpaA-_nvMZsQjzogvuCTwD4frW74QBSks5qj8vqgaJpZM4Jskfu
.

mrose17 · 2016-08-27T05:45:17Z

@willy-b - i like that plan... please proceed

cc: @bbondy

diracdeltas · 2016-08-27T06:41:40Z

@willy-b agree; that sounds like a good plan. you can just hide the receipt link column for now.

willy-b · 2016-08-27T07:04:12Z

Sorry for the delay, coming right up!

On Fri, Aug 26, 2016 at 11:41 PM, yan zhu (@bcrypt) <
notifications@github.com> wrote:

@willy-b https://github.com/willy-b agree; that sounds like a good
plan. you can just hide the receipt link column for now.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3391 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGpaA0gMsGvtxGxEXcl_F2jeOsegjwyVks5qj9wogaJpZM4Jskfu
.

willy-b · 2016-08-27T09:02:59Z

The 'Receipt Link' has been disabled and this has been rebased down to two commits ready for review in PR #3473.

Thanks again @diracdeltas and @mrose17 for the code reviews / testing, and for helping me come up to speed on your project as I worked on this :-).

Willy Bruns added 5 commits August 24, 2016 10:13

Remove unused methods from PaymentHistory component

08ec7ba

Restyle "View Payment History" button

8ca3f4a

- restyle "View Payment History" button per updated spec Simple orange text in Account Balance box #2994 (comment)

merge latest from master

bd84215

bbondy assigned diracdeltas Aug 25, 2016

diracdeltas reviewed Aug 25, 2016
View reviewed changes

Replace "View Payment History" literal with l10nId "viewPaymentHistory"

8593ef1

fixes #3391 (comment)

diracdeltas reviewed Aug 25, 2016
View reviewed changes

Willy Bruns added 3 commits August 25, 2016 14:56

"View Payment History" Button should not display if no wallet or no t…

6c8274d

…ransactions fixes #3391 (comment)

Payment History footer should be empty if ledgerData.reconcileStamp…

b30a5e0

… is undefined/null fixes #3391 (comment)

merging latest master

a33b901

diracdeltas reviewed Aug 25, 2016
View reviewed changes

Willy Bruns added 2 commits August 25, 2016 21:12

Fix "View Payment History" styling

fb828b5

- text should not wrap/overflow when window is narrow #3391 (comment) - "View Payment History" should be left-aligned now, since #3396

Add OK button to Payment History modal per spec

fe5ae39

for #2994

diracdeltas reviewed Aug 26, 2016
View reviewed changes

Payment History dialog uses `date.toLocaleDateString for PDF filename

1eded36

fix https://github.com/brave/browser-laptop/pull/3391/files/fe5ae395854edddfa79a61026ec3b16c35fe795b#r76489409 for #2994

diracdeltas reviewed Aug 26, 2016
View reviewed changes

willy-b mentioned this pull request Aug 27, 2016

Add Payment History dialog to Payments Tab (issue #2994) #3473

Merged

1 task

willy-b closed this Aug 27, 2016

alexwykoff added the QA/checked-macOS label Aug 30, 2016

luixxiul added the feature/rewards label Sep 28, 2016

WIP - Add Payment History dialog to Payments Tab (#2994) #3391

WIP - Add Payment History dialog to Payments Tab (#2994) #3391

Conversation

willy-b commented Aug 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Aug 25, 2016

willy-b commented Aug 25, 2016

mrose17 commented Aug 25, 2016

willy-b commented Aug 25, 2016 • edited Loading

diracdeltas commented Aug 25, 2016

Choose a reason for hiding this comment

willy-b Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willy-b Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willy-b commented Aug 25, 2016

mrose17 commented Aug 26, 2016

willy-b commented Aug 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradleyrichter commented Aug 26, 2016

willy-b commented Aug 26, 2016

Choose a reason for hiding this comment

diracdeltas commented Aug 26, 2016

willy-b commented Aug 27, 2016

bradleyrichter commented Aug 27, 2016

bbondy commented Aug 27, 2016

mrose17 commented Aug 27, 2016

willy-b commented Aug 27, 2016

mrose17 commented Aug 27, 2016

diracdeltas commented Aug 27, 2016

willy-b commented Aug 27, 2016

willy-b commented Aug 27, 2016

willy-b commented Aug 25, 2016 •

edited

Loading

willy-b Aug 25, 2016 •

edited

Loading

willy-b Aug 25, 2016 •

edited

Loading