From a212ef399f78633d7627b633f733f8c525c30aa3 Mon Sep 17 00:00:00 2001 From: Vlad Okhrimenko Date: Fri, 22 Aug 2014 14:04:16 +0300 Subject: [PATCH] Uniquify horizon messages returned in a single response A network operation can fail because of different reasons - yet in many places just one error message is provided. Combined with too broad exception clause, and incorrect assumptions on the reasons of failure (e.g. Neutron service being unavailable causes all other sorts of errors like inability to Allocate IP or Associate it) this leads to multiple errors when just one would suffice. The fix aims to provide sensible error messages in case the network service is down. This includes returning only unique messages in a single response. Partial-Bug: #1301374 Change-Id: Id6c620ca51f7703f35c0c172e39fdf237fa42278 Co-Authored-By: Timur Sufiev --- horizon/messages.py | 38 +++++++++++++------ .../dashboards/admin/instances/tests.py | 5 ++- .../access_and_security/floating_ips/views.py | 5 +++ .../floating_ips/workflows.py | 6 ++- .../project/access_and_security/tabs.py | 11 ++++++ .../dashboards/project/instances/tests.py | 5 ++- 6 files changed, 55 insertions(+), 15 deletions(-) diff --git a/horizon/messages.py b/horizon/messages.py index c1b36adbd0..b0fde0c901 100644 --- a/horizon/messages.py +++ b/horizon/messages.py @@ -23,20 +23,34 @@ from django.utils.encoding import force_text from django.utils.safestring import SafeData # noqa +def horizon_message_already_queued(request, message): + _message = force_text(message) + if request.is_ajax(): + for tag, msg, extra in request.horizon['async_messages']: + if _message == msg: + return True + else: + for msg in _messages.get_messages(request)._queued_messages: + if msg.message == _message: + return True + return False + + 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] - # if message is marked as safe, pass "safe" tag as extra_tags so that - # client can skip HTML escape for the message when rendering - if isinstance(message, SafeData): - extra_tags = extra_tags + ' safe' - request.horizon['async_messages'].append([tag, - force_text(message), - extra_tags]) - else: - return _messages.add_message(request, level, message, - extra_tags, fail_silently) + if not horizon_message_already_queued(request, message): + if request.is_ajax(): + tag = constants.DEFAULT_TAGS[level] + # if message is marked as safe, pass "safe" tag as extra_tags so + # that client can skip HTML escape for the message when rendering + if isinstance(message, SafeData): + extra_tags = extra_tags + ' safe' + request.horizon['async_messages'].append([tag, + force_text(message), + extra_tags]) + else: + return _messages.add_message(request, level, message, + extra_tags, fail_silently) def debug(request, message, extra_tags='', fail_silently=False): diff --git a/openstack_dashboard/dashboards/admin/instances/tests.py b/openstack_dashboard/dashboards/admin/instances/tests.py index f19aec3f99..d040f7ae18 100644 --- a/openstack_dashboard/dashboards/admin/instances/tests.py +++ b/openstack_dashboard/dashboards/admin/instances/tests.py @@ -118,7 +118,10 @@ class InstanceViewTest(test.BaseAdminViewTests): res = self.client.get(INDEX_URL) instances = res.context['table'].data self.assertTemplateUsed(res, 'admin/instances/index.html') - self.assertMessageCount(res, error=len(servers)) + # Since error messages produced for each instance are identical, + # there will be only one error message for all instances + # (messages de-duplication). + self.assertMessageCount(res, error=1) self.assertItemsEqual(instances, servers) @test.create_stubs({api.nova: ('server_list',)}) diff --git a/openstack_dashboard/dashboards/project/access_and_security/floating_ips/views.py b/openstack_dashboard/dashboards/project/access_and_security/floating_ips/views.py index 5f5485d382..281d51031e 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/floating_ips/views.py +++ b/openstack_dashboard/dashboards/project/access_and_security/floating_ips/views.py @@ -24,6 +24,8 @@ Views for managing floating IPs. from django.core.urlresolvers import reverse_lazy from django.utils.translation import ugettext_lazy as _ +from neutronclient.common import exceptions as neutron_exc + from horizon import exceptions from horizon import forms from horizon import workflows @@ -60,6 +62,9 @@ class AllocateView(forms.ModalFormView): def get_initial(self): try: pools = api.network.floating_ip_pools_list(self.request) + except neutron_exc.ConnectionFailed: + pools = [] + exceptions.handle(self.request) except Exception: pools = [] exceptions.handle(self.request, diff --git a/openstack_dashboard/dashboards/project/access_and_security/floating_ips/workflows.py b/openstack_dashboard/dashboards/project/access_and_security/floating_ips/workflows.py index f4c3fe9c0b..eb1ab5359f 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/floating_ips/workflows.py +++ b/openstack_dashboard/dashboards/project/access_and_security/floating_ips/workflows.py @@ -15,6 +15,8 @@ from django.core.urlresolvers import reverse from django.utils.translation import ugettext_lazy as _ +from neutronclient.common import exceptions as neutron_exc + from horizon import exceptions from horizon import forms from horizon import workflows @@ -59,10 +61,12 @@ class AssociateIPAction(workflows.Action): def populate_ip_id_choices(self, request, context): ips = [] + redirect = reverse('horizon:project:access_and_security:index') try: ips = api.network.tenant_floating_ip_list(self.request) + except neutron_exc.ConnectionFailed: + exceptions.handle(self.request, redirect=redirect) except Exception: - redirect = reverse('horizon:project:access_and_security:index') exceptions.handle(self.request, _('Unable to retrieve floating IP addresses.'), redirect=redirect) diff --git a/openstack_dashboard/dashboards/project/access_and_security/tabs.py b/openstack_dashboard/dashboards/project/access_and_security/tabs.py index c9dd14037e..d524a8ac9e 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/tabs.py +++ b/openstack_dashboard/dashboards/project/access_and_security/tabs.py @@ -22,6 +22,8 @@ from django.utils.translation import ugettext_lazy as _ from horizon import exceptions from horizon import tabs +from neutronclient.common import exceptions as neutron_exc + from openstack_dashboard.api import keystone from openstack_dashboard.api import network from openstack_dashboard.api import nova @@ -46,6 +48,9 @@ class SecurityGroupsTab(tabs.TableTab): def get_security_groups_data(self): try: security_groups = network.security_group_list(self.request) + except neutron_exc.ConnectionFailed: + security_groups = [] + exceptions.handle(self.request) except Exception: security_groups = [] exceptions.handle(self.request, @@ -80,6 +85,9 @@ class FloatingIPsTab(tabs.TableTab): def get_floating_ips_data(self): try: floating_ips = network.tenant_floating_ip_list(self.request) + except neutron_exc.ConnectionFailed: + floating_ips = [] + exceptions.handle(self.request) except Exception: floating_ips = [] exceptions.handle(self.request, @@ -87,6 +95,9 @@ class FloatingIPsTab(tabs.TableTab): try: floating_ip_pools = network.floating_ip_pools_list(self.request) + except neutron_exc.ConnectionFailed: + floating_ip_pools = [] + exceptions.handle(self.request) except Exception: floating_ip_pools = [] exceptions.handle(self.request, diff --git a/openstack_dashboard/dashboards/project/instances/tests.py b/openstack_dashboard/dashboards/project/instances/tests.py index dfa3b8e2bf..2d08d6c22a 100644 --- a/openstack_dashboard/dashboards/project/instances/tests.py +++ b/openstack_dashboard/dashboards/project/instances/tests.py @@ -198,7 +198,10 @@ class InstanceTests(helpers.TestCase): instances = res.context['instances_table'].data self.assertTemplateUsed(res, 'project/instances/index.html') - self.assertMessageCount(res, error=len(servers)) + # Since error messages produced for each instance are identical, + # there will be only one error message for all instances + # (messages de-duplication) + self.assertMessageCount(res, error=1) self.assertItemsEqual(instances, self.servers.list()) @helpers.create_stubs({