Merge lp:~salgado/launchpad/polls-three-o into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

Convert a few poll pages to 3.0

== Proposed fix ==

Convert the easy edit pages and turn the action menu into a related
pages section at the bottom.

before/after screenshots at
https://devpad.canonical.com/~salgado/poll-shots/

== Pre-implementation notes ==

== Implementation details ==

== Tests ==

./bin/test -vvt stories.team-polls

== Demo and Q/A ==

= 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/poll-newoption.pt
  lib/lp/registry/stories/team-polls/edit-options.txt
  lib/lp/registry/templates/team-polls.pt
  lib/lp/registry/templates/polloption-edit.pt
  lib/lp/registry/browser/poll.py
  lib/lp/registry/templates/poll-edit.pt
  lib/lp/registry/stories/team-polls/create-poll-options.txt

[Jeroen T. Vermeulen (jtv)]

Hi Salgado,

I stubbed my toe a few times looking at the UI changes, until I realized
that I'm really missing a breadcrumb for the poll. Once you've drilled
down into a poll, everything you do in there refers back to the team.
On the edit-option page I found no reference to the poll whatsoever:
it's like you're editing a direct property of the team itself. Having
the poll shown as a first-class entity would make a big difference, I
think. But that's not for a template conversion to fix.

There were a few small inconsistencies, or at least things that I think
are inconsistent: adding an option gives me "Show options" and "Change
details" as related pages, plus a Save button, but editing one gives me
Save plus the Cancel link. Is there a reason why these are so
different?

Something that may force the issue is that the "Change details" link on
the new-option page is for the poll. Had I not known where this UI
element came from, I might have expected it to apply to the option I'm
adding. So I think it'd be more intuitive to remove these "related
pages" links from the new-option page and add a Cancel link back to the
poll instead.

Similarly, unless Martin has a rule against it, I think the poll edit
form should have a Cancel link. Right now there's no direct way back to
the poll page.

(The same goes for the options overview by the way, which is the only
place where I would really expect to be able to add or edit options. It
looks to me as though the options page wants to be integrated into the
poll page, with an "Add new option" link inline and without any remnants
of this old actions menu anywhere. Maybe that's what you have planned
anyway.)

Another very minor annoyance is that some of the page headings look a
little redundant. Sometimes they include the poll name, sometimes they
don't. Where they do, they duplicate the poll title displayed in small
font right above the heading. But I don't see any way of fixing that
without either removing that small title (which seems to be baked into
the application menu above it) or removing the poll name from the page
title--both of which sound like bad alternatives.

Speaking of headings: your branch sets the label for the poll edit form
to "Edit poll details." What exactly that text should read is an
ongoing discussion (started by Curtis on the mailing list), but more to
the point, I don't see it show up anywhere! If you provide a page_title
in this view it'll get picked up (and the one in pagetitles.py will no
longer be needed), but label doesn't seem to be.

Apart from that, the code as such looks good to me. Nice use of a mixin
for the navigation menu, though in this case I'd leave it out altogether
instead of migrating it to the 3.0 setup.

Jeroen

review: Needs Fixing

[Guilherme Salgado (salgado)]

On Wed, 2009-08-19 at 08:13 +0000, Jeroen T. Vermeulen wrote:
> Review: Needs Fixing
> Hi Salgado,
>
> I stubbed my toe a few times looking at the UI changes, until I realized
> that I'm really missing a breadcrumb for the poll. Once you've drilled
> down into a poll, everything you do in there refers back to the team.
> On the edit-option page I found no reference to the poll whatsoever:
> it's like you're editing a direct property of the team itself. Having
> the poll shown as a first-class entity would make a big difference, I
> think. But that's not for a template conversion to fix.

I think everybody who's used polls had a similar experience to yours.
They were written a long time ago and were left mostly untouched since
then, even when we redesigned navigation/UI for 1.0 and 2.0

>
> There were a few small inconsistencies, or at least things that I think
> are inconsistent: adding an option gives me "Show options" and "Change
> details" as related pages, plus a Save button, but editing one gives me
> Save plus the Cancel link. Is there a reason why these are so
> different?

That's because URL of these related pages are relative, so they work on
IPoll pages (like +newoption) but not on IPollOption ones (like
+option/id). The Cancel link was added there only to make it possible
to return to the +options page, but all forms should have a Cancel link.

>
> Something that may force the issue is that the "Change details" link on
> the new-option page is for the poll. Had I not known where this UI
> element came from, I might have expected it to apply to the option I'm
> adding. So I think it'd be more intuitive to remove these "related
> pages" links from the new-option page and add a Cancel link back to the
> poll instead.

I think that's a good thing to do.

>
> Similarly, unless Martin has a rule against it, I think the poll edit
> form should have a Cancel link. Right now there's no direct way back to
> the poll page.

