Hi Curtis,

This branch makes a nice coherent chunk. My suggestions are fairly minor.

merge-conditional

-Edwin


>=== modified file 'lib/lp/registry/doc/distroseries.txt'
>--- lib/lp/registry/doc/distroseries.txt	2009-12-10 20:28:03 +0000
>+++ lib/lp/registry/doc/distroseries.txt	2010-01-20 16:13:26 +0000
>@@ -500,7 +500,35 @@
> netapplet are translatable in Hoary.
> 
> 
>-== DistroSeries can build meta objects for packages ==
>+Packages that need linking and packagings that need upstream information
>+-----------------------------------------------------------------------
>+
>+A distroseries a getPriorizedUnlinkedSourcePackages() method that returns


The beginning of this sentence is confusing.


>+a prioritized list of `ISourcePackage` that need a packaging link to an


Since `ISourcePackage` is not pluralized, it feels odd when I get
to the verb "need" which implies that the noun should be plural.


>+`IProductSeries` to provide the the upstream information to share bugs,
>+translations, and code.
>+
>+    >>> for source_package in hoary.getPriorizedUnlinkedSourcePackages():
>+    ...     print source_package.name
>+    pmount
>+    alsa-utils
>+    cnews
>+    libstdc++
>+    linux-source-2.6.15
>+
>+
>+A distroseries a getPriorizedlPackagings() method that returns a prioritized


Beginning of this sentence also.


>+list of `IPackaging` that need more information about the upstream project to
>+share bugs, translations, and code.
>+
>+    >>> for packaging in hoary.getPriorizedlPackagings():
>+    ...     print packaging.sourcepackagename.name
>+    netapplet
>+    evolution
>+
>+
>+DistroSeries can build meta objects for packages
>+------------------------------------------------
> 
>    >>> from canonical.launchpad.interfaces import (
>    ...     ISourcePackage,
>
>=== modified file 'lib/lp/registry/model/distroseries.py'
>--- lib/lp/registry/model/distroseries.py	2009-12-22 17:41:46 +0000
>+++ lib/lp/registry/model/distroseries.py	2010-01-20 16:13:26 +0000
>@@ -48,6 +48,7 @@
>     get_bug_tags, get_bug_tags_open_count)
> from lp.bugs.model.bugtarget import BugTargetBase
> from lp.bugs.model.bugtask import BugTask
>+from lp.bugs.interfaces.bugtask import UNRESOLVED_BUGTASK_STATUSES
> from lp.soyuz.model.component import Component
> from lp.soyuz.model.distroarchseries import (
>     DistroArchSeries, DistroArchSeriesSet, PocketChroot)
>@@ -306,7 +307,7 @@
>         """See `IDistroSeries`."""
>         # Avoid circular import failures.
>         # We join to SourcePackageName, ProductSeries, and Product to cache
>-        # the objects that are implcitly needed to work with a
>+        # the objects that are implicitly needed to work with a
>         # Packaging object.
>         from lp.registry.model.product import Product
>         from lp.registry.model.productseries import ProductSeries
>@@ -329,6 +330,118 @@
>             packaging
>             for (packaging, spn, product_series, product) in results]
> 
>+    def getPriorizedUnlinkedSourcePackages(self):
>+        """See `IDistroSeries`.
>+
>+        The prioritization is a heuristic rule using bug hotness,
>+        translatable messages, and the source package release's component.
>+        """
>+        find_spec = (
>+            SourcePackageName,
>+            SQL("""
>+                coalesce(heat, 0) + coalesce(po_messages, 0) +
>+                CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END AS score"""))
>+        joins, conditions = self._current_sourcepackage_joins_and_conditions
>+        origin = SQL(joins)
>+        condition = SQL(conditions + "AND packaging.id IS NULL")
>+        results = IStore(self).using(origin).find(find_spec, condition)
>+        results = results.order_by('score DESC')
>+        return [SourcePackage(sourcepackagename=spn, distroseries=self)
>+                for (spn, score) in results]
>+
>+    def getPriorizedlPackagings(self):
>+        """See `IDistroSeries`.
>+
>+        The prioritization is a heuristic rule using  the branch, bug hotness,


s/using  the/using the/


>+        translatable messages, and the source package release's component.
>+        """
>+        # Avoid circular import failures.
>+        # We join to SourcePackageName, ProductSeries, and Product to cache
>+        # the objects that are implcitly needed to work with a
>+        # Packaging object.
>+        from lp.registry.model.product import Product
>+        from lp.registry.model.productseries import ProductSeries


