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
=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js 2011-01-27 02:09:37 +0000
+++ lib/lp/app/javascript/picker.js 2011-02-09 16:20:30 +0000
@@ -112,6 +112,7 @@
112 var content_uri_has_changed = false;112 var content_uri_has_changed = false;
113 // The entry is an XHTML document with a <dl> node at the root. We113 // The entry is an XHTML document with a <dl> node at the root. We
114 // want to process each <dt><dd> tag pair under that root.114 // want to process each <dt><dd> tag pair under that root.
115 var page_uri = null;
115 xhtml.all('dl *').each(function(element) {116 xhtml.all('dl *').each(function(element) {
116 if (element.get('tagName') == 'DT') {117 if (element.get('tagName') == 'DT') {
117 current_field = element.get('innerHTML');118 current_field = element.get('innerHTML');
@@ -121,6 +122,8 @@
121 success_message_node = Y.Node.create('<span></span>');122 success_message_node = Y.Node.create('<span></span>');
122 rendered_content = element.get('innerHTML');123 rendered_content = element.get('innerHTML');
123 success_message_node.set('innerHTML', rendered_content);124 success_message_node.set('innerHTML', rendered_content);
125 } else if (current_field == 'web_link') {
126 page_uri = element.get('innerHTML');
124 } else if (current_field == 'self_link') {127 } else if (current_field == 'self_link') {
125 picker._resource_uri = element.get('innerHTML');128 picker._resource_uri = element.get('innerHTML');
126 content_uri_has_changed = (129 content_uri_has_changed = (
@@ -130,15 +133,10 @@
130 });133 });
131 activator.renderSuccess(success_message_node);134 activator.renderSuccess(success_message_node);
132 show_hide_buttons();135 show_hide_buttons();
133 if (editing_main_context && content_uri_has_changed) {136 if (editing_main_context && content_uri_has_changed
134 // XXX Tim Penhey 2011-01-18 Bug #316694:137 && page_uri !== null) {
135 // This is a slightly nasty hack that saves us from the need138 window.location = page_uri;
136 // to have a more established way of getting the web URL of139 }
137 // an API object. Once such a solution is available we should
138 // fix this.
139 var new_url = picker._resource_uri.replace('/api/devel', '');
140 window.location = new_url;
141 }
142 };140 };
144 var patch_payload = {};142 var patch_payload = {};