From c1b2527319c6bb6068d5714946b32def00b061dc Mon Sep 17 00:00:00 2001 From: Maciej Barelkowski Date: Fri, 23 Nov 2018 13:13:16 +0100 Subject: [PATCH] feat(deploy-toolbar): improve error handling * use endpoint url provided by client * display user-friendly error messages * do not throw error for response other than JSON Closes #838 --- app/lib/createDeployer.js | 93 ++++++---- app/lib/index.js | 41 ++--- app/lib/util/renderer.js | 6 +- app/test/helper/mock/fetch.js | 1 + app/test/spec/deploy-spec.js | 167 ++++++++++++++---- .../app/__tests__/DeployDiagramModalSpec.js | 17 +- .../DeployDiagramModal.js | 24 ++- .../app/deploy-diagram-modal/ErrorMessage.js | 9 +- .../deploy-diagram-modal/ErrorMessage.less | 4 + .../src/app/deploy-diagram-modal/Success.js | 9 +- .../src/app/deploy-diagram-modal/Success.less | 4 + client/src/app/deploy-diagram-modal/View.js | 8 +- .../getCamundaBpmErrorMessage.js | 10 ++ .../error-messages/getNetworkErrorMessage.js | 13 ++ .../getStatusCodeErrorMessage.js | 23 +++ .../error-messages/index.js | 9 + 16 files changed, 324 insertions(+), 114 deletions(-) create mode 100644 client/src/app/deploy-diagram-modal/error-messages/getCamundaBpmErrorMessage.js create mode 100644 client/src/app/deploy-diagram-modal/error-messages/getNetworkErrorMessage.js create mode 100644 client/src/app/deploy-diagram-modal/error-messages/getStatusCodeErrorMessage.js create mode 100644 client/src/app/deploy-diagram-modal/error-messages/index.js diff --git a/app/lib/createDeployer.js b/app/lib/createDeployer.js index 853cf85604..2dd7780139 100644 --- a/app/lib/createDeployer.js +++ b/app/lib/createDeployer.js @@ -8,55 +8,74 @@ function createDeployer({ fetch, fs, FormData }) { /** * Deploy diagram to the given endpoint URL. */ - return function deploy(url, { deploymentName, tenantId, file = {} }, cb) { + return async function deploy(url, { deploymentName, tenantId, file = {} }, cb) { - // callback is optional - cb = cb || noop; + try { + // callback is optional + cb = cb || noop; - if (!deploymentName) { - return cb( - new Error( - 'Failed to deploy process, deployment name must be provided.' - ) - ); - } + if (!deploymentName) { + throw new Error('Failed to deploy process, deployment name must be provided.'); + } - if (!file.fileType || !file.name || !file.path) { - return cb( - new Error( - 'Failed to deploy process, file name and path must be provided.' - ) - ); - } + if (!file.name || !file.path) { + throw new Error('Failed to deploy process, file name and path must be provided.'); + } - if (!url) { - return cb( - new Error( - 'Failed to deploy process, endpoint url must not be empty.' - ) - ); - } + if (!url) { + throw new Error('Failed to deploy process, endpoint url must not be empty.'); + } - const form = new FormData(); + const form = new FormData(); - form.append('deployment-name', deploymentName); + form.append('deployment-name', deploymentName); - if (tenantId) { - form.append('tenant-id', tenantId); - } + if (tenantId) { + form.append('tenant-id', tenantId); + } + + form.append(file.name, fs.createReadStream(file.path)); + + const serverResponse = await fetch(url, { method: 'POST', body: form }); + + if (!serverResponse.ok) { + const error = await getErrorFromResponse(serverResponse); + throw error; + } + + let response; - form.append(file.name, fs.createReadStream(file.path)); + try { + response = await serverResponse.json(); + } catch (error) { + response = serverResponse.statusText; + } + + return cb(null, response); + } catch (error) { + return cb(error); + } - fetch(url, { method: 'POST', body: form }) - .then(res => res.json()) - .then(json => cb(null, json)) - .catch(function(error) { - cb(error); - }); }; } module.exports = createDeployer; -function noop() { } \ No newline at end of file +// helpers ////// +function noop() { } + + +async function getErrorFromResponse(response) { + const error = new Error(); + + try { + const body = await response.json(); + error.message = body.message; + } catch (_) { + error.code = response.status; + error.message = response.statusText; + } + + return error; +} diff --git a/app/lib/index.js b/app/lib/index.js index be23b1c39e..665f488c4a 100644 --- a/app/lib/index.js +++ b/app/lib/index.js @@ -167,31 +167,7 @@ renderer.on('dialog:show', async function(options, done) { // deploying ////////// // TODO: remove and add as plugin instead -renderer.on('deploy', function(data, done) { - var workspaceConfig = config.get('workspace', { endpoints: [] }); - - var endpointUrl = (workspaceConfig.endpoints || [])[0]; - - if (!endpointUrl) { - - let err = new Error('no deploy endpoint configured'); - - console.error('failed to deploy', err); - return done(err.message); - } - - deploy(endpointUrl, data, function(err, result) { - - if (err) { - console.error('failed to deploy', err); - - return done(err.message); - } - - done(null, result); - }); - -}); +renderer.on('deploy', handleDeployment); // filesystem ////////// @@ -430,5 +406,20 @@ app.on('ready', function() { }); +function handleDeployment(data, done) { + const { endpointUrl } = data; + + deploy(endpointUrl, data, function(error, result) { + + if (error) { + console.error('failed to deploy', error); + + return done(error); + } + + done(null, result); + }); +} + // expose app module.exports = app; diff --git a/app/lib/util/renderer.js b/app/lib/util/renderer.js index bf343c09c9..31ee824df7 100644 --- a/app/lib/util/renderer.js +++ b/app/lib/util/renderer.js @@ -5,7 +5,8 @@ var electron = require('electron'), app = electron.app; const { - assign + assign, + pick } = require('min-dash'); @@ -15,8 +16,9 @@ function on(event, callback, that) { function done(...doneArgs) { var actualArgs = doneArgs.map(function(e) { + // error.message and error.code are not enumerable if (e instanceof Error) { - return assign({}, e); + return assign({}, pick(e, [ 'message', 'code' ]), e); } return e; diff --git a/app/test/helper/mock/fetch.js b/app/test/helper/mock/fetch.js index a6bd2b3d5b..be62568023 100644 --- a/app/test/helper/mock/fetch.js +++ b/app/test/helper/mock/fetch.js @@ -2,6 +2,7 @@ var RESPONSE_OK = { mocked: true }; module.exports = function(url, options) { return Promise.resolve({ + ok: true, json: () => Promise.resolve(RESPONSE_OK) }); }; diff --git a/app/test/spec/deploy-spec.js b/app/test/spec/deploy-spec.js index 5893cb0367..a6a2661766 100644 --- a/app/test/spec/deploy-spec.js +++ b/app/test/spec/deploy-spec.js @@ -1,41 +1,36 @@ 'use strict'; -var sinon = require('sinon'); +const sinon = require('sinon'); -var createDeployer = require('../../lib/createDeployer'); +const createDeployer = require('../../lib/createDeployer'); -var fetch = require('../helper/mock/fetch'), - fs = require('../helper/mock/fs'), - FormData = require('../helper/mock/form-data'); +const fetch = require('../helper/mock/fetch'), + fs = require('../helper/mock/fs'), + FormData = require('../helper/mock/form-data'); describe('deploy', function() { - var fetchSpy; + let fetchSpy; beforeEach(() => { fetchSpy = sinon.spy(fetch); }); + afterEach(sinon.restore); + it('should deploy with provided parameters', function(done) { // given - var deploy = createDeploy(fetchSpy); - - const data = { - deploymentName: 'some deployment name', - tenantId: 'some tenant id', - file: { - name: 'some name', - fileType: 'bpmn', - path: 'some/path' - } - }; + const deploy = createDeploy(fetchSpy); + + const data = getDeploymentData({ tenantId: 'someTenantId' }); const url = 'some/url'; const expectedForm = new FormData(); + expectedForm.append('deployment-name', data.deploymentName); expectedForm.append(data.file.name, fs.createReadStream(data.file.path)); expectedForm.append('tenant-id', data.tenantId); @@ -58,32 +53,126 @@ describe('deploy', function() { }); - it('should handle backend error', function(done) { + it('should deploy even without tenant id provided', function(done) { // given - function failingFetch(url) { - return Promise.reject(new Error('backend unavailable')); + const deploy = createDeploy(fetchSpy); + + const data = getDeploymentData(); + + const url = 'some/url'; + + const expectedForm = new FormData(); + + expectedForm.append('deployment-name', data.deploymentName); + expectedForm.append(data.file.name, fs.createReadStream(data.file.path)); + + // when + deploy(url, data, (err, data) => { + + // then + expect(fetchSpy).to.have.been.calledWith( + url, + { body: expectedForm, method: 'POST' } + ); + + expect(err).not.to.exist; + expect(data).to.eql(fetch.RESPONSE_OK); + + done(); + }); + + }); + + + it('should NOT throw error when response is OK but not a JSON', function(done) { + + // given + const okResponse = 'OK'; + + function fetchResolvingToText() { + return Promise.resolve({ + ok: true, + statusText: okResponse, + json() { + return Promise.reject(new Error('fail on json parse')); + } + }); } // given - var deploy = createDeploy(failingFetch); - - const data = { - deploymentName: 'some deployment name', - tenantId: undefined, - file: { - name: 'some name', - fileType: 'bpmn', - path: 'some/path' - } - }; + const deploy = createDeploy(fetchResolvingToText); + + const data = getDeploymentData(); + + // when + deploy('some/url', data, (err, data) => { + + // then + expect(err).to.not.exist; + expect(data).to.eql(okResponse); + + done(); + }); + }); + + + it('should handle fetch error', function(done) { + + // given + const fetchError = 'FETCH_ERROR'; + + function failingFetch() { + return Promise.reject(new Error(fetchError)); + } + + // given + const deploy = createDeploy(failingFetch); + + const data = getDeploymentData(); + + // when + deploy('some/url', data, (err, data) => { + + // then + expect(err).to.exist; + expect(err.message).to.eql(fetchError); + + expect(data).not.to.exist; + + done(); + }); + }); + + + it('should return error with proper code for backend error', function(done) { + + // given + const errorStatus = 500, + errorStatusText = 'INTERNAL SERVER ERROR'; + + function failingFetch() { + return Promise.resolve({ + ok: false, + status: errorStatus, + statusText: errorStatusText, + json() { + return Promise.reject(new Error('fail on json parse')); + } + }); + } + + // given + const deploy = createDeploy(failingFetch); + + const data = getDeploymentData(); // when deploy('some/url', data, (err, data) => { // then expect(err).to.exist; - expect(err.message).to.eql('backend unavailable'); + expect(err.code).to.eql(errorStatus); expect(data).not.to.exist; @@ -94,10 +183,22 @@ describe('deploy', function() { }); + +// helpers ///////// function createDeploy(fetch) { return createDeployer({ fetch, fs, FormData }); -} \ No newline at end of file +} + +function getDeploymentData(options = {}) { + return Object.assign({ + deploymentName: 'some deployment name', + file: { + name: 'some name', + path: 'some/path' + } + }, options); +} diff --git a/client/src/app/__tests__/DeployDiagramModalSpec.js b/client/src/app/__tests__/DeployDiagramModalSpec.js index d805119584..f20513b8fb 100644 --- a/client/src/app/__tests__/DeployDiagramModalSpec.js +++ b/client/src/app/__tests__/DeployDiagramModalSpec.js @@ -16,11 +16,9 @@ describe('', function() { }); - it('should set state.error to error message when onDeploy throws error', async function() { + it('should set state.error when onDeploy throws error', async function() { // given - const errorMessage = 'error'; - - const onDeployStub = sinon.stub().throws({ message: errorMessage }); + const onDeployStub = sinon.stub().rejects(new Error('errorMessage')); const wrapper = shallow(); const instance = wrapper.instance(); @@ -29,14 +27,15 @@ describe('', function() { await instance.handleDeploy(new Event('click')); // expect - expect(instance.state.error).to.be.equal(errorMessage); + expect(instance.state.error).to.be.a('string').not.eql(''); + expect(instance.state.success).to.eql(''); + expect(instance.state.isLoading).to.be.false; }); - it('should set state.success to success message when onDeploy resolves', async function() { + it('should set state.success when onDeploy resolves', async function() { // given const endpointUrl = 'http://example.com'; - const successMessage = `Successfully deployed diagram to ${endpointUrl}`; const onDeployStub = sinon.stub().resolves(); @@ -48,7 +47,9 @@ describe('', function() { await instance.handleDeploy(new Event('click')); // expect - expect(instance.state.success).to.be.equal(successMessage); + expect(instance.state.success).to.be.a('string').not.eql(''); + expect(instance.state.error).to.eql(''); + expect(instance.state.isLoading).to.be.false; }); }); diff --git a/client/src/app/deploy-diagram-modal/DeployDiagramModal.js b/client/src/app/deploy-diagram-modal/DeployDiagramModal.js index 821c0eeb4e..ef04cb243f 100644 --- a/client/src/app/deploy-diagram-modal/DeployDiagramModal.js +++ b/client/src/app/deploy-diagram-modal/DeployDiagramModal.js @@ -2,6 +2,8 @@ import React from 'react'; import View from './View'; +import errorMessageFunctions from './error-messages'; + const defaultState = { isLoading: false, @@ -28,7 +30,7 @@ class DeployDiagramModal extends React.Component { isLoading: true }); - const payload = this.getPayloadFromState(); + const payload = this.getDeploymentPayload(); try { await this.props.onDeploy(payload); @@ -39,10 +41,12 @@ class DeployDiagramModal extends React.Component { error: '' }); } catch (error) { + const errorMessage = this.getErrorMessage(error); + this.setState({ isLoading: false, success: '', - error: error.message + error: errorMessage }); } } @@ -70,7 +74,7 @@ class DeployDiagramModal extends React.Component { />; } - getPayloadFromState() { + getDeploymentPayload() { const payload = { endpointUrl: this.state.endpointUrl, deploymentName: this.state.deploymentName, @@ -79,6 +83,20 @@ class DeployDiagramModal extends React.Component { return payload; } + + getErrorMessage(error) { + let errorMessage; + + for (const getMessage of errorMessageFunctions) { + errorMessage = getMessage(error); + + if (errorMessage) { + return errorMessage; + } + } + + return 'Unknown error occurred.'; + } } export default DeployDiagramModal; diff --git a/client/src/app/deploy-diagram-modal/ErrorMessage.js b/client/src/app/deploy-diagram-modal/ErrorMessage.js index 18e39c7f09..34af4f793a 100644 --- a/client/src/app/deploy-diagram-modal/ErrorMessage.js +++ b/client/src/app/deploy-diagram-modal/ErrorMessage.js @@ -3,9 +3,14 @@ import React from 'react'; import css from './ErrorMessage.less'; -const ErrorMessage = ({ message }) => ( +const ErrorMessage = ({ heading, message }) => (
- Error: { message } +

