Code review comment for lp:~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part2

Hi Edwin,

This change looks great. I marked a couple of niggly bits but that's
all.

--bac

> === added file 'database/schema/patch-2207-56-0.sql'
> --- database/schema/patch-2207-56-0.sql 1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2207-56-0.sql 2010-06-04 18:48:29 +0000
> @@ -0,0 +1,24 @@
> +-- Copyright 2009 Canonical Ltd. This software is licensed under the

2010

(The contents of the patch are left for a db review.)

> === modified file 'lib/lp/bugs/tests/test_bugheat.py'
> --- lib/lp/bugs/tests/test_bugheat.py 2010-06-01 09:21:32 +0000
> +++ lib/lp/bugs/tests/test_bugheat.py 2010-06-04 18:48:29 +0000
> @@ -7,8 +7,11 @@

Edwin this test is really readable and easy to follow. Good job.

> import unittest
>
> +from storm.store import Store
> +
> from canonical.testing import LaunchpadZopelessLayer
>
> +from lp.testing import TestCaseWithFactory
> from lp.testing.factory import LaunchpadObjectFactory
>
>
> @@ -58,6 +61,85 @@
> def setUp(self):
> self.target = self.factory.makeDistributionSourcePackage()
>
> +class DistributionSourcePackageNullBugHeatCacheTest(
> + TestCaseWithFactory):
> + """Ensure distro source package cache values start at None."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + TestCaseWithFactory.setUp(self)
> + self.target = self.factory.makeDistributionSourcePackage()
> +
> + def test_null_max_bug_heat(self):
> + self.assertEqual(None, self.target.max_bug_heat)
> +
> + def test_null_total_bug_heat(self):
> + self.assertEqual(None, self.target.total_bug_heat)
> +
> + def test_null_bug_count(self):
> + self.assertEqual(None, self.target.bug_count)
> +
> +
> +class DistributionSourcePackageZeroRecalculateBugHeatCacheTest(
> + TestCaseWithFactory):
> + """Ensure distro source package cache values become zero properly."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + TestCaseWithFactory.setUp(self)
> + self.target = self.factory.makeDistributionSourcePackage()
> + self.target.recalculateBugHeatCache()
> +
> + def test_zero_max_bug_heat(self):
> + self.assertEqual(0, self.target.max_bug_heat)
> +
> + def test_zero_total_bug_heat(self):
> + self.assertEqual(0, self.target.total_bug_heat)
> +
> + def test_zero_bug_count(self):
> + self.assertEqual(0, self.target.bug_count)
> +
> +
> +class DistributionSourcePackageMultipleBugsRecalculateBugHeatCacheTest(
> + TestCaseWithFactory):
> + """Ensure distro source package cache values are set properly."""
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + TestCaseWithFactory.setUp(self)
> + self.target = self.factory.makeDistributionSourcePackage()
> + self.bugtask1 = self.factory.makeBugTask(target=self.target)
> + self.bugtask2 = self.factory.makeBugTask(target=self.target)
> + # Bug heat gets calculated by complicated rules in a db
> + # stored procedure. We will override them here to avoid
> + # testing inconsitencies if those values are calculated
> + # differently in the future.
> + # target.recalculateBugHeatCache() should be called
> + # automatically by bug.setHeat().
> + bug1 = self.bugtask1.bug
> + bug2 = self.bugtask2.bug
> + bug1.setHeat(7)
> + bug2.setHeat(19)
> + Store.of(bug1).flush()
> + self.max_heat = max(bug1.heat, bug2.heat)
> + self.total_heat = sum([bug1.heat, bug2.heat])
> +
> + def test_max_bug_heat(self):
> + self.assertEqual(self.max_heat, self.target.max_bug_heat)
> +
> + def test_total_bug_heat(self):
> + self.assertEqual(self.total_heat, self.target.total_bug_heat)
> + self.failUnless(
> + self.target.total_bug_heat > self.target.max_bug_heat,
> + "Total bug heat should be more than the max bug heat, "
> + "since we know that multiple bugs have nonzero heat.")
> +
> + def test_bug_count(self):
> + self.assertEqual(2, self.target.bug_count)
> +
>
> class SourcePackageMaxHeatByTargetTest(
> MaxHeatByTargetBase, unittest.TestCase):

