Merge lp:~wgrant/launchpad/picker-sorting into lp:launchpad

[William Grant]

[r=jcsackett][bug=172702,255282,504676,722412,787578] Sort the project picker by relevance, and the person picker by affiliation.

The project and person pickers are hilariously bad at returning relevant matches. Until recently, the person picker found all even slightly relevant results and sorted them alphabetically. It's now slightly better, but the project picker still does it alphabetically. This branch hopefully fixes all these problems.

The person picker is now context-sensitive. If the context is within a pillar, people who have touched the project or distribution receive a rank bonus, boosting them towards the top of the results. For this purpose, "touched the project" is defined as having at least 10 karma in the relevant context. The scaling factor is the base-10 logarithm of the karma value, as that seems to work well on dogfood for both large and small projects, with wide varieties of contribution levels. That's all easily tweakable later, at any rate. Surprisingly, it doesn't seem to perform too badly either.

I also downscaled the hardcoded ranks for exact/prefix matches of some person attributes. Their ordering was not changed. The new affiliation bonus is behind its own feature flag, inside the existing new ranking algorithm which is behind another.

This has the downside of not working for teams, as they do not have karma. While there are some possible paths for investigation (eg. average karma of the members), teams names tend to be less ambiguous and more easily searchable than people, so it's probably OK as it is.

The project picker is a bit simpler. It adds a new, flagged ranking algorithm that is a bit less useless than displayname. An exact name match is first, followed by ranked FTI matches. This seems to work pretty well. Just like teams, I found that affiliation ranking was less necessary here, as project names tend to be far more unique.

[j.c.sackett (jcsackett)]

William--

This looks good to land.

[j.c.sackett (jcsackett)]

review: Approve

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-05-12 02:09:42 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-06-09 09:46:25 +0000
@@ -705,8 +705,8 @@
                         conf.bugtask_path,
                         "target_link",
                         bugtarget_content.get('id'),
-                        {"step_title": "Search products",
-                         "header": "Change product"});
+                        {"step_title": "Search projects",
+                         "header": "Change project"});
             }
         }
 

=== modified file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py	2010-12-02 19:45:50 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py	2011-06-09 09:46:25 +0000
@@ -6,16 +6,29 @@
 __metaclass__ = type
 
 from storm.store import Store
+from zope.component import getUtility
 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 canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.registry.interfaces.person import (
     PersonVisibility,
     TeamSubscriptionPolicy,
     )
+from lp.registry.interfaces.karma import IKarmaCacheManager
+from lp.services.features.testing import FeatureFixture
 from lp.testing import TestCaseWithFactory
+from lp.testing.dbuser import dbuser
+
+
+PERSON_AFFILIATION_RANK_FLAG = {
+    'disclosure.picker_enhancements.enabled': 'on',
+    'disclosure.person_affiliation_rank.enabled': 'on',
+    }
 
 
 class VocabularyTestBase:
@@ -30,10 +43,90 @@
         return self.vocabulary_registry.get(context, self.vocabulary_name)
 
     def searchVocabulary(self, context, text):
-        Store.of(context).flush()
+        if Store.of(context) is not None:
+            Store.of(context).flush()
         vocabulary = self.getVocabulary(context)
         return removeSecurityProxy(vocabulary)._doSearch(text)
 
