Merge lp:~barry/launchpad/445713-menus into lp:launchpad

[Barry Warsaw]

[Barry Warsaw (barry)]

 reviewer rockstar code ui
 reviewer bac ui

= Summary =

Bug 445713 describes an ugliness that slipped through the cracks during the UI
3.0 conversion. A project (nee product) has a +edit page that allows you to
change the details of the project. At the bottom of this page are 4 or 5
links that allow you to

 * Change details
 * Change branding
 * Change people
 * Review project
 * Administer

The problem with these is that this menu is hiding out below the "Change or
Cancel" controls. This menu should be removed and any actions which are not
already exposed, should be added to the project's +index page.

== Proposed fix ==

* Add the "Change branding" link inlined on project/+edit
* Add linked icons to the +edit-people page next to the maintainer and driver
  sections
* Replace product-edit-people.pt, product-edit.pt, and product-admin.pt with
  generic-edit.pt
* Remove all traces of the ProductEditNavigationMenu

== Pre-implementation notes ==

I briefly talked with Curtis about the page redesign.

== Implementation details ==

Along the way, I re-organized the project/+review-license page so that it was
more readable, and more accurately reflected reality. I will request that bac
review the new text.

This was my first experiment with bzr-pipelines. I ran into a few snags but I
think it generally worked well. I tried to segregate the drive-by cleanups
into a separate branch, and it would be great if you could review that too as
a follow up:

    lp:~barry/launchpad/445713-cleanups

== Tests ==

% bin/test -vv -t product-menus -t xx-product-driver -t xx-product-edt

or for a more thorough regression test:

% bin/test -vv -t product

== Demo and Q/A ==

* Visit http://launchpad.dev/firefox
* Observe that the maintainer and driver has an edit icon
* Observe that under the project description, there is a "Change branding" link
* Click on "Change branding"; this takes you to the +branding page
* Observe that there is no navigation menu below the "Change Branding" button
* Go back to http://launchpad.dev/firefox
* Click on the edit icon next to Sample Person (Maintainer)
* Observer that this takes you to the +edit-people page
* Observe that there is no navigation menu
* Enter 'salgado' in the Driver field and click "Save changes"
* Observe that Salgado is now the Driver
* Observer that the icon next to Maintainer Sample Person's name takes you to
  the +edit-people page too.

* Observe that the Change details link is already on the +index page
* Observe that the Administer link is already on the +index page
* Observe that the Review project link is already on the +index page

* Observe that http://launchpad.dev/firefox/+edit page has no navigation menu
* Observe that http://launchpad.dev/firefox/+admin page has no navigation menu
* Observe that http://launchpad.dev/firefox/+review-license page has no
  navigation menu

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/templates/product-index.pt
  lib/lp/registry/stories/product/xx-product-driver.txt
  li...

[Brad Crittenden (bac)]

Hi Barry,

You've asked me to do a UI review but I should confess I'm not even a UI reviewer mentat. So this review isn't really worth the paper it's written on.

Despite the shady nature of my credentials I think the changes you've made are great. (Your screenshot was awful, though, as it didn't show the good bits.)

So you get an enthusiastic +0 from me. Don't spend it all in one place.

review: Approve (ui*)

[Paul Hummer (rockstar)]

Looks good. One thing I'd suggest for next time with a UI review is screenshots. That way, I don't actually have to set the branch up to run and such.

review: Approve (code ui)

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-10-05 22:14:18 +0000
+++ lib/lp/registry/browser/configure.zcml	2009-10-08 21:10:25 +0000
@@ -1477,7 +1477,7 @@
         class="lp.registry.browser.product.ProductEditPeopleView">
         <browser:page
             name="+edit-people"
-            template="../templates/product-edit-people.pt"/>
+            template="../../app/templates/generic-edit.pt"/>
     </browser:pages>
     <browser:page
         name="+edit"
@@ -1485,7 +1485,7 @@
         facet="overview"
         class="lp.registry.browser.product.ProductEditView"
         permission="launchpad.Edit"
-        template="../templates/product-edit.pt"/>
+        template="../../app/templates/generic-edit.pt"/>
     <browser:page
         name="+branding"
         for="lp.registry.interfaces.product.IProduct"
