Merge lp:~allenap/launchpad/subscribe-to-tag-bug-151129-5 into lp:launchpad

[Gavin Panella]

[r=edwin-grubbs][ui=none][bug=681326][incr] New class BugSubscriptionInfo which begins to encapsulate all subscription information about a bug.

New class BugSubscriptionInfo that aims to make the murky world of bug
subscriptions a little more understandable, and perform better.

This branch is quite long, but a lot of it is tweaks to tests to
adjust them to work with tuples instead of lists and stuff like
that. Along with another change, this branch can be mentally segmented
into three:

- Provide an in-app twin of the person_sort_key stored procedure (166
  lines of diff).

    lib/lp/registry/model/person.py
    lib/lp/registry/tests/test_person_sort_key.py

- New BugSubscriptionInfo class (728 lines of diff).

    lib/lp/bugs/model/bug.py
    lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py

  Only a couple of Bug.get*Subscri*s methods have been switched over
  to use the new BugSubscriptionInfo class for now.

- Test updates and a few minor code changes (192 lines of diff).

[Edwin Grubbs (edwin-grubbs)]

Hi Gavin,

This is an impressive undertaking. Unfortunately, I'm having difficulty understanding the intent that led to some of the design decisions.

The BugSubscriptionSet seems to be intended just to eager load the subscribers and their ValidPersonCache value, if the list of subscriptions is iterated over, but to not make that extra query any sooner than necessary. I'm wondering why you wouldn't just use the pre_iter_hook as used in Bug._indexed_messages().

Perhaps someone with more knowledge of the prerequisite branches would be better suited for reviewing this branch, but if not, I will be back at 15:00UTC, and I think talking on IRC will make this easier.

-Edwin

[Gavin Panella (allenap)]

Edwin, yes please, let's have a chat this afternoon. I could do with another opinion.

[Edwin Grubbs (edwin-grubbs)]

Hi Gavin,

Thanks for explaining the motives behind the BugSubscriptionSet. Since it is unlikely that you will need to batch the results, BugSubscriptionSet provides all the advantages of a DecoratedResultSet pre_iter_hook with a little less complexity and potentially more efficiency.

I have a few other comments below.

-Edwin

>=== modified file 'lib/lp/bugs/model/bug.py'
>--- lib/lp/bugs/model/bug.py 2010-11-05 09:16:14 +0000
>+++ lib/lp/bugs/model/bug.py 2010-12-08 21:12:34 +0000
>@@ -1954,6 +1936,193 @@
> operator.itemgetter(0))
>
>
>+def load_people(*where):
>+ """Get subscribers from subscriptions.
>+
>+ Also preloads `ValidPersonCache` records if they exist.
>+
>+ :param people: An iterable sequence of `Person` IDs.
>+ :return: A `DecoratedResultSet` of `Person` objects. The corresponding
>+ `ValidPersonCache` records are loaded simultaneously.
>+ """
>+ # Don't order the results; they will be used in set operations.
>+ join = LeftJoin(
>+ Person, ValidPersonCache, Person.id == ValidPersonCache.id)
>+ return imap(
>+ operator.itemgetter(0), IStore(Person).using(join).find(
>+ (Person, ValidPersonCache), *where).order_by())

I just landed a branch that adds functions just like load_people().
You can either use:
    getUtility(IPersonSet).getPrecachedPersonsFromIDs(
        ids, need_validity=True)
or if you want to specify the conditions:
    PersonSet()._getPrecachedPersons(
        joins, conditions, need_validity=True)

>+class BugSubscriberSet(frozenset):
>+
>+ @cachedproperty
>+ def sorted(self):
>+ return tuple(sorted(self, key=person_sort_key))
>+
>+
>+class BugSubscriptionSet(frozenset):
>+ @cachedproperty
>+ def sorted(self):
>+ self.subscribers # Pre-load subscribers.
>+ sort_key = lambda sub: person_sort_key(sub.person)
>+ return tuple(sorted(self, key=sort_key))
>+
>+ @cachedproperty
>+ def subscribers(self):
>+ if len(self) == 0:
>+ return BugSubscriberSet()
>+ else:
>+ condition = Person.id.is_in(
>+ removeSecurityProxy(subscription).person_id
>+ for subscription in self)
>+ return BugSubscriberSet(load_people(condition))
>+
>+
>+class StructuralSubscriptionSet(frozenset):
>+
>+ @cachedproperty
>+ def sorted(self):
>+ self.subscribers # Pre-load subscribers.
>+ sort_key = lambda sub: person_sort_key(sub.subscriber)
>+ return tuple(sorted(self, key=sort_key))
>+
>+ @cachedproperty
>+ def subscribers(self):
>+ if len(self) == 0:
>+ return BugSubscriberSet()
>+ else:
>+ condition = Person.id.is_in(
>+ removeSecurityProxy(subscription).subscriberID
>+ for subscription in self)
>+ return BugSubscriberSet(load_people(condition))

These three classes and their properties would benefit from docstrings.

>+
>+
>+# XXX; GavinPanella 2010-12-08: Subclasses of frozenset don't appear to be

If you are planning to remove this XXX before you land it, can you add a
note to BRANCH.TODO?

>+# granted those permissions given to froz...

review: Approve (code)

[Gavin Panella (allenap)]

Thanks for the review!

In summary I've:

- used PersonSet()._getPrecachedPersons() -- it's awesome :)

- added docstrings to the *Set classes.

- added a note about the XXX to BRANCH.TODO. Still not sure what I'm
  going to do about it.

- removed the XXX about the bug supervisor. The bug supervisor gets
  enough mail as it is.

- given test_person_sort_key a big overhaul, and your layer suggestion
  makes it run much faster, thanks.

If you're interested, there's a diff of the changes at:

  http://paste.ubuntu.com/546982/

Nothing controversial though.

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2010-12-01 11:26:57 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2010-12-24 15:20:32 +0000
@@ -15,7 +15,6 @@
 
 from lazr.delegates import delegates
 from simplejson import dumps
-
 from zope import formlib
 from zope.app.form import CustomWidgetFactory
 from zope.app.form.browser.itemswidgets import RadioWidget
