Merge lp:~jcsackett/launchpad/bugtasks-deacitvated-context-632847 into lp:launchpad

[j.c.sackett]

[r=gmb][bug=632847] Adds check to prevent bugtasks for deactivated products from showing up for people who can't see the product.

Summary
=======

Per bug 632847, bugtasks for deactivated projects can show up in search results. Users clicking on links for these bugtasks then get a 404. The best fix for this is to make sure that users that can't verify the deactivated project (i.e. aren't registry admins) don't see those bugtasks.

Preimplementation
=================

Spoke with Deryck Hodge and Curtis Hovey

Proposed Fix
============

Add a step in the query building for bug search that checks is the searching user is in registry admins; if they're not, filter out bugtasks for deactivated products.

Implementation
==============

lib/lp/bugs/browser/tests/test_buglisting.py
--------------------------------------------
Tests added.

lib/lp/bugs/model/bugtask.py
----------------------------
A check is added to see if the searching user is in registry admins; if they're not, an extra clause is added to the query to remove products that are deactivated.

lib/lp/registry/browser/tests/test_person_view.py
-------------------------------------------------
Drive by clean up of whitespace while trying to decide where to place new tests.

Tests
=====

bin/test -t test_buglisting

Demo & QA
=========

Using the link provided in the bug (though switch to qastaging). If you are signed in and are a member of registry admins, you should see the bugtask reported as problematic in the bugtask. If you are not in registry admins (or logout), you should not see it.

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_buglisting.py
  lib/lp/bugs/model/bugtask.py
  lib/lp/registry/browser/tests/test_person_view.py

[Graham Binns (gmb)]

