launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/packageupload-encapsulate-more-shit into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/packageupload-encapsulate-more-shit/+merge/62978

Quick change to make PackageUpload acceptance and rejection work with PackageCopyJobs.

I'm not happy with the "state transition dance" but then changing the job system to cope with this corner case seems wrong.  Tell me what you think.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/packageupload-encapsulate-more-shit/+merge/62978
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/packageupload-encapsulate-more-shit into lp:launchpad/db-devel.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-05-30 13:52:37 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-05-31 13:34:43 +0000
@@ -137,7 +137,7 @@
         :raises: NotFoundError if there is no job with the specified id, or
             its job_type does not match the desired subclass.
         """
-        job = PackageCopyJob.get(job_id)
+        job = IStore(PackageCopyJob).get(PackageCopyJob, job_id)
         if job.job_type != cls.class_job_type:
             raise NotFoundError(
                 'No object found with id %d and type %s' % (job_id,

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2011-05-29 22:53:03 +0000
+++ lib/lp/soyuz/model/queue.py	2011-05-31 13:34:43 +0000
@@ -401,7 +401,24 @@
         """See `IPackageUpload`."""
         assert not self.is_delayed_copy, 'Cannot process delayed copies.'
 
+        if self.package_copy_job is not None:
+            if self.status == PackageUploadStatus.REJECTED:
+                raise QueueInconsistentStateError(
+                    "Can't resurrect rejected syncs")
+            # Circular imports :(
+            from lp.soyuz.model.packagecopyjob import PlainPackageCopyJob
+            # Release the job hounds, Smithers.
+            self.setAccepted()
+            job = PlainPackageCopyJob.get(self.package_copy_job_id)
+            job.resume()
+            # The copy job will send emails as appropriate.  We don't
+            # need to worry about closing bugs from syncs, although we
+            # should probably give karma but that needs more work to
+            # fix here.
+            return
+
         self.setAccepted()
+
         changes_file_object = StringIO.StringIO(self.changesfile.read())
         # We explicitly allow unsigned uploads here since the .changes file
         # is pulled from the librarian which are stripped of their
@@ -438,6 +455,18 @@
     def rejectFromQueue(self, logger=None, dry_run=False):
         """See `IPackageUpload`."""
         self.setRejected()
+        if self.package_copy_job is not None:
+            # Circular imports :(
+            from lp.soyuz.model.packagecopyjob import PlainPackageCopyJob
+            job = PlainPackageCopyJob.get(self.package_copy_job_id)
+            # Do the state transition dance.
+            job.queue()
+            job.start()
+            job.fail()
+            # This possibly should be sending a rejection email but I
+            # don't think we need them for sync rejections.
+            return
+
         changes_file_object = StringIO.StringIO(self.changesfile.read())
         # We allow unsigned uploads since they come from the librarian,
         # which are now stored unsigned.

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2011-05-29 22:53:03 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2011-05-31 13:34:43 +0000
@@ -21,6 +21,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.log.logger import BufferLogger
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.mail import stub
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -31,6 +32,7 @@
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.queue import (
     IPackageUploadSet,
+    QueueInconsistentStateError,
     )
 from lp.soyuz.interfaces.section import ISectionSet
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
@@ -383,3 +385,33 @@
 
         self.assertEqual(
             expected_metadata, plain_copy_job.metadata)
+
+    def test_acceptFromQueue_with_copy_job(self):
+        # acceptFromQueue should accept the upload and resume the copy
+        # job.
+        plain_copy_job = self.factory.makePlainPackageCopyJob()
+        pcj = removeSecurityProxy(plain_copy_job).context
+        pu = self.factory.makePackageUpload(package_copy_job=pcj)
+        self.assertEqual(PackageUploadStatus.NEW, pu.status)
+        plain_copy_job.suspend()
+
+        pu.acceptFromQueue(announce_list=None)
+
+        self.assertEqual(PackageUploadStatus.ACCEPTED, pu.status)
+        self.assertEqual(JobStatus.WAITING, plain_copy_job.status)
+
+    def test_rejectFromQueue_with_copy_job(self):
+        # rejectFromQueue will reject the upload and fail the copy job.
+        plain_copy_job = self.factory.makePlainPackageCopyJob()
+        pcj = removeSecurityProxy(plain_copy_job).context
+        pu = self.factory.makePackageUpload(package_copy_job=pcj)
+        plain_copy_job.suspend()
+
+        pu.rejectFromQueue()
+
+        self.assertEqual(PackageUploadStatus.REJECTED, pu.status)
+        self.assertEqual(JobStatus.FAILED, plain_copy_job.status)
+
+        # It cannot be resurrected after rejection.
+        self.assertRaises(
+            QueueInconsistentStateError, pu.acceptFromQueue, None)