@@ -490,7 +489,7 @@
         """
         direct_subscriptions = [
             SubscriptionAttrDecorator(subscription)
-            for subscription in self.context.getDirectSubscriptions()]
+            for subscription in self.context.getDirectSubscriptions().sorted]
         can_unsubscribe = []
         cannot_unsubscribe = []
         for subscription in direct_subscriptions:

=== modified file 'lib/lp/bugs/doc/bugzilla-import.txt'
--- lib/lp/bugs/doc/bugzilla-import.txt	2010-12-23 12:55:53 +0000
+++ lib/lp/bugs/doc/bugzilla-import.txt	2010-12-24 15:20:32 +0000
@@ -104,6 +104,7 @@
   ...     def getDuplicates(self):
   ...         return duplicates
 
+  >>> from itertools import chain
   >>> from zope.component import getUtility
   >>> from canonical.launchpad.ftests import login
   >>> from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
@@ -133,8 +134,9 @@
   ...         print 'Nick: %s' % bug.name
   ...     print 'Subscribers:'
   ...     subscriber_names = sorted(
-  ...         p.displayname for p in (bug.getDirectSubscribers() +
-  ...                                 bug.getIndirectSubscribers()))
+  ...         p.displayname for p in chain(
+  ...             bug.getDirectSubscribers(),
+  ...             bug.getIndirectSubscribers()))
   ...     for subscriber_name in subscriber_names:
   ...         print '    %s' % subscriber_name
   ...     for task in bug.bugtasks:

=== modified file 'lib/lp/bugs/doc/initial-bug-contacts.txt'
--- lib/lp/bugs/doc/initial-bug-contacts.txt	2010-12-23 12:55:53 +0000
+++ lib/lp/bugs/doc/initial-bug-contacts.txt	2010-12-24 15:20:32 +0000
@@ -98,9 +98,11 @@
 Foo Bar, a package subscriber to ubuntu mozilla-firefox and ubuntu
 pmount is currently not subscribed to bug 1.
 
+    >>> from itertools import chain
     >>> def subscriber_names(bug):
-    ...     subscribers = (
-    ...         bug.getDirectSubscribers() + bug.getIndirectSubscribers())
+    ...     subscribers = chain(
+    ...         bug.getDirectSubscribers(),
+    ...         bug.getIndirectSubscribers())
     ...     return sorted(subscriber.displayname for subscriber in subscribers)
 
     >>> subscriber_names(bug_one_in_ubuntu_firefox.bug)

=== modified file 'lib/lp/bugs/doc/security-teams.txt'
--- lib/lp/bugs/doc/security-teams.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/security-teams.txt	2010-12-24 15:20:32 +0000
@@ -4,6 +4,7 @@
 Responsibility for security-related bugs, are modelled in Launchpad with
 a "security contact" on a Distribution or a Product.
 
+    >>> from itertools import chain
     >>> from zope.component import getUtility
     >>> from lp.bugs.interfaces.securitycontact import IHasSecurityContact
     >>> from lp.registry.interfaces.distribution import IDistributionSet
@@ -60,8 +61,9 @@
 Shuttleworth are both subscribed to the bug.
 
     >>> def subscriber_names(bug):
-    ...     subscribers = (
-    ...         bug.getDirectSubscribers() + bug.getIndirectSubscribers())
+    ...     subscribers = chain(
+    ...         bug.getDirectSubscribers(),
+    ...         bug.getIndirectSubscribers())
     ...     return sorted(subscriber.name for subscriber in subscribers)
 
     >>> subscriber_names(bug)

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-12-23 19:12:19 +0000
+++ lib/lp/bugs/model/bug.py	2010-12-24 15:20:32 +0000
@@ -25,6 +25,7 @@
     timedelta,
     )
 from email.Utils import make_msgid
+from functools import wraps
 import operator
 import re
 
@@ -47,7 +48,6 @@
 from storm.expr import (
     And,
     Count,
-    Func,
     LeftJoin,
     Max,
     Not,
@@ -175,9 +175,13 @@
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.interfaces.structuralsubscription import (
+    IStructuralSubscriptionTarget,
+    )
 from lp.registry.model.person import (
     Person,
-    ValidPersonCache,
+    person_sort_key,
+    PersonSet,
     )
 from lp.registry.model.pillar import pillar_sort_key
 from lp.registry.model.teammembership import TeamParticipation
@@ -796,24 +800,8 @@
 
     def getDirectSubscriptions(self):
         """See `IBug`."""
-        # Cache valid persons so that <person>.is_valid_person can
-        # return from the cache. This operation was previously done at
-        # the same time as retrieving the bug subscriptions (as a left
-        # join). However, this ran slowly (far from optimal query
-        # plan), so we're doing it as two queries now.
-        valid_persons = Store.of(self).find(
-            (Person, ValidPersonCache),
-            Person.id == ValidPersonCache.id,
-            ValidPersonCache.id == BugSubscription.person_id,
-            BugSubscription.bug == self)
-        # Suck in all the records so that they're actually cached.
-        list(valid_persons)
-        # Do the main query.
-        return Store.of(self).find(
-            BugSubscription,
-            BugSubscription.person_id == Person.id,
-            BugSubscription.bug == self).order_by(
-            Func('person_sort_key', Person.displayname, Person.name))
+        return BugSubscriptionInfo(
+            self, BugNotificationLevel.NOTHING).direct_subscriptions
 
     def getDirectSubscribers(self, recipients=None, level=None):
         """See `IBug`.
@@ -825,18 +813,11 @@
         """
         if level is None:
             level = BugNotificationLevel.NOTHING
-
-        subscribers = list(
-            Person.select("""
-                Person.id = BugSubscription.person
-                AND BugSubscription.bug = %s
-                AND BugSubscription.bug_notification_level >= %s""" %
-                sqlvalues(self.id, level),
-                orderBy="displayname", clauseTables=["BugSubscription"]))
+        subscriptions = BugSubscriptionInfo(self, level).direct_subscriptions
         if recipients is not None:
-            for subscriber in subscribers:
+            for subscriber in subscriptions.subscribers:
                 recipients.addDirectSubscriber(subscriber)
-        return subscribers
+        return subscriptions.subscribers.sorted
 
     def getIndirectSubscribers(self, recipients=None, level=None):
         """See `IBug`.
@@ -1958,6 +1939,209 @@
             operator.itemgetter(0))
 
 
