From c8100804cddbb379b860339b7db82cdd67813470 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Feb 23 2017 08:21:21 +0000 Subject: Implement review suggestions --- diff --git a/hubs/tests/test_fedora_hubs_flask_api.py b/hubs/tests/test_fedora_hubs_flask_api.py index 812159a..b704c1d 100644 --- a/hubs/tests/test_fedora_hubs_flask_api.py +++ b/hubs/tests/test_fedora_hubs_flask_api.py @@ -264,7 +264,6 @@ class HubsAPITest(hubs.tests.APPTest): self.assertEqual( json.loads(result.get_data(as_text=True)), {"status": "ADDED"}) - #self.assertEqual(urlparse(result.location).path, "/ralph/edit") result = self.app.get('/ralph/edit') self.assertEqual(result.status_code, 200) page_html = result.get_data(as_text=True) diff --git a/hubs/views/utils.py b/hubs/views/utils.py index 2aecbb8..31a78d8 100644 --- a/hubs/views/utils.py +++ b/hubs/views/utils.py @@ -4,11 +4,15 @@ import datetime import flask import functools import hubs.models +import logging from six.moves.urllib import parse as urlparse from sqlalchemy.orm.exc import NoResultFound +log = logging.getLogger(__name__) + + def get_hub(name, session=None): """ Utility shorthand to get a hub and 404 if not found. """ if session is None: @@ -44,7 +48,13 @@ class WidgetConfigError(Exception): def create_widget_instance(hub, widget, position): - """View helper to instanciate a widget on a hub.""" + """View helper to instanciate a widget on a hub. + + Arguments: + hub (hubs.models.Hub): Instance of the hub to create the widget in. + widget (hubs.widgets.base.Widget): Widget to instanciate. + position (str): either ``left`` or ``right``. + """ widget_instance = hubs.models.Widget( hub=hub, plugin=widget.name, index=-1, left=(position == 'left')) @@ -59,6 +69,12 @@ def create_widget_instance(hub, widget, position): def configure_widget_instance(widget_instance): + """Configure a widget instance using the request form data. + + Arguments: + widget_instance (hubs.models.Widget): Instance of the widget that is to + be configured. + """ config = {} for param in widget_instance.module.get_parameters(): val = flask.request.form.get(param.name) @@ -67,7 +83,7 @@ def configure_widget_instance(widget_instance): 'You must provide a value for: %s' % param.name) try: config[param.name] = param.validator.from_string(val) - except Exception as err: + except ValueError as err: raise WidgetConfigError('Invalid data provided, error: %s' % err) # Updating in-place is not supported, it's a class property. cur_config = widget_instance.config or {} @@ -79,6 +95,10 @@ def configure_widget_instance(widget_instance): try: flask.g.db.commit() except Exception as err: + log.warn( + "Could not save the configuration of widget " + "%s to the database: %s.", + widget_instance.idx, err) raise WidgetConfigError( 'Could not save the configuration to the database ' 'if the error persists, please warn an admin') @@ -115,3 +135,12 @@ def is_safe_url(target): urlparse.urljoin(flask.request.host_url, target)) return test_url.scheme in ('http', 'https') and \ ref_url.netloc == test_url.netloc + + +def get_position(): + # Use request.values instead of request.form to allow the position to be + # passed in the query string. + position = flask.request.values.get('position', '') + if position not in ['right', 'left']: + flask.abort(400, 'Invalid position provided') + return position diff --git a/hubs/views/widget.py b/hubs/views/widget.py index 637ec25..1d21e17 100644 --- a/hubs/views/widget.py +++ b/hubs/views/widget.py @@ -1,6 +1,5 @@ from __future__ import unicode_literals, absolute_import -import datetime import flask from hubs.app import app @@ -9,15 +8,14 @@ from pkg_resources import resource_isdir from .utils import ( create_widget_instance, configure_widget_instance, get_hub, get_widget_instance, login_required, WidgetConfigError, + get_position, ) @app.route('//add', methods=['GET', 'POST']) def hub_add_widget(name): hub = get_hub(name) - position = flask.request.values.get('position', '') - if position not in ['right', 'left']: - flask.abort(400, 'Invalid position provided') + position = get_position() widgets = [ widget for widget in registry @@ -49,11 +47,7 @@ def hub_add_widget(name): @app.route('//add/', methods=['GET', 'POST']) def widget_add(hub, widget): hub = get_hub(hub) - # Use request.values instead of request.form to allow the position to be - # passed in the query string. - position = flask.request.values.get('position', '') - if position not in ['right', 'left']: - flask.abort(400, 'Invalid position provided') + position = get_position() try: widget = registry[widget] except KeyError: diff --git a/hubs/widgets/base.py b/hubs/widgets/base.py index 8ae225a..c2a8dca 100644 --- a/hubs/widgets/base.py +++ b/hubs/widgets/base.py @@ -209,6 +209,9 @@ class Widget(object): position (str): the position where the widget should be added. It must be either ``left`` or ``right``, and must be in the supported positions for this widget (see :py:attr:`position`). + + Returns: + The URL to add this widget, as a string. """ return "%s?position=%s" % ( flask.url_for('widget_add', hub=hub.name, widget=self.name), @@ -223,6 +226,9 @@ class Widget(object): hub (hubs.models.Hub): The hub instance in the database. widget_instance (hubs.models.Widget): The widget instance in the database that must be configured. + + Returns: + The URL to edit this widget, as a string. """ return flask.url_for( 'widget_edit', hub=hub.name, idx=widget_instance.idx)