launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/broken-xmlrpc-lookup into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1025368 in Launchpad itself: "Codehosting xmlrpc not handling project with trailing path"
  https://bugs.launchpad.net/launchpad/+bug/1025368

For more details, see:
https://code.launchpad.net/~abentley/launchpad/broken-xmlrpc-lookup/+merge/115582

= 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
-- 
https://code.launchpad.net/~abentley/launchpad/broken-xmlrpc-lookup/+merge/115582
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/broken-xmlrpc-lookup into lp:launchpad.

=== modified file 'lib/lp/app/browser/tests/test_launchpad.py'
--- lib/lp/app/browser/tests/test_launchpad.py	2012-05-31 03:54:13 +0000
+++ lib/lp/app/browser/tests/test_launchpad.py	2012-07-18 16:13:26 +0000
@@ -271,6 +271,14 @@
         path = urlappend(branch.unique_name, '+edit')
         self.assertRedirects(path, canonical_url(branch, view_name='+edit'))
 
+    def test_alias_trailing_path_redirect(self):
+        # Redirects also support trailing path segments with aliases.
+        branch = self.factory.makeProductBranch()
+        with person_logged_in(branch.product.owner):
+            branch.product.development_focus.branch = branch
+        path = '%s/+edit' % branch.product.name
+        self.assertRedirects(path, canonical_url(branch, view_name='+edit'))
+
     def test_product_series_redirect(self):
         # Traversing to /+branch/<product>/<series> redirects to the branch
         # for that series, if there is one.
@@ -279,15 +287,6 @@
         self.assertRedirects(
             ICanHasLinkedBranch(series).bzr_path, canonical_url(branch))
 
-    def test_nonexistent_product_series(self):
-        # /+branch/<product>/<series> displays an error message if there is
-        # no such series.
-        product = self.factory.makeProduct()
-        non_existent = 'nonexistent'
-        requiredMessage = u"No such product series: '%s'." % non_existent
-        path = '%s/%s' % (product.name, non_existent)
-        self.assertDisplaysError(path, requiredMessage)
-
     def test_no_branch_for_series(self):
         # If there's no branch for a product series, display a
         # message telling the user there is no linked branch.

=== modified file 'lib/lp/code/interfaces/branchlookup.py'
--- lib/lp/code/interfaces/branchlookup.py	2012-07-04 20:45:06 +0000
+++ lib/lp/code/interfaces/branchlookup.py	2012-07-18 16:13:26 +0000
@@ -142,8 +142,6 @@
             component of the path.
         :raises NoSuchProduct: If we can't find a product that matches the
             product component of the path.
-        :raises NoSuchProductSeries: If the product series component doesn't
-            match an existing series.
         :raises NoSuchDistroSeries: If the distro series component doesn't
             match an existing series.
         :raises NoSuchSourcePackageName: If the source packagae referred to

=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py	2012-07-05 14:53:47 +0000
+++ lib/lp/code/model/branchlookup.py	2012-07-18 16:13:26 +0000
@@ -9,8 +9,6 @@
 __all__ = []
 
 
-import re
-
 from bzrlib.urlutils import escape
 from lazr.enum import DBItem
 from lazr.uri import (
@@ -399,10 +397,14 @@
         else:
             # If the first element doesn't start with a tilde, then maybe
             # 'path' is a shorthand notation for a branch.
-            # Ignore anything following /.bzr
-            prefix = re.match('^(.*?)(/?.bzr(/.*)?)?$', path).group(1)
-            object_with_branch_link = getUtility(
-                ILinkedBranchTraverser).traverse(prefix)
+            try:
+                object_with_branch_link = getUtility(
+                    ILinkedBranchTraverser).traverse(path)
+            except NoSuchProductSeries as e:
+                # If ProductSeries lookup failed, the segment after product
+                # name referred to a location under a Product development
+                # focus branch.
+                object_with_branch_link = e.product
             branch, bzr_path = self._getLinkedBranchAndPath(
                 object_with_branch_link)
             suffix = path[len(bzr_path) + 1:]

=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py	2012-07-03 19:42:30 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py	2012-07-18 16:13:26 +0000
@@ -711,9 +711,9 @@
         # between a trailing path and an attempt to load a non-existent series
         # branch.
         product = make_product_with_branch(self.factory)
-        self.assertRaises(
-            NoSuchProductSeries,
-            self.branch_lookup.getByLPPath, '%s/other/bits' % product.name)
+        self.assertEqual(
+            (product.development_focus.branch, 'other/bits'),
+            self.branch_lookup.getByLPPath('%s/other/bits' % product.name))
 
     def test_product_with_bzr_suffix(self):
         # A '.bzr' suffix is returned correctly.
@@ -737,10 +737,6 @@
         self.assertEqual('.bzr/extra', suffix)
 
     def test_too_long_product_series(self):
-        # If the provided path points to an existing product series with a
-        # linked branch but is followed by extra path segments, then we return
-        # the linked branch but chop off the extra segments. We might want to
-        # change this behaviour in future.
         branch = self.factory.makeBranch()
         series = self.factory.makeProductSeries(branch=branch)
         result = self.branch_lookup.getByLPPath(
@@ -748,10 +744,6 @@
         self.assertEqual((branch, u'other/bits'), result)
 
     def test_too_long_sourcepackage(self):
-        # If the provided path points to an existing source package with a
-        # linked branch but is followed by extra path segments, then we return
-        # the linked branch but chop off the extra segments. We might want to
-        # change this behaviour in future.
         package = self.factory.makeSourcePackage()
         branch = self.factory.makePackageBranch(sourcepackage=package)
         with person_logged_in(package.distribution.owner):