+def load_people(*where):
+    """Get subscribers from subscriptions.
+
+    Also preloads `ValidPersonCache` records if they exist.
+
+    :param people: An iterable sequence of `Person` IDs.
+    :return: A `DecoratedResultSet` of `Person` objects. The corresponding
+        `ValidPersonCache` records are loaded simultaneously.
+    """
+    return PersonSet()._getPrecachedPersons(
+        origin=[Person], conditions=where, need_validity=True)
+
+
+class BugSubscriberSet(frozenset):
+    """A set of bug subscribers
+
+    Every member should provide `IPerson`.
+    """
+
+    @cachedproperty
+    def sorted(self):
+        """A sorted tuple of this set's members.
+
+        Sorted with `person_sort_key`, the default sort key for `Person`.
+        """
+        return tuple(sorted(self, key=person_sort_key))
+
+
+class BugSubscriptionSet(frozenset):
+    """A set of bug subscriptions."""
+
+    @cachedproperty
+    def sorted(self):
+        """A sorted tuple of this set's members.
+
+        Sorted with `person_sort_key` of the subscription owner.
+        """
+        self.subscribers  # Pre-load subscribers.
+        sort_key = lambda sub: person_sort_key(sub.person)
+        return tuple(sorted(self, key=sort_key))
+
+    @cachedproperty
+    def subscribers(self):
+        """A `BugSubscriberSet` of the owners of this set's members."""
+        if len(self) == 0:
+            return BugSubscriberSet()
+        else:
+            condition = Person.id.is_in(
+                removeSecurityProxy(subscription).person_id
+                for subscription in self)
+            return BugSubscriberSet(load_people(condition))
+
+
+class StructuralSubscriptionSet(frozenset):
+    """A set of structural subscriptions."""
+
+    @cachedproperty
+    def sorted(self):
+        """A sorted tuple of this set's members.
+
+        Sorted with `person_sort_key` of the subscription owner.
+        """
+        self.subscribers  # Pre-load subscribers.
+        sort_key = lambda sub: person_sort_key(sub.subscriber)
+        return tuple(sorted(self, key=sort_key))
+
+    @cachedproperty
+    def subscribers(self):
+        """A `BugSubscriberSet` of the owners of this set's members."""
+        if len(self) == 0:
+            return BugSubscriberSet()
+        else:
+            condition = Person.id.is_in(
+                removeSecurityProxy(subscription).subscriberID
+                for subscription in self)
+            return BugSubscriberSet(load_people(condition))
+
+
+# XXX: GavinPanella 2010-12-08 bug=694057: Subclasses of frozenset don't
+# appear to be granted those permissions given to frozenset. This would make
+# writing ZCML tedious, so I've opted for registering custom checkers (see
+# lp_sitecustomize for some other jiggery pokery in the same vein) while I
+# seek a better solution.
+from zope.security import checker
+checker_for_frozen_set = checker.getCheckerForInstancesOf(frozenset)
+checker_for_subscriber_set = checker.NamesChecker(["sorted"])
+checker_for_subscription_set = checker.NamesChecker(["sorted", "subscribers"])
+checker.BasicTypes[BugSubscriberSet] = checker.MultiChecker(
+    (checker_for_frozen_set.get_permissions,
+     checker_for_subscriber_set.get_permissions))
+checker.BasicTypes[BugSubscriptionSet] = checker.MultiChecker(
+    (checker_for_frozen_set.get_permissions,
+     checker_for_subscription_set.get_permissions))
+checker.BasicTypes[StructuralSubscriptionSet] = checker.MultiChecker(
+    (checker_for_frozen_set.get_permissions,
+     checker_for_subscription_set.get_permissions))
+
+
+def freeze(factory):
+    """Return a decorator that wraps returned values with `factory`."""
+
+    def decorate(func):
+        """Decorator that wraps returned values."""
+
+        @wraps(func)
+        def wrapper(*args, **kwargs):
+            return factory(func(*args, **kwargs))
+        return wrapper
+
+    return decorate
+
+
+class BugSubscriptionInfo:
+    """Represents bug subscription sets."""
+
+    def __init__(self, bug, level):
+        self.bug = bug
+        self.level = level
+
+    @cachedproperty
+    @freeze(BugSubscriptionSet)
+    def direct_subscriptions(self):
+        """The bug's direct subscriptions."""
+        return IStore(BugSubscription).find(
+            BugSubscription,
+            BugSubscription.bug_notification_level >= self.level,
+            BugSubscription.bug == self.bug)
+
+    @cachedproperty
+    @freeze(BugSubscriptionSet)
+    def duplicate_subscriptions(self):
+        """Subscriptions to duplicates of the bug."""
+        if self.bug.private:
+            return ()
+        else:
+            return IStore(BugSubscription).find(
+                BugSubscription,
+                BugSubscription.bug_notification_level >= self.level,
+                BugSubscription.bug_id == Bug.id,
+                Bug.duplicateof == self.bug)
+
+    @cachedproperty
+    @freeze(StructuralSubscriptionSet)
+    def structural_subscriptions(self):
+        """Structural subscriptions to the bug's targets."""
+        query_arguments = []
+        for bugtask in self.bug.bugtasks:
+            if IStructuralSubscriptionTarget.providedBy(bugtask.target):
+                query_arguments.append((bugtask.target, bugtask))
+                if bugtask.target.parent_subscription_target is not None:
+                    query_arguments.append(
+                        (bugtask.target.parent_subscription_target, bugtask))
+            if ISourcePackage.providedBy(bugtask.target):
+                # Distribution series bug tasks with a package have the source
+                # package set as their target, so we add the distroseries
+                # explicitly to the set of subscription targets.
+                query_arguments.append((bugtask.distroseries, bugtask))
+            if bugtask.milestone is not None:
+                query_arguments.append((bugtask.milestone, bugtask))
+        # Build the query.
+        union = lambda left, right: (
+            removeSecurityProxy(left).union(
+                removeSecurityProxy(right)))
+        queries = (
+            target.getSubscriptionsForBugTask(bugtask, self.level)
+            for target, bugtask in query_arguments)
+        return reduce(union, queries)
+
+    @cachedproperty
+    @freeze(BugSubscriberSet)
+    def all_assignees(self):
+        """Assignees of the bug's tasks."""
+        assignees = Select(BugTask.assigneeID, BugTask.bug == self.bug)
+        return load_people(Person.id.is_in(assignees))
+
+    @cachedproperty
+    @freeze(BugSubscriberSet)
+    def all_pillar_owners_without_bug_supervisors(self):
+        """Owners of pillars for which no Bug supervisor is configured."""
+        for bugtask in self.bug.bugtasks:
+            pillar = bugtask.pillar
+            if pillar.bug_supervisor is None:
+                yield pillar.owner
+
+    @cachedproperty
+    def also_notified_subscribers(self):
+        """All subscribers except direct and dupe subscribers."""
+        if self.bug.private:
+            return BugSubscriberSet()
+        else:
+            return BugSubscriberSet().union(
+                self.structural_subscriptions.subscribers,
+                self.all_pillar_owners_without_bug_supervisors,
+                self.all_assignees).difference(
+                self.direct_subscriptions.subscribers)
+
+    @cachedproperty
+    def indirect_subscribers(self):
+        """All subscribers except direct subscribers."""
+        return self.also_notified_subscribers.union(
+            self.duplicate_subscriptions.subscribers)
+
+
 class BugSet:
     """See BugSet."""
     implements(IBugSet)

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-12-15 16:41:42 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-12-24 15:20:32 +0000
@@ -22,6 +22,7 @@
 
 
 import datetime
