Merge lp:~abentley/launchpad/data-download-view into lp:launchpad

[Aaron Bentley]

Extract DataDownloadView

= Summary =
Extract DataDownloadView from CodeOfConductDownloadView and BaseRdfView

== Proposed fix ==
See above

== Pre-implementation notes ==
Discussed Content-type change with sinzui.

== Implementation details ==
Changed Content-Type of downloaded CoC from the fictitious 'application/text' to the standard 'text/plain'. We believe this was a hack to ensure users were prompted to save the CoC, but the Content-disposition of "attachment" achieves this well.

The filename parameter of the Content-disposition header is now always quoted, in compliance with the RFC.

Changed the indentation of a couple of test cases, to fix lint errors.

Subclasses of BaseRdfView must now specify the extension, as well as the rest
of the filename.

== Tests ==
bin/test -t test_codeofconduct -t test_person -t test_product -t xx-productrelease-rdf.txt -t xx-productseries-rdf.txt

== Demo and Q/A ==
Visit https://qastaging.launchpad.net/codeofconduct/1.1/+download and you should be prompted to save the file. The text should be the Code of Conduct.

Visit https://qastaging.launchpad.net/launchpad/+rdf and you should be prompted to save the file. The text should be RDF.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/__init__.py
  lib/lp/registry/browser/codeofconduct.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/productrelease.py
  lib/lp/registry/browser/productseries.py
  lib/lp/registry/browser/tests/test_codeofconduct.py
  lib/lp/registry/browser/tests/test_person.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/stories/productrelease/xx-productrelease-rdf.txt
  lib/lp/registry/stories/productseries/xx-productseries-rdf.txt
  lib/lp/services/webapp/publisher.py

[Richard Harding (rharding)]

review: Approve (code*)

[j.c.sackett (jcsackett)]

I have nothing to add. This is a very solid change, thanks Aaron.

review: Approve

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__.py	2012-01-19 22:59:03 +0000
+++ lib/lp/registry/browser/__init__.py	2012-01-27 19:21:54 +0000
@@ -37,6 +37,7 @@
     )
 from lp.services.webapp.publisher import (
     canonical_url,
+    DataDownloadView,
     LaunchpadView,
     )
 
@@ -271,17 +272,15 @@
         self.updateContextFromData(data)
 
 
-class BaseRdfView:
+class BaseRdfView(DataDownloadView):
     """A view that sets its mime-type to application/rdf+xml."""
 
     template = None
     filename = None
 
-    def __init__(self, context, request):
-        self.context = context
-        self.request = request
+    content_type = 'application/rdf+xml'
 
-    def __call__(self):
+    def getBody(self):
         """Render RDF output, and return it as a string encoded in UTF-8.
 
         Render the page template to produce RDF output.
@@ -289,10 +288,6 @@
 
         As a side-effect, HTTP headers are set for the mime type
         and filename for download."""
-        self.request.response.setHeader('Content-Type', 'application/rdf+xml')
-        self.request.response.setHeader(
-            'Content-Disposition', 'attachment; filename=%s.rdf' % (
-             self.filename))
         unicodedata = self.template()
         encodeddata = unicodedata.encode('utf-8')
         return encodeddata
 
=== modified file 'lib/lp/registry/browser/codeofconduct.py'
--- lib/lp/registry/browser/codeofconduct.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/codeofconduct.py	2012-01-27 19:21:54 +0000
@@ -45,6 +45,7 @@
     Link,
     )
 from lp.services.webapp.interfaces import ILaunchBag
+from lp.services.webapp.publisher import DataDownloadView
 
 
 class SignedCodeOfConductSetNavigation(GetitemNavigation):
@@ -132,34 +133,25 @@
         return self.context.title
 
 
-class CodeOfConductDownloadView:
+class CodeOfConductDownloadView(DataDownloadView):
     """Download view class for CoC page.
 
-    This view does not use a template, but uses a __call__ method
-    that returns a file to the browser.
+    This view provides a text file with "Content-disposition: attachment",
+    causing browsers to download rather than display it.
     """
 
-    def __init__(self, context, request):
-        self.context = context
-        self.request = request
+    content_type = 'text/plain'
 
-    def __call__(self):
-        """Set response headers to download an attachment, and return
-        CoC file data.
-        """
+    def getBody(self):
         # Use the context attribute 'content' as data to return.
         # Avoid open the CoC file again.
-        content = self.context.content
+        return self.context.content
 
