Merge lp:~dooferlad/launchpad/upcomingwork-expand-all into lp:launchpad

[James Tunnicliffe]

Upcoming work page should have expand all links

-- Summary
Feature Request: Upcoming work page should have expand all links. I also added collapse all and set back to default links since these seemed like logical extensions of the same feature (and likely to be requested if they weren’t present).

-- Proposed fix
Add JavaScript links to upcoming work view to expand / collapse / restore all work item lists for a blueprint.

-- Pre-implementation notes
None.

-- Implementation details
person-upcomingwork.pt updated with three new JS functions, one per link type. While initializing expanders, each expander object and its default state is stored in a new array, expanders[blueprint index][work item list index][expander object, default state]; This array is used to look up the required expanders when one of the links is selected.

-- LOC Rationale
Added a few lines because of new functionality. These will be more than offset by:
https://code.launchpad.net/~danilo/launchpad/kill-feedback-requests/+merge/106119

-- Tests
None.

-- Demo and Q/A
1. In a dev instance run http://paste.ubuntu.com/992291/ to generate some work items
2. Visit https://launchpad.dev/~hwdb-team/+upcomingwork and select the new links on the right hand side of the Blueprint column. If feeling adventurous you can create a new milestone for one of the blueprints that is within the next 60 days, but on a different day to the existing milestone and that will show up as a separate table. Links should only modify one table.

-- Lint
None.

[Curtis Hovey (sinzui)]

This JavaScript belongs in a module and requires a YUI test to verify it works. The lint report of the module will catch errors that commonly break older browsers as well as pointing out style issue. JavaScript uses 4-space identation

Avoid
    var things = new Array();
Use
   var things = [];

The floated div must be before the text "Blueprint" to display consistently across browsers. There is leading white-space.

I think this template is creating non-unique ids. This breaks many browsers. A proper test would catch this.

review: Needs Fixing (code)

[James Tunnicliffe (dooferlad)]

Thanks for the review.

I haven't started on tests yet, but I have moved the existing code around as requested (I hope), so feedback on that is welcome.

[Curtis Hovey (sinzui)]

Thank you for extracting the code.

Consider using
    !Y.Lang.isArray(expanders[index])
for
    expanders[index] === undefined
The code needs an array. or just use
   !Y.Lang.isValue(expanders[index])
if you just want to be sure there is something.

[James Tunnicliffe (dooferlad)]

Hi,

I am not sure how best to proceed with testing. I have got a test JS and HTML set up with a bit of example layout in the HTML for the JS to search through. It is my intention to run setUpWorkItemExpanders and maybe mock Y.lp.app.widgets.expander so I can avoid loading it and test that expanders array looks correct after it has run.

Other tests would start using the same template but simulate clicks to test each on click function is run when expected.

So, to do this I would need to pass in Y.lp.app.widgets.expander as a parameter to setUpWorkItemExpanders. I wasn't going to bother testing the parameters to Expander(expander_icon, widget_body) since I think the contents of expanders will demonstrate that we have extracted the data needed from the DOM.

One odd thing is that the JS console at the moment (in Chrome) contains:

TestRunner: Testing began at Fri May 25 2012 15:53:01 GMT+0100 (BST).
TestRunner: Test suite "lp.workitems.expanders Tests" started.
TestRunner: Testing began at Fri May 25 2012 15:53:01 GMT+0100 (BST).
TestRunner: Test suite "yuitests1337957581177" started.
TestRunner: Test case "setUpWorkItemExpanders test" started.
TestRunner: Test suite "yuitests1337957581177" completed.
Passed:0 Failed:0 Total:0 (0 ignored)
TestRunner: Testing completed at Fri May 25 2012 15:53:01 GMT+0100 (BST).
Passed:0 Failed:0 Total:0 (0 ignored)
TestRunner: test_setUpWorkItemExpanders: failed.
Unexpected error: Cannot call method 'reversible_slide_out' of undefined
TestRunner: Test case "setUpWorkItemExpanders test" completed.
Passed:0 Failed:1 Total:1 (0 ignored)
TestRunner: Test suite "lp.workitems.expanders Tests" completed.
Passed:0 Failed:1 Total:1 (0 ignored)
TestRunner: Testing completed at Fri May 25 2012 15:53:03 GMT+0100 (BST).
Passed:0 Failed:1 Total:1 (0 ignored)

I don't know where the Test suite "yuitests<random number>" came from. Only they are visible in the browser based console. I have pushed my branch so you can have a look at it, if you want to.

[Curtis Hovey (sinzui)]

You can ignore the random numbers. YUI assigns a unique id to every node it creates.

