launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01437
[Merge] lp:~allenap/launchpad/wire-up-filter-subs-bug-655567 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/wire-up-filter-subs-bug-655567 into lp:launchpad with lp:~allenap/launchpad/subs-for-bugtask-bug-656194 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#655567 Wire up getSubscriptionsForBug() into the notifications machinery
https://bugs.launchpad.net/bugs/655567
Replace the use of PersonSet.getSubscribersForTargets() with
bug.getStructuralSubscribers(), and remove the former from the
code-base entirely.
There are also a small sprinkling of lint fixes. One I feel I should
comment on:
{{{
- def to_messages(rows):
- return [row[0] for row in rows]
+ to_messages = lambda rows: [row[0] for row in rows]
else:
- def to_messages(rows):
- return rows
+ to_messages = lambda rows: rows
}}}
While it's equivalent code, this silences a warning in pyflakes. While
we probably should fix pyflakes, that's out of scope for now, and
keeping lint warnings to a minimum is useful for collaboration.
I have also fixed a small bug in format-new-and-modified-imports which
caused it to fail when files are renamed.
--
https://code.launchpad.net/~allenap/launchpad/wire-up-filter-subs-bug-655567/+merge/37857
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/wire-up-filter-subs-bug-655567 into lp:launchpad.
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2010-10-06 18:53:53 +0000
+++ lib/lp/bugs/configure.zcml 2010-10-07 15:02:46 +0000
@@ -706,6 +706,7 @@
getSubscribersForPerson
indexed_messages
getAlsoNotifiedSubscribers
+ getStructuralSubscribers
getBugWatch
canBeNominatedFor
getNominationFor
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2010-10-03 15:30:06 +0000
+++ lib/lp/bugs/interfaces/bug.py 2010-10-07 15:02:46 +0000
@@ -491,6 +491,12 @@
from duplicates.
"""
+ def getStructuralSubscribers(recipients=None, level=None):
+ """Return `IPerson`s subscribed to this bug's targets.
+
+ This takes into account bug subscription filters.
+ """
+
def getSubscriptionsFromDuplicates():
"""Return IBugSubscriptions subscribed from dupes of this bug."""
@@ -499,9 +505,9 @@
def getSubscribersForPerson(person):
"""Find the persons or teams by which person is subscribed.
-
+
This call should be quite cheap to make and performs a single query.
-
+
:return: An IResultSet.
"""
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-10-04 06:20:04 +0000
+++ lib/lp/bugs/model/bug.py 2010-10-07 15:02:46 +0000
@@ -51,7 +51,6 @@
In,
LeftJoin,
Max,
- Min,
Not,
Or,
Select,
@@ -156,14 +155,13 @@
from lp.bugs.model.bugnomination import BugNomination
from lp.bugs.model.bugnotification import BugNotification
from lp.bugs.model.bugsubscription import BugSubscription
+from lp.bugs.model.bugtarget import OfficialBugTag
from lp.bugs.model.bugtask import (
BugTask,
bugtask_sort_key,
- BugTaskSet,
get_bug_privacy_filter,
NullBugTask,
)
-from lp.bugs.model.bugtarget import OfficialBugTag
from lp.bugs.model.bugwatch import BugWatch
from lp.hardwaredb.interfaces.hwdb import IHWSubmissionBugSet
from lp.registry.enum import BugNotificationLevel
@@ -172,10 +170,7 @@
IDistributionSourcePackage,
)
from lp.registry.interfaces.distroseries import IDistroSeries
-from lp.registry.interfaces.person import (
- IPersonSet,
- validate_public_person,
- )
+from lp.registry.interfaces.person import validate_public_person
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.productseries import IProductSeries
from lp.registry.interfaces.series import SeriesStatus
@@ -458,11 +453,9 @@
store = Store.of(self)
message_by_id = {}
if include_parents:
- def to_messages(rows):
- return [row[0] for row in rows]
+ to_messages = lambda rows: [row[0] for row in rows]
else:
- def to_messages(rows):
- return rows
+ to_messages = lambda rows: rows
def eager_load_owners(messages):
# Because we may have multiple owners, we spend less time in storm
# with very large bugs by not joining and instead querying a second
@@ -960,31 +953,12 @@
also_notified_subscribers = set()
- structural_subscription_targets = set()
-
for bugtask in self.bugtasks:
if bugtask.assignee:
also_notified_subscribers.add(bugtask.assignee)
if recipients is not None:
recipients.addAssignee(bugtask.assignee)
- if IStructuralSubscriptionTarget.providedBy(bugtask.target):
- structural_subscription_targets.add(bugtask.target)
- if bugtask.target.parent_subscription_target is not None:
- structural_subscription_targets.add(
- bugtask.target.parent_subscription_target)
-
- 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.
- structural_subscription_targets.add(
- bugtask.distroseries)
-
- if bugtask.milestone is not None:
- structural_subscription_targets.add(bugtask.milestone)
-
# If the target's bug supervisor isn't set,
# we add the owner as a subscriber.
pillar = bugtask.pillar
@@ -993,22 +967,73 @@
if recipients is not None:
recipients.addRegistrant(pillar.owner, pillar)
- person_set = getUtility(IPersonSet)
- target_subscribers = person_set.getSubscribersForTargets(
- structural_subscription_targets, recipients=recipients,
- level=level)
-
- also_notified_subscribers.update(target_subscribers)
+ # Structural subscribers.
+ also_notified_subscribers.update(
+ self.getStructuralSubscribers(
+ recipients=recipients, level=level))
# Direct subscriptions always take precedence over indirect
# subscriptions.
direct_subscribers = set(self.getDirectSubscribers())
+
# Remove security proxy for the sort key, but return
# the regular proxied object.
return sorted(
(also_notified_subscribers - direct_subscribers),
key=lambda x: removeSecurityProxy(x).displayname)
+ def getStructuralSubscribers(self, recipients=None, level=None):
+ """See `IBug`. """
+ query_arguments = []
+ for bugtask in self.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))
+
+ if len(query_arguments) == 0:
+ return EmptyResultSet()
+
+ if level is None:
+ # If level is not specified, default to NOTHING so that all
+ # subscriptions are found. XXX: Perhaps this should go in
+ # getSubscriptionsForBugTask()?
+ level = BugNotificationLevel.NOTHING
+
+ # Build the query.
+ union = lambda left, right: left.union(right)
+ queries = (
+ target.getSubscriptionsForBugTask(bugtask, level)
+ for target, bugtask in query_arguments)
+ subscriptions = reduce(union, queries)
+
+ # Pull all the subscriptions in.
+ subscriptions = list(subscriptions)
+
+ # Prepare a query for the subscribers.
+ subscribers = Store.of(self).find(
+ Person, Person.id.is_in(
+ subscription.subscriberID
+ for subscription in subscriptions))
+
+ if recipients is not None:
+ # We need to process subscriptions, so pull all the subscribes
+ # into the cache, then update recipients with the subscriptions.
+ subscribers = list(subscribers)
+ for subscription in subscriptions:
+ recipients.addStructuralSubscriber(
+ subscription.subscriber, subscription.target)
+
+ return subscribers
+
def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
level=None,
include_master_dupe_subscribers=False):
=== renamed file 'lib/lp/bugs/tests/test_bug.py' => 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py 2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2010-10-07 15:02:46 +0000
@@ -5,7 +5,11 @@
__metaclass__ = type
+from storm.store import ResultSet
+
from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
+from lp.registry.enum import BugNotificationLevel
from lp.registry.interfaces.person import PersonVisibility
from lp.registry.model.structuralsubscription import StructuralSubscription
from lp.testing import (
@@ -13,6 +17,7 @@
person_logged_in,
TestCaseWithFactory,
)
+from lp.testing.matchers import StartsWith
class TestBug(TestCaseWithFactory):
@@ -34,8 +39,9 @@
def test_get_subscribers_for_person_indirect_subscription(self):
bug = self.factory.makeBug()
person = self.factory.makePerson()
- team1 = self.factory.makeTeam(members=[person])
- team2 = self.factory.makeTeam(members=[person])
+ [team1, team2] = (
+ self.factory.makeTeam(members=[person]),
+ self.factory.makeTeam(members=[person]))
with person_logged_in(person):
bug.subscribe(team1, person)
self.assertEqual([team1], list(bug.getSubscribersForPerson(person)))
@@ -125,3 +131,84 @@
dupe_bug.subscribe(team, member)
dupe_bug.markAsDuplicate(bug)
self.assertTrue(team in bug.getSubscribersFromDuplicates())
+
+
+class TestBugStructuralSubscribers(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_getStructuralSubscribers_no_subscribers(self):
+ # If there are no subscribers for any of the bug's targets then no
+ # subscribers will be returned by getStructuralSubscribers().
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(product=product)
+ subscribers = bug.getStructuralSubscribers()
+ self.assertIsInstance(subscribers, ResultSet)
+ self.assertEqual([], list(subscribers))
+
+ def test_getStructuralSubscribers_single_target(self):
+ # Subscribers for any of the bug's targets are returned.
+ subscriber = self.factory.makePerson()
+ login_person(subscriber)
+ product = self.factory.makeProduct()
+ product.addBugSubscription(subscriber, subscriber)
+ bug = self.factory.makeBug(product=product)
+ self.assertEqual([subscriber], list(bug.getStructuralSubscribers()))
+
+ def test_getStructuralSubscribers_multiple_targets(self):
+ # Subscribers for any of the bug's targets are returned.
+ actor = self.factory.makePerson()
+ login_person(actor)
+
+ subscriber1 = self.factory.makePerson()
+ subscriber2 = self.factory.makePerson()
+
+ product1 = self.factory.makeProduct(owner=actor)
+ product1.addBugSubscription(subscriber1, subscriber1)
+ product2 = self.factory.makeProduct(owner=actor)
+ product2.addBugSubscription(subscriber2, subscriber2)
+
+ bug = self.factory.makeBug(product=product1)
+ bug.addTask(actor, product2)
+
+ subscribers = bug.getStructuralSubscribers()
+ self.assertIsInstance(subscribers, ResultSet)
+ self.assertEqual(set([subscriber1, subscriber2]), set(subscribers))
+
+ def test_getStructuralSubscribers_recipients(self):
+ # If provided, getStructuralSubscribers() calls the appropriate
+ # methods on a BugNotificationRecipients object.
+ subscriber = self.factory.makePerson()
+ login_person(subscriber)
+ product = self.factory.makeProduct()
+ product.addBugSubscription(subscriber, subscriber)
+ bug = self.factory.makeBug(product=product)
+ recipients = BugNotificationRecipients()
+ subscribers = bug.getStructuralSubscribers(recipients=recipients)
+ # The return value is a list only when populating recipients.
+ self.assertIsInstance(subscribers, list)
+ self.assertEqual([subscriber], recipients.getRecipients())
+ reason, header = recipients.getReason(subscriber)
+ self.assertThat(
+ reason, StartsWith(
+ u"You received this bug notification because "
+ u"you are subscribed to "))
+ self.assertThat(header, StartsWith(u"Subscriber "))
+
+ def test_getStructuralSubscribers_level(self):
+ # getStructuralSubscribers() respects the given level.
+ subscriber = self.factory.makePerson()
+ login_person(subscriber)
+ product = self.factory.makeProduct()
+ subscription = product.addBugSubscription(subscriber, subscriber)
+ subscription.bug_notification_level = BugNotificationLevel.METADATA
+ bug = self.factory.makeBug(product=product)
+ self.assertEqual(
+ [subscriber], list(
+ bug.getStructuralSubscribers(
+ level=BugNotificationLevel.METADATA)))
+ subscription.bug_notification_level = BugNotificationLevel.METADATA
+ self.assertEqual(
+ [], list(
+ bug.getStructuralSubscribers(
+ level=BugNotificationLevel.COMMENTS)))
=== modified file 'lib/lp/registry/doc/structural-subscriptions.txt'
--- lib/lp/registry/doc/structural-subscriptions.txt 2010-10-06 18:53:53 +0000
+++ lib/lp/registry/doc/structural-subscriptions.txt 2010-10-07 15:02:46 +0000
@@ -198,61 +198,6 @@
name16
-Retrieving structural subscribers of targets
-============================================
-
-Subscribers of one or more targets are retrieved by
-PersonSet.getSubscribersForTargets.
-
-XXX Abel Deuring 2008-07-11 We must remove the security proxy, because
-PersonSet.getSubscribersForTargets() accesses the private attribute
-firefox._targets_args. This attribute should be renamed. Bug #247525.
-
- >>> from zope.security.proxy import removeSecurityProxy
- >>> firefox_no_proxy = removeSecurityProxy(firefox)
- >>> from lp.registry.interfaces.person import IPersonSet
- >>> person_set = getUtility(IPersonSet)
- >>> firefox_subscribers = person_set.getSubscribersForTargets(
- ... [firefox_no_proxy])
- >>> print_bug_subscribers(firefox_subscribers)
- name12
-
-If PersonSet.getSubscribersForTargets() is passed a
-BugNotificationLevel in its `level` parameter, only structural
-subscribers with that notification level or higher will be returned.
-The subscription level of ff_sub, the only structural subscription
-to firefox, is NOTHING, hence we get an empty list if we pass any
-larger bug notification level.
-
- >>> firefox_subscribers = person_set.getSubscribersForTargets(
- ... [firefox_no_proxy], level=BugNotificationLevel.LIFECYCLE)
- >>> print len(firefox_subscribers)
- 0
-
-If the bug notification level of ff_sub is increased, the subscription
-is included in the result of PersonSet.getSubscribersForTarget().
-
- >>> ff_sub.level = BugNotificationLevel.LIFECYCLE
- >>> firefox_subscribers = person_set.getSubscribersForTargets(
- ... [firefox_no_proxy])
- >>> print_bug_subscribers(firefox_subscribers)
- name12
- >>> ff_sub.level = BugNotificationLevel.METADATA
- >>> firefox_subscribers = person_set.getSubscribersForTargets(
- ... [firefox_no_proxy])
- >>> print_bug_subscribers(firefox_subscribers)
- name12
-
-More than one target can be passed to PersonSet.getSubscribersForTargets().
-
- >>> ubuntu_no_proxy = removeSecurityProxy(ubuntu)
- >>> ubuntu_or_firefox_subscribers = person_set.getSubscribersForTargets(
- ... [firefox_no_proxy, ubuntu_no_proxy])
- >>> print_bug_subscribers(ubuntu_or_firefox_subscribers)
- name12
- name16
-
-
Target type display
===================
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-09-29 20:05:17 +0000
+++ lib/lp/registry/interfaces/person.py 2010-10-07 15:02:46 +0000
@@ -2006,17 +2006,6 @@
specified product are returned.
"""
- def getSubscribersForTargets(targets, recipients=None):
- """Return the set of subscribers for `targets`.
-
- :param targets: The sequence of targets for which to get the
- subscribers.
- :param recipients: An optional instance of
- `BugNotificationRecipients`.
- If present, all found subscribers will be
- added to it.
- """
-
def updatePersonalStandings():
"""Update the personal standings of some people.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-09-29 14:38:30 +0000
+++ lib/lp/registry/model/person.py 2010-10-07 15:02:46 +0000
@@ -4060,59 +4060,6 @@
Person.id in (%s)
''' % branch_clause)
- def getSubscribersForTargets(self, targets, recipients=None, level=None):
- """See `IPersonSet`. """
- if len(targets) == 0:
- return set()
- target_criteria = []
- for target in targets:
- # target_args is a mapping from query arguments
- # to query values.
- target_args = target._target_args
- target_criteria_clauses = []
- for key, value in target_args.items():
- if value is not None:
- target_criteria_clauses.append(
- '%s = %s' % (key, quote(value)))
- else:
- target_criteria_clauses.append(
- '%s IS NULL' % key)
- if level is not None:
- target_criteria_clauses.append('bug_notification_level >= %s'
- % quote(level.value))
-
- target_criteria.append(
- '(%s)' % ' AND '.join(target_criteria_clauses))
-
- # Build a UNION query, since using OR slows down the query a lot.
- subscriptions = StructuralSubscription.select(target_criteria[0])
- for target_criterion in target_criteria[1:]:
- subscriptions = subscriptions.union(
- StructuralSubscription.select(target_criterion))
-
- # Listify the result, since we want to loop over it multiple times.
- subscriptions = list(subscriptions)
-
- # We can't use prejoins in UNION queries, so populate the cache
- # by getting all the subscribers.
- subscriber_ids = [
- subscription.subscriberID for subscription in subscriptions]
- if len(subscriber_ids) > 0:
- # Pull in ValidPersonCache records in addition to Person
- # records to warm up the cache.
- list(IStore(Person).find(
- (Person, ValidPersonCache),
- In(Person.id, subscriber_ids),
- ValidPersonCache.id == Person.id))
-
- subscribers = set()
- for subscription in subscriptions:
- subscribers.add(subscription.subscriber)
- if recipients is not None:
- recipients.addStructuralSubscriber(
- subscription.subscriber, subscription.target)
- return subscribers
-
def updatePersonalStandings(self):
"""See `IPersonSet`."""
cur = cursor()
=== modified file 'utilities/format-new-and-modified-imports'
--- utilities/format-new-and-modified-imports 2010-09-28 09:50:49 +0000
+++ utilities/format-new-and-modified-imports 2010-10-07 15:02:46 +0000
@@ -9,5 +9,5 @@
#
bzr status --short "$@" | \
- awk '/^.[MN] .*[.]py$/ { print $2 }' | \
+ awk '/^.[MN] .*[.]py$/ { print $NF }' | \
xargs -r "$(dirname "$0")/format-imports"
