Code review comment for lp:~al-maisan/launchpad/unembargo-443075

Hi Muharem,

nice work, just a few cosmetic remarks, see below.

Abel

> === modified file 'lib/lp/soyuz/model/queue.py'
> --- lib/lp/soyuz/model/queue.py 2009-10-01 07:05:22 +0000
> +++ lib/lp/soyuz/model/queue.py 2009-10-09 09:50:27 +0000
> @@ -367,15 +367,6 @@
> assert self.sources.count() == 1, (
> 'Source is mandatory for delayed copies.')
> self.setAccepted()
> - # The second assert guarantees that we'll actually have a SPR.
> - spr = self.mySourcePackageRelease()
> - # Use the changesfile of the original upload.
> - changes_file_object = StringIO.StringIO(
> - spr.package_upload.changesfile.read())
> - self.notify(
> - announce_list=self.distroseries.changeslist,
> - changes_file_object=changes_file_object, allow_unsigned=True)
> - self.syncUpdate()
>
> def rejectFromQueue(self, logger=None, dry_run=False):
> """See `IPackageUpload`."""
> @@ -496,6 +487,7 @@
> else:
> return None
>
> + @property
> def mySourcePackageRelease(self):
> """The source package release related to this queue item.

As a Property, this should be named my_source_package_release

> === modified file 'lib/lp/soyuz/tests/test_packageupload.py'
> --- lib/lp/soyuz/tests/test_packageupload.py 2009-09-29 17:16:01 +0000
> +++ lib/lp/soyuz/tests/test_packageupload.py 2009-10-09 09:50:27 +0000
> @@ -198,6 +198,9 @@
> self.assertEquals(
> PackageUploadStatus.ACCEPTED, delayed_copy.status)
>
> + # Make sure no announcement email was sent at this point.
> + self.assertEquals(len(stub.test_emails), 0)
> +
> self.layer.txn.commit()
> self.layer.switchDbUser(self.dbuser)
>
> @@ -210,22 +213,26 @@
>
> # Check the announcement email.
> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
> - # This is now a MIMEMultipart message.
> msg = message_from_string(raw_msg)
> body = msg.get_payload(0)
> body = body.get_payload(decode=True)
>
> - self.assertEquals(from_addr, '<email address hidden>')
> self.assertEquals(
> - to_addrs, ['<email address hidden>'])
> + str(to_addrs), "['<email address hidden>']")
>
> expected_subject = (
> '[ubuntutest/breezy-autotest-security]\n\t'
> 'dist-upgrader_20060302.0120_all.tar.gz, foocomm 1.0-2 (Accepted)')
> self.assertEquals(msg['Subject'], expected_subject)
>
> - self.assertTrue(body.startswith('foocomm (1.0-2) breezy; urgency=low'))
> -
> + self.assertEquals(body,
> + 'foocomm (1.0-2) breezy; urgency=low\n\n'
> + ' * Initial version\n\n'
> + 'Date: Thu, 16 Feb 2006 15:34:09 +0000\n'
> + 'Changed-By: Foo Bar <email address hidden>\n'
> + 'Maintainer: Launchpad team <email address hidden>\n'
> + 'http://launchpad.dev/ubuntutest/breezy-autotest/+source/foocomm/1.0-2\n')

This line is a bit too long. Could you split itto two lines? (OK,
breaking the URL into two line does ot look nice either...

> +

Please remove the whitespace in the line above.

> self.layer.switchDbUser('launchpad')
>
> # One source and 2 binaries are pending publication. They all were
>
> === modified file 'lib/lp/soyuz/tests/test_publishing.py'
> --- lib/lp/soyuz/tests/test_publishing.py 2009-09-10 08:56:15 +0000
> +++ lib/lp/soyuz/tests/test_publishing.py 2009-10-09 09:50:27 +0000
> @@ -385,6 +385,41 @@
> return [BinaryPackagePublishingHistory.get(pub.id)
> for pub in secure_pub_binaries]
>
> + def _findFile(self, top, name_fragment):

A name like _findChangesFile would be a better, I think. The method
does not find any file having name_fragment in its name ;)

> + """File with given name fragment in directory tree starting at top."""
> + for root, dirs, files in os.walk(top, topdown=False):
> + for name in files:
> + if name.endswith('.changes') and name.find(name_fragment) > -1:
> + return os.path.join(root, name)
> + return None
> +
> + def createSource(
> + self, archive, sourcename, version, distroseries=None,
> + new_version=None):
> + """Create source with meaningful '.changes' file."""
> + top = 'lib/lp/archiveuploader/tests/data/suite'
> + name_fragment = '%s_%s' % (sourcename, version)
> + changesfile_path = self._findFile(top, name_fragment)
> +

Please remove the whitespace in the line above.

> + source = None
> +
> + if changesfile_path is not None:
> + if new_version is None:
> + new_version = version
> + changesfile_content = ''
> + handle = open(changesfile_path, 'r')
> + try:
> + changesfile_content = handle.read()
> + finally:
> + handle.close()
> +

Please remove the whitespace in the line above.

> + source = self.getPubSource(
> + sourcename=sourcename, archive=archive, version=new_version,
> + changes_file_content=changesfile_content,
> + distroseries=distroseries)
> +

Please remove the whitespace in the line above.

> + return source
> +
>
> class TestNativePublishingBase(unittest.TestCase, SoyuzTestPublisher):
> layer = LaunchpadZopelessLayer
>
>