launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/answers-hide-inactive-projects into lp:launchpad.

Commit message:
Hide questions on inactive projects from the results of non-pillar-specific searches.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/answers-hide-inactive-projects/+merge/331981

This would have been about 9000 times easier if the question search infrastructure weren't an unholy mess of SQLObject and hand-written SQL, but such is life.  I first tried to do this in January 2016 but ran into incomprehensible test failures and gave up; the bit I'd missed back then was the fix to QuestionPersonSearch.getTableJoins.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/answers-hide-inactive-projects into lp:launchpad.

=== modified file 'lib/lp/answers/doc/questionsets.txt'
--- lib/lp/answers/doc/questionsets.txt	2015-07-07 12:46:21 +0000
+++ lib/lp/answers/doc/questionsets.txt	2017-10-07 02:46:42 +0000
@@ -175,9 +175,12 @@
     >>> from lp.registry.interfaces.product import IProductSet
     >>> firefox = getUtility(IProductSet).getByName('firefox')
     >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
+    >>> inactive = factory.makeProduct()
     >>> login('admin@xxxxxxxxxxxxx')
     >>> firefox.answers_usage = ServiceUsage.LAUNCHPAD
     >>> ubuntu.answers_usage = ServiceUsage.LAUNCHPAD
+    >>> inactive.answers_usage = ServiceUsage.LAUNCHPAD
+    >>> inactive.active = False
     >>> transaction.commit()
 
 This method can be used to retrieve the projects that are the most actively
@@ -207,6 +210,7 @@
     ...     ('ubuntu', 3),
     ...     ('firefox', 2),
     ...     ('landscape', 1),
+    ...     (inactive.name, 5),
     ...     ))
 
 A question is created just before the time limit on Launchpad.
@@ -232,7 +236,7 @@
     LAUNCHPAD
 
     # Launchpad is not returned because the question was not asked in
-    # the last 60 days.
+    # the last 60 days.  Inactive projects are not returned either.
     >>> for project in question_set.getMostActiveProjects():
     ...     print project.displayname
     Ubuntu

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2016-08-09 10:43:45 +0000
+++ lib/lp/answers/model/question.py	2017-10-07 02:46:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Question models."""
@@ -755,7 +755,7 @@
                     LEFT OUTER JOIN Distribution ON (
                         Question.distribution = Distribution.id)
                 WHERE
-                    (Product.answers_usage = %s
+                    ((Product.answers_usage = %s AND Product.active)
                     OR Distribution.answers_usage = %s)
                     AND Question.datecreated > (
                         current_timestamp -interval '60 days')
@@ -920,6 +920,10 @@
             constraints.append("""
                 Question.product = Product.id AND Product.active AND
                 Product.project = %s""" % sqlvalues(self.projectgroup))
+        else:
+            constraints.append("""
+                ((Question.product = Product.id AND Product.active) OR
+                 Question.product IS NULL)""")
 
         return constraints
 
@@ -929,6 +933,8 @@
             return self.getMessageJoins(self.needs_attention_from)
         elif self.projectgroup:
             return self.getProductJoins()
+        elif not self.product and not self.distribution:
+            return self.getActivePillarJoins()
         else:
             return []
 
@@ -941,6 +947,8 @@
                 AND QuestionMessage.owner = %s""" % sqlvalues(person))]
         if self.projectgroup:
             joins.extend(self.getProductJoins())
+        elif not self.product and not self.distribution:
+            joins.extend(self.getActivePillarJoins())
 
         return joins
 
@@ -950,6 +958,10 @@
         return [('JOIN Product '
                  'ON Question.product = Product.id')]
 
+    def getActivePillarJoins(self):
+        """Create the joins needed to select constraints on active pillars."""
+        return [('LEFT OUTER JOIN Product ON Question.product = Product.id')]
+
     def getConstraints(self):
         """Return a list of SQL constraints to use for this search."""
 
@@ -1176,8 +1188,7 @@
 
         if QuestionParticipation.COMMENTER in self.participation:
             message_joins = self.getMessageJoins(self.person)
-            if not set(joins).intersection(set(message_joins)):
-                joins.extend(message_joins)
+            joins.extend([join for join in message_joins if join not in joins])
 
         return joins
 

=== modified file 'lib/lp/answers/tests/test_question.py'
--- lib/lp/answers/tests/test_question.py	2016-08-23 03:49:28 +0000
+++ lib/lp/answers/tests/test_question.py	2017-10-07 02:46:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -8,6 +8,7 @@
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
+from lp.answers.interfaces.questioncollection import IQuestionSet
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.testing import (
     admin_logged_in,
@@ -64,3 +65,13 @@
         self.factory.makeQuestion(target=inactive)
         removeSecurityProxy(inactive).active = False
         self.assertContentEqual([question], group.searchQuestions())
+
+    def test_global_with_inactive_products_not_in_results(self):
+        product = self.factory.makeProduct()
+        inactive = self.factory.makeProduct()
+        active_question = self.factory.makeQuestion(target=product)
+        inactive_question = self.factory.makeQuestion(target=inactive)
+        removeSecurityProxy(inactive).active = False
+        questions = list(getUtility(IQuestionSet).searchQuestions())
+        self.assertIn(active_question, questions)
+        self.assertNotIn(inactive_question, questions)