+    @property
+    def filename(self):
         # Build a fancy filename:
         # - Use title with no spaces and append '.txt'
-        filename = self.context.title.replace(' ', '') + '.txt'
-
-        self.request.response.setHeader('Content-Type', 'application/text')
-        self.request.response.setHeader('Content-Length', len(content))
-        self.request.response.setHeader(
-            'Content-Disposition', 'attachment; filename="%s"' % filename)
-        return content
+        return self.context.title.replace(' ', '') + '.txt'
 
 
 class CodeOfConductSetView(LaunchpadView):
 
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-01-25 04:56:50 +0000
+++ lib/lp/registry/browser/person.py	2012-01-27 19:21:54 +0000
@@ -1195,7 +1195,7 @@
 
     @property
     def filename(self):
-        return self.context.name
+        return '%s.rdf' % self.context.name
 
 
 class PersonRdfContentsView:
 
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/product.py	2012-01-27 19:21:54 +0000
@@ -1814,7 +1814,7 @@
 
     @property
     def filename(self):
-        return self.context.name
+        return '%s.rdf' % self.context.name
 
 
 class Icon:
 
=== modified file 'lib/lp/registry/browser/productrelease.py'
--- lib/lp/registry/browser/productrelease.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/productrelease.py	2012-01-27 19:21:54 +0000
@@ -263,7 +263,7 @@
 
     @property
     def filename(self):