@@ -1499,7 +1499,7 @@
         facet="overview"
         class="lp.registry.browser.product.ProductAdminView"
         permission="launchpad.Admin"
-        template="../templates/product-admin.pt"/>
+        template="../../app/templates/generic-edit.pt"/>
     <browser:page
         name="+review-license"
         for="lp.registry.interfaces.product.IProduct"
@@ -1567,7 +1567,6 @@
         classes="
             ProductActionNavigationMenu
             ProductBugsMenu
-            ProductEditNavigationMenu
             ProductFacets
             ProductNavigationMenu
             ProductOverviewMenu

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2009-10-01 16:10:42 +0000
+++ lib/lp/registry/browser/product.py	2009-10-08 21:10:25 +0000
@@ -15,7 +15,6 @@
     'ProductDistributionsView',
     'ProductDownloadFileMixin',
     'ProductDownloadFilesView',
-    'ProductEditNavigationMenu',
     'ProductEditPeopleView',
     'ProductEditView',
     'ProductFacets',
@@ -349,15 +348,6 @@
     """A marker interface for the 'Change details' navigation menu."""
 
 
-class ProductEditNavigationMenu(NavigationMenu, ProductEditLinksMixin):
-    """A sub-menu for different aspects of editing a Product's details."""
-
-    usedfor = IProductEditMenu
-    facet = 'overview'
-    title = 'Change project'
-    links = ('edit', 'branding', 'reassign', 'review_license', 'administer')
-
-
 class IProductActionMenu(Interface):
     """A marker interface for the global action navigation menu."""
 
@@ -390,6 +380,7 @@
         'review_license',
         'branchvisibility',
         'rdf',
+        'branding',
         ]
 
     def top_contributors(self):

=== modified file 'lib/lp/registry/browser/tests/product-menus.txt'
--- lib/lp/registry/browser/tests/product-menus.txt	2009-08-12 04:27:34 +0000
+++ lib/lp/registry/browser/tests/product-menus.txt	2009-10-08 21:10:25 +0000
@@ -1,4 +1,6 @@
-= The product menu classes =
+========================
+The product menu classes
+========================
 
 Each link specified by menu classes for products should be accessible.
 In most cases, the context for links will be the product itself, which
@@ -7,11 +9,7 @@
 The check_menu_links() function is defined here to make it simple to
 test all the product menu classes below.
 
-    >>> import traceback
-    >>> import sys
-    >>> import os
     >>> from zope.component import getMultiAdapter
-    >>> from canonical.lazr.utils import safe_hasattr
     >>> from canonical.lazr.testing.menus import make_fake_request
     >>> from canonical.launchpad.webapp.publisher import canonical_url
     >>> from lp.registry.interfaces.product import IProductSet
@@ -34,8 +32,7 @@
     ...     print is_sane_menu
 
     >>> from lp.registry.browser.product import (
-    ...     ProductBugsMenu,
-    ...     ProductEditNavigationMenu, ProductNavigationMenu,
+    ...     ProductBugsMenu, ProductNavigationMenu,
     ...     ProductOverviewMenu, ProductSpecificationsMenu)
     >>> product = getUtility(IProductSet)['firefox']
 
@@ -51,11 +48,9 @@
     >>> check_menu_links(ProductNavigationMenu(product))
     True
 
-    >>> check_menu_links(ProductEditNavigationMenu(product))
-    True
-
-
-== ProductBranchesMenu and ProductTranslationsMenu ==
+
+ProductBranchesMenu and ProductTranslationsMenu
+===============================================
 
 We are not testing ProductBranchesMenu.code_import because it
 points to a view (/+code-imports/+new), which is not easily tested.

=== modified file 'lib/lp/registry/stories/product/xx-product-driver.txt'
--- lib/lp/registry/stories/product/xx-product-driver.txt	2009-09-18 17:38:50 +0000
+++ lib/lp/registry/stories/product/xx-product-driver.txt	2009-10-08 21:10:25 +0000
@@ -1,3 +1,4 @@
+===========================
 Appointing a Product Driver
 ===========================
 