It seems like it would be clearer to move these imports directly
below the comment line about "Avoid circular import failures."


>+        find_spec = (
>+            Packaging, SourcePackageName, ProductSeries, Product,
>+            SQL("""
>+                CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END +
>+                    CASE WHEN Product.bugtracker IS NULL
>+                        THEN coalesce(heat, 10) ELSE 0 END +
>+                    CASE WHEN ProductSeries.translations_autoimport_mode = 1
>+                        THEN coalesce(po_messages, 10) ELSE 0 END +
>+                    CASE WHEN ProductSeries.branch IS NULL THEN 500
>+                        ELSE 0 END AS score"""))
>+        joins, conditions = self._current_sourcepackage_joins_and_conditions
>+        origin = SQL(joins + """
>+            JOIN ProductSeries
>+                ON Packaging.productseries = ProductSeries.id
>+            JOIN Product
>+                ON ProductSeries.product = Product.id
>+            """)
>+        condition = SQL(conditions + "AND packaging.id IS NOT NULL")
>+        results = IStore(self).using(origin).find(find_spec, condition)
>+        results = results.order_by('score DESC')
>+        return [packaging
>+                for (packaging, spn, series, product, score) in results]
>+
>+    @property
>+    def _current_sourcepackage_joins_and_conditions(self):
>+        """The SQL joins and conditions to prioritise source packages."""
>+        heat_score = ("""
>+            LEFT JOIN (
>+                SELECT
>+                    BugTask.sourcepackagename,
>+                    sum(Bug.hotness) AS heat


It's not obvious elsewhere what the difference is between heat and
hotness. Can you rename it either hotness_sum, heat_sum, or total_heat?



>+                FROM BugTask
>+                    JOIN Bug
>+                        ON bugtask.bug = Bug.id
>+                WHERE
>+                    BugTask.sourcepackagename is not NULL
>+                    AND BugTask.distribution = %s
>+                    AND BugTask.status in %s
>+                GROUP BY BugTask.sourcepackagename
>+                ) bugs
>+                ON SourcePackageName.id = bugs.sourcepackagename
>+            """ % sqlvalues(self.distribution, UNRESOLVED_BUGTASK_STATUSES))
>+        message_score = ("""
>+            LEFT JOIN (
>+                SELECT
>+                    POTemplate.sourcepackagename,
>+                    POTemplate.distroseries,
>+                    SUM(POTemplate.messagecount) / 2 AS po_messages
>+                FROM POTemplate
>+                WHERE
>+                    POTemplate.sourcepackagename is not NULL
>+                    AND POTemplate.distroseries = %s
>+                GROUP BY
>+                    POTemplate.sourcepackagename,
>+                    POTemplate.distroseries
>+                ) messages
>+                ON SourcePackageName.id = messages.sourcepackagename
>+                AND DistroSeries.id = messages.distroseries
>+            """ % sqlvalues(self))
>+        joins = ("""
>+            SourcePackageName
>+            JOIN SourcePackageRelease spr
>+                ON SourcePackageName.id = spr.sourcepackagename
>+            JOIN SourcePackagePublishingHistory spph
>+                ON spr.id = spph.sourcepackagerelease
>+            JOIN archive
>+                ON spph.archive = Archive.id
>+            JOIN DistroSeries
>+                ON spph.distroseries = DistroSeries.id
>+            LEFT JOIN Packaging
>+                ON SourcePackageName.id = Packaging.sourcepackagename
>+                AND Packaging.distroseries = DistroSeries.id
>+            """ + heat_score + message_score)


