Split PaymentsTab into its own component #6529

jkup · 2017-01-05T01:30:27Z

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).

This ticket both splits PaymentsTab out into its own component and adds automated tests for its functionality.

@bsclifton I'm not thrilled with my fakeSetting API, can you help me make it a little more scalable? Maybe in another ticket? Eventually I assume we'll need more than just true for all or false for all which is all mine can handle!

Auditors @bsclifton @bbondy @cezaraugusto please let me know what you think!

Test Plan:
npm run unittest -- --grep="Preferences component"

Ready for people to take a look at this! You can change line 15 on paymentsTabTest.js to be describe.only and then with watch-all running you can just run npm run unittest.

#6456 #6455 #6422

bsclifton · 2017-01-05T09:06:54Z

I tried running this locally (macOS) via npm run unittest and ran into the following:

1) Preferences component loads "before all" hook:
     Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined.
      at invariant (node_modules/fbjs/lib/invariant.js:44:15)
      at ReactCompositeComponentWrapper.instantiateReactComponent [as _instantiateReactComponent] (node_modules/react-dom/lib/instantiateReactComponent.js:68:134)
      at ReactCompositeComponentWrapper.performInitialMount (node_modules/react-dom/lib/ReactCompositeComponent.js:367:22)
      at ReactCompositeComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:258:21)
      at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:46:35)
      at ReactCompositeComponentWrapper.performInitialMount (node_modules/react-dom/lib/ReactCompositeComponent.js:371:34)
      at ReactCompositeComponentWrapper.mountComponent (node_modules/react-dom/lib/ReactCompositeComponent.js:258:21)
      at Object.mountComponent (node_modules/react-dom/lib/ReactReconciler.js:46:35)
      at mountComponentIntoNode (node_modules/react-dom/lib/ReactMount.js:104:32)
      at ReactReconcileTransaction.perform (node_modules/react-dom/lib/Transaction.js:140:20)
      at batchedMountComponentIntoNode (node_modules/react-dom/lib/ReactMount.js:126:15)
      at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-dom/lib/Transaction.js:140:20)
      at Object.batchedUpdates (node_modules/react-dom/lib/ReactDefaultBatchingStrategy.js:62:26)
      at Object.batchedUpdates (node_modules/react-dom/lib/ReactUpdates.js:97:27)
      at Object._renderNewRootComponent (node_modules/react-dom/lib/ReactMount.js:320:18)
      at Object._renderSubtreeIntoContainer (node_modules/react-dom/lib/ReactMount.js:401:32)
      at Object.render (node_modules/react-dom/lib/ReactMount.js:422:23)
      at Object.renderIntoDocument (node_modules/react-dom/lib/ReactTestUtils.js:79:21)
      at renderWithOptions (node_modules/enzyme/build/react-compat.js:187:26)
      at new ReactWrapper (node_modules/enzyme/build/ReactWrapper.js:94:59)
      at mount (node_modules/enzyme/build/mount.js:19:10)
      at Context.<anonymous> (test/unit/about/preferencesTest.js:40:21)

When I scrolled up to where the test was executed, you can see the additional errors logged via stdout:

Preferences component
    loads
Warning: Failed prop type: The prop `Component` is marked as required in `<<anonymous>>`, but its value is `undefined`.
    in Unknown
Warning: Failed prop type: The prop `props` is marked as required in `<<anonymous>>`, but its value is `undefined`.
    in Unknown
Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components).
      1) "before all" hook

Is there something I missed?

jkup · 2017-01-05T19:01:42Z

@bsclifton sorry! was messing around with them late last night and made a breaking change. Should be good now although two of the (unrelated) preferencesTest.js tests are broken in master. I'll follow up on those separately with @bbondy.

cezaraugusto

@jkup AFAIU this PR only cover tests for payments, correct? Would the split of payments tab component be a follow-up, then?

Current tests LGTM ++

bsclifton

Tested it out, everything works as expected (I updated the original post to show a way to only run these ones). Great job! 😄

bsclifton · 2017-01-06T05:22:23Z

@cezaraugusto this PR is also breaking the payments tab out from preferences.js into it's own file

bsclifton · 2017-01-06T06:39:42Z

If this looks good to you (either @cezaraugusto or @bbondy), please approve using GitHub and then mark with the ready-to-merge label 😄

cezaraugusto · 2017-01-06T16:46:35Z

ok sorry, GitHub grouped paymentsTab.js which I missed.

LGTM

luixxiul · 2017-01-08T05:53:47Z

Please make sure that there are several PRs which will be possibly conflicted if this is merged first.
See: https://github.com/brave/browser-laptop/pulls?utf8=%E2%9C%93&q=is%3Apr%20is%3Aopen%20label%3Afeature%2Fledger%20-label%3Atests%20-label%3APR%2Fwork-in-progress%20

Could I remove the ready-for-merge label for now, so that this PR will not be merged first? thanks

bsclifton · 2017-01-25T23:09:59Z

Rebasing this now... should be a great cleanup 😄

bsclifton · 2017-01-26T00:38:05Z

Gonna watch CI on this one; should be good to merge though! 😄

bsclifton · 2017-01-26T18:08:12Z

Reran failing tests locally; a few unrelated tests seem to be perma-failing ☹️

I believe it's safe to merge this since the ledger tests are passing and the failing tests are not impacted by these code changes

@bbondy