They all should have a Cancel button.

>
> (The same goes for the options overview by the way, which is the only
> place where I would really expect to be able to add or edit options. It
> looks to me as though the options page wants to be integrated into the
> poll page, with an "Add new option" link inline and without any remnants
> of this old actions menu anywhere. Maybe that's what you have planned
> anyway.)

In fact, my only plan is to migrate these pages without making things
any worse than they already are and spend the least possible amount of
time doing so.

But I really liked your suggestion, so I filed
https://bugs.edge.launchpad.net/bugs/415896

>
> Another very minor annoyance is that some of the page headings look a
> little redundant. Sometimes they include the poll name, sometimes they
> don't. Where they do, they duplicate the poll title displayed in small
> font right above the heading. But I don't see any way of fixing that
> without either removing that small title (which seems to be baked into
> the application menu above it) or removing the poll name from the page
> title--both of which sound like bad alternatives.
>
> Speaking of headings: your branch sets the label for the pol...

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-08-18 04:57:21 +0000
+++ lib/lp/registry/browser/configure.zcml	2009-08-18 20:24:56 +0000
@@ -600,7 +600,8 @@
         <browser:menus
             module="lp.registry.browser.poll"
             classes="
-                PollContextMenu"/>
+                PollContextMenu
+                PollEditNavigationMenu"/>
         <browser:defaultView
             for="lp.registry.interfaces.poll.IPoll"
             name="+index"/>
 
=== modified file 'lib/lp/registry/browser/poll.py'
--- lib/lp/registry/browser/poll.py	2009-06-25 04:06:00 +0000
+++ lib/lp/registry/browser/poll.py	2009-08-18 20:24:56 +0000
@@ -7,6 +7,7 @@
     'BasePollView',
     'PollAddView',
     'PollContextMenu',
+    'PollEditNavigationMenu',
     'PollEditView',
     'PollNavigation',
     'PollOptionAddView',
@@ -17,13 +18,14 @@
 
 from zope.event import notify
 from zope.component import getUtility
