Merge lp:~stub/launchpad/oops into lp:launchpad

[Stuart Bishop]

[r=rvb][no-qa] Attach a link to the previous OOPS generated in the request, if any.

Attach a link to the previous OOPS generated in the request, if any.

[Raphaël Badin (rvb)]

Looks good.

One remark:

12 + if last_oopsid:

Not sure it's worth it but maybe it would be more safe/explicit to write: "if last_oopsid is not None:".

review: Approve

[Stuart Bishop (stub)]

Changed. Thanks!

[lp:~stub/launchpad/oops]

10005. By Stuart Bishop on 2011-11-14

Clarity

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2011-11-08 14:05:08 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-11-14 11:21:26 +0000
@@ -161,6 +161,14 @@
 _ignored_exceptions_for_unauthenticated_users = set(['Unauthorized'])
 
 
+def attach_previous_oopsid(report, context):
+    """Add a link to the previous OOPS generated this request, if any."""
+    request = context.get('http_request')
+    last_oopsid = getattr(request, 'oopsid', None)
+    if last_oopsid is not None:
+        report['last_oops'] = last_oopsid
+
+
 def attach_http_request(report, context):
     """Add request metadata into the error report.
 
@@ -312,6 +320,8 @@
         # In the zope environment we track how long a script / http
         # request has been running for - this is useful data!
         self._oops_config.on_create.append(attach_adapter_duration)
+        # Any previous OOPS reports generated this request.
+        self._oops_config.on_create.append(attach_previous_oopsid)
 
         def add_publisher(publisher):
             if publisher_adapter is not None:

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-10-27 05:40:56 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-11-14 11:21:26 +0000
@@ -184,6 +184,27 @@
         # - a notify publisher
         self.assertEqual(oops_config.publishers[2], notify_publisher)
 
+    def test_multiple_raises_in_request(self):
+        """An OOPS links to the previous OOPS in the request, if any."""
+        utility = ErrorReportingUtility()
+        del utility._oops_config.publishers[0]
+
+        request = TestRequestWithPrincipal()
+        try:
+            raise ArbitraryException('foo')
+        except ArbitraryException:
+            report = utility.raising(sys.exc_info(), request)
+
+        self.assertFalse('last_oops' in report)
+        last_oopsid = request.oopsid
+        try:
+            raise ArbitraryException('foo')
+        except ArbitraryException:
+            report = utility.raising(sys.exc_info(), request)
+
+        self.assertTrue('last_oops' in report)
+        self.assertEqual(report['last_oops'], last_oopsid)
+
     def test_raising_with_request(self):
         """Test ErrorReportingUtility.raising() with a request"""
         utility = ErrorReportingUtility()