Refactor of paymentTab #7481

NejcZdovc · 2017-03-03T17:21:38Z

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

Resolves #7501

Auditors

@bsclifton @cezaraugusto

Test plan

Everything should be the same as was before the change

NejcZdovc · 2017-03-04T23:18:35Z

Open questions:

~~do we need to translate this?~~
~~should I add aphrodite for ModalOverlay in this PR or create a separate PR. I changed a lot of things in this PR already~~
~~how to fix this mock problem with png images~~

Preferences component "before all" hook:
     SyntaxError: app/extensions/brave/img/private_internet_access.png: Unexpected character '�' (1:0)

And I will squash after reviews are done, so that it's easier to track changes when reviewing.

cc @bsclifton @cezaraugusto

NejcZdovc · 2017-03-05T12:59:11Z

Question number 3 was solved with @bsclifton help. We only have two more 😃

cezaraugusto · 2017-03-07T21:57:06Z

About #7481 (comment):

Yes, not necessarily on this PR if you don't want but as a follow-up definitely
I think this PR addressed several components and we can move that for another PR

NejcZdovc · 2017-03-08T11:13:19Z

@cezaraugusto I agree. I added translations and we should leave ModalOverlay for another PR. All questions answered.

luixxiul · 2017-03-08T17:03:37Z

@NejcZdovc https://github.com/brave/browser-laptop/pull/7481/files/47e03f86912b9990546346794fbe39ffa8e144c6..b9cdefc48cbee03f0449319587b173ea065b7539 resolves #6364 as well. Thanks.

cezaraugusto · 2017-03-11T00:33:38Z

needs rebase.

@NejcZdovc how doable it is to split each refactored/moved component in its own PR? This way we can speed-up code review and reduce regression risks.

cezaraugusto · 2017-03-11T00:34:43Z

If you do please open an issue as tracking reference, similar to #7380

NejcZdovc · 2017-03-11T06:49:48Z

If it's ok with you I would rebase it after I squash it, otherwise merge mistake is quite possible, because I created 14 commits. I presume that there will be more conflicts, because it will take some time to review it and new changes will land in the master. If you don't need multiple commits for easier review, I can squash it right now.

I think it would take too time consuming to split it now. I started really slow and small, but things are so connected (to do big changes) that I end up with one big PR. Sorry about that.

cezaraugusto · 2017-03-11T17:14:49Z

ok np, just fix conflict/squash and we can start reviewing

NejcZdovc · 2017-03-11T18:44:21Z

@cezaraugusto conflict resolved, you can start with the review

cezaraugusto · 2017-03-11T23:31:02Z

app/renderer/components/preferences/payment/disabledContent.js

+
+// style
+const globalStyles = require('../../styles/global')
+const paymentStyles = require('../../styles/payment')


cezaraugusto · 2017-03-11T23:35:18Z

app/renderer/components/preferences/payment/advanceSettings.js

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+


I think there's a typo on file name: advanceSettings -> advancedSettings

cezaraugusto · 2017-03-11T23:41:49Z

app/extensions/brave/locales/en-US/preferences.properties

+minimumPageTimeHigh=1 minute
+minimumVisitsLow=1 visit
+minimumVisitsMedium=5 visits
+minimumVisitsHigh=10 visits


thanks for this

bsclifton

Feedback left, great job 😄 This has some really nice cleanup which makes both maintaining the code and testing easier

Ping me once you've addressed the feedback by Cezar and I and we can squash + merge 😄

bsclifton · 2017-03-12T07:16:54Z

app/common/lib/ledgerUtil.js

@@ -39,3 +40,41 @@ module.exports.shouldTrackView = (view, responseList) => {
  }
  return false
 }
+
+module.exports.btcToCurrencyString = (btc, ledgerData) => {


Awesome! thanks for pulling this out- will make testing easy

bsclifton · 2017-03-12T07:17:36Z

app/renderer/components/preferences/payment/advancedSettings.js

+    const minDuration = this.props.ledgerData.getIn(['synopsisOptions', 'minDuration'])
+    const minPublisherVisits = this.props.ledgerData.getIn(['synopsisOptions', 'minPublisherVisits'])
+
+    // TODO translate seconds, minute and visits


You can remove this TODO since you put it into the preferences.properties 😄

bsclifton · 2017-03-12T07:35:58Z

app/renderer/components/preferences/payment/enabledContent.js

+    } else if (ledgerData.get('created')) {
+      const transactions = ledgerData.get('transactions')
+      const pendingFunds = Number(ledgerData.get('unconfirmed') || 0)
+


This entire method (walletStatus) could benefit from moving over to ledgerUtil 😄 (it would need to be reworked to take ledgerData as input). That would also make mocking it easier

bsclifton · 2017-03-12T07:38:56Z

less/about/preferences.less

-            }
-          }
-        }
-
        .bitcoinDashboard {


Love seeing all this LESS being deleted 😄

bsclifton · 2017-03-12T07:39:27Z

less/about/preferences.less

  }
 }