+
+class TestValidPersonOrTeamVocabulary(VocabularyTestBase,
+                                      TestCaseWithFactory):
+    """Test that the ValidPersonOrTeamVocabulary behaves as expected.
+
+    Most tests are in lib/lp/registry/doc/vocabularies.txt.
+    """
+
+    layer = LaunchpadZopelessLayer
+    vocabulary_name = 'ValidPersonOrTeam'
+
+    def addKarma(self, person, value, product=None, distribution=None):
+        if product:
+            kwargs = dict(product_id=product.id)
+        elif distribution:
+            kwargs = dict(distribution_id=distribution.id)
+        with dbuser('karma'):
+            getUtility(IKarmaCacheManager).new(
+                value, person.id, None, **kwargs)
+
+    def test_people_with_karma_sort_higher(self):
+        self.useFixture(FeatureFixture(PERSON_AFFILIATION_RANK_FLAG))
+        exact_person = self.factory.makePerson(
+            name='fooix', displayname='Fooix Bar')
+        prefix_person = self.factory.makePerson(
+            name='fooix-bar', displayname='Fooix Bar')
+        contributor_person = self.factory.makePerson(
+            name='bar', displayname='Fooix Bar')
+        product = self.factory.makeProduct()
+
+        # Exact is better than prefix is better than FTI.
+        self.assertEqual(
+            [exact_person, prefix_person, contributor_person],
+            list(self.searchVocabulary(product, u'fooix')))
+
+        # But karma can bump people up, behind the exact match.
+        self.addKarma(contributor_person, 500, product=product)
+        self.assertEqual(
+            [exact_person, contributor_person, prefix_person],
+            list(self.searchVocabulary(product, u'fooix')))
+
+        self.addKarma(prefix_person, 500, product=product)
+        self.assertEqual(
+            [exact_person, prefix_person, contributor_person],
+            list(self.searchVocabulary(product, u'fooix')))
+
+    def assertKarmaContextConstraint(self, expected, context):
+        """Check that the karma context constraint works.
+
+        Confirms that the karma context constraint matches the expected
+        value, and that a search with it works.
+        """
+        if expected is not None:
+            expected = expected % context.id
+        self.assertEquals(
+            expected,
+            removeSecurityProxy(
+                self.getVocabulary(context))._karma_context_constraint)
+        with FeatureFixture(PERSON_AFFILIATION_RANK_FLAG):
+            self.searchVocabulary(context, 'foo')
+
+    def test_product_karma_context(self):
+        self.assertKarmaContextConstraint(
+            'product = %d', self.factory.makeProduct())
+
+    def test_project_karma_context(self):
+        self.assertKarmaContextConstraint(
+            'project = %d', self.factory.makeProject())
+
+    def test_distribution_karma_context(self):
+        self.assertKarmaContextConstraint(
+            'distribution = %d', self.factory.makeDistribution())
+
+    def test_root_karma_context(self):
+        self.assertKarmaContextConstraint(None, None)
+
+
+class TeamMemberVocabularyTestBase(VocabularyTestBase):
+
     def test_open_team_cannot_be_a_member_of_a_closed_team(self):
         context_team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.MODERATED)
@@ -88,7 +181,8 @@
             vocabulary.step_title)
 
 
-class TestValidTeamMemberVocabulary(VocabularyTestBase, TestCaseWithFactory):
+class TestValidTeamMemberVocabulary(TeamMemberVocabularyTestBase,
+                                    TestCaseWithFactory):
     """Test that the ValidTeamMemberVocabulary behaves as expected."""
 
     layer = DatabaseFunctionalLayer
@@ -109,7 +203,8 @@
         self.assertNotIn(team, self.searchVocabulary(team, team.name))
 
 
-class TestValidTeamOwnerVocabulary(VocabularyTestBase, TestCaseWithFactory):
+class TestValidTeamOwnerVocabulary(TeamMemberVocabularyTestBase,
+                                   TestCaseWithFactory):
     """Test that the ValidTeamOwnerVocabulary behaves as expected."""
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/registry/tests/test_product_vocabularies.py'
--- lib/lp/registry/tests/test_product_vocabularies.py	2010-12-22 20:44:25 +0000
+++ lib/lp/registry/tests/test_product_vocabularies.py	2011-06-09 09:46:25 +0000
@@ -7,12 +7,16 @@
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.registry.vocabularies import ProductVocabulary
+from lp.services.features.testing import FeatureFixture
 from lp.testing import (
     celebrity_logged_in,
     TestCaseWithFactory,
     )
 
 
+PICKER_ENHANCEMENTS_FLAG = {'disclosure.picker_enhancements.enabled': 'on'}
+
+
 class TestProductVocabulary(TestCaseWithFactory):
     """Test that the ProductVocabulary behaves as expected."""
     layer = DatabaseFunctionalLayer
@@ -56,6 +60,28 @@
         self.assertEqual(
             [a_product, z_product, self.product], list(result))
 
+    def test_order_by_relevance(self):
+        # When the flag is enabled, the most relevant result is first.
+        bar_product = self.factory.makeProduct(
+            name='foo-bar', displayname='Foo bar', summary='quux')
+        quux_product = self.factory.makeProduct(
+            name='foo-quux', displayname='Foo quux')
+        with FeatureFixture(PICKER_ENHANCEMENTS_FLAG):
+            result = self.vocabulary.search('quux')
+        self.assertEqual(
+            [quux_product, bar_product], list(result))
+
+    def test_exact_match_is_first(self):
+        # When the flag is enabled, an exact name match always wins.
+        the_quux_product = self.factory.makeProduct(
+            name='the-quux', displayname='The quux')
+        quux_product = self.factory.makeProduct(
+            name='quux', displayname='The quux')
+        with FeatureFixture(PICKER_ENHANCEMENTS_FLAG):
+            result = self.vocabulary.search('quux')
+        self.assertEqual(
+            [quux_product, the_quux_product], list(result))
+
     def test_inactive_products_are_excluded(self):
         # Inactive products are not in the vocabulary.
         with celebrity_logged_in('registry_experts'):

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-06-09 08:07:52 +0000
+++ lib/lp/registry/vocabularies.py	2011-06-09 09:46:25 +0000
@@ -115,6 +115,7 @@
     IStoreSelector,
     MAIN_STORE,
     )
