Merge lp:~edwin-grubbs/launchpad/bug-655802-person-vocabulary-timeout into lp:launchpad

[Edwin Grubbs]

Summary
-------

Fixes soft timeouts when the AJAX Picker searches the
ValidPersonOrTeamVocabulary or its subclasses.

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

Rewrote query to remove private_inner_select subquery, which sped up the
query. Don't check the Account table, since the person record is
considered broken if the account status is active, and it doesn't have a
preferred email address. The EmailAddress is actually queried twice.
First, to search all VALIDATED and PREFERRED email addresses for certain
text. Secondly, to verify that any non-team has a PREFERRED email.
    lib/lp/registry/vocabularies.py

Added test to verify that the extra_clause is getting applied to the
query in _doSearch() when that method is passed in a search term.
Previously, the the picker would allow you to select a private team
as a member of itself. Later, it would fail because the __contains__()
method constructs a different query without a search term.
    lib/lp/registry/tests/test_person_vocabularies.py

Tests
-----

./bin/test -vv -t test_person_vocabularies

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

* Open http://launchpad.dev/~launchpad-beta-testers
  * Click on the "Add member" and search for "John Arbash Meinel".
  * It should not timeout.

[Curtis Hovey (sinzui)]

Thanks for working on this. I think this is good to land, but I have a one question/remark. I do not understand why the test is on the LaunchpadFunctionalLayer? I see database objects, but no library objects in the test. I think the test will run on the DatabaseFunctionalLayer.

review: Approve (code)

