Launchpad itself

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

  1. vostok-traverse-distro
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 11327
Proposed branch: lp:~mwhudson/launchpad/vostok-traverse-distro
Merge into: lp:launchpad
Prerequisite: lp:~mwhudson/launchpad/vostok-main-template
Diff against target: 137 lines (+71/-2)
4 files modified
lib/lp/testing/factory.py (+5/-2)
lib/lp/vostok/browser/configure.zcml (+5/-0)
lib/lp/vostok/publisher.py (+18/-0)
lib/lp/vostok/tests/test_navigation.py (+43/-0)
To merge this branch: bzr merge lp:~mwhudson/launchpad/vostok-traverse-distro
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Robert Collins (community) Needs Fixing
Review via email: mp+31241@code.launchpad.net

Commit message

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

Description of the change

Hi,

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

Cheers,
mwh

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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
Revision history for this message
Robert Collins (lifeless) wrote :

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
Revision history for this message
Tim Penhey (thumper) wrote :

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.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Guilherme Salgado (salgado) wrote :

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.

Revision history for this message
Tim Penhey (thumper) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/testing/factory.py'
2--- lib/lp/testing/factory.py 2010-08-05 21:25:12 +0000
3+++ lib/lp/testing/factory.py 2010-08-06 02:38:50 +0000
4@@ -1669,7 +1669,7 @@
5 return library_file_alias
6
7 def makeDistribution(self, name=None, displayname=None, owner=None,
8- members=None, title=None):
9+ members=None, title=None, aliases=None):
10 """Make a new distribution."""
11 if name is None:
12 name = self.getUniqueString()
13@@ -1684,9 +1684,12 @@
14 owner = self.makePerson()
15 if members is None:
16 members = self.makeTeam(owner)
17- return getUtility(IDistributionSet).new(
18+ distro = getUtility(IDistributionSet).new(
19 name, displayname, title, description, summary, domainname,
20 members, owner)
21+ if aliases is not None:
22+ removeSecurityProxy(distro).setAliases(aliases)
23+ return distro
24
25 def makeDistroRelease(self, distribution=None, version=None,
26 status=SeriesStatus.DEVELOPMENT,
27
28=== modified file 'lib/lp/vostok/browser/configure.zcml'
29--- lib/lp/vostok/browser/configure.zcml 2010-07-15 08:55:32 +0000
30+++ lib/lp/vostok/browser/configure.zcml 2010-08-06 02:38:50 +0000
31@@ -27,4 +27,9 @@
32 layer="lp.vostok.publisher.VostokLayer"
33 />
34
35+ <browser:navigation
36+ module="lp.vostok.publisher"
37+ classes="VostokRootNavigation"
38+ />
39+
40 </configure>
41
42=== modified file 'lib/lp/vostok/publisher.py'
43--- lib/lp/vostok/publisher.py 2010-07-30 03:50:17 +0000
44+++ lib/lp/vostok/publisher.py 2010-08-06 02:38:50 +0000
45@@ -7,19 +7,24 @@
46 __all__ = [
47 'VostokBrowserRequest',
48 'VostokLayer',
49+ 'VostokRootNavigation',
50 'vostok_request_publication_factory',
51 ]
52
53
54+from zope.component import getUtility
55 from zope.interface import implements, Interface
56 from zope.publisher.interfaces.browser import (
57 IBrowserRequest, IDefaultBrowserLayer)
58
59+from canonical.launchpad.webapp import canonical_url, Navigation
60 from canonical.launchpad.webapp.publication import LaunchpadBrowserPublication
61 from canonical.launchpad.webapp.servers import (
62 LaunchpadBrowserRequest, VirtualHostRequestPublicationFactory)
63 from canonical.launchpad.webapp.vhosts import allvhosts
64
65+from lp.registry.interfaces.distribution import IDistributionSet
66+
67
68 class VostokLayer(IBrowserRequest, IDefaultBrowserLayer):
69 """The Vostok layer."""
70@@ -53,6 +58,19 @@
71 implements(IVostokRoot)
72
73
74+class VostokRootNavigation(Navigation):
75+
76+ usedfor = IVostokRoot
77+
78+ def traverse(self, name):
79+ distro = getUtility(IDistributionSet)[name]
80+ if distro is not None and distro.name != name:
81+ # This distro was accessed through one of its aliases, so we
82+ # must redirect to its canonical URL.
83+ return self.redirectSubTree(canonical_url(distro), status=301)
84+ return distro
85+
86+
87 class VostokBrowserPublication(LaunchpadBrowserPublication):
88 root_object_interface = IVostokRoot
89
90
91=== added file 'lib/lp/vostok/tests/test_navigation.py'
92--- lib/lp/vostok/tests/test_navigation.py 1970-01-01 00:00:00 +0000
93+++ lib/lp/vostok/tests/test_navigation.py 2010-08-06 02:38:50 +0000
94@@ -0,0 +1,43 @@
95+# Copyright 2010 Canonical Ltd. This software is licensed under the
96+# GNU Affero General Public License version 3 (see the file LICENSE).
97+
98+"""Tests for vostok's root navigation."""
99+
100+__metaclass__ = type
101+
102+from zope.publisher.interfaces import NotFound
103+from zope.security.proxy import removeSecurityProxy
104+
105+from canonical.launchpad.webapp import urlparse
106+from canonical.testing.layers import DatabaseFunctionalLayer
107+
108+from lp.testing import TestCaseWithFactory
109+from lp.testing.publication import test_traverse
110+
111+
112+class TestRootNavigation(TestCaseWithFactory):
113+
114+ layer = DatabaseFunctionalLayer
115+
116+ def test_traverse_to_distributions(self):
117+ # We can traverse to a distribution by name from the vostok
118+ # root.
119+ distro = self.factory.makeDistribution()
120+ obj, view, request = test_traverse('http://vostok.dev/' + distro.name)
121+ self.assertEqual(distro, obj)
122+
123+ def test_traverse_to_distribution_aliases(self):
124+ # When we traverse to a distribution using one of its aliases, we're
125+ # redirected to the distribution's page on the vostok vhost.
126+ distro = self.factory.makeDistribution(aliases=['f00'])
127+ obj, view, request = test_traverse('http://vostok.dev/f00')
128+ naked_view = removeSecurityProxy(view)
129+ parse_result = urlparse(naked_view.target)
130+ self.assertEqual('vostok.dev', parse_result.netloc)
131+ self.assertEqual('/' + distro.name, parse_result.path)
132+
133+ def test_can_not_traverse_to_projects(self):
134+ # We cannot traverse to a project from the vostok root.
135+ path = self.factory.makeProject().name
136+ self.assertRaises(
137+ NotFound, test_traverse, 'http://vostok.dev/' + path)