Code review comment for lp://staging/~gmb/launchpad/show-oops-ids-bug-564686

I've added an oops-id formatter and tests to go with it. Here's a diff of the new code:

=== modified file 'lib/canonical/launchpad/webapp/tales.py'
--- lib/canonical/launchpad/webapp/tales.py 2010-05-05 20:01:56 +0000
+++ lib/canonical/launchpad/webapp/tales.py 2010-05-17 15:54:18 +0000
@@ -3025,6 +3025,16 @@
         else:
             return id_

+ def oops_id(self):
+ """Format an OOPS ID for display."""
+ if not getUtility(ILaunchBag).developer:
+ # We only linkify OOPS IDs for Launchpad developers.
+ return self._stringtoformat
+
+ root_url = config.launchpad.oops_root_url
+ url = root_url + self._stringtoformat
+ return '<a href="%s">%s</a>' % (url, self._stringtoformat)
+
     def traverse(self, name, furtherPath):
         if name == 'nl_to_br':
             return self.nl_to_br()
@@ -3063,6 +3073,8 @@
                 return self.css_id(furtherPath.pop())
             else:
                 return self.css_id()
+ elif name == 'oops-id':
+ return self.oops_id()
         else:
             raise TraversalError(name)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_tales.py'
--- lib/canonical/launchpad/webapp/tests/test_tales.py 2009-11-18 00:22:41 +0000
+++ lib/canonical/launchpad/webapp/tests/test_tales.py 2010-05-17 16:43:54 +0000
@@ -6,9 +6,12 @@
 from textwrap import dedent
 import unittest

+from zope.component import getUtility
 from zope.testing.doctestunit import DocTestSuite

+from canonical.config import config
 from canonical.launchpad.testing.pages import find_tags_by_class
+from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.tales import FormattersAPI
 from canonical.testing import DatabaseFunctionalLayer
 from lp.testing import TestCase
@@ -263,6 +266,44 @@
         self.assertEqual(3, line_count)

+class TestOOPSFormatter(TestCase):
+ """A test case for the oops_id() string formatter."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_doesnt_linkify_for_non_developers(self):
+ # OOPS IDs won't be linkified for non-developers.
+ oops_id = 'OOPS-12345TEST'
+ formatter = FormattersAPI(oops_id)
+ formatted_string = formatter.oops_id()
+
+ self.assertEqual(
+ oops_id, formatted_string,
+ "Formatted string should be '%s', was '%s'" % (
+ oops_id, formatted_string))
+
+ def _setDeveloper(self):
+ """Override ILaunchBag.developer for testing purposes."""
+ launch_bag = getUtility(ILaunchBag)
+ launch_bag.setDeveloper(True)
+
+ def test_linkifies_for_developers(self):
+ # OOPS IDs will be linkified for Launchpad developers.
+ oops_id = 'OOPS-12345TEST'
+ formatter = FormattersAPI(oops_id)
+
+ self._setDeveloper()
+ formatted_string = formatter.oops_id()
+
+ expected_string = '<a href="%s">%s</a>' % (
+ config.launchpad.oops_root_url + oops_id, oops_id)
+
+ self.assertEqual(
+ expected_string, formatted_string,
+ "Formatted string should be '%s', was '%s'" % (
+ expected_string, formatted_string))
+
+
 def test_suite():
     """Return this module's doctest Suite. Unit tests are also run."""
     suite = unittest.TestSuite()

=== modified file 'lib/lp/bugs/browser/bugwatch.py'
--- lib/lp/bugs/browser/bugwatch.py 2010-05-17 15:21:47 +0000
+++ lib/lp/bugs/browser/bugwatch.py 2010-05-17 15:54:18 +0000
@@ -180,7 +180,6 @@
         """Return a list of dicts representing recent watch activity."""
         activity_items = []
         for activity in self.context.activity:
- oops_str = None
             if activity.result in BUG_WATCH_ACTIVITY_SUCCESS_STATUSES:
                 icon = "/@@/yes"
                 completion_message = "completed successfully"
@@ -189,24 +188,12 @@
                 completion_message = (
                     "failed with error '%s'" % activity.result.title)

- if activity.oops_id is not None:
- if getUtility(ILaunchBag).developer:
- # If the user is an LP developer we linkify the
- # OOPS ID.
- root_url = config.launchpad.oops_root_url
- oops_url = root_url + activity.oops_id
- oops_str = '<a href="%s">%s</a>' % (
- oops_url, activity.oops_id)
- else:
- oops_str = activity.oops_id
-
-
             activity_items.append({
                 'icon': icon,
                 'date': activity.activity_date,
                 'completion_message': completion_message,
                 'result_text': activity.result.title,
- 'oops_str': oops_str,
+ 'oops_id': activity.oops_id,
                 })

         return activity_items

=== modified file 'lib/lp/bugs/templates/bugwatch-portlet-activity.pt'
--- lib/lp/bugs/templates/bugwatch-portlet-activity.pt 2010-05-17 15:21:47 +0000
+++ lib/lp/bugs/templates/bugwatch-portlet-activity.pt 2010-05-17 15:54:18 +0000
@@ -37,8 +37,8 @@
         <img tal:attributes="src activity/icon; title activity/result_text" />
         Update
         <tal:message replace="activity/completion_message" />
- <tal:oopsid condition="activity/oops_str">
- (<tal:oops_link replace="structure activity/oops_str" />)
+ <tal:oopsid condition="activity/oops_id">
+ (<tal:oops_link replace="structure activity/oops_id/fmt:oops-id" />)
         </tal:oopsid>
         <tal:time replace="activity/date/fmt:displaydate" />
       </div>