Merge lp:~sinzui/launchpad/wax-and-wane into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to fix from simple nuisances in launchpad.

    lp:~sinzui/launchpad/wax-and-wane
    Diff size: 206
    Launchpad bug: https://bugs.launchpad.net/bugs/wax-and-wane
    Test command: ./bin/test -vv \
        -t mailinglist-message-views
        -t milestone-views
    Pre-implementation: no one
    Target release: 10.01

Fix from simple nuisances in launchpad
--------------------------------------------------------------------

Bug #504024 [invalid list moderation markup breaks webkit]
    The HeldMessageView and message-moderation.pt create ill-formed markup
    that makes the message moderation page unusable in webkit browsers. I
    have observed two issues:

    1) The template uses `structure view/author`, but the author field is not
    HTML. The angle brackets are not escaped, creating an unclosed tag.

    2) The view's initialize() method is using `paragraphs.append('\n<p>\n')`
    which creates an unclosed paragraph. (this markup was deprecated in 1994
    and it invalid XHTML which the page claims to be).

    Both of these errors can cause form submittal errors.

Bug #505919 [milestones assignees wrap poorly]
    The assignee name wraps to the next line and it is difficult to see
    understand that the number belongs to the next name.

Bug #505917 [Sort milestone bugs by status by default]
    People working the bugs need to see status, then importance.

Rules
-----

Bug #504024 [invalid list moderation markup breaks webkit]
    * Add a <p> before each added paragraph and change the markup added
      afterwards to </p>
    * Remove the `structure` command

Bug #505919 [milestones assignees wrap poorly]
    * Add a CSS class to prevent the count and name from wrapping.
    * ADDENDUM: white-space: nowrap does not work because the comma is
      allowed to wrap. The pre value works if the markup is has no line breaks

Bug #505917 [Sort milestone bugs by status by default]
    * Change the sort order in the view.

QA
--

Bug #504024 [invalid list moderation markup breaks webkit]
    * Visit https://edge.launchpad.net/~launchpad-users/+mailinglist-moderate
    * Verify the From address shows the real address in the VIAGRA messages.
    * Expand a real message (not an empty spam)
    * Verify the paragraphs are shown as paragraphs.

Bug #505919 [milestones assignees wrap poorly]
    * Visit https://edge.launchpad.net/launchpad-registry/+milestone/10.01
    * Verify assignee names do not wrap in activities portlet.

Bug #505917 [Sort milestone bugs by status by default]
    * Visit https://edge.launchpad.net/launchpad-registry/+milestone/10.01
    * Verify that the bugs are listed from triaged, in progress, fix committed
      to fix released.

Lint
----

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css
  lib/lp/registry/browser/mailinglists.py
  lib/lp/registry/browser/milestone.py
  lib/lp/registry/browser/tests/mailinglist-message-views.txt
  lib/lp/registry/browser/tests/milestone-views.txt
  lib/lp/registry/browser/tests/test_views.py
  lib/lp/registry/templates/milestone-index.pt

Test
----

    * lib/lp/registry/browser/tests/test_views.py
      * Registered mailinglist-message-views.tx...

[Brad Crittenden (bac)]

Hi Curtis,

Thanks for these changes. Everything looks really good. I would suggest you put a test into _append_paragraph to ensure current_paragraph is not empty. This could happen if you closed the paragraph right before you hit line 20. It's not big deal but we shouldn't generate an empty <p></p> if we can easily help it.

review: Approve (code)

[Curtis Hovey (sinzui)]

Hi Brad.

On Tue, 2010-01-12 at 14:18 +0000, Brad Crittenden wrote:
> Review: Approve code
> Hi Curtis,
>
> Thanks for these changes. Everything looks really good. I would
> suggest you put a test into _append_paragraph to ensure
> current_paragraph is not empty. This could happen if you closed the
> paragraph right before you hit line 20. It's not big deal but we
> shouldn't generate an empty <p></p> if we can easily help it.

Thanks for your insight. I discounted the end case because leading and
trailing white space are stripped before the paragraphs are formatted.
But putting the rule in the helper method makes the rule clear and
universal. So I moved it.