+from itertools import chain
 from operator import attrgetter
 
 from lazr.enum import DBItem
@@ -290,8 +291,9 @@
     @property
     def bug_subscribers(self):
         """See `IBugTask`."""
-        indirect_subscribers = self.bug.getIndirectSubscribers()
-        return self.bug.getDirectSubscribers() + indirect_subscribers
+        return tuple(
+            chain(self.bug.getDirectSubscribers(),
+                  self.bug.getIndirectSubscribers()))
 
     @property
     def bugtargetdisplayname(self):

=== added file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2010-12-24 15:20:32 +0000
@@ -0,0 +1,446 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `BugSubscriptionInfo`."""
+
+__metaclass__ = type
+
+from contextlib import contextmanager
+
+from storm.store import Store
+from testtools.matchers import Equals
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.model.bug import (
+    BugSubscriberSet,
+    BugSubscriptionInfo,
+    BugSubscriptionSet,
+    load_people,
+    StructuralSubscriptionSet,
+    )
+from lp.registry.enum import BugNotificationLevel
+from lp.registry.model.person import Person
+from lp.testing import (
+    person_logged_in,
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestLoadPeople(TestCaseWithFactory):
+    """Tests for `load_people`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test(self):
+        expected = [
+            self.factory.makePerson(),
+            self.factory.makeTeam(),
+            ]
+        observed = load_people(
+            Person.id.is_in(person.id for person in expected))
+        self.assertEqual(set(expected), set(observed))
+
+
+class TestSubscriptionRelatedSets(TestCaseWithFactory):
+    """Tests for *Set classes related to subscriptions."""
+
+    layer = DatabaseFunctionalLayer
+
+    name_pairs = ("A", "xa"), ("C", "xd"), ("B", "xb"), ("C", "xc")
+    name_pairs_sorted = ("A", "xa"), ("B", "xb"), ("C", "xc"), ("C", "xd")
+
+    def setUp(self):
+        super(TestSubscriptionRelatedSets, self).setUp()
+        make_person = lambda (displayname, name): (
+            self.factory.makePerson(displayname=displayname, name=name))
+        subscribers = dict(
+            (name_pair, make_person(name_pair))
+            for name_pair in self.name_pairs)
+        self.subscribers_set = frozenset(subscribers.itervalues())
+        self.subscribers_sorted = tuple(
+            subscribers[name_pair] for name_pair in self.name_pairs_sorted)
+
+    def test_BugSubscriberSet(self):
+        subscriber_set = BugSubscriberSet(self.subscribers_set)
+        self.assertIsInstance(subscriber_set, frozenset)
+        self.assertEqual(self.subscribers_set, subscriber_set)
+        self.assertEqual(self.subscribers_sorted, subscriber_set.sorted)
+
+    def test_BugSubscriptionSet(self):
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            subscriptions = frozenset(
+                bug.subscribe(subscriber, subscriber)
+                for subscriber in self.subscribers_set)
+        subscription_set = BugSubscriptionSet(subscriptions)
+        self.assertIsInstance(subscription_set, frozenset)
+        self.assertEqual(subscriptions, subscription_set)
+        # BugSubscriptionSet.sorted returns a tuple of subscriptions ordered
+        # by subscribers.
+        self.assertEqual(
+            self.subscribers_sorted, tuple(
+                subscription.person
+                for subscription in subscription_set.sorted))
+        # BugSubscriptionSet.subscribers returns a BugSubscriberSet of the
+        # subscription's subscribers.
+        self.assertIsInstance(subscription_set.subscribers, BugSubscriberSet)
+        self.assertEqual(self.subscribers_set, subscription_set.subscribers)
+
+    def test_StructuralSubscriptionSet(self):
+        product = self.factory.makeProduct()
+        with person_logged_in(product.owner):
+            subscriptions = frozenset(
+                product.addSubscription(subscriber, subscriber)
+                for subscriber in self.subscribers_set)
+        subscription_set = StructuralSubscriptionSet(subscriptions)
+        self.assertIsInstance(subscription_set, frozenset)
+        self.assertEqual(subscriptions, subscription_set)
+        # StructuralSubscriptionSet.sorted returns a tuple of subscriptions
+        # ordered by subscribers.
+        self.assertEqual(
+            self.subscribers_sorted, tuple(
+                subscription.subscriber
+                for subscription in subscription_set.sorted))
+        # StructuralSubscriptionSet.subscribers returns a BugSubscriberSet of
+        # the subscription's subscribers.
+        self.assertIsInstance(subscription_set.subscribers, BugSubscriberSet)
+        self.assertEqual(self.subscribers_set, subscription_set.subscribers)
+
+
+class TestBugSubscriptionInfo(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugSubscriptionInfo, self).setUp()
+        self.target = self.factory.makeProduct(
+            bug_supervisor=self.factory.makePerson())
+        self.bug = self.factory.makeBug(product=self.target)
+        # Unsubscribe the bug filer to make the tests more readable.
+        with person_logged_in(self.bug.owner):
+            self.bug.unsubscribe(self.bug.owner, self.bug.owner)
+
+    def getInfo(self):
+        return BugSubscriptionInfo(
+            self.bug, BugNotificationLevel.NOTHING)
+
+    def test_direct(self):
+        # The set of direct subscribers.
+        subscribers = (
+            self.factory.makePerson(),
+            self.factory.makePerson())
+        with person_logged_in(self.bug.owner):
+            subscriptions = tuple(
+                self.bug.subscribe(subscriber, subscriber)
+                for subscriber in subscribers)
+        found_subscriptions = self.getInfo().direct_subscriptions
+        self.assertEqual(set(subscriptions), found_subscriptions)
+        self.assertEqual(subscriptions, found_subscriptions.sorted)
+        self.assertEqual(set(subscribers), found_subscriptions.subscribers)
+        self.assertEqual(subscribers, found_subscriptions.subscribers.sorted)
+
+    def test_duplicate(self):
+        # The set of subscribers from duplicate bugs.
+        found_subscriptions = self.getInfo().duplicate_subscriptions
+        self.assertEqual(set(), found_subscriptions)
+        self.assertEqual((), found_subscriptions.sorted)
+        self.assertEqual(set(), found_subscriptions.subscribers)
+        self.assertEqual((), found_subscriptions.subscribers.sorted)
+        duplicate_bug = self.factory.makeBug(product=self.target)
+        with person_logged_in(duplicate_bug.owner):
+            duplicate_bug.markAsDuplicate(self.bug)
+            duplicate_bug_subscription = (
+                duplicate_bug.getSubscriptionForPerson(
+                    duplicate_bug.owner))
+        found_subscriptions = self.getInfo().duplicate_subscriptions
+        self.assertEqual(
+            set([duplicate_bug_subscription]),
+            found_subscriptions)
+        self.assertEqual(
+            (duplicate_bug_subscription,),
+            found_subscriptions.sorted)
+        self.assertEqual(
+            set([duplicate_bug.owner]),
+            found_subscriptions.subscribers)
+        self.assertEqual(
+            (duplicate_bug.owner,),
+            found_subscriptions.subscribers.sorted)
+
+    def test_duplicate_for_private_bug(self):
+        # The set of subscribers from duplicate bugs is always empty when the
+        # master bug is private.
+        duplicate_bug = self.factory.makeBug(product=self.target)
+        with person_logged_in(duplicate_bug.owner):
+            duplicate_bug.markAsDuplicate(self.bug)
+        with person_logged_in(self.bug.owner):
+            self.bug.setPrivate(True, self.bug.owner)
+        found_subscriptions = self.getInfo().duplicate_subscriptions
+        self.assertEqual(set(), found_subscriptions)
+        self.assertEqual((), found_subscriptions.sorted)
+        self.assertEqual(set(), found_subscriptions.subscribers)
+        self.assertEqual((), found_subscriptions.subscribers.sorted)
+
+    def test_structural(self):
+        # The set of structural subscribers.
+        subscribers = (
+            self.factory.makePerson(),
+            self.factory.makePerson())
+        with person_logged_in(self.bug.owner):
+            subscriptions = tuple(
+                self.target.addBugSubscription(subscriber, subscriber)
+                for subscriber in subscribers)
+        found_subscriptions = self.getInfo().structural_subscriptions
+        self.assertEqual(set(subscriptions), found_subscriptions)
+        self.assertEqual(subscriptions, found_subscriptions.sorted)
+        self.assertEqual(set(subscribers), found_subscriptions.subscribers)
+        self.assertEqual(subscribers, found_subscriptions.subscribers.sorted)
+
+    def test_all_assignees(self):
+        # The set of bugtask assignees for bugtasks that have been assigned.
+        found_assignees = self.getInfo().all_assignees
+        self.assertEqual(set(), found_assignees)
+        self.assertEqual((), found_assignees.sorted)
+        bugtask = self.bug.default_bugtask
+        with person_logged_in(bugtask.pillar.bug_supervisor):
+            bugtask.transitionToAssignee(self.bug.owner)
+        found_assignees = self.getInfo().all_assignees
+        self.assertEqual(set([self.bug.owner]), found_assignees)
+        self.assertEqual((self.bug.owner,), found_assignees.sorted)
+        bugtask2 = self.factory.makeBugTask(bug=self.bug)
+        with person_logged_in(bugtask2.pillar.owner):
+            bugtask2.transitionToAssignee(bugtask2.owner)
+        found_assignees = self.getInfo().all_assignees
+        self.assertEqual(
+            set([self.bug.owner, bugtask2.owner]),
+            found_assignees)
+        self.assertEqual(
+            (self.bug.owner, bugtask2.owner),
+            found_assignees.sorted)
+
+    def test_all_pillar_owners_without_bug_supervisors(self):
+        # The set of owners of pillars for which no bug supervisor is
+        # configured.
+        [bugtask] = self.bug.bugtasks
+        found_owners = (
+            self.getInfo().all_pillar_owners_without_bug_supervisors)
+        self.assertEqual(set(), found_owners)
+        self.assertEqual((), found_owners.sorted)
+        # Clear the supervisor for the first bugtask's target.
+        with person_logged_in(bugtask.target.owner):
+            bugtask.target.setBugSupervisor(None, bugtask.owner)
+        found_owners = (
+            self.getInfo().all_pillar_owners_without_bug_supervisors)
+        self.assertEqual(set([bugtask.pillar.owner]), found_owners)
+        self.assertEqual((bugtask.pillar.owner,), found_owners.sorted)
+        # Add another bugtask with a bug supervisor.
+        target2 = self.factory.makeProduct(bug_supervisor=None)
+        bugtask2 = self.factory.makeBugTask(bug=self.bug, target=target2)
+        found_owners = (
+            self.getInfo().all_pillar_owners_without_bug_supervisors)
+        self.assertEqual(
+            set([bugtask.pillar.owner, bugtask2.pillar.owner]),
+            found_owners)
+        self.assertEqual(
+            (bugtask.pillar.owner, bugtask2.pillar.owner),
+            found_owners.sorted)
+
+    def test_also_notified_subscribers(self):
+        # The set of also notified subscribers.
+        found_subscribers = self.getInfo().also_notified_subscribers
+        self.assertEqual(set(), found_subscribers)
+        self.assertEqual((), found_subscribers.sorted)
+        # Add an assignee, a bug supervisor and a structural subscriber.
+        bugtask = self.bug.default_bugtask
+        assignee = self.factory.makePerson()
+        with person_logged_in(bugtask.pillar.bug_supervisor):
+            bugtask.transitionToAssignee(assignee)
+        supervisor = self.factory.makePerson()
+        with person_logged_in(bugtask.target.owner):
+            bugtask.target.setBugSupervisor(supervisor, supervisor)
+        structural_subscriber = self.factory.makePerson()
+        with person_logged_in(structural_subscriber):
+            bugtask.target.addSubscription(
+                structural_subscriber, structural_subscriber)
+        # Add a direct subscription.
+        direct_subscriber = self.factory.makePerson()
+        with person_logged_in(self.bug.owner):
+            self.bug.subscribe(direct_subscriber, direct_subscriber)
+        # The direct subscriber does not appear in the also notified set, but
+        # the assignee, supervisor and structural subscriber do.
+        found_subscribers = self.getInfo().also_notified_subscribers
+        self.assertEqual(
+            set([assignee, supervisor, structural_subscriber]),
+            found_subscribers)
+        self.assertEqual(
+            (assignee, supervisor, structural_subscriber),
+            found_subscribers.sorted)
+
+    def test_also_notified_subscribers_for_private_bug(self):
+        # The set of also notified subscribers is always empty when the master
+        # bug is private.
+        assignee = self.factory.makePerson()
+        bugtask = self.bug.default_bugtask
+        with person_logged_in(bugtask.pillar.bug_supervisor):
+            bugtask.transitionToAssignee(assignee)
+        with person_logged_in(self.bug.owner):
+            self.bug.setPrivate(True, self.bug.owner)
+        found_subscribers = self.getInfo().also_notified_subscribers
+        self.assertEqual(set(), found_subscribers)
+        self.assertEqual((), found_subscribers.sorted)
+
+    def test_indirect_subscribers(self):
+        # The set of indirect subscribers is the union of also notified
+        # subscribers and subscribers to duplicates.
+        assignee = self.factory.makePerson()
+        bugtask = self.bug.default_bugtask
+        with person_logged_in(bugtask.pillar.bug_supervisor):
+            bugtask.transitionToAssignee(assignee)
+        duplicate_bug = self.factory.makeBug(product=self.target)
+        with person_logged_in(duplicate_bug.owner):
+            duplicate_bug.markAsDuplicate(self.bug)
+        found_subscribers = self.getInfo().indirect_subscribers
+        self.assertEqual(
+            set([assignee, duplicate_bug.owner]),
+            found_subscribers)
+        self.assertEqual(
+            (assignee, duplicate_bug.owner),
+            found_subscribers.sorted)
+
+
+class TestBugSubscriptionInfoQueries(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugSubscriptionInfoQueries, self).setUp()
+        self.target = self.factory.makeProduct()
+        self.bug = self.factory.makeBug(product=self.target)
+        self.info = BugSubscriptionInfo(
+            self.bug, BugNotificationLevel.NOTHING)
+        # Get the Storm cache into a known state.
+        self.store = Store.of(self.bug)
+        self.store.invalidate()
+        self.store.reload(self.bug)
+        self.bug.bugtasks
+        self.bug.tags
+
+    @contextmanager
+    def exactly_x_queries(self, count):
+        # Assert that there are exactly `count` queries sent to the database
+        # in this context. Flush first to ensure we don't count things that
+        # happened before entering this context.
+        self.store.flush()
+        condition = HasQueryCount(Equals(count))
+        with StormStatementRecorder() as recorder:
+            yield recorder
+        self.assertThat(recorder, condition)
+
+    def exercise_subscription_set(self, set_name):
+        # Looking up subscriptions takes a single query.
+        with self.exactly_x_queries(1):
+            getattr(self.info, set_name)
+        # Getting the subscribers results in one additional query.
+        with self.exactly_x_queries(1):
+            getattr(self.info, set_name).subscribers
+        # Everything is now cached so no more queries are needed.
+        with self.exactly_x_queries(0):
+            getattr(self.info, set_name).subscribers
+            getattr(self.info, set_name).subscribers.sorted
+
+    def exercise_subscription_set_sorted_first(self, set_name):
+        # Looking up subscriptions takes a single query.
+        with self.exactly_x_queries(1):
+            getattr(self.info, set_name)
+        # Getting the sorted subscriptions takes one additional query.
+        with self.exactly_x_queries(1):
+            getattr(self.info, set_name).sorted
+        # Everything is now cached so no more queries are needed.
+        with self.exactly_x_queries(0):
+            getattr(self.info, set_name).subscribers
+            getattr(self.info, set_name).subscribers.sorted
+
+    def test_direct_subscriptions(self):
+        self.exercise_subscription_set(
+            "direct_subscriptions")
+
+    def test_direct_subscriptions_sorted_first(self):
+        self.exercise_subscription_set_sorted_first(
+            "direct_subscriptions")
+
+    def make_duplicate_bug(self):
+        duplicate_bug = self.factory.makeBug(product=self.target)
+        with person_logged_in(duplicate_bug.owner):
+            duplicate_bug.markAsDuplicate(self.bug)
+
+    def test_duplicate_subscriptions(self):
+        self.make_duplicate_bug()
+        self.exercise_subscription_set(
+            "duplicate_subscriptions")
+
+    def test_duplicate_subscriptions_sorted_first(self):
+        self.make_duplicate_bug()
+        self.exercise_subscription_set_sorted_first(
+            "duplicate_subscriptions")
+
+    def test_duplicate_subscriptions_for_private_bug(self):
+        self.make_duplicate_bug()
+        with person_logged_in(self.bug.owner):
+            self.bug.setPrivate(True, self.bug.owner)
+        with self.exactly_x_queries(0):
+            self.info.duplicate_subscriptions
+        with self.exactly_x_queries(0):
+            self.info.duplicate_subscriptions.subscribers
+
+    def add_structural_subscriber(self):
+        with person_logged_in(self.bug.owner):
+            self.target.addSubscription(self.bug.owner, self.bug.owner)
+
+    def test_structural_subscriptions(self):
+        self.add_structural_subscriber()
+        self.exercise_subscription_set(
+            "structural_subscriptions")
+
+    def test_structural_subscriptions_sorted_first(self):
+        self.add_structural_subscriber()
+        self.exercise_subscription_set_sorted_first(
+            "structural_subscriptions")
+
+    def test_all_assignees(self):
+        with self.exactly_x_queries(1):
+            self.info.all_assignees
+
+    def test_all_pillar_owners_without_bug_supervisors(self):
+        # Getting all bug supervisors and pillar owners can take several
+        # queries. However, there are typically few tasks so the trade for
+        # simplicity of implementation is acceptable. Only the simplest case
+        # is tested here.
+        with self.exactly_x_queries(2):
+            self.info.all_pillar_owners_without_bug_supervisors
+
+    def test_also_notified_subscribers(self):
+        with self.exactly_x_queries(6):
+            self.info.also_notified_subscribers
+
+    def test_also_notified_subscribers_later(self):
+        # When also_notified_subscribers is referenced after some other sets
+        # in BugSubscriptionInfo are referenced, everything comes from cache.
+        self.info.all_assignees
+        self.info.all_pillar_owners_without_bug_supervisors
+        self.info.direct_subscriptions.subscribers
+        self.info.structural_subscriptions.subscribers
+        with self.exactly_x_queries(0):
+            self.info.also_notified_subscribers
+
+    def test_indirect_subscribers(self):
+        with self.exactly_x_queries(7):
+            self.info.indirect_subscribers
+
+    def test_indirect_subscribers_later(self):
+        # When indirect_subscribers is referenced after some other sets in
+        # BugSubscriptionInfo are referenced, everything comes from cache.
+        self.info.also_notified_subscribers
+        self.info.duplicate_subscriptions.subscribers
+        with self.exactly_x_queries(0):
+            self.info.indirect_subscribers

