Merge lp:~mwhudson/launchpad/vostok-traverse-distro into lp:launchpad

[Michael Hudson-Doyle]

Add a navigation for the vostok root that allows you to traverse to distributions

Hi,

This branch adds a navigation for the vostok root that allows you to traverse
to distributions.

Cheers,
mwh

[Tim Penhey (thumper)]

Agh - perhaps it is just me, but __get_item__ on I<any>Set just seems wrong to
me.

> return self.redirectSubTree(canonical_url(distro), status=301)

That will take the user out of the vostok vhost. Is that what you want?

If not, you probably want to set the root as part of the canonical_url call.

review: Needs Information

[Robert Collins (lifeless)]

This change is faulty:

- 'Unrecognized branch type: %r' % (branch_type,))
+ 'Unrecognized branch type: %r' % branch_type)

if branch_type is a tuple with length != 1, it will blow up in your version, and for a error code path thats undesirable. I haven't done a full review - was glancing in email and this jumped out at me.

review: Needs Fixing

[Tim Penhey (thumper)]

On Fri, 30 Jul 2010 16:04:43 you wrote:
> Review: Needs Fixing
>
> This change is faulty:
>
> - 'Unrecognized branch type: %r' % (branch_type,))
> + 'Unrecognized branch type: %r' % branch_type)
>
> if branch_type is a tuple with length != 1, it will blow up in your
> version, and for a error code path thats undesirable. I haven't done a
> full review - was glancing in email and this jumped out at me.

Yeah, but branch_type is always a BranchType enum value, which is why I let
this slide.

[Michael Hudson-Doyle (mwhudson)]

On Thu, 29 Jul 2010 23:44:46 -0000, Tim Penhey <email address hidden> wrote:
> Review: Needs Information
> Agh - perhaps it is just me, but __get_item__ on I<any>Set just seems wrong to
> me.

Changed to getByName.

> > return self.redirectSubTree(canonical_url(distro), status=301)
>
> That will take the user out of the vostok vhost. Is that what you want?

Actually it won't.

> If not, you probably want to set the root as part of the canonical_url call.

I think canonical_url looks at the current request to find the root
site.

I also made the change Rob suggested.

Cheers,
mwh

[Robert Collins (lifeless)]

On Fri, Jul 30, 2010 at 6:33 AM, Tim Penhey <email address hidden> wrote:

>> if branch_type is a tuple with length != 1, it will blow up in your
>> version, and for a error code path thats undesirable. I haven't done a
>> full review - was glancing in email and this jumped out at me.
>
> Yeah, but branch_type is always a BranchType enum value, which is why I let
> this slide.

From IRC - its good to keep reflexes from having to deal with false
signals - while its true here that its always not a tuple, in the
general case we should be wary of direct substitution like this, so
its nice to have it clearly safe.

-Rob

[Guilherme Salgado (salgado)]

lib/lp/vostok/tests/test_navigation.py will break when you merge devel because NotFoundError was moved to lp.app.errors. I've noticed that because I have a branch which adds more stuff on top of this one.

[Tim Penhey (thumper)]

You should import NotFound from lp.app.errors, but apart from that it looks good.

review: Approve

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-08-05 21:25:12 +0000
+++ lib/lp/testing/factory.py	2010-08-06 02:38:50 +0000
@@ -1669,7 +1669,7 @@
         return library_file_alias
 
     def makeDistribution(self, name=None, displayname=None, owner=None,
-                         members=None, title=None):
+                         members=None, title=None, aliases=None):
         """Make a new distribution."""
         if name is None:
             name = self.getUniqueString()
@@ -1684,9 +1684,12 @@
             owner = self.makePerson()
         if members is None:
             members = self.makeTeam(owner)
