Merge lp:~elachuni/ubuntu-webcatalog/convoy into lp:ubuntu-webcatalog

[Anthony Lenton]

Added a combo loader view using lp:convoy to load multiple css stylesheets or javascript files.

Overview
========
This branch adds a combo loader view using lp:convoy to load multiple css stylesheets or javascript files. It also plugs this into YUI so that all the javascript YUI pulls in is piled into a single request.

Details
=======
This improvement comes from a recent discussion on canonical-tech, the view is largely based on Sidnei's combo view for Django.
Using the combo loader all pages need at least three requests less (as the four stylesheets pulled in by base.html are
all requested together), and the application details page goes down from 34 requests to 8.

This change is part of a sequence of three MPs to add a nifty "exhibits" widget to the front page. The next branch will add the YUI3 carrousel widget.

[Michael Nelson (michael.nelson)]

On Mon, Mar 5, 2012 at 7:13 PM, Anthony Lenton
<email address hidden> wrote:
> Anthony Lenton has proposed merging lp:~elachuni/ubuntu-webcatalog/convoy into lp:ubuntu-webcatalog with lp:~elachuni/ubuntu-webcatalog/exhibit-widget as a prerequisite.
>
> Requested reviews:
>  Canonical Consumer Applications Hackers (canonical-ca-hackers)
>
> For more details, see:
> https://code.launchpad.net/~elachuni/ubuntu-webcatalog/convoy/+merge/95963
>
> Overview
> ========
> This branch adds a combo loader view using lp:convoy to load multiple css stylesheets or javascript files.  It also plugs this into YUI so that all the javascript YUI pulls in is piled into a single request.

Excellent! All looks great... I had a few questions/thoughts below
inline that we may want to think about, but approving as is.

>
> Details
> =======
> This improvement comes from a recent discussion on canonical-tech, the view is largely based on Sidnei's combo view for Django.
> Using the combo loader all pages need at least three requests less (as the four stylesheets pulled in by base.html are
> all requested together), and the application details page goes down from 34 requests to 8.
>
> This change is part of a sequence of three MPs to add a nifty "exhibits" widget to the front page.  The next branch will add the YUI3 carrousel widget.
>
> --
> https://code.launchpad.net/~elachuni/ubuntu-webcatalog/convoy/+merge/95963
> You are subscribed to branch lp:ubuntu-webcatalog.
>
> === modified file '.bzrignore'
> --- .bzrignore  2011-09-15 10:53:40 +0000
> +++ .bzrignore  2012-03-05 18:12:18 +0000
> === modified file 'django_project/config/main.cfg'
> --- django_project/config/main.cfg      2012-03-05 18:12:17 +0000
> +++ django_project/config/main.cfg      2012-03-05 18:12:18 +0000
> @@ -50,7 +50,7 @@
>                               webcatalog.context_processors.google_analytics_id
>                               webcatalog.context_processors.user_agent
>
> -static_root = ./django_project/static/
> +static_root = ./src/webcatalog/static/

Hrm.. it sounds like we're not using this in the way it's meant to be
used... right, from memory we haven't started using the static files
support really. static_root should be point at where we want static
files collated when running the collectstatic management command:

https://docs.djangoproject.com/en/1.3/howto/static-files/#deploying-static-files-in-a-nutshell

Although maybe that's fine for development... might be worth us
reviewing the way we're handling these static files.

>  static_url = /assets/
>  admin_media_prefix = /assets/admin/
>
>
> === modified file 'fabtasks/bootstrap.py'
> --- fabtasks/bootstrap.py       2012-01-06 14:00:18 +0000
> +++ fabtasks/bootstrap.py       2012-03-05 18:12:18 +0000
> @@ -125,6 +125,8 @@
>     _symlink(
>         "branches/django-openid-auth/django_openid_auth",
>         "django_project/django_openid_auth")
> +    _get_or_pull_bzr_branch("lp:convoy", "convoy", revision=20)
> +    _symlink("branches/convoy/convoy", "django_project/convoy")

