From a5a1e4013542c64ebdbe7a01e43a773c7a8d3f21 Mon Sep 17 00:00:00 2001 From: Gabriel Hurley Date: Tue, 12 Jun 2012 19:30:45 -0700 Subject: [PATCH] Improved message handling. * Login page no longer displays user messages inappropriately. Instead, login errors are handled as form errors. Fixes bug 1012467. * Messages triggered during an AJAX call are no longer added to the standard message queue (which causes them to appear on subsequent unrelated requests). Instead, they are encoded and sent back with the AJAX response, allowing them to be displayed to the user client-side when they are relevant. Fixes bug 1008799. * Adds the last couple "compress" tags to the _scripts.html template to completely implement blueprint asset-compression. Change-Id: I967f32b44603ded7ec95bd0b86e7d997c6a8b352 --- horizon/exceptions.py | 2 +- horizon/forms/base.py | 10 +++ horizon/messages.py | 75 +++++++++++++++++++ horizon/middleware.py | 13 +++- horizon/static/horizon/js/horizon.js | 13 +++- horizon/static/horizon/js/modals.js | 6 +- horizon/templates/horizon/auth/_login.html | 1 - horizon/tests/auth_tests.py | 5 +- horizon/tests/message_tests.py | 37 +++++++++ horizon/views/auth_forms.py | 40 +++++----- .../static/dashboard/less/horizon.less | 21 ++++++ openstack_dashboard/templates/_scripts.html | 6 +- .../templates/_stylesheets.html | 1 - 13 files changed, 203 insertions(+), 27 deletions(-) create mode 100644 horizon/messages.py create mode 100644 horizon/tests/message_tests.py diff --git a/horizon/exceptions.py b/horizon/exceptions.py index 01188277f..3414107da 100644 --- a/horizon/exceptions.py +++ b/horizon/exceptions.py @@ -23,7 +23,6 @@ import os import sys from django.conf import settings -from django.contrib import messages from django.http import HttpRequest from django.utils import termcolors from django.utils.translation import ugettext as _ @@ -34,6 +33,7 @@ from glanceclient.common import exceptions as glanceclient from keystoneclient import exceptions as keystoneclient from novaclient import exceptions as novaclient +from horizon import messages LOG = logging.getLogger(__name__) PALETTE = termcolors.PALETTES[termcolors.DEFAULT_PALETTE] diff --git a/horizon/forms/base.py b/horizon/forms/base.py index faec9af94..5b71284a2 100644 --- a/horizon/forms/base.py +++ b/horizon/forms/base.py @@ -22,6 +22,7 @@ from datetime import date import logging from django import forms +from django.forms.forms import NON_FIELD_ERRORS from django.core.urlresolvers import reverse from django.utils import dates @@ -55,6 +56,15 @@ class SelfHandlingForm(forms.Form): kwargs['initial'] = initial super(SelfHandlingForm, self).__init__(*args, **kwargs) + def api_error(self, message): + """ + Adds an error to the form's error dictionary after validation + based on problems reported via the API. This is useful when you + wish for API errors to appear as errors on the form rather than + using the messages framework. + """ + self._errors[NON_FIELD_ERRORS] = self.error_class([message]) + def get_success_url(self, request=None): """ Returns the URL to redirect to after a successful handling. diff --git a/horizon/messages.py b/horizon/messages.py new file mode 100644 index 000000000..906d3d052 --- /dev/null +++ b/horizon/messages.py @@ -0,0 +1,75 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 +# +# Copyright 2012 Nebula, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +""" +Drop-in replacement for django.contrib.messages which handles Horizon's +messaging needs (e.g. AJAX communication, etc.). +""" + +from django.contrib import messages as _messages +from django.contrib.messages import constants + + +def add_message(request, level, message, extra_tags='', fail_silently=False): + """ + Attempts to add a message to the request using the 'messages' app. + """ + if request.is_ajax(): + tag = constants.DEFAULT_TAGS[level] + request.horizon['async_messages'].append([tag, message]) + else: + return _messages.add_message(request, level, message, + extra_tags, fail_silently) + + +def debug(request, message, extra_tags='', fail_silently=False): + """ + Adds a message with the ``DEBUG`` level. + """ + add_message(request, constants.DEBUG, message, extra_tags=extra_tags, + fail_silently=fail_silently) + + +def info(request, message, extra_tags='', fail_silently=False): + """ + Adds a message with the ``INFO`` level. + """ + add_message(request, constants.INFO, message, extra_tags=extra_tags, + fail_silently=fail_silently) + + +def success(request, message, extra_tags='', fail_silently=False): + """ + Adds a message with the ``SUCCESS`` level. + """ + add_message(request, constants.SUCCESS, message, extra_tags=extra_tags, + fail_silently=fail_silently) + + +def warning(request, message, extra_tags='', fail_silently=False): + """ + Adds a message with the ``WARNING`` level. + """ + add_message(request, constants.WARNING, message, extra_tags=extra_tags, + fail_silently=fail_silently) + + +def error(request, message, extra_tags='', fail_silently=False): + """ + Adds a message with the ``ERROR`` level. + """ + add_message(request, constants.ERROR, message, extra_tags=extra_tags, + fail_silently=fail_silently) diff --git a/horizon/middleware.py b/horizon/middleware.py index f141ff362..e755cee64 100644 --- a/horizon/middleware.py +++ b/horizon/middleware.py @@ -33,6 +33,7 @@ from django.utils.encoding import iri_to_uri from horizon import exceptions from horizon import users +from horizon.openstack.common import jsonutils LOG = logging.getLogger(__name__) @@ -59,7 +60,9 @@ class HorizonMiddleware(object): request.__class__.user_logout = user_logout request.__class__.user = users.LazyUser() - request.horizon = {'dashboard': None, 'panel': None} + request.horizon = {'dashboard': None, + 'panel': None, + 'async_messages': []} def process_exception(self, request, exception): """ @@ -101,4 +104,12 @@ class HorizonMiddleware(object): redirect_response = http.HttpResponse() redirect_response['X-Horizon-Location'] = response['location'] return redirect_response + if request.horizon['async_messages']: + messages = request.horizon['async_messages'] + # TODO(gabriel): When we have an async connection to the + # client (e.g. websockets) this should be pushed to the + # socket queue rather than being sent via a header. + # The header method has notable drawbacks (length limits, + # etc.) and is not meant as a long-term solution. + response['X-Horizon-Messages'] = jsonutils.dumps(messages) return response diff --git a/horizon/static/horizon/js/horizon.js b/horizon/static/horizon/js/horizon.js index 2a54b9d55..af4271f50 100644 --- a/horizon/static/horizon/js/horizon.js +++ b/horizon/static/horizon/js/horizon.js @@ -23,6 +23,14 @@ var Horizon = function() { // Load client-side template fragments and compile them. horizon.templates.compile_templates(); + // Bind AJAX message handling. + $("body").ajaxComplete(function(event, request, settings){ + var message_array = $.parseJSON(horizon.ajax.get_messages(request)); + $(message_array).each(function (index, item) { + horizon.alert(item[0], item[1]); + }); + }); + // Bind event handlers to confirm dangerous actions. $("body").on("click", "form button.btn-danger", function (evt) { horizon.datatables.confirm(this); @@ -323,7 +331,7 @@ var Horizon = function() { params = {"type": type, "type_capitalized": horizon.utils.capitalize(type), "message": message}; - return $(template.render(params)).prependTo("#main_content .messages"); + return $(template.render(params)).hide().prependTo("#main_content .messages").fadeIn(100); }; horizon.clearErrorMessages = function() { @@ -350,6 +358,9 @@ var Horizon = function() { // This will be our jQuery queue container. _queue: [], _active: [], + get_messages: function (request) { + return request.getResponseHeader("X-Horizon-Messages"); + }, // Function to add a new call to the queue. queue: function(opts) { var complete = opts.complete, diff --git a/horizon/static/horizon/js/modals.js b/horizon/static/horizon/js/modals.js index 604956fe9..13a70cbe1 100644 --- a/horizon/static/horizon/js/modals.js +++ b/horizon/static/horizon/js/modals.js @@ -127,8 +127,10 @@ horizon.addInitFunction(function() { } } else { - // Generic error handler. Really generic. - horizon.alert("error", "An error occurred. Please try again."); + if (!horizon.ajax.get_messages(jqXHR)) { + // Generic error handler. Really generic. + horizon.alert("error", "An error occurred. Please try again."); + } } }, success: horizon.modals.success diff --git a/horizon/templates/horizon/auth/_login.html b/horizon/templates/horizon/auth/_login.html index a73a926ec..6e09c8f61 100644 --- a/horizon/templates/horizon/auth/_login.html +++ b/horizon/templates/horizon/auth/_login.html @@ -7,7 +7,6 @@ {% block form_action %}{% url horizon:auth_login %}{% endblock %} {% block modal-body %} - {% include "horizon/_messages.html" %}
{% if next %}{% endif %} {% include "horizon/common/_form_fields.html" %} diff --git a/horizon/tests/auth_tests.py b/horizon/tests/auth_tests.py index 4c1ed3903..87132ab66 100644 --- a/horizon/tests/auth_tests.py +++ b/horizon/tests/auth_tests.py @@ -149,8 +149,11 @@ class AuthViewTests(test.TestCase): res = self.client.post(reverse('horizon:auth_login'), form_data, follow=True) - self.assertTemplateUsed(res, 'horizon/auth/login.html') + # Verify that API error messages are rendered, but not using the + # messages framework. + self.assertContains(res, "Invalid user name or password.") + self.assertNotContains(res, 'class="messages"') def test_login_exception(self): self.mox.StubOutWithMock(api, 'token_create') diff --git a/horizon/tests/message_tests.py b/horizon/tests/message_tests.py new file mode 100644 index 000000000..a314813eb --- /dev/null +++ b/horizon/tests/message_tests.py @@ -0,0 +1,37 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2012 Nebula, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from django import http + +from horizon import messages +from horizon import middleware +from horizon import test +from horizon.openstack.common import jsonutils + + +class MessageTests(test.TestCase): + def test_middleware_header(self): + req = self.request + expected = ["error", "Giant ants are attacking San Francisco!"] + self.assertTrue("async_messages" in req.horizon) + self.assertItemsEqual(req.horizon['async_messages'], []) + req.META['HTTP_X_REQUESTED_WITH'] = 'XMLHttpRequest' + messages.error(req, expected[1]) + self.assertItemsEqual(req.horizon['async_messages'], [expected]) + res = http.HttpResponse() + res = middleware.HorizonMiddleware().process_response(req, res) + self.assertEqual(res['X-Horizon-Messages'], + jsonutils.dumps([expected])) diff --git a/horizon/views/auth_forms.py b/horizon/views/auth_forms.py index 8beb832f4..f98dce50a 100644 --- a/horizon/views/auth_forms.py +++ b/horizon/views/auth_forms.py @@ -26,7 +26,6 @@ import logging from django import shortcuts from django.conf import settings -from django.contrib import messages from django.contrib.auth import REDIRECT_FIELD_NAME from django.utils.translation import ugettext as _ from django.views.decorators.debug import sensitive_variables @@ -80,6 +79,14 @@ class Login(forms.SelfHandlingForm): @sensitive_variables("data") def handle(self, request, data): + """ Process the user's login via Keystone. + + Note: We don't use the messages framework here (including messages + created by ``exceptions.handle`` beause they will not be displayed + on the login page (intentionally). Instead we add all error messages + to the form's ``non_field_errors``, causing them to appear as + errors on the form itself. + """ if 'user_name' in request.session: if request.session['user_name'] != data['username']: # To avoid reusing another user's session, create a @@ -110,9 +117,8 @@ class Login(forms.SelfHandlingForm): tenants = api.tenant_list_for_token(request, token.id) except: msg = _('Unable to authenticate for that project.') - exceptions.handle(request, - message=msg, - escalate=True) + exceptions.handle(request, ignore=True) + return self.api_error(msg) _set_session_data(request, token) user = users.get_user_from_request(request) redirect = redirect_to or base.Horizon.get_user_home(user) @@ -125,17 +131,18 @@ class Login(forms.SelfHandlingForm): data['username'], data['password']) except keystone_exceptions.Unauthorized: - exceptions.handle(request, - _('Invalid user name or password.')) + msg = _('Invalid user name or password.') + exceptions.handle(request, ignore=True) + return self.api_error(msg) except: # If we get here we don't want to show a stack trace to the # user. However, if we fail here, there may be bad session # data that's been cached already. request.user_logout() - exceptions.handle(request, - message=_("An error occurred authenticating." - " Please try again later."), - escalate=True) + msg = _("An error occurred authenticating. " + "Please try again later.") + exceptions.handle(request, ignore=True) + return self.api_error(msg) # Unscoped token request.session['unscoped_token'] = unscoped_token.id @@ -146,16 +153,13 @@ class Login(forms.SelfHandlingForm): try: tenants = api.tenant_list_for_token(request, unscoped_token.id) except: - exceptions.handle(request) + exceptions.handle(request, ignore=True) tenants = [] # Abort if there are no valid tenants for this user if not tenants: - messages.error(request, - _('You are not authorized for any projects.') % - {"user": data['username']}, - extra_tags="login") - return + msg = _('You are not authorized for any projects.') + return self.api_error(msg) # Create a token. # NOTE(gabriel): Keystone can return tenants that you're @@ -175,8 +179,8 @@ class Login(forms.SelfHandlingForm): exceptions.handle(request, ignore=True) token = None if token is None: - raise exceptions.NotAuthorized( - _("You are not authorized for any available projects.")) + msg = _("You are not authorized for any available projects.") + return self.api_error(msg) _set_session_data(request, token) user = users.get_user_from_request(request) diff --git a/openstack_dashboard/static/dashboard/less/horizon.less b/openstack_dashboard/static/dashboard/less/horizon.less index 1320389cf..2acf300c9 100644 --- a/openstack_dashboard/static/dashboard/less/horizon.less +++ b/openstack_dashboard/static/dashboard/less/horizon.less @@ -1,3 +1,5 @@ +@import "../../bootstrap/less/bootstrap.less"; + /* new clearfix */ .clearfix:after { visibility: hidden; @@ -436,6 +438,25 @@ table form { width: 1px; } +.messages { + position: absolute; + z-index: 9999; + top: 20px; + right: 20px; + width: 300px; + .alert-block { + -webkit-box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3); + -moz-box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3); + box-shadow: 0 3px 7px rgba(0, 0, 0, 0.3); + &.alert-error { + border: 1px solid @red; + } + &.alert-success { + border: 1px solid @green; + } + } +} + .alert-block .alert-actions { margin-top: -23px; margin-right: -23px; diff --git a/openstack_dashboard/templates/_scripts.html b/openstack_dashboard/templates/_scripts.html index 8443cfa62..7589a1574 100644 --- a/openstack_dashboard/templates/_scripts.html +++ b/openstack_dashboard/templates/_scripts.html @@ -1,3 +1,6 @@ +{% load compress %} + +{% compress js %} {% comment %} jQuery and Plugins {% endcomment %} @@ -25,6 +28,7 @@ +{% endcompress %} -{% comment %} Client-side Templates {% endcomment %} +{% comment %} Client-side Templates (These should *not* be inside the "compress" tag.) {% endcomment %} {% include "horizon/client_side/templates.html" %} diff --git a/openstack_dashboard/templates/_stylesheets.html b/openstack_dashboard/templates/_stylesheets.html index e082819f1..66c40a6fc 100644 --- a/openstack_dashboard/templates/_stylesheets.html +++ b/openstack_dashboard/templates/_stylesheets.html @@ -1,7 +1,6 @@ {% load compress %} {% compress css %} - {% endcompress %}