Merge pull request #4694 from MetaMask/i4660-improve-send-token-error-ui

Improve send token error ux.
This commit is contained in:
Dan J Miller 2018-07-05 16:25:59 -02:30 committed by GitHub
commit d3920007fa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
18 changed files with 320 additions and 17 deletions

View File

@ -21,6 +21,7 @@ export default class SendAmountRow extends Component {
selectedToken: PropTypes.object,
setMaxModeTo: PropTypes.func,
tokenBalance: PropTypes.string,
updateGasFeeError: PropTypes.func,
updateSendAmount: PropTypes.func,
updateSendAmountError: PropTypes.func,
updateGas: PropTypes.func,
@ -35,6 +36,7 @@ export default class SendAmountRow extends Component {
primaryCurrency,
selectedToken,
tokenBalance,
updateGasFeeError,
updateSendAmountError,
} = this.props
@ -48,6 +50,19 @@ export default class SendAmountRow extends Component {
selectedToken,
tokenBalance,
})
if (selectedToken) {
updateGasFeeError({
amount,
amountConversionRate,
balance,
conversionRate,
gasTotal,
primaryCurrency,
selectedToken,
tokenBalance,
})
}
}
updateAmount (amount) {

View File

@ -13,7 +13,7 @@ import {
import {
sendAmountIsInError,
} from './send-amount-row.selectors'
import { getAmountErrorObject } from '../../send.utils'
import { getAmountErrorObject, getGasFeeErrorObject } from '../../send.utils'
import {
setMaxModeTo,
updateSendAmount,
@ -44,6 +44,9 @@ function mapDispatchToProps (dispatch) {
return {
setMaxModeTo: bool => dispatch(setMaxModeTo(bool)),
updateSendAmount: newAmount => dispatch(updateSendAmount(newAmount)),
updateGasFeeError: (amountDataObject) => {
dispatch(updateSendErrors(getGasFeeErrorObject(amountDataObject)))
},
updateSendAmountError: (amountDataObject) => {
dispatch(updateSendErrors(getAmountErrorObject(amountDataObject)))
},

View File

@ -13,6 +13,7 @@ const propsMethodSpies = {
updateSendAmount: sinon.spy(),
updateSendAmountError: sinon.spy(),
updateGas: sinon.spy(),
updateGasFeeError: sinon.spy(),
}
sinon.spy(SendAmountRow.prototype, 'updateAmount')
@ -36,6 +37,7 @@ describe('SendAmountRow Component', function () {
selectedToken={ { address: 'mockTokenAddress' } }
setMaxModeTo={propsMethodSpies.setMaxModeTo}
tokenBalance={'mockTokenBalance'}
updateGasFeeError={propsMethodSpies.updateGasFeeError}
updateSendAmount={propsMethodSpies.updateSendAmount}
updateSendAmountError={propsMethodSpies.updateSendAmountError}
updateGas={propsMethodSpies.updateGas}
@ -47,6 +49,7 @@ describe('SendAmountRow Component', function () {
propsMethodSpies.setMaxModeTo.resetHistory()
propsMethodSpies.updateSendAmount.resetHistory()
propsMethodSpies.updateSendAmountError.resetHistory()
propsMethodSpies.updateGasFeeError.resetHistory()
SendAmountRow.prototype.validateAmount.resetHistory()
SendAmountRow.prototype.updateAmount.resetHistory()
})
@ -72,6 +75,32 @@ describe('SendAmountRow Component', function () {
)
})
it('should call updateGasFeeError if selectedToken is truthy', () => {
assert.equal(propsMethodSpies.updateGasFeeError.callCount, 0)
instance.validateAmount('someAmount')
assert.equal(propsMethodSpies.updateGasFeeError.callCount, 1)
assert.deepEqual(
propsMethodSpies.updateGasFeeError.getCall(0).args,
[{
amount: 'someAmount',
amountConversionRate: 'mockAmountConversionRate',
balance: 'mockBalance',
conversionRate: 7,
gasTotal: 'mockGasTotal',
primaryCurrency: 'mockPrimaryCurrency',
selectedToken: { address: 'mockTokenAddress' },
tokenBalance: 'mockTokenBalance',
}]
)
})
it('should call not updateGasFeeError if selectedToken is falsey', () => {
wrapper.setProps({ selectedToken: null })
assert.equal(propsMethodSpies.updateGasFeeError.callCount, 0)
instance.validateAmount('someAmount')
assert.equal(propsMethodSpies.updateGasFeeError.callCount, 0)
})
})
describe('updateAmount', () => {

View File

@ -33,7 +33,10 @@ proxyquire('../send-amount-row.container.js', {
getTokenBalance: (s) => `mockTokenBalance:${s}`,
},
'./send-amount-row.selectors': { sendAmountIsInError: (s) => `mockInError:${s}` },
'../../send.utils': { getAmountErrorObject: (mockDataObject) => ({ ...mockDataObject, mockChange: true }) },
'../../send.utils': {
getAmountErrorObject: (mockDataObject) => ({ ...mockDataObject, mockChange: true }),
getGasFeeErrorObject: (mockDataObject) => ({ ...mockDataObject, mockGasFeeErrorChange: true }),
},
'../../../../actions': actionSpies,
'../../../../ducks/send.duck': duckActionSpies,
})
@ -66,6 +69,7 @@ describe('send-amount-row container', () => {
beforeEach(() => {
dispatchSpy = sinon.spy()
mapDispatchToPropsObject = mapDispatchToProps(dispatchSpy)
duckActionSpies.updateSendErrors.resetHistory()
})
describe('setMaxModeTo()', () => {
@ -92,6 +96,18 @@ describe('send-amount-row container', () => {
})
})
describe('updateGasFeeError()', () => {
it('should dispatch an action', () => {
mapDispatchToPropsObject.updateGasFeeError({ some: 'data' })
assert(dispatchSpy.calledOnce)
assert(duckActionSpies.updateSendErrors.calledOnce)
assert.deepEqual(
duckActionSpies.updateSendErrors.getCall(0).args[0],
{ some: 'data', mockGasFeeErrorChange: true }
)
})
})
describe('updateSendAmountError()', () => {
it('should dispatch an action', () => {
mapDispatchToPropsObject.updateSendAmountError({ some: 'data' })

View File

@ -8,6 +8,7 @@ export default class SendGasRow extends Component {
static propTypes = {
conversionRate: PropTypes.number,
convertedCurrency: PropTypes.string,
gasFeeError: PropTypes.bool,
gasLoadingError: PropTypes.bool,
gasTotal: PropTypes.string,
showCustomizeGasModal: PropTypes.func,
@ -19,11 +20,16 @@ export default class SendGasRow extends Component {
convertedCurrency,
gasLoadingError,
gasTotal,
gasFeeError,
showCustomizeGasModal,
} = this.props
return (
<SendRowWrapper label={`${this.context.t('gasFee')}:`}>
<SendRowWrapper
label={`${this.context.t('gasFee')}:`}
showError={gasFeeError}
errorType={'gasFee'}
>
<GasFeeDisplay
conversionRate={conversionRate}
convertedCurrency={convertedCurrency}

View File

@ -4,7 +4,7 @@ import {
getCurrentCurrency,
getGasTotal,
} from '../../send.selectors.js'
import { sendGasIsInError } from './send-gas-row.selectors.js'
import { getGasLoadingError, gasFeeIsInError } from './send-gas-row.selectors.js'
import { showModal } from '../../../../actions'
import SendGasRow from './send-gas-row.component'
@ -15,7 +15,8 @@ function mapStateToProps (state) {
conversionRate: getConversionRate(state),
convertedCurrency: getCurrentCurrency(state),
gasTotal: getGasTotal(state),
gasLoadingError: sendGasIsInError(state),
gasFeeError: gasFeeIsInError(state),
gasLoadingError: getGasLoadingError(state),
}
}

View File

@ -1,9 +1,14 @@
const selectors = {
sendGasIsInError,
gasFeeIsInError,
getGasLoadingError,
}
module.exports = selectors
function sendGasIsInError (state) {
function getGasLoadingError (state) {
return state.send.errors.gasLoading
}
function gasFeeIsInError (state) {
return Boolean(state.send.errors.gasFee)
}

View File

@ -18,6 +18,7 @@ describe('SendGasRow Component', function () {
wrapper = shallow(<SendGasRow
conversionRate={20}
convertedCurrency={'mockConvertedCurrency'}
gasFeeError={'mockGasFeeError'}
gasLoadingError={false}
gasTotal={'mockGasTotal'}
showCustomizeGasModal={propsMethodSpies.showCustomizeGasModal}
@ -36,9 +37,13 @@ describe('SendGasRow Component', function () {
it('should pass the correct props to SendRowWrapper', () => {
const {
label,
showError,
errorType,
} = wrapper.find(SendRowWrapper).props()
assert.equal(label, 'gasFee_t:')
assert.equal(showError, 'mockGasFeeError')
assert.equal(errorType, 'gasFee')
})
it('should render a GasFeeDisplay as a child of the SendRowWrapper', () => {

View File

@ -22,7 +22,10 @@ proxyquire('../send-gas-row.container.js', {
getCurrentCurrency: (s) => `mockConvertedCurrency:${s}`,
getGasTotal: (s) => `mockGasTotal:${s}`,
},
'./send-gas-row.selectors.js': { sendGasIsInError: (s) => `mockGasLoadingError:${s}` },
'./send-gas-row.selectors.js': {
getGasLoadingError: (s) => `mockGasLoadingError:${s}`,
gasFeeIsInError: (s) => `mockGasFeeError:${s}`,
},
'../../../../actions': actionSpies,
})
@ -35,6 +38,7 @@ describe('send-gas-row container', () => {
conversionRate: 'mockConversionRate:mockState',
convertedCurrency: 'mockConvertedCurrency:mockState',
gasTotal: 'mockGasTotal:mockState',
gasFeeError: 'mockGasFeeError:mockState',
gasLoadingError: 'mockGasLoadingError:mockState',
})
})

View File

@ -1,11 +1,12 @@
import assert from 'assert'
import {
sendGasIsInError,
gasFeeIsInError,
getGasLoadingError,
} from '../send-gas-row.selectors.js'
describe('send-gas-row selectors', () => {
describe('sendGasIsInError()', () => {
describe('getGasLoadingError()', () => {
it('should return send.errors.gasLoading', () => {
const state = {
send: {
@ -15,7 +16,33 @@ describe('send-gas-row selectors', () => {
},
}
assert.equal(sendGasIsInError(state), 'abc')
assert.equal(getGasLoadingError(state), 'abc')
})
})
describe('gasFeeIsInError()', () => {
it('should return true if send.errors.gasFee is truthy', () => {
const state = {
send: {
errors: {
gasFee: 'def',
},
},
}
assert.equal(gasFeeIsInError(state), true)
})
it('should return false send.errors.gasFee is falsely', () => {
const state = {
send: {
errors: {
gasFee: null,
},
},
}
assert.equal(gasFeeIsInError(state), false)
})
})

View File

@ -3,6 +3,7 @@ import PropTypes from 'prop-types'
import PersistentForm from '../../../lib/persistent-form'
import {
getAmountErrorObject,
getGasFeeErrorObject,
getToAddressForGasUpdate,
doesAmountErrorRequireUpdate,
} from './send.utils'
@ -112,7 +113,19 @@ export default class SendTransactionScreen extends PersistentForm {
selectedToken,
tokenBalance,
})
updateSendErrors(amountErrorObject)
const gasFeeErrorObject = selectedToken
? getGasFeeErrorObject({
amount,
amountConversionRate,
balance,
conversionRate,
gasTotal,
primaryCurrency,
selectedToken,
tokenBalance,
})
: { gasFee: null }
updateSendErrors(Object.assign(amountErrorObject, gasFeeErrorObject))
}
if (!uninitialized) {
@ -143,6 +156,10 @@ export default class SendTransactionScreen extends PersistentForm {
this.updateGas()
}
componentWillUnmount () {
this.props.resetSendState()
}
render () {
const { history } = this.props

View File

@ -28,6 +28,7 @@ import {
setGasTotal,
} from '../../actions'
import {
resetSendState,
updateSendErrors,
} from '../../ducks/send.duck'
import {
@ -87,5 +88,6 @@ function mapDispatchToProps (dispatch) {
}))
},
updateSendErrors: newError => dispatch(updateSendErrors(newError)),
resetSendState: () => dispatch(resetSendState()),
}
}

View File

@ -30,6 +30,7 @@ module.exports = {
estimateGasPriceFromRecentBlocks,
generateTokenTransferData,
getAmountErrorObject,
getGasFeeErrorObject,
getToAddressForGasUpdate,
isBalanceSufficient,
isTokenBalanceSufficient,
@ -110,9 +111,9 @@ function getAmountErrorObject ({
tokenBalance,
}) {
let insufficientFunds = false
if (gasTotal && conversionRate) {
if (gasTotal && conversionRate && !selectedToken) {
insufficientFunds = !isBalanceSufficient({
amount: selectedToken ? '0x0' : amount,
amount,
amountConversionRate,
balance,
conversionRate,
@ -149,6 +150,34 @@ function getAmountErrorObject ({
return { amount: amountError }
}
function getGasFeeErrorObject ({
amount,
amountConversionRate,
balance,
conversionRate,
gasTotal,
primaryCurrency,
}) {
let gasFeeError = null
if (gasTotal && conversionRate) {
const insufficientFunds = !isBalanceSufficient({
amount: '0x0',
amountConversionRate,
balance,
conversionRate,
gasTotal,
primaryCurrency,
})
if (insufficientFunds) {
gasFeeError = INSUFFICIENT_FUNDS_ERROR
}
}
return { gasFee: gasFeeError }
}
function calcTokenBalance ({ selectedToken, usersToken }) {
const { decimals } = selectedToken || {}
return calcTokenAmount(usersToken.balance.toString(), decimals) + ''

View File

@ -12,9 +12,11 @@ const propsMethodSpies = {
updateAndSetGasTotal: sinon.spy(),
updateSendErrors: sinon.spy(),
updateSendTokenBalance: sinon.spy(),
resetSendState: sinon.spy(),
}
const utilsMethodStubs = {
getAmountErrorObject: sinon.stub().returns({ amount: 'mockAmountError' }),
getGasFeeErrorObject: sinon.stub().returns({ gasFee: 'mockGasFeeError' }),
doesAmountErrorRequireUpdate: sinon.stub().callsFake(obj => obj.balance !== obj.prevBalance),
}
@ -50,6 +52,7 @@ describe('Send Component', function () {
updateAndSetGasTotal={propsMethodSpies.updateAndSetGasTotal}
updateSendErrors={propsMethodSpies.updateSendErrors}
updateSendTokenBalance={propsMethodSpies.updateSendTokenBalance}
resetSendState={propsMethodSpies.resetSendState}
/>)
})
@ -58,6 +61,7 @@ describe('Send Component', function () {
SendTransactionScreen.prototype.updateGas.resetHistory()
utilsMethodStubs.doesAmountErrorRequireUpdate.resetHistory()
utilsMethodStubs.getAmountErrorObject.resetHistory()
utilsMethodStubs.getGasFeeErrorObject.resetHistory()
propsMethodSpies.updateAndSetGasTotal.resetHistory()
propsMethodSpies.updateSendErrors.resetHistory()
propsMethodSpies.updateSendTokenBalance.resetHistory()
@ -77,6 +81,15 @@ describe('Send Component', function () {
})
})
describe('componentWillUnmount', () => {
it('should call this.props.resetSendState', () => {
propsMethodSpies.resetSendState.resetHistory()
assert.equal(propsMethodSpies.resetSendState.callCount, 0)
wrapper.instance().componentWillUnmount()
assert.equal(propsMethodSpies.resetSendState.callCount, 1)
})
})
describe('componentDidUpdate', () => {
it('should call doesAmountErrorRequireUpdate with the expected params', () => {
utilsMethodStubs.getAmountErrorObject.resetHistory()
@ -133,8 +146,51 @@ describe('Send Component', function () {
)
})
it('should call updateSendErrors with the expected params', () => {
it('should call getGasFeeErrorObject if doesAmountErrorRequireUpdate returns true and selectedToken is truthy', () => {
utilsMethodStubs.getGasFeeErrorObject.resetHistory()
wrapper.instance().componentDidUpdate({
from: {
balance: 'balanceChanged',
},
})
assert.equal(utilsMethodStubs.getGasFeeErrorObject.callCount, 1)
assert.deepEqual(
utilsMethodStubs.getGasFeeErrorObject.getCall(0).args[0],
{
amount: 'mockAmount',
amountConversionRate: 'mockAmountConversionRate',
balance: 'mockBalance',
conversionRate: 10,
gasTotal: 'mockGasTotal',
primaryCurrency: 'mockPrimaryCurrency',
selectedToken: 'mockSelectedToken',
tokenBalance: 'mockTokenBalance',
}
)
})
it('should not call getGasFeeErrorObject if doesAmountErrorRequireUpdate returns false', () => {
utilsMethodStubs.getGasFeeErrorObject.resetHistory()
wrapper.instance().componentDidUpdate({
from: { address: 'mockAddress', balance: 'mockBalance' },
})
assert.equal(utilsMethodStubs.getGasFeeErrorObject.callCount, 0)
})
it('should not call getGasFeeErrorObject if doesAmountErrorRequireUpdate returns true but selectedToken is falsy', () => {
utilsMethodStubs.getGasFeeErrorObject.resetHistory()
wrapper.setProps({ selectedToken: null })
wrapper.instance().componentDidUpdate({
from: {
balance: 'balanceChanged',
},
})
assert.equal(utilsMethodStubs.getGasFeeErrorObject.callCount, 0)
})
it('should call updateSendErrors with the expected params if selectedToken is falsy', () => {
propsMethodSpies.updateSendErrors.resetHistory()
wrapper.setProps({ selectedToken: null })
wrapper.instance().componentDidUpdate({
from: {
balance: 'balanceChanged',
@ -143,7 +199,22 @@ describe('Send Component', function () {
assert.equal(propsMethodSpies.updateSendErrors.callCount, 1)
assert.deepEqual(
propsMethodSpies.updateSendErrors.getCall(0).args[0],
{ amount: 'mockAmountError'}
{ amount: 'mockAmountError', gasFee: null }
)
})
it('should call updateSendErrors with the expected params if selectedToken is truthy', () => {
propsMethodSpies.updateSendErrors.resetHistory()
wrapper.setProps({ selectedToken: 'someToken' })
wrapper.instance().componentDidUpdate({
from: {
balance: 'balanceChanged',
},
})
assert.equal(propsMethodSpies.updateSendErrors.callCount, 1)
assert.deepEqual(
propsMethodSpies.updateSendErrors.getCall(0).args[0],
{ amount: 'mockAmountError', gasFee: 'mockGasFeeError' }
)
})

View File

@ -12,6 +12,7 @@ const actionSpies = {
}
const duckActionSpies = {
updateSendErrors: sinon.spy(),
resetSendState: sinon.spy(),
}
proxyquire('../send.container.js', {
@ -152,6 +153,17 @@ describe('send container', () => {
})
})
describe('resetSendState()', () => {
it('should dispatch an action', () => {
mapDispatchToPropsObject.resetSendState()
assert(dispatchSpy.calledOnce)
assert.equal(
duckActionSpies.resetSendState.getCall(0).args.length,
0
)
})
})
})
})

View File

@ -17,7 +17,11 @@ const {
} = require('../send.constants')
const stubs = {
addCurrencies: sinon.stub().callsFake((a, b, obj) => a + b),
addCurrencies: sinon.stub().callsFake((a, b, obj) => {
if (String(a).match(/^0x.+/)) a = Number(String(a).slice(2))
if (String(b).match(/^0x.+/)) b = Number(String(b).slice(2))
return a + b
}),
conversionUtil: sinon.stub().callsFake((val, obj) => parseInt(val, 16)),
conversionGTE: sinon.stub().callsFake((obj1, obj2) => obj1.value >= obj2.value),
multiplyCurrencies: sinon.stub().callsFake((a, b) => `${a}x${b}`),
@ -49,6 +53,7 @@ const {
estimateGasPriceFromRecentBlocks,
generateTokenTransferData,
getAmountErrorObject,
getGasFeeErrorObject,
getToAddressForGasUpdate,
calcTokenBalance,
isBalanceSufficient,
@ -143,6 +148,18 @@ describe('send utils', () => {
primaryCurrency: 'ABC',
expectedResult: { amount: INSUFFICIENT_FUNDS_ERROR },
},
'should not return insufficientFunds error if selectedToken is truthy': {
amount: '0x0',
amountConversionRate: 2,
balance: 1,
conversionRate: 3,
gasTotal: 17,
primaryCurrency: 'ABC',
selectedToken: { symbole: 'DEF', decimals: 0 },
decimals: 0,
tokenBalance: 'sometokenbalance',
expectedResult: { amount: null },
},
'should return insufficientTokens error if token is selected and isTokenBalanceSufficient returns false': {
amount: '0x10',
amountConversionRate: 2,
@ -163,6 +180,32 @@ describe('send utils', () => {
})
})
describe('getGasFeeErrorObject()', () => {
const config = {
'should return insufficientFunds error if isBalanceSufficient returns false': {
amountConversionRate: 2,
balance: 16,
conversionRate: 3,
gasTotal: 17,
primaryCurrency: 'ABC',
expectedResult: { gasFee: INSUFFICIENT_FUNDS_ERROR },
},
'should return null error if isBalanceSufficient returns true': {
amountConversionRate: 2,
balance: 16,
conversionRate: 3,
gasTotal: 15,
primaryCurrency: 'ABC',
expectedResult: { gasFee: null },
},
}
Object.entries(config).map(([description, obj]) => {
it(description, () => {
assert.deepEqual(getGasFeeErrorObject(obj), obj.expectedResult)
})
})
})
describe('calcTokenBalance()', () => {
it('should return the calculated token blance', () => {
assert.equal(calcTokenBalance({
@ -222,6 +265,7 @@ describe('send utils', () => {
describe('isTokenBalanceSufficient()', () => {
it('should correctly call conversionUtil and return the result of calling conversionGTE', () => {
stubs.conversionGTE.resetHistory()
stubs.conversionUtil.resetHistory()
const result = isTokenBalanceSufficient({
amount: '0x10',
tokenBalance: 123,

View File

@ -6,6 +6,7 @@ const CLOSE_FROM_DROPDOWN = 'metamask/send/CLOSE_FROM_DROPDOWN'
const OPEN_TO_DROPDOWN = 'metamask/send/OPEN_TO_DROPDOWN'
const CLOSE_TO_DROPDOWN = 'metamask/send/CLOSE_TO_DROPDOWN'
const UPDATE_SEND_ERRORS = 'metamask/send/UPDATE_SEND_ERRORS'
const RESET_SEND_STATE = 'metamask/send/RESET_SEND_STATE'
// TODO: determine if this approach to initState is consistent with conventional ducks pattern
const initState = {
@ -42,6 +43,8 @@ export default function reducer ({ send: sendState = initState }, action = {}) {
...action.value,
},
})
case RESET_SEND_STATE:
return extend({}, initState)
default:
return newState
}
@ -70,3 +73,7 @@ export function updateSendErrors (errorObject) {
value: errorObject,
}
}
export function resetSendState () {
return { type: RESET_SEND_STATE }
}

View File

@ -24,6 +24,7 @@ describe('Send Duck', () => {
const OPEN_TO_DROPDOWN = 'metamask/send/OPEN_TO_DROPDOWN'
const CLOSE_TO_DROPDOWN = 'metamask/send/CLOSE_TO_DROPDOWN'
const UPDATE_SEND_ERRORS = 'metamask/send/UPDATE_SEND_ERRORS'
const RESET_SEND_STATE = 'metamask/send/RESET_SEND_STATE'
describe('SendReducer()', () => {
it('should initialize state', () => {
@ -105,6 +106,15 @@ describe('Send Duck', () => {
})
)
})
it('should return the initial state in response to a RESET_SEND_STATE action', () => {
assert.deepEqual(
SendReducer(mockState, {
type: RESET_SEND_STATE,
}),
Object.assign({}, initState)
)
})
})
describe('openFromDropdown', () => {