Launchpad itself

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

  1. hidden-project-configuration-636000
  2. Merge into devel
Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 11854
Proposed branch: lp:~jcsackett/launchpad/hidden-project-configuration-636000
Merge into: lp:launchpad
Diff against target: 393 lines (+248/-31)
8 files modified
lib/lp/app/javascript/lp.js (+6/-0)
lib/lp/registry/browser/product.py (+5/-0)
lib/lp/registry/browser/tests/test_product.py (+33/-0)
lib/lp/registry/javascript/team.js (+5/-5)
lib/lp/registry/templates/pillar-involvement-portlet.pt (+52/-24)
lib/lp/registry/templates/product-index.pt (+33/-0)
lib/lp/registry/windmill/tests/test_product_configuration_hidden.py (+90/-0)
lib/lp/testing/service_usage_helpers.py (+24/-2)
To merge this branch: bzr merge lp:~jcsackett/launchpad/hidden-project-configuration-636000
Reviewer Review Type Date Requested Status
Henning Eggers (community) ui Approve
Leonard Richardson (community) code Approve
Review via email: mp+39306@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

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)
Revision history for this message
Leonard Richardson (leonardr) wrote :

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

Revision history for this message
j.c.sackett (jcsackett) wrote :

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.

Revision history for this message
j.c.sackett (jcsackett) wrote :

> 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.

