Merge lp:~stevenk/launchpad/queue-no-changes into lp:launchpad

[Steve Kowalik]

[r=julian-edwards][bug=810355] Support rejecting PackageUploads that have no changes file associated with them.

Support rejecting PackageUploads that have no changes file associated with them.

[Julian Edwards (julian-edwards)]

Thanks for making this fix Steve. Just a couple of small suggestions:

8 + if not changesfile_object:

This violates our coding style, using "is not None" if you mean that.

22 + if self.changesfile:

Same thing here.

Also you need an extra unit test for the notify() change.

review: Needs Fixing

[Robert Collins (lifeless)]

On Thu, Jul 14, 2011 at 9:58 PM, Julian Edwards
<email address hidden> wrote:
> Review: Needs Fixing
> Thanks for making this fix Steve.  Just a couple of small suggestions:
>
> 8       + if not changesfile_object:
>
> This violates our coding style, using "is not None" if you mean that.

Not anymore. We fixed that in Texas.
"Truth conditionals
Remember that False, None, [], and 0 are not the same although they
all evaluate to False in a boolean context. If this matters in your
code, be sure to check explicitly for either of them.

Also, checking the length may be an expensive operation. Casting to
bool may avoid this if the object specializes by implementing
nonzero."

> 22      + if self.changesfile:
>
> Same thing here.

-Rob

[Julian Edwards (julian-edwards)]

On Thursday 14 July 2011 11:16:30 you wrote:
> > 8 + if not changesfile_object:
> >
> > This violates our coding style, using "is not None" if you mean that.
>
> Not anymore. We fixed that in Texas.
> "Truth conditionals
> Remember that False, None, [], and 0 are not the same although they
> all evaluate to False in a boolean context. If this matters in your
> code, be sure to check explicitly for either of them.

Why was this not advertised more widely?

[Julian Edwards (julian-edwards)]

Make the test check for no email getting sent, and then it's r=me. Checking debug output is a little unhealthy :)

review: Approve

=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py	2011-06-14 20:45:03 +0000
+++ lib/lp/soyuz/adapters/notification.py	2011-07-18 02:47:35 +0000
@@ -160,6 +160,8 @@
     if spr is None and not bprs and not customfiles:
         # We do not have enough context to do a normal notification, so
         # reject what we do have.
+        if changesfile_object is None:
+            return
         reject_changes_file(
             blamer, changesfile_object.name, changes, archive, distroseries,
             summary_text, logger=logger)

=== modified file 'lib/lp/soyuz/adapters/tests/test_notification.py'
--- lib/lp/soyuz/adapters/tests/test_notification.py	2011-06-14 20:45:03 +0000
+++ lib/lp/soyuz/adapters/tests/test_notification.py	2011-07-18 02:47:35 +0000
@@ -155,6 +155,15 @@
         self.assertIn(
             'No recipients have a preferred email.', logger.getLogBuffer())
 
+    def test_reject_with_no_changes(self):
+        # If we don't have any files and no changes content, nothing happens.
+        archive = self.factory.makeArchive()
+        distroseries = self.factory.makeDistroSeries()
+        pocket = self.factory.getAnyPocket()
+        notify(None, None, (), (), archive, distroseries, pocket)
+        notifications = pop_notifications()
+        self.assertEqual(0, len(notifications))
+
     def _run_recipients_test(self, changes, blamer, maintainer, changer):
         distribution = self.factory.makeDistribution()
         archive = self.factory.makeArchive(

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2011-07-07 10:31:38 +0000
+++ lib/lp/soyuz/model/queue.py	2011-07-18 02:47:35 +0000
@@ -562,7 +562,10 @@
             # don't think we need them for sync rejections.
             return
 
-        changes_file_object = StringIO.StringIO(self.changesfile.read())
+        if self.changesfile is None:
+            changes_file_object = None
+        else:
+            changes_file_object = StringIO.StringIO(self.changesfile.read())
         # We allow unsigned uploads since they come from the librarian,
         # which are now stored unsigned.
         self.notify(
@@ -824,6 +827,8 @@
     def _getChangesDict(self, changes_file_object=None):
         """Return a dictionary with changes file tags in it."""
         if changes_file_object is None:
+            if self.changesfile is None:
+                return {}, ''
             changes_file_object = self.changesfile
         changes_content = changes_file_object.read()
 

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2011-07-11 08:31:46 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2011-07-18 02:47:35 +0000
@@ -838,3 +838,10 @@
         self.assertEqual(
             list(reversed(ordered_uploads)),
             list(getUtility(IPackageUploadSet).getAll(series)))
+
+    def test_rejectFromQueue_no_changes_file(self):
+        # If the PackageUpload has no changesfile, we can still reject it.
+        pu = self.factory.makePackageUpload()
+        pu.changesfile = None
+        pu.rejectFromQueue()
+        self.assertEqual(PackageUploadStatus.REJECTED, pu.status)