Merge lp:~rvb/maas/images-link-bug-1427469 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix link to the images page: the call to Django's reverse() method isn't done in the context of a request and thus Django can't automatically add the 'script prefix' (i.e. the path prefix) when computing the URL.

[Gavin Panella (allenap)]

Looks good, but do you foresee this as a common problem?

review: Approve

[Raphaël Badin (rvb)]

> Looks good, but do you foresee this as a common problem?

If we use reverse() outside of a Django context (i.e. a request context) then yes, we will need to use the same trick. That being said, the Twisted code is rarely in charge of doing things that require calling reverse()… that's why I decided to fix this quickly instead of creating a proper utility. If and when we see this pattern being repeated, we will refactor this into a utility.

[ubuntudotcom1 (ubuntudotcom1)]

hmm.. so
abs_path = urlparse("http://localhost:5240/MAAS").path
absolute_reverse('images', base_url=abs_path)

should return '/MAAS/images' correct?

[Raphaël Badin (rvb)]

Right, there was something fishy due to how urljoin works and how Django silently adds the script prefix when you call reverse() in a Django context.

I've fixed the problem (by adding a dedicated utility) now, please have another look.

[Gavin Panella (allenap)]

Still looks good, but I have a few questions this time.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/images-link-bug-1427469 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-iscpy python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-pyparsing python-seamicroclient python-simple...

=== modified file 'src/maasserver/bootresources.py'
--- src/maasserver/bootresources.py	2015-02-20 16:52:57 +0000
+++ src/maasserver/bootresources.py	2015-03-09 20:52:18 +0000
@@ -28,7 +28,6 @@
 import threading
 import time
 
-from django.core.urlresolvers import reverse
 from django.db import (
     close_old_connections,
     connections,
@@ -63,7 +62,10 @@
     LargeFile,
     NodeGroup,
     )
-from maasserver.utils import absolute_reverse
+from maasserver.utils import (
+    absolute_reverse,
+    absolute_url_reverse,
+    )
 from maasserver.utils.orm import (
     get_one,
     transactional,
@@ -1022,6 +1024,7 @@
             # If the cluster is on the same machine as the region, it's
             # possible that the cluster has images and the region does not. We
             # can provide a better message to the user in this case.
+            images_link = absolute_url_reverse('images')
             boot_images_locally = list_boot_images()
             if len(boot_images_locally) > 0:
                 warning = (
@@ -1029,11 +1032,11 @@
                     "does not. Nodes will not be able to provision until you "
                     "import boot images into the region. Visit the "
                     "<a href=\"%s\">boot images</a> page to start the "
-                    "import." % reverse('images'))
+                    "import." % images_link)
             else:
                 warning = (
                     "Boot image import process not started. Nodes will not "
                     "be able to provision without boot images. Visit the "
                     "<a href=\"%s\">boot images</a> page to start the "
-                    "import." % reverse('images'))
+                    "import." % images_link)
             register_persistent_error(COMPONENT.IMPORT_PXE_FILES, warning)

=== modified file 'src/maasserver/tests/test_bootresources.py'
--- src/maasserver/tests/test_bootresources.py	2015-03-03 13:46:14 +0000
+++ src/maasserver/tests/test_bootresources.py	2015-03-09 20:52:18 +0000
@@ -30,7 +30,10 @@
 from django.http import StreamingHttpResponse
 from django.test.client import Client
 from fixtures import Fixture