=== modified file 'lib/lp/registry/browser/mailinglists.py'
--- lib/lp/registry/browser/mailinglists.py 2010-01-09 16:48:38 +0000
+++ lib/lp/registry/browser/mailinglists.py 2010-01-12 15:56:02 +0000
@@ -158,7 +158,7 @@
         for lineno, line in enumerate(details.splitlines()):
             if lineno > 20:
                 break
- if len(line.strip()) == 0 and len(current_paragraph) > 0:
+ if len(line.strip()) == 0:
                 self._append_paragraph(paragraphs, current_paragraph)
                 current_paragraph = []
             else:
@@ -167,6 +167,10 @@
         self.body_details = u''.join(paragraphs)

     def _append_paragraph(self, paragraphs, current_paragraph):
+ if len(current_paragraph) == 0:
+ # There is nothing to append. The message has multiple
+ # blank lines.
+ return
         paragraphs.append(u'\n<p>\n')
         paragraphs.append(u'\n'.join(current_paragraph))
         paragraphs.append(u'\n</p>\n')

--
__Curtis C. Hovey_________
http://launchpad.net/

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css	2010-01-06 02:02:18 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css	2010-01-13 14:18:16 +0000
@@ -276,6 +276,9 @@
     display: inline;
     padding: 0 1.5em 0 0;
     }