@@ -5,13 +6,9 @@
 
     >>> browser = setupBrowser(auth='Basic [e-mail]:test')
     >>> browser.open('http://launchpad.dev/firefox')
-    >>> browser.getLink('Change details').click()
-    >>> browser.url
-    'http://launchpad.dev/firefox/+edit'
-
-    >>> browser.getLink('Change people').click()
-    >>> browser.url
-    'http://launchpad.dev/firefox/+edit-people'
+    >>> browser.getLink(url='+edit-people').click()
+    >>> print browser.url
+    http://launchpad.dev/firefox/+edit-people
 
     >>> print browser.title
     Change the roles of Mozilla Firefox's people...
@@ -22,15 +19,16 @@
 After changing the driver, the user is redirected to the overview page
 where a message inform him of the driver change.
 
-    >>> browser.url
-    'http://launchpad.dev/firefox'
+    >>> print browser.url
+    http://launchpad.dev/firefox
 
     >>> for tag in find_tags_by_class(browser.contents, 'informational'):
-    ...     tag.renderContents()
-    'Successfully changed the driver to Sample Person'
+    ...     print tag.renderContents()
+    Successfully changed the driver to Sample Person
 
 Sample Person is listed as the driver of the product.
 
     >>> main = find_main_content(browser.contents)
     >>> print extract_text(main.find(id='driver'))
     Driver: Sample Person
+    ...

=== modified file 'lib/lp/registry/stories/product/xx-product-edit.txt'
--- lib/lp/registry/stories/product/xx-product-edit.txt	2009-09-23 10:17:34 +0000
+++ lib/lp/registry/stories/product/xx-product-edit.txt	2009-10-08 21:10:25 +0000
@@ -1,3 +1,4 @@
+===========================
 Product Edit and Administer
 ===========================
 
@@ -39,7 +40,7 @@
 
 
 Setting Development Focus
--------------------------
+=========================
 
 The Firefox product's trunk series is set as the development focus:
 
@@ -72,7 +73,7 @@
 
 
 Administering Products
-----------------------
+======================
 
 Regular users can't access the 'Administer' link.
 
@@ -114,7 +115,7 @@
 
 
 Renaming Products
------------------
+=================
 
 First a user adds a product named newproductname.
 
@@ -175,7 +176,7 @@
 
 
 Changing Maintainer and Registrant
-----------------------------------
+==================================
 
 Administrators can change the owner of a project.
 
@@ -183,6 +184,7 @@
     ...     'http://launchpad.dev/newproductname3')
     >>> print extract_text(find_tag_by_id(admin_browser.contents, 'owner'))
     Maintainer: No Privileges Person
+    ...
 
     >>> admin_browser.open(
     ...     'http://launchpad.dev/newproductname3/+admin')
@@ -190,6 +192,7 @@
     >>> admin_browser.getControl(name='field.actions.change').click()
     >>> print extract_text(find_tag_by_id(admin_browser.contents, 'owner'))
     Maintainer: Celso Providelo
+    ...
 
 And the registrant can also be changed, even though this should rarely
 happen. The registrant is a read-only field that is set when the product
@@ -222,7 +225,7 @@
 
 
 Registry Experts
-----------------
+================
 
 Regular Joes can't edit any old project:
 
@@ -254,7 +257,7 @@
 
 
 Display error when trying to remove all licenses
-------------------------------------------------
+================================================
 
 After a product has one or more licenses selected, it can never again
 have zero licenses.
@@ -272,7 +275,7 @@
 
 
 Launchpad developers can use the reviewer whiteboard
-----------------------------------------------------
+====================================================
 
 A member of the Registry Experts team can use the reviewer whiteboard on
 the +review-license page for storing random notes about a project. At
@@ -325,7 +328,7 @@
 
 
 Deactivate a product
---------------------
+====================
 
 The Admins and Registry Experts can deactivate a project.
 
@@ -379,7 +382,7 @@
 
 
 Setting the remote project
---------------------------
+==========================
 
 The remote_product field - which stores a Product's ID on the remote bug
 tracker - can be set from the +edit page, too.
