launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02484
[Merge] lp:~allenap/launchpad/subscribe-to-tag-bug-151129-6 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/subscribe-to-tag-bug-151129-6 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#681326 getSubscriptionsForBugTask() is being called too many times
https://bugs.launchpad.net/bugs/681326
For more details, see:
https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-6/+merge/48295
Some more work on infiltrating BugSubscriptionInfo into the position
of knowing all about subscriptions, and some other bits:
lib/lp/bugs/browser/bug.py
Use the new Bug.getSubscriptionInfo() method in BugViewMixin.
lib/lp/bugs/configure.zcml
Security declaration for BugSubscriptionInfo and
Bug.getSubscriptionInfo().
lib/lp/bugs/doc/bugsubscription.txt
BugSubscriptionInfo is immutable, and returns immutable data types,
so there are tuples here where there were once lists.
lib/lp/bugs/interfaces/bug.py
Interface declaration for the new getSubscriptionInfo() method.
lib/lp/bugs/model/bug.py
Implementation of getSubscriptionInfo(). I've also used it in
getDirectSubscriptions(), getDirectSubscribers() and
getSubscribersFromDuplicates().
BugSubscriptionInfo now implements IHasBug. This is true, and it
also means that a security adapter can be selected for it. See the
changes in test_bugsubscriptioninfo.py.
I've put a lot of the design goals I had in mind for
BugSubscriptionInfo into its docstring. Trying to convince people
that it's a good idea to use :)
Implementation of the duplicate_only_subscriptions set.
lib/lp/bugs/model/tests/test_bug.py
Test for getSubscriptionInfo().
lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py
Test for the new duplicate_only_subscriptions set.
Test for BugSubscriptionInfo permissions.
--
https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-6/+merge/48295
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/subscribe-to-tag-bug-151129-6 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2011-01-18 20:49:35 +0000
+++ lib/lp/bugs/browser/bug.py 2011-02-02 11:14:58 +0000
@@ -414,39 +414,28 @@
"""Mix-in class to share methods between bug and portlet views."""
@cachedproperty
+ def subscription_info(self):
+ return IBug(self.context).getSubscriptionInfo()
+
+ @property
def direct_subscribers(self):
"""Return the list of direct subscribers."""
- if IBug.providedBy(self.context):
- return set(self.context.getDirectSubscribers())
- elif IBugTask.providedBy(self.context):
- return set(self.context.bug.getDirectSubscribers())
- else:
- raise NotImplementedError(
- 'direct_subscribers is not provided by %s' % self)
+ return self.subscription_info.direct_subscriptions.subscribers
- @cachedproperty
+ @property
def duplicate_subscribers(self):
"""Return the list of subscribers from duplicates.
- Don't use getSubscribersFromDuplicates here because that method
- omits a user if the user is also a direct or indirect subscriber.
- getSubscriptionsFromDuplicates doesn't, so find person objects via
- this method.
+ This includes all subscribers who are also direct or indirect
+ subscribers.
"""
- if IBug.providedBy(self.context):
- dupe_subs = self.context.getSubscriptionsFromDuplicates()
- return set(sub.person for sub in dupe_subs)
- elif IBugTask.providedBy(self.context):
- dupe_subs = self.context.bug.getSubscriptionsFromDuplicates()
- return set(sub.person for sub in dupe_subs)
- else:
- raise NotImplementedError(
- 'duplicate_subscribers is not implemented for %s' % self)
+ return self.subscription_info.duplicate_subscriptions.subscribers
@cachedproperty
def subscriber_ids(self):
"""Return a dictionary mapping a css_name to user name."""
- subscribers = self.direct_subscribers.union(
+ subscribers = set().union(
+ self.direct_subscribers,
self.duplicate_subscribers)
# The current user has to be in subscribers_id so
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-01-31 14:07:47 +0000
+++ lib/lp/bugs/configure.zcml 2011-02-02 11:14:58 +0000
@@ -638,6 +638,31 @@
set_schema=".interfaces.bugsubscriptionfilter.IBugSubscriptionFilterAttributes" />
</class>
+ <!-- BugSubscriptionInfo -->
+
+ <class class=".model.bug.BugSubscriptionInfo">
+ <!-- Instance variables -->
+ <require
+ permission="launchpad.View"
+ attributes="
+ bug
+ level
+ " />
+ <!-- Properties -->
+ <require
+ permission="launchpad.View"
+ attributes="
+ all_assignees
+ all_pillar_owners_without_bug_supervisors
+ also_notified_subscribers
+ direct_subscriptions
+ duplicate_only_subscriptions
+ duplicate_subscriptions
+ indirect_subscribers
+ structural_subscriptions
+ " />
+ </class>
+
<facet
facet="bugs">
@@ -719,6 +744,7 @@
getSubscriptionsFromDuplicates
getSubscribersForPerson
getSubscriptionForPerson
+ getSubscriptionInfo
indexed_messages
getAlsoNotifiedSubscribers
getBugWatch
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2011-01-27 09:47:06 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2011-02-02 11:14:58 +0000
@@ -101,7 +101,7 @@
>>> linux_source_bug.getAlsoNotifiedSubscribers()
[<Person at ...>]
>>> linux_source_bug.getSubscribersFromDuplicates()
- []
+ ()
It is also possible to get the list of indirect subscribers for an
individual bug task.
@@ -137,7 +137,7 @@
Ubuntu Team
>>> linux_source_bug.getSubscribersFromDuplicates()
- []
+ ()
>>> print_displayname(linux_source_bug.getAlsoNotifiedSubscribers())
Sample Person
@@ -161,7 +161,7 @@
Ubuntu Team
>>> linux_source_bug.getSubscribersFromDuplicates()
- []
+ ()
>>> print_displayname(linux_source_bug.getAlsoNotifiedSubscribers())
No Privileges Person
Sample Person
@@ -365,7 +365,7 @@
>>> subscription_keybuk = linux_source.addBugSubscription(
... keybuk, keybuk)
>>> linux_source_bug.getSubscribersFromDuplicates()
- []
+ ()
When a bug is marked private, all its indirect subscribers become direct
subscribers.
@@ -409,7 +409,7 @@
[]
>>> linux_source_bug.getSubscribersFromDuplicates()
- []
+ ()
Direct subscriptions always take precedence over indirect
subscriptions. So, if we unmark the above bug as private,
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2011-01-31 14:07:47 +0000
+++ lib/lp/bugs/interfaces/bug.py 2011-02-02 11:14:58 +0000
@@ -529,6 +529,12 @@
If no such `BugSubscription` exists, return None.
"""
+ def getSubscriptionInfo(level):
+ """Return a `BugSubscriptionInfo` at the given `level`.
+
+ :param level: A member of `BugNotificationLevel`.
+ """
+
def getBugNotificationRecipients(duplicateof=None, old_bug=None,
include_master_dupe_subscribers=False):
"""Return a complete INotificationRecipientSet instance.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-01-31 09:24:13 +0000
+++ lib/lp/bugs/model/bug.py 2011-02-02 11:14:58 +0000
@@ -26,6 +26,7 @@
)
from email.Utils import make_msgid
from functools import wraps
+from itertools import chain
import operator
import re
@@ -90,6 +91,7 @@
)
from canonical.launchpad.helpers import shortlist
from canonical.launchpad.interfaces.launchpad import (
+ IHasBug,
ILaunchpadCelebrities,
IPersonRoles,
)
@@ -822,10 +824,13 @@
BugSubscription.bug_id == self.id).order_by(BugSubscription.id)
return DecoratedResultSet(results, operator.itemgetter(1))
+ def getSubscriptionInfo(self, level=BugNotificationLevel.NOTHING):
+ """See `IBug`."""
+ return BugSubscriptionInfo(self, level)
+
def getDirectSubscriptions(self):
"""See `IBug`."""
- return BugSubscriptionInfo(
- self, BugNotificationLevel.NOTHING).direct_subscriptions
+ return self.getSubscriptionInfo().direct_subscriptions
def getDirectSubscribers(self, recipients=None, level=None):
"""See `IBug`.
@@ -837,7 +842,7 @@
"""
if level is None:
level = BugNotificationLevel.NOTHING
- subscriptions = BugSubscriptionInfo(self, level).direct_subscriptions
+ subscriptions = self.getSubscriptionInfo(level).direct_subscriptions
if recipients is not None:
for subscriber in subscriptions.subscribers:
recipients.addDirectSubscriber(subscriber)
@@ -851,8 +856,8 @@
"""
# "Also notified" and duplicate subscribers are mutually
# exclusive, so return both lists.
- indirect_subscribers = (
- self.getAlsoNotifiedSubscribers(recipients, level) +
+ indirect_subscribers = chain(
+ self.getAlsoNotifiedSubscribers(recipients, level),
self.getSubscribersFromDuplicates(recipients, level))
# Remove security proxy for the sort key, but return
@@ -887,37 +892,16 @@
See the comment in getDirectSubscribers for a description of the
recipients argument.
"""
- if self.private:
- return []
-
- if level is None:
- notification_level = BugNotificationLevel.NOTHING
- else:
- notification_level = level
-
- dupe_details = dict(
- Store.of(self).find(
- (Person, Bug),
- BugSubscription.person == Person.id,
- BugSubscription.bug_id == Bug.id,
- BugSubscription.bug_notification_level >= notification_level,
- Bug.duplicateof == self.id))
-
- dupe_subscribers = set(dupe_details)
-
- # Direct and "also notified" subscribers take precedence over
- # subscribers from dupes. Note that we don't supply recipients
- # here because we are doing this to /remove/ subscribers.
- dupe_subscribers -= set(self.getDirectSubscribers())
- dupe_subscribers -= set(self.getAlsoNotifiedSubscribers())
+ info = self.getSubscriptionInfo()
if recipients is not None:
- for subscriber in dupe_subscribers:
+ # Pre-load duplicate bugs.
+ list(self.duplicates)
+ for subscription in info.duplicate_only_subscriptions:
recipients.addDupeSubscriber(
- subscriber, dupe_details[subscriber])
+ subscription.person, subscription.bug)
- return sorted(
- dupe_subscribers, key=operator.attrgetter("displayname"))
+ return info.duplicate_only_subscriptions.subscribers.sorted
def getSubscribersForPerson(self, person):
"""See `IBug."""
@@ -2091,7 +2075,30 @@
class BugSubscriptionInfo:
- """Represents bug subscription sets."""
+ """Represents bug subscription sets.
+
+ The intention for this class is to encapsulate all calculations of
+ subscriptions and subscribers for a bug. Some design considerations:
+
+ * Immutable.
+
+ * Set-based.
+
+ * Sets are cached.
+
+ * Usable with a *snapshot* of a bug. This is interesting for two reasons:
+
+ - Event subscribers commonly deal with snapshots. An instance of this
+ class could be added to a custom snapshot so that multiple subscribers
+ can share the information it contains.
+
+ - Use outside of the web request. A serialized snapshot could be used to
+ calculate subscribers for a particular bug state. This could help us
+ to move even more bug mail processing out of the web request.
+
+ """
+
+ implements(IHasBug)
def __init__(self, bug, level):
self.bug = bug
@@ -2120,6 +2127,22 @@
Bug.duplicateof == self.bug)
@cachedproperty
+ @freeze(BugSubscriptionSet)
+ def duplicate_only_subscriptions(self):
+ """Subscripitions to duplicates of the bug.
+
+ Excludes subscriptions for people who have a direct subscription or
+ are also notified for another reason.
+ """
+ self.duplicate_subscriptions.subscribers # Pre-load subscribers.
+ higher_precedence = (
+ self.direct_subscriptions.subscribers.union(
+ self.also_notified_subscribers))
+ return (
+ subscription for subscription in self.duplicate_subscriptions
+ if subscription.person not in higher_precedence)
+
+ @cachedproperty
@freeze(StructuralSubscriptionSet)
def structural_subscriptions(self):
"""Structural subscriptions to the bug's targets."""
@@ -2138,13 +2161,14 @@
if bugtask.milestone is not None:
query_arguments.append((bugtask.milestone, bugtask))
# Build the query.
+ empty = EmptyResultSet()
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)
+ return reduce(union, queries, empty)
@cachedproperty
@freeze(BugSubscriberSet)
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2010-11-12 18:05:45 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2011-02-02 11:14:58 +0000
@@ -4,6 +4,7 @@
__metaclass__ = type
from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.model.bug import BugSubscriptionInfo
from lp.registry.enum import BugNotificationLevel
from lp.registry.interfaces.person import PersonVisibility
from lp.registry.model.structuralsubscription import StructuralSubscription
@@ -240,3 +241,16 @@
self.assertTrue(
subscriber not in duplicate_subscribers,
"Subscriber should not be in duplicate_subscribers.")
+
+ def test_getSubscriptionInfo(self):
+ # getSubscriptionInfo() returns a BugSubscriptionInfo object.
+ bug = self.factory.makeBug()
+ with person_logged_in(bug.owner):
+ info = bug.getSubscriptionInfo()
+ self.assertIsInstance(info, BugSubscriptionInfo)
+ self.assertEqual(bug, info.bug)
+ self.assertEqual(BugNotificationLevel.NOTHING, info.level)
+ # A level can also be specified.
+ with person_logged_in(bug.owner):
+ info = bug.getSubscriptionInfo(BugNotificationLevel.METADATA)
+ self.assertEqual(BugNotificationLevel.METADATA, info.level)
=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2010-12-06 20:58:55 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2011-02-02 11:14:58 +0000
@@ -9,7 +9,10 @@
from storm.store import Store
from testtools.matchers import Equals
+from zope.component import queryAdapter
+from zope.security.checker import getChecker
+from canonical.launchpad.webapp.interfaces import IAuthorization
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.model.bug import (
BugSubscriberSet,
@@ -18,6 +21,9 @@
load_people,
StructuralSubscriptionSet,
)
+from lp.bugs.security import (
+ PublicToAllOrPrivateToExplicitSubscribersForBugTask,
+ )
from lp.registry.enum import BugNotificationLevel
from lp.registry.model.person import Person
from lp.testing import (
@@ -182,6 +188,28 @@
self.assertEqual(set(), found_subscriptions.subscribers)
self.assertEqual((), found_subscriptions.subscribers.sorted)
+ def test_duplicate_only(self):
+ # The set of duplicate subscriptions where the subscriber has no other
+ # subscriptions.
+ 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_only_subscriptions
+ self.assertEqual(
+ set([duplicate_bug_subscription]),
+ found_subscriptions)
+ # If a user is subscribed to a duplicate bug and is a bugtask
+ # assignee, for example, their duplicate subscription will not be
+ # included.
+ with person_logged_in(self.target.owner):
+ self.bug.default_bugtask.transitionToAssignee(
+ duplicate_bug_subscription.person)
+ found_subscriptions = self.getInfo().duplicate_only_subscriptions
+ self.assertEqual(set(), found_subscriptions)
+
def test_structural(self):
# The set of structural subscribers.
subscribers = (
@@ -309,6 +337,30 @@
found_subscribers.sorted)
+class TestBugSubscriptionInfoPermissions(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test(self):
+ bug = self.factory.makeBug()
+ info = bug.getSubscriptionInfo()
+ checker = getChecker(info)
+
+ # BugSubscriptionInfo objects are immutable.
+ self.assertEqual({}, checker.set_permissions)
+
+ # All attributes require launchpad.View.
+ permissions = set(checker.get_permissions.itervalues())
+ self.assertEqual(set(["launchpad.View"]), permissions)
+
+ # The security adapter for launchpad.View lets anyone reference the
+ # attributes unless the bug is private, in which case only explicit
+ # subscribers are permitted.
+ adapter = queryAdapter(info, IAuthorization, "launchpad.View")
+ self.assertIsInstance(
+ adapter, PublicToAllOrPrivateToExplicitSubscribersForBugTask)
+
+
class TestBugSubscriptionInfoQueries(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
