Launchpad itself

Merge lp:~thumper/launchpad/fix-canonical-url-for-api-2 into lp:launchpad

  1. fix-canonical-url-for-api-2
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/fix-canonical-url-for-api-2
Merge into: lp:launchpad
Diff against target: 122 lines (+32/-15)
4 files modified
lib/canonical/launchpad/doc/canonical_url.txt (+9/-0)
lib/canonical/launchpad/doc/tales.txt (+6/-1)
lib/canonical/launchpad/webapp/publisher.py (+6/-4)
lib/canonical/launchpad/webapp/tales.py (+11/-10)
To merge this branch: bzr merge lp:~thumper/launchpad/fix-canonical-url-for-api-2
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+14876@code.launchpad.net

Commit message

Fix the fmt:api_url to return the relative path only for URLs.

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

The current implementation of the api_url tales formatter doesn't do what it thinks it does. Setting the rootsite to None does not mean that the site isn't shown.

To fix this, I've added an option to canonical_url to force the local path only. I've also fixed the doctest for tales to make a distribution instead of a product where the example clearly indicates that the object should be a distribution. Also added the missing api_url to the Branch formatter, and added tests to cover these areas.

Revision history for this message
Curtis Hovey (sinzui) wrote :

This looks good to land. I have no comment.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/canonical_url.txt'
2--- lib/canonical/launchpad/doc/canonical_url.txt 2009-08-19 23:13:23 +0000
3+++ lib/canonical/launchpad/doc/canonical_url.txt 2009-11-14 22:50:26 +0000
4@@ -520,6 +520,15 @@
5 >>> canonical_url(countryset_instance, request=api_request)
6 u'http://api.launchpad.dev/countries'
7
8+It is often the case that the web application wants to provide URLs that will
9+be written out onto the pages that the Javascript can process using the
10+LP.client code to get access to the object entries using the API. In these
11+cases, the "force_local_path" parameter can be passed to canonical_url to have
12+only the relative local path returned.
13+
14+ >>> canonical_url(countryset_instance, force_local_path=True)
15+ u'/countries'
16+
17
18 == The end ==
19
20
21=== modified file 'lib/canonical/launchpad/doc/tales.txt'
22--- lib/canonical/launchpad/doc/tales.txt 2009-11-05 10:51:36 +0000
23+++ lib/canonical/launchpad/doc/tales.txt 2009-11-14 22:50:26 +0000
24@@ -319,10 +319,15 @@
25 >>> print test_tales("product/fmt:api_url", product=freewidget)
26 /freewidget
27
28- >>> debuntu = factory.makeProduct(name='debuntu')
29+ >>> debuntu = factory.makeDistribution(name='debuntu')
30 >>> print test_tales("distro/fmt:api_url", distro=debuntu)
31 /debuntu
32
33+ >>> branch = factory.makeProductBranch(
34+ ... owner=bob, product=freewidget, name='fix-bug')
35+ >>> print test_tales("branch/fmt:api_url", branch=branch)
36+ /~bob/freewidget/fix-bug
37+
38 >>> login(ANONYMOUS)
39
40
41
42=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
43--- lib/canonical/launchpad/webapp/publisher.py 2009-09-18 01:34:06 +0000
44+++ lib/canonical/launchpad/webapp/publisher.py 2009-11-14 22:50:26 +0000
45@@ -430,7 +430,7 @@
46
47 def canonical_url(
48 obj, request=None, rootsite=None, path_only_if_possible=False,
49- view_name=None):
50+ view_name=None, force_local_path=False):
51 """Return the canonical URL string for the object.
52
53 If the canonical url configuration for the given object binds it to a
54@@ -455,6 +455,7 @@
55 for the current request, return a url containing only the path.
56 :param view_name: Provide the canonical url for the specified view,
57 rather than the default view.
58+ :param force_local_path: Strip off the site no matter what.
59 :raises: NoCanonicalUrl if a canonical url is not available.
60 """
61 urlparts = [urldata.path
62@@ -517,9 +518,10 @@
63 root_url = request.getRootURL(rootsite)
64
65 path = u'/'.join(reversed(urlparts))
66- if (path_only_if_possible and
67- request is not None and
68- root_url.startswith(request.getApplicationURL())
69+ if ((path_only_if_possible and
70+ request is not None and
71+ root_url.startswith(request.getApplicationURL()))
72+ or force_local_path
73 ):
74 return unicode('/' + path)
75 return unicode(root_url + path)
76
77=== modified file 'lib/canonical/launchpad/webapp/tales.py'
78--- lib/canonical/launchpad/webapp/tales.py 2009-10-22 13:06:53 +0000
79+++ lib/canonical/launchpad/webapp/tales.py 2009-11-14 22:50:26 +0000
80@@ -471,8 +471,7 @@
81 try:
82 url = canonical_url(
83 self._context, path_only_if_possible=True,
84- rootsite=rootsite,
85- view_name=view_name)
86+ rootsite=rootsite, view_name=view_name)
87 except Unauthorized:
88 url = ""
89 return url
90@@ -480,14 +479,15 @@
91 def api_url(self, context):
92 """Return the object's (partial) canonical web service URL.
93
94- This method returns everything that goes after the web service
95- version number. It's the same as 'url', but without any view
96- name.
97+ This method returns everything that goes after the web service version
98+ number. Effectively the canonical URL but only the relative part with
99+ no site.
100 """
101-
102- # Some classes override the rootsite. We always want a path-only
103- # URL, so we override it to nothing.
104- return self.url(rootsite=None)
105+ try:
106+ url = canonical_url(self._context, force_local_path=True)
107+ except Unauthorized:
108+ url = ""
109+ return url
110
111 def traverse(self, name, furtherPath):
112 if name.startswith('link:') or name.startswith('url:'):
113@@ -1368,7 +1368,8 @@
114
115 traversable_names = {
116 'link': 'link', 'url': 'url', 'project-link': 'projectLink',
117- 'title-link': 'titleLink', 'bzr-link': 'bzrLink'}
118+ 'title-link': 'titleLink', 'bzr-link': 'bzrLink',
119+ 'api_url': 'api_url'}
120
121 def _args(self, view_name):
122 """Generate a dict of attributes for string template expansion."""