-from maasserver import bootresources
+from maasserver import (
+    bootresources,
+    utils as utils_module,
+    )
 from maasserver.bootresources import (
     BootResourceStore,
     download_all_boot_resources,
@@ -1260,6 +1263,9 @@
         self.expectThat(kwargs, Equals({}))
 
     def test__adds_warning_if_boot_images_exists_on_cluster_not_region(self):
+        abs_path = "/%s/%s/" % (factory.make_string(), factory.make_string())
+        self.patch(
+            utils_module.settings, 'DEFAULT_MAAS_URL', 'http://%s' % abs_path)
         list_boot_images = self.patch(bootresources, "list_boot_images")
         list_boot_images.return_value = [make_rpc_boot_image()]
 
@@ -1273,11 +1279,16 @@
         the region. Visit the <a href="%s">boot images</a> page to start the
         import.
         """
+        images_link = abs_path + 'images/'
         self.assertEqual(
-            normalise_whitespace(error_expected % reverse('images')),
+            normalise_whitespace(error_expected % images_link),
             error_observed)
 
     def test__adds_warning_if_boot_image_import_not_started(self):
+        abs_path = "/%s/%s/" % (factory.make_string(), factory.make_string())
+        self.patch(
+            utils_module.settings, 'DEFAULT_MAAS_URL', 'http://%s' % abs_path)
+
         list_boot_images = self.patch(bootresources, "list_boot_images")
         list_boot_images.return_value = []
 
@@ -1290,8 +1301,9 @@
         provision without boot images. Visit the <a href="%s">boot images</a>
         page to start the import.
         """
+        images_link = abs_path + 'images/'
         self.assertEqual(
-            normalise_whitespace(error_expected % reverse('images')),
+            normalise_whitespace(error_expected % images_link),
             error_observed)
 
     def test__removes_warning_if_boot_image_process_started(self):

=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2015-02-19 21:49:54 +0000
+++ src/maasserver/utils/__init__.py	2015-03-09 20:52:18 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = [
     'absolute_reverse',
+    'absolute_reverse_url',
     'build_absolute_uri',
     'find_nodegroup',
     'get_db_state',
@@ -28,7 +29,10 @@
 from functools import wraps
 import re
 from urllib import urlencode
-from urlparse import urljoin
+from urlparse import (
+    urljoin,
+    urlparse,
+    )
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
@@ -86,6 +90,38 @@
     return url
 
 
+def absolute_url_reverse(view_name, query=None, *args, **kwargs):
+    """Returns the absolute path (i.e. starting with '/') for the given view.
+
+    This utility is meant to be used by methods that need to compute URLs but
+    run outside of Django and thus don't have the 'script prefix' transparently
+    added the the URL.
+
+    :param view_name: Django's view function name/reference or URL pattern
+        name for which to compute the absolute URL.
+    :param query: Optional query argument which will be passed down to
+        urllib.urlencode.  The result of that call will be appended to the
+        resulting url.
+    :param args: Positional arguments for Django's 'reverse' method.
+    :param kwargs: Named arguments for Django's 'reverse' method.
+    """
+    abs_path = urlparse(settings.DEFAULT_MAAS_URL).path
+    if not abs_path.endswith('/'):
+        # Add trailing '/' to get urljoin to behave.
+        abs_path = abs_path + '/'
+    # Force prefix to be '' so that Django doesn't use the 'script prefix' (
+    # which might be there or not depending on whether or not the thread local
+    # variable has been initialized).
+    reverse_link = reverse(view_name, prefix='', *args, **kwargs)
+    if reverse_link.startswith('/'):
+        # Drop the leading '/'.
+        reverse_link = reverse_link[1:]
+    url = urljoin(abs_path, reverse_link)
+    if query is not None:
+        url += '?%s' % urlencode(query, doseq=True)
+    return url
+
+
 def build_absolute_uri(request, path):
     """Return absolute URI corresponding to given absolute path.
 

=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2015-02-19 21:49:54 +0000
+++ src/maasserver/utils/tests/test_utils.py	2015-03-09 20:52:18 +0000
@@ -32,6 +32,7 @@
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import (
     absolute_reverse,
+    absolute_url_reverse,
     build_absolute_uri,
     find_nodegroup,
     get_db_state,
@@ -83,6 +84,35 @@
         self.assertEqual(expected_url, absolute_url)
 
 
+class TestAbsoluteUrlReverse(MAASServerTestCase):
+
+    def test_absolute_url_reverse_uses_path_from_DEFAULT_MAAS_URL(self):
+        path = "/%s/%s" % (factory.make_string(), factory.make_string())
+        maas_url = 'http://%s%s' % (factory.make_string(), path)
+        self.patch(settings, 'DEFAULT_MAAS_URL', maas_url)
+        absolute_url = absolute_url_reverse('settings')
+        expected_url = path + reverse('settings')
+        self.assertEqual(expected_url, absolute_url)
+
+    def test_absolute_url_reverse_copes_with_trailing_slash(self):
+        path = "/%s/%s/" % (factory.make_string(), factory.make_string())
+        maas_url = 'http://%s%s' % (factory.make_string(), path)
+        self.patch(settings, 'DEFAULT_MAAS_URL', maas_url)
+        absolute_url = absolute_url_reverse('settings')
+        expected_url = path[:-1] + reverse('settings')
+        self.assertEqual(expected_url, absolute_url)
+
+    def test_absolute_url_reverse_uses_query_string(self):
+        path = "/%s/%s" % (factory.make_string(), factory.make_string())
+        maas_url = 'http://%s%s' % (factory.make_string(), path)
+        self.patch(settings, 'DEFAULT_MAAS_URL', maas_url)
+        parameters = {factory.make_string(): factory.make_string()}
+        absolute_url = absolute_url_reverse('settings', query=parameters)
+        expected_url = path + "%s?%s" % (
+            reverse('settings'), urlencode(parameters))
+        self.assertEqual(expected_url, absolute_url)
+
+
 class GetDbStateTest(MAASServerTestCase):
     """Testing for the method `get_db_state`."""