← Back to team overview

launchpad-reviewers team mailing list archive

lp:~edwin-grubbs/launchpad/bug-577492-needspackaging-duplicates into lp:launchpad/devel

 

Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-577492-needspackaging-duplicates into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #528648 restore needs_packaging for +portlet-package-summary
  https://bugs.launchpad.net/bugs/528648
  #577492 +needs-packaging lists duplicates
  https://bugs.launchpad.net/bugs/577492


Summary
-------

Sped up +needs-packaging page and the needs-packaging portlet on the
distroseries +index page by making getPrioritizedUnlinkedSourcePackages() 
return a result set, so that batching will result in a limited query.

Previously, the result set was turned into a list in order to format
each item as a dictionary. Therefore, it would load 13,000 source
packages, and then iterate just over the first batch.

I also fixed the name of getPrioritizedPackagings() and made it
return a result set instead of a list.

Tests
-----

./bin/test -vv -t 'test_distroseries|doc/distroseries.txt'

Demo and Q/A
------------

* Open https://staging.launchpad.net/ubuntu/maverick/+needs-packaging
  * It should not timeout.
* Open https://staging.launchpad.net/ubuntu/maverick/
  * The "Upstream packaging" portlet should say
    "Needs more information or linking to upstream" and list some source
    packages.

Lint
----

There are over 700 lint errors due to bad indentation in doctests, so I
will add those changes later, so that reviewers aren't scared away.
-- 
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-577492-needspackaging-duplicates/+merge/31112
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-577492-needspackaging-duplicates into lp:launchpad/devel.
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2010-05-24 22:04:19 +0000
+++ lib/lp/registry/browser/distroseries.py	2010-07-28 02:18:47 +0000
@@ -333,9 +333,7 @@
     @cachedproperty
     def needs_linking(self):
         """Return a list of 10 packages most in need of upstream linking.""" 
-        # XXX sinzui 2010-02-26 bug=528648: This method causes a timeout.
-        # return self.context.getPrioritizedUnlinkedSourcePackages()[:10]
-        return None
+        return self.context.getPrioritizedUnlinkedSourcePackages()[:10]
 
     milestone_can_release = False
 
@@ -481,7 +479,7 @@
     @cachedproperty
     def cached_packagings(self):
         """The batched upstream packaging links."""
-        packagings = self.context.getPrioritizedlPackagings()
+        packagings = self.context.getPrioritizedPackagings()
         navigator = BatchNavigator(packagings, self.request, size=20)
         navigator.setHeadings('packaging', 'packagings')
         return navigator

=== added file 'lib/lp/registry/browser/tests/test_distroseries_views.py'
--- lib/lp/registry/browser/tests/test_distroseries_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_distroseries_views.py	2010-07-28 02:18:47 +0000
@@ -0,0 +1,52 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+
+from storm.zope.interfaces import IResultSet
+
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.testing import LaunchpadZopelessLayer
+from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+
+
+class TestDistroSeriesNeedsPackagesView(TestCaseWithFactory):
+    """Test the distroseries +needs-packaging view."""
+
+    layer = LaunchpadZopelessLayer
+
+    def test_cached_unlinked_packages(self):
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
+        view = create_initialized_view(distroseries, '+needs-packaging')
+        naked_packages = removeSecurityProxy(view.cached_unlinked_packages)
+        self.assertTrue(
+            IResultSet.providedBy(
+                view.cached_unlinked_packages.currentBatch().list),
+            '%s should batch IResultSet so that slicing will limit the '
+            'query' % view.cached_unlinked_packages.currentBatch().list)
+
+
+class TestDistroSeriesView(TestCaseWithFactory):
+    """Test the distroseries +index view."""
+
+    layer = LaunchpadZopelessLayer
+
+    def test_needs_linking(self):
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
+        view = create_initialized_view(distroseries, '+index')
+        self.assertTrue(
+            IResultSet.providedBy(view.needs_linking),
+            '%s should implement IResultSet so that slicing will limit the '
+            'query' % view.needs_linking)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/doc/distroseries.txt'
--- lib/lp/registry/doc/distroseries.txt	2010-07-20 09:36:17 +0000
+++ lib/lp/registry/doc/distroseries.txt	2010-07-28 02:18:47 +0000
@@ -457,11 +457,11 @@
     linux-source-2.6.15  0  0
 
 
-The distroseries getPrioritizedlPackagings() method that returns a prioritized
+The distroseries getPrioritizedPackagings() method that returns a prioritized
 list of `IPackaging` that need more information about the upstream project to
 share bugs, translations, and code.
 
