From a860100156108253b27a7751106ee7044256d6c2 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Nov 02 2017 11:16:37 +0000 Subject: Handle widget configuration errors better --- diff --git a/hubs/static/client/app/components/AddWidget/AddWidgetDialog.js b/hubs/static/client/app/components/AddWidget/AddWidgetDialog.js index efce753..83247e2 100644 --- a/hubs/static/client/app/components/AddWidget/AddWidgetDialog.js +++ b/hubs/static/client/app/components/AddWidget/AddWidgetDialog.js @@ -76,8 +76,19 @@ class AddWidgetDialog extends React.Component { this.state.selectedWidget.name, this.state.widgetConfig, this.props.position) + ).then( + result => { + this.props.handleDialogCloseClicked(); + }, + error => { + this.setState((prevState, props) => ({ + selectedWidget: { + ...prevState.selectedWidget, + configError: error.message, + } + })); + } ); - this.props.handleDialogCloseClicked(e); } render() { diff --git a/hubs/static/client/app/components/SimpleWidgetConfig.js b/hubs/static/client/app/components/SimpleWidgetConfig.js index 24d8e24..8c88482 100644 --- a/hubs/static/client/app/components/SimpleWidgetConfig.js +++ b/hubs/static/client/app/components/SimpleWidgetConfig.js @@ -39,6 +39,11 @@ export default class SimpleWidgetConfig extends React.Component { : fields } + { this.props.widget.configError && +
+ { this.props.widget.configError } +
+ } ); } diff --git a/hubs/static/client/app/core/actions/widget.js b/hubs/static/client/app/core/actions/widget.js index e7784a4..c9da158 100644 --- a/hubs/static/client/app/core/actions/widget.js +++ b/hubs/static/client/app/core/actions/widget.js @@ -71,8 +71,7 @@ export function saveConfig(widgetId, config) { return dispatch(putConfigSucceeded(widgetId)); }, error => { - dispatch(addFlashMessage(error.message, "error")); - return putConfigFailure(error.message); + return dispatch(putConfigFailure(widgetId, error.message)); }) } } @@ -137,8 +136,8 @@ export function addWidget(name, config, position) { return dispatch(fetchWidgets()); }, error => { - dispatch(addFlashMessage(error.message, "error")); - return widgetAddFailure(error.message); + dispatch(widgetAddFailure(error.message)); + throw error; }) } } diff --git a/hubs/static/client/app/core/reducers/widget.js b/hubs/static/client/app/core/reducers/widget.js index 524e024..1277ba8 100644 --- a/hubs/static/client/app/core/reducers/widget.js +++ b/hubs/static/client/app/core/reducers/widget.js @@ -22,6 +22,7 @@ export default function singleWidgetReducer(state, action) { ...state, // Optimism failed... ;-( config: state.old.config, + configError: action.message, }; default: return state diff --git a/hubs/tests/test_widget_validators.py b/hubs/tests/test_widget_validators.py index 96d887f..77a2a54 100644 --- a/hubs/tests/test_widget_validators.py +++ b/hubs/tests/test_widget_validators.py @@ -17,7 +17,9 @@ class ValidatorsTest(APPTest): def test_integer(self): self.assertEqual(validators.Integer("1"), 1) - self.assertRaises(ValueError, validators.Integer, "text") + with self.assertRaises(ValueError) as cm: + validators.Integer("text") + self.assertEqual(str(cm.exception), "text is not an integer") @unittest.skip("Not implemented yet") def test_link(self): diff --git a/hubs/tests/views/test_api_hub_widget.py b/hubs/tests/views/test_api_hub_widget.py index 5684d29..c5e5689 100644 --- a/hubs/tests/views/test_api_hub_widget.py +++ b/hubs/tests/views/test_api_hub_widget.py @@ -177,6 +177,41 @@ class TestAPIHubWidgets(APPTest): Widget.plugin == "about", ).count(), 2) + def test_post_invalid_config(self): + self.assertEqual( + Widget.query.join(Hub).filter( + Hub.name == "ralph", + Widget.plugin == "meetings", + ).count(), 1) + data = { + "name": "meetings", + "config": { + 'calendar': 'infra', + 'n_meetings': 'NOT-A-NUMBER', + }, + 'position': 'right', + } + user = FakeAuthorization('ralph') + with auth_set(app, user): + result = self.app.post( + '/api/hubs/ralph/widgets/', + content_type="application/json", + data=json.dumps(data)) + self.assertEqual(result.status_code, 200) + self.assertEqual( + json.loads(result.get_data(as_text=True)), + { + "status": "ERROR", + "message": ( + "Invalid data provided: NOT-A-NUMBER is not an integer" + ), + }) + self.assertEqual( + Widget.query.join(Hub).filter( + Hub.name == "ralph", + Widget.plugin == "meetings", + ).count(), 1) + class TestAPIHubWidget(APPTest): diff --git a/hubs/utils/views.py b/hubs/utils/views.py index 0fe1e2b..d95d8d7 100644 --- a/hubs/utils/views.py +++ b/hubs/utils/views.py @@ -97,12 +97,7 @@ def create_widget_instance(hub, widget, position, config=None): left=(position == 'left')) flask.g.db.add(widget_instance) flask.g.db.flush() # will populate Widget.config - try: - configure_widget_instance(widget_instance, config) - except WidgetConfigError as e: - flask.flash(e.args[0], "error") - else: - flask.flash("The widget has been added.") + configure_widget_instance(widget_instance, config) # reorder widgets reorder_widgets(hub, widget_instance.left) return widget_instance @@ -128,7 +123,7 @@ def configure_widget_instance(widget_instance, widget_config): try: config = widget_instance.module.validate_parameters(values) except ValueError as err: - raise WidgetConfigError('Invalid data provided, error: %s' % err) + raise WidgetConfigError('Invalid data provided: %s' % err) # Updating in-place is not supported, it's a class property. cur_config = widget_instance.config or {} cur_config = cur_config.copy() diff --git a/hubs/views/api/hub_widget.py b/hubs/views/api/hub_widget.py index 3ef777d..c818cc4 100644 --- a/hubs/views/api/hub_widget.py +++ b/hubs/views/api/hub_widget.py @@ -50,7 +50,11 @@ def api_hub_widgets(hub): if widget_name not in available_widgets: flask.abort(400, 'Unknown widget') widget = registry[widget_name] - create_widget_instance(hub, widget, position, widget_config) + try: + create_widget_instance(hub, widget, position, widget_config) + except WidgetConfigError as e: + flask.g.db.rollback() + return flask.jsonify({"status": "ERROR", "message": e.args[0]}) flask.g.db.commit() return flask.jsonify({"status": "OK"}) user_can_config = hub.allows(user, "config") diff --git a/hubs/widgets/validators.py b/hubs/widgets/validators.py index 47f185a..ef19ad0 100644 --- a/hubs/widgets/validators.py +++ b/hubs/widgets/validators.py @@ -41,7 +41,10 @@ def Text(value): def Integer(value): """Raises an error if the value can't be converted to an integer.""" - return int(value) + try: + return int(value) + except ValueError: + raise ValueError("{} is not an integer".format(value)) def Link(value):