+// TODO check what to remove


Was there more that needed to be removed? or did you need to test anything which had been removed so far?

Just a left over

bsclifton · 2017-03-12T07:39:45Z

test/unit/about/preferencesTest.js

+    mockery.registerMock('../../../../extensions/brave/img/coinbase_2x.png')
+    mockery.registerMock('../../../../extensions/brave/img/coinbase_logo.png')
+    mockery.registerMock('../../../../extensions/brave/img/android_download.svg')
+    mockery.registerMock('../../../../extensions/brave/img/ios_download.svg')


cezaraugusto

Awesome, thanks for all the hard-work. Bold - and very welcome - changes.

I left some comments and the only blocker is the crash of secondary key on ledger recovery.

cezaraugusto · 2017-03-11T23:43:44Z

app/renderer/components/preferences/payment/advanceSettings.js

@@ -0,0 +1,148 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


I think there's a typo on file name -> advanceSettings/advancedSettings. This is replicated although the changes

@bsclifton fixed this

cezaraugusto · 2017-03-11T23:55:22Z

app/renderer/components/preferences/payment/ledgerBackup.js

+          />
+          <div className={css(styles.keyContainer)}>
+            <h3 className={css(styles.keyContainer__h3)} data-l10n-id='secondKey' />
+            <span className={css(styles.keyContainer__span)}>{passphrase}</span>


app crashes if I try to copy key 2

Hmmm, it's not crashing for me. Can you please provide some reproduction steps?

STR:

Clear profile

Enable payments

Go to advanced settings

Click backup your wallet

Key 2 doesn't exist

Try to copy key 2

I tried this steps, but key 2 exists for me and copy is working as well. So the problem is not in a copy but in fact that key 2 doesn't exists

As discussed on a Slack, this is specific to one ledger-state.json file and I can't reproduce it with my ledger or on fresh profile, so I think this is not related to this PR

cezaraugusto · 2017-03-12T00:03:57Z

app/renderer/components/preferences/payment/history.js

+    }
+
+    return <div className={css(styles.historyFooter)}>
+      <div className={css(styles.nextPayement)}>


cezaraugusto · 2017-03-12T00:10:49Z

app/renderer/components/settings.js

    return <div {...props}>
      <SwitchControl id={this.props.prefKey}
        small={this.props.small}
        disabled={this.props.disabled}
        onClick={this.onClick}
-        checkedOn={this.props.checked !== undefined ? this.props.checked : getSetting(this.props.prefKey, this.props.settings)} />
-      <label className={css(this.props.small && settingCheckboxStyles.label)} data-l10n-id={this.props.dataL10nId} htmlFor={this.props.prefKey} />
+        checkedOn={this.props.checked !== undefined ? this.props.checked : getSetting(this.props.prefKey, this.props.settings)}


(minor) can you stringify undefined to 'undefined'?

Why would we want to compare it as a string?

I meant to typeof this.props.checked !== 'undefined', but doesn't matter result is the same, let's keep as-is, sorry

cezaraugusto · 2017-03-12T09:42:44Z

js/about/preferences.js

@@ -85,224 +83,6 @@ const braveryPermissionNames = {
  'noScript': ['boolean', 'number']
 }

