Skip to content

Commit

Permalink
feat(client): errors (import/export/modeling) are escalated to app
Browse files Browse the repository at this point in the history
Related to #896
  • Loading branch information
philippfromme committed Sep 13, 2018
1 parent d6c5c23 commit 5f836d7
Show file tree
Hide file tree
Showing 7 changed files with 111 additions and 27 deletions.
7 changes: 7 additions & 0 deletions client/src/app/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,12 @@ export class App extends Component {
});
}

handleTabError = (tab, error) => {
console.warn('App#handleTabError', tab, error);

// TODO: show error in log
}

handleTabChanged = (tab, properties = {}) => {

let {
Expand Down Expand Up @@ -799,6 +805,7 @@ export class App extends Component {
key={ activeTab.id }
tab={ activeTab }
onChanged={ this.handleTabChanged }
onError={ this.handleTabError }
onShown={ this.handleTabShown }
onContextMenu={ this.openTabMenu }
ref={ this.tabRef }
Expand Down
47 changes: 46 additions & 1 deletion client/src/app/__tests__/AppSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ describe('<App>', function() {
});


it.only('should export SVG', async function() {
it('should export SVG', async function() {

// given
await app.createDiagram();
Expand Down Expand Up @@ -639,6 +639,51 @@ describe('<App>', function() {

});


describe('errors', function() {

let app, openedTabs;

beforeEach(async function() {
const rendered = createApp(mount);

app = rendered.app;

const file1 = createFile('1.bpmn');

openedTabs = [

This comment has been minimized.

Copy link
@nikku

nikku Sep 16, 2018

Member

Why not simply openTabs = await app.openFiles(...)?

This comment has been minimized.

Copy link
@philippfromme

philippfromme Sep 17, 2018

Author Contributor

Copy and paste lazyness. 🐶

...(await app.openFiles([ file1 ])),
];

// assume
const {
tabs,
activeTab
} = app.state;

expect(tabs).to.eql(openedTabs);
expect(activeTab).to.eql(openedTabs[0]);
});


// TODO(philippfromme): spy is not called, why?
it.skip('should error', function() {

// given
const handleTabErrorSpy = sinon.spy(app, 'handleTabError');

const tab = app.tabRef.current;

// when
tab.triggerAction('error', 'foo');

// then
expect(handleTabErrorSpy).to.have.been.called;
// expect(handleTabErrorSpy).to.have.been.calledWith(openedTabs[0], 'foo');
});

});

});

});
Expand Down
8 changes: 8 additions & 0 deletions client/src/app/__tests__/mocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ class FakeTab extends Component {
}

triggerAction(action, options) {
const {
tab
} = this.props;

console.log('FakeTab#triggerAction', action, options);

if (action === 'save') {
Expand All @@ -21,6 +25,10 @@ class FakeTab extends Component {
if (action === 'export-as') {
return 'CONTENTS';
}

if (action === 'error') {
this.props.onError(tab, options);
}
}

render() {
Expand Down
9 changes: 8 additions & 1 deletion client/src/app/tabs/MultiSheetTab.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ class MultiSheetTab extends CachedComponent {
);
}

handleError = (error) => {
const { tab } = this.props;

This comment has been minimized.

Copy link
@nikku

nikku Sep 16, 2018

Member

Please use either destructuring for all members of props or none.


this.props.onError(tab, error);
}

handleContextMenu = (event, context) => {

const {
Expand Down Expand Up @@ -216,7 +222,8 @@ class MultiSheetTab extends CachedComponent {
activeSheet={ activeSheet }
onSheetsChanged={ this.sheetsChanged }
onContextMenu={ this.handleContextMenu }
onChanged={ this.handleChanged } />
onChanged={ this.handleChanged }
onError={ this.handleError } />
</TabContainer>

<TabLinks
Expand Down
26 changes: 14 additions & 12 deletions client/src/app/tabs/bpmn/BpmnEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ export class BpmnEditor extends CachedComponent {

console.warn('element template errors', errors);

// TODO(nikku): handle element template errors
errors.forEach(error => {
this.props.onError(error);
});
}

handleError = (event) => {
Expand All @@ -170,7 +172,7 @@ export class BpmnEditor extends CachedComponent {

console.warn('modeling error', error);

// TODO(nikku): handle modeling error
this.props.onError(error);
}

updateState = (event) => {
Expand Down Expand Up @@ -245,6 +247,10 @@ export class BpmnEditor extends CachedComponent {
// TODO(nikku): handle errors
// TODO(nikku): apply default element templates to initial diagram
modeler.importXML(xml, (err) => {
if (err) {
this.props.onError(err);
}

this.setState({
loading: false
});
Expand All @@ -265,6 +271,8 @@ export class BpmnEditor extends CachedComponent {
modeler.lastXML = xml;

if (err) {
this.props.onError(err);

return reject(err);
}

Expand All @@ -284,13 +292,17 @@ export class BpmnEditor extends CachedComponent {
let contents;

if (err) {
this.props.onError(err);

reject(err);

This comment has been minimized.

Copy link
@nikku

nikku Sep 16, 2018

Member

Return statement missing? return reject(err)?

Test coverage missing?

This comment has been minimized.

Copy link
@philippfromme

philippfromme Sep 17, 2018

Author Contributor

Ooops. 😨

}

if (type !== 'svg') {
try {
contents = generateImage(type, svg);
} catch (err) {
this.props.onError(err);

return reject(err);
}
} else {
Expand All @@ -316,16 +328,6 @@ export class BpmnEditor extends CachedComponent {
modeler.get('editorActions').trigger(action, context);
}

saveDiagram = () => {
const {
modeler
} = this.getCached();

modeler.saveXML((err, result) => {
console.log(result);
});
}

handleSetColor = (fill, stroke) => {
this.triggerAction('setColor', {
fill,
Expand Down
23 changes: 10 additions & 13 deletions client/src/app/tabs/cmmn/CmmnEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export class CmmnEditor extends CachedComponent {

console.warn('modeling error', error);

// TODO(nikku): handle modeling error
this.props.onError(error);
}

updateState = (event) => {
Expand Down Expand Up @@ -174,9 +174,10 @@ export class CmmnEditor extends CachedComponent {

modeler.lastXML = xml;

// TODO(nikku): handle errors
modeler.importXML(xml, function(err) {

if (err) {
this.props.onError(err);
}
});
}
}
Expand All @@ -194,6 +195,8 @@ export class CmmnEditor extends CachedComponent {
modeler.lastXML = xml;

if (err) {
this.props.onError(err);

return reject(err);
}

Expand All @@ -213,13 +216,17 @@ export class CmmnEditor extends CachedComponent {
let contents;

if (err) {
this.props.onError(err);

reject(err);

This comment has been minimized.

Copy link
@nikku

nikku Sep 16, 2018

Member

Return statement missing? Test coverage missing?

This comment has been minimized.

Copy link
@philippfromme

philippfromme Sep 17, 2018

Author Contributor

Ooops, I did it again... 💃

}

if (type !== 'svg') {
try {
contents = generateImage(type, svg);
} catch (err) {
this.props.onError(err);

return reject(err);
}
} else {
Expand All @@ -245,16 +252,6 @@ export class CmmnEditor extends CachedComponent {
modeler.get('editorActions').trigger(action, context);
}

saveDiagram = () => {
const {
modeler
} = this.getCached();

modeler.saveXML((err, result) => {
console.log(result);
});
}

handleSetColor = (fill, stroke) => {
this.triggerAction('setColor', {
fill,
Expand Down
18 changes: 18 additions & 0 deletions client/src/app/tabs/dmn/DmnEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ class DmnEditor extends CachedComponent {
modeler[fn]('views.changed', this.viewsChanged);

modeler[fn]('view.contentChanged', this.viewContentChanged);

modeler[fn]('error', this.handleError);
}

checkDirty = () => {
Expand Down Expand Up @@ -154,6 +156,8 @@ class DmnEditor extends CachedComponent {
if (error) {
console.error('imported with error', error);

this.props.onError(error);

return;
}

Expand Down Expand Up @@ -284,6 +288,16 @@ class DmnEditor extends CachedComponent {
this.setState(newState);
}

handleError = (event) => {
const {
error
} = event;

console.warn('modeling error', error);

this.props.onError(error);
}

checkImport = () => {
const {
modeler
Expand Down Expand Up @@ -353,6 +367,8 @@ class DmnEditor extends CachedComponent {
modeler.lastXML = xml;

if (err) {
this.props.onError(err);

return reject(err);
}

Expand Down Expand Up @@ -381,6 +397,8 @@ class DmnEditor extends CachedComponent {
try {
contents = generateImage(type, svg);
} catch (err) {
this.props.onError(err);

return reject(err);
}
} else {
Expand Down

0 comments on commit 5f836d7

Please sign in to comment.