Merge lp:~danilo/launchpad/bug728370-descriptions into lp:launchpad

[Данило Шеган]

[r=allenap][bug=728370][incr] Make bug subscription descriptions nicer and collapse all the non-direct non-personal along with structural subscriptions into "other subscriptions" box. You do what?

= 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

[Gavin Panella (allenap)]

Wow, the subscriptions stuff is looking *awesome* these days.

This branch looks good. I have a few comments, but nothing major.

[1]

+ var header_link = Y.Node.create('<a></a>')
+ .set('href', '#')

Is this needed? Ah, maybe for IE?

[2]

+ Y.Assert.areNotSame(
+ undefined,

Y.Assert.isNotUndefined() is slightly more concise.

[3]

+ var node = module._get_other_descriptions_node(info);
+ var link = node.one('#other-subscriptions-header a');
+ var list = node.one('#other-subscriptions-list');

IDs are /meant/ to be unique in the page so you could skip the first
line above and just use Y.one().

[4]

+ this.wait(function() {
[...]
+ this.wait(function() {
[...]
+ }, 500);
+ }, 500);

Are these delays long enough? How long is the animation meant to take?

[5]

- 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');

http://developer.yahoo.com/yui/3/examples/event/event-timing.html
seems to suggest that it's not safe to modify the DOM until domready
fires:

    The 'contentready' event fired for the element
    'contentContainer'. That element and all of its children are
    present in the DOM.

    The 'domready' event fired. The DOM is now safe to modify via
    script.

setup_bug_subscriptions() calls at least one function that modifies
the DOM, so I suspect this change should be reverted.

[6]

+ content_node = content_node.one('#overlay-container');

Like before, and in a few other places, you can just use Y.one() here,
not that it matters much.

Becauase IDs are, afaik, meant to be unique in a DOM tree, I dislike
them; they're like the global variables of the DOM, and I try to avoid
using them, especially in widgets. Do you have any opinions either
way, or are you aware of anyone else's?

review: Approve

[Данило Шеган (danilo)]

У чет, 14. 04 2011. у 13:22 +0000, Gavin Panella пише:
> Review: Approve
> Wow, the subscriptions stuff is looking *awesome* these days.

Thanks. It's all the yellow-goo over it :)
Though, we are building on the sound foundation you guys did
previously :)

> This branch looks good. I have a few comments, but nothing major.
>
>
> [1]
>
> + var header_link = Y.Node.create('<a></a>')
> + .set('href', '#')
>
> Is this needed? Ah, maybe for IE?

Not needed at all, other than the mouse pointer doesn't turn into a
"click me" pointer. Basically, the <a> tag is considered an anchor
instead of a link. I am thinking of keeping it, any better suggestions?

>
> [2]
>
> + Y.Assert.areNotSame(
> + undefined,
>
> Y.Assert.isNotUndefined() is slightly more concise.

Ah, thanks. Fixed.

>
> [3]
>
> + var node = module._get_other_descriptions_node(info);
> + var link = node.one('#other-subscriptions-header a');
> + var list = node.one('#other-subscriptions-list');
>
> IDs are /meant/ to be unique in the page so you could skip the first
> line above and just use Y.one().

Except that the stuff is not added to the page DOM yet. The method
returns a node which other methods are in charge of adding to the page:
and since this is trying to be more of a unit test, I wouldn't want to
add it.

> [4]
>
> + this.wait(function() {
> [...]
> + this.wait(function() {
> [...]
> + }, 500);
> + }, 500);
>
> Are these delays long enough? How long is the animation meant to take?

They consistently worked for me (and anything lower consistently failed
for me). It's taking whatever the default is for animation. I'll check
what it is and use that + some small leverage.

>
> [5]
>
> - 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');
>
> http://developer.yahoo.com/yui/3/examples/event/event-timing.html
> seems to suggest that it's not safe to modify the DOM until domready
> fires:
>
> The 'contentready' event fired for the element
> 'contentContainer'. That element and all of its children are
> present in the DOM.
>
> The 'domready' event fired. The DOM is now safe to modify via
> script.
>
> setup_bug_subscriptions() calls at least one function that modifies
> the DOM, so I suspect this change should be reverted.

Well, it's put inside 'contentready' for #other-subscriptions-list,
which is constructed in show_subscription_description, which is called
in 'domready' event. '#other-subscriptions-list' is *not* present in
the DOM before show_subscription_description() runs.

>
> [6]
>
> + content_node = content_node.one('#overlay-container');
>
> Like before, and in a few other places, you can just use Y.one() here,
...

=== 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 15:39:15 +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 15:39:15 +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,84 @@
             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.isNotUndefined(
+            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.isNotUndefined(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 +1304,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 15:39:15 +0000
@@ -20,14 +20,31 @@
           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() {
+              // This happens only after show_subscription_description()
+              // is called in 'domready' handler, because that's what
+              // inserts '#other-subscriptions-list' into DOM.
+              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 15:39:15 +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);