Revision history for this message
Henning Eggers (henninge) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
104 // Grab the collapsibles.104 // Grab the collapsibles.
105 Y.all('.collapsible').each(function(collapsible) {105 Y.all('.collapsible').each(function(collapsible) {
106106
107 // Try to grab the legend in the usual way.
107 var legend = collapsible.one('legend');108 var legend = collapsible.one('legend');
109 if (legend === null) {
110 // If it's null, this might be a collapsible div, not fieldset,
111 // so try to grap the div's "legend".
112 legend = collapsible.one('.config-options');
113 }
108 if (legend === null ||114 if (legend === null ||
109 legend.one('.collapseIcon') !== null) {115 legend.one('.collapseIcon') !== null) {
110 // If there's no legend there's not much we can do,116 // If there's no legend there's not much we can do,
111117
=== 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 @@
473 undone = scale - done473 undone = scale - done
474 return dict(done=done, undone=undone)474 return dict(done=done, undone=undone)
475475
476 @property
477 def registration_done(self):
478 """A boolean indicating that the services are fully configured."""
479 return (self.registration_completeness['done'] == 100)
480
476481
477class ProductNavigationMenu(NavigationMenu):482class ProductNavigationMenu(NavigationMenu):
478483
479484
=== 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 @@
12from zope.security.proxy import removeSecurityProxy12from zope.security.proxy import removeSecurityProxy
1313
14from canonical.testing.layers import DatabaseFunctionalLayer14from canonical.testing.layers import DatabaseFunctionalLayer
15from lp.app.enums import ServiceUsage
15from lp.registry.browser.product import ProductLicenseMixin16from lp.registry.browser.product import ProductLicenseMixin
16from lp.registry.interfaces.product import License17from lp.registry.interfaces.product import License
17from lp.testing import (18from lp.testing import (
@@ -19,6 +20,7 @@
19 TestCaseWithFactory,20 TestCaseWithFactory,
20 )21 )
21from lp.testing.mail_helpers import pop_notifications22from lp.testing.mail_helpers import pop_notifications
23from lp.testing.service_usage_helpers import set_service_usage
22from lp.testing.views import create_view24from lp.testing.views import create_view
2325
2426
@@ -102,5 +104,36 @@
102 self.assertEqual('2005-06-15', result)104 self.assertEqual('2005-06-15', result)
103105
104106
107class TestProductConfiguration(TestCaseWithFactory):
108 """Tests the configuration links and helpers."""
109
110 layer = DatabaseFunctionalLayer
111
112 def setUp(self):
113 super(TestProductConfiguration, self).setUp()
114 self.product = self.factory.makeProduct()
115
116 def test_registration_not_done(self):
117 # The registration done property on the product index view
118 # tells you if all the configuration work is done, based on
119 # usage enums.
120
121 # At least one usage enum is unknown, so registration done is false.
122 self.assertEqual(
123 self.product.codehosting_usage,
124 ServiceUsage.UNKNOWN)
125 view = create_view(self.product, '+get-involved')
126 self.assertFalse(view.registration_done)
127
128 set_service_usage(
129 self.product.name,
130 codehosting_usage="EXTERNAL",
131 bug_tracking_usage="LAUNCHPAD",
132 answers_usage="EXTERNAL",
133 translations_usage="NOT_APPLICABLE")
134 view = create_view(self.product, '+get-involved')
135 self.assertTrue(view.registration_done)
136
137
105def test_suite():138def test_suite():
106 return unittest.TestLoader().loadTestsFromName(__name__)139 return unittest.TestLoader().loadTestsFromName(__name__)
107140
=== 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 @@
1/** Copyright (c) 2009, Canonical Ltd. All rights reserved.1/** Copyright (c) 2009-2010, Canonical Ltd. All rights reserved.
2 *2 *
3 * Objects for subscription handling.3 * Team add member animations and ui.
4 *4 *
5 * @module lp.subscriber5 * @module lp.registry.team
6 */6 */
77
8YUI.add('lp.registry.team', function(Y) {8YUI.add('lp.registry.team', function(Y) {
99
=== 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 @@
34 <tal:editor condition="context/required:launchpad.Edit"34 <tal:editor condition="context/required:launchpad.Edit"
35 define="registration view/registration_completeness">35 define="registration view/registration_completeness">
3636
37 <tal:show_configuration condition="registration">37 <tal:show-registration condition="registration">
38 <h2>Configuration Progress</h2>38 <h2>Configuration Progress</h2>
39 <div class="centered" id="progressbar">39 <div class="centered" id="progressbar">
40 <table width="80%" border="1">40 <table width="80%" border="1">
41 <tr>41 <tr>
42 <td>42 <td>
43 <img43 <img
44 tal:attributes="alt string:${registration/done}% registration complete;44 tal:attributes="alt string:${registration/done}% registration complete;
45 title string:${registration/done}% registration complete;45 title string:${registration/done}% registration complete;
@@ -47,28 +47,56 @@
47 style string:width: ${registration/done}%;"47 style string:width: ${registration/done}%;"
48 src="/@@/green-bar"48 src="/@@/green-bar"
49 height="10"/>49 height="10"/>
50 </td>
51 </tr>
52 </table>
53 </div>
54
55 <div id="show-hide-configs"
56 tal:attributes="class
57 python: 'collapsible collapsed'
58 if view.registration_done
59 else 'collapsible'">
60 <div class="config-options"
61 style="margin-bottom: 0.5em">Configuration options</div>
62 <table
63 tal:condition="view/configuration_links"
64 style="width: 100%; padding-top: 1em"
65 id="configuration_links">
66 <tal:item repeat="link_status view/configuration_links">
67 <tr>
68 <td>
69 <a tal:replace="structure link_status/link/fmt:link" />
70 </td>
71
72 <td style="text-align: right;" tal:condition="registration">
73 <tal:yes-no
74 replace="structure link_status/configured/image:boolean"/>
50 </td>75 </td>
51 </tr>76 </tr>
52 </table>77 </tal:item>
78 </table>
53 </div>79 </div>
54 </tal:show_configuration>80 </tal:show-registration>
5581
56 <table style="width: 100%; padding-top: 1em"82 <tal:no-registration condition="not:registration">
57 tal:condition="view/configuration_links"83 <table
58 id="configuration_links">84 tal:condition="view/configuration_links"
59 <tal:item repeat="link_status view/configuration_links">85 style="width: 100%; padding-top: 1em"
60 <tr>86 id="configuration_links">
61 <td>87 <tal:item repeat="link_status view/configuration_links">
62 <a tal:replace="structure link_status/link/fmt:link" />88 <tr>
63 </td>89 <td>
6490 <a tal:replace="structure link_status/link/fmt:link" />
65 <td style="text-align: right;" tal:condition="registration">91 </td>
66 <tal:yes-no replace="structure link_status/configured/image:boolean" />92
67 </td>93 <td style="text-align: right;" tal:condition="registration">
6894 <tal:yes-no
69 </tr>95 replace="structure link_status/configured/image:boolean"/>
70 </tal:item>96 </td>
71 </table>97 </tr>
7298 </tal:item>
99 </table>
100 </tal:no-registration>
73 </tal:editor>101 </tal:editor>
74</div>102</div>
75103
=== 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 @@
12 <tal:registrant replace="structure context/registrant/fmt:link" />12 <tal:registrant replace="structure context/registrant/fmt:link" />
13 </tal:registering>13 </tal:registering>
1414
15<head>
16 <tal:head-epilogue metal:fill-slot="head_epilogue">
17 <style type="text/css">
18 div.collapsible {
19 border:medium none;
20 margin:0;
21 padding:16px 0 0;
22 }
23
24 div.collapsed {
25 display: None
26 }
27 </style>
28
29 <noscript>
30 <style type="text/css">
31 div.collapsible, div.collapsed {display: block;}
32 </style>
33 </noscript>
34
35 <script type="text/javascript"
36 tal:content="string:
37 YUI().use('lp.registry.pillar', function(Y) {
38 Y.on('load',
39 function(e) {
40 Y.lp.registry.pillar.activate_collapsible_div();
41 },
42 window);
43 });
44 "/>
45 </tal:head-epilogue>
46</head>
47
15 <body>48 <body>
16 <tal:main metal:fill-slot="main"49 <tal:main metal:fill-slot="main"
17 define="overview_menu context/menu:overview">50 define="overview_menu context/menu:overview">
1851
=== 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 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4import unittest
5import transaction
6
7from canonical.launchpad.windmill.testing import (
8 constants,
9 lpuser,
10 )
11from lp.registry.windmill.testing import RegistryWindmillLayer
12from lp.testing import WindmillTestCase
13from lp.testing.service_usage_helpers import set_service_usage
14
15
16class TestProductHiddenConfiguration(WindmillTestCase):
17 """Test the Configuration links show/hide controls on products.
18
19 Controls only work with javascript enabled.
20 """
21
22 layer = RegistryWindmillLayer
23 suite_name = "Product configuration links hidden"
24
25 def setUp(self):
26 super(TestProductHiddenConfiguration, self).setUp()
27 self.product = self.factory.makeProduct(name='hidden-configs')
28 transaction.commit()
29
30 def test_not_fully_configured_starts_shown(self):
31 # A product that is not fully configured displays the links on
32 # page load, but they can be hidden.
33 client = self.client
34
35 client.open(url=u'http://launchpad.dev:8085/hidden-configs')
36 client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
37 lpuser.FOO_BAR.ensure_login(client)
38
39 # We can only safely use this class selector in this test b/c there's
40 # only one collapsible element on this page.
41 client.asserts.assertNotProperty(
42 classname='collapseWrapper',
43 validator='className|lazr-closed')
44
45 # When the "Configuration links" link is clicked and the actual links are
46 # shown, the collapsible wrapper collapses, hiding the links.
47 client.click(link=u"Configuration options")
48 client.waits.forElement(
49 classname="collapseWrapper lazr-closed",
50 timeout=constants.FOR_ELEMENT)
51 client.asserts.assertProperty(
52 classname='collapseWrapper',
53 validator='className|lazr-closed')
54
55 def test_configured_starts_collapsed(self):
56 # A product that is fully configured hides the links on page
57 # load, but they can be hidden.
58 set_service_usage(
59 self.product.name,
60 codehosting_usage="EXTERNAL",
61 bug_tracking_usage="LAUNCHPAD",
62 answers_usage="EXTERNAL",
63 translations_usage="NOT_APPLICABLE")
64 transaction.commit()
65
66 client = self.client
67
68 client.open(url=u'http://launchpad.dev:8085/hidden-configs')
69 client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
70 lpuser.FOO_BAR.ensure_login(client)
71 client.waits.forElement(
72 classname='collapseWrapper lazr-closed',
73 timeout=constants.FOR_ELEMENT)
74 client.asserts.assertProperty(
75 classname='collapseWrapper',
76 validator='className|lazr-closed')
77
78 # When the "Configuration links" link is clicked and the actual links are
79 # hidden, the collapsible wrapper opens, showing the links.
80 client.click(link=u"Configuration options")
81 client.waits.forElement(
82 classname="lazr-opened",
83 timeout=constants.FOR_ELEMENT)
84 client.asserts.assertProperty(
85 classname='collapseWrapper',
86 validator='className|lazr-open')
87
88
89def test_suite():
90 return unittest.TestLoader().loadTestsFromName(__name__)
091
=== 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 @@
4"""Helper functions dealing with IServiceUsage."""4"""Helper functions dealing with IServiceUsage."""
5__metaclass__ = type5__metaclass__ = type
66
7import transaction
8from zope.component import getUtility7from zope.component import getUtility
98
10from lp.app.enums import ServiceUsage9from lp.app.enums import ServiceUsage
10from lp.code.enums import BranchType
11from lp.registry.interfaces.pillar import IPillarNameSet11from lp.registry.interfaces.pillar import IPillarNameSet
12from lp.registry.interfaces.product import IProduct
12from lp.testing import (13from lp.testing import (
13 login,14 login,
14 logout,15 logout,
@@ -27,7 +28,28 @@
27 pillar.official_malone = (service_usage == ServiceUsage.LAUNCHPAD)28 pillar.official_malone = (service_usage == ServiceUsage.LAUNCHPAD)
28 if service_usage == ServiceUsage.EXTERNAL:29 if service_usage == ServiceUsage.EXTERNAL:
29 pillar.bugtracker = factory.makeBugTracker()30 pillar.bugtracker = factory.makeBugTracker()
31
32 # if we're setting codehosting on product things get trickier.
33 elif attr == 'codehosting_usage' and IProduct.providedBy(pillar):
34 if service_usage == ServiceUsage.LAUNCHPAD:
35 branch = factory.makeProductBranch(product=pillar)
36 product_series = factory.makeProductSeries(
37 product=pillar,
38 branch=branch)
39 pillar.development_focus = product_series
40 elif service_usage == ServiceUsage.EXTERNAL:
41 branch = factory.makeProductBranch(
42 product=pillar,
43 branch_type=BranchType.MIRRORED)
44 product_series = factory.makeProductSeries(
45 product=pillar,
46 branch=branch)
47 pillar.development_focus = product_series
48 elif service_usage == ServiceUsage.UNKNOWN:
49 branch = factory.makeProductBranch(product=pillar)
50 product_series = factory.makeProductSeries(
51 product=pillar)
52 pillar.development_focus = product_series
30 else:53 else:
31 setattr(pillar, attr, service_usage)54 setattr(pillar, attr, service_usage)
32 #transaction.commit()
33 logout()55 logout()