Ledger refactoring #11037

NejcZdovc · 2017-09-20T10:56:00Z

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 #3433 (consolidate state in app/ledger.js with appState)
Resolves #9641 (Ledger pin publishers 0 for unpinned sites)
Resolves #9740 (Second key for Brave Payments is not displayed on the UI after recovering wallet)
Resolves #9980 (Sites visited before payment enabling is shown as 0 views)
Resolves #10036 (Sites shows up with 0 views in payments)
Resolves #10716 (Publisher entry jumps position when enabled/disabled)
Resolves #10955 (Site added instantly to the ledger)
Resolves #11080 (site that i've never visited shows up in the synopsis)
Resolves #11173 (Ledger transfer PDF opens another window)
Resolves #11209 (Add state to the ledger)

Work progress:

move functions into ledgerApi file
- create util file
- add unit tests
create ledger reducer
- create reducer
- add unit tests
create site settings reducer
- create reducer
- add unit tests
move event store into appState
- move logic
- add unit test
move ledger state into appState
- move logic
- add unit test
remove static call of appState
- is done
pass state into ledger functions
- is done
create ledger state helper
- is done
make it work
- ledger table
- wallet recovery
- transaction view
create update script for session
- clear pageData when closing browser from the session
- crate upgrade path from ledger-synopsis.json to appState
- move locationInfo into ledger->locations

Auditors:

Test Plan:

Ledger Refactor Test Cases - MacOS Ledger Refactor Test Cases - MacOS #11214
Ledger Refactor Test Cases - Win x64 Ledger Refactor Test Cases - Win x64 #11217

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-09-20T16:32:56Z

Codecov Report

Merging #11037 into master will decrease coverage by 2.89%.
The diff coverage is 28.05%.

@@            Coverage Diff            @@
##           master   #11037     +/-   ##
=========================================
- Coverage   54.33%   51.43%   -2.9%     
=========================================
  Files         255      263      +8     
  Lines       22070    24541   +2471     
  Branches     3447     3928    +481     
=========================================
+ Hits        11992    12623    +631     
- Misses      10078    11918   +1840

Flag	Coverage Δ
#unittest	`51.43% <28.05%> (-2.9%)`	⬇️

Impacted Files	Coverage Δ
js/about/aboutActions.js	`9.32% <ø> (-1.97%)`	⬇️
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
js/constants/messages.js	`100% <ø> (ø)`	⬆️
app/browser/tabs.js	`29.07% <10%> (+0.2%)`	⬆️
app/browser/reducers/pageDataReducer.js	`100% <100%> (ø)`
js/about/preferences.js	`42.32% <100%> (ø)`	⬆️
...pp/renderer/components/navigation/navigationBar.js	`98.75% <100%> (+0.03%)`	⬆️
app/common/lib/pageDataUtil.js	`100% <100%> (ø)`
app/common/lib/publisherUtil.js	`100% <100%> (+2.77%)`	⬆️
...r/components/preferences/payment/enabledContent.js	`94.06% <100%> (ø)`	⬆️
... and 29 more

NejcZdovc · 2017-09-29T12:17:29Z

There are 3 tests failing, but as far as I can see they are not related to my changes, but tab handler can't be found. I see correct data on the screen while test is running, it just can't verify it at the end.

Failing tests

 4) Ledger table 2 publishers pin publisher:
     Error: Promise was rejected with the following reason: Cannot read property 'get' of undefined
      at elements("[data-tbody-index="0"] [data-row-index="0"] [data-test-id="siteName"]") - getText.js:18:17
      at getText("[data-tbody-index="0"] [data-row-index="0"] [data-test-id="siteName"]") - ledgerTableTest.js:104:23

  5) Ledger table 2 publishers pin excluded publisher:
     Error: element ("[data-tbody-index="1"] [data-row-index="0"] [data-switch-status="false"]") still not visible after 10000ms
      at elements("[data-tbody-index="1"] [data-row-index="0"] [data-switch-status="false"]") - isVisible.js:54:17
      at isVisible("[data-tbody-index="1"] [data-row-index="0"] [data-switch-status="false"]") - waitForVisible.js:73:22

  6) Ledger table 4 publishers pin 3 publishers and check unpinned value:
     Promise was rejected with the following reason: timeout
  Error

Error:

tabByIndex(0)
tabHandles()
tabHandles() => handles.length = 1; handles[0] = "CDwindow-240a244d-b002-4d96-a5d4-ae02559af1c8";

srirambv · 2017-10-02T12:47:21Z

~~Modal overlay is not correct~~

Its due to missing second key. Not an issue. My bad

NejcZdovc · 2017-10-02T12:52:37Z

@srirambv I pulled in @luixxiul PR so buttons should be ok. Key 2 fix was pushed to the ledger library and will be fixed with new version