=== modified file 'lib/lp/bugs/stories/initial-bug-contacts/20-file-upstream-bug.txt'
--- lib/lp/bugs/stories/initial-bug-contacts/20-file-upstream-bug.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/bugs/stories/initial-bug-contacts/20-file-upstream-bug.txt	2010-12-24 15:20:32 +0000
@@ -19,13 +19,15 @@
 supervisor), Landscape Developers (another former bug supervisor) and
 Robert Collins (the current bug supervisor) are subscribed to this bug:
 
+    >>> from itertools import chain
     >>> from zope.component import getUtility
     >>> from lp.bugs.interfaces.bug import IBugSet
     >>> from canonical.launchpad.ftests import login, logout, ANONYMOUS
 
     >>> def subscriber_names(bug):
-    ...     subscribers = (
-    ...         bug.getDirectSubscribers() + bug.getIndirectSubscribers())
+    ...     subscribers = chain(
+    ...         bug.getDirectSubscribers(),
+    ...         bug.getIndirectSubscribers())
     ...     return sorted(subscriber.displayname for subscriber in subscribers)
 
     >>> login(ANONYMOUS)

=== modified file 'lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt'
--- lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt	2010-12-24 15:20:32 +0000
@@ -23,13 +23,15 @@
 subscriber), mark, the distro bug supervisor kamion, and foobar, who is
 subscribed to the distribution.
 