I'm wondering if this would be easier to read if they were
defined as a single string variable with keyword substitution using
sqlvalues(distro=self.distribution, distroseries=self, statuses=...)


>+        conditions = ("""
>+            DistroSeries.id = %s
>+            AND spph.status IN %s
>+            AND archive.purpose = %s
>+            """ % sqlvalues(
>+                self, active_publishing_status, ArchivePurpose.PRIMARY))
>+        return (joins, conditions)
>+
>     @property
>     def supported(self):
>         return self.status in [
>
>=== modified file 'lib/lp/registry/tests/test_distroseries.py'
>--- lib/lp/registry/tests/test_distroseries.py	2010-01-07 06:45:03 +0000
>+++ lib/lp/registry/tests/test_distroseries.py	2010-01-20 16:13:26 +0000
>@@ -10,18 +10,23 @@
> import transaction
> 
> from zope.component import getUtility
>+from zope.security.proxy import removeSecurityProxy
> 
>+from canonical.database.sqlbase import cursor
> from canonical.launchpad.ftests import ANONYMOUS, login
> from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
> from lp.registry.interfaces.distroseries import (
>     IDistroSeriesSet, NoSuchDistroSeries)
> from lp.registry.interfaces.pocket import PackagePublishingPocket
>+from lp.soyuz.interfaces.component import IComponentSet
> from lp.soyuz.interfaces.distroseriessourcepackagerelease import (
>     IDistroSeriesSourcePackageRelease)
> from lp.soyuz.interfaces.publishing import (
>     active_publishing_status, PackagePublishingStatus)
> from lp.testing import TestCase, TestCaseWithFactory
> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>+from lp.translations.interfaces.translations import (
>+    TranslationsBranchImportMode)
> from canonical.testing import (
>     DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
> 
>@@ -170,6 +175,118 @@
>         self.assertEqual(suite, distroseries.getSuite(pocket))
> 
> 
>+class TestDistroSeriesPackaging(TestCaseWithFactory):
>+
>+    layer = DatabaseFunctionalLayer
>+
>+    def setUp(self):
>+        super(TestDistroSeriesPackaging, self).setUp()
>+        self.series = self.factory.makeDistroRelease()
>+        self.user = self.series.distribution.owner
>+        login('<email address hidden>')
>+        component_set = getUtility(IComponentSet)
>+        self.packages = {}
>+        self.main_component = component_set['main']
>+        self.universe_component = component_set['universe']
>+        self.makeSeriesPackage('normal')
>+        self.makeSeriesPackage('translatable', messages=120)
>+        self.makeSeriesPackage('hot', hotness=100)
>+        self.makeSeriesPackage('hot-translatable', hotness=80, messages=60)
>+        self.makeSeriesPackage('main', is_main=True)
>+        self.makeSeriesPackage('linked')
>+        self.linkPackage('linked')
>+        transaction.commit()
>+        login(ANONYMOUS)
>+
>+    def makeSeriesPackage(self, name,
>+                          is_main=False, hotness=None, messages=None):
>+        # Make a published source package.
>+        if is_main:
>+            component = self.main_component
>+        else:
>+            component = self.universe_component
>+        sourcepackagename = self.factory.makeSourcePackageName(name)
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=sourcepackagename, distroseries=self.series,
>+            component=component)
>+        source_package = self.factory.makeSourcePackage(
>+            sourcepackagename=sourcepackagename, distroseries=self.series)
>+        if hotness is not None:
>+            bugtask = self.factory.makeBugTask(
>+                target=source_package, owner=self.user)
>+            # hotness is not exposed in the model yet.
>+            # bugtask.bug.hotness = hotness
>+            cur = cursor()
>+            cur.execute("UPDATE Bug SET hotness = %d WHERE id = %d" % (
>+                (hotness, bugtask.bug.id)))
>+        if messages is not None:
>+            template = self.factory.makePOTemplate(
>+                distroseries=self.series, sourcepackagename=sourcepackagename,
>+                owner=self.user)
>+            removeSecurityProxy(template).messagecount = messages
>+        self.packages[name] = source_package
>+
>+    def linkPackage(self, name):
>+        product_series = self.factory.makeProductSeries()
>+        product_series.setPackaging(
>+            self.series, self.packages[name].sourcepackagename, self.user)
>+        return product_series
>+
>+    def test_getPriorizedUnlinkedSourcePackages(self):
>+        # Verify the ordering of source packages that need linking.
>+        source_packages = self.series.getPriorizedUnlinkedSourcePackages()
>+        names = [package.name for package in source_packages]
>+        expected = [
>+            u'main', u'hot-translatable', u'hot', u'translatable', u'normal']
>+        self.assertEqual(expected, names)
>+
>+    def test_getPriorizedlPackagings(self):
>+        # Verify the ordering of packagings that need more upstream info.
>+        for name in ['main', 'hot-translatable', 'hot', 'translatable']:
>+            self.linkPackage(name)
>+        packagings = self.series.getPriorizedlPackagings()
>+        names = [packaging.sourcepackagename.name for packaging in packagings]
>+        expected = [
>+            u'main', u'hot-translatable', u'hot', u'translatable', u'linked']
>+        self.assertEqual(expected, names)
>+
>+    def test_getPriorizedlPackagings_bug_tracker(self):
>+        # Verify the ordering of packagings without and without a bug tracker.


Double withouts.


>+        self.linkPackage('hot')
>+        self.makeSeriesPackage('cold')
>+        product_series = self.linkPackage('cold')
>+        product_series.product.bugtraker = self.factory.makeBugTracker()
>+        packagings = self.series.getPriorizedlPackagings()
>+        names = [packaging.sourcepackagename.name for packaging in packagings]
>+        expected = [u'hot', u'linked', u'cold']
>+        self.assertEqual(expected, names)
>+
>+    def test_getPriorizedlPackagings_branch(self):
>+        # Verify the ordering of packagings without and without a branch.


Again.


>+        self.linkPackage('translatable')
>+        self.makeSeriesPackage('withbranch')
>+        product_series = self.linkPackage('withbranch')
>+        product_series.branch = self.factory.makeBranch()
>+        packagings = self.series.getPriorizedlPackagings()
>+        names = [packaging.sourcepackagename.name for packaging in packagings]
>+        expected = [u'translatable', u'linked', u'withbranch']
>+        self.assertEqual(expected, names)
>+
>+    def test_getPriorizedlPackagings_translation(self):
>+        # Verify the ordering of translatable packagings that are and are not
>+        # configured to import.
>+        self.linkPackage('translatable')
>+        self.makeSeriesPackage('importabletranslatable')
>+        product_series = self.linkPackage('importabletranslatable')
>+        product_series.branch = self.factory.makeBranch()
>+        product_series.translations_autoimport_mode = (
>+            TranslationsBranchImportMode.IMPORT_TEMPLATES)
>+        packagings = self.series.getPriorizedlPackagings()
>+        names = [packaging.sourcepackagename.name for packaging in packagings]
>+        expected = [u'translatable', u'linked', u'importabletranslatable']
>+        self.assertEqual(expected, names)
>+
>+
> class TestDistroSeriesSet(TestCaseWithFactory):
> 
>     layer = DatabaseFunctionalLayer
>@@ -203,8 +320,7 @@
>             translatables, self._ref_translatables(),
>             "A newly created distroseries should not be translatable but "
>             "translatables() returns %r instead of %r." % (
>-                translatables, self._ref_translatables())
>-            )
>+                translatables, self._ref_translatables()))
> 
>         new_sourcepackagename = self.factory.makeSourcePackageName()
>         new_potemplate = self.factory.makePOTemplate(
>@@ -217,8 +333,7 @@
>             "After assigning a PO template, a distroseries should be "
>             "translatable but translatables() returns %r instead of %r." % (
>                 translatables,
>-                self._ref_translatables(u"sampleseries"))
>-            )
>+                self._ref_translatables(u"sampleseries")))
> 
>         new_distroseries.hide_all_translations = True
>         transaction.commit()
>