Part of brave#11037 PR

srirambv · 2017-10-02T14:01:24Z

Issue 1:Separation bar has a bump over the include column

Issue 14 happened on clean profile with visit to github.com

Issue 2 : Manually changed % value is not retained
STR:

Visit 3-4 sites to get the list in ledger table
Pin all the sites in list
Manually change value to 3 for one of the pinned site, value is not retained but auto calculated

Issue 3: Verified publisher icon shown before loading the site.
STR:

Clean build PR
Enable payments
Visit any site, Verified publisher icon is shown momentarily before the unverified publisher icon is shown.

Issue 4: Verified publisher icon shown only after tab focus changes
STR:

Clean build from PR
Enable payments
Visit brianbondy.com, keep the tab active, Verified publisher icon is not shown
Switch focus to other tab and com back to brianbondy.com, shows verified publisher icon in URL

Issue 5: Publisher lilst not shown when min visit is changed to different value
STR:

Clean build from PR
Enable payments
Visit any site ensure its added to the list
Change min visit to 5 from 1
Visit a new site in a new tab, visit more than 5 times in a new tab each time, site is not added to list

Issue 6: Click on publisher icon does not toggle include/exclude
STR:

Clean build from PR
Enable payments
Visit any site ensure publisher is included, click on the icon doesn't exclude
Clear profile and create a new one
Enable payment and disable auto include publisher
Visit any site ensure its showing greyed out publisher icon, click on the icon to include, doesn't turn yellow to include the publisher.

Issue 7: Opening contribution statement shows about:blank
STR

Build from PR
Enable payments and close browser
Run npm run add-simulated-payment-history 50 to generate payment history
Open payment history and click on any history item
Opens in a new tab shows about:blank
Open payment history and right click on any entry and open in a new tab
Shows about:contribution#<ContributionID>

luixxiul · 2017-10-02T15:10:14Z

It should be fixed with https://github.com/brave/browser-laptop/pull/10789/files#diff-f14732b3ea914a26c2215676e7208907R347

bsclifton · 2017-10-02T22:13:37Z

app/renderer/components/navigation/publisherToggle.js

