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

Proposed by Steve Kowalik
Status: Merged
Approved by: Steve Kowalik
Approved revision: no longer in the source branch.
Merged at revision: 13455
Proposed branch: lp:~stevenk/launchpad/queue-no-changes
Merge into: lp:launchpad
Diff against target: 72 lines (+24/-1)
4 files modified
lib/lp/soyuz/adapters/notification.py (+2/-0)
lib/lp/soyuz/adapters/tests/test_notification.py (+9/-0)
lib/lp/soyuz/model/queue.py (+6/-1)
lib/lp/soyuz/tests/test_packageupload.py (+7/-0)
To merge this branch: bzr merge lp:~stevenk/launchpad/queue-no-changes
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+67913@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

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
Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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?

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
160 if spr is None and not bprs and not customfiles:160 if spr is None and not bprs and not customfiles:
161 # We do not have enough context to do a normal notification, so161 # We do not have enough context to do a normal notification, so
162 # reject what we do have.162 # reject what we do have.
163 if changesfile_object is None:
164 return
163 reject_changes_file(165 reject_changes_file(
164 blamer, changesfile_object.name, changes, archive, distroseries,166 blamer, changesfile_object.name, changes, archive, distroseries,
165 summary_text, logger=logger)167 summary_text, logger=logger)
166168
=== 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 @@
155 self.assertIn(155 self.assertIn(
156 'No recipients have a preferred email.', logger.getLogBuffer())156 'No recipients have a preferred email.', logger.getLogBuffer())
157157
158 def test_reject_with_no_changes(self):
159 # If we don't have any files and no changes content, nothing happens.
160 archive = self.factory.makeArchive()
161 distroseries = self.factory.makeDistroSeries()
162 pocket = self.factory.getAnyPocket()
163 notify(None, None, (), (), archive, distroseries, pocket)
164 notifications = pop_notifications()
165 self.assertEqual(0, len(notifications))
166
158 def _run_recipients_test(self, changes, blamer, maintainer, changer):167 def _run_recipients_test(self, changes, blamer, maintainer, changer):
159 distribution = self.factory.makeDistribution()168 distribution = self.factory.makeDistribution()
160 archive = self.factory.makeArchive(169 archive = self.factory.makeArchive(
161170
=== 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 @@
562 # don't think we need them for sync rejections.562 # don't think we need them for sync rejections.
563 return563 return
564564
565 changes_file_object = StringIO.StringIO(self.changesfile.read())565 if self.changesfile is None:
566 changes_file_object = None
567 else:
568 changes_file_object = StringIO.StringIO(self.changesfile.read())
566 # We allow unsigned uploads since they come from the librarian,569 # We allow unsigned uploads since they come from the librarian,
567 # which are now stored unsigned.570 # which are now stored unsigned.
568 self.notify(571 self.notify(
@@ -824,6 +827,8 @@
824 def _getChangesDict(self, changes_file_object=None):827 def _getChangesDict(self, changes_file_object=None):
825 """Return a dictionary with changes file tags in it."""828 """Return a dictionary with changes file tags in it."""
826 if changes_file_object is None:829 if changes_file_object is None:
830 if self.changesfile is None:
831 return {}, ''
827 changes_file_object = self.changesfile832 changes_file_object = self.changesfile
828 changes_content = changes_file_object.read()833 changes_content = changes_file_object.read()
829834
830835
=== 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 @@
838 self.assertEqual(838 self.assertEqual(
839 list(reversed(ordered_uploads)),839 list(reversed(ordered_uploads)),
840 list(getUtility(IPackageUploadSet).getAll(series)))840 list(getUtility(IPackageUploadSet).getAll(series)))
841
842 def test_rejectFromQueue_no_changes_file(self):
843 # If the PackageUpload has no changesfile, we can still reject it.
844 pu = self.factory.makePackageUpload()
845 pu.changesfile = None
846 pu.rejectFromQueue()
847 self.assertEqual(PackageUploadStatus.REJECTED, pu.status)