+ { heading } +

+

+ { message } +

); diff --git a/client/src/app/deploy-diagram-modal/ErrorMessage.less b/client/src/app/deploy-diagram-modal/ErrorMessage.less index 51763f5a82..8b9b072b62 100644 --- a/client/src/app/deploy-diagram-modal/ErrorMessage.less +++ b/client/src/app/deploy-diagram-modal/ErrorMessage.less @@ -6,6 +6,10 @@ color: #721c24; background-color: #f8d7da; border-color: #f5c6cb; + + p { + margin: .5em; + } } :local(.ErrorContent) { diff --git a/client/src/app/deploy-diagram-modal/Success.js b/client/src/app/deploy-diagram-modal/Success.js index 2853d73ac2..ae0d585270 100644 --- a/client/src/app/deploy-diagram-modal/Success.js +++ b/client/src/app/deploy-diagram-modal/Success.js @@ -3,9 +3,14 @@ import React from 'react'; import css from './Success.less'; -const Success = ({ message }) => ( +const Success = ({ heading, message }) => (
- Success: { message } +

+ { heading } +

+

+ { message } +

); diff --git a/client/src/app/deploy-diagram-modal/Success.less b/client/src/app/deploy-diagram-modal/Success.less index 3ed7b7b95e..23f09233d5 100644 --- a/client/src/app/deploy-diagram-modal/Success.less +++ b/client/src/app/deploy-diagram-modal/Success.less @@ -6,6 +6,10 @@ color: #155724; background-color: #d4edda; border-color: #c3e6cb; + + p { + margin: .5em; + } } :local(.SuccessContent) { diff --git a/client/src/app/deploy-diagram-modal/View.js b/client/src/app/deploy-diagram-modal/View.js index 4c7c09d5d8..b3fe78b577 100644 --- a/client/src/app/deploy-diagram-modal/View.js +++ b/client/src/app/deploy-diagram-modal/View.js @@ -9,6 +9,10 @@ import Success from './Success'; import { View as style } from './View.less'; +const SUCCESS_HEADING = '✅ Successfully deployed the diagram.'; +const ERROR_HEADING = '❌ The diagram could not be deployed.'; + + const View = (props) => { const { error, @@ -26,9 +30,9 @@ const View = (props) => { { isLoading && } - { success && } + { success && } - { error && } + { error && } diff --git a/client/src/app/deploy-diagram-modal/error-messages/getCamundaBpmErrorMessage.js b/client/src/app/deploy-diagram-modal/error-messages/getCamundaBpmErrorMessage.js new file mode 100644 index 0000000000..6ca183a8de --- /dev/null +++ b/client/src/app/deploy-diagram-modal/error-messages/getCamundaBpmErrorMessage.js @@ -0,0 +1,10 @@ +const ERROR_MESSAGE = { + BPMN_PARSING_ERROR: 'Server could not parse the diagram. Please check log for errors and then redeploy.' +}; + + +export default function getCamundaBpmErrorMessage(error) { + if (/^ENGINE-09005/.test(error.message)) { + return ERROR_MESSAGE.BPMN_PARSING_ERROR; + } +} diff --git a/client/src/app/deploy-diagram-modal/error-messages/getNetworkErrorMessage.js b/client/src/app/deploy-diagram-modal/error-messages/getNetworkErrorMessage.js new file mode 100644 index 0000000000..4530519ab6 --- /dev/null +++ b/client/src/app/deploy-diagram-modal/error-messages/getNetworkErrorMessage.js @@ -0,0 +1,13 @@ +const ERROR_MESSAGE = { + NO_INTERNET_CONNECTION: 'Could not connect to the server. Please check you Internet connection status.' +}; + + +export default function getNetworkErrorMessage(error) { + switch (error.code) { + case 'ECONNRESET': + case 'ECONNREFUSED': + case 'ENOTFOUND': + return ERROR_MESSAGE.NO_INTERNET_CONNECTION; + } +} \ No newline at end of file diff --git a/client/src/app/deploy-diagram-modal/error-messages/getStatusCodeErrorMessage.js b/client/src/app/deploy-diagram-modal/error-messages/getStatusCodeErrorMessage.js new file mode 100644 index 0000000000..0a23b79c0f --- /dev/null +++ b/client/src/app/deploy-diagram-modal/error-messages/getStatusCodeErrorMessage.js @@ -0,0 +1,23 @@ +const ERROR_MESSAGE = { + UNAUTHORIZED: 'The deployment was unauthorized. Please use valid credentials.', + FORBIDDEN: 'The deployment was not permitted for your credentials. Please use different credentials.', + NOT_FOUND: 'Could not find the provided URL. Please check the endpoint URL.', + INTERNAL_SERVER_ERROR: 'There was an unknown server related issue. Please check the server status.', + SERVER_UNAVAILABLE: 'Server is currently unavailable. Please try again later.' +}; + + +export default function getStatusCodeErrorMessage(error) { + switch (error.code) { + case 401: + return ERROR_MESSAGE.UNAUTHORIZED; + case 403: + return ERROR_MESSAGE.FORBIDDEN; + case 404: + return ERROR_MESSAGE.NOT_FOUND; + case 500: + return ERROR_MESSAGE.INTERNAL_SERVER_ERROR; + case 503: + return ERROR_MESSAGE.SERVER_UNAVAILABLE; + } +} \ No newline at end of file diff --git a/client/src/app/deploy-diagram-modal/error-messages/index.js b/client/src/app/deploy-diagram-modal/error-messages/index.js new file mode 100644 index 0000000000..8472441af1 --- /dev/null +++ b/client/src/app/deploy-diagram-modal/error-messages/index.js @@ -0,0 +1,9 @@ +import getCamundaBpmErrorMessage from './getCamundaBpmErrorMessage'; +import getNetworkErrorMessage from './getNetworkErrorMessage'; +import getStatusCodeErrorMessage from './getStatusCodeErrorMessage'; + +export default [ + getCamundaBpmErrorMessage, + getNetworkErrorMessage, + getStatusCodeErrorMessage +];