Merge lp:~leonardr/launchpad/use-web-link into lp:launchpad

Proposed by Tim Penhey
Status: Merged
Approved by: Leonard Richardson
Approved revision: no longer in the source branch.
Merged at revision: 12356
Proposed branch: lp:~leonardr/launchpad/use-web-link
Merge into: lp:launchpad
Diff against target: 40 lines (+7/-9)
1 file modified
lib/lp/app/javascript/picker.js (+7/-9)
To merge this branch: bzr merge lp:~leonardr/launchpad/use-web-link
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email:

Commit message

[r=jtv] [ui=none][bug=316694][incr] Take advantage of web_link when changing the URL in the browser bar, rather than hacking self_link into web_link.

Description of the change

This branch removes some tech debt. Previously, when the picker edit widget made a PATCH request that changed the URL of the context object (such as changing the target of a bug), the new URL was determined by search-and-replacing the new self_link.

Now that bug 316694 has been fixed, the document returned by the PATCH request includes a web_link, which is the value we were hacking out of self_link. This branch makes the code that retrieves self_link (which we need, to see whether the URL has changed) also look for web_link, and use it directly rather than deriving it from self_link.

There's at least one other place where we hack self_link into web_link, but it's not as easy to fix, so I'm submitting this branch in the meantime.

To test:

1. Visit

Edit the project by clicking the edit button beneath "Affects", doing a search for "launchpad", and selecting Launchpad as the project.

The URL in the browser address bar should change to

2. Create a recipe for a branch.

Change its owner, and the browser address bar should also change.

Change its daily build archive, and the browser address bar should stay as is.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

I can't say I like the "&&" at the beginning of a line, but I won't force my tastes on you. Otherwise, looks fine.

As per IRC, we're not foreseeing any legitimate scenario under which web_link might not be present, so the one obvious path of breakage is not to be expected.


review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/app/javascript/picker.js'
2--- lib/lp/app/javascript/picker.js 2011-01-27 02:09:37 +0000
3+++ lib/lp/app/javascript/picker.js 2011-02-09 16:20:30 +0000
4@@ -112,6 +112,7 @@
5 var content_uri_has_changed = false;
6 // The entry is an XHTML document with a <dl> node at the root. We
7 // want to process each <dt><dd> tag pair under that root.
8+ var page_uri = null;
9 xhtml.all('dl *').each(function(element) {
10 if (element.get('tagName') == 'DT') {
11 current_field = element.get('innerHTML');
12@@ -121,6 +122,8 @@
13 success_message_node = Y.Node.create('<span></span>');
14 rendered_content = element.get('innerHTML');
15 success_message_node.set('innerHTML', rendered_content);
16+ } else if (current_field == 'web_link') {
17+ page_uri = element.get('innerHTML');
18 } else if (current_field == 'self_link') {
19 picker._resource_uri = element.get('innerHTML');
20 content_uri_has_changed = (
21@@ -130,15 +133,10 @@
22 });
23 activator.renderSuccess(success_message_node);
24 show_hide_buttons();
25- if (editing_main_context && content_uri_has_changed) {
26- // XXX Tim Penhey 2011-01-18 Bug #316694:
27- // This is a slightly nasty hack that saves us from the need
28- // to have a more established way of getting the web URL of
29- // an API object. Once such a solution is available we should
30- // fix this.
31- var new_url = picker._resource_uri.replace('/api/devel', '');
32- window.location = new_url;
33- }
34+ if (editing_main_context && content_uri_has_changed
35+ && page_uri !== null) {
36+ window.location = page_uri;
37+ }
38 };
40 var patch_payload = {};