Code review comment for lp:~abentley/launchpad/build-mail3

Aaron--

This looks pretty good. I have one commenting suggestion below which isn't terribly important, and one potentially important performance question at the bottom of the diff below.

> === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
> --- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-01-27 16:13:58 +0000
> +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-01-27 16:13:59 +0000
> @@ -2012,11 +2022,23 @@
> BuildUploadHandler(self.uploadprocessor, self.incoming_folder,
> leaf_name).process()
> self.layer.txn.commit()
> + return build
> +
> + def testSourcePackageRecipeBuild_fail(self):
> + build = self.doFailureRecipeBuild()
> self.assertEquals(BuildStatus.FAILEDTOUPLOAD, build.status)
> self.assertEquals(None, build.builder)
> self.assertIsNot(None, build.duration)
> self.assertIsNot(None, build.upload_log)
>
> + def testSourcePackageRecipeBuild_fail_mail(self):
> + self.doFailureRecipeBuild()
> + (mail,) = pop_notifications()
> + subject = mail['Subject'].replace('\n\t', ' ')
> + self.assertIn('Failed to upload', subject)
> + body = mail.get_payload(decode=True)
> + self.assertIn('Upload Log: http', body)
> +
> def testBuildWithInvalidStatus(self):
> # Builds with an invalid (non-UPLOADING) status should trigger
> # a warning but be left alone.

A comment to the effect
+ # When a recipe fails it should include the log in the mail.
in testSourcePackageRecipeBuild_fail_mail would be good.

> @@ -192,9 +191,23 @@
> # PackageUploadSource objects which are related.
> sources = SQLMultipleJoin('PackageUploadSource',
> joinColumn='packageupload')
> + # Does not include source builds.
> builds = SQLMultipleJoin('PackageUploadBuild',
> joinColumn='packageupload')
>
> + def getSourceBuild(self):
> + #avoid circular import
> + from lp.code.model.sourcepackagerecipebuild import (
> + SourcePackageRecipeBuild)
> + from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
> + return Store.of(self).find(
> + SourcePackageRecipeBuild,
> + SourcePackageRecipeBuild.id ==
> + SourcePackageRelease.source_package_recipe_build_id,
> + SourcePackageRelease.id ==
> + PackageUploadSource.sourcepackagereleaseID,
> + PackageUploadSource.packageupload == self.id).one()
> +
> # Also the custom files associated with the build.
> customfiles = SQLMultipleJoin('PackageUploadCustom',
> joinColumn='packageupload')
> @@ -488,6 +501,10 @@
> """See `IPackageUpload`."""
> return self.builds
>
> + @cachedproperty
> + def from_build(self):
> + return bool(self.builds) or self.getSourceBuild()
> +

Is there any chance of a cold cache use of this method causing performance issues? I'm not saying it will, just asking. I have no idea how large the query fired for this could be.