=== added file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py	2010-10-20 15:48:52 +0000
@@ -0,0 +1,46 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the person vocabularies."""
+
+__metaclass__ = type
+
+from storm.store import Store
+from zope.schema.vocabulary import getVocabularyRegistry
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.ftests import login_person
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import PersonVisibility
+from lp.testing import TestCaseWithFactory
+
+
+class TestValidTeamMemberVocabulary(TestCaseWithFactory):
+    """Test that the ValidTeamMemberVocabulary behaves as expected."""
+
+    layer = DatabaseFunctionalLayer
+
+    def searchVocabulary(self, team, text):
+        vocabulary_registry = getVocabularyRegistry()
+        naked_vocabulary = removeSecurityProxy(
+            vocabulary_registry.get(team, 'ValidTeamMember'))
+        return naked_vocabulary._doSearch(text)
+
+    def test_public_team_cannot_be_a_member_of_itself(self):
+        # A public team should be filtered by the vocab.extra_clause
+        # when provided a search term.
+        team_owner = self.factory.makePerson()
+        login_person(team_owner)
+        team = self.factory.makeTeam(owner=team_owner)
+        Store.of(team).flush()
+        self.assertFalse(team in self.searchVocabulary(team, team.name))
+
+    def test_private_team_cannot_be_a_member_of_itself(self):
+        # A private team should be filtered by the vocab.extra_clause
+        # when provided a search term.
+        team_owner = self.factory.makePerson()
+        login_person(team_owner)
+        team = self.factory.makeTeam(
+            owner=team_owner, visibility=PersonVisibility.PRIVATE)
+        Store.of(team).flush()
+        self.assertFalse(team in self.searchVocabulary(team, team.name))

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-09-27 20:47:58 +0000
+++ lib/lp/registry/vocabularies.py	2010-10-20 15:48:52 +0000
@@ -94,11 +94,9 @@
     SQLBase,
     sqlvalues,
     )
-from canonical.launchpad.database.account import Account
 from canonical.launchpad.database.emailaddress import EmailAddress
 from canonical.launchpad.database.stormsugar import StartsWith
 from canonical.launchpad.helpers import shortlist
-from canonical.launchpad.interfaces.account import AccountStatus
 from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import IStore
@@ -109,7 +107,6 @@
     IStoreSelector,
     MAIN_STORE,
     )
-from lp.app.browser.tales import DateTimeFormatterAPI
 from canonical.launchpad.webapp.vocabulary import (
     BatchedCountableIterator,
     CountableIterator,
@@ -118,6 +115,7 @@
     NamedSQLObjectVocabulary,
     SQLObjectVocabularyBase,
     )
+from lp.app.browser.tales import DateTimeFormatterAPI
 from lp.blueprints.interfaces.specification import ISpecification
 from lp.bugs.interfaces.bugtask import (
     IBugTask,
@@ -524,16 +522,11 @@
             # TeamParticipation table, which is very expensive.  The search
             # for private teams does need that table but the number of private
             # teams is very small so the cost is not great.
-            valid_email_statuses = (
-                EmailAddressStatus.VALIDATED,
-                EmailAddressStatus.PREFERRED,
-                )
 
             # First search for public persons and teams that match the text.
             public_tables = [
                 Person,
                 LeftJoin(EmailAddress, EmailAddress.person == Person.id),
-                LeftJoin(Account, EmailAddress.account == Account.id),
                 ]
 
             # Create an inner query that will match public persons and teams
@@ -565,10 +558,15 @@
                     FROM Person, EmailAddress
                     WHERE EmailAddress.person = Person.id
                         AND lower(email) LIKE ? || '%%'
+                        AND EmailAddress.status IN (?, ?)
                     ) AS public_subquery
                 ORDER BY rank DESC
                 LIMIT ?
-                """, (text, text, text, text, text, self.LIMIT))
+                """, (text, text, text, text, text,
+                      EmailAddressStatus.VALIDATED.value,
+                      EmailAddressStatus.PREFERRED.value,
+                      self.LIMIT))
+
 
             public_result = self.store.using(*public_tables).find(
                 Person,
@@ -579,12 +577,9 @@
                     Or(# A valid person-or-team is either a team...
                        # Note: 'Not' due to Bug 244768.
                        Not(Person.teamowner == None),
-
-                       # Or a person who has an active account and a working
-                       # email address.
-                       And(Account.status == AccountStatus.ACTIVE,
-                           EmailAddress.status.is_in(valid_email_statuses))),
-                    self.extra_clause))
+                       # Or a person who has a preferred email address.
+                       EmailAddress.status == EmailAddressStatus.PREFERRED),
+                    ))
             # The public query doesn't need to be ordered as it will be done
             # at the end.
             public_result.order_by()
@@ -596,33 +591,28 @@
             # Searching for private teams that match can be easier since we
             # are only interested in teams.  Teams can have email addresses
             # but we're electing to ignore them here.
-            private_inner_select = SQL("""
-                SELECT Person.id
-                FROM Person
-                WHERE Person.fti @@ ftq(?)
-                LIMIT ?
-                """, (text, self.LIMIT))
             private_result = self.store.using(*private_tables).find(
                 Person,
                 And(
-                    Person.id.is_in(private_inner_select),
+                    SQL('Person.fti @@ ftq(?)', [text]),
                     private_query,
                     )
                 )
 
-            # The private query doesn't need to be ordered as it will be done
-            # at the end, and we need to eliminate the default ordering.
-            private_result.order_by()
+            private_result.order_by(SQL('rank(fti, ftq(?)) DESC', [text]))
+            private_result.config(limit=self.LIMIT)
 
             combined_result = public_result.union(private_result)
             # Eliminate default ordering.
             combined_result.order_by()
             # XXX: BradCrittenden 2009-04-26 bug=217644: The use of Alias and
-            # is a work-around for .count() not working with the 'distinct'
-            # option.
+            # _get_select() is a work-around for .count() not working
+            # with the 'distinct' option.
             subselect = Alias(combined_result._get_select(), 'Person')
             exact_match = (Person.name == text)
-            result = self.store.using(subselect).find((Person, exact_match))
+            result = self.store.using(subselect).find(
+                (Person, exact_match),
+                self.extra_clause)
         # XXX: BradCrittenden 2009-05-07 bug=373228: A bug in Storm prevents
         # setting the 'distinct' and 'limit' options in a single call to
         # .config().  The work-around is to split them up.  Note the limit has
@@ -755,8 +745,8 @@
             Person.id NOT IN (
                 SELECT team FROM TeamParticipation
                 WHERE person = %d
-                ) AND Person.id != %d
-            """ % (self.team.id, self.team.id)
+                )
+            """ % self.team.id
 
 
 class ValidTeamOwnerVocabulary(ValidPersonOrTeamVocabulary):