Merge lp:~jcsackett/launchpad/bugs-in-deactivated-pillars into lp:launchpad

[j.c.sackett]

[r=lifeless][bug=632847] Removes deactivated product bugtasks from bug search listings.

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. So for those searches that can return arbitrary product related bugtasks, we strip out the deactivated products.

This is the second attempt at fixing this bug; the previous branch had to be rolled back as it introduced performance issues. (See https://code.launchpad.net/~jcsackett/launchpad/bugtasks-deacitvated-context-632847/+merge/48925)

Proposed
========

Remove the bugtasks with deactivated products when a search would return arbitrary product results (e.g. we're not searching for bugtasks on a particular product, distribution, &c).

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

Spoke with Deryck Hodge and Curtis Hovey on the intial branch.

Spoke with Robert Collins regarding performance issues in the previous branch that was rolled back.

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

A check in buildQuery in the bugtask search system determines if the search can return arbitrary products; if it can, it adds a Left Join to Product and a where clause to filter out bugtasks targeting a deactivated product.

Tests
=====

bin/test -t test_buglisting

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

Using the link provided in the bug (though switch to qastaging). You should no longer see the problematic bugtask in the search result, but should see the other bugtask for the bug in question.

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/bugs/model/bugtask.py
    2487: E301 expected 1 blank line, found 0

[Robert Collins (lifeless)]

In https://bugs.launchpad.net/launchpad/+bug/421901/comments/14 we had a more complex join - we enforce the active flag in the join, which reduces the data brought into intermediary tables. Was there a particular reason for eliding that aspect here?

[j.c.sackett (jcsackett)]

> In https://bugs.launchpad.net/launchpad/+bug/421901/comments/14 we had a more
> complex join - we enforce the active flag in the join, which reduces the data
> brought into intermediary tables. Was there a particular reason for eliding
> that aspect here?

As I mentioned on IRC, the only reason was distraction in assembling the python code. The proper join has been pushed up.

[Robert Collins (lifeless)]

You can write this in storm:
       extra_clauses.append(Or(BugTask.productID==None, Product.active))

review: Approve

[j.c.sackett (jcsackett)]

> You can write this in storm:
> extra_clauses.append(Or(BugTask.productID==None, Product.active))

Thanks, Robert.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-02-23 19:22:41 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-02-25 14:55:14 +0000
@@ -1923,7 +1923,9 @@
                 ssub_match_milestone)
 
             join_tables.append(
-                (Product, LeftJoin(Product, BugTask.productID == Product.id)))
+                (Product, LeftJoin(Product, And(
+                                BugTask.productID == Product.id,
+                                Product.active))))
             join_tables.append(
                 (StructuralSubscription,
                  Join(StructuralSubscription, join_clause)))
@@ -1932,6 +1934,28 @@
                 % sqlvalues(params.structural_subscriber))
             has_duplicate_results = True
 
+
+        # Remove bugtasks from deactivated products, if necessary.
+        # We don't have to do this if
+        # 1) We're searching on bugtasks for a specific product
+        # 2) We're searching on bugtasks for a specific productseries
+        # 3) We're searching on bugtasks for a distribution
+        # 4) We're searching for bugtasks for a distroseries
+        # because in those instances we don't have arbitrary products which
+        # may be deactivated showing up in our search.
+        if (params.product is None and
+            params.distribution is None and
+            params.productseries is None and
+            params.distroseries is None):
+            # Prevent circular import problems.
+            from lp.registry.model.product import Product
+            extra_clauses.append(
+                "(Bugtask.product IS NULL OR Product.active = TRUE)")
+            join_tables.append(
+                (Product, LeftJoin(Product, And(
+                                BugTask.productID == Product.id,
+                                Product.active))))
+
         if params.component:
             clauseTables += [SourcePackagePublishingHistory,
                              SourcePackageRelease]
@@ -2099,7 +2123,6 @@
         if not decorators:
             decorator = lambda x: x
         else:
-
             def decorator(obj):
                 for decor in decorators:
                     obj = decor(obj)
@@ -2236,7 +2259,8 @@
                 AND MessageChunk.fti @@ ftq(%s))""" % searchtext_quoted
         text_search_clauses = [
             "Bug.fti @@ ftq(%s)" % searchtext_quoted,
-            "BugTask.fti @@ ftq(%s)" % searchtext_quoted]
+            "BugTask.fti @@ ftq(%s)" % searchtext_quoted,
+            ]
         no_targetnamesearch = bool(features.getFeatureFlag(
             'malone.disable_targetnamesearch'))
         if not no_targetnamesearch:
@@ -2374,8 +2398,9 @@
         origin = [BugTask]
         already_joined = set(origin)
         for table, join in join_tables:
-            origin.append(join)
-            already_joined.add(table)
+            if table not in already_joined:
+                origin.append(join)
+                already_joined.add(table)
         for table, join in prejoin_tables:
             if table not in already_joined:
                 origin.append(join)
 
=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2011-02-17 03:47:28 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2011-02-25 14:55:14 +0000
@@ -14,8 +14,6 @@
 from storm.store import Store
 from testtools.matchers import (
     Equals,
-    LessThan,
-    Not,
     )
 from zope.component import getUtility
 
@@ -24,7 +22,10 @@
     any,
     greater_than,
     )
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from canonical.testing.layers import (
+    LaunchpadFunctionalLayer,
+    DatabaseFunctionalLayer,
+    )
 from lp.bugs.interfaces.bugattachment import BugAttachmentType
 from lp.bugs.interfaces.bugtask import (
     BugBlueprintSearch,
@@ -532,6 +533,41 @@
         self.assertSearchFinds(params, self.bugtasks[:1])
 
 
+class DeactivatedProductBugTaskTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(DeactivatedProductBugTaskTestCase, 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,
+            description="Monkeys are bad.")
+        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)
+        admin = getUtility(IPersonSet).getByEmail('[e-mail]')
+        with person_logged_in(admin):
+            self.inactive_product.active = False
+
+    def test_deactivated_listings_not_seen(self):
+        # Someone without permission to see deactiveated projects does
+        # not see bugtasks for deactivated projects.
+        nopriv = getUtility(IPersonSet).getByEmail('[e-mail]')
+        bugtask_set = getUtility(IBugTaskSet)
+        param = BugTaskSearchParams(user=None, fast_searchtext='Monkeys')
+        results = bugtask_set.search(param, _noprejoins=True)
+        self.assertEqual([self.active_bugtask], list(results))
+
+
 class ProductAndDistributionTests:
     """Tests which are useful for distributions and products."""