+    >>> from itertools import chain
     >>> from zope.component import getUtility
     >>> from lp.bugs.interfaces.bug import IBugSet
     >>> from canonical.launchpad.ftests import login, logout, ANONYMOUS
 
     >>> def subscriber_names(bug):
-    ...     subscribers = (
-    ...         bug.getDirectSubscribers() + bug.getIndirectSubscribers())
+    ...     subscribers = chain(
+    ...         bug.getDirectSubscribers(),
+    ...         bug.getIndirectSubscribers())
     ...     return sorted(subscriber.name for subscriber in subscribers)
 
     >>> login(ANONYMOUS)
@@ -72,4 +74,3 @@
     [u'mark', u'ubuntu-team']
 
     >>> logout()
-

=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2010-12-23 19:42:05 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2010-12-24 15:20:32 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from itertools import chain
 import unittest
 
 from lazr.lifecycle.event import ObjectModifiedEvent
@@ -31,8 +32,8 @@
     )
 from lp.testing import TestCaseWithFactory
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.mail_helpers import pop_notifications
 from lp.testing.matchers import Contains
-from lp.testing.mail_helpers import pop_notifications
 
 
 class TestNotificationRecipientsOfPrivateBugs(unittest.TestCase):
@@ -170,8 +171,8 @@
         self.bug = self.factory.makeBug()
         self.dupe_bug = self.factory.makeBug()
         self.dupe_bug.markAsDuplicate(self.bug)
-        self.dupe_subscribers = set(
-            self.dupe_bug.getDirectSubscribers() +
+        self.dupe_subscribers = set().union(
+            self.dupe_bug.getDirectSubscribers(),
             self.dupe_bug.getIndirectSubscribers())
 
     def test_comment_notifications(self):
@@ -246,9 +247,9 @@
         self.pillar.official_malone = True
         [bugtask] = self.bug.bugtasks
         all_subscribers = set(
-            [person.name for person in
-             self.bug.getDirectSubscribers() +
-             self.bug.getIndirectSubscribers()])
+            [person.name for person in chain(
+                    self.bug.getDirectSubscribers(),
+                    self.bug.getIndirectSubscribers())])
         bugtask_before_modification = Snapshot(
             bugtask, providing=providedBy(bugtask))
         bugtask.transitionToStatus(
@@ -268,9 +269,9 @@
         self.pillar.official_malone = False
         [bugtask] = self.bug.bugtasks
         all_subscribers = set(
-            [person.name for person in
-             self.bug.getDirectSubscribers() +
-             self.bug.getIndirectSubscribers()])
+            [person.name for person in chain(
+                    self.bug.getDirectSubscribers(),
+                    self.bug.getIndirectSubscribers())])
         bugtask_before_modification = Snapshot(
             bugtask, providing=providedBy(bugtask))
         bugtask.transitionToStatus(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-12-20 15:06:51 +0000
+++ lib/lp/registry/model/person.py	2010-12-24 15:20:32 +0000
@@ -15,6 +15,7 @@
     'JoinTeamEvent',
     'Owner',
     'Person',
+    'person_sort_key',
     'PersonLanguage',
     'PersonSet',
     'SSHKey',
@@ -22,7 +23,8 @@
     'TeamInvitationEvent',
     'ValidPersonCache',
     'WikiName',
-    'WikiNameSet']
+    'WikiNameSet',
+    ]
 
 from datetime import (
     datetime,
@@ -339,6 +341,16 @@
     return value
 
 
+_person_sort_re = re.compile("(?:[^\w\s]|[\d_])", re.U)
+
+def person_sort_key(person):
+    """Identical to `person_sort_key` in the database."""
+    # Strip noise out of displayname. We do not have to bother with
+    # name, as we know it is just plain ascii.
+    displayname = _person_sort_re.sub(u'', person.displayname.lower())
+    return "%s, %s" % (displayname.strip(), person.name)
+
+
 class Person(
     SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
     HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin):

=== modified file 'lib/lp/registry/tests/test_person_sort_key.py'
--- lib/lp/registry/tests/test_person_sort_key.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/tests/test_person_sort_key.py	2010-12-24 15:20:32 +0000
@@ -1,84 +1,103 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test the person_sort_key stored procedure."""
+"""Test the person_sort_key stored procedure and its in-app twin."""
 
 __metaclass__ = type
 
-import unittest
-
-from canonical.testing.layers import LaunchpadLayer
-
-
-class TestPersonSortKey(unittest.TestCase):
-    layer = LaunchpadLayer
+from canonical.testing.layers import DatabaseLayer
+from lp.registry.model.person import person_sort_key
+from lp.testing import TestCase
+
+
+class TestPersonSortKeyBase:
+
+    def test_composition(self):
+        # person_sort_key returns the concatenation of the display name and
+        # the name for use in sorting.
+        self.assertSortKeysEqual(
+            u"Stuart Bishop", u"stub",
+            u"stuart bishop, stub")
+
+    def test_whitespace(self):
+        # Leading and trailing whitespace is removed.
+        self.assertSortKeysEqual(
+            u" Stuart Bishop\t", u"stub",
+            u"stuart bishop, stub")
+
+    def test_valid_name_is_assumed(self):
+        # 'name' is assumed to be lowercase and not containing anything we
+        # don't want. This should never happen as the valid_name database
+        # constraint should prevent it.
+        self.assertSortKeysEqual(
+            u"Stuart Bishop", u" stub42!!!",
+            u"stuart bishop,  stub42!!!")
+
+    def test_strip_all_but_letters_and_whitespace(self):
+        # Everything except for letters and whitespace is stripped.
+        self.assertSortKeysEqual(
+            u"-= Mass1v3 T0SSA =-", u"tossa",
+            u"massv tssa, tossa")
+
+    def test_non_ascii_allowed(self):
+        # Non ASCII letters are currently allowed. Eventually they should
+        # become transliterated to ASCII but we don't do this yet.
+        self.assertSortKeysEqual(
+            u"Bj\N{LATIN SMALL LETTER O WITH DIAERESIS}rn", "bjorn",
+            u"bj\xf6rn, bjorn")
+
+    def test_unicode_case_conversion(self):
+        # Case conversion is handled correctly using Unicode.
+        self.assertSortKeysEqual(
+            u"Bj\N{LATIN CAPITAL LETTER O WITH DIAERESIS}rn", "bjorn",
+            u"bj\xf6rn, bjorn") # Lower case o with diaeresis
+
+
+class TestPersonSortKeyInDatabase(TestPersonSortKeyBase, TestCase):
+
+    layer = DatabaseLayer
 
     def setUp(self):
+        super(TestPersonSortKeyInDatabase, self).setUp()
         self.con = self.layer.connect()
         self.cur = self.con.cursor()
 
     def tearDown(self):
+        super(TestPersonSortKeyInDatabase, self).tearDown()
         self.con.close()
 
-    def person_sort_key(self, displayname, name):
-        '''Calls the person_sort_key stored procedure
+    def get_person_sort_key(self, displayname, name):
+        '''Calls the `person_sort_key` stored procedure.
 
         Note that although the stored procedure returns a UTF-8 encoded
         string, our database driver converts that to Unicode for us.
         '''
+        # Note that as we are testing a PostgreSQL stored procedure, we should
+        # pass it UTF-8 encoded strings to match our database encoding.
         self.cur.execute(
-                "SELECT person_sort_key(%(displayname)s, %(name)s)", vars()
-                )
+            "SELECT person_sort_key(%s, %s)", (
+                displayname.encode("UTF-8"), name.encode("UTF-8")))
         return self.cur.fetchone()[0]
 
-    def test_person_sort_key(self):
-
-        # person_sort_key returns the concatenation of the display name
-        # and the name for use in sorting.
-        self.failUnlessEqual(
-                self.person_sort_key("Stuart Bishop", "stub"),
-                "stuart bishop, stub"
-                )
-
-        # Leading and trailing whitespace is removed
-        self.failUnlessEqual(
-                self.person_sort_key(" Stuart Bishop\t", "stub"),
-                "stuart bishop, stub"
-                )
-
-        # 'name' is assumed to be lowercase and not containing anything
-        # we don't want. This should never happen as the valid_name database
-        # constraint should prevent it.
-        self.failUnlessEqual(
-                self.person_sort_key("Stuart Bishop", " stub42!!!"),
-                "stuart bishop,  stub42!!!"
-                )
-
-        # Everything except for letters and whitespace is stripped.
-        self.failUnlessEqual(
-                self.person_sort_key("-= Mass1v3 T0SSA =-", "tossa"),
-                "massv tssa, tossa"
-                )
-
-        # Non ASCII letters are currently allowed. Eventually they should
-        # become transliterated to ASCII but we don't do this yet.
-        # Note that as we are testing a PostgreSQL stored procedure, we
-        # should pass it UTF-8 encoded strings to match our database encoding.
-        self.failUnlessEqual(
-                self.person_sort_key(
-                    u"Bj\N{LATIN SMALL LETTER O WITH DIAERESIS}rn".encode(
-                        "UTF-8"), "bjorn"),
-                u"bj\xf6rn, bjorn"
-                )
-
-        # Case conversion is handled correctly using Unicode
-        self.failUnlessEqual(
-                self.person_sort_key(
-                    u"Bj\N{LATIN CAPITAL LETTER O WITH DIAERESIS}rn".encode(
-                        "UTF-8"), "bjorn"),
-                u"bj\xf6rn, bjorn" # Lower case o with diaeresis
-                )
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    def assertSortKeysEqual(self, displayname, name, expected):
+        # The sort key from the database matches the expected sort key.
+        self.assertEqual(
+            expected, self.get_person_sort_key(
+                displayname, name))
+
+
+class PersonNames:
+    """A fake with enough information for `person_sort_key`."""
+
+    def __init__(self, displayname, name):
+        self.displayname = displayname
+        self.name = name
+
+
+class TestPersonSortKeyInProcess(TestPersonSortKeyBase, TestCase):
+
+    def assertSortKeysEqual(self, displayname, name, expected):
+        # The sort key calculated in-process matches the expected sort key.
+        self.assertEqual(
+            expected, person_sort_key(
+                PersonNames(displayname, name)))