Skip to content

Commit

Permalink
Fixes #1047: better error handling for layer loading (#1084)
Browse files Browse the repository at this point in the history
  • Loading branch information
mbarto committed Oct 5, 2016
1 parent d535cf9 commit d121962
Show file tree
Hide file tree
Showing 12 changed files with 93 additions and 15 deletions.
11 changes: 11 additions & 0 deletions web/client/actions/__tests__/layers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var {
CHANGE_LAYER_PROPERTIES,
LAYER_LOADING,
LAYER_LOAD,
LAYER_ERROR,
ADD_LAYER,
SHOW_SETTINGS,
HIDE_SETTINGS,
Expand All @@ -26,6 +27,7 @@ var {
updateNode,
layerLoading,
layerLoad,
layerError,
addLayer,
showSettings,
hideSettings,
Expand Down Expand Up @@ -107,6 +109,15 @@ describe('Test correctness of the layers actions', () => {
expect(retval.layerId).toBe(testVal);
});

it('a layer is not loaded with errors', () => {
const testVal = 'layer1';
const retval = layerError(testVal);

expect(retval).toExist();
expect(retval.type).toBe(LAYER_ERROR);
expect(retval.layerId).toBe(testVal);
});

it('add layer', () => {
const testVal = 'layer1';
const retval = addLayer(testVal);
Expand Down
12 changes: 10 additions & 2 deletions web/client/actions/layers.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const REMOVE_NODE = 'REMOVE_NODE';
const UPDATE_NODE = 'UPDATE_NODE';
const LAYER_LOADING = 'LAYER_LOADING';
const LAYER_LOAD = 'LAYER_LOAD';
const LAYER_ERROR = 'LAYER_ERROR';
const INVALID_LAYER = 'INVALID_LAYER';
const ADD_LAYER = 'ADD_LAYER';
const SHOW_SETTINGS = 'SHOW_SETTINGS';
Expand Down Expand Up @@ -116,6 +117,13 @@ function layerLoad(layerId) {
};
}

function layerError(layerId) {
return {
type: LAYER_ERROR,
layerId: layerId
};
}

function addLayer(layer) {
return {
type: ADD_LAYER,
Expand Down Expand Up @@ -209,9 +217,9 @@ function getLayerCapabilities(layer, options) {


module.exports = {changeLayerProperties, changeGroupProperties, toggleNode, sortNode, removeNode, invalidLayer,
updateNode, layerLoading, layerLoad, addLayer, showSettings, hideSettings, updateSettings,
updateNode, layerLoading, layerLoad, layerError, addLayer, showSettings, hideSettings, updateSettings,
getDescribeLayer, getLayerCapabilities,
CHANGE_LAYER_PROPERTIES, CHANGE_GROUP_PROPERTIES, TOGGLE_NODE, SORT_NODE,
REMOVE_NODE, UPDATE_NODE, LAYER_LOADING, LAYER_LOAD, ADD_LAYER,
REMOVE_NODE, UPDATE_NODE, LAYER_LOADING, LAYER_LOAD, LAYER_ERROR, ADD_LAYER,
SHOW_SETTINGS, HIDE_SETTINGS, UPDATE_SETTINGS, INVALID_LAYER
};
5 changes: 5 additions & 0 deletions web/client/components/TOC/DefaultLayer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ var DefaultLayer = React.createClass({
<VisibilityCheck checkType={this.props.visibilityCheckType} propertiesChangeHandler={this.props.propertiesChangeHandler}/>
<Title onClick={this.props.onToggle}/>
<InlineSpinner loading={this.props.node.loading}/>
<LayersTool key="loadingerror"
style={{"display": this.props.node.loadingError ? "block" : "none", color: "red", cursor: "default"}}
glyph="ban-circle"
tooltip="toc.loadingerror"
/>
{this.renderCollapsible()}
{this.renderTools()}
</Node>
Expand Down
6 changes: 3 additions & 3 deletions web/client/components/TOC/__tests__/DefaultLayer-test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('test DefaultLayer module component', () => {
expect(comp).toExist();
const domNode = ReactDOM.findDOMNode(comp);
expect(domNode).toExist();
const tool = ReactDOM.findDOMNode(TestUtils.scryRenderedDOMComponentsWithClass(comp, "glyphicon")[0]);
const tool = ReactDOM.findDOMNode(TestUtils.scryRenderedDOMComponentsWithClass(comp, "glyphicon")[1]);
expect(tool).toExist();
tool.click();
expect(spy.calls.length).toBe(1);
Expand All @@ -155,7 +155,7 @@ describe('test DefaultLayer module component', () => {
expect(comp).toExist();
const domNode = ReactDOM.findDOMNode(comp);
expect(domNode).toExist();
const tool = ReactDOM.findDOMNode(TestUtils.scryRenderedDOMComponentsWithClass(comp, "glyphicon")[0]);
const tool = ReactDOM.findDOMNode(TestUtils.scryRenderedDOMComponentsWithClass(comp, "glyphicon")[1]);
expect(tool).toExist();
tool.click();
expect(spy.calls.length).toBe(1);
Expand Down Expand Up @@ -235,7 +235,7 @@ describe('test DefaultLayer module component', () => {
expect(component).toExist();
// let's find the settings tool and click on it
const tool = ReactDOM.findDOMNode(
TestUtils.scryRenderedDOMComponentsWithClass(component, "glyphicon")[0]);
TestUtils.scryRenderedDOMComponentsWithClass(component, "glyphicon")[1]);
expect(tool).toExist();
tool.click();
// the onSettings method must have been invoked
Expand Down
21 changes: 15 additions & 6 deletions web/client/components/TOC/fragments/LayersTool.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,19 @@
*/

const React = require('react');
const {Glyphicon} = require('react-bootstrap');
const {Glyphicon, OverlayTrigger, Tooltip} = require('react-bootstrap');
const Message = require('../../I18N/Message');

require("./css/layertool.css");


const LayersTool = React.createClass({
propTypes: {
node: React.PropTypes.object,
onClick: React.PropTypes.func,
style: React.PropTypes.object,
glyph: React.PropTypes.string
glyph: React.PropTypes.string,
tooltip: React.PropTypes.tooltip
},
getDefaultProps() {
return {
Expand All @@ -23,10 +28,14 @@ const LayersTool = React.createClass({
};
},
render() {
return (
<Glyphicon className="toc-layer-tool" style={this.props.style}
glyph={this.props.glyph}
onClick={(options) => this.props.onClick(this.props.node, options || {})}/>);
const tool = (<Glyphicon className="toc-layer-tool" style={this.props.style}
glyph={this.props.glyph}
onClick={(options) => this.props.onClick(this.props.node, options || {})}/>);
return this.props.tooltip ? (
<OverlayTrigger placement="bottom" overlay={(<Tooltip><strong><Message msgId={this.props.tooltip}/></strong></Tooltip>)}>
{tool}
</OverlayTrigger>) : tool;

}
});

Expand Down
3 changes: 3 additions & 0 deletions web/client/components/map/leaflet/Map.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ let LeafletMap = React.createClass({
onMouseMove: React.PropTypes.func,
onLayerLoading: React.PropTypes.func,
onLayerLoad: React.PropTypes.func,
onLayerError: React.PropTypes.func,
resize: React.PropTypes.number,
measurement: React.PropTypes.object,
changeMeasurementState: React.PropTypes.func,
Expand All @@ -53,6 +54,7 @@ let LeafletMap = React.createClass({
projection: "EPSG:3857",
onLayerLoading: () => {},
onLayerLoad: () => {},
onLayerError: () => {},
resize: 0,
registerHooks: true,
style: {},
Expand Down Expand Up @@ -133,6 +135,7 @@ let LeafletMap = React.createClass({
}
event.layer.on('loading', (loadingEvent) => { this.props.onLayerLoading(loadingEvent.target.layerId); });
event.layer.on('load', (loadEvent) => { this.props.onLayerLoad(loadEvent.target.layerId); });
event.layer.on('tileerror', (errorEvent) => { this.props.onLayerError(errorEvent.target.layerId); });
}
});

Expand Down
3 changes: 2 additions & 1 deletion web/client/plugins/map/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
const React = require('react');

const {changeMapView, clickOnMap} = require('../../actions/map');
const {layerLoading, layerLoad, invalidLayer} = require('../../actions/layers');
const {layerLoading, layerLoad, layerError, invalidLayer} = require('../../actions/layers');
const {changeMousePosition} = require('../../actions/mousePosition');
const {changeMeasurementState} = require('../../actions/measurement');
const {changeLocateState, onLocateError} = require('../../actions/locate');
Expand All @@ -33,6 +33,7 @@ module.exports = (mapType, actions) => {
onMouseMove: changeMousePosition,
onLayerLoading: layerLoading,
onLayerLoad: layerLoad,
onLayerError: layerError,
onInvalidLayer: invalidLayer
}, actions), (stateProps, dispatchProps, ownProps) => {
return assign({}, ownProps, stateProps, assign({}, dispatchProps, {
Expand Down
26 changes: 26 additions & 0 deletions web/client/reducers/__tests__/layers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,32 @@ describe('Test the layers reducer', () => {
expect(state.flat.filter((layer) => layer.loading).length).toBe(0);
});

it('a layer load ends with error, loadingError flag is updated', () => {
const action1 = {
type: 'LAYER_ERROR',
layerId: "layer1"
};

const action2 = {
type: 'LAYER_ERROR',
layerId: "layer2"
};

var originalLoadingLayers = {flat: [{id: "layer1", name: "layer1", loading: true}, {id: "layer2", name: "layer2", loading: true}]};
var state = layers(originalLoadingLayers, action1);

expect(state.flat.filter((layer) => layer.name === 'layer1')[0].loading).toBe(false);
expect(state.flat.filter((layer) => layer.name === 'layer2')[0].loading).toBe(true);
expect(state.flat.filter((layer) => layer.loading).length).toBe(1);
expect(state.flat.filter((layer) => layer.loadingError).length).toBe(1);

state = layers(state, action2);
expect(state.flat.filter((layer) => layer.name === 'layer1')[0].loading).toBe(false);
expect(state.flat.filter((layer) => layer.name === 'layer2')[0].loading).toBe(false);
expect(state.flat.filter((layer) => layer.loading).length).toBe(0);
expect(state.flat.filter((layer) => layer.loadingError).length).toBe(2);
});

it('change group properties', () => {
let testAction = {
type: "CHANGE_GROUP_PROPERTIES",
Expand Down
12 changes: 9 additions & 3 deletions web/client/reducers/layers.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* LICENSE file in the root directory of this source tree.
*/

var {LAYER_LOADING, LAYER_LOAD, CHANGE_LAYER_PROPERTIES, CHANGE_GROUP_PROPERTIES,
var {LAYER_LOADING, LAYER_LOAD, LAYER_ERROR, CHANGE_LAYER_PROPERTIES, CHANGE_GROUP_PROPERTIES,
TOGGLE_NODE, SORT_NODE, REMOVE_NODE, UPDATE_NODE, ADD_LAYER,
SHOW_SETTINGS, HIDE_SETTINGS, UPDATE_SETTINGS, INVALID_LAYER
} = require('../actions/layers');
Expand Down Expand Up @@ -120,13 +120,19 @@ function layers(state = [], action) {
switch (action.type) {
case LAYER_LOADING: {
const newLayers = (state.flat || []).map((layer) => {
return layer.id === action.layerId ? assign({}, layer, {loading: true}) : layer;
return layer.id === action.layerId ? assign({}, layer, {loading: true, loadingError: false}) : layer;
});
return assign({}, state, {flat: newLayers});
}
case LAYER_LOAD: {
const newLayers = (state.flat || []).map((layer) => {
return layer.id === action.layerId ? assign({}, layer, {loading: false}) : layer;
return layer.id === action.layerId ? assign({}, layer, {loading: false, loadingError: false}) : layer;
});
return assign({}, state, {flat: newLayers});
}
case LAYER_ERROR: {
const newLayers = (state.flat || []).map((layer) => {
return layer.id === action.layerId ? assign({}, layer, {loading: false, loadingError: true}) : layer;
});
return assign({}, state, {flat: newLayers});
}
Expand Down
3 changes: 3 additions & 0 deletions web/client/translations/data.en-US
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@
"errorDefault" : "Network error"
}
},
"toc": {
"loadingerror": "The layer has not been loaded correctly"
},
"print":{
"paneltitle": "Print",
"layout": "Layout",
Expand Down
3 changes: 3 additions & 0 deletions web/client/translations/data.fr-FR
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@
"errorDefault" : "Une erreur est survenue sur le serveur"
}
},
"toc": {
"loadingerror": "La couche n'a pas été chargée correctement"
},
"print":{
"paneltitle": "Impression",
"layout": "Mise en page",
Expand Down
3 changes: 3 additions & 0 deletions web/client/translations/data.it-IT
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@
"errorDefault" : "Errore di rete"
}
},
"toc": {
"loadingerror": "Il livello non è stato caricato correttamente"
},
"print":{
"paneltitle": "Stampa",
"layout": "Layout",
Expand Down

0 comments on commit d121962

Please sign in to comment.