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

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.

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

How much do you like javascript?

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

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(...)

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

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?

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?

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?

Made a small change to ensure unproxied variables were called naked_xxx

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)