Merge lp:~wallyworld/launchpad/tales-linkify-broken-links into lp:launchpad

[Ian Booth]

Fix the +branch/xxxx parsing functionality to traverse to a product/series/distropackage if the linked branch does not exist (or is private). Display a better error message is the URL is totally broken.

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 to let the user know the target has no linked branch and stay on the current page if an ICanHasLinkedBranch target is resolved instead of a branch
3. Add a page error and stay on the current page if the URL is totally invalid instead of going to a 404 page

Implementation details

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

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

[Tim Penhey (thumper)]

Hi Ian,

The code path firstly looks things up and then determins the url based on what
it found. Instead of remembering everything, you could just determine the
url, especially given that we only care about the trailing at one place.
In fact, I'm not even sure we want to deal with the trailing at all.

        try:
            # first check for a valid branch url
            try:
                branch, trailing = getUtility(IBranchLookup).getByLPPath(path)
                url = canonical_url(branch)
            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)

They haven't landed at lp:whatever instead. All you need to say is something
like:

    There is no branch linked for lp:whatever.

I'm beginning to wonder if we want to send them to the target at all...

                self.request.response.addNotification(userMessage)
                url = canonical_url(target, rootsite='mainsite')
        except (CannotHaveLinkedBranch, InvalidNamespace,
                InvalidProductName, NotFoundError) as e:
            self.request.response.addErrorNotification(
                    "Invalid branch lp:%s. %s" % (path, e.message))
            url = self.request.getHeader('referer')

That way you don't need the:
    target = branch = trailing = None

And skip the appending of the trailing.

I'm also a little concerned that you are getting a deprecation warning about
e.message. The docs say you should just use __str__, so...
    "Invalid branch lp:%s. %s" % (path, e))
should work fine.

[Tim Penhey (thumper)]

Also... I've had an thought...

How much do you like javascript?

[Ian Booth (wallyworld)]

Changes made to implementation:

- for private or non existent branches, do not redirect to target
product/series etc, stay on current page and display message
- improve error message for invalid links
- other minor improvements

[Tim Penhey (thumper)]

Getting pretty close now...

> if len(error_msg)==0:

PEP-8 says spaces either side of the operator, although I think

  if error_msg == '':

Is more obvious.

The _validateNotificationContext has a extra blank lines it doesn't need.
You can set the linked branch when you are making a series with the factory
using branch=...

Setting the development focus should be:
  ICanHasLinkedBranch(product).setBranch(...)

review: Needs Fixing

[Ian Booth (wallyworld)]

>
> The _validateNotificationContext has a extra blank lines it doesn't need.

Sometimes I like to leave blank lines to aid readability, although the
following one should go:

+ self.assertEqual(len(notifications), 1)
+ self.assertEquals(notifications[0].level, level)
+
+ self.assertEqual(notification, notifications[0].message)

I can remove the others as well.

> You can set the linked branch when you are making a series with the factory
> using branch=...
>

I don't know about that, unless I am missing something :-)

lp.testing.factory.py:
    def makeProductSeries(self, product=None, name=None, owner=None,
                          summary=None, date_created=None):
        """Create and return a new ProductSeries."""

There's no named parameter for branch. Hence it complains if one tries
to use branch=.

[Tim Penhey (thumper)]

On Wed, 15 Sep 2010 17:25:59 you wrote:
> lp.testing.factory.py:
> def makeProductSeries(self, product=None, name=None, owner=None,
> summary=None, date_created=None):
> """Create and return a new ProductSeries."""
>
> There's no named parameter for branch. Hence it complains if one tries
> to use branch=.

Found it. It is called makeSeries. Damn. We now have two factory methods to
do the same thing. Can you please consolidate?

[Ian Booth (wallyworld)]

A usage analysis shows:

215 usages of makeProductSeries
38 usages of makeSeries

There's too many changes to do a simple drive by. I think we should land
this branch as is and then do the consolidation in another branch.

Ok?

On 15/09/10 18:01, Tim Penhey wrote:
> On Wed, 15 Sep 2010 17:25:59 you wrote:
>> lp.testing.factory.py:
>> def makeProductSeries(self, product=None, name=None, owner=None,
>> summary=None, date_created=None):
>> """Create and return a new ProductSeries."""
>>
>> There's no named parameter for branch. Hence it complains if one tries
>> to use branch=.
>
> Found it. It is called makeSeries. Damn. We now have two factory methods to
> do the same thing. Can you please consolidate?