@@ -75,6 +75,7 @@ class PublisherToggle extends React.Component {
  }

  render () {
+    console.log(this.props)


should this be removed?

correct I already removed it, just didn't push it yet :)

Part of brave#11037 PR

Resolves brave#11009

@NejcZdovc

- siteSettingsReducerTest (should be 100% coverage now) - add more tests for ledgerUtil (btcToCurrencyString, etc) Auditors: @NejcZdovc

Part of brave#11037 PR

@NejcZdovc

Auditors: @NejcZdovc

Part of brave#11037 PR

Ledger refactoring

NejcZdovc · 2017-10-03T09:12:11Z

master de5f381
0.20 dddbcee
0.19 38c63b0

Part of brave#11037 PR

NejcZdovc added feature/rewards refactoring labels Sep 20, 2017

NejcZdovc added this to the 0.19.x (Beta Channel) milestone Sep 20, 2017

NejcZdovc self-assigned this Sep 20, 2017

NejcZdovc added the PR/work-in-progress ⚒ label Sep 20, 2017

NejcZdovc force-pushed the refactor/#11009-ledger branch 2 times, most recently from 449cc52 to 84b4913 Compare September 20, 2017 14:21

NejcZdovc force-pushed the refactor/#11009-ledger branch 2 times, most recently from 30004b6 to ef0318a Compare September 21, 2017 06:25

NejcZdovc mentioned this pull request Sep 21, 2017

replace momentjs with date-fns #11062

Merged

NejcZdovc force-pushed the refactor/#11009-ledger branch from d1dcd81 to 2cb1122 Compare September 22, 2017 09:10

This was referenced Sep 23, 2017

Sites shows up with 0 views in payments #10036

Closed

Payments calculations gone wrong #10534

Closed

NejcZdovc force-pushed the refactor/#11009-ledger branch 4 times, most recently from 0988c37 to 0b02da5 Compare September 26, 2017 08:25

bsclifton force-pushed the refactor/#11009-ledger branch from d1b2f7d to 23f8260 Compare September 26, 2017 18:40

NejcZdovc force-pushed the refactor/#11009-ledger branch 2 times, most recently from 8a38c5a to aff082f Compare September 26, 2017 18:51

This was referenced Sep 26, 2017

Verified publisher icon not shown on manual upgrade #10814

Closed

Escape key not closing add funds dialog #3800

Closed

NejcZdovc force-pushed the refactor/#11009-ledger branch 2 times, most recently from 9765c47 to 296e85e Compare September 29, 2017 08:47

NejcZdovc force-pushed the refactor/#11009-ledger branch from 296e85e to 291900b Compare September 29, 2017 12:32

NejcZdovc requested review from mrose17, bbondy and bsclifton September 29, 2017 12:34

NejcZdovc force-pushed the refactor/#11009-ledger branch from d72b629 to 233dae1 Compare October 2, 2017 10:03

NejcZdovc force-pushed the refactor/#11009-ledger branch 2 times, most recently from facc8fc to ad111f2 Compare October 2, 2017 12:57

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Oct 2, 2017

Moving ledger into appState part 2

3064556

Part of brave#11037 PR

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Oct 2, 2017

Addresses review comments

ad111f2

Part of brave#11037 PR

bsclifton reviewed Oct 2, 2017

View reviewed changes

NejcZdovc mentioned this pull request Oct 3, 2017

Opening contribution statement shows about:blank #11243

Closed

NejcZdovc force-pushed the refactor/#11009-ledger branch 2 times, most recently from c340c89 to 19ec356 Compare October 3, 2017 07:13

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Oct 3, 2017

Addresses review comments

7042450

Part of brave#11037 PR

NejcZdovc and others added 4 commits October 3, 2017 09:13

Moves eventStore into appStore

7079a5b

Resolves brave#11009

Add / update unit tests

2ec3ea8

- siteSettingsReducerTest (should be 100% coverage now) - add more tests for ledgerUtil (btcToCurrencyString, etc) Auditors: @NejcZdovc

Moving ledger into appState part 2

d5bbac2

Part of brave#11037 PR

Adding unit tests for ledgerReducer

63804e8

Auditors: @NejcZdovc

NejcZdovc force-pushed the refactor/#11009-ledger branch from 19ec356 to 2c3c647 Compare October 3, 2017 07:14

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Oct 3, 2017

Addresses review comments

8d5e0a9

Part of brave#11037 PR

NejcZdovc added 2 commits October 3, 2017 09:49

Addresses review comments

22bb84e

Part of brave#11037 PR

Temporary fix for startup problem

a4deb1b

NejcZdovc force-pushed the refactor/#11009-ledger branch from 2c3c647 to a4deb1b Compare October 3, 2017 07:50

NejcZdovc merged commit de5f381 into brave:master Oct 3, 2017

NejcZdovc added a commit that referenced this pull request Oct 3, 2017

Merge pull request #11037 from NejcZdovc/refactor/#11009-ledger

dddbcee

Ledger refactoring

NejcZdovc added a commit that referenced this pull request Oct 3, 2017

Merge pull request #11037 from NejcZdovc/refactor/#11009-ledger

38c63b0

Ledger refactoring

kjozwiak mentioned this pull request Oct 3, 2017

Min page time and min visit selections are not being saved in Advanced Settings #11260

Closed

syuan100 pushed a commit to syuan100/browser-laptop that referenced this pull request Nov 9, 2017

Moving ledger into appState part 2

17f4dfe

Part of brave#11037 PR

syuan100 pushed a commit to syuan100/browser-laptop that referenced this pull request Nov 9, 2017

Addresses review comments

130246b

Part of brave#11037 PR

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ledger refactoring #11037

Ledger refactoring #11037

NejcZdovc commented Sep 20, 2017 •

edited

Loading

codecov-io commented Sep 20, 2017 •

edited

Loading

NejcZdovc commented Sep 29, 2017 •

edited

Loading

srirambv commented Oct 2, 2017 •

edited

Loading

NejcZdovc commented Oct 2, 2017 •

edited

Loading

srirambv commented Oct 2, 2017 •

edited by NejcZdovc

Loading

luixxiul commented Oct 2, 2017

bsclifton Oct 2, 2017

NejcZdovc Oct 3, 2017

NejcZdovc commented Oct 3, 2017

Ledger refactoring #11037

Ledger refactoring #11037

Conversation

NejcZdovc commented Sep 20, 2017 • edited Loading

codecov-io commented Sep 20, 2017 • edited Loading

Codecov Report

NejcZdovc commented Sep 29, 2017 • edited Loading

srirambv commented Oct 2, 2017 • edited Loading

NejcZdovc commented Oct 2, 2017 • edited Loading

srirambv commented Oct 2, 2017 • edited by NejcZdovc Loading

luixxiul commented Oct 2, 2017

bsclifton Oct 2, 2017

Choose a reason for hiding this comment

NejcZdovc Oct 3, 2017

Choose a reason for hiding this comment

NejcZdovc commented Oct 3, 2017

NejcZdovc commented Sep 20, 2017 •

edited

Loading

codecov-io commented Sep 20, 2017 •

edited

Loading

NejcZdovc commented Sep 29, 2017 •

edited

Loading

srirambv commented Oct 2, 2017 •

edited

Loading

NejcZdovc commented Oct 2, 2017 •

edited

Loading

srirambv commented Oct 2, 2017 •

edited by NejcZdovc

Loading