Merge lp:~wallyworld/launchpad/tales-linkify-broken-links into lp:launchpad
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 | ||||
Related bugs: |
|
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/
Description of the change
Bug 404131 describes some issues with the linkification functionality of tales.Formatter
Proposed fix
The redirect_branch() method in lib/canonical/
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/
- lib/canonical/
Tests
bin/test -vvm canonical.
Changed tests:
These tests have primarily been changed to reflect the removal of the NotFound exceptions
- test_no_
- test_nonexisten
- test_product_
- test_nonexisten
- test_no_
- test_too_
- test_invalid_
New tests:
These tests were perhaps missing from the original set
- test_private_branch
- test_private_
- test_private_
- test_distro_
- test_private_
lint
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical
lib/canonical
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:
branch, trailing = getUtility( IBranchLookup) .getByLPPath( path) url(branch)
target = getUtility( ILinkedBranchTr averser) .traverse( path)
userMessage = (
" The requested branch does not exist. "
" You have landed at lp:%s instead." % path)
# first check for a valid branch url
try:
url = canonical_
except (NoLinkedBranch):
# a valid ICanHasLinkedBranch target exists but there's no
# branch or it's not visible, so get the target instead
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...
url = canonical_
except (CannotHaveLink
url = self.request.
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.