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

Proposed by Ian Booth
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 11564
Proposed branch: lp:~wallyworld/launchpad/tales-linkify-broken-links
Merge into: lp:launchpad
Diff against target: 376 lines (+218/-39)
4 files modified
.bzrignore (+2/-0)
lib/canonical/launchpad/browser/launchpad.py (+30/-10)
lib/canonical/launchpad/browser/tests/test_launchpad.py (+184/-27)
lib/lp/testing/factory.py (+2/-2)
To merge this branch: bzr merge lp:~wallyworld/launchpad/tales-linkify-broken-links
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+35268@code.launchpad.net

Commit message

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.

Description of the change

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

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

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.

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

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

How much do you like javascript?

Revision history for this message
Ian Booth (wallyworld) wrote :

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

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

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
Revision history for this message
Ian Booth (wallyworld) wrote :

>
> 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=.

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

Revision history for this message
Ian Booth (wallyworld) wrote :

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?

Revision history for this message
Ian Booth (wallyworld) wrote :

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?

Revision history for this message
Ian Booth (wallyworld) wrote :

Made a small change to ensure unproxied variables were called naked_xxx

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2010-09-05 20:29:40 +0000
3+++ .bzrignore 2010-09-16 06:06:06 +0000
4@@ -78,3 +78,5 @@
5 lib/canonical/buildd/debian/*
6 lib/canonical/buildd/launchpad-files/*
7 *.pt.py
8+.project
9+.pydevproject
10
11=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
12--- lib/canonical/launchpad/browser/launchpad.py 2010-08-24 10:45:57 +0000
13+++ lib/canonical/launchpad/browser/launchpad.py 2010-09-16 06:06:06 +0000
14@@ -514,19 +514,39 @@
15
16 'foo' can be the unique name of the branch, or any of the aliases for
17 the branch.
18+ If 'foo' resolves to an ICanHasLinkedBranch instance but the linked
19+ branch is not yet set, redirect back to the referring page with a
20+ suitable notification message.
21+ If 'foo' is completely invalid, redirect back to the referring page
22+ with a suitable error message.
23 """
24+
25+ # The default url to go to will be back to the referring page (in
26+ # the case that there is an error resolving the branch url)
27+ url = self.request.getHeader('referer')
28 path = '/'.join(self.request.stepstogo)
29 try:
30- branch_data = getUtility(IBranchLookup).getByLPPath(path)
31- except (CannotHaveLinkedBranch, NoLinkedBranch, InvalidNamespace,
32- InvalidProductName):
33- raise NotFoundError
34- branch, trailing = branch_data
35- if branch is None:
36- raise NotFoundError
37- url = canonical_url(branch)
38- if trailing is not None:
39- url = urlappend(url, trailing)
40+ # first check for a valid branch url
41+ try:
42+ branch_data = getUtility(IBranchLookup).getByLPPath(path)
43+ branch, trailing = branch_data
44+ url = canonical_url(branch)
45+ if trailing is not None:
46+ url = urlappend(url, trailing)
47+
48+ except (NoLinkedBranch):
49+ # a valid ICanHasLinkedBranch target exists but there's no
50+ # branch or it's not visible
51+ self.request.response.addNotification(
52+ "The target %s does not have a linked branch." % path)
53+
54+ except (CannotHaveLinkedBranch, InvalidNamespace,
55+ InvalidProductName, NotFoundError) as e:
56+ error_msg = str(e)
57+ if error_msg == '':
58+ error_msg = "Invalid branch lp:%s." % path
59+ self.request.response.addErrorNotification(error_msg)
60+
61 return self.redirectSubTree(url)
62
63 @stepto('+builds')
64
65=== modified file 'lib/canonical/launchpad/browser/tests/test_launchpad.py'
66--- lib/canonical/launchpad/browser/tests/test_launchpad.py 2010-08-20 20:31:18 +0000
67+++ lib/canonical/launchpad/browser/tests/test_launchpad.py 2010-09-16 06:06:06 +0000
68@@ -1,7 +1,7 @@
69 # Copyright 2009 Canonical Ltd. This software is licensed under the
70 # GNU Affero General Public License version 3 (see the file LICENSE).
71
72-"""Tests for traversal from the root branch object.."""
73+"""Tests for traversal from the root branch object."""
74
75 __metaclass__ = type
76
77@@ -13,17 +13,21 @@
78
79 from canonical.launchpad.browser.launchpad import LaunchpadRootNavigation
80 from canonical.launchpad.interfaces.account import AccountStatus
81+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
82 from canonical.launchpad.webapp import canonical_url
83+from canonical.launchpad.webapp.interfaces import BrowserNotificationLevel
84 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
85 from canonical.launchpad.webapp.url import urlappend
86 from canonical.testing.layers import DatabaseFunctionalLayer
87 from lp.app.errors import GoneError
88+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
89 from lp.registry.interfaces.person import (
90 IPersonSet,
91 PersonVisibility,
92 )
93 from lp.testing import (
94 login_person,
95+ person_logged_in,
96 TestCaseWithFactory,
97 )
98 from lp.testing.views import create_view
99@@ -31,6 +35,48 @@
100
101 class TraversalMixin:
102
103+ def _validateNotificationContext(
104+ self, request, notification=None,
105+ level=BrowserNotificationLevel.INFO):
106+ """Check the browser notifications associated with the request.
107+
108+ Ensure that the notification instances attached to the request match
109+ the expected values for text and type.
110+
111+ :param notification: The exact notification text to validate. If None
112+ then we don't care what the notification text is, so long as there
113+ is some.
114+ : param level: the required notification level
115+ """
116+
117+ notifications = request.notifications
118+ if notification is None:
119+ self.assertEquals(len(notifications), 0)
120+ return
121+ self.assertEqual(len(notifications), 1)
122+ self.assertEquals(notifications[0].level, level)
123+ self.assertEqual(notification, notifications[0].message)
124+
125+ def assertDisplaysNotification(
126+ self, path, notification=None,
127+ level=BrowserNotificationLevel.INFO):
128+ """Assert that an invalid path redirects back to referrer.
129+
130+ The request object is expected to have a notification message to
131+ display to the user to explain the reason for the error.
132+
133+ :param path: The path to check
134+ :param notification: The exact notification text to validate. If None
135+ then we don't care what the notification text is, so long as there
136+ is some.
137+ : param level: the required notification level
138+ """
139+
140+ redirection = self.traverse(path)
141+ self.assertIs(redirection.target, None)
142+ self._validateNotificationContext(
143+ redirection.request, notification, level)
144+
145 def assertNotFound(self, path):
146 self.assertRaises(NotFound, self.traverse, path)
147
148@@ -58,8 +104,8 @@
149 class TestBranchTraversal(TestCaseWithFactory, TraversalMixin):
150 """Branches are traversed to from IPersons. Test we can reach them.
151
152- This class tests the `PersonNavigation` class to see that we can traverse
153- to branches from such objects.
154+ This class tests the `LaunchpadRootNavigation` class to see that we can
155+ traverse to branches from URLs of the form +branch/xxxx.
156 """
157
158 layer = DatabaseFunctionalLayer
159@@ -75,26 +121,107 @@
160
161 def test_no_such_unique_name(self):
162 # Traversing to /+branch/<unique_name> where 'unique_name' is for a
163- # branch that doesn't exist will generate a 404.
164+ # branch that doesn't exist will display an error message.
165 branch = self.factory.makeAnyBranch()
166- self.assertNotFound(branch.unique_name + 'wibble')
167+ bad_name = branch.unique_name + 'wibble'
168+ requiredMessage = "No such branch: '%s'." % (branch.name+"wibble")
169+ self.assertDisplaysNotification(
170+ bad_name, requiredMessage,
171+ BrowserNotificationLevel.ERROR)
172+
173+ def test_private_branch(self):
174+ # If an attempt is made to access a private branch, display an error.
175+ branch = self.factory.makeProductBranch()
176+ branch_unique_name = branch.unique_name
177+ naked_product = removeSecurityProxy(branch.product)
178+ ICanHasLinkedBranch(naked_product).setBranch(branch)
179+ removeSecurityProxy(branch).private = True
180+
181+ any_user = self.factory.makePerson()
182+ login_person(any_user)
183+ requiredMessage = "No such branch: '%s'." % branch_unique_name
184+ self.assertDisplaysNotification(
185+ branch_unique_name,
186+ requiredMessage,
187+ BrowserNotificationLevel.ERROR)
188
189 def test_product_alias(self):
190 # Traversing to /+branch/<product> redirects to the page for the
191 # branch that is the development focus branch for that product.
192 branch = self.factory.makeProductBranch()
193- product = removeSecurityProxy(branch.product)
194- product.development_focus.branch = branch
195- self.assertRedirects(product.name, canonical_url(branch))
196+ naked_product = removeSecurityProxy(branch.product)
197+ ICanHasLinkedBranch(naked_product).setBranch(branch)
198+ self.assertRedirects(naked_product.name, canonical_url(branch))
199+
200+ def test_private_branch_for_product(self):
201+ # If the development focus of a product is private, display a
202+ # message telling the user there is no linked branch.
203+ branch = self.factory.makeProductBranch()
204+ naked_product = removeSecurityProxy(branch.product)
205+ ICanHasLinkedBranch(naked_product).setBranch(branch)
206+ removeSecurityProxy(branch).private = True
207+
208+ any_user = self.factory.makePerson()
209+ login_person(any_user)
210+ requiredMessage = (u"The target %s does not have a linked branch."
211+ % naked_product.name)
212+ self.assertDisplaysNotification(
213+ naked_product.name,
214+ requiredMessage,
215+ BrowserNotificationLevel.NOTICE)
216
217 def test_nonexistent_product(self):
218- # Traversing to /+branch/<no-such-product> generates a 404.
219- self.assertNotFound('non-existent')
220+ # Traversing to /+branch/<no-such-product> displays an error message.
221+ non_existent = 'non-existent'
222+ requiredMessage = u"No such product: '%s'." % non_existent
223+ self.assertDisplaysNotification(
224+ non_existent, requiredMessage,
225+ BrowserNotificationLevel.ERROR)
226
227 def test_product_without_dev_focus(self):
228- # Traversing to a product without a development focus generates a 404.
229+ # Traversing to a product without a development focus displays a
230+ # user message on the same page.
231 product = self.factory.makeProduct()
232- self.assertNotFound(product.name)
233+ requiredMessage = (u"The target %s does not have a linked branch."
234+ % product.name)
235+ self.assertDisplaysNotification(
236+ product.name,
237+ requiredMessage,
238+ BrowserNotificationLevel.NOTICE)
239+
240+ def test_distro_package_alias(self):
241+ # Traversing to /+branch/<distro>/<sourcepackage package> redirects
242+ # to the page for the branch that is the development focus branch
243+ # for that package.
244+ sourcepackage = self.factory.makeSourcePackage()
245+ branch = self.factory.makePackageBranch(sourcepackage=sourcepackage)
246+ distro_package = sourcepackage.distribution_sourcepackage
247+ ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
248+ registrant = ubuntu_branches.teamowner
249+ target = ICanHasLinkedBranch(distro_package)
250+ with person_logged_in(registrant):
251+ target.setBranch(branch, registrant)
252+ self.assertRedirects("%s" % target.bzr_path, canonical_url(branch))
253+
254+ def test_private_branch_for_distro_package(self):
255+ # If the development focus of a distro package is private, display a
256+ # message telling the user there is no linked branch.
257+ sourcepackage = self.factory.makeSourcePackage()
258+ branch = self.factory.makePackageBranch(
259+ sourcepackage=sourcepackage, private=True)
260+ distro_package = sourcepackage.distribution_sourcepackage
261+ ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
262+ registrant = ubuntu_branches.teamowner
263+ with person_logged_in(registrant):
264+ ICanHasLinkedBranch(distro_package).setBranch(branch, registrant)
265+
266+ any_user = self.factory.makePerson()
267+ login_person(any_user)
268+ path = ICanHasLinkedBranch(distro_package).bzr_path
269+ requiredMessage = (u"The target %s does not have a linked branch."
270+ % path)
271+ self.assertDisplaysNotification(
272+ path, requiredMessage, BrowserNotificationLevel.NOTICE)
273
274 def test_trailing_path_redirect(self):
275 # If there are any trailing path segments after the branch identifier,
276@@ -106,33 +233,63 @@
277 def test_product_series_redirect(self):
278 # Traversing to /+branch/<product>/<series> redirects to the branch
279 # for that series, if there is one.
280- branch = self.factory.makeProductBranch()
281- product = branch.product
282- series = self.factory.makeProductSeries(product=product)
283- removeSecurityProxy(series).branch = branch
284+ branch = self.factory.makeBranch()
285+ series = self.factory.makeProductSeries(branch=branch)
286 self.assertRedirects(
287- '%s/%s' % (product.name, series.name), canonical_url(branch))
288+ ICanHasLinkedBranch(series).bzr_path, canonical_url(branch))
289
290 def test_nonexistent_product_series(self):
291- # /+branch/<product>/<series> generates a 404 if there is no such
292- # series.
293+ # /+branch/<product>/<series> displays an error message if there is
294+ # no such series.
295 product = self.factory.makeProduct()
296- self.assertNotFound('%s/nonexistent' % product.name)
297+ non_existent = 'nonexistent'
298+ requiredMessage = u"No such product series: '%s'." % non_existent
299+ self.assertDisplaysNotification(
300+ '%s/%s' % (product.name, non_existent),
301+ requiredMessage,
302+ BrowserNotificationLevel.ERROR)
303
304 def test_no_branch_for_series(self):
305- # If there's no branch for a product series, generate a 404.
306+ # If there's no branch for a product series, display a
307+ # message telling the user there is no linked branch.
308 series = self.factory.makeProductSeries()
309- self.assertNotFound('%s/%s' % (series.product.name, series.name))
310+ path = ICanHasLinkedBranch(series).bzr_path
311+ requiredMessage = ("The target %s does not have a linked branch."
312+ % path)
313+ self.assertDisplaysNotification(
314+ path, requiredMessage, BrowserNotificationLevel.NOTICE)
315+
316+ def test_private_branch_for_series(self):
317+ # If the development focus of a product series is private, display a
318+ # message telling the user there is no linked branch.
319+ branch = self.factory.makeBranch(private=True)
320+ series = self.factory.makeProductSeries(branch=branch)
321+
322+ any_user = self.factory.makePerson()
323+ login_person(any_user)
324+ path = ICanHasLinkedBranch(series).bzr_path
325+ requiredMessage = (u"The target %s does not have a linked branch."
326+ % path)
327+ self.assertDisplaysNotification(
328+ path, requiredMessage, BrowserNotificationLevel.NOTICE)
329
330 def test_too_short_branch_name(self):
331- # 404 if the thing following +branch is a unique name that's too short
332- # to be a real unique name.
333+ # error notification if the thing following +branch is a unique name
334+ # that's too short to be a real unique name.
335 owner = self.factory.makePerson()
336- self.assertNotFound('~%s' % owner.name)
337+ requiredMessage = (u"Cannot understand namespace name: '%s'"
338+ % owner.name)
339+ self.assertDisplaysNotification(
340+ '~%s' % owner.name,
341+ requiredMessage,
342+ BrowserNotificationLevel.ERROR)
343
344 def test_invalid_product_name(self):
345- # 404 if the thing following +branch has an invalid product name.
346- self.assertNotFound('a')
347+ # error notification if the thing following +branch has an invalid
348+ # product name.
349+ self.assertDisplaysNotification(
350+ 'a', u"Invalid name for product: a.",
351+ BrowserNotificationLevel.ERROR)
352
353
354 class TestPersonTraversal(TestCaseWithFactory, TraversalMixin):
355
356=== modified file 'lib/lp/testing/factory.py'
357--- lib/lp/testing/factory.py 2010-09-10 03:20:48 +0000
358+++ lib/lp/testing/factory.py 2010-09-16 06:06:06 +0000
359@@ -881,7 +881,7 @@
360 return product
361
362 def makeProductSeries(self, product=None, name=None, owner=None,
363- summary=None, date_created=None):
364+ summary=None, date_created=None, branch=None):
365 """Create and return a new ProductSeries."""
366 if product is None:
367 product = self.makeProduct()
368@@ -895,7 +895,7 @@
369 # so we remove the security proxy before creating the series.
370 naked_product = removeSecurityProxy(product)
371 series = naked_product.newSeries(
372- owner=owner, name=name, summary=summary)
373+ owner=owner, name=name, summary=summary, branch=branch)
374 if date_created is not None:
375 series.datecreated = date_created
376 return ProxyFactory(series)