launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03321
[Merge] lp:~danilo/launchpad/bug728370-descriptions into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug728370-descriptions into lp:launchpad with lp:~danilo/launchpad/bug728370-direct-subs as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug728370-descriptions/+merge/57656
= Bug 728370: prettyfication =
This branch prettyfies the listing of bug subscriptions on a single bug +subscriptions page. So far, we've only rendered all the potential descriptions one below the other, and this branch puts each one of them inside a separate box similar to the boxes structural subscriptions are in.
Follow-up work will introduce actions inside these "boxes" (such as "unsubscribe this team from duplicate bug").
== Implementation details ==
We are not touching structural subscriptions, other than putting them inside the "other subscriptions" slide-out box. This uncovered a few subtle annoyances in structural-subscription setup code which cleared the entire content div (and/or hard-coded it, which is why it worked), when it should have only gotten rid of the temporary overlay div. These are the only changes in structural-subscription.js and the tests still pass.
In the change itself, we only extend get_other_subscriptions_node to provide a clickable header that slides out the box. We also include some CSS to render other subscriptions in the same way as structural ones. Unfortunately, it's not trivial to reuse exactly the same CSS.
== Tests ==
lib/lp/bugs/javascript/tests/test_subscription.html
lib/lp/registry/javascript/tests/test_structural_subscription.html
== Demo and Q/A ==
See https://devpad.canonical.com/~danilo/screencasts/other-subscriptions.ogv
https://bugs.launchpad.dev/firefox/+bug/1/+subscriptions
To facilitate the demo:
- set 'malone.advanced-structural-subscriptions.enabled default 1 on'
on https://launchpad.dev/+feature-rules
- add a few structural subscriptions to firefox using
https://bugs.launchpad.dev/firefox/+subscriptions
- add a few structural subscriptions for firefox package using
https://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+subscriptions
- make bug #2 duplicate of #1, subscribe one of your teams to #2
- subscribe yourself to bug #1
...
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/subscription.js
lib/lp/bugs/javascript/tests/test_subscription.js
lib/lp/bugs/templates/bug-subscription-list.pt
lib/lp/registry/javascript/structural-subscription.js
--
https://code.launchpad.net/~danilo/launchpad/bug728370-descriptions/+merge/57656
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug728370-descriptions into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/subscription.js'
--- lib/lp/bugs/javascript/subscription.js 2011-04-13 15:47:48 +0000
+++ lib/lp/bugs/javascript/subscription.js 2011-04-14 11:16:20 +0000
@@ -100,8 +100,7 @@
/* These are the owner variations. */
var _OWNER = (
- "the owner of the {pillar_type} " +
- "{pillar}, which has no bug supervisor.");
+ "the owner of {pillar}, which has no bug supervisor.");
/* These are the actual strings to use. */
Y.mix(reasons, {
YOU_OWNER: _BECAUSE_YOU_ARE + _OWNER,
@@ -584,22 +583,73 @@
*/
function get_other_descriptions_node(info, extra_data) {
var subs = gather_nondirect_subscriptions(info);
- if (subs.length > 0) {
- var other_node = Y.Node.create('<div></div>')
+ if (subs.length > 0 || has_structural_subscriptions()) {
+ var node = Y.Node.create('<div></div>')
.set('id', 'other-subscriptions');
+ var header = Y.Node.create('<div></div>')
+ .set('id', 'other-subscriptions-header');
+ var header_link = Y.Node.create('<a></a>')
+ .set('href', '#')
+ .set('text', 'Other subscriptions');
+ header.appendChild(header_link);
+ node.appendChild(header);
+ var list = Y.Node.create('<div></div>')
+ .set('id', 'other-subscriptions-list');
+ node.appendChild(list);
+
+ setup_slider(list, header_link);
for (var index in subs) {
- other_node.appendChild(
+ list.appendChild(
get_single_description_node(subs[index], extra_data));
}
- return other_node;
+ return node;
} else {
return undefined;
}
}
namespace._get_other_descriptions_node = get_other_descriptions_node;
+/**
+ * Are there any structural subscriptions that need to be rendered.
+ */
+function has_structural_subscriptions() {
+ return (LP.cache.subscription_info &&
+ LP.cache.subscription_info.length > 0);
+}
+
+/**
+ * Sets up a slider that slides the `body` in and out when `header`
+ * is clicked.
+ */
+function setup_slider(body, header) {
+ // Hide the widget body contents.
+ body.addClass('lazr-closed');
+ body.setStyle('display', 'none');
+
+ // Ensure that the widget header uses the correct sprite icon
+ // and gets the styling for javascript actions applied.
+ header.addClass('sprite');
+ header.addClass('treeCollapsed');
+ header.addClass('js-action');
+
+ var slide;
+ function toggle_body_visibility(e) {
+ e.halt();
+ if (!slide) {
+ slide = Y.lazr.effects.slide_out(body);
+ header.replaceClass('treeCollapsed', 'treeExpanded');
+ } else {
+ slide.set('reverse', !slide.get('reverse'));
+ header.toggleClass('treeExpanded');
+ header.toggleClass('treeCollapsed');
+ }
+ slide.stop();
+ slide.run();
+ }
+ header.on('click', toggle_body_visibility);
+}
/**
* Add descriptions for all non-structural subscriptions to the page.
@@ -632,5 +682,5 @@
namespace.show_subscription_description = show_subscription_description
}, '0.1', {requires: [
- 'dom', 'node', 'substitute'
+ 'dom', 'event', 'node', 'substitute', 'lazr.effects',
]});
=== modified file 'lib/lp/bugs/javascript/tests/test_subscription.js'
--- lib/lp/bugs/javascript/tests/test_subscription.js 2011-04-13 14:19:19 +0000
+++ lib/lp/bugs/javascript/tests/test_subscription.js 2011-04-14 11:16:20 +0000
@@ -1149,6 +1149,7 @@
as_owner: _constructCategory(),
count: 1
};
+ window.LP = { cache: {} };
Y.Assert.areSame(
undefined,
module._get_other_descriptions_node(info));
@@ -1191,6 +1192,85 @@
2, node.all('.subscription-description').size());
},
+ test_no_direct_has_structural_subscriptions: function() {
+ // With no non-personal subscriptions, and a structural
+ // subscription, the node is still constructed because
+ // structural subscriptions go there as well.
+ var info = {
+ direct: _constructCategory([{ bug: {} }]),
+ from_duplicate: _constructCategory(),
+ as_assignee: _constructCategory(),
+ as_owner: _constructCategory(),
+ count: 1
+ };
+ window.LP = { cache: { subscription_info: ['1'] } };
+ Y.Assert.areNotSame(
+ undefined,
+ module._get_other_descriptions_node(info));
+ },
+
+ test_header: function() {
+ // There is a subscription on the duplicate bug.
+ var info = {
+ direct: _constructCategory(),
+ from_duplicate: _constructCategory([{ bug: {id: 1} }]),
+ as_assignee: _constructCategory(),
+ as_owner: _constructCategory(),
+ count: 1
+ };
+
+ // A returned node contains the 'other-subscriptions-header'
+ // div with the link.
+ var node = module._get_other_descriptions_node(info);
+ var header = node.one('#other-subscriptions-header');
+ Y.Assert.areNotSame(undefined, header);
+ var link = header.one('a');
+ Y.Assert.areEqual('Other subscriptions', link.get('text'));
+ },
+
+ test_header_slideout: function() {
+ // Clicking on the header slides-out the box, and
+ // clicking it again slides it back in.
+ var info = {
+ direct: _constructCategory(),
+ from_duplicate: _constructCategory([{ bug: {id: 1} }]),
+ as_assignee: _constructCategory(),
+ as_owner: _constructCategory(),
+ count: 1
+ };
+
+ // A returned node contains the 'other-subscriptions-header'
+ // div with the link.
+ var node = module._get_other_descriptions_node(info);
+ var link = node.one('#other-subscriptions-header a');
+ var list = node.one('#other-subscriptions-list');
+
+ // Initially, the list is hidden.
+ Y.Assert.isTrue(link.hasClass('treeCollapsed'));
+ Y.Assert.isTrue(list.hasClass('lazr-closed'));
+ Y.Assert.areEqual('none', list.getStyle('display'));
+
+ // Clicking the link slides out the list of other subscriptions.
+ Y.Event.simulate(Y.Node.getDOMNode(link), 'click');
+ this.wait(function() {
+ Y.Assert.isFalse(link.hasClass('treeCollapsed'));
+ Y.Assert.isTrue(link.hasClass('treeExpanded'));
+ Y.Assert.isFalse(list.hasClass('lazr-closed'));
+ Y.Assert.areNotEqual('none', list.getStyle('display'));
+
+ // Clicking it again, slides it back in.
+ // It has to be nested inside 'wait' because we need
+ // to wait for the first click to "finish".
+ Y.Event.simulate(Y.Node.getDOMNode(link), 'click');
+
+ this.wait(function() {
+ Y.Assert.isTrue(link.hasClass('treeCollapsed'));
+ Y.Assert.isFalse(link.hasClass('treeExpanded'));
+ Y.Assert.isTrue(list.hasClass('lazr-closed'));
+ }, 500);
+ }, 500);
+ },
+
}));
/**
@@ -1225,7 +1305,7 @@
bug_id: 1,
count: 0
};
- window.LP = {};
+ window.LP = { cache: {} };
module.show_subscription_description(this.config);
this.wait(function() {
Y.Assert.areEqual(
=== modified file 'lib/lp/bugs/templates/bug-subscription-list.pt'
--- lib/lp/bugs/templates/bug-subscription-list.pt 2011-04-13 14:24:36 +0000
+++ lib/lp/bugs/templates/bug-subscription-list.pt 2011-04-14 11:16:20 +0000
@@ -20,14 +20,28 @@
var ss_module = Y.lp.registry.structural_subscription;
var info_module = Y.lp.bugs.subscription;
Y.on('domready', function() {
- ss_module.setup_bug_subscriptions(
- {content_box: "#structural-subscription-content-box"});
info_module.show_subscription_description(
{description_box: "#subscription-description-box"});
});
+ Y.on('contentready', function() {
+ ss_module.setup_bug_subscriptions(
+ {content_box: "#other-subscriptions-list"});
+ }, '#other-subscriptions-list');
});
</script>
-
+ <style>
+
+ div#other-subscriptions-header {
+ margin-top: 1em;
+ }
+
+ div.subscription-description {
+ margin: 0px;
+ margin-top: 2em;
+ padding: 1em;
+ border: 1px solid #ddd;
+ }
+ </style>
</tal:head-epilogue>
</head>
<body>
=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js 2011-04-12 00:14:41 +0000
+++ lib/lp/registry/javascript/structural-subscription.js 2011-04-14 11:16:20 +0000
@@ -362,6 +362,8 @@
* Reset the overlay form to initial values.
*/
function clear_overlay(content_node, no_recipient_picker) {
+ content_node = content_node.one('#overlay-container');
+
if (no_recipient_picker) {
set_recipient_label(content_node, undefined);
} else {
@@ -693,6 +695,12 @@
var content_node = Y.one(content_box_id);
if (!Y.Lang.isValue(content_node)) {
Y.error("Node not found: " + content_box_id);
+ } else {
+ var container_node = content_node.one('#overlay-container');
+ if (Y.Lang.isValue(container_node)) {
+ container_node.remove();
+ container_node.destroy();
+ }
}
var container = Y.Node.create(
'<div id="overlay-container"><dl>' +
@@ -1032,16 +1040,14 @@
* Show an overlay for editing a subscription.
*/
function show_edit_overlay(config, subscription, filter_info, filter_id) {
- Y.one(config.content_box).empty();
- var content_node = Y.one(config.content_box),
- overlay_id = setup_overlay(config.content_box, true),
- submit_button = Y.Node.create(
+ var content_node = Y.one(config.content_box);
+ var overlay_id = setup_overlay(config.content_box, true);
+ clear_overlay(content_node, true);
+ var submit_button = Y.Node.create(
'<button type="submit" name="field.actions.create" ' +
'value="Save Changes" class="lazr-pos lazr-btn" ' +
'>OK</button>');
- clear_overlay(content_node, true);
-
var context = {
filter_info: filter_info,
filter_id: filter_id
@@ -1157,7 +1163,7 @@
* Attach activation (click) handlers to all of the edit links on the page.
*/
function wire_up_edit_links(config) {
- var listing = Y.one('#subscription-listing');
+ var listing = Y.one(config.content_box);
var subscription_info = LP.cache.subscription_info;
var filter_id = 0;
var i;
@@ -1261,7 +1267,7 @@
function fill_in_bug_subscriptions(config) {
validate_config(config);
- var listing = Y.one('#subscription-listing');
+ var listing = Y.one(config.content_box);
var subscription_info = LP.cache.subscription_info;
var top_node = Y.Node.create(
'<div class="yui-g"><div id="structural-subscriptions"></div></div>');
@@ -1542,7 +1548,6 @@
*/
function show_add_overlay(config) {
var content_node = Y.one(config.content_box);
- content_node.empty();
var overlay_id = setup_overlay(config.content_box);
clear_overlay(content_node, false);
