launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03222
[Merge] lp:~bac/launchpad/bug-751397 into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-751397 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #751397 in Launchpad itself: "Structural subscription overlay sometimes rendering incorrectly on the page not as an overlay"
https://bugs.launchpad.net/launchpad/+bug/751397
For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-751397/+merge/56617
= Summary =
In YUI3, Y.fail was renamed Y.error so that Y.fail could be moved to the
test infrastructure. Y.fail should no longer be used in production code
but we had many instances of it. They presented no harm in our tests
since the test infrastructure was requiring the 'test' module but in
production Y.fail is undefined and would mask the true error that needed
to be reported.
Also, I do not invoke the structural subscription JS if the bugtarget
does not use LP for bug tracking, which was the point of the original bug.
== Proposed fix ==
s/Y.fail/Y.error/ftw
== Pre-implementation notes ==
Chat with Deryck.
== Implementation details ==
As above. Also some de-linting, which dominates the diff. Some tests
needed the _should clauses changed from 'fail' to 'error'.
== Tests ==
Windmill tests.
== Demo and Q/A ==
Open https://bugs.launchpad.net/thunderbird and witness that no errors
are raised in the console.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/tests/test_lp_collapsibles.js
lib/lp/bugs/javascript/filebug_dupefinder.js
lib/lp/app/javascript/lp.js
lib/lp/app/javascript/client.js
lib/lp/bugs/templates/bugtarget-bugs.pt
lib/lp/registry/javascript/tests/test_structural_subscription.js
lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js
lib/lp/registry/javascript/structural-subscription.js
--
https://code.launchpad.net/~bac/launchpad/bug-751397/+merge/56617
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-751397 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2011-04-05 12:10:01 +0000
+++ lib/lp/app/javascript/client.js 2011-04-06 17:15:47 +0000
@@ -1,7 +1,8 @@
/* Copyright 2009-2011 Canonical Ltd. This software is licensed under the
* GNU Affero General Public License version 3 (see the file LICENSE).
*
- * Utility methods and classes to deal with the Launchpad API using Javascript.
+ * Utility methods and classes to deal with the Launchpad API using
+ * Javascript.
*
* @module Y.lp.client
*/
@@ -188,7 +189,8 @@
if (old_value != new_value) {
fields_changed.push(name);
cache[name] = new_value;
- var field_updated_event_name = 'lp:' + cache_name + ':' + name + ':changed';
+ var field_updated_event_name =
+ 'lp:' + cache_name + ':' + name + ':changed';
var new_value_html = entry.getHTML(name);
var event = {
'name': name,
@@ -232,7 +234,7 @@
update_cached_object(name, cached_object, entry);
}
}
-}
+};
module.wrap_resource_on_success = function(ignore, response, args) {
var client = args[0];
@@ -564,10 +566,12 @@
var update_cache = false;
on.success = module.wrap_resource_on_success;
var client = this;
- var y_config = { on: on,
- 'arguments': [client, uri, old_on_success, update_cache],
- 'headers': headers,
- data: data};
+ var y_config = {
+ on: on,
+ 'arguments': [client, uri, old_on_success, update_cache],
+ 'headers': headers,
+ data: data
+ };
Y.io(uri, y_config);
},
@@ -611,10 +615,12 @@
};
var client = this;
var update_cache = false;
- var y_config = { method: "POST",
- on: on,
- 'arguments': [client, uri, old_on_success, update_cache],
- data: data};
+ var y_config = {
+ method: "POST",
+ on: on,
+ 'arguments': [client, uri, old_on_success, update_cache],
+ data: data
+ };
Y.io(uri, y_config);
},
@@ -740,9 +746,11 @@
'Timeout error, please try again in a few minutes.');
// If it was a server error...
} else if (o.status >= 500) {
- var server_error = 'Server error, please contact an administrator.';
+ var server_error =
+ 'Server error, please contact an administrator.';
if (o.getResponseHeader('X-Lazr-OopsId')) {
- server_error = server_error + ' OOPS ID:' + o.getResponseHeader('X-Lazr-OOPSid');
+ server_error = server_error + ' OOPS ID:' +
+ o.getResponseHeader('X-Lazr-OOPSid');
}
self.showError(server_error);
// Otherwise we send some sane text as an error
@@ -867,11 +875,11 @@
*/
initializer: function(config) {
if (!Y.Lang.isString(config.patch)) {
- Y.fail("missing config: 'patch' containing the attribute name");
+ Y.error("missing config: 'patch' containing the attribute name");
}
if (!Y.Lang.isString(config.resource)) {
- Y.fail("missing config: 'resource' containing the URL to patch");
+ Y.error("missing config: 'resource' containing the URL to patch");
}
// Save the config object that the user passed in so that we can pass
=== modified file 'lib/lp/app/javascript/lp.js'
--- lib/lp/app/javascript/lp.js 2011-03-24 15:31:53 +0000
+++ lib/lp/app/javascript/lp.js 2011-04-06 17:15:47 +0000
@@ -68,10 +68,10 @@
// If either the wrapper or the icon is null, raise an error.
if (wrapper_div === null) {
- Y.fail("Collapsible has no wrapper div.");
+ Y.error("Collapsible has no wrapper div.");
}
if (icon === null) {
- Y.fail("Collapsible has no icon.");
+ Y.error("Collapsible has no icon.");
}
return wrapper_div;
}
=== modified file 'lib/lp/app/javascript/tests/test_lp_collapsibles.js'
--- lib/lp/app/javascript/tests/test_lp_collapsibles.js 2011-02-03 20:55:58 +0000
+++ lib/lp/app/javascript/tests/test_lp_collapsibles.js 2011-04-06 17:15:47 +0000
@@ -14,9 +14,11 @@
name: "activate_collapsibles",
_should: {
- fail: {
- test_toggle_collapsible_fails_on_wrapperless_collapsible: true,
- test_toggle_collapsible_fails_on_iconless_collapsible: true,
+ error: {
+ test_toggle_collapsible_fails_on_wrapperless_collapsible:
+ new Error('Collapsible has no wrapper div.'),
+ test_toggle_collapsible_fails_on_iconless_collapsible:
+ new Error('Collapsible has no icon.')
}
},
=== modified file 'lib/lp/bugs/javascript/filebug_dupefinder.js'
--- lib/lp/bugs/javascript/filebug_dupefinder.js 2011-02-04 15:03:32 +0000
+++ lib/lp/bugs/javascript/filebug_dupefinder.js 2011-04-06 17:15:47 +0000
@@ -466,7 +466,6 @@
};
namespace.setup_dupe_finder = function() {
- Y.log("In setup_dupe_finder");
Y.on('domready', function() {
config = {on: {success: set_up_dupe_finder,
failure: function() {}}};
@@ -485,4 +484,5 @@
};
}, "0.1", {"requires": [
- "base", "io", "oop", "node", "event", "lazr.formoverlay", "lazr.effects"]});
+ "base", "io", "oop", "node", "event", "lazr.formoverlay",
+ "lazr.effects"]});
=== modified file 'lib/lp/bugs/templates/bugtarget-bugs.pt'
--- lib/lp/bugs/templates/bugtarget-bugs.pt 2011-03-29 22:34:04 +0000
+++ lib/lp/bugs/templates/bugtarget-bugs.pt 2011-04-06 17:15:47 +0000
@@ -12,16 +12,19 @@
<metal:block fill-slot="head_epilogue">
<meta tal:condition="not: view/bug_tracking_usage/enumvalue:LAUNCHPAD"
name="robots" content="noindex,nofollow" />
- <script type="text/javascript"
- tal:condition="
- request/features/malone.advanced-structural-subscriptions.enabled">
- LPS.use('lp.registry.structural_subscription', function(Y) {
- var module = Y.lp.registry.structural_subscription;
- Y.on('domready', function() {
- module.setup({content_box: "#structural-subscription-content-box"});
- });
- });
- </script>
+ <tal:uses_launchpad_bugtracker
+ condition="view/bug_tracking_usage/enumvalue:LAUNCHPAD">
+ <script type="text/javascript"
+ tal:condition="
+ request/features/malone.advanced-structural-subscriptions.enabled">
+ LPS.use('lp.registry.structural_subscription', function(Y) {
+ var module = Y.lp.registry.structural_subscription;
+ Y.on('domready', function() {
+ module.setup({content_box: "#structural-subscription-content-box"});
+ });
+ });
+ </script>
+ </tal:uses_launchpad_bugtracker>
<style type="text/css">
p#more-hot-bugs {float:right; margin-top:7px;}
</style>
@@ -177,6 +180,10 @@
<p id="no-bugs-report"><a href="+filebug">Report a bug.</a></p>
</tal:no_hot_bugs>
+ <div class="yui-u">
+ <div id="structural-subscription-content-box"></div>
+ </div>
+
</tal:uses_launchpad_bugtracker>
<p id="no-malone"
@@ -224,10 +231,6 @@
</p>
</div>
- <div class="yui-u">
- <div id="structural-subscription-content-box"></div>
- </div>
-
</div><!-- main -->
</body>
</html>
=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js 2011-04-04 16:22:20 +0000
+++ lib/lp/registry/javascript/structural-subscription.js 2011-04-06 17:15:47 +0000
@@ -638,6 +638,9 @@
*/
function setup_overlay(content_box_id, hide_recipient_picker) {
var content_node = Y.one(content_box_id);
+ if (!Y.Lang.isValue(content_node)) {
+ Y.error("Node not found: " + content_box_id);
+ }
var container = Y.Node.create(
'<div id="overlay-container"><dl>' +
' <dt>Bug mail recipient</dt>' +
@@ -652,7 +655,8 @@
' description help</span></a> ' +
' </dd>' +
' <dt>Receive mail for bugs affecting' +
- ' <span id="structural-subscription-context-title"></span> that</dt>' +
+ ' <span id="structural-subscription-context-title"></span> that' +
+ ' </dt>' +
' <dd>' +
' <div id="events">' +
' <input type="radio" name="events"' +
@@ -690,7 +694,8 @@
' <dt></dt>' +
' <dd style="margin-left:25px;">' +
' <div id="accordion-overlay"' +
- ' style="position:relative; overflow:hidden;"></div>' +
+ ' style="position:relative; overflow:hidden;">' +
+ ' </div>' +
' </dd>' +
' </dl>' +
' </div> ' +
@@ -1337,7 +1342,7 @@
// Modify the menu-link-subscribe-to-bug-mail link to be visible.
var link = Y.one(link_id);
if (!Y.Lang.isValue(link)) {
- Y.fail('Link to set as the pop-up link not found.');
+ Y.error('Link to set as the pop-up link not found.');
}
link.removeClass('invisible-link');
link.addClass('visible-link');
@@ -1366,6 +1371,7 @@
}; // setup
}, '0.1', {requires: [
- 'dom', 'node', 'lazr.anim', 'lazr.formoverlay', 'lazr.overlay',
- 'lazr.effects', 'lp.app.errors', 'lp.client', 'gallery-accordion'
+ 'dom', 'node', 'lazr.anim', 'lazr.formoverlay',
+ 'lazr.overlay','lazr.effects', 'lp.app.errors', 'lp.client',
+ 'gallery-accordion'
]});
=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js 2011-04-04 15:59:28 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js 2011-04-06 17:15:47 +0000
@@ -298,6 +298,8 @@
_should: {
error: {
+ test_setup_overlay_missing_content_box: new Error(
+ 'Node not found: #sir-not-appearing-in-this-test')
}
},
@@ -348,6 +350,14 @@
header.get('text'));
},
+ test_setup_overlay_missing_content_box: function() {
+ // Pass in a content_box with a missing id to trigger an error.
+ this.configuration.content_box =
+ '#sir-not-appearing-in-this-test';
+ module.setup(this.configuration);
+ module._setup_overlay(this.configuration.content_box);
+ },
+
test_initial_state: function() {
// When initialized the <div> elements for the filter
// wrapper and the accordion wrapper should be collapsed.
=== modified file 'lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js'
--- lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js 2011-02-24 00:23:04 +0000
+++ lib/lp/soyuz/javascript/lp_dynamic_dom_updater.js 2011-04-06 17:15:47 +0000
@@ -344,8 +344,8 @@
* @private
*/
_handleFailure: function(id, request) {
- Y.fail("LP.DynamicDomUpdater for " + this.get("host") +
- " failed to get dynamic data.");
+ Y.error("LP.DynamicDomUpdater for " + this.get("host") +
+ " failed to get dynamic data.");
}
});
