launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05051
[Merge] lp:~wallyworld/launchpad/bug-contact-auto-unsubscribe-672596 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-contact-auto-unsubscribe-672596 into lp:launchpad with lp:~wallyworld/launchpad/private-bug-subscriptions-854405 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #672596 in Launchpad itself: "When marking a security bug non-security, the security contact should be unsubscribed"
https://bugs.launchpad.net/launchpad/+bug/672596
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-contact-auto-unsubscribe-672596/+merge/76553
== Implementation ==
This branch unsubscribes bug supervisors when a bug is made public and security contacts when it is made non-security. Emails are sent to the affected people letting them know they have been unsubscribed.
The BugNotificationRecipients class was extended to add 2 new recipient types:
addBugSupervisor
addSecurityContact
The UnsubscribedFromBug bug change class was extended to optionally trigger sending of email notifications - normally unsubscribing from a bug doesn't send an email. But we need to be able to tell it to when we auto unsubscribe as a result of bug status change.
== Tests ==
Add 2 new tests to TestBugPrivateAndSecurityRelatedUpdatesMixin:
- test_bugSupervisorUnsubscribedIfBugMadePublic
- test_securityContactUnsubscribedIfBugNotSecurityRelated
Also tweak some existing doc tests.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/adapters/bugchange.py
lib/lp/bugs/doc/bugsubscription.txt
lib/lp/bugs/mail/bugnotificationrecipients.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/tests/test_bug.py
lib/lp/bugs/scripts/bugnotification.py
--
https://code.launchpad.net/~wallyworld/launchpad/bug-contact-auto-unsubscribe-672596/+merge/76553
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-contact-auto-unsubscribe-672596 into lp:launchpad.
=== modified file 'lib/lp/bugs/adapters/bugchange.py'
--- lib/lp/bugs/adapters/bugchange.py 2011-08-16 19:14:51 +0000
+++ lib/lp/bugs/adapters/bugchange.py 2011-09-22 11:14:28 +0000
@@ -179,9 +179,11 @@
class UnsubscribedFromBug(BugChangeBase):
"""A user got unsubscribed from a bug."""
- def __init__(self, when, person, unsubscribed_user):
+ def __init__(self, when, person, unsubscribed_user, **kwargs):
super(UnsubscribedFromBug, self).__init__(when, person)
self.unsubscribed_user = unsubscribed_user
+ self.send_notification = kwargs.get('send_notification', False)
+ self.notification_text = kwargs.get('notification_text')
def getBugActivity(self):
"""See `IBugChange`."""
@@ -191,7 +193,10 @@
def getBugNotification(self):
"""See `IBugChange`."""
- return None
+ if self.send_notification and self.notification_text:
+ return {'text': '** %s' % self.notification_text}
+ else:
+ return None
class BugConvertedToQuestion(BugChangeBase):
=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2011-09-22 11:14:27 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2011-09-22 11:14:28 +0000
@@ -430,20 +430,22 @@
>>> print_displayname(linux_source_bug.getDirectSubscribers())
Foo Bar
Mark Shuttleworth
- Robert Collins
- Ubuntu Team
>>> print_displayname(linux_source_bug.getIndirectSubscribers())
Martin Pitt
No Privileges Person
+ Robert Collins
Sample Person
Scott James Remnant
+ Ubuntu Team
>>> print_displayname(linux_source_bug.getAlsoNotifiedSubscribers())
Martin Pitt
No Privileges Person
+ Robert Collins
Sample Person
Scott James Remnant
+ Ubuntu Team
To find out which email addresses should receive a notification email on
a bug, and why, IBug.getBugNotificationRecipients() assembles an
@@ -458,8 +460,8 @@
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
('no-priv@xxxxxxxxxxxxx', u'Subscriber (linux-source-2.6.15 in Ubuntu)'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
- ('support@xxxxxxxxxx', u'Subscriber @ubuntu-team'),
+ ('robertc@xxxxxxxxxxxxxxxxx', u'Subscriber (Mozilla Firefox)'),
+ ('support@xxxxxxxxxx', u'Subscriber (Ubuntu) @ubuntu-team'),
('test@xxxxxxxxxxxxx', 'Assignee')]
If IBug.getBugNotificationRecipients() is passed a BugNotificationLevel
@@ -472,8 +474,8 @@
>>> [(address, recipients.getReason(address)[1]) for address in addresses]
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
- ('support@xxxxxxxxxx', u'Subscriber @ubuntu-team'),
+ ('robertc@xxxxxxxxxxxxxxxxx', u'Subscriber (Mozilla Firefox)'),
+ ('support@xxxxxxxxxx', u'Subscriber (Ubuntu) @ubuntu-team'),
('test@xxxxxxxxxxxxx', 'Assignee')]
When Sample Person is unsubscribed from linux_source_bug, he is no
@@ -487,8 +489,8 @@
>>> [(address, recipients.getReason(address)[1]) for address in addresses]
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
- ('support@xxxxxxxxxx', u'Subscriber @ubuntu-team'),
+ ('robertc@xxxxxxxxxxxxxxxxx', u'Subscriber (Mozilla Firefox)'),
+ ('support@xxxxxxxxxx', u'Subscriber (Ubuntu) @ubuntu-team'),
('test@xxxxxxxxxxxxx', 'Assignee')]
...but remains included for the level LIFECYCLE.
@@ -501,8 +503,8 @@
[('foo.bar@xxxxxxxxxxxxx', 'Subscriber'),
('mark@xxxxxxxxxxx', 'Subscriber'),
('no-priv@xxxxxxxxxxxxx', u'Subscriber (linux-source-2.6.15 in Ubuntu)'),
- ('robertc@xxxxxxxxxxxxxxxxx', 'Subscriber'),
- ('support@xxxxxxxxxx', u'Subscriber @ubuntu-team'),
+ ('robertc@xxxxxxxxxxxxxxxxx', u'Subscriber (Mozilla Firefox)'),
+ ('support@xxxxxxxxxx', u'Subscriber (Ubuntu) @ubuntu-team'),
('test@xxxxxxxxxxxxx', 'Assignee')]
To find out if someone is already directly subscribed to a bug, call
=== modified file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py 2011-05-31 19:45:53 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py 2011-09-22 11:14:28 +0000
@@ -107,6 +107,28 @@
text = "are a bug assignee"
self._addReason(person, text, reason)
+ def addBugSupervisor(self, person):
+ """Registers a bug supervisor of a bugtask's pillar of this bug."""
+ reason = "Bug Supervisor"
+ if person.isTeam():
+ text = ("are a member of %s, which is a bug supervisor"
+ % person.displayname)
+ reason += " @%s" % person.name
+ else:
+ text = "are a bug supervisor"
+ self._addReason(person, text, reason)
+
+ def addSecurityContact(self, person):
+ """Registers a security contact of a bugtask's pillar of this bug."""
+ reason = "Security Contact"
+ if person.isTeam():
+ text = ("are a member of %s, which is a security contact"
+ % person.displayname)
+ reason += " @%s" % person.name
+ else:
+ text = "are a security contact"
+ self._addReason(person, text, reason)
+
def addStructuralSubscriber(self, person, target):
"""Registers a structural subscriber to this bug's target."""
reason = "Subscriber (%s)" % target.displayname
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-09-22 11:14:27 +0000
+++ lib/lp/bugs/model/bug.py 2011-09-22 11:14:28 +0000
@@ -822,6 +822,7 @@
person = unsubscribed_by
ignore_permissions = kwargs.get('ignore_permissions', False)
+ recipients = kwargs.get('recipients')
for sub in self.subscriptions:
if sub.person.id == person.id:
if (not ignore_permissions
@@ -832,8 +833,9 @@
person.displayname))
self.addChange(UnsubscribedFromBug(
- when=UTC_NOW, person=unsubscribed_by,
- unsubscribed_user=person))
+ when=UTC_NOW, person=unsubscribed_by,
+ unsubscribed_user=person, **kwargs),
+ recipients=recipients)
store = Store.of(sub)
store.remove(sub)
# Make sure that the subscription removal has been
@@ -1740,6 +1742,29 @@
result.add(who)
return result
+ def getAutoRemovedSubscribers(self, for_private, for_security_related):
+ """Return the to be removed subscribers for bug with given attributes.
+
+ When a bug's privacy or security related attributes change, some
+ existing subscribers may need to be automatically removed.
+ The rules are:
+ security=false ->
+ auto removed subscribers = (bugtask security contacts)
+ privacy=false ->
+ auto removed subscribers = (bugtask bug supervisors)
+
+ """
+ bug_supervisors = []
+ security_contacts = []
+ for pillar in self.affected_pillars:
+ if (self.security_related and not for_security_related
+ and pillar.security_contact):
+ security_contacts.append(pillar.security_contact)
+ if (self.private and not for_private
+ and pillar.bug_supervisor):
+ bug_supervisors.append(pillar.bug_supervisor)
+ return bug_supervisors, security_contacts
+
def reconcileSubscribers(self, for_private, for_security_related, who):
""" Ensure only appropriate people are subscribed to private bugs.
@@ -1761,6 +1786,28 @@
self.getSubscriptionInfo().direct_subscribers)
required_subscribers = self.getRequiredSubscribers(
for_private, for_security_related, who)
+ removed_bug_supervisors, removed_security_contacts = (
+ self.getAutoRemovedSubscribers(for_private, for_security_related))
+ for subscriber in removed_bug_supervisors:
+ recipients = BugNotificationRecipients()
+ recipients.addBugSupervisor(subscriber)
+ notification_text = ("This bug is no longer private so you will "
+ "no longer be notified of changes.")
+ self.unsubscribe(
+ subscriber, who, ignore_permissions=True,
+ send_notification=True,
+ notification_text=notification_text,
+ recipients=recipients)
+ for subscriber in removed_security_contacts:
+ recipients = BugNotificationRecipients()
+ recipients.addSecurityContact(subscriber)
+ notification_text = ("This bug is no longer security related so "
+ "you will no longer be notified of changes.")
+ self.unsubscribe(
+ subscriber, who, ignore_permissions=True,
+ send_notification=True,
+ notification_text=notification_text,
+ recipients=recipients)
# If this bug is for a project that is marked as having private bugs
# by default, and the bug is private or security related, we will
=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py 2011-09-22 11:14:27 +0000
+++ lib/lp/bugs/model/tests/test_bug.py 2011-09-22 11:14:28 +0000
@@ -1,7 +1,5 @@
# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-from canonical.launchpad.interfaces.lpstorm import IStore
-from lp.bugs.model.bugnotification import BugNotificationRecipient
__metaclass__ = type
@@ -10,11 +8,14 @@
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.bugs.enum import (
BugNotificationLevel,
BugNotificationStatus,
)
+from lp.bugs.model.bugnotification import BugNotificationRecipient
+from lp.bugs.scripts.bugnotification import get_email_notifications
from lp.bugs.interfaces.bugnotification import IBugNotificationSet
from lp.bugs.interfaces.bugtask import BugTaskStatus
from lp.bugs.model.bug import (
@@ -496,20 +497,22 @@
# Used with the various setPrivateAndSecurityRelated tests to create
# a bug and add some initial subscribers.
bug_owner = self.factory.makePerson(name='bugowner')
- bug_supervisor = self.factory.makePerson(name='bugsupervisor')
+ bug_supervisor = self.factory.makePerson(
+ name='bugsupervisor', email='bugsupervisor@xxxxxxxxxxx')
product_owner = self.factory.makePerson(name='productowner')
bug_product = self.factory.makeProduct(
owner=product_owner, bug_supervisor=bug_supervisor)
if self.private_project:
removeSecurityProxy(bug_product).private_bugs = True
- bug = self.factory.makeBug(
- owner=bug_owner,
- product=bug_product,
- private=private_security_related,
- security_related=private_security_related)
+ bug = self.factory.makeBug(owner=bug_owner, product=bug_product)
+ with person_logged_in(bug_owner):
+ bug.setPrivacyAndSecurityRelated(
+ private_security_related, private_security_related, bug_owner)
owner_a = self.factory.makePerson(name='ownera')
- security_contact_a = self.factory.makePerson(name='securitycontacta')
- bug_supervisor_a = self.factory.makePerson(name='bugsupervisora')
+ security_contact_a = self.factory.makePerson(
+ name='securitycontacta', email='securitycontacta@xxxxxxxxxxx')
+ bug_supervisor_a = self.factory.makePerson(
+ name='bugsupervisora', email='bugsupervisora@xxxxxxxxxxx')
driver_a = self.factory.makePerson(name='drivera')
product_a = self.factory.makeProduct(
owner=owner_a,
@@ -517,7 +520,8 @@
bug_supervisor=bug_supervisor_a,
driver=driver_a)
owner_b = self.factory.makePerson(name='ownerb')
- security_contact_b = self.factory.makePerson(name='securitycontactb')
+ security_contact_b = self.factory.makePerson(
+ name='securitycontactb', email='securitycontactb@xxxxxxxxxxx')
product_b = self.factory.makeProduct(
owner=owner_b,
security_contact=security_contact_b)
@@ -652,7 +656,10 @@
bug.setPrivacyAndSecurityRelated(
private=False, security_related=False, who=who)
subscribers = set(bug.getDirectSubscribers())
- expected_direct_subscribers.remove(bugtask_a.pillar.security_contact)
+ expected_direct_subscribers.difference_update(
+ (default_bugtask.pillar.security_contact,
+ default_bugtask.pillar.bug_supervisor,
+ bugtask_a.pillar.security_contact))
self.assertContentEqual(expected_direct_subscribers, subscribers)
def test_setPillarOwnerSubscribedIfNoBugSupervisor(self):
@@ -687,13 +694,110 @@
subscribers
)
- def _fetch_notifications(self, bug, header, body):
- return IStore(BugNotification).find(
+ def _fetch_notifications(self, bug, reason_header):
+ store = IStore(BugNotification)
+ return store.find(
BugNotification,
- BugNotification.id == BugNotificationRecipient.bug_notificationID,
- BugNotificationRecipient.reason_header == header,
- BugNotificationRecipient.reason_body == body,
- BugNotification.bug == bug)
+ BugNotificationRecipient.bug_notificationID == BugNotification.id,
+ BugNotificationRecipient.reason_header == reason_header,
+ BugNotification.bug == bug,
+ BugNotification.status == BugNotificationStatus.PENDING,
+ BugNotification.date_emailed == None)
+
+ def _check_email_content(self, message, expected_headers,
+ expected_body_text):
+ # Ensure that the email header values and message body text is as
+ # expected.
+ headers = {}
+ for header in ['X-Launchpad-Message-Rationale',
+ 'X-Launchpad-Bug-Security-Vulnerability',
+ 'X-Launchpad-Bug-Private']:
+ if message[header]:
+ headers[header] = message[header]
+ self.assertEqual(expected_headers, headers)
+ body_text = message.get_payload(decode=True)
+ self.assertTrue(expected_body_text in body_text)
+
+ def _check_notifications(self, bug, expected_recipients,
+ expected_body_text, expected_reason_body,
+ is_private, is_security_related, role):
+ # Ensure that the content of the pending email notifications is
+ # correct.
+ notifications = self._fetch_notifications(bug, role)
+ actual_recipients = []
+ email_notifications = get_email_notifications(notifications)
+ for bug_notifications, omitted, messages in email_notifications:
+ for message in messages:
+ expected_headers = {
+ 'X-Launchpad-Bug-Private':
+ 'yes' if is_private else 'no',
+ 'X-Launchpad-Bug-Security-Vulnerability':
+ 'yes' if is_security_related else 'no',
+ 'X-Launchpad-Message-Rationale': role,
+ }
+ self._check_email_content(
+ message, expected_headers, expected_body_text)
+ expected_reason_header = role
+ for notification in bug_notifications:
+ for recipient in notification.recipients:
+ self.assertEqual(
+ expected_reason_header, recipient.reason_header)
+ self.assertEqual(
+ expected_reason_body, recipient.reason_body)
+ actual_recipients.append(recipient.person)
+ self.assertContentEqual(expected_recipients, actual_recipients)
+
+ def test_bugSupervisorUnsubscribedIfBugMadePublic(self):
+ # The bug supervisors are unsubscribed if a bug is made public and an
+ # email is sent telling them they have been unsubscribed.
+
+ (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
+ self.createBugTasksAndSubscribers(private_security_related=True))
+
+ with person_logged_in(bug_owner):
+ bug.subscribe(bugtask_a.pillar.bug_supervisor, bug_owner)
+ who = self.factory.makePerson(name="who")
+ bug.setPrivacyAndSecurityRelated(
+ private=False, security_related=True, who=who)
+ subscribers = bug.getDirectSubscribers()
+ self.assertFalse(bugtask_a.pillar.bug_supervisor in subscribers)
+
+ expected_recipients = [
+ default_bugtask.pillar.bug_supervisor,
+ bugtask_a.pillar.bug_supervisor,
+ ]
+ expected_body_text = '** This bug is no longer private'
+ expected_reason_body = ('You received this bug notification '
+ 'because you are a bug supervisor.')
+ self._check_notifications(
+ bug, expected_recipients, expected_body_text,
+ expected_reason_body, False, True, 'Bug Supervisor')
+
+ def test_securityContactUnsubscribedIfBugNotSecurityRelated(self):
+ # The security contacts are unsubscribed if a bug has security_related
+ # set to false and an email is sent telling them they have been
+ # unsubscribed.
+
+ (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
+ self.createBugTasksAndSubscribers(private_security_related=True))
+
+ with person_logged_in(bug_owner):
+ bug.subscribe(bugtask_a.pillar.security_contact, bug_owner)
+ who = self.factory.makePerson(name="who")
+ bug.setPrivacyAndSecurityRelated(
+ private=True, security_related=False, who=who)
+ subscribers = bug.getDirectSubscribers()
+ self.assertFalse(bugtask_a.pillar.security_contact in subscribers)
+
+ expected_recipients = [
+ bugtask_a.pillar.security_contact,
+ ]
+ expected_body_text = '** This bug is no longer security related'
+ expected_reason_body = ('You received this bug notification '
+ 'because you are a security contact.')
+ self._check_notifications(
+ bug, expected_recipients, expected_body_text,
+ expected_reason_body, True, False, 'Security Contact')
class TestBugPrivateAndSecurityRelatedUpdatesPrivateProject(
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py 2011-08-16 02:08:25 +0000
+++ lib/lp/bugs/scripts/bugnotification.py 2011-09-22 11:14:28 +0000
@@ -105,7 +105,8 @@
key = get_activity_key(notification)
if (notification.is_comment or
key is None or
- old_values[key] != new_values[key]):
+ old_values[key] != new_values[key] or
+ (old_values[key] is None and new_values[key] is None)):
# We will report this notification.
filtered_notifications.append(notification)
for subscription_source in notification.recipients:
