Merge lp:~edwin-grubbs/launchpad/bug-682727-bug-count-timeout into lp:launchpad

[Edwin Grubbs]

[r=gmb][ui=none][bug=682727] Fix timeout on $project/+series page by simplifying bug count query.

Summary
-------

This branch fixes a timeout on the $project/+series page.

I simplified the permission query for bug counts on milestones, since
the query which determines which private bugs a user can view is costly,
and displaying a count including all private bugs doesn't reveal any
sensitive information.

Implementation details
----------------------

configs/testrunner/launchpad.conf
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/tests/test_bugtaskset.py
lib/lp/testing/factory.py

Tests
-----

./bin/test -vvp lp.bugs.tests.test_bugtaskset

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

* Open https://code.qastaging.launchpad.net/obsolete-junk/+series
  * It should not timeout.

Lint
----

No lint.

[Graham Binns (gmb)]

Hi Edwin,

Couple of minor nitpicks here, but otherwise this is r=me.

60 + def get_counts(self, user):
61 + counts = self.bugtask_set.getStatusCountsForProductSeries(
62 + user, self.series)
63 + return [(BugTaskStatus.items[status_id], count)
64 + for status_id, count in counts]

The indentation for these statements is a bit weird. I think that it
should look like this:

    counts = self.bugtask_set.getStatusCountsForProductSeries(
        user, self.series)
    return [
        (BugTaskStatus.items[status_id], count)
        for status_id, count in counts]

186 + [bugtask] = bug.bugtasks

I know that the context makes it unlikely that we'll have > 1 bugtask on
the bug at this point, but for safety's (and readability's) sake I'd
prefer this to be:

    bugtask = bug.default_bugtask

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-12-23 11:35:12 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-01-04 17:02:31 +0000
@@ -2485,9 +2485,13 @@
 
     def getStatusCountsForProductSeries(self, user, product_series):
         """See `IBugTaskSet`."""
-        bug_privacy_filter = get_bug_privacy_filter(user)
-        if bug_privacy_filter != "":
-            bug_privacy_filter = 'AND ' + bug_privacy_filter
+        if user is None:
+            bug_privacy_filter = 'AND Bug.private = FALSE'
+        else:
+            # Since the count won't reveal sensitive information, and
+            # since the get_bug_privacy_filter() check for non-admins is
+            # costly, don't filter those bugs at all.
+            bug_privacy_filter = ''
         cur = cursor()
 
         # The union is actually much faster than a LEFT JOIN with the

=== added file 'lib/lp/bugs/tests/test_bugtaskset.py'
--- lib/lp/bugs/tests/test_bugtaskset.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bugtaskset.py	2011-01-04 17:02:31 +0000
@@ -0,0 +1,91 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugTaskSet."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.interfaces.bugtask import (
+    BugTaskStatus,
+    IBugTaskSet,
+    )
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
+
+
+class TestStatusCountsForProductSeries(TestCaseWithFactory):
+    """Test BugTaskSet.getStatusCountsForProductSeries()."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestStatusCountsForProductSeries, self).setUp()
+        self.bugtask_set = getUtility(IBugTaskSet)
+        self.owner = self.factory.makePerson()
+        login_person(self.owner)
+        self.product = self.factory.makeProduct(owner=self.owner)
+        self.series = self.factory.makeProductSeries(product=self.product)
+        self.milestone = self.factory.makeMilestone(productseries=self.series)
+
+    def get_counts(self, user):
+        counts = self.bugtask_set.getStatusCountsForProductSeries(
+                user, self.series)
+        return [
+            (BugTaskStatus.items[status_id], count)
+            for status_id, count in counts]
+
+    def test_privacy_and_counts_for_unauthenticated_user(self):
+        # An unauthenticated user should see bug counts for each status
+        # that do not include private bugs.
+        self.factory.makeBug(milestone=self.milestone)
+        self.factory.makeBug(milestone=self.milestone, private=True)
+        self.factory.makeBug(series=self.series)
+        self.factory.makeBug(series=self.series, private=True)
+        self.assertEqual(
+            [(BugTaskStatus.NEW, 2)],
+            self.get_counts(None))
+
+    def test_privacy_and_counts_for_owner(self):
+        # The owner should see bug counts for each status that do
+        # include all private bugs.
+        self.factory.makeBug(milestone=self.milestone)
+        self.factory.makeBug(milestone=self.milestone, private=True)
+        self.factory.makeBug(series=self.series)
+        self.factory.makeBug(series=self.series, private=True)
+        self.assertEqual(
+            [(BugTaskStatus.NEW, 4)],
+            self.get_counts(self.owner))
+
+    def test_privacy_and_counts_for_other_user(self):
+        # A random authenticated user should see bug counts for each
+        # status that do include all private bugs, since it is costly to
+        # query just the private bugs that the user has access to view,
+        # and this query may be run many times on a single page.
+        self.factory.makeBug(milestone=self.milestone)
+        self.factory.makeBug(milestone=self.milestone, private=True)
+        self.factory.makeBug(series=self.series)
+        self.factory.makeBug(series=self.series, private=True)
+        other = self.factory.makePerson()
+        self.assertEqual(
+            [(BugTaskStatus.NEW, 4)],
+            self.get_counts(other))
+
+    def test_multiple_statuses(self):
+        # Test that separate counts are provided for each status that
+        # bugs are found in.
+        for status in (BugTaskStatus.INVALID, BugTaskStatus.OPINION):
+            self.factory.makeBug(milestone=self.milestone, status=status)
+            self.factory.makeBug(series=self.series, status=status)
+        for i in range(3):
+            self.factory.makeBug(series=self.series)
+        self.assertEqual(
+            [(BugTaskStatus.INVALID, 2),
+             (BugTaskStatus.OPINION, 2),
+             (BugTaskStatus.NEW, 3),
+            ],
+            self.get_counts(None))

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-12-24 10:03:15 +0000
+++ lib/lp/testing/factory.py	2011-01-04 17:02:31 +0000
@@ -7,7 +7,6 @@
 
 This module should not contain tests (but it should be tested).
 """
