Merge lp:~cjwatson/launchpad/report-pcj-oops into lp:launchpad

[Colin Watson]

Display a slightly more helpful error message if copying a package to a PPA OOPSes.

== Summary ==

Bug 1031092: When a package copy job OOPSes, the message displayed on Archive:+packages is not terribly informative.

== Proposed fix ==

Override PlainPackageCopyJob.notifyOops to store an "internal error" message in the job's error_message. The text I used is largely copied from BaseRunnableJob.getOopsMailController, with minor contextual adjustments.

== LOC Rationale ==

+24. This is part of removing the old synchronous copy path in Archive:+copy-packages and using the asynchronous path for everyone. Besides, I have 3911 lines of credit.

== Tests ==

bin/test -vvct test_packagecopyjob

== Demo and Q/A ==

This is easier to test while bug 1031089 is still open, since that provides a handy way to get a job to OOPS. Make sure you're in ~launchpad-beta-testers; copy a package from one PPA to another; delete it from the target PPA; copy it again. The message on the target PPA's +packages should be a bit more informative than before.

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-07-30 16:48:37 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-08-08 09:33:36 +0000
@@ -513,6 +513,15 @@
         if pu is not None:
             pu.setRejected()
 
+    def notifyOops(self, oops):
+        """See `IRunnableJob`."""
+        if not self.error_message:
+            self.reportFailure(
+                "Launchpad encountered an internal error while copying this"
+                " package.  It was logged with id %s.  Sorry for the"
+                " inconvenience." % oops["id"])
+        super(PlainPackageCopyJob, self).notifyOops(oops)
+
     def run(self):
         """See `IRunnableJob`."""
         try:
@@ -524,7 +533,7 @@
             logger = logging.getLogger()
             logger.info("Job:\n%s\nraised CannotCopy:\n%s" % (self, e))
             self.abort()  # Abort the txn.
-            self.reportFailure(e)
+            self.reportFailure(unicode(e))
 
             # If there is an associated PackageUpload we need to reject it,
             # else it will sit in ACCEPTED forever.
@@ -636,9 +645,8 @@
             for dsd in candidates
                 if dsd.parent_series.distributionID == source_distro_id]
 
-    def reportFailure(self, cannotcopy_exception):
+    def reportFailure(self, message):
         """Attempt to report failure to the user."""
-        message = unicode(cannotcopy_exception)
         if self.target_archive.purpose != ArchivePurpose.PPA:
             dsds = self.findMatchingDSDs()
             comment_source = getUtility(IDistroSeriesDifferenceCommentSource)

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-07-31 15:34:45 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-08-08 09:33:36 +0000
@@ -8,7 +8,10 @@
 
 from storm.store import Store
 from testtools.content import text_content
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    MatchesRegex,
+    MatchesStructure,
+    )
 import transaction
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
@@ -417,6 +420,20 @@
             "Destination pocket must be 'release' for a PPA.",
             job.error_message)
 
+    def test_target_ppa_message_unexpected_error(self):
+        # When copying to a PPA archive, unexpected errors are stored in the
+        # job's metadata with an apologetic message.
+        job = self.makePPAJob()
+        removeSecurityProxy(job).attemptCopy = FakeMethod(failure=Exception())
+        self.runJob(job)
+        self.assertEqual(JobStatus.FAILED, job.status)
+        self.assertThat(
+            job.error_message,
+            MatchesRegex(
+                "Launchpad encountered an internal error while copying this"
+                " package.  It was logged with id .*.  Sorry for the"
+                " inconvenience."))
+
     def test_run(self):
         # A proper test run synchronizes packages.
 
@@ -1525,7 +1542,7 @@
     def test_reportFailure(self):
         job = self.makeJob()
         switch_dbuser(self.dbuser)
-        removeSecurityProxy(job).reportFailure(CannotCopy("Mommy it hurts"))
+        removeSecurityProxy(job).reportFailure("Mommy it hurts")
 
 
 class TestPackageCopyJobSource(TestCaseWithFactory):