> === modified file 'lib/lp/registry/interfaces/distributionsourcepackage.py'
> --- lib/lp/registry/interfaces/distributionsourcepackage.py 2010-02-12 13:39:56 +0000
> +++ lib/lp/registry/interfaces/distributionsourcepackage.py 2010-06-04 18:48:29 +0000
> @@ -78,6 +78,22 @@
> "no such package -- this occurs when there is no current series for "
> "the distribution.")
>
> + total_bug_heat = Attribute(
> + "Sum of the bug heat for all the bugs matching the distribution "
> + "and sourcepackagename of the IDistributionSourcePackage.")
> +
> + max_bug_heat = Attribute(
> + "Maximum bug heat for a single bug matching the distribution "
> + "and sourcepackagename of the IDistributionSourcePackage.")
> +
> + bug_count = Attribute(
> + "Number of bugs matching the distribution and sourcepackagename "
> + "of the IDistributionSourcePackage.")
> +
> + po_message_count = Attribute(
> + "Number of translations matching the distribution and "
> + "sourcepackagename of the IDistributionSourcePackage.")
> +
> def getReleasesAndPublishingHistory():
> """Return a list of all releases of this source package in this
> distribution and their correspodning publishing history.

Typo: corresponding (not yours)

> === modified file 'lib/lp/registry/model/distributionsourcepackage.py'
> --- lib/lp/registry/model/distributionsourcepackage.py 2010-03-19 11:13:00 +0000
> +++ lib/lp/registry/model/distributionsourcepackage.py 2010-06-04 18:48:29 +0000
> @@ -54,6 +59,40 @@
> CustomLanguageCode, HasCustomLanguageCodesMixin)
>
>
> +class DistributionSourcePackageProperty:
> + def __init__(self, attrname):
> + self.attrname = attrname
> +
> + def __get__(self, obj, class_):
> + return getattr(obj._self_in_database, self.attrname, None)
> +
> + def __set__(self, obj, value):
> + if obj._self_in_database is None:
> + # Log an oops without raising an error.
> + exception = AssertionError(
> + "DistributionSourcePackage record should have been created "
> + "earlier in the database for distro=%s, sourcepackagename=%s"
> + % (obj.distribution.name, obj.sourcepackagename.name))

Will this generate an OOPS report?

> + getUtility(IErrorReportingUtility).raising(
> + (exception.__class__, exception, None))
> + spph = Store.of(obj.distribution).find(
> + SourcePackagePublishingHistory,
> + SourcePackagePublishingHistory.distroseriesID ==
> + DistroSeries.id,
> + DistroSeries.distributionID == obj.distribution.id,
> + SourcePackagePublishingHistory.sourcepackagereleaseID ==
> + SourcePackageRelease.id,
> + SourcePackageRelease.sourcepackagenameID ==
> + obj.sourcepackagename.id
> + ).order_by(Desc(SourcePackagePublishingHistory.id)).first()
> + if spph is None:
> + section = getUtility(ISectionSet)['misc']
> + else:
> + section = spph.section
> + obj._new(obj.distribution, obj.sourcepackagename, section)
> + setattr(obj._self_in_database, self.attrname, value)
> +
> +
> class DistributionSourcePackage(BugTargetBase,
> SourcePackageQuestionTargetMixin,
> StructuralSubscriptionTargetMixin,
> @@ -72,6 +111,14 @@
> IDistributionSourcePackage, IHasBugHeat, IHasCustomLanguageCodes,
> IQuestionTarget)
>
> + bug_reporting_guidelines = DistributionSourcePackageProperty(
> + 'bug_reporting_guidelines')
> + max_bug_heat = DistributionSourcePackageProperty('max_bug_heat')
> + total_bug_heat = DistributionSourcePackageProperty('total_bug_heat')
> + bug_count = DistributionSourcePackageProperty('bug_count')
> + po_message_count = DistributionSourcePackageProperty('po_message_count')
> + section = DistributionSourcePackageProperty('section')
> +
> def __init__(self, distribution, sourcepackagename):
> self.distribution = distribution
> self.sourcepackagename = sourcepackagename