@@ -391,4 +394,3 @@
     >>> user_browser.open('http://launchpad.dev/thunderbird/+edit')
     >>> print user_browser.getControl('Remote project').value
     Thunderbird
-

=== removed file 'lib/lp/registry/templates/product-admin.pt'
--- lib/lp/registry/templates/product-admin.pt	2009-08-03 14:38:55 +0000
+++ lib/lp/registry/templates/product-admin.pt	1970-01-01 00:00:00 +0000
@@ -1,17 +0,0 @@
-<html
-  xmlns="http://www.w3.org/1999/xhtml"
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  metal:use-macro="view/macro:page/main_only"
-  i18n:domain="launchpad">
-  <body>
-    <div metal:fill-slot="main">
-      <div class="top-portlet">
-        <div metal:use-macro="context/@@launchpad_form/form" />
-      </div>
-
-      <tal:menu replace="structure view/@@+related-pages" />
-    </div>
-  </body>
-</html>

=== removed file 'lib/lp/registry/templates/product-edit-people.pt'
--- lib/lp/registry/templates/product-edit-people.pt	2009-08-03 14:38:55 +0000
+++ lib/lp/registry/templates/product-edit-people.pt	1970-01-01 00:00:00 +0000
@@ -1,18 +0,0 @@
-<html
-  xmlns="http://www.w3.org/1999/xhtml"
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  metal:use-macro="view/macro:page/main_only"
-  i18n:domain="launchpad">
-<body>
-  <div metal:fill-slot="main">
-    <div class="top-portlet">
-      <div metal:use-macro="context/@@launchpad_form/form" />
-    </div>
-
-    <tal:menu replace="structure view/@@+related-pages" />
-  </div>
-</body>
-</html>
-

=== removed file 'lib/lp/registry/templates/product-edit.pt'
--- lib/lp/registry/templates/product-edit.pt	2009-08-03 14:38:55 +0000
+++ lib/lp/registry/templates/product-edit.pt	1970-01-01 00:00:00 +0000
@@ -1,23 +0,0 @@
-<html
-  xmlns="http://www.w3.org/1999/xhtml"
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  metal:use-macro="view/macro:page/main_only"
-  i18n:domain="launchpad">
-    <body>
-      <div metal:fill-slot="main">
-        <div class="top-portlet">
-          <div metal:use-macro="context/@@launchpad_form/form">
-            <p metal:fill-slot="extra_info">
-              Your changes to the <span tal:replace="context/displayname">
-              project</span> details below will be visible to other users
-              after you submit them.
-            </p>
-          </div>
-        </div>
-
-        <tal:menu replace="structure view/@@+related-pages" />
-      </div>
-    </body>
-</html>

=== modified file 'lib/lp/registry/templates/product-index.pt'
--- lib/lp/registry/templates/product-index.pt	2009-09-22 17:39:22 +0000
+++ lib/lp/registry/templates/product-index.pt	2009-10-08 21:10:25 +0000
@@ -36,6 +36,7 @@
           detailed information about the open source project in question.
         </div>
 
+        <div tal:replace="structure overview_menu/branding/fmt:link" />
         <ul id="external-links" class="horizontal"
           tal:condition="view/has_external_links">
           <li tal:condition="view/should_display_homepage">
@@ -62,7 +63,10 @@
 
               <dl id="owner" tal:condition="context/owner">
                 <dt>Maintainer:</dt>
-                <dd><a tal:replace="structure context/owner/fmt:link" /></dd>
+                <dd>
+                  <a tal:replace="structure context/owner/fmt:link" />
+                  <a tal:replace="structure overview_menu/reassign/fmt:icon" />
+                </dd>
               </dl>
 
               <dl id="aliases" tal:condition="context/aliases">
@@ -76,9 +80,13 @@
               <dl id="driver">
                 <dt>Driver:</dt>
                 <dd tal:condition="view/effective_driver">
-                  <a tal:replace="structure view/effective_driver/fmt:link" /></dd>
+                  <a tal:replace="structure view/effective_driver/fmt:link" />
+                  <a tal:replace="structure overview_menu/reassign/fmt:icon" />
+                </dd>
                 <dd tal:condition="not: view/effective_driver">