Just one comment: test_listing_seen_with_permission needs a comment at the start of it stating the expected behaviour (it's obvious from the test, but I don't want to have to parse the test to understand what it's testing).

Other than that this is great; r=me.

review: Approve (code)

[Robert Collins (lifeless)]

Uhm
this is a correlated subquery; generally this about the worst performing thing we can do: it repeats the same work hundreds of thousands of times.

Worse, we're adding a lookup to product for every bug found, even if we're search in a context where this is impossible and irrelevant - e.g. distro or product searches.

I'm going to roll this back out today to avoid blocking deploys : we have massive issues with bug search already.

review: Needs Fixing

=== modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
--- lib/lp/bugs/browser/tests/test_buglisting.py	2011-02-08 05:54:20 +0000
+++ lib/lp/bugs/browser/tests/test_buglisting.py	2011-02-10 17:40:33 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -21,15 +21,60 @@
 from lp.testing import (
     BrowserTestCase,
     login_person,
+    login,
+    logout,
     person_logged_in,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
-
 from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
 
 
+class DeactivatedContextBugTaskTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(DeactivatedContextBugTaskTestCase, self).setUp()
+        self.person = self.factory.makePerson()
+        self.active_product = self.factory.makeProduct()
+        self.inactive_product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=self.active_product)
+        self.active_bugtask = self.factory.makeBugTask(
+            bug=bug,
+            target=self.active_product)
+        self.inactive_bugtask = self.factory.makeBugTask(
+            bug=bug,
+            target=self.inactive_product)
+        with person_logged_in(self.person):
+            self.active_bugtask.transitionToAssignee(self.person)
+            self.inactive_bugtask.transitionToAssignee(self.person)
+        login('[e-mail]')
+        self.inactive_product.active = False
+        logout()
+
+    def test_listing_not_seen_without_permission(self):
+        # Someone without permission to see deactiveated projects does
+        # not see bugtasks for deactivated projects.
+        login('[e-mail]')
+        view = create_initialized_view(self.person, "+bugs")
+        self.assertEqual([self.active_bugtask], list(view.searchUnbatched()))
+
+    def test_listing_seen_with_permission(self):
+        # Someone with permission to see deactiveated projects
+        # can see bugtasks for deactivated projects.
+        login('[e-mail]')
+        registry_owner = getUtility(
+            ILaunchpadCelebrities).registry_experts.teamowner
+        logout()
+        login_person(registry_owner)
+        view = create_initialized_view(self.person, "+bugs")
+        self.assertEqual(
+            sorted([self.active_bugtask, self.inactive_bugtask]),
+            sorted(list(view.searchUnbatched())))
+
+
 class TestBugTaskSearchListingPage(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-02-08 22:12:39 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-02-10 17:40:33 +0000
@@ -78,6 +78,9 @@
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
+from canonical.launchpad.interfaces.launchpad import (
+    IPersonRoles,
+    )
 from canonical.launchpad.helpers import shortlist
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import IStore
@@ -1775,6 +1778,15 @@
         if params.status is not None:
             extra_clauses.append(self._buildStatusClause(params.status))
 
+        if params.user is not None:
+            user_roles = IPersonRoles(params.user)
+            if not user_roles.in_registry_experts:
+                extra_clauses.append(
+                    """NOT EXISTS (SELECT TRUE
+                                FROM Product
+                                WHERE Product.id = BugTask.product
+                                AND Product.active = False)""")
+
         if params.milestone:
             if IProjectGroupMilestone.providedBy(params.milestone):
                 where_cond = """

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2011-02-08 11:13:57 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2011-02-10 17:40:33 +0000
@@ -177,10 +177,11 @@
         self.add_bug(bugtask_count)
         login_person(self.owner)
         view = create_initialized_view(milestone, '+index')
-        # Eliminate permission check for the admin team from the
-        # recorded queries by loading it now. If the test ever breaks,
-        # the person fixing it won't waste time trying to track this
+        # Eliminate permission checks for the admin team and registry_experts
+        # from the recorded queries by loading it now. If the test ever
+        # breaks, the person fixing it won't waste time trying to track them
         # query down.
+        getUtility(ILaunchpadCelebrities).registry_experts
         getUtility(ILaunchpadCelebrities).admin
         with StormStatementRecorder() as recorder:
             bugtasks = list(view.bugtasks)
@@ -244,7 +245,7 @@
     def test_milestone_eager_loading(self):
         # Verify that the number of queries does not increase with more
         # bugs with different assignees.
-        query_limit = 34
+        query_limit = 37
         self.add_bug(3)
         self.assert_milestone_page_query_count(
             self.milestone, query_limit=query_limit)
@@ -260,7 +261,10 @@
         # is very large already, if the test fails due to such a change,
         # please cut some more of the existing fat out of it rather than
         # increasing the cap.
-        page_query_limit = 34
+
+        # Fetch some things into the cache so we don't muck with query
+        # counts.
+        page_query_limit = 37
         product = self.factory.makeProduct()
         login_person(product.owner)
         milestone = self.factory.makeMilestone(
@@ -424,12 +428,12 @@
         #  7. Load links to branches.
         bugtask_count = 10
         self.assert_bugtasks_query_count(
-            self.milestone, bugtask_count, query_limit=9)
+            self.milestone, bugtask_count, query_limit=11)
 
     def test_milestone_eager_loading(self):
         # Verify that the number of queries does not increase with more
         # bugs with different assignees.
-        query_limit = 33
+        query_limit = 36
         self.add_bug(3)
         self.assert_milestone_page_query_count(
             self.milestone, query_limit=query_limit)

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2011-01-14 21:20:46 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2011-02-10 17:40:33 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -7,7 +7,6 @@
 from storm.expr import LeftJoin
 from storm.store import Store
 from testtools.matchers import (
-    Equals,
     LessThan,
     )
 from zope.component import getUtility
@@ -28,16 +27,14 @@
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
-
 from lp.app.errors import NotFoundError
 from lp.bugs.model.bugtask import BugTask
 from lp.buildmaster.enums import BuildStatus
 from lp.registry.browser.person import (
     PersonEditView,
     PersonView,
-    TeamInvitationView)
-
-
+    TeamInvitationView,
+    )
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.registry.interfaces.person import (
     PersonVisibility,
@@ -47,7 +44,6 @@
     ITeamMembershipSet,
     TeamMembershipStatus,
     )
-
 from lp.registry.model.karma import KarmaCategory
 from lp.registry.model.milestone import milestone_sort_key
 from lp.soyuz.enums import (