Control: tag -1 + confirmed
Hello Thomas,
I spent some time on horizon this evening and I came up with this
set of patches. It fixes all regressions that I saw except one:
FAIL: test_update_project_when_default_role_does_not_exist
(openstack_dashboard.dashboards.admin.projects.tests.UpdateProjectWorkflowTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/home/rhertzog/tmp/django17/horizon/openstack_dashboard/test/helpers.py", line
83, in instance_stub_out
return fn(self, *args, **kwargs)
File
"/home/rhertzog/tmp/django17/horizon/openstack_dashboard/dashboards/admin/projects/tests.py",
line 1458, in test_update_project_when_default_role_does_not_exist
self.client.get(url)
AssertionError: NotFound not raised
I haven't understood this one and it's already late so I don't want to spend
more time on it.
You'll see that the patches will need some discussion with upstream because
at least in one case I'm not sure that my fix is correct, it might be that the
test data need to be fixed...
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Discover the Debian Administrator's Handbook:
→ http://debian-handbook.info/get/
>From b8e61396deb3447fd56afb9ece426d914158e1a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <[email protected]>
Date: Mon, 4 Aug 2014 21:46:37 +0200
Subject: [PATCH 1/5] Fix TEMPLATE_DIRS setting for Django 1.7 compatibility
---
horizon/test/settings.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/horizon/test/settings.py b/horizon/test/settings.py
index 1502b79..67049d9 100644
--- a/horizon/test/settings.py
+++ b/horizon/test/settings.py
@@ -88,7 +88,7 @@ STATIC_URL = '/static/'
MESSAGE_STORAGE = 'django.contrib.messages.storage.cookie.CookieStorage'
ROOT_URLCONF = 'horizon.test.urls'
-TEMPLATE_DIRS = (os.path.join(ROOT_PATH, 'tests', 'templates'))
+TEMPLATE_DIRS = (os.path.join(ROOT_PATH, 'tests', 'templates'), )
SITE_ID = 1
SITE_BRANDING = 'Horizon'
--
2.0.1
>From 06814a3ffff08781d092a86dcd39ceefbc314b64 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <[email protected]>
Date: Mon, 4 Aug 2014 21:47:20 +0200
Subject: [PATCH 2/5] Rename add_error methods to avoid conflict with Django's
1.7 add_error()
In Django 1.7, Forms have an add_error() method and our definition
was conflicting with Django's method.
---
horizon/workflows/base.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/horizon/workflows/base.py b/horizon/workflows/base.py
index 45f4df8..0e9ddac 100644
--- a/horizon/workflows/base.py
+++ b/horizon/workflows/base.py
@@ -164,7 +164,7 @@ class Action(forms.Form):
text += linebreaks(force_unicode(self.help_text))
return safe(text)
- def add_error(self, message):
+ def add_action_error(self, message):
"""Adds an error to the Action's Step based on API issues."""
self.errors[NON_FIELD_ERRORS] = self.error_class([message])
@@ -432,9 +432,9 @@ class Step(object):
text += self.action.get_help_text()
return safe(text)
- def add_error(self, message):
+ def add_step_error(self, message):
"""Adds an error to the Step based on API issues."""
- self.action.add_error(message)
+ self.action.add_action_error(message)
def has_required_fields(self):
"""Returns True if action contains any required fields."""
@@ -863,4 +863,4 @@ class Workflow(html.HTMLElement):
"""
step = self.get_step(slug)
if step:
- step.add_error(message)
+ step.add_step_error(message)
--
2.0.1
>From 77dc7b17501956a20c2ed3109909b16f28a76946 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <[email protected]>
Date: Mon, 4 Aug 2014 22:27:51 +0200
Subject: [PATCH 3/5] Fix summation code to handle invalid input data
This commit fixes multiple test failures with Django 1.7 that all ended
with this:
File "/usr/lib/python2.7/dist-packages/django/template/base.py", line 734, in resolve
value = self._resolve_lookup(context)
File "/usr/lib/python2.7/dist-packages/django/template/base.py", line 788, in _resolve_lookup
current = current()
File "/home/rhertzog/tmp/django17/horizon/horizon/tables/base.py", line 404, in get_summation
summation = summation_function(data)
File "/home/rhertzog/tmp/django17/horizon/horizon/tables/base.py", line 206, in <lambda>
"average": lambda data: sum(data, 0.0) / len(data)
TypeError: unsupported operand type(s) for +: 'float' and 'str'
With Django 1.6, the template code that looked up the variable behind
get_summation was catching the TypeError exception:
try: # method call (assuming no args required)
current = current()
except TypeError: # arguments *were* required
# GOTCHA: This will also catch any TypeError
# raised in the function itself.
current = settings.TEMPLATE_STRING_IF_INVALID # invalid method call
With Django 1.7, the code has been refined to catch the exception only
when the function really requires argument (which get_summation() doesn't):
try: # method call (assuming no args required)
current = current()
except TypeError:
try:
getcallargs(current)
except TypeError: # arguments *were* required
current = settings.TEMPLATE_STRING_IF_INVALID # invalid method call
else:
raise
So instead of blindly relying on sum(), I introduced a safe_sum() and safe_average()
functions which mimick the behaviour we got with Django 1.6 by returning an empty
string when we have invalid input data.
---
horizon/tables/base.py | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/horizon/tables/base.py b/horizon/tables/base.py
index 10aaa98..63dc087 100644
--- a/horizon/tables/base.py
+++ b/horizon/tables/base.py
@@ -49,6 +49,28 @@ PALETTE = termcolors.PALETTES[termcolors.DEFAULT_PALETTE]
STRING_SEPARATOR = "__"
+def safe_sum(data):
+ """
+ Returns a sum when we have an iterable of numbers, an empty
+ string otherwise.
+ """
+ try:
+ return sum(data)
+ except TypeError:
+ return ''
+
+
+def safe_average(data):
+ """
+ Returns the average when we have an interable of numbers,
+ an empty string otherwise.
+ """
+ try:
+ return sum(data, 0.0) / len(data)
+ except TypeError:
+ return ''
+
+
class Column(html.HTMLElement):
"""A class which represents a single column in a :class:`.DataTable`.
@@ -202,8 +224,8 @@ class Column(html.HTMLElement):
Defaults to ``None``.
"""
summation_methods = {
- "sum": sum,
- "average": lambda data: sum(data, 0.0) / len(data)
+ "sum": safe_sum,
+ "average": safe_average
}
# Used to retain order when instantiating columns on a table
creation_counter = 0
--
2.0.1
>From 50f59bfe78515931fcdd908f003d3f9af39bbb51 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <[email protected]>
Date: Mon, 4 Aug 2014 22:48:43 +0200
Subject: [PATCH 4/5] =?UTF-8?q?Fix=20=E2=80=9CTypeError:=20'SecurityGroup'?=
=?UTF-8?q?=20object=20is=20not=20iterable=E2=80=9D=20test=20failure=20wit?=
=?UTF-8?q?h=20Django=201.7?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The two tests modified here were incorrectly defining
instance.security_groups as a single value instead of a list.
Apparently Django 1.7 is no longer happy trying to iterate something that
is not an iterable.
The other test_instance_details_*() were already doing the correct
thing so just copy over the logic.
---
openstack_dashboard/dashboards/project/instances/tests.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/openstack_dashboard/dashboards/project/instances/tests.py b/openstack_dashboard/dashboards/project/instances/tests.py
index e3e7f23..bb2cfc4 100644
--- a/openstack_dashboard/dashboards/project/instances/tests.py
+++ b/openstack_dashboard/dashboards/project/instances/tests.py
@@ -627,7 +627,7 @@ class InstanceTests(test.TestCase):
api.nova.flavor_get(IsA(http.HttpRequest), server.flavor['id']) \
.AndReturn(self.flavors.first())
api.network.server_security_groups(IsA(http.HttpRequest), server.id) \
- .AndReturn(self.security_groups.first())
+ .AndReturn(self.security_groups.list())
self.mox.ReplayAll()
@@ -654,7 +654,7 @@ class InstanceTests(test.TestCase):
api.nova.flavor_get(IsA(http.HttpRequest), server.flavor['id']) \
.AndReturn(self.flavors.first())
api.network.server_security_groups(IsA(http.HttpRequest), server.id) \
- .AndReturn(self.security_groups.first())
+ .AndReturn(self.security_groups.list())
self.mox.ReplayAll()
--
2.0.1
>From 72e6311506d934cdf7023c37e04ee6356ef9ef6a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <[email protected]>
Date: Mon, 4 Aug 2014 23:32:03 +0200
Subject: [PATCH 5/5] Tentative fix for a test suite failure after the last
change
The exception fixed is the following:
NoReverseMatch: Reverse for 'detail' with arguments '('',)' and keyword arguments '{}' not found. 1 pattern(s) tried: [u'project/volumes/(?P<volume_id>[^/]+)/$']
The test data doesn't include a volumeId attribute to volumes but they
contain an id attribute.
Here I modify the template to rely on the the "id" attribute but maybe
real data has a volumeId attribute and it's the test data in
openstack_dashboard/test/test_data/nova_data.py that needs to be modified
instead.
---
.../project/instances/templates/instances/_detail_overview.html | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/openstack_dashboard/dashboards/project/instances/templates/instances/_detail_overview.html b/openstack_dashboard/dashboards/project/instances/templates/instances/_detail_overview.html
index 286aef8..0968218 100644
--- a/openstack_dashboard/dashboards/project/instances/templates/instances/_detail_overview.html
+++ b/openstack_dashboard/dashboards/project/instances/templates/instances/_detail_overview.html
@@ -119,11 +119,11 @@
{% for volume in instance.volumes %}
<dt>{% trans "Attached To" %}</dt>
<dd>
- <a href="{% url 'horizon:project:volumes:volumes:detail' volume.volumeId %}">
+ <a href="{% url 'horizon:project:volumes:volumes:detail' volume.id %}">
{% if volume.name %}
{{ volume.name }}
{% else %}
- {{ volume.volumeId }}
+ {{ volume.id }}
{% endif %}
</a>
<span> {% trans "on" %} {{ volume.device }}</span>
--
2.0.1
