launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/tales-linkify-broken-links into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #404131 tales linkify create broken links 
  https://bugs.launchpad.net/bugs/404131


Bug 404131 describes some issues with the linkification functionality of tales.FormattersAPI. If a branch URL refers to a product where the development focus is a private branch, a 404 error occurs. Similarly, if no development focus has been set, an error occurs. 

Proposed fix

The redirect_branch() method in lib/canonical/launchpad/browser/launchpad.py tries to resolve the URL as a branch and simply catches a NotFound exception when the URL is invalid (ether non existsent branch or totally bogus URL). The behaviour of this method can be enhanced to:

1. Catch the NoLinkedBranch exception if a branch cannot be resolved and attempt to find an ICanHasLinkedBranch target instead
2. Add a page notification if an ICanHasLinkedBranch target is resolved instead of a branch
3. Add a page error and stay of the current page if the URL is totally invalid instead of going to a 404 page  

Implementation details

The bug report also mentioned concerns about redirecting to the wrong vhost. In the case where an ICanHasLinkedBranch instance is the redirection target instead of a branch, the rootsite is set to "mainsite". For branch redirects, there's no need to make any changes. 

For cases where the URL is invalid, the HTTP referer is used as the redirection target.  

The only code changes were to:
    - lib/canonical/launchpad/browser/launchpad.py
    - lib/canonical/launchpad/browser/tests/test_launchpad.py

There is one deprecation warning:

/home/ian/projects/lp-branches/current/lib/canonical/launchpad/browser/launchpad.py:545: 
    DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
    "Invalid branch lp:%s. %s" % (path, e.message)

Tests

    bin/test -vvm canonical.launchpad.browser.tests.test_launchpad TestBranchTraversal

    Changed tests:
    These tests have primarily been changed to reflect the removal of the NotFound exceptions
    - test_no_such_unique_name
    - test_nonexistent_product
    - test_product_without_dev_focus
    - test_nonexistent_product_series
    - test_no_branch_for_series
    - test_too_short_branch_name
    - test_invalid_product_name

    New tests:
    These tests were perhaps missing from the original set
    - test_private_branch
    - test_private_branch_for_series
    - test_private_branch_for_product
    - test_distro_package_alias
    - test_private_branch_for_distro_package

lint

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/browser/launchpad.py
  lib/canonical/launchpad/browser/tests/test_launchpad.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/tales-linkify-broken-links/+merge/35268
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/tales-linkify-broken-links into lp:launchpad/devel.

