launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01505
[Merge] lp:~gmb/launchpad/make-mailnotification-care-bug-656759 into lp:launchpad/devel
Graham Binns has proposed merging lp:~gmb/launchpad/make-mailnotification-care-bug-656759 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#656759 Bug.getBugNotificationRecipients() should be aware of bug_notification_level
https://bugs.launchpad.net/bugs/656759
This branch makes it possible to have direct (and from-duplicates) bug subscribers for a given BugNotificationLevel.
I've made the following changes:
== lib/lp/bugs/doc/bugsubscription.txt ==
- I've added a test to cover subscribing with a given BugNotificationLevel.
== lib/lp/bugs/interfaces/bug.py ==
- I've updated the method declarations necessary to accomodate the new functionality.
== lib/lp/bugs/model/bug.py ==
- I've updated subscribe(), getDirectSubscribers() and getSubscribersFromDuplicates() so that they now accept a `level` argument. In the latter two this is used to filter the results returned from the database.
== lib/lp/bugs/model/bugsubscription.py ==
- I've updated BugSubscription.__init__() to accept a BugNotificationLevel.
== lib/lp/bugs/tests/test_bug.py ==
- I've added tests for the changes to getDirectSubscribers() and getSubscribersFromDuplicates() on IBug.
--
https://code.launchpad.net/~gmb/launchpad/make-mailnotification-care-bug-656759/+merge/38404
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/make-mailnotification-care-bug-656759 into lp:launchpad/devel.
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2010-10-06 18:53:53 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2010-10-14 10:21:16 +0000
@@ -555,6 +555,18 @@
>>> print subscription.bug_notification_level.name
COMMENTS
+It's possible to subscribe to a bug at a different BugNotificationLevel
+by passing a `level` parameter to subscribe().
+
+ >>> metadata_subscriber = factory.makePerson()
+ >>> metadata_subscribed_bug = factory.makeBug()
+ >>> metadata_subscription = metadata_subscribed_bug.subscribe(
+ ... metadata_subscriber, metadata_subscriber,
+ ... level=BugNotificationLevel.METADATA)
+
+ >>> print metadata_subscription.bug_notification_level.name
+ METADATA
+
To unsubscribe from all dupes for a bug, call
IBug.unsubscribeFromDupes. This is useful because direct subscribers
from dupes are automatically subscribed to dupe targets, so we provide
=== 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-14 10:21:16 +0000
@@ -81,6 +81,7 @@
from lp.bugs.interfaces.bugwatch import IBugWatch
from lp.bugs.interfaces.cve import ICve
from lp.code.interfaces.branchlink import IHasLinkedBranches
+from lp.registry.enum import BugNotificationLevel
from lp.registry.interfaces.mentoringoffer import ICanBeMentored
from lp.registry.interfaces.person import IPerson
from lp.services.fields import (
@@ -428,12 +429,13 @@
person=Reference(IPerson, title=_('Person'), required=True))
@call_with(subscribed_by=REQUEST_USER, suppress_notify=False)
@export_write_operation()
- def subscribe(person, subscribed_by, suppress_notify=True):
+ def subscribe(person, subscribed_by, suppress_notify=True, level=None):
"""Subscribe `person` to the bug.
:param person: the subscriber.
:param subscribed_by: the person who created the subscription.
:param suppress_notify: a flag to suppress notify call.
+ :param level: The BugNotificationLevel for the new subscription.
:return: an `IBugSubscription`.
"""
@@ -470,13 +472,13 @@
def getDirectSubscriptions():
"""A sequence of IBugSubscriptions directly linked to this bug."""
- def getDirectSubscribers():
+ def getDirectSubscribers(recipients=None, level=None):
"""A list of IPersons that are directly subscribed to this bug.
Direct subscribers have an entry in the BugSubscription table.
"""
- def getIndirectSubscribers():
+ def getIndirectSubscribers(recipients=None, level=None):
"""Return IPersons that are indirectly subscribed to this bug.
Indirect subscribers get bugmail, but don't have an entry in the
=== 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-14 10:21:16 +0000
@@ -717,7 +717,8 @@
"""See `IBug`."""
return self.latest_patch_uploaded is not None
- def subscribe(self, person, subscribed_by, suppress_notify=True):
+ def subscribe(self, person, subscribed_by, suppress_notify=True,
+ level=BugNotificationLevel.COMMENTS):
"""See `IBug`."""
# first look for an existing subscription
for sub in self.subscriptions:
@@ -725,7 +726,8 @@
return sub
sub = BugSubscription(
- bug=self, person=person, subscribed_by=subscribed_by)
+ bug=self, person=person, subscribed_by=subscribed_by,
+ bug_notification_level=level)
# Ensure that the subscription has been flushed.
Store.of(sub).flush()
@@ -819,7 +821,7 @@
BugSubscription.bug == self).order_by(
Func('person_sort_key', Person.displayname, Person.name))
- def getDirectSubscribers(self, recipients=None):
+ def getDirectSubscribers(self, recipients=None, level=None):
"""See `IBug`.
The recipients argument is private and not exposed in the
@@ -827,10 +829,15 @@
the relevant subscribers and rationales will be registered on
it.
"""
+ if level is None:
+ level = BugNotificationLevel.COMMENTS
+
subscribers = list(
Person.select("""
- Person.id = BugSubscription.person AND
- BugSubscription.bug = %d""" % self.id,
+ Person.id = BugSubscription.person
+ AND BugSubscription.bug = %s
+ AND BugSubscription.bug_notification_level >= %s""" %
+ sqlvalues(self.id, level),
orderBy="displayname", clauseTables=["BugSubscription"]))
if recipients is not None:
for subscriber in subscribers:
@@ -884,11 +891,17 @@
if self.private:
return []
+ if level is None:
+ notification_level = BugNotificationLevel.COMMENTS
+ 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)
@@ -897,7 +910,7 @@
# 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(level=level))
+ dupe_subscribers -= set(self.getAlsoNotifiedSubscribers())
if recipients is not None:
for subscriber in dupe_subscribers:
=== modified file 'lib/lp/bugs/model/bugsubscription.py'
--- lib/lp/bugs/model/bugsubscription.py 2010-09-28 21:31:50 +0000
+++ lib/lp/bugs/model/bugsubscription.py 2010-10-14 10:21:16 +0000
@@ -50,11 +50,13 @@
"subscribed_by", allow_none=False, validator=validate_person)
subscribed_by = Reference(subscribed_by_id, "Person.id")
- def __init__(self, bug=None, person=None, subscribed_by=None):
+ def __init__(self, bug=None, person=None, subscribed_by=None,
+ bug_notification_level=BugNotificationLevel.COMMENTS):
super(BugSubscription, self).__init__()
self.bug = bug
self.person = person
self.subscribed_by = subscribed_by
+ self.bug_notification_level = bug_notification_level
@property
def display_subscribed_by(self):
=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py 2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/tests/test_bug.py 2010-10-14 10:21:16 +0000
@@ -6,6 +6,7 @@
__metaclass__ = type
from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.enum import BugNotificationLevel
from lp.registry.interfaces.person import PersonVisibility
from lp.registry.model.structuralsubscription import StructuralSubscription
from lp.testing import (
@@ -125,3 +126,128 @@
dupe_bug.subscribe(team, member)
dupe_bug.markAsDuplicate(bug)
self.assertTrue(team in bug.getSubscribersFromDuplicates())
+
+ def test_subscribe_with_level(self):
+ # It's possible to subscribe to a bug at a different
+ # BugNotificationLevel by passing a `level` parameter to
+ # subscribe().
+ bug = self.factory.makeBug()
+ for level in BugNotificationLevel.items:
+ subscriber = self.factory.makePerson()
+ with person_logged_in(subscriber):
+ subscription = bug.subscribe(
+ subscriber, subscriber, level=level)
+ self.assertEqual(level, subscription.bug_notification_level)
+
+ def test_get_direct_subscribers_with_level(self):
+ # It's possible to pass a level parameter to
+ # getDirectSubscribers() to filter the subscribers returned.
+ # When a `level` is passed to getDirectSubscribers(), the
+ # subscribers returned will be those of that level of
+ # subscription or higher.
+ bug = self.factory.makeBug()
+ # We unsubscribe the bug's owner because if we don't there will
+ # be two COMMENTS-level subscribers.
+ with person_logged_in(bug.owner):
+ bug.unsubscribe(bug.owner, bug.owner)
+ notification_levels = sorted(BugNotificationLevel.items)
+ reversed_levels = reversed(notification_levels)
+ subscribers = []
+ for level in reversed_levels:
+ subscriber = self.factory.makePerson()
+ subscribers.append(subscriber)
+ with person_logged_in(subscriber):
+ subscription = bug.subscribe(
+ subscriber, subscriber, level=level)
+ direct_subscribers = bug.getDirectSubscribers(level=level)
+
+ # All the previous subscribers will be included because
+ # their level of subscription is such that they also receive
+ # notifications at the current level.
+ for subscriber in subscribers:
+ self.assertTrue(
+ subscriber in direct_subscribers,
+ "Subscriber at level %s is not in direct_subscribers." %
+ level.name)
+ self.assertEqual(
+ len(subscribers), len(direct_subscribers),
+ "Number of subscribers did not match expected value.")
+
+ def test_get_direct_subscribers_default_level(self):
+ # If no `level` parameter is passed to getDirectSubscribers(),
+ # the assumed `level` is BugNotification.COMMENTS.
+ bug = self.factory.makeBug()
+ level = BugNotificationLevel.COMMENTS
+ subscriber = self.factory.makePerson()
+ with person_logged_in(subscriber):
+ subscription = bug.subscribe(
+ subscriber, subscriber, level=level)
+ direct_subscribers = bug.getDirectSubscribers()
+ self.assertTrue(
+ bug.owner in direct_subscribers,
+ "Bug owner is not in direct_subscribers to COMMENTS.")
+ self.assertTrue(
+ subscriber in direct_subscribers,
+ "Subscriber at level COMMENTS is not in direct_subscribers.")
+ self.assertEqual(
+ 2, len(direct_subscribers),
+ "There should be two direct subscribers to COMMENTS.")
+
+ def test_subscribers_from_dupes_uses_level(self):
+ # When getSubscribersFromDuplicates() is passed a `level`
+ # parameter it will include only subscribers subscribed to
+ # duplicates at that BugNotificationLevel or higher.
+ bug = self.factory.makeBug()
+ duplicate_bug = self.factory.makeBug()
+ with person_logged_in(duplicate_bug.owner):
+ duplicate_bug.markAsDuplicate(bug)
+ # We unsubscribe the owner of the duplicate to avoid muddling
+ # the results retuned by getSubscribersFromDuplicates()
+ duplicate_bug.unsubscribe(
+ duplicate_bug.owner, duplicate_bug.owner)
+ notification_levels = sorted(BugNotificationLevel.items)
+ reversed_levels = reversed(notification_levels)
+ subscribers = []
+ for level in reversed_levels:
+ subscriber = self.factory.makePerson()
+ subscribers.append(subscriber)
+ with person_logged_in(subscriber):
+ subscription = duplicate_bug.subscribe(
+ subscriber, subscriber, level=level)
+ duplicate_subscribers = (
+ bug.getSubscribersFromDuplicates(level=level))
+ # All the previous subscribers will be included because
+ # their level of subscription is such that they also receive
+ # notifications at the current level.
+ for subscriber in subscribers:
+ self.assertTrue(
+ subscriber in duplicate_subscribers,
+ "Subscriber at level %s is not in "
+ "duplicate_subscribers." % level.name)
+ self.assertEqual(
+ len(subscribers), len(duplicate_subscribers),
+ "Number of subscribers did not match expected value.")
+
+ def test_subscribers_from_dupes_overrides_using_level(self):
+ # Bug.getSubscribersFromDuplicates() does not return subscribers
+ # who also have a direct subscription to the master bug provided
+ # that the subscription to the master bug is of the same level
+ # or higher as the subscription to the duplicate.
+ bug = self.factory.makeBug()
+ duplicate_bug = self.factory.makeBug()
+ with person_logged_in(duplicate_bug.owner):
+ duplicate_bug.markAsDuplicate(bug)
+ # We unsubscribe the owner of the duplicate to avoid muddling
+ # the results retuned by getSubscribersFromDuplicates()
+ duplicate_bug.unsubscribe(
+ duplicate_bug.owner, duplicate_bug.owner)
+ subscriber = self.factory.makePerson()
+ with person_logged_in(subscriber):
+ direct_subscription = bug.subscribe(
+ subscriber, subscriber, level=BugNotificationLevel.NOTHING)
+ dupe_subscription = duplicate_bug.subscribe(
+ subscriber, subscriber, level=BugNotificationLevel.METADATA)
+ duplicate_subscribers = bug.getSubscribersFromDuplicates()
+ self.assertTrue(
+ subscriber not in duplicate_subscribers,
+ "Subscriber should not be in duplicate_subscribers.")