-        return getUtility(IDistributionSet).new(
+        distro = getUtility(IDistributionSet).new(
             name, displayname, title, description, summary, domainname,
             members, owner)
+        if aliases is not None:
+            removeSecurityProxy(distro).setAliases(aliases)
+        return distro
 
     def makeDistroRelease(self, distribution=None, version=None,
                           status=SeriesStatus.DEVELOPMENT,
 
=== modified file 'lib/lp/vostok/browser/configure.zcml'
--- lib/lp/vostok/browser/configure.zcml	2010-07-15 08:55:32 +0000
+++ lib/lp/vostok/browser/configure.zcml	2010-08-06 02:38:50 +0000
@@ -27,4 +27,9 @@
      layer="lp.vostok.publisher.VostokLayer"
      />
 
+  <browser:navigation
+      module="lp.vostok.publisher"
+      classes="VostokRootNavigation"
+      />
+
 </configure>
 
=== modified file 'lib/lp/vostok/publisher.py'
--- lib/lp/vostok/publisher.py	2010-07-30 03:50:17 +0000
+++ lib/lp/vostok/publisher.py	2010-08-06 02:38:50 +0000
@@ -7,19 +7,24 @@
 __all__ = [
     'VostokBrowserRequest',
     'VostokLayer',
+    'VostokRootNavigation',
     'vostok_request_publication_factory',
     ]
 
 
+from zope.component import getUtility
 from zope.interface import implements, Interface
 from zope.publisher.interfaces.browser import (
     IBrowserRequest, IDefaultBrowserLayer)
 
+from canonical.launchpad.webapp import canonical_url, Navigation
 from canonical.launchpad.webapp.publication import LaunchpadBrowserPublication
 from canonical.launchpad.webapp.servers import (
     LaunchpadBrowserRequest, VirtualHostRequestPublicationFactory)
 from canonical.launchpad.webapp.vhosts import allvhosts
 
+from lp.registry.interfaces.distribution import IDistributionSet
+
 
 class VostokLayer(IBrowserRequest, IDefaultBrowserLayer):
     """The Vostok layer."""
@@ -53,6 +58,19 @@
     implements(IVostokRoot)
 
 
+class VostokRootNavigation(Navigation):
+
+    usedfor = IVostokRoot
+
+    def traverse(self, name):
+        distro = getUtility(IDistributionSet)[name]
+        if distro is not None and distro.name != name:
+            # This distro was accessed through one of its aliases, so we
+            # must redirect to its canonical URL.
+            return self.redirectSubTree(canonical_url(distro), status=301)
+        return distro
+
+
 class VostokBrowserPublication(LaunchpadBrowserPublication):
     root_object_interface = IVostokRoot
 
 
=== added file 'lib/lp/vostok/tests/test_navigation.py'
--- lib/lp/vostok/tests/test_navigation.py	1970-01-01 00:00:00 +0000
+++ lib/lp/vostok/tests/test_navigation.py	2010-08-06 02:38:50 +0000
@@ -0,0 +1,43 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for vostok's root navigation."""
+
+__metaclass__ = type
+
+from zope.publisher.interfaces import NotFound
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.webapp import urlparse
+from canonical.testing.layers import DatabaseFunctionalLayer
+
+from lp.testing import TestCaseWithFactory
+from lp.testing.publication import test_traverse
+
+
+class TestRootNavigation(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_traverse_to_distributions(self):
+        # We can traverse to a distribution by name from the vostok
+        # root.
+        distro = self.factory.makeDistribution()
+        obj, view, request = test_traverse('http://vostok.dev/' + distro.name)
+        self.assertEqual(distro, obj)
+
+    def test_traverse_to_distribution_aliases(self):
+        # When we traverse to a distribution using one of its aliases, we're
+        # redirected to the distribution's page on the vostok vhost.
+        distro = self.factory.makeDistribution(aliases=['f00'])
+        obj, view, request = test_traverse('http://vostok.dev/f00')
+        naked_view = removeSecurityProxy(view)
+        parse_result = urlparse(naked_view.target)
+        self.assertEqual('vostok.dev', parse_result.netloc)
+        self.assertEqual('/' + distro.name, parse_result.path)
+
+    def test_can_not_traverse_to_projects(self):
+        # We cannot traverse to a project from the vostok root.
+        path = self.factory.makeProject().name
+        self.assertRaises(
+            NotFound, test_traverse, 'http://vostok.dev/' + path)