-    >>> for packaging in hoary.getPrioritizedlPackagings():
+    >>> for packaging in hoary.getPrioritizedPackagings():
     ...     print packaging.sourcepackagename.name
     netapplet
     evolution

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2010-06-08 15:58:04 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2010-07-28 02:18:47 +0000
@@ -414,7 +414,7 @@
         and total_messages (translatable messages).
         """
 
-    def getPrioritizedlPackagings():
+    def getPrioritizedPackagings():
         """Return a list of packagings that need more upstream information."""
 
     def getMostRecentlyLinkedPackagings():

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-07-15 15:01:18 +0000
+++ lib/lp/registry/model/distroseries.py	2010-07-28 02:18:47 +0000
@@ -19,7 +19,7 @@
     BoolCol, StringCol, ForeignKey, SQLMultipleJoin, IntCol,
     SQLObjectNotFound, SQLRelatedJoin)
 
-from storm.locals import And, Desc, Join, SQL
+from storm.locals import Desc, Join, SQL
 from storm.store import Store
 
 from zope.component import getUtility
@@ -335,14 +335,19 @@
         condition = SQL(conditions + "AND packaging.id IS NULL")
         results = IStore(self).using(origin).find(find_spec, condition)
         results = results.order_by('score DESC', SourcePackageName.name)
-        return [{
-                 'package': SourcePackage(
+        results = results.config(distinct=True)
+
+        def decorator(result):
+            spn, score, bug_count, total_messages = result
+            return {
+                'package': SourcePackage(
                     sourcepackagename=spn, distroseries=self),
-                 'bug_count': bug_count,
-                 'total_messages': total_messages}
-                for (spn, score, bug_count, total_messages) in results]
+                'bug_count': bug_count,
+                'total_messages': total_messages,
+                }
+        return DecoratedResultSet(results, decorator)
 
-    def getPrioritizedlPackagings(self):
+    def getPrioritizedPackagings(self):
         """See `IDistroSeries`.
 
         The prioritization is a heuristic rule using the branch, bug heat,
@@ -352,18 +357,6 @@
         # the objects that are implcitly needed to work with a
         # Packaging object.
         # Avoid circular import failures.
-        from lp.registry.model.product import Product
-        from lp.registry.model.productseries import ProductSeries
-        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(total_bug_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
@@ -372,10 +365,19 @@
                 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, SourcePackageName.name ASC')
-        return [packaging
-                for (packaging, spn, series, product, score) in results]
+        results = IStore(self).using(origin).find(Packaging, condition)
+        return results.order_by("""
+            (
+                CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END
+                + CASE WHEN Product.bugtracker IS NULL
+                    THEN coalesce(total_bug_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
+            ) DESC,
+            SourcePackageName.name ASC
+            """)
 
     @property
     def _current_sourcepackage_joins_and_conditions(self):

=== modified file 'lib/lp/registry/templates/distroseries-portlet-packaging.pt'
--- lib/lp/registry/templates/distroseries-portlet-packaging.pt	2010-05-27 04:10:17 +0000
+++ lib/lp/registry/templates/distroseries-portlet-packaging.pt	2010-07-28 02:18:47 +0000
@@ -56,7 +56,7 @@
     </dt>
     <dd tal:condition="view/needs_linking">
       <ul class="horizontal">
-          <li repeat="package view/needs_linking">
+          <li tal:repeat="package view/needs_linking">
             <a class="sprite package-source"
                tal:attributes="href package/package/fmt:url"
                tal:content="package/package/name">evolution</a>

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2010-07-20 17:50:45 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2010-07-28 02:18:47 +0000
@@ -271,17 +271,17 @@
             u'main', u'hot-translatable', u'hot', u'translatable', u'normal']
         self.assertEqual(expected, names)
 
-    def test_getPrioritizedlPackagings(self):
+    def test_getPrioritizedPackagings(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.getPrioritizedlPackagings()
+        packagings = self.series.getPrioritizedPackagings()
         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_getPrioritizedlPackagings_bug_tracker(self):
+    def test_getPrioritizedPackagings_bug_tracker(self):
         # Verify the ordering of packagings with and without a bug tracker.
         self.linkPackage('hot')
         self.makeSeriesPackage('cold')
@@ -289,12 +289,12 @@
         naked_product_series = remove_security_proxy_and_shout_at_engineer(
             product_series)
         naked_product_series.product.bugtraker = self.factory.makeBugTracker()
-        packagings = self.series.getPrioritizedlPackagings()
+        packagings = self.series.getPrioritizedPackagings()
         names = [packaging.sourcepackagename.name for packaging in packagings]
         expected = [u'hot', u'cold', u'linked']
         self.assertEqual(expected, names)
 
-    def test_getPrioritizedlPackagings_branch(self):
+    def test_getPrioritizedPackagings_branch(self):
         # Verify the ordering of packagings with and without a branch.
         self.linkPackage('translatable')
         self.makeSeriesPackage('withbranch')
@@ -302,12 +302,12 @@
         naked_product_series = remove_security_proxy_and_shout_at_engineer(
             product_series)
         naked_product_series.branch = self.factory.makeBranch()
-        packagings = self.series.getPrioritizedlPackagings()
+        packagings = self.series.getPrioritizedPackagings()
         names = [packaging.sourcepackagename.name for packaging in packagings]
         expected = [u'translatable', u'linked', u'withbranch']
         self.assertEqual(expected, names)
 
-    def test_getPrioritizedlPackagings_translation(self):
+    def test_getPrioritizedPackagings_translation(self):
         # Verify the ordering of translatable packagings that are and are not
         # configured to import.
         self.linkPackage('translatable')
@@ -318,7 +318,7 @@
         naked_product_series.branch = self.factory.makeBranch()
         naked_product_series.translations_autoimport_mode = (
             TranslationsBranchImportMode.IMPORT_TEMPLATES)
-        packagings = self.series.getPrioritizedlPackagings()
+        packagings = self.series.getPrioritizedPackagings()
         names = [packaging.sourcepackagename.name for packaging in packagings]
         expected = [u'translatable', u'linked', u'importabletranslatable']
         self.assertEqual(expected, names)