The test-suite should only test the functions in the code you wrote. We want to verify that the correct function is wired to the correct element. Since the functions are anonymous, I imagine we want to test that the element has or does not have the "hidden" class.

According to the module you created, it only requires lp.app.widgets.expander, so all but the last dependency can be deleted from the html file.

The html file is not setup to be reset by the test. Take a look at
    lib/lp/code/javascript/tests/test_util.*
Which uses the 'type="text/x-template"' technique to create html fragments for tests. The setup and teardown in the test-suite updates the fixture node to ensure each test has pristine markup.

test_util.js is testing several setup functions that also do hide and show (but not expanders). You might find this test helpful because it illustrates how to verify a method was called during an event, then checks the final state of the element. I think this style of test could be adapted by passing in an example expander config.

[James Tunnicliffe (dooferlad)]

I believe I have addressed the previous comments. Please re-review.

Thanks!

review: Resubmit

[Curtis Hovey (sinzui)]

Lines 256 and 269 are missing the space between the keyword and the predicate:
    if(
should be
    if (

This looks good. I can land this once you fix the space.

review: Approve (code)

[James Tunnicliffe (dooferlad)]

Done.

=== added directory 'lib/lp/blueprints/javascript'
=== added directory 'lib/lp/blueprints/javascript/tests'
=== added file 'lib/lp/blueprints/javascript/tests/test_workitems.html'
--- lib/lp/blueprints/javascript/tests/test_workitems.html	1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/javascript/tests/test_workitems.html	2012-06-13 15:31:25 +0000
@@ -0,0 +1,211 @@
+<!DOCTYPE html>
+<!--
+Copyright 2012 Canonical Ltd.  This software is licensed under the
+GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<html>
+<head>
+  <title>Test Work Item Expanders</title>
+
+  <!-- YUI and test setup -->
+  <script type="text/javascript"
+          src="../../../../../build/js/yui/yui/yui.js">
+  </script>
+  <link rel="stylesheet"
+        href="../../../../../build/js/yui/console/assets/console-core.css" />
+  <link rel="stylesheet"
+        href="../../../../../build/js/yui/console/assets/skins/sam/console.css" />
+  <link rel="stylesheet"
+        href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
+
+  <script type="text/javascript"
+          src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
+
+  <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+
+  <!-- Dependencies -->
+  <script type="text/javascript"
+          src="../../../../../build/js/lp/app/effects/effects.js"></script>
+  <script type="text/javascript"
+          src="../../../../../build/js/lp/app/expander.js"></script>
+
+  <!-- The module under test. -->
+  <script type="text/javascript" src="../workitems.js"></script>
+
+  <!-- The test suite. -->
+  <script type="text/javascript" src="test_workitems.js"></script>
+
+</head>
+<body class="yui3-skin-sam">
+<ul id="suites">
+  <li>lp.workitems.expanders</li>
+</ul>
+<!-- Example markup required by test suite -->
+<div id="test-root">
+  <div id="fixture"></div>
+
+  <script type="text/x-template" id="work-items-test-0">
+  <div class="workitems-group" id="milestone_0">
+    <table class="listing">
+      <thead>
+      <tr>
+        <th>
+          <div style="float: left; font-weight: normal;">All:
+            <a href="#expandall" class="expandall_link"
+               id="expand_milestone_0">Expand</a>
+            <a href="#collapseall" class="collapseall_link"
+               id="collapse_milestone_0">Collapse</a>
+            <a href="#defaultall" class="defaultall_link"
+               id="default_milestone_0">Default</a>
+          </div>
+        </th>
+      </tr>
+      </thead>
+
+      <tbody>
+      <tr class="expandable">
+        <td>
+          <a href="#" class="expander">.</a>
+        </td>
+      </tr>
+      </tbody>
+
+      <tbody class="collapsible-body default-collapsed">
+      <tr class="padded">
+        <td>
+          Content.
+        </td>
+      </tr>
+      </tbody>
+
+      <tbody>
+      <tr class="expandable">
+        <td>
+          <a href="#" class="expander">.</a>
+        </td>
+      </tr>
+      </tbody>
+
+      <tbody class="collapsible-body default-expanded">
+      <tr class="padded">
+        <td>
+          Content.
+        </td>
+      </tr>
+      </tbody>
+
+    </table>
+  </div>
+  </script>
+
+  <script type="text/x-template" id="work-items-test-default-collapsed">
+    <div class="workitems-group" id="milestone_1">
+      <table class="listing">
+        <thead>
+        <tr>
+          <th>
+            <div style="float: left; font-weight: normal;">All:
+              <a href="#expandall" class="expandall_link"
+                 id="expand_milestone_1">Expand</a>
+              <a href="#collapseall" class="collapseall_link"
+                 id="collapse_milestone_1">Collapse</a>
+              <a href="#defaultall" class="defaultall_link"
+                 id="default_milestone_1">Default</a>
+            </div>
+          </th>
+        </tr>
+        </thead>
+
+        <tbody>
+        <tr class="expandable">
+          <td>
+            <a href="#" class="expander">.</a>
+          </td>
+        </tr>
+        </tbody>
+
+        <tbody class="collapsible-body default-collapsed">
+        <tr class="padded">
+          <td>
+            Content.
+          </td>
+        </tr>
+        </tbody>
+
+        <tbody>
+        <tr class="expandable">
+          <td>
+            <a href="#" class="expander">.</a>
+          </td>
+        </tr>
+        </tbody>
+
+        <tbody class="collapsible-body default-collapsed">
+        <tr class="padded">
+          <td>
+            Content.
+          </td>
+        </tr>
+        </tbody>
+
+      </table>
+    </div>
+  </script>
+
+  <script type="text/x-template" id="work-items-test-default-expanded">
+    <div class="workitems-group" id="milestone_2">
+      <table class="listing">
+        <thead>
+        <tr>
+          <th>
+            <div style="float: left; font-weight: normal;">All:
+              <a href="#expandall" class="expandall_link"
+                 id="expand_milestone_2">Expand</a>
+              <a href="#collapseall" class="collapseall_link"
+                 id="collapse_milestone_2">Collapse</a>
+              <a href="#defaultall" class="defaultall_link"
+                 id="default_milestone_2">Default</a>
+            </div>
+          </th>
+        </tr>
+        </thead>
+
+        <tbody>
+        <tr class="expandable">
+          <td>
+            <a href="#" class="expander">.</a>
+          </td>
+        </tr>
+        </tbody>
+
+        <tbody class="collapsible-body default-expanded">
+        <tr class="padded">
+          <td>
+            Content.
+          </td>
+        </tr>
+        </tbody>
+
+        <tbody>
+        <tr class="expandable">
+          <td>
+            <a href="#" class="expander">.</a>
+          </td>
+        </tr>
+        </tbody>
+
+        <tbody class="collapsible-body default-expanded">
+        <tr class="padded">
+          <td>
+            Content.
+          </td>
+        </tr>
+        </tbody>
+
+      </table>
+    </div>
+  </script>
+</div>
+</body>
+</html>

=== added file 'lib/lp/blueprints/javascript/tests/test_workitems.js'
--- lib/lp/blueprints/javascript/tests/test_workitems.js	1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/javascript/tests/test_workitems.js	2012-06-13 15:31:25 +0000
@@ -0,0 +1,153 @@
+YUI().use('lp.testing.runner', 'test', 'console', 'node', 'lazr.picker',
+          'lp.workitems.expanders',
+          'event', 'node-event-simulate', 'dump', function(Y) {
+
+var suite = new Y.Test.Suite("lp.workitems.expanders Tests");
+var module = Y.lp.workitems.expanders;
+
+suite.add(new Y.Test.Case({
+    name: 'setUpWorkItemExpanders test',
+
+    setUp: function() {
+        this.fixture = Y.one("#fixture");
+        module.test__ping_called = false;
+    },
+
+    tearDown: function () {
+        if (this.fixture !== null) {
+          this.fixture.empty();
+        }
+        delete this.fixture;
+        delete module.test__ping_called;
+    },
+
+    _setup_fixture: function(template_selector) {
+        var template = Y.one(template_selector).getContent();
+        var test_node = Y.Node.create(template);
+        this.fixture.append(test_node);
+    },
+
+    _all_expanders_are_closed: function(){
+        var found_open = false;
+        Y.all('.collapsible-body').each(function(e) {
+            found_collapsible_body = true;
+            if (!e.hasClass('unseen'))
+            {
+                found_open = true;
+                return;
+            }
+        });
+        return found_open === false;
+    },
+
+    _all_expanders_are_open: function(){
+        var found_closed = false;
+        Y.all('.collapsible-body').each(function(e) {
+            found_collapsible_body = true;
+            if (e.hasClass('unseen'))
+            {
+                found_closed = true;
+                return;
+            }
+        });
+        return found_closed === false;
+    },
+
+    test_setUpWorkItemExpanders: function() {
+        this._setup_fixture('#work-items-test-0');
+
+        Y.all('.collapsible-body').each(function(e) {
+            Y.Assert.isFalse(e.hasClass('lazr-closed'));
+        });
+
+        Y.all('[class=expandable]').each(function(e) {
+            module._add_expanders(e);
+        });
+
+        Y.all('.collapsible-body').each(function(e) {
+            // For some reason lazr-closed is attached when expander bodies are
+            // in their default state. Once clicked open they get lazr-open
+            // and clicked closed they get the classes lazr-closed and unseen.
+            // This assert is more encapsulating in case this changes.
+            Y.Assert.isTrue(e.hasClass('lazr-closed') ||
+                            e.hasClass('lazr-open'));
+        });
+    },
+
+    attach_ping_catcher: function(event){
+        module._attach_handler(event, function(){
+            module.test__ping_called = true;
+        });
+    },
+
+    test_attach_handler: function() {
+        // Test that _attach_handler attaches a function as expected. This
+        // covers expand, collapse and default functions.
+        this._setup_fixture('#work-items-test-0');
+
+        // Call the expander attach handler function
+        Y.all('.expandall_link').on("click", this.attach_ping_catcher);
+
+        // Check that it attached correctly
+        Y.one('.expandall_link').simulate('click');
+        Y.Assert.isTrue(module.test__ping_called);
+    },
+
+    test_default_closed: function() {
+        // Test that clicking the default link restores expanders to their
+        // initial state.
+
+        this._setup_fixture('#work-items-test-default-collapsed');
+        module.setUpWorkItemExpanders({ no_animation: true });
+
+        // The test document should have all expanders collapsed. Check this.
+        Y.Assert.isTrue(this._all_expanders_are_closed());
+
+        // Expand everything
+        Y.one('.expandall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_open());
+
+        // Return to default (collapsed)
+        Y.one('.defaultall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_closed());
+
+        // Collapse everything (should be no change)
+        Y.one('.collapseall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_closed());
+
+        // Check default link leaves everything closed
+        Y.one('.defaultall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_closed());
+    },
+
+    test_default_open: function() {
+        // Test that clicking the default link restores expanders to their
+        // initial state.
+
+        this._setup_fixture('#work-items-test-default-expanded');
+        module.setUpWorkItemExpanders({ no_animation: true });
+
+        // The test document should have all expanders collapsed. Check this.
+        Y.Assert.isTrue(this._all_expanders_are_closed());
+
+        // Expand everything
+        Y.one('.expandall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_open());
+
+        // Set to default (should be no change)
+        Y.one('.defaultall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_open());
+
+        // Collapse everything
+        Y.one('.collapseall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_closed());
+
+        // Check default link opens everything back up
+        Y.one('.defaultall_link').simulate('click');
+        Y.Assert.isTrue(this._all_expanders_are_open());
+    }
+}));
+
+Y.lp.testing.Runner.run(suite);
+
+});

=== added file 'lib/lp/blueprints/javascript/workitems.js'
--- lib/lp/blueprints/javascript/workitems.js	1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/javascript/workitems.js	2012-06-13 15:31:25 +0000
@@ -0,0 +1,94 @@
+/* Copyright 2012 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Setup for managing subscribers list for bugs.
+ *
+ * @module workitems
+ * @submodule expanders
+ */
+
+YUI.add('lp.workitems.expanders', function(Y) {
+
+    var namespace = Y.namespace('lp.workitems.expanders');
+
+    /**
+     * Record of all expanders and their default state.
+     */
+    var expanders = [];
+
+    function expander_expand(expander, i){
+        expander[0].render(true, false);
+    }
+    namespace._expander_expand = expander_expand;
+
+    function expander_collapse(expander, i){
+        expander[0].render(false, false);
+    }
+    namespace._expander_collapse = expander_collapse;
+
+    function expander_restore_default_state(expander, i){
+        expander[0].render(expander[1], false);
+    }
+    namespace._expander_restore_default_state = expander_restore_default_state;
+
+    /**
+     * Attach an expander to each expandable in the page.
+     */
+    function setUpWorkItemExpanders(expander_config){
+        Y.all('[class=expandable]').each(function(e) {
+            add_expanders(e, expander_config);
+        });
+
+        Y.all('.expandall_link').on("click", function(event){
+            attach_handler(event, expander_expand);
+        });
+
+        Y.all('.collapseall_link').on("click", function(event){
+            attach_handler(event, expander_collapse);
+        });
+
+        Y.all('.defaultall_link').on("click", function(event){
+            attach_handler(event, expander_restore_default_state);
+        });
+    }
+    namespace.setUpWorkItemExpanders = setUpWorkItemExpanders;
+
+    function add_expanders(e, expander_config){
+        var expander_icon = e.one('[class=expander]');
+        // Our parent's first sibling is the tbody we want to collapse.
+        var widget_body = e.ancestor().next();
+
+        if (Y.Lang.isUndefined(expander_config))
+        {
+            expander_config = {};
+        }
+
+        var expander = new Y.lp.app.widgets.expander.Expander(expander_icon,
+                                                              widget_body,
+                                                              expander_config);
+        expander.setUp(true);
+
+        var index = e.ancestor('[class=workitems-group]').get('id');
+
+        // We record the expanders so we can reference them later
+        // First we have an array indexed by each milestone
+        if (!Y.Lang.isValue(expanders[index])){
+            expanders[index] = [];
+        }
+
+        // For each milestone, store an array containing the expander
+        // object and the default state for it
+        default_expanded = widget_body.hasClass('default-expanded');
+        expanders[index].push(new Array(expander, default_expanded));
+    }
+    namespace._add_expanders = add_expanders;
+
+    function attach_handler(event, func){
+        var index = event.currentTarget.get('id');
+        index = index.match(/milestone_\d+/)[0];
+        Y.Array.forEach(expanders[index], func);
+    }
+    namespace._attach_handler = attach_handler;
+
+}, "0.1", {"requires": ["lp.app.widgets.expander"]});
+

=== modified file 'lib/lp/registry/templates/person-upcomingwork.pt'
--- lib/lp/registry/templates/person-upcomingwork.pt	2012-05-31 02:20:41 +0000
+++ lib/lp/registry/templates/person-upcomingwork.pt	2012-06-13 15:31:25 +0000
@@ -10,16 +10,9 @@
 <head>
   <tal:block metal:fill-slot="head_epilogue">
     <script type="text/javascript">
-      LPJS.use('node', 'event', 'lp.app.widgets.expander', function(Y) {
+      LPJS.use('node', 'event', 'lp.app.widgets.expander', 'lp.workitems.expanders', function(Y) {
           Y.on('domready', function() {
-            Y.all('[class=expandable]').each(function(e) {
-              var expander_icon = e.one('[class=expander]');
-              // Our parent's first sibling is the tbody we want to collapse.
-              var widget_body = e.ancestor().next();
-              var expander = new Y.lp.app.widgets.expander.Expander(
-                  expander_icon, widget_body);
-              expander.setUp(true);
-            })
+            Y.lp.workitems.expanders.setUpWorkItemExpanders();
           })
         });
     </script>
@@ -38,7 +31,8 @@
 
 <div metal:fill-slot="main">
 
-  <div tal:repeat="pair view/work_item_containers" class="workitems-group">
+  <div tal:repeat="pair view/work_item_containers" class="workitems-group"
+       tal:attributes="id string:milestone_${repeat/pair/index}">
   <div tal:define="date python: pair[0]; containers python: pair[1]">
     <h2>Work items due in <span tal:replace="date/fmt:date" /></h2>
 
@@ -78,7 +72,17 @@
     <table class="listing">
       <thead>
       <tr>
-        <th>Blueprint</th>
+        <th>
+          <div style="float: right; font-weight: normal;">All:
+            <a href="#expandall" class="expandall_link"
+               tal:attributes="id string:expand_milestone_${repeat/pair/index}">Expand</a>
+            <a href="#collapseall" class="collapseall_link"
+               tal:attributes="id string:collapse_milestone_${repeat/pair/index}">Collapse</a>
+            <a href="#defaultall" class="defaultall_link"
+               tal:attributes="id string:default_milestone_${repeat/pair/index}">Default</a>
+          </div>
+          Blueprint
+        </th>
         <th>Target</th>
         <th>Assignee</th>
         <th>Priority</th>
@@ -111,13 +115,15 @@
 
       <tal:conditional condition="container/has_incomplete_work">
         <tal:block define="global upcoming_work_class_name string:expanded"/>
+        <tal:block define="global expander_init_state string:default-expanded"/>
       </tal:conditional>
 
       <tal:conditional condition="not: container/has_incomplete_work">
         <tal:block define="global upcoming_work_class_name string:"/>
+        <tal:block define="global expander_init_state string:default-collapsed"/>
       </tal:conditional>
 
-      <tbody tal:attributes="class string:collapsible-body ${upcoming_work_class_name}">
+      <tbody tal:attributes="class string:collapsible-body ${upcoming_work_class_name} ${expander_init_state}">
         <tr tal:repeat="workitem container/items" class="padded">
           <td>
             <span tal:condition="not: container/spec|nothing"