launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-797151 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #797151 in Launchpad itself: "Display DSD copy errors differently from comments"
  https://bugs.launchpad.net/launchpad/+bug/797151

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-797151/+merge/64537

= Summary =

The DistroSeriesDifference pages (mainly +localpackagediffs) let archive admins sync packages.  When a sync fails, the error is stored as a DistroSeriesDifferenceComment made by the Launchpad Janitor.

It would be nice if the latest comment as displayed on the DSD page were more easily recognizable as an error.


== Proposed fix ==

Give the latest DSDComment its own view that knows whether the comment is an error from Launchpad.  Have the TAL render an "error" icon if the view says so.


== Pre-implementation notes ==

This is a low-priority task, but one that Julian and I both wanted behind us.  I've gotten a lot done this week and so have earned it; it also gives some other branches a chance to go through review & land, which will make my follow-up branch easier tomorrow.


== Implementation details ==

It would have been nice to do the same for other DSDComments (you can "fold out" the comments list for any DSD) but they are rendered in an entirely different way, basically as generic Messages.  It might be nice to have a "this is an automated message from Launchpad" or "this is an error" facility there, but that's beyond the scope of this branch.


== Tests ==

{{{
./bin/test -vvc lp.registry.browser.tests.test_distroseriesdifferencecomment
}}}


== Demo and Q/A ==

Syncing packages on dogfood usually fails at the moment.  Try that.  The error messages should still show up as comments, but now with little error icons.

Conversely, entering a normal comment should also work but the comment will show up without the icon.
 

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_distroseriesdifferencecomment.py
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/distroseriesdifferencecomment.py
  lib/lp/registry/templates/distroseriesdifferencecomment-latest-comment-fragment.pt
-- 
https://code.launchpad.net/~jtv/launchpad/bug-797151/+merge/64537
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-797151 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2011-06-09 20:43:28 +0000
+++ lib/lp/registry/browser/configure.zcml	2011-06-14 12:37:21 +0000
@@ -200,7 +200,7 @@
     <browser:page
         for="..interfaces.distroseriesdifferencecomment.IDistroSeriesDifferenceComment"
         template="../templates/distroseriesdifferencecomment-latest-comment-fragment.pt"
-        class="canonical.launchpad.webapp.LaunchpadView"
+        class="..browser.distroseriesdifferencecomment.DistroSeriesDifferenceCommentView"
         permission="zope.Public"
         name="+latest-comment-fragment" />
     <adapter

=== added file 'lib/lp/registry/browser/distroseriesdifferencecomment.py'
--- lib/lp/registry/browser/distroseriesdifferencecomment.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/distroseriesdifferencecomment.py	2011-06-14 12:37:21 +0000
@@ -0,0 +1,29 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""View and helper for `DistroSeriesDifferenceComment`."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.launchpad.webapp import LaunchpadView
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+
+
+class DistroSeriesDifferenceCommentView(LaunchpadView):
+    """View class for `DistroSeriesDifferenceComment`.
+
+    :ivar css_class: The CSS class for this comment.  Package copy failures
+        are stored as `DistroSeriesDifferenceComments`, but rendered to be
+        visually recognizable as errors.
+    """
+
+    def __init__(self, *args, **kwargs):
+        super(DistroSeriesDifferenceCommentView, self).__init__(
+            *args, **kwargs)
+        error_persona = getUtility(ILaunchpadCelebrities).janitor
+        if self.context.comment_author == error_persona:
+            self.css_class = "sprite error-icon"
+        else:
+            self.css_class = ""

=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifferencecomment.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifferencecomment.py	2011-06-09 20:43:28 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifferencecomment.py	2011-06-14 12:37:21 +0000
@@ -6,8 +6,10 @@
 __metaclass__ = type
 
 from lxml import html
+from zope.component import getUtility
 
 from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.testing import TestCaseWithFactory
 from lp.testing.views import create_initialized_view
 
@@ -28,3 +30,22 @@
         self.assertEqual(
             "/~%s" % comment.comment_author.name,
             root.find("span").find("a").get("href"))
+
+    def test_comment_is_rendered_with_view_css_class(self):
+        comment = self.factory.makeDistroSeriesDifferenceComment()
+        view = create_initialized_view(comment, '+latest-comment-fragment')
+        view.css_class = self.factory.getUniqueString()
+        root = html.fromstring(view())
+        self.assertEqual(view.css_class, root.find("span").get("class"))
+
+    def test_view_css_class_is_empty_by_default(self):
+        comment = self.factory.makeDistroSeriesDifferenceComment(
+            comment=self.factory.getUniqueString())
+        view = create_initialized_view(comment, '+latest-comment-fragment')
+        self.assertEqual("", view.css_class)
+
+    def test_view_css_class_has_error_sprite_if_from_janitor(self):
+        comment = self.factory.makeDistroSeriesDifferenceComment(
+            owner=getUtility(ILaunchpadCelebrities).janitor)
+        view = create_initialized_view(comment, '+latest-comment-fragment')
+        self.assertEqual("sprite error-icon", view.css_class)

=== modified file 'lib/lp/registry/templates/distroseriesdifferencecomment-latest-comment-fragment.pt'
--- lib/lp/registry/templates/distroseriesdifferencecomment-latest-comment-fragment.pt	2011-06-09 20:38:23 +0000
+++ lib/lp/registry/templates/distroseriesdifferencecomment-latest-comment-fragment.pt	2011-06-14 12:37:21 +0000
@@ -3,8 +3,15 @@
     xmlns:tal="http://xml.zope.org/namespaces/tal";
     xmlns:i18n="http://xml.zope.org/namespaces/i18n";
     i18n:domain="launchpad">
-  <span tal:replace="context/body_text/fmt:shorten/50">I'm on this.</span>
-  <br /><span class="greyed-out greylink"><span
-  tal:replace="context/comment_date/fmt:approximatedate">2005-09-16</span>
-  by <a tal:replace="structure context/comment_author/fmt:link" /></span>
+  <span
+    tal:condition="view/css_class"
+    tal:attributes="class view/css_class"></span>
+  <tal:text tal:replace="context/body_text/fmt:shorten/50">
+    I'm on this.
+  </tal:text>
+  <br />
+  <span class="greyed-out greylink"><tal:date
+    replace="context/comment_date/fmt:approximatedate">2005-09-16</tal:date>
+  by
+  <tal:author replace="structure context/comment_author/fmt:link" /></span>
 </span>