-class BitcoinDashboard extends ImmutableComponent {


If you didn't yet, please reference that PR also closes #7380

f375451 it's already in

cezaraugusto · 2017-03-12T09:52:40Z

less/about/preferences.less

-
-    .nextReconcileDate {
-      margin-bottom: 0;
-    }


YESSSS

The lesser the LESS, the less™

@bsclifton

Resolves brave#7501 brave#7380 brave#6364 Auditors: @bsclifton @cezaraugusto Test plan: - everything should work the same as was before the chage

NejcZdovc · 2017-03-12T17:45:56Z

@cezaraugusto @bsclifton Fixed everything that was found in the review process. PR is also squashed now. Can you please do a final review?

cezaraugusto

++ great work, thanks

bsclifton · 2017-03-12T23:10:50Z

++ great work, @NejcZdovc! Thanks for taking the time to review in detail too, @cezaraugusto 😄

NejcZdovc force-pushed the feature/#7348-refactor branch from 6bc57e5 to 1f9d193 Compare March 3, 2017 17:26

NejcZdovc added the PR/work-in-progress ⚒ label Mar 3, 2017

NejcZdovc self-assigned this Mar 4, 2017

NejcZdovc force-pushed the feature/#7348-refactor branch 5 times, most recently from d0d903e to cd94b65 Compare March 4, 2017 22:59

NejcZdovc added this to the 0.13.6 milestone Mar 4, 2017

NejcZdovc force-pushed the feature/#7348-refactor branch 3 times, most recently from 6a2d73e to b4ae95e Compare March 5, 2017 12:50

NejcZdovc added PR/needs-QA-attention ☕ refactoring refactoring/aphrodite and removed PR/work-in-progress ⚒ labels Mar 5, 2017

NejcZdovc force-pushed the feature/#7348-refactor branch 2 times, most recently from 417a8c7 to d8ab9d5 Compare March 6, 2017 08:41

NejcZdovc requested review from cezaraugusto and bsclifton March 6, 2017 08:42

cezaraugusto mentioned this pull request Mar 7, 2017

Fixed style of some ledger elements #7380

Closed

NejcZdovc force-pushed the feature/#7348-refactor branch 2 times, most recently from 3e8cfe1 to 47e03f8 Compare March 8, 2017 10:16

NejcZdovc mentioned this pull request Mar 8, 2017

Added default download path #7023

Merged

4 tasks

NejcZdovc mentioned this pull request Mar 8, 2017

Payment UI 1.0 #7532

Merged

4 tasks

NejcZdovc force-pushed the feature/#7348-refactor branch from b9cdefc to 81a36a2 Compare March 8, 2017 19:10

This was referenced Mar 9, 2017

Payments features 1.0 #7642

Merged

Fix Advanced Ledger Panel tests (#6911) #7401

Closed

luixxiul added the PR/needs-rebase ۞ label Mar 11, 2017

NejcZdovc force-pushed the feature/#7348-refactor branch from 81a36a2 to 90f2314 Compare March 11, 2017 18:44

NejcZdovc removed the PR/needs-rebase ۞ label Mar 11, 2017

cezaraugusto reviewed Mar 11, 2017

View reviewed changes

bsclifton reviewed Mar 12, 2017

View reviewed changes

cezaraugusto suggested changes Mar 12, 2017

View reviewed changes

Refactor of paymentTab

405efe5

Resolves brave#7501 brave#7380 brave#6364 Auditors: @bsclifton @cezaraugusto Test plan: - everything should work the same as was before the chage

NejcZdovc force-pushed the feature/#7348-refactor branch from e0d5db1 to 405efe5 Compare March 12, 2017 17:45

cezaraugusto approved these changes Mar 12, 2017

View reviewed changes

cezaraugusto merged commit 0bfceeb into brave:master Mar 12, 2017

NejcZdovc removed the PR/needs-QA-attention ☕ label Mar 12, 2017

NejcZdovc mentioned this pull request Mar 13, 2017

Fixed style and layout on payment tab #7501

Closed

luixxiul mentioned this pull request Mar 13, 2017

Fixed options on about:preferences#payments to allow for localization #6364

Closed

		@@ -0,0 +1,148 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Refactor of paymentTab #7481

Refactor of paymentTab #7481

Conversation

NejcZdovc commented Mar 3, 2017 • edited Loading

Auditors

Test plan

NejcZdovc commented Mar 4, 2017 • edited Loading

NejcZdovc commented Mar 5, 2017

cezaraugusto commented Mar 7, 2017

NejcZdovc commented Mar 8, 2017 • edited Loading

luixxiul commented Mar 8, 2017 • edited Loading

cezaraugusto commented Mar 11, 2017

cezaraugusto commented Mar 11, 2017

NejcZdovc commented Mar 11, 2017 • edited Loading

cezaraugusto commented Mar 11, 2017

NejcZdovc commented Mar 11, 2017 • 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

Choose a reason for hiding this comment

bsclifton left a 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

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

Choose a reason for hiding this comment

cezaraugusto left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Mar 12, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

bsclifton commented Mar 12, 2017

NejcZdovc commented Mar 3, 2017 •

edited

Loading

NejcZdovc commented Mar 4, 2017 •

edited

Loading

NejcZdovc commented Mar 8, 2017 •

edited

Loading

luixxiul commented Mar 8, 2017 •

edited

Loading

NejcZdovc commented Mar 11, 2017 •

edited

Loading

NejcZdovc commented Mar 11, 2017 •

edited

Loading