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 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/subscribers/bug.py' >--- lib/lp/bugs/subscribers/bug.py 2010-10-25 13:22:46 +0000 >+++ lib/lp/bugs/subscribers/bug.py 2010-12-08 21:12:34 +0000 >@@ -192,6 +192,9 @@ > if recipients is not None: > recipients.addRegistrant(pillar.owner, pillar) > >+ # XXX: GavinPanella 2010-11-30: What about if the bug supervisor *is* set? >+ # Don't we want to send mail to him/her? This is a reminder that you need a bug number for this XXX. > return sorted( > also_notified_subscribers, > key=attrgetter('displayname')) > >=== 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-08 21:12:34 +0000 > 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" >- ) >+ self.assertSortKeysEqual( >+ u"Stuart Bishop", u"stub", >+ u"stuart bishop, stub") > > # Leading and trailing whitespace is removed >- self.failUnlessEqual( >- self.person_sort_key(" Stuart Bishop\t", "stub"), >- "stuart bishop, stub" >- ) >+ self.assertSortKeysEqual( >+ u" Stuart Bishop\t", u"stub", >+ u"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!!!" >- ) >+ self.assertSortKeysEqual( >+ u"Stuart Bishop", u" stub42!!!", >+ u"stuart bishop, stub42!!!") > > # Everything except for letters and whitespace is stripped. >- self.failUnlessEqual( >- self.person_sort_key("-= Mass1v3 T0SSA =-", "tossa"), >- "massv tssa, tossa" >- ) >+ self.assertSortKeysEqual( >+ u"-= Mass1v3 T0SSA =-", u"tossa", >+ u"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" >- ) >+ self.assertSortKeysEqual( >+ u"Bj\N{LATIN SMALL LETTER O WITH DIAERESIS}rn", "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 >- ) >+ self.assertSortKeysEqual( >+ u"Bj\N{LATIN CAPITAL LETTER O WITH DIAERESIS}rn", "bjorn", >+ u"bj\xf6rn, bjorn") # Lower case o with diaeresis test_person_sort_key() should be split into multiple test methods. The LaunchpadLayer starts up the Librarian, which is not necessary for this test. The DatabaseLayer is much quicker.