launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/series-init-failure-explanations-bug-835024 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #835024 in Launchpad itself: "Unclear why series initialization failed"
  https://bugs.launchpad.net/launchpad/+bug/835024

For more details, see:
https://code.launchpad.net/~allenap/launchpad/series-init-failure-explanations-bug-835024/+merge/74800

This adds an error_description property to
InitializeDistroSeriesJob. This property backs onto the job's metadata
so is meant to be persistent. It is populated by the overridden
notifyUserError() method so that the error can be saved even after the
job's transaction is aborted; see BaseJobRunner.runJobHandleError()
for the mechanism.

I've also switched to using the new JSON property from Storm instead
of the _json_data/metadata hackette. This uncovered a bad test -
test__repr__ for InitializeDistroSeriesJob - which I have also fixed.

Pre-implementation discussion with bigjools. There is a follow-up
branch already in progress to make error_description appear in the UI.

-- 
https://code.launchpad.net/~allenap/launchpad/series-init-failure-explanations-bug-835024/+merge/74800
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/series-init-failure-explanations-bug-835024 into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/distributionjob.py'
--- lib/lp/soyuz/model/distributionjob.py	2011-03-24 14:17:33 +0000
+++ lib/lp/soyuz/model/distributionjob.py	2011-09-09 14:29:52 +0000
@@ -13,8 +13,8 @@
 from storm.locals import (
     And,
     Int,
+    JSON,
     Reference,
-    Unicode,
     )
 from zope.interface import implements
 
@@ -52,7 +52,7 @@
 
     job_type = EnumCol(enum=DistributionJobType, notNull=True)
 
-    _json_data = Unicode('json_data')
+    metadata = JSON('json_data')
 
     def __init__(self, distribution, distroseries, job_type, metadata):
         super(DistributionJob, self).__init__()
@@ -60,17 +60,13 @@
         self.distribution = distribution
         self.distroseries = distroseries
         self.job_type = job_type
-        self._json_data = self.serializeMetadata(metadata)
+        self.metadata = metadata
 
     @classmethod
     def serializeMetadata(cls, metadata_dict):
         """Serialize a dict of metadata into a unicode string."""
         return simplejson.dumps(metadata_dict).decode('utf-8')
 
-    @property
-    def metadata(self):
-        return simplejson.loads(self._json_data)
-
 
 class DistributionJobDerived(BaseRunnableJob):
     """Abstract class for deriving from DistributionJob."""

=== modified file 'lib/lp/soyuz/model/initializedistroseriesjob.py'
--- lib/lp/soyuz/model/initializedistroseriesjob.py	2011-08-04 10:37:58 +0000
+++ lib/lp/soyuz/model/initializedistroseriesjob.py	2011-09-09 14:29:52 +0000
@@ -31,7 +31,10 @@
     DistributionJobDerived,
     )
 from lp.soyuz.model.packageset import Packageset
-from lp.soyuz.scripts.initialize_distroseries import InitializeDistroSeries
+from lp.soyuz.scripts.initialize_distroseries import (
+    InitializationError,
+    InitializeDistroSeries,
+    )
 
 
 class InitializeDistroSeriesJob(DistributionJobDerived):
@@ -41,6 +44,8 @@
     class_job_type = DistributionJobType.INITIALIZE_SERIES
     classProvides(IInitializeDistroSeriesJobSource)
 
+    user_error_types = (InitializationError,)
+
     @classmethod
     def create(cls, child, parents, arches=(), packagesets=(),
                rebuild=False, overlays=(), overlay_pockets=(),
@@ -179,6 +184,10 @@
     def rebuild(self):
         return self.metadata['rebuild']
 
+    @property
+    def error_description(self):
+        return self.metadata.get("error_description")
+
     def run(self):
         """See `IRunnableJob`."""
         ids = InitializeDistroSeries(
@@ -188,6 +197,16 @@
         ids.check()
         ids.initialize()
 
+    def notifyUserError(self, error):
+        """Calls up and slso saves the error text in this job's metadata.
+
+        See `BaseRunnableJob`.
+        """
+        # This method is called when error is an instance of
+        # self.user_error_types.
+        super(InitializeDistroSeriesJob, self).notifyUserError(error)
+        self.metadata = dict(self.metadata, error_description=unicode(error))
+
     def getOopsVars(self):
         """See `IRunnableJob`."""
         vars = super(InitializeDistroSeriesJob, self).getOopsVars()

=== modified file 'lib/lp/soyuz/tests/test_initializedistroseriesjob.py'
--- lib/lp/soyuz/tests/test_initializedistroseriesjob.py	2011-08-02 16:24:30 +0000
+++ lib/lp/soyuz/tests/test_initializedistroseriesjob.py	2011-09-09 14:29:52 +0000
@@ -70,8 +70,8 @@
         packageset2 = self.factory.makePackageset()
 
         overlays = (True, False)
-        overlay_pockets = (('Updates',), ('Release',))
-        overlay_components = (("main",), ("universe",))
+        overlay_pockets = (u'Updates', u'Release')
+        overlay_components = (u"main", u"universe")
         arches = (u'i386', u'amd64')
         packagesets = (packageset1.id, packageset2.id)
         rebuild = False
@@ -84,8 +84,8 @@
             "distribution: {distroseries.distribution.name}, "
             "distroseries: {distroseries.name}, "
             "parent[overlay?/pockets/components]: "
-            "{parent1.name}[True/[u'Updates']/[u'main']],"
-            "{parent2.name}[False/[u'Release']/[u'universe']], "
+            "{parent1.name}[True/Updates/main],"
+            "{parent2.name}[False/Release/universe], "
             "architectures: (u'i386', u'amd64'), "
             "packagesets: [u'{packageset1.name}', u'{packageset2.name}'], "
             "rebuild: False>".format(
@@ -99,7 +99,6 @@
             repr(job)
         )
 
-
     def test_create_with_existing_pending_job(self):
         parent = self.factory.makeDistroSeries()
         distroseries = self.factory.makeDistroSeries()
@@ -197,6 +196,23 @@
         self.assertIsInstance(job, InitializeDistroSeriesJob)
         self.assertEqual(job.distroseries, distroseries)
 
+    def test_error_description_when_no_error(self):
+        # The InitializeDistroSeriesJob.error_description property returns
+        # None when no error description is recorded.
+        parent = self.factory.makeDistroSeries()
+        distroseries = self.factory.makeDistroSeries()
+        job = self.job_source.create(distroseries, [parent.id])
+        self.assertIs(None, removeSecurityProxy(job).error_description)
+
+    def test_error_description_set_when_notifying_about_user_errors(self):
+        # error_description is set by notifyUserError().
+        parent = self.factory.makeDistroSeries()
+        distroseries = self.factory.makeDistroSeries()
+        job = self.job_source.create(distroseries, [parent.id])
+        message = "This is an example message."
+        job.notifyUserError(InitializationError(message))
+        self.assertEqual(message, removeSecurityProxy(job).error_description)
+
 
 class InitializeDistroSeriesJobTestsWithPackages(TestCaseWithFactory):
     """Test case for InitializeDistroSeriesJob."""