-        return '%s-%s-%s' % (
+        return '%s-%s-%s.rdf' % (
             self.context.product.name,
             self.context.productseries.name,
             self.context.version)
 
=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/productseries.py	2012-01-27 19:21:54 +0000
@@ -1240,7 +1240,7 @@
 
     @property
     def filename(self):
-        return '%s-%s' % (self.context.product.name, self.context.name)
+        return '%s-%s.rdf' % (self.context.product.name, self.context.name)
 
 
 class ProductSeriesFileBugRedirect(LaunchpadView):
 
=== modified file 'lib/lp/registry/browser/tests/test_codeofconduct.py'
--- lib/lp/registry/browser/tests/test_codeofconduct.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_codeofconduct.py	2012-01-27 19:21:54 +0000
@@ -7,9 +7,13 @@
 
 from zope.component import getUtility
 
-from lp.registry.interfaces.codeofconduct import ISignedCodeOfConductSet
+from lp.registry.interfaces.codeofconduct import (
+    ICodeOfConductSet,
+    ISignedCodeOfConductSet,
+    )
 from lp.registry.model.codeofconduct import SignedCodeOfConduct
 from lp.testing import (
+    BrowserTestCase,
     login_celebrity,
     TestCaseWithFactory,
     )
@@ -147,3 +151,20 @@
 
     def test_admincomment_required(self):
         self.verify_admincomment_required('Deactivate', '+deactivate')
+
+
+class TestCodeOfConductBrowser(BrowserTestCase):
+    """Test the download view for the CoC."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_response(self):
+        """Ensure the headers and body are as expected."""
+        coc = getUtility(ICodeOfConductSet)['1.1']
+        content = coc.content
+        browser = self.getViewBrowser(coc, '+download')
+        self.assertEqual(content, browser.contents)
+        self.assertEqual(str(len(content)), browser.headers['Content-length'])
+        disposition = 'attachment; filename="UbuntuCodeofConduct-1.1.txt"'
+        self.assertEqual(disposition, browser.headers['Content-disposition'])
+        self.assertEqual('text/plain', browser.headers['Content-type'])
 
=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2012-01-27 19:21:54 +0000
@@ -11,6 +11,7 @@
 from lp.services.config import config
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
+    BrowserTestCase,
     login_person,
     TestCaseWithFactory,
     )
@@ -50,3 +51,19 @@
             '''))
         self.assertEquals(
             'http://prod.launchpad.dev/~eris', self.view.openid_identity_url)
+
+
+class TestPersonRdfView(BrowserTestCase):
+    """Test the RDF view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_headers(self):
+        """The headers for the RDF view of a person should be as expected."""
+        person = self.factory.makePerson()
+        content_disposition = 'attachment; filename="%s.rdf"' % person.name
+        browser = self.getViewBrowser(person, view_name='+rdf')
+        self.assertEqual(
+            content_disposition, browser.headers['Content-disposition'])
+        self.assertEqual(
+            'application/rdf+xml', browser.headers['Content-type'])
 
=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-01-27 19:21:54 +0000
@@ -20,6 +20,7 @@
 from lp.services.config import config
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
+    BrowserTestCase,
     login_celebrity,
     login_person,
     person_logged_in,
@@ -386,3 +387,19 @@
         self.assertTrue(
             'Y.lp.app.choice.addBinaryChoice' in str(
                 content.find(id='fnord-edit-license-approved').parent))
+
+
+class TestProductRdfView(BrowserTestCase):
+    """Test the Product RDF view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_headers(self):
+        """The headers for the RDF view of a product should be as expected."""
+        product = self.factory.makeProduct()
+        browser = self.getViewBrowser(product, view_name='+rdf')
+        content_disposition = 'attachment; filename="%s.rdf"' % product.name
+        self.assertEqual(
+            content_disposition, browser.headers['Content-disposition'])
+        self.assertEqual(
+            'application/rdf+xml', browser.headers['Content-type'])
 
=== modified file 'lib/lp/registry/stories/productrelease/xx-productrelease-rdf.txt'
--- lib/lp/registry/stories/productrelease/xx-productrelease-rdf.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/registry/stories/productrelease/xx-productrelease-rdf.txt	2012-01-27 19:21:54 +0000
@@ -1,13 +1,13 @@
 Check that the productrelease RDF export works.
 
-  >>> print http(r"""
-  ... GET /firefox/trunk/0.9/+rdf HTTP/1.1
-  ... """)
-  HTTP/1.1 200 Ok
-  Content-Disposition: attachment; filename=firefox-trunk-0.9.rdf
-  Content-Length: ...
-  Content-Type: application/rdf+xml
-  Vary: ...
-  <BLANKLINE>
-  <?xml version="1.0" encoding="utf-8"...?>
-  <rdf:RDF ...
+    >>> print http(r"""
+    ... GET /firefox/trunk/0.9/+rdf HTTP/1.1
+    ... """)
+    HTTP/1.1 200 Ok
+    Content-Disposition: attachment; filename="firefox-trunk-0.9.rdf"
+    Content-Length: ...
+    Content-Type: application/rdf+xml
+    Vary: ...
+    <BLANKLINE>
+    <?xml version="1.0" encoding="utf-8"...?>
+    <rdf:RDF ...
 
=== modified file 'lib/lp/registry/stories/productseries/xx-productseries-rdf.txt'
--- lib/lp/registry/stories/productseries/xx-productseries-rdf.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/registry/stories/productseries/xx-productseries-rdf.txt	2012-01-27 19:21:54 +0000
@@ -1,13 +1,13 @@
 Check that the productseries RDF export works.
 
-  >>> print http(r"""
-  ... GET /firefox/trunk/+rdf HTTP/1.1
-  ... """)
-  HTTP/1.1 200 Ok
-  Content-Disposition: attachment; filename=firefox-trunk.rdf
-  Content-Length: ...
-  Content-Type: application/rdf+xml
-  Vary: ...
-  <BLANKLINE>
-  <?xml version="1.0" encoding="utf-8"...?>
-  <rdf:RDF ...
+    >>> print http(r"""
+    ... GET /firefox/trunk/+rdf HTTP/1.1
+    ... """)
+    HTTP/1.1 200 Ok
+    Content-Disposition: attachment; filename="firefox-trunk.rdf"
+    Content-Length: ...
+    Content-Type: application/rdf+xml
+    Vary: ...
+    <BLANKLINE>
+    <?xml version="1.0" encoding="utf-8"...?>
+    <rdf:RDF ...
 
=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py	2012-01-23 04:05:17 +0000
+++ lib/lp/services/webapp/publisher.py	2012-01-27 19:21:54 +0000
@@ -7,6 +7,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'DataDownloadView',
     'LaunchpadContainer',
     'LaunchpadView',
     'LaunchpadXMLRPCView',
@@ -220,6 +221,29 @@
         return cls
 
 
+class DataDownloadView:
+    """Download data without templating.
+
+    Subclasses must provide getBody, content_type and filename.
+    """
+
+    def __init__(self, context, request):
+        self.context = context
+        self.request = request
+
+    def __call__(self):
+        """Set the headers and return the body.
+
+        It is not necessary to supply Content-length, because this is added by
+        the caller.
+        """
+        self.request.response.setHeader('Content-Type', self.content_type)
+        self.request.response.setHeader(
+            'Content-Disposition', 'attachment; filename="%s"' % (
+             self.filename))
+        return self.getBody()
+
+
 class UserAttributeCache:
     """Mix in to provide self.user, cached."""