Merge lp:~jcsackett/launchpad/hidden-project-configuration-636000 into lp:launchpad

[j.c.sackett]

Wraps configuration links in the product involvement portlet with a collapsible that starts collapsed when all configuration steps have been taken.

Summary
=======

As part of bridging-the-gap, a progress bar and other visual indicators were introduced to encourage users to fully configure their projects so we could get upstream data.

Now, when you finish configuring everything, you're still shown all the configuration links despite also being told that you're "done" configuring. To make this less visually irritating but still allow configuration to be changed easily on a fully configured product, this branch introduces a show/hide control around the configuration links. It defaults to showing if configuration isn't done, and hiding otherwise. It's largely borrowed from the show/hide of "Extra options" on other parts of Launchpad (e.g. merge proposals).

Proposed fix
============

Create show/hide control on the configuration links so you're not bothered with the links when you no longer need them, but can still access them.

Preimplementation talk
======================

Spoke with Curtis Hovey about branch goals. Spoke with Edwin Grubbs about setting up javascript in Launchpad and testing it.

Implementation details
======================

The product view has a new method that returns a boolean showing whether or not registration is done. This is used to set the classes on the block showing the configuration links.

A new javascript file, pillar.js, provides a way to set up div marked as collapsible in the same way collapsible fieldsets work in other places in the site. This uses the same toggle collapsible function to provide the actual functionality.

Tests
=====

bin/test -m lp.registry.browser.tests.test_product
bin/test -m lp.registry.windmill.tests.test_product_configuration_hidden

Screenshots
===========

The section shown: http://people.canonical.com/~jc/images/shown.png
The section hidden: http://people.canonical.com/~jc/images/hidden.png

QA
==

Open http://launchpad.dev/firefox. It should start with the configuration area shown, with two configuration steps left. Configure them both (it doesn't matter how, just make sure they're set up beyond a default UNKNOWN state).

Open http://launchpad.dev/firefox again. It should start with the configuration links hidden.

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/templates/base-layout-macros.pt
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/javascript/pillar.js
  lib/lp/registry/javascript/team.js
  lib/lp/registry/templates/pillar-involvement-portlet.pt
  lib/lp/registry/templates/product-index.pt
  lib/lp/registry/windmill/tests/test_product_configuration_hidden.py
  lib/lp/testing/service_usage_helpers.py

./lib/lp/registry/javascript/team.js
     137: Line exceeds 78 characters.

The line that exceeds 78 characters isn't part of the diff, and is arguably most readable at its current length.

[Leonard Richardson (leonardr)]

After a lot of questions I've just got a tech-debt bug that needs filing, a question, and some wording suggestions. I also have some questions I'd like the UI reviewer to answer.

The tech-debt bug: you copy a big chunk of code into activate_collapsible_div. We talked about this on IRC and you explained that you'd tried refactoring, but changing the code to work on <div> instead of <fieldset> made it a huge pain. We agreed that you'd file a tech-debt bug and put an XXX in the code.

The question: I don't know if it makes sense to use waitForPageLoad when the page has already loaded. Is there a better way to wait for some Javascript to run? If so, please change this.

Wording suggestions:

1. s/"safe use this"/"safely use this"
2. In your explanatory text, you need to distinguish between the configuration links and the collapsible element that *contains* the configuration links.

2a. # When the Show link is clicked when it's open, it closes it.
=>
# When the collapsable element is open, clicking the Show link will close it.

2b. "Additionally, on a not fully configured project, it starts by showing
the links, and can be closed."

=>

"On a not fully configured project, the collapsable element starts off
open and displaying the configuration links, but it can be closed."

Questions that I defer to the UI reviewer:

1. Do you need that <br /> on line 270? If so, maybe it should also be conditional on view/configuration_links? A <br /> with nothing after it doesn't look good.
2. "Configuration links" is a little vague and passive. Maybe "Configure this project" would be better wording?

review: Approve (code)

[Leonard Richardson (leonardr)]

