Merge lp:~abentley/launchpad/broken-xmlrpc-lookup into lp:launchpad

Proposed by Aaron Bentley
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 15654
Proposed branch: lp:~abentley/launchpad/broken-xmlrpc-lookup
Merge into: lp:launchpad
Diff against target: 118 lines (+19/-28)
4 files modified
lib/lp/app/browser/tests/test_launchpad.py (+8/-9)
lib/lp/code/interfaces/branchlookup.py (+0/-2)
lib/lp/code/model/branchlookup.py (+8/-6)
lib/lp/code/model/tests/test_branchlookup.py (+3/-11)
To merge this branch: bzr merge lp:~abentley/launchpad/broken-xmlrpc-lookup
Reviewer Review Type Date Requested Status
Deryck Hodge (community) Approve
Review via email: mp+115582@code.launchpad.net

Commit message

Fix +branch/product handling in xmlrpc & loggerhead.

Description of the change

= Summary =
Fix bug #1025368: Codehosting xmlrpc not handling project with trailing path

== Proposed fix ==
Change getByLPPath to handle ProductSeries traversal failures by looking up as a Product.

== Pre-implementation notes ==
Discussed the inherent abiguity of +branch aliases with deryck

== LOC Rationale ==
Removes 9 lines.

== Implementation details ==
Removed stale comments (some support for trailing paths had already been added.)
Special-casing for .bzr pathnames can be removed, since now all paths under a product will fall back to the product if there is no such ProductSeries.

== Tests ==
bin/test -t test_alias_trailing_path_redirect -t test_too_long_product

== Demo and Q/A ==
Go to https://bazaar.qastaging.launchpad.net/+branch/bzrtools/changes
You should see the normal Loggerhead view for a branch. (The same as https://bazaar.qastaging.launchpad.net/~abentley/bzrtools/bzrtools.dev/changes)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/tests/test_branchlookup.py
  lib/lp/code/model/branchlookup.py
  lib/lp/code/interfaces/branchlookup.py
  lib/lp/app/browser/tests/test_launchpad.py

To post a comment you must log in.
Revision history for this message
Deryck Hodge (deryck) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/app/browser/tests/test_launchpad.py'
2--- lib/lp/app/browser/tests/test_launchpad.py 2012-05-31 03:54:13 +0000
3+++ lib/lp/app/browser/tests/test_launchpad.py 2012-07-18 16:13:26 +0000
4@@ -271,6 +271,14 @@
5 path = urlappend(branch.unique_name, '+edit')
6 self.assertRedirects(path, canonical_url(branch, view_name='+edit'))
7
8+ def test_alias_trailing_path_redirect(self):
9+ # Redirects also support trailing path segments with aliases.
10+ branch = self.factory.makeProductBranch()
11+ with person_logged_in(branch.product.owner):
12+ branch.product.development_focus.branch = branch
13+ path = '%s/+edit' % branch.product.name
14+ self.assertRedirects(path, canonical_url(branch, view_name='+edit'))
15+
16 def test_product_series_redirect(self):
17 # Traversing to /+branch/<product>/<series> redirects to the branch
18 # for that series, if there is one.
19@@ -279,15 +287,6 @@
20 self.assertRedirects(
21 ICanHasLinkedBranch(series).bzr_path, canonical_url(branch))
22
23- def test_nonexistent_product_series(self):
24- # /+branch/<product>/<series> displays an error message if there is
25- # no such series.
26- product = self.factory.makeProduct()
27- non_existent = 'nonexistent'
28- requiredMessage = u"No such product series: '%s'." % non_existent
29- path = '%s/%s' % (product.name, non_existent)
30- self.assertDisplaysError(path, requiredMessage)
31-
32 def test_no_branch_for_series(self):
33 # If there's no branch for a product series, display a
34 # message telling the user there is no linked branch.
35
36=== modified file 'lib/lp/code/interfaces/branchlookup.py'
37--- lib/lp/code/interfaces/branchlookup.py 2012-07-04 20:45:06 +0000
38+++ lib/lp/code/interfaces/branchlookup.py 2012-07-18 16:13:26 +0000
39@@ -142,8 +142,6 @@
40 component of the path.
41 :raises NoSuchProduct: If we can't find a product that matches the
42 product component of the path.
43- :raises NoSuchProductSeries: If the product series component doesn't
44- match an existing series.
45 :raises NoSuchDistroSeries: If the distro series component doesn't
46 match an existing series.
47 :raises NoSuchSourcePackageName: If the source packagae referred to
48
49=== modified file 'lib/lp/code/model/branchlookup.py'
50--- lib/lp/code/model/branchlookup.py 2012-07-05 14:53:47 +0000
51+++ lib/lp/code/model/branchlookup.py 2012-07-18 16:13:26 +0000
52@@ -9,8 +9,6 @@
53 __all__ = []
54
55
56-import re
57-
58 from bzrlib.urlutils import escape
59 from lazr.enum import DBItem
60 from lazr.uri import (
61@@ -399,10 +397,14 @@
62 else:
63 # If the first element doesn't start with a tilde, then maybe
64 # 'path' is a shorthand notation for a branch.
65- # Ignore anything following /.bzr
66- prefix = re.match('^(.*?)(/?.bzr(/.*)?)?$', path).group(1)
67- object_with_branch_link = getUtility(
68- ILinkedBranchTraverser).traverse(prefix)
69+ try:
70+ object_with_branch_link = getUtility(
71+ ILinkedBranchTraverser).traverse(path)
72+ except NoSuchProductSeries as e:
73+ # If ProductSeries lookup failed, the segment after product
74+ # name referred to a location under a Product development
75+ # focus branch.
76+ object_with_branch_link = e.product
77 branch, bzr_path = self._getLinkedBranchAndPath(
78 object_with_branch_link)
79 suffix = path[len(bzr_path) + 1:]
80
81=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
82--- lib/lp/code/model/tests/test_branchlookup.py 2012-07-03 19:42:30 +0000
83+++ lib/lp/code/model/tests/test_branchlookup.py 2012-07-18 16:13:26 +0000
84@@ -711,9 +711,9 @@
85 # between a trailing path and an attempt to load a non-existent series
86 # branch.
87 product = make_product_with_branch(self.factory)
88- self.assertRaises(
89- NoSuchProductSeries,
90- self.branch_lookup.getByLPPath, '%s/other/bits' % product.name)
91+ self.assertEqual(
92+ (product.development_focus.branch, 'other/bits'),
93+ self.branch_lookup.getByLPPath('%s/other/bits' % product.name))
94
95 def test_product_with_bzr_suffix(self):
96 # A '.bzr' suffix is returned correctly.
97@@ -737,10 +737,6 @@
98 self.assertEqual('.bzr/extra', suffix)
99
100 def test_too_long_product_series(self):
101- # If the provided path points to an existing product series with a
102- # linked branch but is followed by extra path segments, then we return
103- # the linked branch but chop off the extra segments. We might want to
104- # change this behaviour in future.
105 branch = self.factory.makeBranch()
106 series = self.factory.makeProductSeries(branch=branch)
107 result = self.branch_lookup.getByLPPath(
108@@ -748,10 +744,6 @@
109 self.assertEqual((branch, u'other/bits'), result)
110
111 def test_too_long_sourcepackage(self):
112- # If the provided path points to an existing source package with a
113- # linked branch but is followed by extra path segments, then we return
114- # the linked branch but chop off the extra segments. We might want to
115- # change this behaviour in future.
116 package = self.factory.makeSourcePackage()
117 branch = self.factory.makePackageBranch(sourcepackage=package)
118 with person_logged_in(package.distribution.owner):