-                  Not yet appointed.</dd>
+                  Not yet appointed.
+                  <a tal:replace="structure overview_menu/reassign/fmt:icon" />
+                </dd>
               </dl>
 
               <dl id="uses" tal:define="uses_launchpad view/uses_launchpad_for">
@@ -122,7 +130,7 @@
             </div>
 
             <p>
-              <a tal:replace="structure  overview_menu/rdf/fmt:link" />
+              <a tal:replace="structure overview_menu/rdf/fmt:link" />
             </p>
 
             <div id="project-admin-whiteboard" class="whiteboard"

=== modified file 'lib/lp/registry/templates/product-review-license.pt'
--- lib/lp/registry/templates/product-review-license.pt	2009-08-01 02:08:09 +0000
+++ lib/lp/registry/templates/product-review-license.pt	2009-10-08 21:10:25 +0000
@@ -12,49 +12,51 @@
 <div metal:fill-slot="main">
   <div class="top-portlet">
     <div metal:use-macro="context/@@launchpad_form/form">
-      <div metal:fill-slot="extra_info">
-        <p>
-          Examine the project's overview tab first, then examine if the
-          project is using code, bugs, etc.... Mark the project reviewed now.
-        </p>
-
-        <p>
-          Does the project appear to be legitimate? Are its licenses simple
-          and clearly open and free? Mark the project approved now.
-        </p>
-
-        <p>
-          If the project appears legitimate, yet incomplete, that is fine.
-          Approve the project, consider sending a request to ask the owner to
-          update his project with more details because it will help him
-          get contributors.
-        </p>
-
-        <p>
-          There are a few reasons you may want to withhold approval, and you
-          should contact the project owner if you do.
-        </p>
-
-        <ul class="bulleted">
-          <li>
-            The project has no license.
-          </li>
-          <li>
-            The project has only Other/Open source selected as a license and
-            the license info field does not contain a URL to a license, or
-            contains the whole license
-          </li>
-          <li>
-            The Other/Open source license appears to discriminate against who
-            or how the project's work is used.
-          </li>
-          <li>
-            The project is too vague to know if it is legitimate.
-          </li>
-        </ul>
-
-        <p>
-          If the project looks like a test, a prank, or spam, deactivate it.
+      <div metal:fill-slot="extra_info" class="application-summary" >
+        <h2>Granting approval</h2>
+
+        <ul class="bulleted">
+          <li>Does the project use any Launchpad services, such as code or
+              bugs?  Mark the <strong>project approved</strong>
+              and <strong>reviewed</strong> now.
+          </li>
+          <li>Are the project's licenses simple and obviously open source
+              and/or free software?  Does the project smell legitimate?  Mark
+              the <strong>project approved</strong>
+              and <strong>reviewed</strong> now.
+          </li>
+          <li>Does the project appear legitimate but incomplete?  That's fine,
+              mark the <strong>project approved</strong>
+              and <strong>reviewed</strong> now.  Consider also sending a
+              request to the
+              owner asking them to update the project with more details, so as
+              to help recruit contributors.
+          </li>
+        </ul>
+
+        <h2>Withholding approval</h2>
+        <p>
+          There are a few reasons you may want to withhold approval.  You
+          should always contact the project owner for clarification if you do
+          not grant approval.
+        </p>
+        <ul class="bulleted">
+          <li>The project has no license.</li>
+          <li>The project has only <strong>Other/Open source</strong> selected
+              as a license and the license info field does not contain a URL
+              to a license, or contains the whole license.
+          </li>
+          <li>The <strong>Other/Open source</strong> license appears to
+              discriminate against who can use the project, or how the
+              project's work can be used.
+          </li>
+          <li>The project details are too vague to know if it is legitimate.
+          </li>
+        </ul>
+
+        <p>
+          <strong>If the project looks like a test, a prank, or spam,
+          deactivate it.</strong>
         </p>
 
       <input type="hidden" name="next_url"
@@ -85,8 +87,6 @@
       </div>
     </div>
   </div>
-
-  <tal:menu replace="structure view/@@+related-pages" />
 </div>
 
 </body>