=== modified file '.bzrignore'
--- .bzrignore	2010-09-05 20:29:40 +0000
+++ .bzrignore	2010-09-13 11:34:20 +0000
@@ -78,3 +78,5 @@
 lib/canonical/buildd/debian/*
 lib/canonical/buildd/launchpad-files/*
 *.pt.py
+.project
+.pydevproject

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2010-08-24 10:45:57 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2010-09-13 11:34:20 +0000
@@ -125,7 +125,10 @@
     NoLinkedBranch,
     )
 from lp.code.interfaces.branch import IBranchSet
-from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.interfaces.branchlookup import (
+    IBranchLookup,
+    ILinkedBranchTraverser,
+    )
 from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.hardwaredb.interfaces.hwdb import IHWDBApplication
 from lp.registry.interfaces.announcement import IAnnouncementSet
@@ -514,17 +517,40 @@
 
         'foo' can be the unique name of the branch, or any of the aliases for
         the branch.
+        If 'foo' resolves to an ICanHasLinkedBranch instance but the linked
+        branch is not yet set, then redirect to the ICanHasLinkedBranch
+        instance instead.
+        If 'foo' is completely invalid, redirect back to the referring page
+        with a suitable error message.
         """
         path = '/'.join(self.request.stepstogo)
+        target = branch = trailing = None
         try:
-            branch_data = getUtility(IBranchLookup).getByLPPath(path)
-        except (CannotHaveLinkedBranch, NoLinkedBranch, InvalidNamespace,
-                InvalidProductName):
-            raise NotFoundError
-        branch, trailing = branch_data
-        if branch is None:
-            raise NotFoundError
-        url = canonical_url(branch)
+            # first check for a valid branch url
+            try:
+                branch_data = getUtility(IBranchLookup).getByLPPath(path)
+                branch, trailing = branch_data
+            except (NoLinkedBranch):
+                # a valid ICanHasLinkedBranch target exists but there's no
+                # branch or it's not visible, so get the target instead
+                target = getUtility(ILinkedBranchTraverser).traverse(path)
+                userMessage = (
+                    "The requested branch does not exist. "
+                    "You have landed at lp:%s instead." % path)
+                self.request.response.addNotification(userMessage)
+
+        except (CannotHaveLinkedBranch, InvalidNamespace,
+                InvalidProductName, NotFoundError) as e:
+            self.request.response.addErrorNotification(
+                    "Invalid branch lp:%s. %s" % (path, e.message))
+
+        if branch is not None:
+            url = canonical_url(branch)
+        elif target is not None:
+            url = canonical_url(target, rootsite='mainsite')
+        else:
+            url = self.request.getHeader('referer')
+
         if trailing is not None:
             url = urlappend(url, trailing)
         return self.redirectSubTree(url)

=== modified file 'lib/canonical/launchpad/browser/tests/test_launchpad.py'
--- lib/canonical/launchpad/browser/tests/test_launchpad.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/browser/tests/test_launchpad.py	2010-09-13 11:34:20 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for traversal from the root branch object.."""
+from canonical.launchpad.webapp.interfaces import BrowserNotificationLevel
 
 __metaclass__ = type
 
@@ -13,17 +14,20 @@
 
 from canonical.launchpad.browser.launchpad import LaunchpadRootNavigation
 from canonical.launchpad.interfaces.account import AccountStatus
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.launchpad.webapp.url import urlappend
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.errors import GoneError
+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.registry.interfaces.person import (
     IPersonSet,
     PersonVisibility,
     )
 from lp.testing import (
     login_person,
+    run_with_login,
     TestCaseWithFactory,
     )
 from lp.testing.views import create_view
@@ -31,12 +35,52 @@
 
 class TraversalMixin:
 
+    def _validateNotificationContext(
+        self, request, notification=None,
+        level=BrowserNotificationLevel.INFO):
+
+        notifications = request.notifications
+        if notification is None:
+            self.assertEquals(len(notifications), 0)
+            return
+
+        self.assertEqual(len(notifications), 1)
+        self.assertEquals(notifications[0].level, level)
+
+        self.assertEqual(notification, notifications[0].message.rstrip(' '))
+
+    def assertDisplaysNotification(
+        self, path, notification=None,
+        level=BrowserNotificationLevel.INFO):
+        """Assert that an invalid path redirects back to referrer.
+
+        The request object is expected to have a notification message to
+        display to the user to explain the reason for the error.
+
+        :param path: The path to check
+        :param notification: The exact notification text to validate. If None
+            then we don't care what the notification text is, so long as there
+            is some.
+        : param level: the required notification level
+        """
+
+        redirection = self.traverse(path)
+        self.assertIs(redirection.target, None)
+        self._validateNotificationContext(
+            redirection.request, notification, level)
+
     def assertNotFound(self, path):
         self.assertRaises(NotFound, self.traverse, path)
 
-    def assertRedirects(self, segments, url):
+    def assertRedirects(
+        self, segments, url, notification=None,
+        level=BrowserNotificationLevel.INFO):
+
         redirection = self.traverse(segments)
         self.assertEqual(url, redirection.target)
+        self._validateNotificationContext(
+            redirection.request, notification=notification,
+            level=level)
 
     def traverse(self, path, first_segment):
         """Traverse to 'segments' using a 'LaunchpadRootNavigation' object.
@@ -58,8 +102,8 @@
 class TestBranchTraversal(TestCaseWithFactory, TraversalMixin):
     """Branches are traversed to from IPersons. Test we can reach them.
 
-    This class tests the `PersonNavigation` class to see that we can traverse
-    to branches from such objects.
+    This class tests the `LaunchpadRootNavigation` class to see that we can
+    traverse to branches from URLs of the form +branch/xxxx.
     """
 
     layer = DatabaseFunctionalLayer
@@ -75,9 +119,30 @@
 
     def test_no_such_unique_name(self):
         # Traversing to /+branch/<unique_name> where 'unique_name' is for a
-        # branch that doesn't exist will generate a 404.
+        # branch that doesn't exist will display an error message.
         branch = self.factory.makeAnyBranch()
-        self.assertNotFound(branch.unique_name + 'wibble')
+        bad_name = branch.unique_name + 'wibble'
+        requiredMessage = ("Invalid branch lp:%s. No such branch: '%s'."
+            % (bad_name, branch.name+"wibble"))
+        self.assertDisplaysNotification(
+            bad_name, requiredMessage,
+            BrowserNotificationLevel.ERROR)
+
+    def test_private_branch(self):
+        # If an attempt is made to access a private branch, display an error.
+        branch = self.factory.makeProductBranch()
+        branch_unique_name = branch.unique_name
+        removeSecurityProxy(branch.product).development_focus.branch = branch
+        removeSecurityProxy(branch).private = True
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        requiredMessage = ("Invalid branch lp:%s. No such branch: '%s'."
+            % (branch_unique_name, branch_unique_name))
+        self.assertDisplaysNotification(
+            branch_unique_name,
+            requiredMessage,
+            BrowserNotificationLevel.ERROR)
 
     def test_product_alias(self):
         # Traversing to /+branch/<product> redirects to the page for the
@@ -87,14 +152,89 @@
         product.development_focus.branch = branch
         self.assertRedirects(product.name, canonical_url(branch))
 
+    def test_private_branch_for_product(self):
+        # If the development focus of a product is private, navigate to the
+        # product instead.
+        branch = self.factory.makeProductBranch()
+        product = removeSecurityProxy(branch.product)
+        product.development_focus.branch = branch
+        removeSecurityProxy(branch).private = True
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        requiredMessage = (u"The requested branch does not exist. "
+            "You have landed at lp:%s instead." % product.name)
+        self.assertRedirects(
+            product.name,
+            canonical_url(product),
+            requiredMessage,
+            BrowserNotificationLevel.NOTICE)
+
     def test_nonexistent_product(self):
-        # Traversing to /+branch/<no-such-product> generates a 404.
-        self.assertNotFound('non-existent')
+        # Traversing to /+branch/<no-such-product> displays an error message.
+        non_existent = 'non-existent'
+        requiredMessage = (u"Invalid branch lp:%s. No such product: '%s'."
+            % (non_existent, non_existent))
+        self.assertDisplaysNotification(
+            non_existent, requiredMessage,
+            BrowserNotificationLevel.ERROR)
 
     def test_product_without_dev_focus(self):
-        # Traversing to a product without a development focus generates a 404.
+        # Traversing to a product without a development focus displays a
+        # user message on the same page.
         product = self.factory.makeProduct()
-        self.assertNotFound(product.name)
+        requiredMessage = (u"The requested branch does not exist. "
+            "You have landed at lp:%s instead." % product.name)
+
+        self.assertRedirects(
+            product.name,
+            canonical_url(product),
+            requiredMessage,
+            BrowserNotificationLevel.NOTICE)
+
+    def test_distro_package_alias(self):
+        # Traversing to /+branch/<distro>/<sourcepackage package> redirects
+        # to the page for the branch that is the development focus branch
+        # for that package.
+        sourcepackage = self.factory.makeSourcePackage()
+        branch = self.factory.makePackageBranch(sourcepackage=sourcepackage)
+        distro_package = sourcepackage.distribution_sourcepackage
+        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
+        registrant = ubuntu_branches.teamowner
+        run_with_login(
+            registrant,
+            ICanHasLinkedBranch(distro_package).setBranch, branch, registrant)
+
+        self.assertRedirects(
+            "%s/%s" % (distro_package.distribution.name,
+                distro_package.sourcepackagename.name),
+                canonical_url(branch))
+
+    def test_private_branch_for_distro_package(self):
+        # If the development focus of a distro package is private, navigate
+        # to the distro package instead.
+        sourcepackage = self.factory.makeSourcePackage()
+        branch = self.factory.makePackageBranch(
+            sourcepackage=sourcepackage, private=True)
+        distro_package = sourcepackage.distribution_sourcepackage
+        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
+        registrant = ubuntu_branches.teamowner
+        run_with_login(
+            registrant,
+            ICanHasLinkedBranch(distro_package).setBranch, branch, registrant)
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        requiredMessage = (u"The requested branch does not exist. "
+            "You have landed at lp:%s/%s instead."
+            % (distro_package.distribution.name,
+               distro_package.sourcepackagename.name))
+        self.assertRedirects(
+            "%s/%s" % (distro_package.distribution.name,
+                       distro_package.sourcepackagename.name),
+            canonical_url(distro_package),
+            requiredMessage,
+            BrowserNotificationLevel.NOTICE)
 
     def test_trailing_path_redirect(self):
         # If there are any trailing path segments after the branch identifier,
@@ -114,25 +254,69 @@
             '%s/%s' % (product.name, series.name), canonical_url(branch))
 
     def test_nonexistent_product_series(self):
-        # /+branch/<product>/<series> generates a 404 if there is no such
-        # series.
+        # /+branch/<product>/<series> displays an error message if there is
+        # no such series.
         product = self.factory.makeProduct()
-        self.assertNotFound('%s/nonexistent' % product.name)
+        non_existent = 'nonexistent'
+        requiredMessage = (u"Invalid branch lp:%s/%s. "
+            "No such product series: '%s'."
+            % (product.name, non_existent, non_existent))
+        self.assertDisplaysNotification(
+            '%s/%s' % (product.name, non_existent),
+            requiredMessage,
+            BrowserNotificationLevel.ERROR)
 
     def test_no_branch_for_series(self):
-        # If there's no branch for a product series, generate a 404.
+        # If there's no branch for a product series, navigate to the
+        # product series instead.
         series = self.factory.makeProductSeries()
-        self.assertNotFound('%s/%s' % (series.product.name, series.name))
+        requiredMessage = ("The requested branch does not exist. "
+            "You have landed at lp:%s/%s instead."
+            % (series.product.name, series.name))
+
+        self.assertRedirects(
+            '%s/%s' % (series.product.name, series.name),
+            canonical_url(series),
+            requiredMessage,
+            BrowserNotificationLevel.NOTICE)
+
+    def test_private_branch_for_series(self):
+        # If the development focus of a product series is private, navigate
+        # to the product series instead.
+        branch = self.factory.makeProductBranch()
+        product = branch.product
+        series = self.factory.makeProductSeries(product=product)
+        removeSecurityProxy(series).branch = branch
+        removeSecurityProxy(branch).private = True
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        requiredMessage = (u"The requested branch does not exist. "
+            "You have landed at lp:%s/%s instead."
+            % (product.name, series.name))
+        self.assertRedirects(
+            "%s/%s" % (product.name, series.name),
+            canonical_url(series),
+            requiredMessage,
+            BrowserNotificationLevel.NOTICE)
 
     def test_too_short_branch_name(self):
-        # 404 if the thing following +branch is a unique name that's too short
-        # to be a real unique name.
+        # error notification if the thing following +branch is a unique name
+        # that's too short to be a real unique name.
         owner = self.factory.makePerson()
-        self.assertNotFound('~%s' % owner.name)
+        requiredMessage = (u"Invalid branch lp:~%s. Cannot understand "
+            "namespace name: '%s'" % (owner.name, owner.name))
+        self.assertDisplaysNotification(
+            '~%s' % owner.name,
+            requiredMessage,
+            BrowserNotificationLevel.ERROR)
 
     def test_invalid_product_name(self):
-        # 404 if the thing following +branch has an invalid product name.
-        self.assertNotFound('a')
+        # error notification if the thing following +branch has an invalid
+        # product name.
+        self.assertDisplaysNotification(
+            'a', u"Invalid branch lp:%s." % 'a',
+            BrowserNotificationLevel.ERROR)
 
 
 class TestPersonTraversal(TestCaseWithFactory, TraversalMixin):