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

Display error when service instance creation fails #1173

Merged
merged 1 commit into from
Jul 31, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 7 additions & 3 deletions static_src/actions/service_actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ const serviceActions = {

return cfApi.createServiceInstance(name, spaceGuid, servicePlanGuid)
.then(serviceInstance => serviceActions.fetchInstance(serviceInstance.guid))
.then(serviceActions.createdInstance, serviceActions.errorCreateInstance);
.then(serviceActions.createdInstance)
.catch(serviceActions.errorCreateInstance);
},

createdInstance(serviceInstance) {
Expand All @@ -146,10 +147,13 @@ const serviceActions = {
return Promise.resolve(serviceInstance);
},

errorCreateInstance(err) {
errorCreateInstance(error) {
const { response } = error;
const safeError = (response && response.data) || { code: 500 };
Copy link
Contributor

Choose a reason for hiding this comment

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

coming from a non-JS perspective:

it looks like (response && response.data) could evaluate to true or false.
if true, safeError would be set to true.

if false, it would set safeError to {code: 500};

Now your tests, tell me it doesn't work that way, but just could be a little confusing at first glance.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, don't you also neederror_code to make sure this is a safe error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using this construct in JS, if the first argument evaluates to true, and the second argument is also true, the second argument will be executed. In this instance, executing means storing the value of the second argument in a variable, but you could, for example, pass a function as the second argument.

If the second argument is also false, the fallback will be executed instead.

I concede that this might look a little confusing but it is a pretty common in JS.

If I misunderstood the question, and you already knew this, sorry 😬 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcscottiii For your second comment, error_code needn't be present, the store checks for the existence of one and returns the generic 500 error message if it isn't present.

Copy link
Contributor

@jcscottiii jcscottiii Jul 31, 2017

Choose a reason for hiding this comment

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

Regarding the first one, I've seen it done like this with ternary operators commonly. condition ? happy case : sad case but sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the second one: ahh i gotcha.


AppDispatcher.handleServerAction({
type: serviceActionTypes.SERVICE_INSTANCE_CREATE_ERROR,
error: err
error: safeError
});

return Promise.resolve();
Expand Down
59 changes: 34 additions & 25 deletions static_src/components/create_service_instance.jsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
/**
* Renders the form to create a service instance
*/

import style from 'cloudgov-style/css/cloudgov-style.css';
import PropTypes from 'prop-types';
import React from 'react';
import ReactDOM from 'react-dom';

import Action from './action.jsx';
import { Form, FormText, FormSelect, FormElement, FormError } from './form';
import FormStore from '../stores/form_store';
Expand All @@ -15,7 +12,6 @@ import OrgStore from '../stores/org_store.js';
import SpaceStore from '../stores/space_store.js';
import ServiceInstanceStore from '../stores/service_instance_store.js';
import serviceActions from '../actions/service_actions.js';
import createStyler from '../util/create_styler';
import { validateString } from '../util/validators';

const CREATE_SERVICE_INSTANCE_FORM_GUID = 'create-service-form';
Expand Down Expand Up @@ -48,17 +44,14 @@ export default class CreateServiceInstance extends React.Component {
createError: ServiceInstanceStore.createError
};

// Create the form in the store
FormStore.create(CREATE_SERVICE_INSTANCE_FORM_GUID);

this.validateString = validateString().bind(this);
this._onChange = this._onChange.bind(this);
this._onValidForm = this._onValidForm.bind(this);
this._onCancelForm = this._onCancelForm.bind(this);
this.styler = createStyler(style);
}

componentDidMount() {
FormStore.create(CREATE_SERVICE_INSTANCE_FORM_GUID);
SpaceStore.addChangeListener(this._onChange);
ServiceInstanceStore.addChangeListener(this._onChange);
this.scrollIntoView();
Expand All @@ -80,9 +73,13 @@ export default class CreateServiceInstance extends React.Component {

_onValidForm(errs, values) {
this.setState({ errs }, () => {
const { name, space } = values;
const instanceName = name || null;
const spaceName = space || null;

serviceActions.createInstance(
values.name.value,
values.space.value,
instanceName,
spaceName,
this.props.servicePlan.guid
);
});
Expand All @@ -101,8 +98,18 @@ export default class CreateServiceInstance extends React.Component {
return this.props.servicePlan.name || 'Unknown Service Plan Name';
}

render() {
get validSpaceTargets() {
const currentOrgGuid = OrgStore.currentOrgGuid;
const { spaces } = this.state;

return spaces.filter(space => {
return space.org === currentOrgGuid;
}).map(space => {
return { value: space.guid, label: space.name }
});
}

render() {
let createError;
let createAction = (
<Action label="submit" type="submit">Create service instance</Action>
Expand All @@ -116,14 +123,14 @@ export default class CreateServiceInstance extends React.Component {
createAction = <Loading style="inline" />;
} else if (this.state.createdTempNotification) {
createAction = (
<span className={ this.styler('status', 'status-ok') }>
<span className="status status-ok">
Created! To bind the service instance to an app, go to an application page and use the services panel.
</span>
);
}

return (
<div className = { this.styler('actions-large') }>
<div className="actions-large">
{ createError }
<Form
guid={ CREATE_SERVICE_INSTANCE_FORM_GUID }
Expand All @@ -132,11 +139,13 @@ export default class CreateServiceInstance extends React.Component {
onSubmit={ this._onValidForm }
>
<legend>
Create a service instance for <strong
className={this.styler('actions-callout-inline-block') }>
{ this.serviceName }</strong> using <strong
className={this.styler('actions-callout-inline-block')}>
{ this.servicePlanName }</strong> plan.
Create a service instance for
<strong className="actions-callout-inline-block">
{ this.serviceName }
</strong> using
<strong className="actions-callout-inline-block">
{ this.servicePlanName }
</strong> plan.
</legend>
<FormText
formGuid={ CREATE_SERVICE_INSTANCE_FORM_GUID }
Expand All @@ -150,16 +159,16 @@ export default class CreateServiceInstance extends React.Component {
classes={ ['test-create_service_instance_space'] }
label="Select the space for the service instance"
name="space"
options={ this.state.spaces.filter((space) => {
return space.org === currentOrgGuid;
}).map((space) => {
return { value: space.guid, label: space.name };
})}
options={ this.validSpaceTargets }
validator={ this.validateString }
/>
{ createAction }
<Action label="cancel" style="base" type="outline"
clickHandler={ this._onCancelForm.bind(this) }>
<Action
label="cancel"
style="base"
type="outline"
clickHandler={ this._onCancelForm }
>
Cancel
</Action>
</Form>
Expand Down
24 changes: 6 additions & 18 deletions static_src/components/form/form_error.jsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,14 @@
import PropTypes from 'prop-types';
import React from 'react';
import style from 'cloudgov-style/css/cloudgov-style.css';

import createStyler from '../../util/create_styler';

const propTypes = { message: PropTypes.string };
const defaultProps = { message: '' };

export default class FormError extends React.Component {
constructor(props) {
super(props);
this.styler = createStyler(style);
}

render() {
return (
<span className={ this.styler('error_message')}>
{ this.props.message }
</span>
);
}
}
const FormError = ({ message }) =>
<span className="error_message">
{ message }
</span>;

FormError.propTypes = propTypes;
FormError.defaultProps = defaultProps;

export default FormError;
2 changes: 0 additions & 2 deletions static_src/components/form/form_text.jsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import PropTypes from 'prop-types';
import React from 'react';

import style from 'cloudgov-style/css/cloudgov-style.css';
import FormElement from './form_element.jsx';
import FormError from './form_error.jsx';
import createStyler from '../../util/create_styler';


export default class FormText extends FormElement {
constructor(props) {
super(props);
Expand Down
20 changes: 9 additions & 11 deletions static_src/stores/form_store.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,15 @@ export class FormStore extends BaseStore {
return form.fields[fieldName];
}

create(formGuid, initialData) {
const formFields = Object.keys(initialData || {})
.reduce((fields, fieldName) => {
const formField = {
name: fieldName,
value: initialData[fieldName]
};

return { ...fields, [fieldName]: formField };
}, {});

create(formGuid, initialData = {}) {
const formFields = Object.keys(initialData).reduce((fields, fieldName) => {
const formField = {
name: fieldName,
value: initialData[fieldName]
};

return { ...fields, [fieldName]: formField };
}, {});
const form = { guid: formGuid, fields: formFields };
this.push(form);
return form;
Expand Down
19 changes: 18 additions & 1 deletion static_src/stores/service_instance_store.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,21 @@ const APP_STATE_MAP = {
[OPERATION_RUNNING]: appStates.running
};

const FRIENDLY_ERROR_MAP = {
'CF-ServiceInstanceInvalid': 'Invalid space selected.',
'CF-MessageParseError': 'One or more form fields is blank or invalid.'
};

const getFriendlyError = error => {
const { code, error_code: errorCode } = error;

if (errorCode in FRIENDLY_ERROR_MAP) {
return FRIENDLY_ERROR_MAP[errorCode];
}

return `Error #${code}: please contact cloud.gov support for help troubleshooting this issue.`;
};

export class ServiceInstanceStore extends BaseStore {
constructor() {
super();
Expand Down Expand Up @@ -199,7 +214,9 @@ export class ServiceInstanceStore extends BaseStore {
}

case serviceActionTypes.SERVICE_INSTANCE_CREATE_ERROR: {
this._createError = action.error;
this._createError = {
description: getFriendlyError(action.error)
};
this._createLoading = false;
this.emitChange();
break;
Expand Down
51 changes: 39 additions & 12 deletions static_src/test/unit/actions/service_actions.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,7 @@ describe('serviceActions', function() {
});

describe('createdInstance()', function() {
it('should dispatch a server event of type instance created with service',
function() {
it('dispatchs a server event of type instance created with service', () => {
var expectedInstance = { guid: 'asdfas' };

let expectedParams = {
Expand All @@ -373,19 +372,47 @@ describe('serviceActions', function() {
});
});

describe('errorCreateInstance()', function() {
it('should dispatch a server event of type error create instance', function() {
var expectedErr = { status: 400 };
describe('errorCreateInstance()', () => {
const type = serviceActionTypes.SERVICE_INSTANCE_CREATE_ERROR;
let spy;

let expectedParams = {
error: expectedErr
}
let spy = setupServerSpy(sandbox);
beforeEach(() => {
spy = sandbox.spy(AppDispatcher, 'handleServerAction');
});

serviceActions.errorCreateInstance(expectedErr);
afterEach(() => {
spy.restore();
});

assertAction(spy, serviceActionTypes.SERVICE_INSTANCE_CREATE_ERROR,
expectedParams);
describe('server fault', () => {
it('dispatches the correct error type and a code 500', () => {
const originalError = { message: 'Bad error', stack: [] };
const actual = { code: 500 };

serviceActions.errorCreateInstance(originalError);

const actionInfo = spy.getCall(0).args[0];
Copy link
Contributor Author

@el-mapache el-mapache Jul 28, 2017

Choose a reason for hiding this comment

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

I'm not using the test helpers here as I was having a hard time getting the test to validate when the arguments passed to the dispatch function are objects.

const { type, error } = actionInfo;

expect(spy).toHaveBeenCalledOnce();
expect(actionInfo.type).toEqual(type);
expect(error).toEqual(actual);
});
});

describe('API error/malformed request', () => {
it('dispatches the correct error type and error data object', () => {
const originalError = { response: { data: { hey: 'there' } } };

serviceActions.errorCreateInstance(originalError);

const actionInfo = spy.getCall(0).args[0];
const { type, error } = actionInfo;

expect(spy).toHaveBeenCalledOnce();
expect(actionInfo.type).toEqual(type);
expect(error).toEqual(originalError.response.data);
});
});
});

Expand Down
23 changes: 23 additions & 0 deletions static_src/test/unit/components/create_service_instance.spec.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import '../../global_setup';

import React from 'react';
import CreateServiceInstance from '../../../components/create_service_instance.jsx';
import FormError from '../../../components/form/form_error.jsx';
import Immutable from 'immutable';
import SpaceStore from '../../../stores/space_store';
import ServiceInstanceStore from '../../../stores/service_instance_store';
import { shallow } from 'enzyme';

describe('<CreateServiceInstance />', () => {
beforeEach(() => {
ServiceInstanceStore._createError = { description: 'Bad stuff everyone' };
});

it('displays an error message when ServiceInstanceStore has one', () => {
SpaceStore._data = Immutable.fromJS([]);

const wrapper = shallow(<CreateServiceInstance servicePlan={ {} } />);

expect(wrapper.find(FormError).length).toBe(1);
});
});
1 change: 1 addition & 0 deletions static_src/test/unit/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export function assertAction(spy, type, params) {
expect(spy).toHaveBeenCalledOnce();
let actionInfo = spy.getCall(0).args[0];
expect(actionInfo.type).toEqual(type);

for (let param in params) {
const datum = 'data' in actionInfo ? actionInfo.data[param] : actionInfo[param];
expect(datum).toEqual(params[param]);
Expand Down
Loading