Oh, you also copied some CSS--put that in the tech-debt bug as well.

[j.c.sackett (jcsackett)]

I'm only addressing part of the review in this comment--I'm investigating some things related to the other parts. In particular I may be able to remove the tech debt after all. Funny how things change when you look at code again.

> Wording suggestions:
>
> 1. s/"safe use this"/"safely use this"
> 2. In your explanatory text, you need to distinguish between the configuration links and the collapsible element that *contains* the configuration links.
>
> 2a. # When the Show link is clicked when it's open, it closes it.
> =>
> # When the collapsable element is open, clicking the Show link will close it.
>
> 2b. "Additionally, on a not fully configured project, it starts by showing
> the links, and can be closed."
>
> =>
>
> "On a not fully configured project, the collapsable element starts off
> open and displaying the configuration links, but it can be closed."

I've updated the comments.

> Questions that I defer to the UI reviewer:
>
> 1. Do you need that <br /> on line 270? If so, maybe it should also be conditional on view/configuration_links? A <br /> with nothing after it doesn't look good.

I've killed the br. It was there b/c I thought the spacing looked a little off when I was cleaning this up for review, but on second thought I think it may be better without. So I suppose the new question for the UI reviewer is "do you have suggestions for spacing between the show/hide toggle and the actual links?"

> 2. "Configuration links" is a little vague and passive. Maybe "Configure this project" would be better wording?

I borrowed the passivity from other collapse toggles on the site. Most places we use it just show "Extra options" as the link (e.g. merge proposals). I'm not in love with the language at all. Suggestion from the UI review heartily encouraged.

[j.c.sackett (jcsackett)]

> The tech-debt bug: you copy a big chunk of code into activate_collapsible_div. We talked about this on IRC and you explained that you'd tried refactoring, but changing the code to work on <div> instead of <fieldset> made it a huge pain. We agreed that you'd file a tech-debt bug and put an XXX in the code.

So, in the clarity of a new day I realized that instead of all the complicated "what sort of node am I looking at" logic I was getting into I could just double check for a span with id=legend; throw in some of the structural changes I made to the HTML and the rest of the javascript nastiness was removed.

So now it's a few line change to the regular activate_collapsible function in lp.app.javascript.lp.js

I think everyone can agree that's a good bit better.

> The question: I don't know if it makes sense to use waitForPageLoad when the page has already loaded. Is there a better way to wait for some Javascript to run? If so, please change this.

Turns out forElement can take classname selectors, not just element types. So that's the right kind of wait and the code has been updated.

[Henning Eggers (henninge)]

Thank you for this nice improvement and making Launchpad easier on the eyes ... ;-)

We chatted on IRC and came up with:
- Rename "Configuration links" to "Configuration options"
- Put a 0.5em margin under it.

review: Approve (ui)

