launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/lfav-deleted into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/lfav-deleted/+merge/66275

Instead of asserting that a LibraryFileAlias is not deleted in the inititalize() method of LibraryFileAliasView, return a 410. I'd argue that 410 is a better status code than 404 in this case, since the object is not 'Not Found', since it does still exist, it just has no content. Hence, gone.
-- 
https://code.launchpad.net/~stevenk/launchpad/lfav-deleted/+merge/66275
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/lfav-deleted into lp:launchpad.

=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py	2011-04-27 04:16:20 +0000
+++ lib/canonical/launchpad/browser/librarian.py	2011-06-29 10:51:53 +0000
@@ -13,6 +13,8 @@
     'ProxiedLibraryFileAlias',
     ]
 
+import httplib
+
 from lazr.delegates import delegates
 from zope.publisher.interfaces import NotFound
 from zope.security.interfaces import Unauthorized
@@ -45,13 +47,10 @@
         # Refuse to serve restricted files. We're not sure that no
         # restricted files are being leaked in the traversal hierarchy.
         assert not self.context.restricted
-        # Perhaps we should give a 404 at this point rather than asserting?
-        # If someone has a page open with an attachment link, then someone
-        # else deletes the attachment, this is a normal situation, not an
-        # error. -- RBC 20100726.
-        assert not self.context.deleted, (
-            "LibraryFileAliasView can not operate on deleted librarian files,"
-            " since their URL is undefined.")
+        # If the LFA is deleted, throw a 410.
+        if self.context.deleted:
+            self.request.response.setStatus(httplib.GONE)
+            return
         # Redirect based on the scheme of the request, as set by
         # Apache in the 'X-SCHEME' environment variable, which is
         # mapped to 'HTTP_X_SCHEME.  Note that only some requests

=== added file 'lib/canonical/launchpad/browser/tests/test_librarianfilealiasview.py'
--- lib/canonical/launchpad/browser/tests/test_librarianfilealiasview.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/browser/tests/test_librarianfilealiasview.py	2011-06-29 10:51:53 +0000
@@ -0,0 +1,31 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test LibraryFileAliasView."""
+
+__metaclass__ = type
+
+import httplib
+
+from zope.component import getMultiAdapter
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.testing import TestCaseWithFactory
+
+
+class TestLibraryFileAliasView(TestCaseWithFactory):
+    layer = LaunchpadFunctionalLayer
+
+    def test_deleted_lfa(self):
+        # When we initialise a LibraryFileAliasView against a deleted LFA,
+        # we throw a 410 Gone error.
+        lfa = self.factory.makeLibraryFileAlias()
+        removeSecurityProxy(lfa).content = None
+        self.assertTrue(lfa.deleted)
+        request = LaunchpadTestRequest(
+            environ={'REQUEST_METHOD': 'GET', 'HTTP_X_SCHEME' : 'http' })
+        view = getMultiAdapter((lfa, request), name='+index')
+        view.initialize()
+        self.assertEqual(httplib.GONE, view.request.response.getStatus())