+from canonical.launchpad.webapp.publisher import nearest
 from canonical.launchpad.webapp.vocabulary import (
     BatchedCountableIterator,
     CountableIterator,
@@ -153,7 +154,10 @@
     ITeam,
     PersonVisibility,
     )
-from lp.registry.interfaces.pillar import IPillarName
+from lp.registry.interfaces.pillar import (
+    IPillar,
+    IPillarName,
+    )
 from lp.registry.interfaces.product import (
     IProduct,
     IProductSet,
@@ -194,6 +198,7 @@
     _table = Person
 
     def __init__(self, context=None):
+        super(BasePersonVocabulary, self).__init__(context)
         self.enhanced_picker_enabled = bool(
             getFeatureFlag('disclosure.picker_enhancements.enabled'))
 
@@ -285,7 +290,14 @@
             fti_query = quote(query)
             sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
                     like_query, fti_query)
-            return self._table.select(sql, orderBy=self._orderBy)
+            if getFeatureFlag('disclosure.picker_enhancements.enabled'):
+                order_by = (
+                    '(CASE name WHEN %s THEN 1 '
+                    ' ELSE rank(fti, ftq(%s)) END) DESC, displayname, name'
+                    % (fti_query, fti_query))
+            else:
+                order_by = self._orderBy
+            return self._table.select(sql, orderBy=order_by, limit=100)
         return self.emptySelectResults()
 
 
@@ -489,6 +501,19 @@
         """The storm store."""
         return getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
 
+    @cachedproperty
+    def _karma_context_constraint(self):
+        context = nearest(self.context, IPillar)
+        if IProduct.providedBy(context):
+            karma_context_column = 'product'
+        elif IDistribution.providedBy(context):
+            karma_context_column = 'distribution'
+        elif IProjectGroup.providedBy(context):
+            karma_context_column = 'project'
+        else:
+            return None
+        return '%s = %d' % (karma_context_column, context.id)
+
     def _privateTeamQueryAndTables(self):
         """Return query tables for private teams.
 
@@ -722,8 +747,8 @@
                     SELECT Person.id,
                     (case
                         when person.name=? then 100
-                        when lower(person.name) like ? || '%%' then 75
-                        when lower(person.displayname) like ? || '%%' then 50
+                        when lower(person.name) like ? || '%%' then 5
+                        when lower(person.displayname) like ? || '%%' then 4
                         else rank(fti, ftq(?))
                     end) as rank
                     FROM Person
@@ -731,12 +756,12 @@
                     or lower(Person.displayname) LIKE ? || '%%'
                     or Person.fti @@ ftq(?)
                     UNION ALL
-                    SELECT Person.id, 25 AS rank
+                    SELECT Person.id, 3 AS rank
                     FROM Person, IrcID
                     WHERE Person.id = IrcID.person
                         AND IrcID.nickname = ?
                     UNION ALL
-                    SELECT Person.id, 10 AS rank
+                    SELECT Person.id, 2 AS rank
                     FROM Person, EmailAddress
                     WHERE Person.id = EmailAddress.person
                         AND LOWER(EmailAddress.email) LIKE ? || '%%'
@@ -753,8 +778,8 @@
                 private_ranking_sql = SQL("""
                     (case
                         when person.name=? then 100
-                        when lower(person.name) like ? || '%%' then 75
-                        when lower(person.displayname) like ? || '%%' then 50
+                        when lower(person.name) like ? || '%%' then 5
+                        when lower(person.displayname) like ? || '%%' then 3
                         else rank(fti, ftq(?))
                     end) as rank
                 """, (text, text, text, text))
@@ -815,8 +840,18 @@
                     self.extra_clause),
                 )
             # Better ranked matches go first.
-            result.order_by(
-                SQL("rank desc"), Person.displayname, Person.name)
+            if (getFeatureFlag('disclosure.person_affiliation_rank.enabled')
+                and self._karma_context_constraint):
+                rank_order = SQL("""
+                    rank * COALESCE(
+                        (SELECT LOG(karmavalue) FROM KarmaCache
+                         WHERE person = Person.id AND
+                            %s
+                            AND category IS NULL AND karmavalue > 10),
+                        1) DESC""" % self._karma_context_constraint)
+            else:
+                rank_order = SQL("rank DESC")
+            result.order_by(rank_order, Person.displayname, Person.name)
         result.config(limit=self.LIMIT)
 
         # We will be displaying the person's irc nick(s) and emails in the

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-06-06 13:54:07 +0000
+++ lib/lp/services/features/flags.py	2011-06-09 09:46:25 +0000
@@ -110,6 +110,10 @@
      'boolean',
      ('Enables the display of extra details in the person picker.'),
      ''),
+    ('disclosure.person_affiliation_rank.enabled',
+     'boolean',
+     ('Enables ranking by pillar affiliation in the person picker.'),
+     ''),
     ])
 
 # The set of all flag names that are documented.