launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/report-pcj-oops into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1031092 in Launchpad itself: "OOPSing PPA async copy shows no failure reason in web UI"
  https://bugs.launchpad.net/launchpad/+bug/1031092

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops/+merge/117601

== 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.
-- 
https://code.launchpad.net/~cjwatson/launchpad/report-pcj-oops/+merge/117601
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/report-pcj-oops into lp:launchpad.

=== 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-01 10:14:24 +0000
@@ -513,6 +513,14 @@
         if pu is not None:
             pu.setRejected()
 
+    def notifyOops(self, oops):
+        """See `IRunnableJob`."""
+        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 +532,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 +644,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-01 10:14:24 +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.