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
1=== modified file 'lib/lp/app/javascript/lp.js'
2--- lib/lp/app/javascript/lp.js 2010-01-21 22:21:01 +0000
3+++ lib/lp/app/javascript/lp.js 2010-11-03 15:22:53 +0000
4@@ -104,7 +104,13 @@
5 // Grab the collapsibles.
6 Y.all('.collapsible').each(function(collapsible) {
7
8+ // Try to grab the legend in the usual way.
9 var legend = collapsible.one('legend');
10+ if (legend === null) {
11+ // If it's null, this might be a collapsible div, not fieldset,
12+ // so try to grap the div's "legend".
13+ legend = collapsible.one('.config-options');
14+ }
15 if (legend === null ||
16 legend.one('.collapseIcon') !== null) {
17 // If there's no legend there's not much we can do,
18
19=== modified file 'lib/lp/registry/browser/product.py'
20--- lib/lp/registry/browser/product.py 2010-11-03 00:40:35 +0000
21+++ lib/lp/registry/browser/product.py 2010-11-03 15:22:53 +0000
22@@ -473,6 +473,11 @@
23 undone = scale - done
24 return dict(done=done, undone=undone)
25
26+ @property
27+ def registration_done(self):
28+ """A boolean indicating that the services are fully configured."""
29+ return (self.registration_completeness['done'] == 100)
30+
31
32 class ProductNavigationMenu(NavigationMenu):
33
34
35=== modified file 'lib/lp/registry/browser/tests/test_product.py'
36--- lib/lp/registry/browser/tests/test_product.py 2010-09-27 18:18:11 +0000
37+++ lib/lp/registry/browser/tests/test_product.py 2010-11-03 15:22:53 +0000
38@@ -12,6 +12,7 @@
39 from zope.security.proxy import removeSecurityProxy
40
41 from canonical.testing.layers import DatabaseFunctionalLayer
42+from lp.app.enums import ServiceUsage
43 from lp.registry.browser.product import ProductLicenseMixin
44 from lp.registry.interfaces.product import License
45 from lp.testing import (
46@@ -19,6 +20,7 @@
47 TestCaseWithFactory,
48 )
49 from lp.testing.mail_helpers import pop_notifications
50+from lp.testing.service_usage_helpers import set_service_usage
51 from lp.testing.views import create_view
52
53
54@@ -102,5 +104,36 @@
55 self.assertEqual('2005-06-15', result)
56
57
58+class TestProductConfiguration(TestCaseWithFactory):
59+ """Tests the configuration links and helpers."""
60+
61+ layer = DatabaseFunctionalLayer
62+
63+ def setUp(self):
64+ super(TestProductConfiguration, self).setUp()
65+ self.product = self.factory.makeProduct()
66+
67+ def test_registration_not_done(self):
68+ # The registration done property on the product index view
69+ # tells you if all the configuration work is done, based on
70+ # usage enums.
71+
72+ # At least one usage enum is unknown, so registration done is false.
73+ self.assertEqual(
74+ self.product.codehosting_usage,
75+ ServiceUsage.UNKNOWN)
76+ view = create_view(self.product, '+get-involved')
77+ self.assertFalse(view.registration_done)
78+
79+ set_service_usage(
80+ self.product.name,
81+ codehosting_usage="EXTERNAL",
82+ bug_tracking_usage="LAUNCHPAD",
83+ answers_usage="EXTERNAL",
84+ translations_usage="NOT_APPLICABLE")
85+ view = create_view(self.product, '+get-involved')
86+ self.assertTrue(view.registration_done)
87+
88+
89 def test_suite():
90 return unittest.TestLoader().loadTestsFromName(__name__)
91
92=== modified file 'lib/lp/registry/javascript/team.js'
93--- lib/lp/registry/javascript/team.js 2010-07-15 10:59:22 +0000
94+++ lib/lp/registry/javascript/team.js 2010-11-03 15:22:53 +0000
95@@ -1,8 +1,8 @@
96-/** Copyright (c) 2009, Canonical Ltd. All rights reserved.
97- *
98- * Objects for subscription handling.
99- *
100- * @module lp.subscriber
101+/** Copyright (c) 2009-2010, Canonical Ltd. All rights reserved.
102+ *
103+ * Team add member animations and ui.
104+ *
105+ * @module lp.registry.team
106 */
107
108 YUI.add('lp.registry.team', function(Y) {
109
110=== modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
111--- lib/lp/registry/templates/pillar-involvement-portlet.pt 2010-09-14 21:45:15 +0000
112+++ lib/lp/registry/templates/pillar-involvement-portlet.pt 2010-11-03 15:22:53 +0000
113@@ -34,12 +34,12 @@
114 <tal:editor condition="context/required:launchpad.Edit"
115 define="registration view/registration_completeness">
116
117- <tal:show_configuration condition="registration">
118+ <tal:show-registration condition="registration">
119 <h2>Configuration Progress</h2>
120 <div class="centered" id="progressbar">
121- <table width="80%" border="1">
122- <tr>
123- <td>
124+ <table width="80%" border="1">
125+ <tr>
126+ <td>
127 <img
128 tal:attributes="alt string:${registration/done}% registration complete;
129 title string:${registration/done}% registration complete;
130@@ -47,28 +47,56 @@
131 style string:width: ${registration/done}%;"
132 src="/@@/green-bar"
133 height="10"/>
134+ </td>
135+ </tr>
136+ </table>
137+ </div>
138+
139+ <div id="show-hide-configs"
140+ tal:attributes="class
141+ python: 'collapsible collapsed'
142+ if view.registration_done
143+ else 'collapsible'">
144+ <div class="config-options"
145+ style="margin-bottom: 0.5em">Configuration options</div>
146+ <table
147+ tal:condition="view/configuration_links"
148+ style="width: 100%; padding-top: 1em"
149+ id="configuration_links">
150+ <tal:item repeat="link_status view/configuration_links">
151+ <tr>
152+ <td>
153+ <a tal:replace="structure link_status/link/fmt:link" />
154+ </td>
155+
156+ <td style="text-align: right;" tal:condition="registration">
157+ <tal:yes-no
158+ replace="structure link_status/configured/image:boolean"/>
159 </td>
160 </tr>
161- </table>
162+ </tal:item>
163+ </table>
164 </div>
165- </tal:show_configuration>
166-
167- <table style="width: 100%; padding-top: 1em"
168- tal:condition="view/configuration_links"
169- id="configuration_links">
170- <tal:item repeat="link_status view/configuration_links">
171- <tr>
172- <td>
173- <a tal:replace="structure link_status/link/fmt:link" />
174- </td>
175-
176- <td style="text-align: right;" tal:condition="registration">
177- <tal:yes-no replace="structure link_status/configured/image:boolean" />
178- </td>
179-
180- </tr>
181- </tal:item>
182- </table>
183-
184+ </tal:show-registration>
185+
186+ <tal:no-registration condition="not:registration">
187+ <table
188+ tal:condition="view/configuration_links"
189+ style="width: 100%; padding-top: 1em"
190+ id="configuration_links">
191+ <tal:item repeat="link_status view/configuration_links">
192+ <tr>
193+ <td>
194+ <a tal:replace="structure link_status/link/fmt:link" />
195+ </td>
196+
197+ <td style="text-align: right;" tal:condition="registration">
198+ <tal:yes-no
199+ replace="structure link_status/configured/image:boolean"/>
200+ </td>
201+ </tr>
202+ </tal:item>
203+ </table>
204+ </tal:no-registration>
205 </tal:editor>
206 </div>
207
208=== modified file 'lib/lp/registry/templates/product-index.pt'
209--- lib/lp/registry/templates/product-index.pt 2010-09-30 16:54:32 +0000
210+++ lib/lp/registry/templates/product-index.pt 2010-11-03 15:22:53 +0000
211@@ -12,6 +12,39 @@
212 <tal:registrant replace="structure context/registrant/fmt:link" />
213 </tal:registering>
214
215+<head>
216+ <tal:head-epilogue metal:fill-slot="head_epilogue">
217+ <style type="text/css">
218+ div.collapsible {
219+ border:medium none;
220+ margin:0;
221+ padding:16px 0 0;
222+ }
223+
224+ div.collapsed {
225+ display: None
226+ }
227+ </style>
228+
229+ <noscript>
230+ <style type="text/css">
231+ div.collapsible, div.collapsed {display: block;}
232+ </style>
233+ </noscript>
234+
235+ <script type="text/javascript"
236+ tal:content="string:
237+ YUI().use('lp.registry.pillar', function(Y) {
238+ Y.on('load',
239+ function(e) {
240+ Y.lp.registry.pillar.activate_collapsible_div();
241+ },
242+ window);
243+ });
244+ "/>
245+ </tal:head-epilogue>
246+</head>
247+
248 <body>
249 <tal:main metal:fill-slot="main"
250 define="overview_menu context/menu:overview">
251
252=== added file 'lib/lp/registry/windmill/tests/test_product_configuration_hidden.py'
253--- lib/lp/registry/windmill/tests/test_product_configuration_hidden.py 1970-01-01 00:00:00 +0000
254+++ lib/lp/registry/windmill/tests/test_product_configuration_hidden.py 2010-11-03 15:22:53 +0000
255@@ -0,0 +1,90 @@
256+# Copyright 2010 Canonical Ltd. This software is licensed under the
257+# GNU Affero General Public License version 3 (see the file LICENSE).
258+
259+import unittest
260+import transaction
261+
262+from canonical.launchpad.windmill.testing import (
263+ constants,
264+ lpuser,
265+ )
266+from lp.registry.windmill.testing import RegistryWindmillLayer
267+from lp.testing import WindmillTestCase
268+from lp.testing.service_usage_helpers import set_service_usage
269+
270+
271+class TestProductHiddenConfiguration(WindmillTestCase):
272+ """Test the Configuration links show/hide controls on products.
273+
274+ Controls only work with javascript enabled.
275+ """
276+
277+ layer = RegistryWindmillLayer
278+ suite_name = "Product configuration links hidden"
279+
280+ def setUp(self):
281+ super(TestProductHiddenConfiguration, self).setUp()
282+ self.product = self.factory.makeProduct(name='hidden-configs')
283+ transaction.commit()
284+
285+ def test_not_fully_configured_starts_shown(self):
286+ # A product that is not fully configured displays the links on
287+ # page load, but they can be hidden.
288+ client = self.client
289+
290+ client.open(url=u'http://launchpad.dev:8085/hidden-configs')
291+ client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
292+ lpuser.FOO_BAR.ensure_login(client)
293+
294+ # We can only safely use this class selector in this test b/c there's
295+ # only one collapsible element on this page.
296+ client.asserts.assertNotProperty(
297+ classname='collapseWrapper',
298+ validator='className|lazr-closed')
299+
300+ # When the "Configuration links" link is clicked and the actual links are
301+ # shown, the collapsible wrapper collapses, hiding the links.
302+ client.click(link=u"Configuration options")
303+ client.waits.forElement(
304+ classname="collapseWrapper lazr-closed",
305+ timeout=constants.FOR_ELEMENT)
306+ client.asserts.assertProperty(
307+ classname='collapseWrapper',
308+ validator='className|lazr-closed')
309+
310+ def test_configured_starts_collapsed(self):
311+ # A product that is fully configured hides the links on page
312+ # load, but they can be hidden.
313+ set_service_usage(
314+ self.product.name,
315+ codehosting_usage="EXTERNAL",
316+ bug_tracking_usage="LAUNCHPAD",
317+ answers_usage="EXTERNAL",
318+ translations_usage="NOT_APPLICABLE")
319+ transaction.commit()
320+
321+ client = self.client
322+
323+ client.open(url=u'http://launchpad.dev:8085/hidden-configs')
324+ client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
325+ lpuser.FOO_BAR.ensure_login(client)
326+ client.waits.forElement(
327+ classname='collapseWrapper lazr-closed',
328+ timeout=constants.FOR_ELEMENT)
329+ client.asserts.assertProperty(
330+ classname='collapseWrapper',
331+ validator='className|lazr-closed')
332+
333+ # When the "Configuration links" link is clicked and the actual links are
334+ # hidden, the collapsible wrapper opens, showing the links.
335+ client.click(link=u"Configuration options")
336+ client.waits.forElement(
337+ classname="lazr-opened",
338+ timeout=constants.FOR_ELEMENT)
339+ client.asserts.assertProperty(
340+ classname='collapseWrapper',
341+ validator='className|lazr-open')
342+
343+
344+def test_suite():
345+ return unittest.TestLoader().loadTestsFromName(__name__)
346
347=== modified file 'lib/lp/testing/service_usage_helpers.py'
348--- lib/lp/testing/service_usage_helpers.py 2010-10-06 20:20:46 +0000
349+++ lib/lp/testing/service_usage_helpers.py 2010-11-03 15:22:53 +0000
350@@ -4,11 +4,12 @@
351 """Helper functions dealing with IServiceUsage."""
352 __metaclass__ = type
353
354-import transaction
355 from zope.component import getUtility
356
357 from lp.app.enums import ServiceUsage
358+from lp.code.enums import BranchType
359 from lp.registry.interfaces.pillar import IPillarNameSet
360+from lp.registry.interfaces.product import IProduct
361 from lp.testing import (
362 login,
363 logout,
364@@ -27,7 +28,28 @@
365 pillar.official_malone = (service_usage == ServiceUsage.LAUNCHPAD)
366 if service_usage == ServiceUsage.EXTERNAL:
367 pillar.bugtracker = factory.makeBugTracker()
368+
369+ # if we're setting codehosting on product things get trickier.
370+ elif attr == 'codehosting_usage' and IProduct.providedBy(pillar):
371+ if service_usage == ServiceUsage.LAUNCHPAD:
372+ branch = factory.makeProductBranch(product=pillar)
373+ product_series = factory.makeProductSeries(
374+ product=pillar,
375+ branch=branch)
376+ pillar.development_focus = product_series
377+ elif service_usage == ServiceUsage.EXTERNAL:
378+ branch = factory.makeProductBranch(
379+ product=pillar,
380+ branch_type=BranchType.MIRRORED)
381+ product_series = factory.makeProductSeries(
382+ product=pillar,
383+ branch=branch)
384+ pillar.development_focus = product_series
385+ elif service_usage == ServiceUsage.UNKNOWN:
386+ branch = factory.makeProductBranch(product=pillar)
387+ product_series = factory.makeProductSeries(
388+ product=pillar)
389+ pillar.development_focus = product_series
390 else:
391 setattr(pillar, attr, service_usage)
392- #transaction.commit()
393 logout()