Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix newError noops when not bound to dispacher #4013

Merged
merged 6 commits into from
Jan 3, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions js/src/modals/AddContract/addContract.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import { observer } from 'mobx-react';
import React, { Component, PropTypes } from 'react';
import { FormattedMessage } from 'react-intl';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';

import { newError } from '~/redux/actions';
import { Button, Modal, Form, Input, InputAddress, RadioButtons } from '~/ui';
Expand All @@ -25,13 +27,14 @@ import { AddIcon, CancelIcon, NextIcon, PrevIcon } from '~/ui/Icons';
import Store from './store';

@observer
export default class AddContract extends Component {
class AddContract extends Component {
static contextTypes = {
api: PropTypes.object.isRequired
}

static propTypes = {
contracts: PropTypes.object.isRequired,
newError: PropTypes.func.isRequired,
onClose: PropTypes.func
};

Expand Down Expand Up @@ -244,11 +247,26 @@ export default class AddContract extends Component {
this.onClose();
})
.catch((error) => {
newError(error);
this.props.newError(error);
});
}

onClose = () => {
this.props.onClose();
}
}

function mapStateToProps (state) {
return {};
}

function mapDispatchToProps (dispatch) {
return bindActionCreators({
newError
}, dispatch);
}

export default connect(
mapStateToProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an optimisation:

If you omit [mapDispatchToProps], the component will not be subscribed to the Redux store.

You could pass null, it occurs multiple times in the codebase already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer being explicit and in a once-off situation being explicit about an empty state is not bad. However who knows what happens with Redux making that call each cycle as things changes, it just add unnecessary overheads.

Will update.

mapDispatchToProps
)(AddContract);
47 changes: 38 additions & 9 deletions js/src/modals/AddContract/addContract.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,36 +20,65 @@ import sinon from 'sinon';

import AddContract from './';

import { CONTRACTS, createApi } from './addContract.test.js';
import { CONTRACTS, createApi, createRedux } from './addContract.test.js';

let api;
let component;
let instance;
let onClose;
let reduxStore;

function renderShallow (props) {
function render (props = {}) {
api = createApi();
onClose = sinon.stub();
reduxStore = createRedux();

component = shallow(
<AddContract
{ ...props }
contracts={ CONTRACTS }
onClose={ onClose } />,
{
context: {
api: createApi()
}
}
);
{ context: { store: reduxStore } }
).find('AddContract').shallow({ context: { api } });
instance = component.instance();

return component;
}

describe('modals/AddContract', () => {
describe('rendering', () => {
beforeEach(() => {
renderShallow();
render();
});

it('renders the defauls', () => {
expect(component).to.be.ok;
});
});

describe('onAdd', () => {
it('calls store addContract', () => {
sinon.stub(instance.store, 'addContract').resolves(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case it's not worth it yet, but once we have more stubs, restoring them can easily be forgotten (which breaks the whole point of the stubs then). sandboxes try to address this issue.

return instance.onAdd().then(() => {
expect(instance.store.addContract).to.have.been.called;
instance.store.addContract.restore();
});
});

it('calls closes dialog on success', () => {
sinon.stub(instance.store, 'addContract').resolves(true);
return instance.onAdd().then(() => {
expect(onClose).to.have.been.called;
instance.store.addContract.restore();
});
});

it('adds newError on failure', () => {
sinon.stub(instance.store, 'addContract').rejects('test');
return instance.onAdd().then(() => {
expect(reduxStore.dispatch).to.have.been.calledWith({ error: new Error('test'), type: 'newError' });
instance.store.addContract.restore();
});
});
});
});
13 changes: 12 additions & 1 deletion js/src/modals/AddContract/addContract.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,19 @@ function createApi () {
};
}

function createRedux () {
return {
dispatch: sinon.stub(),
subscribe: sinon.stub(),
getState: () => {
return {};
}
};
}

export {
ABI,
CONTRACTS,
createApi
createApi,
createRedux
};
26 changes: 24 additions & 2 deletions js/src/modals/EditMeta/editMeta.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,24 @@
import { observer } from 'mobx-react';
import React, { Component, PropTypes } from 'react';
import { FormattedMessage } from 'react-intl';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';

import { newError } from '~/redux/actions';
import { Button, Form, Input, InputChip, Modal } from '~/ui';
import { CancelIcon, SaveIcon } from '~/ui/Icons';

import Store from './store';

