launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05171
[Merge] lp:~wallyworld/launchpad/reassign-private-bug-notify-52915 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/reassign-private-bug-notify-52915 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #52915 in Launchpad itself: "Reassigning a private bug to a different product doesn't notify either the new product maintainer or security contact"
https://bugs.launchpad.net/launchpad/+bug/52915
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/reassign-private-bug-notify-52915/+merge/78191
The bug implies that neither project maintainer nor security contact are notified when a private bugtask is re-targetted. For security related bugs, the security contact is automatically subscribed so nothing needs doing for the security contact role.
The email that is sent is the standard "This bugtask has been reassigned" email. The recipient can choose to subscribe themselves if they so wish.
== Implementation ==
Ensure that when a private bugtask is re-targetted to a new project, the project's maintainer and/or bug supervisor is notified. The current business rules allow project maintainers to see private bugs so maintainers always get notified. Bug supervisors only get notified if 1. they can see the bug, and 2. they are not the same person as the maintainer.
A new notification recipient category 'Maintainer' was introduced. This allows maintainers to get emails along the lines of "You received this because you are the maintainer of xxxx". Bug supervisors already have their own email recipient category.
== Tests ==
Add a new test to test_bugchanges:
- test_retarget_private_security_bug_to_product
This tests the various permutations of re-targetting a bug with/without bug supervisor being used etc.
As a drive-by, there were a bunch of asserts which were the wrong way around
ie self.assertEqual(observed, expected) instead of self.assertEqual(expected, observed)
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/configure.zcml
lib/lp/bugs/subscribers/bug.py
lib/lp/bugs/tests/test_bugchanges.py
--
https://code.launchpad.net/~wallyworld/launchpad/reassign-private-bug-notify-52915/+merge/78191
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/reassign-private-bug-notify-52915 into lp:launchpad.
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-09-23 22:38:32 +0000
+++ lib/lp/bugs/configure.zcml 2011-10-05 04:29:25 +0000
@@ -1113,7 +1113,7 @@
<!-- BugNotificationRecipients provides the following
attributes/methods in addition. -->
<allow
- attributes="subscription_filters addFilter"/>
+ attributes="subscription_filters addFilter addBugSupervisor addMaintainer"/>
</class>
<securedutility
provides="lp.bugs.interfaces.bugnotification.IBugNotificationSet"
=== modified file 'lib/lp/bugs/mail/bugnotificationrecipients.py'
--- lib/lp/bugs/mail/bugnotificationrecipients.py 2011-09-22 10:32:41 +0000
+++ lib/lp/bugs/mail/bugnotificationrecipients.py 2011-10-05 04:29:25 +0000
@@ -129,6 +129,17 @@
text = "are a security contact"
self._addReason(person, text, reason)
+ def addMaintainer(self, person):
+ """Registers a maintainer of a bugtask's pillar of this bug."""
+ reason = "Maintainer"
+ if person.isTeam():
+ text = ("are a member of %s, which is a maintainer"
+ % person.displayname)
+ reason += " @%s" % person.name
+ else:
+ text = "are a maintainer"
+ 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/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py 2011-09-21 13:27:17 +0000
+++ lib/lp/bugs/subscribers/bug.py 2011-10-05 04:29:25 +0000
@@ -22,6 +22,7 @@
from canonical.launchpad.webapp.publisher import canonical_url
from lp.bugs.adapters.bugchange import (
BugDuplicateChange,
+ BugTaskTargetChange,
BugTaskAssigneeChange,
get_bug_changes,
)
@@ -32,6 +33,7 @@
from lp.bugs.mail.newbug import generate_bug_add_email
from lp.bugs.model.bug import get_also_notified_subscribers
from lp.registry.interfaces.person import IPerson
+from lp.registry.interfaces.product import IProduct
from lp.services.mail.sendmail import (
format_address,
sendmail,
@@ -171,6 +173,28 @@
level=change.change_level,
include_master_dupe_subscribers=False))
recipients.update(change_recipients)
+ # Additionally, if we are re-targetting a bugtask for a private
+ # bug, we need to ensure the new bug supervisor and maintainer are
+ # notified (if they can view the bug).
+ # If they are the same person, only send one notification.
+ if (isinstance(change, BugTaskTargetChange) and
+ old_bugtask is not None and bug_delta.bug.private):
+ bugtask_deltas = bug_delta.bugtask_deltas
+ if not isinstance(bugtask_deltas, (list, tuple)):
+ bugtask_deltas = [bugtask_deltas]
+ for bugtask_delta in bugtask_deltas:
+ if not bugtask_delta.target:
+ continue
+ new_target = bugtask_delta.bugtask.target
+ if not new_target or not IProduct.providedBy(new_target):
+ continue
+ if bug_delta.bug.userCanView(new_target.owner):
+ recipients.addMaintainer(new_target.owner)
+ if (new_target.bug_supervisor and not
+ new_target.bug_supervisor.inTeam(new_target.owner) and
+ bug_delta.bug.userCanView(new_target.bug_supervisor)):
+ recipients.addBugSupervisor(
+ new_target.bug_supervisor)
bug_delta.bug.addChange(change, recipients=recipients)
=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py 2011-09-29 20:42:57 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py 2011-10-05 04:29:25 +0000
@@ -128,57 +128,64 @@
new_notifications = self.getNewNotifications(bug)
if expected_activity is None:
- self.assertEqual(len(new_activities), 0)
+ self.assertEqual(0, len(new_activities))
else:
if isinstance(expected_activity, dict):
expected_activities = [expected_activity]
else:
expected_activities = expected_activity
- self.assertEqual(len(new_activities), len(expected_activities))
+ self.assertEqual(len(expected_activities), len(new_activities))
for expected_activity in expected_activities:
added_activity = new_activities.pop(0)
self.assertEqual(
- added_activity.person, expected_activity['person'])
- self.assertEqual(
- added_activity.whatchanged,
- expected_activity['whatchanged'])
- self.assertEqual(
- added_activity.oldvalue,
- expected_activity.get('oldvalue'))
- self.assertEqual(
- added_activity.newvalue,
- expected_activity.get('newvalue'))
- self.assertEqual(
- added_activity.message, expected_activity.get('message'))
+ expected_activity['person'], added_activity.person)
+ self.assertEqual(
+ expected_activity['whatchanged'],
+ added_activity.whatchanged)
+ self.assertEqual(
+ expected_activity.get('oldvalue'),
+ added_activity.oldvalue)
+ self.assertEqual(
+ expected_activity.get('newvalue'),
+ added_activity.newvalue)
+ self.assertEqual(
+ expected_activity.get('message'), added_activity.message)
if expected_notification is None:
- self.assertEqual(len(new_notifications), 0)
+ self.assertEqual(0, len(new_notifications))
else:
if isinstance(expected_notification, dict):
expected_notifications = [expected_notification]
else:
expected_notifications = expected_notification
self.assertEqual(
- len(new_notifications), len(expected_notifications))
+ len(expected_notifications), len(new_notifications))
for expected_notification in expected_notifications:
added_notification = new_notifications.pop(0)
self.assertEqual(
- added_notification.message.text_contents,
- expected_notification['text'])
- self.assertEqual(
- added_notification.message.owner,
- expected_notification['person'])
- self.assertEqual(
- added_notification.is_comment,
- expected_notification.get('is_comment', False))
+ expected_notification['text'],
+ added_notification.message.text_contents)
+ self.assertEqual(
+ expected_notification['person'],
+ added_notification.message.owner)
+ self.assertEqual(
+ expected_notification.get('is_comment', False),
+ added_notification.is_comment)
expected_recipients = expected_notification.get('recipients')
+ expected_recipient_reasons = (
+ expected_notification.get('recipient_reasons'))
if expected_recipients is None:
expected_recipients = bug.getBugNotificationRecipients(
level=BugNotificationLevel.METADATA)
self.assertEqual(
+ set(expected_recipients),
set(recipient.person
- for recipient in added_notification.recipients),
- set(expected_recipients))
+ for recipient in added_notification.recipients))
+ if expected_recipient_reasons:
+ self.assertEqual(
+ set(expected_recipient_reasons),
+ set(recipient.reason_header
+ for recipient in added_notification.recipients))
def assertRecipients(self, expected_recipients):
notifications = self.getNewNotifications()
@@ -1011,6 +1018,82 @@
expected_activity=expected_activity,
expected_notification=expected_notification)
+ def _test_retarget_private_security_bug_to_product(self,
+ bug, maintainer,
+ bug_supervisor=None):
+ # When a private security related bug has a bugtask retargetted to a
+ # different product, a notification is sent to the new bug supervisor
+ # and maintainer. If they are the same person, only one notification
+ # is sent. They only get notifications if they can see the bug.
+
+ # Create the private bug.
+ bug_task = bug.bugtasks[0]
+ new_target = self.factory.makeProduct(
+ owner=maintainer, bug_supervisor=bug_supervisor)
+ self.saveOldChanges(bug)
+
+ bug_task_before_modification = Snapshot(
+ bug_task, providing=providedBy(bug_task))
+ bug_task.transitionToTarget(new_target)
+ notify(ObjectModifiedEvent(
+ bug_task, bug_task_before_modification,
+ ['target', 'product'], user=self.user))
+
+ expected_activity = {
+ 'person': self.user,
+ 'whatchanged': 'affects',
+ 'oldvalue': bug_task_before_modification.bugtargetname,
+ 'newvalue': bug_task.bugtargetname,
+ }
+
+ expected_recipients = [self.user]
+ expected_reasons = ['Subscriber']
+ if bug.userCanView(maintainer):
+ expected_recipients.append(maintainer)
+ expected_reasons.append('Maintainer')
+ if (bug_supervisor and not bug_supervisor.inTeam(maintainer)
+ and bug.userCanView(bug_supervisor)):
+ expected_recipients.append(bug_supervisor)
+ expected_reasons.append('Bug Supervisor')
+ expected_notification = {
+ 'text': u"** Project changed: %s => %s" % (
+ bug_task_before_modification.bugtargetname,
+ bug_task.bugtargetname),
+ 'person': self.user,
+ 'recipients': expected_recipients,
+ 'recipient_reasons': expected_reasons
+ }
+ self.assertRecordedChange(
+ expected_activity=expected_activity,
+ expected_notification=expected_notification, bug=bug)
+
+ def test_retarget_private_security_bug_to_product(self):
+ # A series of tests for re-targetting a private bug task.
+ bug = self.factory.makeBug(
+ product=self.product, owner=self.user, private=True)
+ maintainer = self.factory.makePerson()
+ bug_supervisor = self.factory.makePerson()
+
+ # Test with no bug supervisor
+ self._test_retarget_private_security_bug_to_product(bug, maintainer)
+ # Test with bug supervisor = maintainer.
+ self._test_retarget_private_security_bug_to_product(
+ bug, maintainer, maintainer)
+ # Test with different bug supervisor
+ self._test_retarget_private_security_bug_to_product(
+ bug, maintainer, bug_supervisor)
+
+ # Now make the bug visible to the bug supervisor and re-test.
+ with person_logged_in(bug.default_bugtask.pillar.owner):
+ bug.default_bugtask.transitionToAssignee(bug_supervisor)
+
+ # Test with bug supervisor = maintainer.
+ self._test_retarget_private_security_bug_to_product(
+ bug, maintainer, maintainer)
+ # Test with different bug supervisor
+ self._test_retarget_private_security_bug_to_product(
+ bug, maintainer, bug_supervisor)
+
def test_target_bugtask_to_sourcepackage(self):
# When a bugtask's target is changed, BugActivity and
# BugNotification get updated.