+.pre {
+    white-space: pre;
+    }
 .table-actions:nth-child(2) {
     /* text-align only works here because the <li> also has the
        style display:inline.
 
=== modified file 'lib/lp/registry/browser/mailinglists.py'
--- lib/lp/registry/browser/mailinglists.py	2009-08-16 17:21:54 +0000
+++ lib/lp/registry/browser/mailinglists.py	2010-01-13 14:18:16 +0000
@@ -130,7 +130,8 @@
         # The author field is very close to what the details has, except that
         # the view wants to include a link to the person's overview page.
         self.author = '<a href="%s">%s</a>' % (
-            canonical_url(self.details.author), self.details.sender)
+            canonical_url(self.details.author),
+            escape(self.details.sender))
 
     def initialize(self):
         """See `LaunchpadView`."""
@@ -158,13 +159,21 @@
             if lineno > 20:
                 break
             if len(line.strip()) == 0:
-                paragraphs.append(u'\n'.join(current_paragraph))
-                paragraphs.append('\n<p>\n')
+                self._append_paragraph(paragraphs, current_paragraph)
                 current_paragraph = []
             else:
                 current_paragraph.append(line)
+        self._append_paragraph(paragraphs, current_paragraph)
+        self.body_details = u''.join(paragraphs)
+
+    def _append_paragraph(self, paragraphs, current_paragraph):
+        if len(current_paragraph) == 0:
+            # There is nothing to append. The message has multiple
+            # blank lines.
+            return
+        paragraphs.append(u'\n<p>\n')
         paragraphs.append(u'\n'.join(current_paragraph))
-        self.body_details = u''.join(paragraphs)
+        paragraphs.append(u'\n</p>\n')
 
     def _remove_leading_blank_lines(self):
         """Strip off any leading blank lines.
 
=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py	2009-12-05 18:37:28 +0000
+++ lib/lp/registry/browser/milestone.py	2010-01-13 14:18:16 +0000
@@ -214,7 +214,7 @@
         """The list of non-conjoined bugtasks targeted to this milestone."""
         user = getUtility(ILaunchBag).user
         params = BugTaskSearchParams(user, milestone=self.context,
-                    orderby=['-importance', 'datecreated', 'id'],
+                    orderby=['status', '-importance', 'id'],
                     omit_dupes=True)
         tasks = getUtility(IBugTaskSet).search(params)
         # We could replace all the code below with a simple
 
=== added file 'lib/lp/registry/browser/tests/mailinglist-message-views.txt'
--- lib/lp/registry/browser/tests/mailinglist-message-views.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/mailinglist-message-views.txt	2010-01-13 14:18:16 +0000
@@ -0,0 +1,74 @@
+Held message view
+-----------------
+
+The HeldMessageView formats a held message so that it can be moderated by
+the mailing list administrator.
+
+    >>> from canonical.launchpad.interfaces import IMessageSet
+
+    >>> team, mlist = factory.makeTeamAndMailingList('parrot', 'owner')
+
+    >>> login('[e-mail]')
+    >>> message_set = getUtility(IMessageSet)
+    >>> message = message_set.fromEmail("""\
+    ... From: Carlos <[e-mail]>
+    ... To: [e-mail]
+    ... Subject: monkey
+    ... Message-ID: <monkey>
+    ... Date: Fri, 01 Aug 2000 01:09:00 -0000
+    ...
+    ... First paragraph.
+    ...
+    ... Second paragraph.
+    ...
+    ... Third paragraph.
+    ... """)
+    >>> held_message = mlist.holdMessage(message)
+    >>> transaction.commit()
+
+    >>> login_person(team.teamowner)
+    >>> view = create_initialized_view(
+    ...     held_message, name='+moderation', principal=team.teamowner)
+    >>> print view.widget_name
+    field.%3Cmonkey%3E
+    >>> print view.message_id
+    <monkey>
+    >>> print view.subject
+    monkey
+    >>> print view.date
+    2000-08-01 01:09:00+00:00
+    >>> print view.author
+    <a href="http://launchpad.dev/~carlos">Carlos &lt;[e-mail]&gt;</a>
+    >>> print view.body_summary
+    First paragraph.
+    >>> print view.body_details
+    <p>
+    Second paragraph.
+    </p>
+    <p>
+    Third paragraph.
+    </p>
+
+
+View helpers
+------------
+
+The view uses a helper method to format the paragraphs.
+
+    >>> paragraphs = []
+    >>> current_paragraph = ['line 1', 'line 2']
+    >>> view._append_paragraph(paragraphs, current_paragraph)
+    >>> for line in paragraphs:
+    ...     print line
+    <p>
+    line 1
+    line 2
+    </p>
+
+The _append_paragraph method ignores empty paragraphs.
+
+    >>> paragraphs = []
+    >>> current_paragraph = []
+    >>> view._append_paragraph(paragraphs, current_paragraph)
+    >>> len(paragraphs)
+    0
 
=== modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
--- lib/lp/registry/browser/tests/milestone-views.txt	2009-12-02 15:18:08 +0000
+++ lib/lp/registry/browser/tests/milestone-views.txt	2010-01-13 14:18:16 +0000
@@ -72,11 +72,14 @@
 The has_bugs_or_specs boolean property can be used to verify if the
 milestone has any bugs or specifications.
 
+    >>> from lp.bugs.interfaces.bugtask import BugTaskStatus
+
     >>> view.has_bugs_or_specs
     False
 
     >>> bug = factory.makeBug(title="kiwi")
     >>> bugtask = factory.makeBugTask(bug=bug, target=milestone.product)
+    >>> bugtask.transitionToStatus(BugTaskStatus.FIXCOMMITTED, person)
     >>> bugtask.transitionToAssignee(person)
     >>> bugtask.milestone = milestone
     >>> spec = factory.makeSpecification(
@@ -142,6 +145,7 @@
 
     >>> bug = factory.makeBug(title="emo")
     >>> bugtask = factory.makeBugTask(bug=bug, target=milestone.product)
+    >>> bugtask.transitionToAssignee(person)
     >>> bugtask.milestone = milestone
     >>> spec = factory.makeSpecification(
     ...     product=milestone.product, title='ostrich')
@@ -154,6 +158,15 @@
     >>> print view.specification_count_text
     <strong>2 blueprints</strong>
 
+Bugtasks are ordered by status (fix released last), and importance
+(critical first).
+
+    >>> for bugtask in view.bugtasks:
+    ...     assignee = bugtask.assignee
+    ...     print bugtask.bug.title, assignee.name, bugtask.status.title
+    emo   puffin-owner  New
+    kiwi  puffin-owner  Fix Committed
+
 The view provides a list of StatusCounts that summarise the targeted
 specifications and bugtasks.
 
@@ -174,7 +187,8 @@
 
     >>> for status_count in view.bugtask_status_counts:
     ...     print '%s: %s' % (status_count.status.title, status_count.count)
-    New: 2
+    New: 1
+    Fix Committed: 1
 
 The assignment_counts property returns all the users and count of bugs and
 specifications assigned to them.
@@ -711,4 +725,3 @@
     >>> view = create_initialized_view(milestone, '+delete')
     >>> check_permission('launchpad.Edit', view)
     False
-
 
=== modified file 'lib/lp/registry/browser/tests/test_views.py'
--- lib/lp/registry/browser/tests/test_views.py	2009-08-18 02:00:17 +0000
+++ lib/lp/registry/browser/tests/test_views.py	2010-01-13 14:18:16 +0000
@@ -21,6 +21,7 @@
 # that require something special like the librarian or mailman must run
 # on a layer that sets those services up.
 special_test_layer = {
+    'mailinglist-message-views.txt': LaunchpadFunctionalLayer,
     'milestone-views.txt': LaunchpadFunctionalLayer,
     'person-views.txt': LaunchpadFunctionalLayer,
     'user-to-user-views.txt': LaunchpadFunctionalLayer,
 
=== modified file 'lib/lp/registry/doc/message-holds.txt'
--- lib/lp/registry/doc/message-holds.txt	2009-05-12 21:39:35 +0000
+++ lib/lp/registry/doc/message-holds.txt	2010-01-13 14:18:16 +0000
@@ -377,58 +377,6 @@
     ----------------------------------------
 
 
-== Held message formatting ==
-
-When held messages are displayed in the u/i, they will be formatted in such a
-way that we can show the body summary with an ellipsis to review more body
-details.  Because formatting email messages for the web can get tricky, we
-take a fairly conservative 80/20 approach.
-
-    >>> from lp.registry.browser.mailinglists import HeldMessageView
-    >>> from canonical.launchpad.interfaces import (
-    ...     IHeldMessageDetails, IPersonSet)
-    >>> from zope.component import getUtility
-    >>> from zope.interface import implements
-
-First, we craft an object like what the view expects.
-
-    >>> person = getUtility(IPersonSet).getByName('name12')
-    >>> class FakeHeldMessageDetails:
-    ...     implements(IHeldMessageDetails)
-    ...     message_id = '<zebra>'
-    ...     subject = 'A subject'
-    ...     date = 'Today'
-    ...     author = person
-    ...     sender = '[e-mail]'
-    ...     body = """\
-    ... What are the Guadamen?  I think I might want to be come one
-    ... but I'm not sure.  Are they men who like guacamole?  Or are
-    ... they men who live on Gauadalcanal?  I used to live on Guadalcanal
-    ... and boy, do I sure like avocados!
-    ...
-    ... Please consider me for Guadamanhood.
-    ...
-    ... -Guy
-    ... """
-
-    >>> view = HeldMessageView(FakeHeldMessageDetails(), None)
-    >>> view.initialize()
-    >>> view.widget_name
-    'field.%3Czebra%3E'
-    >>> view.author
-    u'<a href="http://launchpad.dev/~name12">[e-mail]</a>'
-    >>> view.body_summary
-    'What are the Guadamen?  I think I might want to be come one'
-    >>> print view.body_details
-    but I'm not sure.  Are they men who like guacamole?  Or are
-    they men who live on Gauadalcanal?  I used to live on Guadalcanal
-    and boy, do I sure like avocados!
-    <p>
-    Please consider me for Guadamanhood.
-    <p>
-    -Guy
-
-
 == Posting privileges ==
 
 Message approvals are also used to derived posting privileges for non-team
 
=== modified file 'lib/lp/registry/templates/milestone-index.pt'
--- lib/lp/registry/templates/milestone-index.pt	2009-12-11 19:54:04 +0000
+++ lib/lp/registry/templates/milestone-index.pt	2010-01-13 14:18:16 +0000
@@ -147,11 +147,10 @@
             <dl>
               <dt>Assignees:</dt>
               <dd tal:define="count_statuses view/assignment_counts">
-                <tal:statuses repeat="count_status count_statuses">
-                  <strong tal:content="count_status/count">2</strong>&nbsp;<a
+                <span class="pre"
+                  tal:repeat="count_status count_statuses"><strong tal:content="count_status/count">2</strong> <a
                     tal:replace="structure count_status/status/fmt:link" /><tal:comma
-                    condition="not: repeat/count_status/end">,</tal:comma>
-                </tal:statuses>
+                    condition="not: repeat/count_status/end">,</tal:comma></span>
                 <tal:no-statuses condition="not: count_statuses">
                   No users assigned to blueprints and bugs.
                 </tal:no-statuses>