[Ian Booth (wallyworld)]

Initial work to add branch as a named parameter to
factory.makeProductSeries() is done. This allows about 4 or 5 lines of
code to be deleted from browser.test_launchpad.py

On 15/09/10 18:01, Tim Penhey wrote:
> On Wed, 15 Sep 2010 17:25:59 you wrote:
>> lp.testing.factory.py:
>> def makeProductSeries(self, product=None, name=None, owner=None,
>> summary=None, date_created=None):
>> """Create and return a new ProductSeries."""
>>
>> There's no named parameter for branch. Hence it complains if one tries
>> to use branch=.
>
> Found it. It is called makeSeries. Damn. We now have two factory methods to
> do the same thing. Can you please consolidate?

[Ian Booth (wallyworld)]

Made a small change to ensure unproxied variables were called naked_xxx

[Tim Penhey (thumper)]

Last thing:

   target = ICanHasLinkedBranch(distro_package)
   run_with_login(
       registrant,
       target.setBranch, branch, registrant)

IMO reads more easily as:

    with person_logged_in(registrant):
        ICanHasLinkedBranch(distro_package).setBranch(branch, registrant)

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2010-09-05 20:29:40 +0000
+++ .bzrignore	2010-09-16 06:06:06 +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-16 06:06:06 +0000
@@ -514,19 +514,39 @@
 
         '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, redirect back to the referring page with a
+        suitable notification message.
+        If 'foo' is completely invalid, redirect back to the referring page
+        with a suitable error message.
         """
+
+        # The default url to go to will be back to the referring page (in
+        # the case that there is an error resolving the branch url)
+        url = self.request.getHeader('referer')
         path = '/'.join(self.request.stepstogo)
         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)
-        if trailing is not None:
-            url = urlappend(url, trailing)
+            # first check for a valid branch url
+            try:
+                branch_data = getUtility(IBranchLookup).getByLPPath(path)
+                branch, trailing = branch_data
+                url = canonical_url(branch)
+                if trailing is not None:
+                    url = urlappend(url, trailing)
+
+            except (NoLinkedBranch):
+                # a valid ICanHasLinkedBranch target exists but there's no
+                # branch or it's not visible
+                self.request.response.addNotification(
+                    "The target %s does not have a linked branch." % path)
+
+        except (CannotHaveLinkedBranch, InvalidNamespace,
+                InvalidProductName, NotFoundError) as e:
+            error_msg = str(e)
+            if error_msg == '':
+                error_msg = "Invalid branch lp:%s." % path
+            self.request.response.addErrorNotification(error_msg)
+
         return self.redirectSubTree(url)
 
     @stepto('+builds')

=== 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-16 06:06:06 +0000
@@ -1,7 +1,7 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for traversal from the root branch object.."""
+"""Tests for traversal from the root branch object."""
 
 __metaclass__ = type
 
@@ -13,17 +13,21 @@
 
 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.interfaces import BrowserNotificationLevel
 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,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.views import create_view
@@ -31,6 +35,48 @@
 
 class TraversalMixin:
 
+    def _validateNotificationContext(
+        self, request, notification=None,
+        level=BrowserNotificationLevel.INFO):
+        """Check the browser notifications associated with the request.
+
+        Ensure that the notification instances attached to the request match
+        the expected values for text and type.
+
+        :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
+        """
+
+        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)
+
+    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)
 