=== modified file 'lib/lp/app/javascript/lp.js'
--- lib/lp/app/javascript/lp.js	2010-01-21 22:21:01 +0000
+++ lib/lp/app/javascript/lp.js	2010-11-03 15:22:53 +0000
@@ -104,7 +104,13 @@
         // Grab the collapsibles.
         Y.all('.collapsible').each(function(collapsible) {
 
+            // Try to grab the legend in the usual way.
             var legend = collapsible.one('legend');
+            if (legend === null) {
+                // If it's null, this might be a collapsible div, not fieldset,
+                // so try to grap the div's "legend".
+                legend = collapsible.one('.config-options');
+            }
             if (legend === null ||
                 legend.one('.collapseIcon') !== null) {
                 // If there's no legend there's not much we can do,

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2010-11-03 00:40:35 +0000
+++ lib/lp/registry/browser/product.py	2010-11-03 15:22:53 +0000
@@ -473,6 +473,11 @@
         undone = scale - done
         return dict(done=done, undone=undone)
 
+    @property
+    def registration_done(self):
+        """A boolean indicating that the services are fully configured."""
+        return (self.registration_completeness['done'] == 100)
+
 
 class ProductNavigationMenu(NavigationMenu):
 

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2010-09-27 18:18:11 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2010-11-03 15:22:53 +0000
@@ -12,6 +12,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.enums import ServiceUsage
 from lp.registry.browser.product import ProductLicenseMixin
 from lp.registry.interfaces.product import License
 from lp.testing import (
@@ -19,6 +20,7 @@
     TestCaseWithFactory,
     )
 from lp.testing.mail_helpers import pop_notifications
+from lp.testing.service_usage_helpers import set_service_usage
 from lp.testing.views import create_view
 
 
@@ -102,5 +104,36 @@
         self.assertEqual('2005-06-15', result)
 
 
+class TestProductConfiguration(TestCaseWithFactory):
+    """Tests the configuration links and helpers."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProductConfiguration, self).setUp()
+        self.product = self.factory.makeProduct()
+
+    def test_registration_not_done(self):
+        # The registration done property on the product index view
+        # tells you if all the configuration work is done, based on
+        # usage enums.
+
+        # At least one usage enum is unknown, so registration done is false.
+        self.assertEqual(
+            self.product.codehosting_usage,
+            ServiceUsage.UNKNOWN)
+        view = create_view(self.product, '+get-involved')
+        self.assertFalse(view.registration_done)
+
+        set_service_usage(
+            self.product.name,
+            codehosting_usage="EXTERNAL",
+            bug_tracking_usage="LAUNCHPAD",
+            answers_usage="EXTERNAL",
+            translations_usage="NOT_APPLICABLE")
+        view = create_view(self.product, '+get-involved')
+        self.assertTrue(view.registration_done)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/javascript/team.js'
--- lib/lp/registry/javascript/team.js	2010-07-15 10:59:22 +0000
+++ lib/lp/registry/javascript/team.js	2010-11-03 15:22:53 +0000
@@ -1,8 +1,8 @@
-/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
- *
- * Objects for subscription handling.
- *
- * @module lp.subscriber
+/** Copyright (c) 2009-2010, Canonical Ltd. All rights reserved.
+ *
+ * Team add member animations and ui.
+ *
+ * @module lp.registry.team
  */
 
 YUI.add('lp.registry.team', function(Y) {

=== modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
--- lib/lp/registry/templates/pillar-involvement-portlet.pt	2010-09-14 21:45:15 +0000
+++ lib/lp/registry/templates/pillar-involvement-portlet.pt	2010-11-03 15:22:53 +0000
@@ -34,12 +34,12 @@
   <tal:editor condition="context/required:launchpad.Edit"
               define="registration view/registration_completeness">
 
-    <tal:show_configuration condition="registration">
+    <tal:show-registration condition="registration">
       <h2>Configuration Progress</h2>
       <div class="centered" id="progressbar">
-          <table width="80%" border="1">
-            <tr>
-              <td>
+        <table width="80%" border="1">
+          <tr>
+            <td>
               <img
                  tal:attributes="alt string:${registration/done}% registration complete;
                                  title string:${registration/done}% registration complete;
@@ -47,28 +47,56 @@
                                  style string:width: ${registration/done}%;"
                  src="/@@/green-bar"
                  height="10"/>
+            </td>
+          </tr>
+        </table>
+      </div>
+
+      <div id="show-hide-configs"
+         tal:attributes="class
+           python: 'collapsible collapsed'
+             if view.registration_done
+             else 'collapsible'">
+        <div class="config-options"
+             style="margin-bottom: 0.5em">Configuration options</div>
+        <table
+          tal:condition="view/configuration_links"
+          style="width: 100%; padding-top: 1em"
+          id="configuration_links">
+          <tal:item repeat="link_status view/configuration_links">
+            <tr>
+              <td>
+                <a tal:replace="structure link_status/link/fmt:link" />
+              </td>
+
+              <td style="text-align: right;" tal:condition="registration">
+                <tal:yes-no
+                  replace="structure link_status/configured/image:boolean"/>
               </td>
             </tr>
-          </table>
+          </tal:item>
+        </table>
       </div>
-    </tal:show_configuration>
-
-    <table style="width: 100%; padding-top: 1em"
-           tal:condition="view/configuration_links"
-           id="configuration_links">
-      <tal:item repeat="link_status view/configuration_links">
-        <tr>
-          <td>
-            <a tal:replace="structure link_status/link/fmt:link" />
-          </td>
-
-          <td style="text-align: right;" tal:condition="registration">
-            <tal:yes-no replace="structure link_status/configured/image:boolean" />
-          </td>
-
-        </tr>
-      </tal:item>
-    </table>
-
+    </tal:show-registration>
+
+    <tal:no-registration condition="not:registration">
+      <table
+        tal:condition="view/configuration_links"
+        style="width: 100%; padding-top: 1em"
+        id="configuration_links">
+        <tal:item repeat="link_status view/configuration_links">
+          <tr>
+            <td>
+              <a tal:replace="structure link_status/link/fmt:link" />
+            </td>
+
+            <td style="text-align: right;" tal:condition="registration">
+              <tal:yes-no
+                replace="structure link_status/configured/image:boolean"/>
+            </td>
+          </tr>
+        </tal:item>
+      </table>
+    </tal:no-registration>
   </tal:editor>
 </div>

=== modified file 'lib/lp/registry/templates/product-index.pt'
--- lib/lp/registry/templates/product-index.pt	2010-09-30 16:54:32 +0000
+++ lib/lp/registry/templates/product-index.pt	2010-11-03 15:22:53 +0000
@@ -12,6 +12,39 @@
     <tal:registrant replace="structure context/registrant/fmt:link" />
   </tal:registering>
 
+<head>
+  <tal:head-epilogue metal:fill-slot="head_epilogue">
+    <style type="text/css">
+      div.collapsible {
+        border:medium none;
+        margin:0;
+        padding:16px 0 0;
+      }
+
+      div.collapsed {
+        display: None
+      }
+    </style>
+
+    <noscript>
+      <style type="text/css">
+        div.collapsible, div.collapsed {display: block;}
+      </style>
+    </noscript>
+
+        <script type="text/javascript"
+                tal:content="string:
+          YUI().use('lp.registry.pillar', function(Y) {
+              Y.on('load',
+                  function(e) {
+                      Y.lp.registry.pillar.activate_collapsible_div();
+                  },
+                  window);
+          });
+        "/>
+  </tal:head-epilogue>
+</head>
+
   <body>
     <tal:main metal:fill-slot="main"
       define="overview_menu context/menu:overview">

=== added file 'lib/lp/registry/windmill/tests/test_product_configuration_hidden.py'
--- lib/lp/registry/windmill/tests/test_product_configuration_hidden.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/windmill/tests/test_product_configuration_hidden.py	2010-11-03 15:22:53 +0000
@@ -0,0 +1,90 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import unittest
+import transaction
+
+from canonical.launchpad.windmill.testing import (
+    constants,
+    lpuser,
+    )
+from lp.registry.windmill.testing import RegistryWindmillLayer
+from lp.testing import WindmillTestCase
+from lp.testing.service_usage_helpers import set_service_usage
+
+
+class TestProductHiddenConfiguration(WindmillTestCase):
+    """Test the Configuration links show/hide controls on products.
+
+    Controls only work with javascript enabled.
+    """
+
+    layer = RegistryWindmillLayer
+    suite_name = "Product configuration links hidden"
+
+    def setUp(self):
+        super(TestProductHiddenConfiguration, self).setUp()
+        self.product = self.factory.makeProduct(name='hidden-configs')
+        transaction.commit()
+
+    def test_not_fully_configured_starts_shown(self):
+        # A product that is not fully configured displays the links on
+        # page load, but they can be hidden.
+        client = self.client
+
+        client.open(url=u'http://launchpad.dev:8085/hidden-configs')
+        client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        lpuser.FOO_BAR.ensure_login(client)
+
+        # We can only safely use this class selector in this test b/c there's
+        # only one collapsible element on this page.
+        client.asserts.assertNotProperty(
+            classname='collapseWrapper',
+            validator='className|lazr-closed')
+
+        # When the "Configuration links" link is clicked and the actual links are
+        # shown, the collapsible wrapper collapses, hiding the links.
+        client.click(link=u"Configuration options")
+        client.waits.forElement(
+            classname="collapseWrapper lazr-closed",
+            timeout=constants.FOR_ELEMENT)
+        client.asserts.assertProperty(
+            classname='collapseWrapper',
+            validator='className|lazr-closed')
+
+    def test_configured_starts_collapsed(self):
+        # A product that is fully configured hides the links on page
+        # load, but they can be hidden.
+        set_service_usage(
+            self.product.name,
+            codehosting_usage="EXTERNAL",
+            bug_tracking_usage="LAUNCHPAD",
+            answers_usage="EXTERNAL",
+            translations_usage="NOT_APPLICABLE")
+        transaction.commit()
+
+        client = self.client
+
+        client.open(url=u'http://launchpad.dev:8085/hidden-configs')
+        client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        lpuser.FOO_BAR.ensure_login(client)
+        client.waits.forElement(
+            classname='collapseWrapper lazr-closed',
+            timeout=constants.FOR_ELEMENT)
+        client.asserts.assertProperty(
+            classname='collapseWrapper',
+            validator='className|lazr-closed')
+
+        # When the "Configuration links" link is clicked and the actual links are
+        # hidden, the collapsible wrapper opens, showing the links.
+        client.click(link=u"Configuration options")
+        client.waits.forElement(
+            classname="lazr-opened",
+            timeout=constants.FOR_ELEMENT)
+        client.asserts.assertProperty(
+            classname='collapseWrapper',
+            validator='className|lazr-open')
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/testing/service_usage_helpers.py'
--- lib/lp/testing/service_usage_helpers.py	2010-10-06 20:20:46 +0000
+++ lib/lp/testing/service_usage_helpers.py	2010-11-03 15:22:53 +0000
@@ -4,11 +4,12 @@
 """Helper functions dealing with IServiceUsage."""
 __metaclass__ = type
 
-import transaction
 from zope.component import getUtility
 
 from lp.app.enums import ServiceUsage
+from lp.code.enums import BranchType
 from lp.registry.interfaces.pillar import IPillarNameSet
+from lp.registry.interfaces.product import IProduct
 from lp.testing import (
     login,
     logout,
@@ -27,7 +28,28 @@
             pillar.official_malone = (service_usage == ServiceUsage.LAUNCHPAD)
             if service_usage == ServiceUsage.EXTERNAL:
                 pillar.bugtracker = factory.makeBugTracker()
+
+        # if we're setting codehosting on product things get trickier.
+        elif attr == 'codehosting_usage' and IProduct.providedBy(pillar):
+            if service_usage == ServiceUsage.LAUNCHPAD:
+                branch = factory.makeProductBranch(product=pillar)
+                product_series = factory.makeProductSeries(
+                    product=pillar,
+                    branch=branch)
+                pillar.development_focus = product_series
+            elif service_usage == ServiceUsage.EXTERNAL:
+                branch = factory.makeProductBranch(
+                    product=pillar,
+                    branch_type=BranchType.MIRRORED)
+                product_series = factory.makeProductSeries(
+                    product=pillar,
+                    branch=branch)
+                pillar.development_focus = product_series
+            elif service_usage == ServiceUsage.UNKNOWN:
+                branch = factory.makeProductBranch(product=pillar)
+                product_series = factory.makeProductSeries(
+                    product=pillar)
+                pillar.development_focus = product_series
         else:
             setattr(pillar, attr, service_usage)
-    #transaction.commit()
     logout()