launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bugs-in-deactivated-pillars into lp:launchpad.

Requested reviews:
  Robert Collins (lifeless)
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #632847 Bug page OOPS when viewed in deactivated project context
  https://bugs.launchpad.net/bugs/632847

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/bugs-in-deactivated-pillars/+merge/50000

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

-- 
https://code.launchpad.net/~jcsackett/launchpad/bugs-in-deactivated-pillars/+merge/50000
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/bugs-in-deactivated-pillars into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
--- lib/lp/bugs/browser/tests/test_buglisting.py	2011-02-13 23:02:49 +0000
+++ lib/lp/bugs/browser/tests/test_buglisting.py	2011-02-16 16:17:44 +0000
@@ -20,7 +20,9 @@
 from lp.registry.model.person import Person
 from lp.testing import (
     BrowserTestCase,
+    login,
     login_person,
+    logout,
     person_logged_in,
     StormStatementRecorder,
     TestCaseWithFactory,
@@ -30,6 +32,37 @@
 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('admin@xxxxxxxxxxxxx')
+        self.inactive_product.active = False
+        logout()
+
+    def test_deactivated_listings_not_seen(self):
+        # Someone without permission to see deactiveated projects does
+        # not see bugtasks for deactivated projects.
+        login('no-priv@xxxxxxxxxxxxx')
+        view = create_initialized_view(self.person, "+bugs")
+        self.assertEqual([self.active_bugtask], list(view.searchUnbatched()))
+
+
 class TestBugTaskSearchListingPage(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-02-15 03:27:27 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-02-16 16:17:44 +0000
@@ -1781,6 +1781,26 @@
             if where_cond is not None:
                 extra_clauses.append("BugTask.%s %s" % (arg_name, where_cond))
 
+        # 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)")
+            join_tables.append(
+                (Product, LeftJoin(Product, BugTask.productID == Product.id)))
+
+
         if params.status is not None:
             extra_clauses.append(self._buildStatusClause(params.status))
 
@@ -2214,7 +2234,7 @@
                 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'))
@@ -2458,10 +2478,11 @@
         else:
             requested_joins = kwargs.get('prejoins', [])
             # NB: We could save later work by predicting what sort of targets
-            # we might be interested in here, but as at any point we're dealing
-            # with relatively few results, this is likely to be a small win.
+            # we might be interested in here, but as at any point we're
+            # dealing with relatively few results, this is likely to be a
+            # small win.
             prejoins = [
-                (Bug, Join(Bug, BugTask.bug == Bug.id))
+                (Bug, Join(Bug, BugTask.bug == Bug.id)),
                 ] + requested_joins
             def eager_load(results):
                 product_ids = set([row[0].productID for row in results])