@@ -58,8 +104,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,26 +121,107 @@
 
     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 = "No such branch: '%s'." % (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
+        naked_product = removeSecurityProxy(branch.product)
+        ICanHasLinkedBranch(naked_product).setBranch(branch)
+        removeSecurityProxy(branch).private = True
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        requiredMessage = "No such branch: '%s'." % 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
         # branch that is the development focus branch for that product.
         branch = self.factory.makeProductBranch()
-        product = removeSecurityProxy(branch.product)
-        product.development_focus.branch = branch
-        self.assertRedirects(product.name, canonical_url(branch))
+        naked_product = removeSecurityProxy(branch.product)
+        ICanHasLinkedBranch(naked_product).setBranch(branch)
+        self.assertRedirects(naked_product.name, canonical_url(branch))
+
+    def test_private_branch_for_product(self):
+        # If the development focus of a product is private, display a
+        # message telling the user there is no linked branch.
+        branch = self.factory.makeProductBranch()
+        naked_product = removeSecurityProxy(branch.product)
+        ICanHasLinkedBranch(naked_product).setBranch(branch)
+        removeSecurityProxy(branch).private = True
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        requiredMessage = (u"The target %s does not have a linked branch."
+            % naked_product.name)
+        self.assertDisplaysNotification(
+            naked_product.name,
+            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"No such product: '%s'." % 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 target %s does not have a linked branch."
+            % product.name)
+        self.assertDisplaysNotification(
+            product.name,
+            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
+        target = ICanHasLinkedBranch(distro_package)
+        with person_logged_in(registrant):
+            target.setBranch(branch, registrant)
+        self.assertRedirects("%s" % target.bzr_path, canonical_url(branch))
+
+    def test_private_branch_for_distro_package(self):
+        # If the development focus of a distro package is private, display a
+        # message telling the user there is no linked branch.
+        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
+        with person_logged_in(registrant):
+            ICanHasLinkedBranch(distro_package).setBranch(branch, registrant)
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        path = ICanHasLinkedBranch(distro_package).bzr_path
+        requiredMessage = (u"The target %s does not have a linked branch."
+            % path)
+        self.assertDisplaysNotification(
+            path, requiredMessage, BrowserNotificationLevel.NOTICE)
 
     def test_trailing_path_redirect(self):
         # If there are any trailing path segments after the branch identifier,
@@ -106,33 +233,63 @@
     def test_product_series_redirect(self):
         # Traversing to /+branch/<product>/<series> redirects to the branch
         # for that series, if there is one.
-        branch = self.factory.makeProductBranch()
-        product = branch.product
-        series = self.factory.makeProductSeries(product=product)
-        removeSecurityProxy(series).branch = branch
+        branch = self.factory.makeBranch()
+        series = self.factory.makeProductSeries(branch=branch)
         self.assertRedirects(
-            '%s/%s' % (product.name, series.name), canonical_url(branch))
+            ICanHasLinkedBranch(series).bzr_path, 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"No such product series: '%s'." % 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, display a
+        # message telling the user there is no linked branch.
         series = self.factory.makeProductSeries()
-        self.assertNotFound('%s/%s' % (series.product.name, series.name))
+        path = ICanHasLinkedBranch(series).bzr_path
+        requiredMessage = ("The target %s does not have a linked branch."
+            % path)
+        self.assertDisplaysNotification(
+            path, requiredMessage, BrowserNotificationLevel.NOTICE)
+
+    def test_private_branch_for_series(self):
+        # If the development focus of a product series is private, display a
+        # message telling the user there is no linked branch.
+        branch = self.factory.makeBranch(private=True)
+        series = self.factory.makeProductSeries(branch=branch)
+
+        any_user = self.factory.makePerson()
+        login_person(any_user)
+        path = ICanHasLinkedBranch(series).bzr_path
+        requiredMessage = (u"The target %s does not have a linked branch."
+            % path)
+        self.assertDisplaysNotification(
+            path, 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"Cannot understand namespace name: '%s'"
+            % 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 name for product: a.",
+            BrowserNotificationLevel.ERROR)
 
 
 class TestPersonTraversal(TestCaseWithFactory, TraversalMixin):

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-09-10 03:20:48 +0000
+++ lib/lp/testing/factory.py	2010-09-16 06:06:06 +0000
@@ -881,7 +881,7 @@
         return product
 
     def makeProductSeries(self, product=None, name=None, owner=None,
-                          summary=None, date_created=None):
+                          summary=None, date_created=None, branch=None):
         """Create and return a new ProductSeries."""
         if product is None:
             product = self.makeProduct()
@@ -895,7 +895,7 @@
         # so we remove the security proxy before creating the series.
         naked_product = removeSecurityProxy(product)
         series = naked_product.newSeries(
-            owner=owner, name=name, summary=summary)
+            owner=owner, name=name, summary=summary, branch=branch)
         if date_created is not None:
             series.datecreated = date_created
         return ProxyFactory(series)