-from lp.code.model.recipebuild import RecipeBuildRecord
 
 __metaclass__ = type
 __all__ = [
@@ -161,6 +160,7 @@
     PreviewDiff,
     StaticDiff,
     )
+from lp.code.model.recipebuild import RecipeBuildRecord
 from lp.codehosting.codeimport.worker import CodeImportSourceDetails
 from lp.hardwaredb.interfaces.hwdb import (
     HWSubmissionFormat,
@@ -1371,26 +1371,41 @@
     def makeBug(self, product=None, owner=None, bug_watch_url=None,
                 private=False, date_closed=None, title=None,
                 date_created=None, description=None, comment=None,
-                status=None, distribution=None):
+                status=None, distribution=None, milestone=None, series=None):
         """Create and return a new, arbitrary Bug.
 
         The bug returned uses default values where possible. See
         `IBugSet.new` for more information.
 
         :param product: If the product is not set, and if the parameter
-            distribution is not set, a product is created and this is
-            used as the primary bug target.
+            distribution, milestone, and series are not set, a product
+            is created and this is used as the primary bug target.
         :param owner: The reporter of the bug. If not set, one is created.
         :param bug_watch_url: If specified, create a bug watch pointing
             to this URL.
         :param distribution: If set, the distribution is used as the
             default bug target.
+        :param milestone: If set, the milestone.target must match the product
+            or distribution parameters, or the those parameters must be None.
+        :param series: If set, the series.product must match the product
+            parameter, or the series.distribution must match the distribution
+            parameter, or the those parameters must be None.
 
         At least one of the parameters distribution and product must be
         None, otherwise, an assertion error will be raised.
         """
         if product is None and distribution is None:
-            product = self.makeProduct()
+            if milestone is not None:
+                # One of these will be None.
+                product = milestone.product
+                distribution = milestone.distribution
+            elif series is not None:
+                if IProductSeries.providedBy(series):
+                    product = series.product
+                else:
+                    distribution = series.distribution
+            else:
+                product = self.makeProduct()
         if owner is None:
             owner = self.makePerson()
         if title is None:
@@ -1407,10 +1422,16 @@
         if bug_watch_url is not None:
             # fromText() creates a bug watch associated with the bug.
             getUtility(IBugWatchSet).fromText(bug_watch_url, bug, owner)
+        bugtask = bug.default_bugtask
         if date_closed is not None:
-            [bugtask] = bug.bugtasks
             bugtask.transitionToStatus(
                 BugTaskStatus.FIXRELEASED, owner, when=date_closed)
+        if milestone is not None:
+            bugtask.transitionToMilestone(milestone, milestone.target.owner)
+        else:
+            task = bug.addTask(owner, series)
+            task.transitionToStatus(status, owner)
+
         return bug
 
     def makeBugTask(self, bug=None, target=None, owner=None):