launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/reject-reason-over-api into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #31750 in Launchpad itself: "rejects should allow (and require) reasons"
  https://bugs.launchpad.net/launchpad/+bug/31750

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/reject-reason-over-api/+merge/165803

Add the ability to specify the rejection reason to IPackageUpload.rejectFromQueue() using the API. It will not get recorded into the DB, but will be in the e-mail sent to the uploader informing them of the rejection.
-- 
https://code.launchpad.net/~stevenk/launchpad/reject-reason-over-api/+merge/165803
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/reject-reason-over-api into lp:launchpad.

=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2013-04-16 21:13:11 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2013-05-27 02:23:27 +0000
@@ -365,9 +365,13 @@
         """
 
     @export_write_operation()
+    @operation_parameters(
+        rejection_comment=TextLine(
+            title=_("Rejection comment"), required=False))
     @call_with(user=REQUEST_USER)
     @operation_for_version("devel")
-    def rejectFromQueue(logger=None, dry_run=False, user=None):
+    def rejectFromQueue(logger=None, dry_run=False, user=None,
+                        rejection_comment=None):
         """Call setRejected, do a syncUpdate, and send notification email."""
 
     def realiseUpload(logger=None):

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2013-05-24 01:34:27 +0000
+++ lib/lp/soyuz/model/queue.py	2013-05-27 02:23:27 +0000
@@ -604,7 +604,8 @@
             client = AuditorClient()
             client.send(self, 'packageupload-accepted', user)
 
-    def rejectFromQueue(self, logger=None, dry_run=False, user=None):
+    def rejectFromQueue(self, logger=None, dry_run=False, user=None,
+                        rejection_comment=None):
         """See `IPackageUpload`."""
         self.setRejected()
         if self.package_copy_job is not None:
@@ -623,11 +624,16 @@
             changes_file_object = None
         else:
             changes_file_object = StringIO.StringIO(self.changesfile.read())
+        if rejection_comment:
+            summary_text = "Rejected by %s: %s" % (
+                user.displayname, rejection_comment)
+        else:
+            summary_text = "Rejected by %s." % user.displayname
         # We allow unsigned uploads since they come from the librarian,
         # which are now stored unsigned.
         self.notify(
             logger=logger, dry_run=dry_run,
-            changes_file_object=changes_file_object)
+            changes_file_object=changes_file_object, summary_text=summary_text)
         self.syncUpdate()
         if bool(getFeatureFlag('auditor.enabled')):
             client = AuditorClient()

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2013-04-16 21:13:11 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2013-05-27 02:23:27 +0000
@@ -337,6 +337,18 @@
         upload.rejectFromQueue()
         self.assertEqual(0, len(stub.test_emails))
 
+    def test_rejectFromQueue_source_with_reason(self):
+        # Rejecting a source package with a reason includes it in the email to
+        # the uploader.
+        self.test_publisher.prepareBreezyAutotest()
+        upload, uploader = self.makeSourcePackageUpload()
+        person = self.factory.makePerson()
+        upload.rejectFromQueue(user=person, rejection_comment='Because.')
+        self.assertEqual(1, len(stub.test_emails))
+        self.assertIn(
+            'Rejected:\nRejected by %s: Because.' % person.displayname,
+            stub.test_emails[0][-1])
+
 
 class TestPackageUploadPrivacy(TestCaseWithFactory):
     """Test PackageUpload security."""
@@ -472,13 +484,13 @@
         self.factory.makeBinaryPackageRelease(build=build)
         upload.addBuild(build)
         name_array = [
-            build.build.binarypackages[0].name for build in upload.builds]
+            b.build.binarypackages[0].name for b in upload.builds]
         name_array.extend(
             [b.build.source_package_release.name for b in upload.builds])
         names = ' '.join(sorted(name_array))
         self.assertEqual(names, upload.searchable_names)
         self.assertContentEqual(
-            [build.build.binarypackages[0].version for build in upload.builds],
+            [b.build.binarypackages[0].version for b in upload.builds],
             upload.searchable_versions)
 
     def test_searchables_for_builds_duplication(self):