launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/rel-img-fix into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/rel-img-fix/+merge/112737

This branch fixes a problem triggered by the fact that we now use the combo loader to load MAAS's JS libraries.  The images referenced in the CSS files could not be found because they are referenced using address relative to the combo loader address.

I've fixed that by improving the way the combo loader works: it now can take an optional parameter to indicate that requests for a single file of an unknown type (i.e. not "js" or "css") should be redirected to a specific location (i.e. some address where static assets can be found).
-- 
https://code.launchpad.net/~rvb/maas/rel-img-fix/+merge/112737
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/rel-img-fix into lp:maas.

=== modified file 'src/maasserver/static/css/base.css'
--- src/maasserver/static/css/base.css	2012-04-17 10:30:16 +0000
+++ src/maasserver/static/css/base.css	2012-06-29 10:11:22 +0000
@@ -3,7 +3,7 @@
     padding: 0;
     }
 body {
-    background: #2C001E url(../img/bg_dots.png) repeat top center;
+    background: #2C001E url(../?img/bg_dots.png) repeat top center;
     }
 table {
     width: 100%;

=== modified file 'src/maasserver/static/css/layout.css'
--- src/maasserver/static/css/layout.css	2012-04-05 06:23:59 +0000
+++ src/maasserver/static/css/layout.css	2012-06-29 10:11:22 +0000
@@ -26,7 +26,7 @@
     }
 #header {
     min-height: 64px;
-    background: #dd4814 url(../img/header_stripes.png) repeat-x top left;
+    background: #dd4814 url(../?img/header_stripes.png) repeat-x top left;
     -moz-border-radius: 0 0 4px 4px;
     -webkit-border-radius: 0 0 4px 4px;
     border-radius: 0 0 4px 4px;
@@ -59,7 +59,7 @@
     }
 #right-nav li.active a,
 #main-nav li.active a {
-    background: rgba(0,0,0,0.2) url(../img/header_tail.png) no-repeat bottom center;
+    background: rgba(0,0,0,0.2) url(../?img/header_tail.png) no-repeat bottom center;
     }
 #right-nav {
     float: right;
@@ -107,7 +107,7 @@
     color: #fff;
     margin: 0 20px;
     padding: 14px 0 20px 0;
-    background: transparent url(../img/content_tail.png) no-repeat 50px bottom;
+    background: transparent url(../?img/content_tail.png) no-repeat 50px bottom;
     }
 #content {
     padding: 20px 20px 40px 20px;

=== modified file 'src/maasserver/tests/test_views_combo.py'
--- src/maasserver/tests/test_views_combo.py	2012-06-26 13:22:11 +0000
+++ src/maasserver/tests/test_views_combo.py	2012-06-29 10:11:22 +0000
@@ -19,6 +19,7 @@
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from django.test.client import RequestFactory
+from maasserver.testing import extract_redirect
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maasserver.views.combo import (
@@ -78,6 +79,23 @@
              (httplib.OK, expected_content),
              (response.status_code, response.content))
 
+    def test_get_combo_redirects_if_unknown_type(self):
+        # The optional parameter 'default_redirect' allows to configure
+        # a default address where requests for files of unknown types will be
+        # redirected.
+        # Create a test file with an unknown extension.
+        test_file_name = "%s.%s" % (
+            factory.getRandomString(), factory.getRandomString())
+        redirect_root = factory.getRandomString()
+        view = get_combo_view(
+            factory.getRandomString(), default_redirect=redirect_root)
+        rf = RequestFactory()
+        request = rf.get("/test/?%s" % test_file_name)
+        response = view(request)
+        self.assertEqual(
+            '%s%s' % (redirect_root, test_file_name),
+            extract_redirect(response))
+
 
 # String used by convoy to replace missing files.
 CONVOY_MISSING_FILE = "/* [missing] */"
@@ -141,6 +159,14 @@
         # No sign of a missing css file.
         self.assertNotIn(CONVOY_MISSING_FILE, response.content)
 
+    def test_maas_load_image(self):
+        img_path = 'img/bg_dots.png'
+        url = '%s?%s' % (reverse('combo-maas'), img_path)
+        response = self.client.get(url)
+        self.assertEqual(
+            '%s%s' % (settings.STATIC_URL, img_path),
+            extract_redirect(response))
+
     def test_raphael_load_js(self):
         requested_files = ['raphael-min.js']
         url = '%s?%s' % (reverse('combo-raphael'), '&'.join(requested_files))

=== modified file 'src/maasserver/urls_combo.py'
--- src/maasserver/urls_combo.py	2012-06-26 13:22:11 +0000
+++ src/maasserver/urls_combo.py	2012-06-29 10:11:22 +0000
@@ -13,7 +13,7 @@
 __all__ = []
 
 
-from django.conf import settings as django_settings
+from django.conf import settings
 from django.conf.urls.defaults import (
     patterns,
     url,
@@ -23,13 +23,15 @@
 
 urlpatterns = patterns('',
     url(
-        r'^maas/', get_combo_view(), name='combo-maas'),
+        r'^maas/', get_combo_view(
+            location='', default_redirect=settings.STATIC_URL),
+        name='combo-maas'),
     url(
         r'^raphael/',
-        get_combo_view(location=django_settings.RAPHAELJS_LOCATION),
+        get_combo_view(location=settings.RAPHAELJS_LOCATION),
         name='combo-raphael'),
     url(
         r'^yui/',
-        get_combo_view(location=django_settings.YUI_LOCATION),
+        get_combo_view(location=settings.YUI_LOCATION),
         name='combo-yui'),
 )

=== modified file 'src/maasserver/views/combo.py'
--- src/maasserver/views/combo.py	2012-06-26 13:22:11 +0000
+++ src/maasserver/views/combo.py	2012-06-29 10:11:22 +0000
@@ -26,6 +26,7 @@
     HttpResponse,
     HttpResponseBadRequest,
     HttpResponseNotFound,
+    HttpResponseRedirect,
     )
 
 # Static root computed from this very file.
@@ -60,19 +61,24 @@
         return os.path.join(LOCAL_STATIC_ROOT, location)
 
 
-def get_combo_view(location=''):
+def get_combo_view(location='', default_redirect=None):
     """Return a Django view to serve static resources using a combo loader.
 
     :param location: An optional absolute or relative location.
     :type location: basestring
+    :param default_redirect: An optional address where requests for one file
+        of an unknown file type will be redirected.  If this parameter is
+        omitted, such requests will lead to a "Bad request" response.
+    :type location: basestring
     :return: A Django view method.
     :rtype: callable
     """
     location = get_absolute_location(location)
-    return partial(combo_view, location=location)
-
-
-def combo_view(request, location, encoding='utf8'):
+    return partial(
+        combo_view, location=location, default_redirect=default_redirect)
+
+
+def combo_view(request, location, default_redirect=None, encoding='utf8'):
     """Handle a request for combining a set of files.
 
     The files are searched in the absolute location `abs_location` (if
@@ -85,6 +91,9 @@
             content_type = 'text/javascript; charset=UTF-8'
         elif fnames[0].endswith('.css'):
             content_type = 'text/css'
+        elif default_redirect is not None and len(fnames) == 1:
+            return HttpResponseRedirect(
+                "%s%s" % (default_redirect, fnames[0]))
         else:
             return HttpResponseBadRequest("Invalid file type requested.")
         content = "".join(