…committed) (no more currency symbols, payment history button now always shows (but will have text saying when next contribution is) Auditors: @bbondy, @mrose17

Preferences tests (redo of #6529)

bsclifton · 2017-01-30T00:34:24Z

re-assigned 0.13.1 milestone after merge was redone properly

jkup added automated-tests QA/no-qa-needed tests labels Jan 5, 2017

jkup added this to the 0.13.1 milestone Jan 5, 2017

jkup requested a review from bsclifton January 5, 2017 01:30

jkup removed the QA/no-qa-needed label Jan 5, 2017

jkup force-pushed the preferences-tests branch from 55fc7e2 to ca440fc Compare January 5, 2017 01:32

jkup changed the title ~~Preferences tests~~ Split PaymentsTab into its own component Jan 5, 2017

willy-b mentioned this pull request Jan 5, 2017

Add "Import recovery keys" button to Payments->Advanced->Recover #5809

Merged

4 tasks

jkup force-pushed the preferences-tests branch from ca440fc to b220900 Compare January 5, 2017 02:48

luixxiul added PR/needs-QA-attention ☕ feature/rewards labels Jan 5, 2017

bsclifton requested review from bbondy and cezaraugusto January 5, 2017 08:51

bsclifton mentioned this pull request Jan 5, 2017

Add Auto-suggest sites option, to prefs and urlBar #6422

Closed

jkup force-pushed the preferences-tests branch 2 times, most recently from f746c78 to a7e6e75 Compare January 5, 2017 18:59

cezaraugusto reviewed Jan 6, 2017

View reviewed changes

bsclifton approved these changes Jan 6, 2017

View reviewed changes

cezaraugusto approved these changes Jan 6, 2017

View reviewed changes

cezaraugusto added PR/ready-for-merge and removed PR/needs-QA-attention ☕ labels Jan 6, 2017

luixxiul added the release-notes/exclude label Jan 6, 2017

luixxiul removed the PR/ready-for-merge label Jan 8, 2017

bsclifton force-pushed the 0.13.1-branch branch 6 times, most recently from 65c7753 to 238cfd3 Compare January 25, 2017 19:19

bsclifton force-pushed the preferences-tests branch from a7e6e75 to 7aac987 Compare January 26, 2017 00:37

luixxiul removed the PR/needs-rebase ۞ label Jan 26, 2017

bsclifton changed the base branch from 0.13.1-branch to master January 26, 2017 18:08

jkup and others added 2 commits January 26, 2017 11:18

Split PaymentsTab into its own component

744e10c

Updated tests (lots of things have changed since this was originally …

522f0e3

…committed) (no more currency symbols, payment history button now always shows (but will have text saying when next contribution is) Auditors: @bbondy, @mrose17

bsclifton force-pushed the preferences-tests branch from 7aac987 to 522f0e3 Compare January 26, 2017 18:22

bsclifton merged commit fb5b958 into master Jan 26, 2017

bsclifton deleted the preferences-tests branch January 26, 2017 18:22

bsclifton restored the preferences-tests branch January 26, 2017 19:06

bsclifton mentioned this pull request Jan 26, 2017

Preferences tests (redo of https://github.com/brave/browser-laptop/pull/6529) #6876

Merged

bsclifton added a commit that referenced this pull request Jan 26, 2017

Merge pull request #6876 from brave/preferences-tests

ad2cd84

Preferences tests (redo of #6529)

luixxiul added the reverted label Jan 27, 2017

luixxiul removed this from the 0.13.1 milestone Jan 27, 2017

luixxiul mentioned this pull request Jan 27, 2017

automated test failure: payment tab #6891

Closed

1 task

bsclifton added this to the 0.13.1 milestone Jan 30, 2017

bsclifton removed the reverted label Jan 30, 2017

This was referenced Feb 3, 2017

Manual Tests for Windows x64 0.13.2 RC2 #7030

Closed

Manual Tests for Windows ia-32 0.13.2 RC3 #7029

Closed

Manual Tests for Linux 0.13.2 RC3 #7028

Closed

Manual Tests for OS X 0.13.2 RC2 #7027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split PaymentsTab into its own component #6529

Split PaymentsTab into its own component #6529

jkup commented Jan 5, 2017 •

edited by luixxiul

Loading

bsclifton commented Jan 5, 2017 •

edited

Loading

jkup commented Jan 5, 2017

cezaraugusto left a comment

bsclifton left a comment

bsclifton commented Jan 6, 2017

bsclifton commented Jan 6, 2017

cezaraugusto commented Jan 6, 2017

luixxiul commented Jan 8, 2017 •

edited

Loading

bsclifton commented Jan 25, 2017

bsclifton commented Jan 26, 2017

bsclifton commented Jan 26, 2017

bsclifton commented Jan 30, 2017

Split PaymentsTab into its own component #6529

Split PaymentsTab into its own component #6529

Conversation

jkup commented Jan 5, 2017 • edited by luixxiul Loading

bsclifton commented Jan 5, 2017 • edited Loading

jkup commented Jan 5, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Jan 6, 2017

bsclifton commented Jan 6, 2017

cezaraugusto commented Jan 6, 2017

luixxiul commented Jan 8, 2017 • edited Loading

bsclifton commented Jan 25, 2017

bsclifton commented Jan 26, 2017

bsclifton commented Jan 26, 2017

bsclifton commented Jan 30, 2017

jkup commented Jan 5, 2017 •

edited by luixxiul

Loading

bsclifton commented Jan 5, 2017 •

edited

Loading

luixxiul commented Jan 8, 2017 •

edited

Loading