And it's available as a package to install for deploy? If so, we
should be updating the dependencies too right (that's a separate
branch now isn't it.. ...

[Michael Nelson (michael.nelson)]

review: Approve

[ISD Branch Mangler (isd-branches-mangler)]

The prerequisite lp:~elachuni/ubuntu-webcatalog/exhibit-widget has not yet been merged into lp:ubuntu-webcatalog.

=== modified file '.bzrignore'
--- .bzrignore	2011-09-15 10:53:40 +0000
+++ .bzrignore	2012-03-05 18:12:18 +0000
@@ -2,6 +2,7 @@
 src/ubuntu_webcatalog.egg-info/
 virtualenv/
 django_project/django_openid_auth
+django_project/convoy
 django_project/local_settings.py
 django_project/static/
 django_project/media_root_dev/*

=== modified file 'django_project/config/main.cfg'
--- django_project/config/main.cfg	2012-03-05 18:12:17 +0000
+++ django_project/config/main.cfg	2012-03-05 18:12:18 +0000
@@ -50,7 +50,7 @@
                               webcatalog.context_processors.google_analytics_id
                               webcatalog.context_processors.user_agent
 
-static_root = ./django_project/static/
+static_root = ./src/webcatalog/static/
 static_url = /assets/
 admin_media_prefix = /assets/admin/
 

=== modified file 'fabtasks/bootstrap.py'
--- fabtasks/bootstrap.py	2012-01-06 14:00:18 +0000
+++ fabtasks/bootstrap.py	2012-03-05 18:12:18 +0000
@@ -125,6 +125,8 @@
     _symlink(
         "branches/django-openid-auth/django_openid_auth",
         "django_project/django_openid_auth")
+    _get_or_pull_bzr_branch("lp:convoy", "convoy", revision=20)
+    _symlink("branches/convoy/convoy", "django_project/convoy")
 
 def bootstrap():
     virtualenv_create()

=== modified file 'src/webcatalog/templates/light/404.html'
--- src/webcatalog/templates/light/404.html	2011-07-04 11:52:26 +0000
+++ src/webcatalog/templates/light/404.html	2012-03-05 18:12:18 +0000
@@ -5,7 +5,7 @@
 {% block header %}{% endblock %}
 
 {% block head_extra %}
-        <link rel="stylesheet" type="text/css" href="{{ ASSET_ROOT }}/assets/light/css/404.css"/>
+    <link rel="stylesheet" type="text/css" href="{% url wc-combo %}?light/css/reset.css&light/css/styles.css&light/css/404.css&light/css/forms.css"/>
 {% endblock %}
 
 {% block content_container %}

=== modified file 'src/webcatalog/templates/light/index.1col.html'
--- src/webcatalog/templates/light/index.1col.html	2012-01-06 14:24:49 +0000
+++ src/webcatalog/templates/light/index.1col.html	2012-03-05 18:12:18 +0000
@@ -3,10 +3,9 @@
 <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en" dir="ltr">
     <head>
         <title>{% block title %}{% endblock %}</title>
-        <link rel="stylesheet" type="text/css" href="/assets/light/css/reset.css"/>
-        <link rel="stylesheet" type="text/css" href="/assets/light/css/styles.css"/>
-        {% block head_extra %}{% endblock %}
-        <link rel="stylesheet" type="text/css" href="/assets/light/css/forms.css"/>
+        {% block head_extra %}
+        <link rel="stylesheet" type="text/css" href="{% url wc-combo %}?light/css/reset.css&light/css/styles.css&light/css/forms.css"/>
+        {% endblock %}
     </head>
     <body>
         <div id="container">

=== modified file 'src/webcatalog/templates/webcatalog/application_detail.html'
--- src/webcatalog/templates/webcatalog/application_detail.html	2011-09-16 09:47:19 +0000
+++ src/webcatalog/templates/webcatalog/application_detail.html	2012-03-05 18:12:18 +0000
@@ -8,7 +8,7 @@
 {{ block.super }}
 <script src="{{ STATIC_URL }}yui/3.4.0/build/yui/yui-min.js"></script>
 <script>
-YUI().use('io-base', 'node-base', function (Y) {
+YUI({debug: false, combine: true, comboBase: '{% url wc-combo %}?', root: 'yui/3.4.0/build/'}).use('io-base', 'node-base', function (Y) {
     function complete(id, obj){
         var reviews_html = obj.responseText;
         var reviews_div = Y.one('#reviews_placeholder');

=== modified file 'src/webcatalog/templates/webcatalog/base.html'
--- src/webcatalog/templates/webcatalog/base.html	2011-04-13 19:52:23 +0000
+++ src/webcatalog/templates/webcatalog/base.html	2012-03-05 18:12:18 +0000
@@ -3,7 +3,7 @@
 {% block menus %}{% endblock %}
 
 {% block head_extra %}
-  <link rel="stylesheet" type="text/css" href="{{ STATIC_URL }}css/webcatalog.css"/>
+    <link rel="stylesheet" type="text/css" href="{% url wc-combo %}?light/css/reset.css&light/css/styles.css&css/webcatalog.css&light/css/forms.css"/>
 {% endblock %}
 
 {% block search %}

=== modified file 'src/webcatalog/tests/test_views.py'
--- src/webcatalog/tests/test_views.py	2012-03-05 18:12:17 +0000
+++ src/webcatalog/tests/test_views.py	2012-03-05 18:12:18 +0000
@@ -26,6 +26,7 @@
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
+from django.test import TestCase
 
 from mock import patch
 from rnrclient import ReviewDetails
@@ -42,6 +43,7 @@
     'IndexTestCase',
     'OverviewTestCase',
     'SearchTestCase',
+    'ComboViewTestCase',
     ]
 
 
@@ -638,3 +640,35 @@
             response, 'webcatalog/application_review_list_snippet.html')
         self.assertContains(response, 'review_summary1')
         self.assertContains(response, 'review_summary2')
+
+
+class ComboViewTestCase(TestCase):
+    """Tests for ComboView."""
+
+    def test_no_fnames_in_request(self):
+        """If there are no filenames in the request, a 404 is returned."""
+        response = self.client.get(reverse('wc-combo'))
+        self.assertEqual(404, response.status_code)
+        self.assertEqual('text/plain', response['Content-Type'])
+
+    def test_content_type_for_js(self):
+        """
+        If the requested files are JS files, the response content-type will be
+        set to 'text/javascript'.
+        """
+        response = self.client.get(reverse('wc-combo') + '?js/utils.js')
+        self.assertEqual(200, response.status_code)
+        self.assertEqual('text/javascript', response['Content-Type'])
+        expected = '/* js/utils.js */\n/* [missing] */\n'
+        self.assertEqual(expected, response.content)
+
+    def test_content_type_for_css(self):
+        """
+        If the requested files are CSS files, the response content-type will be
+        set to 'text/css'.
+        """
+        response = self.client.get(reverse('wc-combo') + '?js/foo.css')
+        self.assertEqual(200, response.status_code)
+        expected = '/* js/foo.css */\n/* [missing] */\n'
+        self.assertEqual('text/css', response['Content-Type'])
+        self.assertEqual(expected, response.content)

=== modified file 'src/webcatalog/urls.py'
--- src/webcatalog/urls.py	2011-09-15 07:16:30 +0000
+++ src/webcatalog/urls.py	2012-03-05 18:12:18 +0000
@@ -45,7 +45,7 @@
         'application_reviews', name="wc-package-reviews"),
     url(r'^search/$', 'search', name="wc-search"),
     url(r'^search/(?P<distro>[-.+\w]+)/$', 'search', name="wc-search"),