@observer
export default class EditMeta extends Component {
class EditMeta extends Component {
static contextTypes = {
api: PropTypes.object.isRequired
}

static propTypes = {
account: PropTypes.object.isRequired,
newError: PropTypes.func.isRequired,
onClose: PropTypes.func.isRequired
}

Expand Down Expand Up @@ -138,6 +142,24 @@ export default class EditMeta extends Component {

return this.store
.save()
.then(() => this.props.onClose());
.then(() => this.props.onClose())
.catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

this.props.newError(error);
});
}
}

function mapStateToProps (state) {
return {};
}

function mapDispatchToProps (dispatch) {
return bindActionCreators({
newError
}, dispatch);
}

export default connect(
mapStateToProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

mapDispatchToProps
)(EditMeta);
36 changes: 26 additions & 10 deletions js/src/modals/EditMeta/editMeta.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,27 @@ import sinon from 'sinon';

import EditMeta from './';

import { ACCOUNT, createApi } from './editMeta.test.js';
import { ACCOUNT, createApi, createRedux } from './editMeta.test.js';

let api;
let component;
let instance;
let onClose;
let reduxStore;

function render (props) {
api = createApi();
onClose = sinon.stub();
reduxStore = createRedux();

component = shallow(
<EditMeta
{ ...props }
account={ ACCOUNT }
onClose={ onClose } />,
{
context: {
api: createApi()
}
}
);
{ context: { store: reduxStore } }
).find('EditMeta').shallow({ context: { api } });
instance = component.instance();

return component;
}
Expand All @@ -56,15 +58,29 @@ describe('modals/EditMeta', () => {
});

describe('onSave', () => {
it('calls store.save() & props.onClose', () => {
const instance = component.instance();
it('calls store.save', () => {
sinon.spy(instance.store, 'save');

instance.onSave().then(() => {
return instance.onSave().then(() => {
expect(instance.store.save).to.have.been.called;
instance.store.save.restore();
});
});

it('closes the dialog on success', () => {
return instance.onSave().then(() => {
expect(onClose).to.have.been.called;
});
});

it('adds newError on failure', () => {
sinon.stub(instance.store, 'save').rejects('test');

return instance.onSave().then(() => {
expect(reduxStore.dispatch).to.have.been.calledWith({ error: new Error('test'), type: 'newError' });
instance.store.save.restore();
});
});
});
});
});
13 changes: 12 additions & 1 deletion js/src/modals/EditMeta/editMeta.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,19 @@ function createApi () {
};
}

function createRedux () {
return {
dispatch: sinon.stub(),
subscribe: sinon.stub(),
getState: () => {
return {};
}
};
}

export {
ACCOUNT,
ADDRESS,
createApi
createApi,
createRedux
};
4 changes: 1 addition & 3 deletions js/src/modals/EditMeta/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import { action, computed, observable, transaction } from 'mobx';

import { newError } from '~/redux/actions';
import { validateName } from '~/util/validation';

export default class Store {
Expand Down Expand Up @@ -92,8 +91,7 @@ export default class Store {
])
.catch((error) => {
console.error('onSave', error);

newError(error);
throw error;
});
}
}
24 changes: 21 additions & 3 deletions js/src/modals/PasswordManager/passwordManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import { Tabs, Tab } from 'material-ui/Tabs';
import { observer } from 'mobx-react';
import React, { Component, PropTypes } from 'react';
import { FormattedMessage } from 'react-intl';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';

import { newError, showSnackbar } from '~/redux/actions';
import { Button, Modal, IdentityName, IdentityIcon } from '~/ui';
Expand All @@ -42,13 +44,14 @@ const TABS_ITEM_STYLE = {
};

@observer
export default class PasswordManager extends Component {
class PasswordManager extends Component {
static contextTypes = {
api: PropTypes.object.isRequired
}

static propTypes = {
account: PropTypes.object.isRequired,
newError: PropTypes.func.isRequired,
onClose: PropTypes.func
}

Expand Down Expand Up @@ -347,15 +350,30 @@ export default class PasswordManager extends Component {
}
})
.catch((error) => {
newError(error);
this.props.newError(error);
});
}

testPassword = () => {
return this.store
.testPassword()
.catch((error) => {
newError(error);
this.props.newError(error);
});
}
}

function mapStateToProps (state) {
return {};
}

function mapDispatchToProps (dispatch) {
return bindActionCreators({
newError
}, dispatch);
}

export default connect(
mapStateToProps,
mapDispatchToProps
)(PasswordManager);
Loading