launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02122
[Merge] lp:~allenap/launchpad/subscribe-to-tag-bug-151129-5 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/subscribe-to-tag-bug-151129-5 into lp:launchpad with lp:~allenap/launchpad/subscribe-to-tag-bug-151129-4 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#151129 Can't subscribe to a tag
https://bugs.launchpad.net/bugs/151129
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).
--
https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-5/+merge/43101
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/subscribe-to-tag-bug-151129-5 into lp:launchpad.
=== 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-08 16:24:13 +0000
@@ -490,7 +490,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-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/bugzilla-import.txt 2010-12-08 16:24:13 +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-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/initial-bug-contacts.txt 2010-12-08 16:24:13 +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-08 16:24:13 +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-11-05 09:16:14 +0000
+++ lib/lp/bugs/model/bug.py 2010-12-08 16:24:13 +0000
@@ -25,6 +25,11 @@
timedelta,
)
from email.Utils import make_msgid
+from functools import wraps
+from itertools import (
+ chain,
+ imap,
+ )
import operator
import re
@@ -47,7 +52,6 @@
from storm.expr import (
And,
Count,
- Func,
LeftJoin,
Max,
Not,
@@ -175,8 +179,12 @@
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,
+ person_sort_key,
ValidPersonCache,
)
from lp.registry.model.pillar import pillar_sort_key
@@ -796,24 +804,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 +817,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`.
@@ -1954,6 +1939,188 @@
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())
+
+
+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))
+
+
+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(
+ chain(self.all_assignees,
+ self.all_pillar_owners_without_bug_supervisors,
+ self.structural_subscriptions.subscribers)).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-01 11:26:57 +0000
+++ lib/lp/bugs/model/bugtask.py 2010-12-08 16:24:13 +0000
@@ -22,6 +22,7 @@
import datetime
+from itertools import chain
from operator import attrgetter
from lazr.enum import DBItem
@@ -47,10 +48,6 @@
EmptyResultSet,
Store,
)
-from storm.zope.interfaces import (
- IResultSet,
- ISQLObjectResultSet,
- )
from zope.component import getUtility
from zope.interface import (
alsoProvides,
@@ -291,8 +288,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-08 16:24:13 +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-08 16:24:13 +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-08 16:24:13 +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/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 16:24:13 +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?
+
return sorted(
also_notified_subscribers,
key=attrgetter('displayname'))
=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py 2010-10-19 22:06:16 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py 2010-12-08 16:24:13 +0000
@@ -164,8 +164,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):
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-12-03 16:33:03 +0000
+++ lib/lp/registry/model/person.py 2010-12-08 16:24:13 +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,
@@ -332,6 +334,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-08 16:24:13 +0000
@@ -1,13 +1,22 @@
# 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
+from lp.registry.model.person import person_sort_key
+
+
+class PersonNames:
+ """A fake with enough information for `person_sort_key`."""
+
+ def __init__(self, displayname, name):
+ self.displayname = displayname
+ self.name = name
class TestPersonSortKey(unittest.TestCase):
@@ -20,64 +29,64 @@
def tearDown(self):
self.con.close()
- def person_sort_key(self, displayname, name):
- '''Calls the person_sort_key stored procedure
+ def db_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 assertSortKeysEqual(self, displayname, name, expected):
+ # The sort key from the database matches the expected sort key.
+ self.assertEqual(
+ expected, self.db_person_sort_key(
+ displayname, name))
+ # The sort key calculated in-process matches the expected sort key.
+ self.assertEqual(
+ expected, person_sort_key(
+ PersonNames(displayname, name)))
+
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
def test_suite():
