Code review comment for lp:~sinzui/launchpad/wax-and-wane

Revision history for this message
Curtis Hovey (sinzui) wrote :

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.txt to run on the
        LaunchpadFunctionalLayer
    * lib/lp/registry/browser/tests/mailinglist-message-views.txt
      * Added a test to verify the messages fields wrapped by the view.
    * lib/lp/registry/browser/tests/milestone-views.txt
      * Updated the test to verify the bugs are sorted by status first.

NOTE: The trasitionToAssign() call in @@ -142,6 +145,7 @@ is a conundrum.
    * The call to transitionToStatus() changed the count of bugs assigned
      to puffin-owner. This is very wrong. After careful reading I discovered
      that puffin-owner was only assigned one bug--something in the test
      setup was wrong and that the new change illustrated that. I added the
      call to trasitionToAssign() so that there were two uses of it in the
      test to match the two I verity.
    * I then wasted my weekend trying to understand how the test passed before
      my changes. I suspected that status counts were cached incorrectly,
      but I could not locate an error in the code. I was hopeful that I
      had found the missing python time that we see in milestone timeouts,
      but my tests with hundred of counted bugs did not reveal a looping
      error.

Implementation
--------------

    * lib/canonical/launchpad/icing/style-3-0.css
      * Added a .pre class for markup must must behave as a pre. (works for
        inline elements unlike the HTML element which is always block)
    * lib/lp/registry/browser/mailinglists.py
      * cgi escaped the message sender
      * Extracted the paragraph append rules to a helper and used it in both
        places. The diff is a but messed up here. I discovered that it was
        possible to get empty paragraphs when testing, so I added a rule to
        avoid adding.
    * lib/lp/registry/browser/milestone.py
      * Add status tot he sort and removed datecreated (because it will be
        the same order as id).
    * lib/lp/registry/templates/milestone-index.pt
      * Updated th markup to use the new .pre class to prevent the assignee
        count items from wrapping.

« Back to merge proposal