+from zope.interface import implements, Interface
 from zope.lifecycleevent import ObjectCreatedEvent
 from zope.app.form.browser import TextWidget
 
 from canonical.launchpad.webapp import (
     action, canonical_url, ContextMenu, custom_widget,
     enabled_with_permission, LaunchpadEditFormView, LaunchpadFormView, Link,
-    Navigation, stepthrough)
+    Navigation, NavigationMenu, stepthrough)
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from lp.registry.interfaces.poll import (
     IPoll, IPollOption, IPollOptionSet, IPollSubset, IVoteSet, PollAlgorithm,
@@ -31,13 +33,10 @@
 from canonical.launchpad.helpers import shortlist
 
 
-class PollContextMenu(ContextMenu):
-
-    usedfor = IPoll
-    links = ['showall', 'addnew', 'edit']
+class PollEditLinksMixin:
 
     def showall(self):
-        text = 'Show option details'
+        text = 'Show options'
         return Link('+options', text, icon='info')
 
     @enabled_with_permission('launchpad.Edit')
@@ -51,6 +50,21 @@
         return Link('+edit', text, icon='edit')
 
 
+class PollContextMenu(ContextMenu, PollEditLinksMixin):
+    usedfor = IPoll
+    links = ['showall', 'addnew', 'edit']
+
+
+class IPollEditMenu(Interface):
+    """A marker interface for the 'Change details' navigation menu."""
+
+
+class PollEditNavigationMenu(NavigationMenu, PollEditLinksMixin):
+    usedfor = IPollEditMenu
+    facet = 'overview'
+    links = ['showall', 'addnew', 'edit']
+
+
 class PollNavigation(Navigation):
 
     usedfor = IPoll
@@ -353,6 +367,7 @@
 
 class PollEditView(LaunchpadEditFormView):
 
+    implements(IPollEditMenu)
     schema = IPoll
     label = "Edit poll details"
     field_names = ["name", "title", "proposition", "allowspoilt", "dateopens",
@@ -372,6 +387,11 @@
     field_names = ["name", "title"]
     custom_widget("title", TextWidget, width=30)
 
+    @property
+    def cancel_url(self):
+        """See `LaunchpadFormView`."""
+        return canonical_url(self.context.poll, view_name='+options')
+
     @action("Save", name="save")
     def save_action(self, action, data):
         self.updateContextFromData(data)
@@ -381,6 +401,7 @@
 class PollOptionAddView(LaunchpadFormView):
     """Create a new option in a given poll."""
 
+    implements(IPollEditMenu)
     schema = IPollOption
     label = "Create new poll option"
     field_names = ["name", "title"]
 
=== modified file 'lib/lp/registry/stories/team-polls/create-poll-options.txt'
--- lib/lp/registry/stories/team-polls/create-poll-options.txt	2009-04-17 10:32:16 +0000
+++ lib/lp/registry/stories/team-polls/create-poll-options.txt	2009-08-18 20:24:56 +0000
@@ -37,7 +37,7 @@
 And here we see the option listed as one of the active options for this
 poll.
 
-    >>> team_admin_browser.getLink('Show option details').click()
+    >>> team_admin_browser.getLink('Show options').click()
     >>> team_admin_browser.url
     'http://launchpad.dev/~ubuntu-team/+poll/dpl-2080/+options'
 
 
=== modified file 'lib/lp/registry/stories/team-polls/edit-options.txt'
--- lib/lp/registry/stories/team-polls/edit-options.txt	2008-10-15 17:56:31 +0000
+++ lib/lp/registry/stories/team-polls/edit-options.txt	2009-08-18 20:24:56 +0000
@@ -7,7 +7,7 @@
     >>> user_browser.getLink('A public poll that never closes').click()
     >>> user_browser.url
     'http://launchpad.dev/~ubuntu-team/+poll/never-closes4'
-    >>> user_browser.getLink('Show option details').click()
+    >>> user_browser.getLink('Show options').click()
     >>> user_browser.url
     'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+options'
     >>> user_browser.getLink('Edit')
@@ -21,7 +21,7 @@
     >>> browser = setupBrowser(
     ...     auth='Basic [e-mail]:jdub')
     >>> browser.open('http://launchpad.dev/~ubuntu-team/+poll/never-closes4')
-    >>> browser.getLink('Show option details').click()
+    >>> browser.getLink('Show options').click()
     >>> browser.url
     'http://launchpad.dev/~ubuntu-team/+poll/never-closes4/+options'
     >>> browser.getLink('[Edit]').click()
@@ -43,7 +43,7 @@
     >>> browser.url
     'http://launchpad.dev/~ubuntu-team/+poll/not-yet-opened'
 
-    >>> browser.getLink('Show option details').click()
+    >>> browser.getLink('Show options').click()
     >>> browser.url
     'http://launchpad.dev/~ubuntu-team/+poll/not-yet-opened/+options'
 
 
=== modified file 'lib/lp/registry/templates/poll-edit.pt'
--- lib/lp/registry/templates/poll-edit.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/poll-edit.pt	2009-08-18 20:24:56 +0000
@@ -3,19 +3,12 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="context/@@main_template/master"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 
 <body>
 
-  <metal:portlets fill-slot="portlets">
-    <div tal:replace="structure context/@@+portlet-details" />
-  </metal:portlets>
-
   <div metal:fill-slot="main">
 
     <div tal:condition="context/isNotYetOpened">
@@ -33,6 +26,8 @@
       opens it can't be edited anymore.</p>
     </div>
     
+    <tal:menu replace="structure view/@@+related-pages" /> 
+
   </div>
 
 </body>
 
=== modified file 'lib/lp/registry/templates/poll-newoption.pt'
--- lib/lp/registry/templates/poll-newoption.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/poll-newoption.pt	2009-08-18 20:24:56 +0000
@@ -3,19 +3,12 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="context/@@main_template/master"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 
 <body>
 
-  <metal:portlets fill-slot="portlets">
-    <div tal:replace="structure context/@@+portlet-details" />
-  </metal:portlets>
-
   <div metal:fill-slot="main">
 
     <tal:block condition="context/isNotYetOpened">
@@ -37,6 +30,8 @@
       </p>
     </tal:block>
 
+    <tal:menu replace="structure view/@@+related-pages" /> 
+
   </div>
 
 </body>
 
=== modified file 'lib/lp/registry/templates/polloption-edit.pt'
--- lib/lp/registry/templates/polloption-edit.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/polloption-edit.pt	2009-08-18 20:24:56 +0000
@@ -3,19 +3,12 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="context/@@main_template/master"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 
 <body>
 
-  <metal:portlets fill-slot="portlets">
-    <div tal:replace="structure context/poll/@@+portlet-details" />
-  </metal:portlets>
-
   <div metal:fill-slot="main">
 
     <tal:block condition="context/poll/isNotYetOpened">
 
=== modified file 'lib/lp/registry/templates/team-polls.pt'
--- lib/lp/registry/templates/team-polls.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/team-polls.pt	2009-08-18 20:24:56 +0000
@@ -78,8 +78,8 @@
 
     <br />
     <tal:block tal:condition="request/lp:person">
-      <ul class="add" tal:condition="context/required:launchpad.Edit">
-        <li><a href="+newpoll">Set up a new poll</a></li>
+      <ul tal:condition="context/required:launchpad.Edit">
+        <li><a class="sprite add" href="+newpoll">Set up a new poll</a></li>
       </ul>
     </tal:block>