+    url(r'^combo/$', 'combo_view', name='wc-combo'),
 
     (r'^api/', include('webcatalog.api.urls')),
-
 )

=== modified file 'src/webcatalog/views.py'
--- src/webcatalog/views.py	2012-03-05 18:12:17 +0000
+++ src/webcatalog/views.py	2012-03-05 18:12:18 +0000
@@ -23,14 +23,16 @@
     )
 
 import operator
+import os
 from random import randint
 from urllib import urlencode
 
+from convoy.combo import combine_files, parse_qs
 from django.conf import settings
 from django.core.paginator import Paginator
 from django.core.urlresolvers import reverse
 from django.db.models import Q
-from django.http import HttpResponseRedirect
+from django.http import HttpResponseRedirect, HttpResponse
 from django.shortcuts import (
     get_object_or_404,
     render_to_response,
@@ -203,3 +205,23 @@
         template = 'webcatalog/application_review_list.html'
 
     return render_to_response(template, RequestContext(request, context))
+
+
+def combo_view(request):
+    """Handle a request for combining a set of files."""
+    fnames = parse_qs(request.META.get("QUERY_STRING", ""))
+    content_type = "text/plain"
+
+    if fnames:
+        if fnames[0].endswith(".js"):
+            content_type = "text/javascript"
+        elif fnames[0].endswith(".css"):
+            content_type = "text/css"
+        content = combine_files(fnames, os.path.abspath(settings.STATIC_ROOT),
+              resource_prefix=settings.MEDIA_URL, rewrite_urls=True)
+        # We're turning the generator returned by combine_files into a string
+        # here since GZipMiddleware would consume it if not. See Bug #822888.
+        return HttpResponse(
+            content_type=content_type, status=200,
+            content="".join(content))
+    return HttpResponse(content_type=content_type, status=404)