Merge lp:~stevenk/launchpad/destroy-cause-unknownrecipienterror into lp:launchpad

[Steve Kowalik]

Ignore new subscribers that are not in the recipient list when filtering for the assignee in the depths of the bug zope subscribers.

When changing assignee, we want to filter it out of the "normal" notification mail that other subscribers recieve. To this end in the zope subscriber function receives a list of subscribers that will be notified by set difference using the two objects on the event. We then trawl through this list looking for the assignee so we can remove them.

So, this is fine. In the case that we only change the assignee, *or* all new subscribers are in the recipients list we build. Where this house of cards completely falls down is if we have a structural subscriber on a milestone with LIFECYCLE set. (LIFECYCLE subscribers only get notified on a new bug or a bug being closed.) When the assignee is changed and the milestone is set in *one action* using BugTask:+editstatus, we go through the list of new subscribers, and then raise an UnknownRecipientError because the structural subscriber isn't in the recipient list, and then OOPS.

Yay for layering violations. I've worked around this by checking if the person is in the recipients list first, and added a verbose comment to that effect.

I have also gone through test_bugtask.py and removed a bunch of excess whitespace to cause this branch to be a net LoC loss. Also, correct a thinko that I noticed.

[Ian Booth (wallyworld)]

review: Approve

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-08-30 06:18:38 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-09-20 04:52:24 +0000
@@ -41,6 +41,7 @@
     BugTaskListingItem,
     BugTasksAndNominationsView,
     )
+from lp.bugs.enums import BugNotificationLevel
 from lp.bugs.feed.bug import (
     BugTargetBugsFeed,
     PersonBugsFeed,
@@ -277,6 +278,30 @@
             self.assertEqual(1, len(view.errors))
             self.assertEqual(product, bug.default_bugtask.target)
 
+    def test_changing_milestone_and_assignee_with_lifecycle(self):
+        # Changing the milestone and assignee of a bugtask when the milestone
+        # has a LIFECYCLE structsub is fine. Also see bug 1036882.
+        subscriber = self.factory.makePerson()
+        product = self.factory.makeProduct(official_malone=True)
+        milestone = self.factory.makeMilestone(product=product)
+        with person_logged_in(subscriber):
+            structsub = milestone.addBugSubscription(subscriber, subscriber)
+            structsub.bug_filters[0].bug_notification_level = (
+                BugNotificationLevel.LIFECYCLE)
+        bug = self.factory.makeBug(target=product)
+        with person_logged_in(product.owner):
+            form_data = {
+                '%s.milestone' % product.name: milestone.id,
+                '%s.assignee.option' % product.name:
+                    '%s.assignee.assign_to_me' % product.name,
+                '%s.assignee' % product.name: product.owner.name,
+                '%s.actions.save' % product.name: 'Save Changes',
+                }
+            create_initialized_view(
+                bug.default_bugtask, name='+editstatus', form=form_data)
+            self.assertEqual(product.owner, bug.default_bugtask.assignee)
+            self.assertEqual(milestone, bug.default_bugtask.milestone)
+
     def test_bugtag_urls_are_encoded(self):
         # The link to bug tags are encoded to protect against special chars.
         product = self.factory.makeProduct(name='foobar')
@@ -294,8 +319,7 @@
     def test_information_type(self):
         owner = self.factory.makePerson()
         bug = self.factory.makeBug(
-            owner=owner,
-            information_type=InformationType.USERDATA)
+            owner=owner, information_type=InformationType.USERDATA)
         login_person(owner)
         bugtask = self.factory.makeBugTask(bug=bug)
         view = create_initialized_view(bugtask, name="+index")
@@ -432,17 +456,13 @@
         self.useFixture(FeatureFixture(
             {'bugs.affected_count_includes_dupes.disabled': ''}))
         self.makeDuplicate()
-        self.assertEqual(
-            3, self.view.total_users_affected_count)
+        self.assertEqual(3, self.view.total_users_affected_count)
         self.assertEqual(
             "This bug affects 3 people. Does this bug affect you?",
             self.view.affected_statement)
         self.assertEqual(
-            "This bug affects 3 people",
-            self.view.anon_affected_statement)
-        self.assertEqual(
-            self.view.other_users_affected_count,
-            3)
+            "This bug affects 3 people", self.view.anon_affected_statement)
+        self.assertEqual(self.view.other_users_affected_count, 3)
 
     def test_counts_affected_by_duplicate(self):
         self.useFixture(FeatureFixture(
@@ -455,11 +475,8 @@
             "This bug affects 3 people. Does this bug affect you?",
             self.view.affected_statement)
         self.assertEqual(
-            "This bug affects 4 people",
-            self.view.anon_affected_statement)
-        self.assertEqual(
-            self.view.other_users_affected_count,
-            3)
+            "This bug affects 4 people", self.view.anon_affected_statement)
+        self.assertEqual(self.view.other_users_affected_count, 3)
 
     def test_counts_affected_by_master(self):
         self.useFixture(FeatureFixture(
@@ -472,11 +489,8 @@
             "This bug affects you and 3 other people",
             self.view.affected_statement)
         self.assertEqual(
-            "This bug affects 4 people",
-            self.view.anon_affected_statement)
-        self.assertEqual(
-            self.view.other_users_affected_count,
-            3)
+            "This bug affects 4 people", self.view.anon_affected_statement)
+        self.assertEqual(self.view.other_users_affected_count, 3)
 
     def test_counts_affected_by_duplicate_not_by_master(self):
         self.useFixture(FeatureFixture(
@@ -493,11 +507,8 @@
         # either 3 or 4 people.  However at the moment the "No" answer on the
         # master is more authoritative than the "Yes" on the dupe.
         self.assertEqual(
-            "This bug affects 3 people",
-            self.view.anon_affected_statement)
-        self.assertEqual(
-            self.view.other_users_affected_count,
-            3)
+            "This bug affects 3 people", self.view.anon_affected_statement)
+        self.assertEqual(self.view.other_users_affected_count, 3)
 
     def test_total_users_affected_count_without_dupes(self):
         self.useFixture(FeatureFixture(
@@ -505,25 +516,20 @@
         self.makeDuplicate()
         self.refresh()
         # Does not count the two users of bug2, so just 1.
-        self.assertEqual(
-            1, self.view.total_users_affected_count)
+        self.assertEqual(1, self.view.total_users_affected_count)
         self.assertEqual(
             "This bug affects 1 person. Does this bug affect you?",
             self.view.affected_statement)
         self.assertEqual(
-            "This bug affects 1 person",
-            self.view.anon_affected_statement)
-        self.assertEqual(
-            1,
-            self.view.other_users_affected_count)
+            "This bug affects 1 person", self.view.anon_affected_statement)
+        self.assertEqual(1, self.view.other_users_affected_count)
 
     def test_affected_statement_no_one_affected(self):
         self.bug.markUserAffected(self.bug.owner, False)
         self.failUnlessEqual(
             0, self.view.other_users_affected_count)
         self.failUnlessEqual(
-            "Does this bug affect you?",
-            self.view.affected_statement)
+            "Does this bug affect you?", self.view.affected_statement)
 
     def test_affected_statement_only_you(self):
         self.view.context.markUserAffected(self.view.user, True)
@@ -532,8 +538,7 @@
         self.failUnlessEqual(
             0, self.view.other_users_affected_count)
         self.failUnlessEqual(
-            "This bug affects you",
-            self.view.affected_statement)
+            "This bug affects you", self.view.affected_statement)
 
     def test_affected_statement_only_not_you(self):
         self.view.context.markUserAffected(self.view.user, False)
@@ -542,8 +547,7 @@
         self.failUnlessEqual(
             0, self.view.other_users_affected_count)
         self.failUnlessEqual(
-            "This bug doesn't affect you",
-            self.view.affected_statement)
+            "This bug doesn't affect you", self.view.affected_statement)
 
     def test_affected_statement_1_person_not_you(self):
         self.assertIs(None, self.bug.isUserAffected(self.view.user))
@@ -565,8 +569,7 @@
     def test_affected_statement_1_person_and_not_you(self):
         self.view.context.markUserAffected(self.view.user, False)
         self.failIf(self.bug.isUserAffected(self.view.user))
-        self.failUnlessEqual(
-            1, self.view.other_users_affected_count)
+        self.failUnlessEqual(1, self.view.other_users_affected_count)
         self.failUnlessEqual(
             "This bug affects 1 person, but not you",
             self.view.affected_statement)
@@ -575,8 +578,7 @@
         self.assertIs(None, self.bug.isUserAffected(self.view.user))
         other_user = self.factory.makePerson()
         self.view.context.markUserAffected(other_user, True)
-        self.failUnlessEqual(
-            2, self.view.other_users_affected_count)
+        self.failUnlessEqual(2, self.view.other_users_affected_count)
         self.failUnlessEqual(
             "This bug affects 2 people. Does this bug affect you?",
             self.view.affected_statement)
@@ -586,8 +588,7 @@
         self.failUnless(self.bug.isUserAffected(self.view.user))
         other_user = self.factory.makePerson()
         self.view.context.markUserAffected(other_user, True)
-        self.failUnlessEqual(
-            2, self.view.other_users_affected_count)
+        self.failUnlessEqual(2, self.view.other_users_affected_count)
         self.failUnlessEqual(
             "This bug affects you and 2 other people",
             self.view.affected_statement)
@@ -597,8 +598,7 @@
         self.failIf(self.bug.isUserAffected(self.view.user))
         other_user = self.factory.makePerson()
         self.view.context.markUserAffected(other_user, True)
-        self.failUnlessEqual(
-            2, self.view.other_users_affected_count)
+        self.failUnlessEqual(2, self.view.other_users_affected_count)
         self.failUnlessEqual(
             "This bug affects 2 people, but not you",
             self.view.affected_statement)
@@ -611,15 +611,13 @@
     def test_anon_affected_statement_1_user_affected(self):
         self.failUnlessEqual(1, self.bug.users_affected_count)
         self.failUnlessEqual(
-            "This bug affects 1 person",
-            self.view.anon_affected_statement)
+            "This bug affects 1 person", self.view.anon_affected_statement)
 
     def test_anon_affected_statement_2_users_affected(self):
         self.view.context.markUserAffected(self.view.user, True)
         self.failUnlessEqual(2, self.bug.users_affected_count)
         self.failUnlessEqual(
-            "This bug affects 2 people",
-            self.view.anon_affected_statement)
+            "This bug affects 2 people", self.view.anon_affected_statement)
 
     def test_getTargetLinkTitle_product(self):
         # The target link title is always none for products.
@@ -690,8 +688,7 @@
         bug_task = self.factory.makeBugTask(
             bug=self.bug, target=target, publish=False)
         self.view.initialize()
-        self.assertTrue(
-            target in self.view.target_releases.keys())
+        self.assertTrue(target in self.view.target_releases.keys())
         self.assertEqual(
             'Latest release: 2.0, uploaded to universe on '
             '2008-07-18 10:20:30+00:00 by Tim (tim), maintained by Jim (jim)',
@@ -714,8 +711,7 @@
         bug_task = self.factory.makeBugTask(
             bug=self.bug, target=target, publish=False)
         self.view.initialize()
-        self.assertTrue(
-            target in self.view.target_releases.keys())
+        self.assertTrue(target in self.view.target_releases.keys())
         self.assertEqual(
             'Latest release: 2.0, uploaded to universe on '
             '2008-07-18 10:20:30+00:00 by Tim (tim), maintained by Jim (jim)',
@@ -803,8 +799,7 @@
         product_foo = self.factory.makeProduct(name="foo")
         foo_bug = self.factory.makeBug(target=product_foo)
         assignee = self.factory.makeTeam(
-            name="assignee",
-            visibility=PersonVisibility.PRIVATE)
+            name="assignee", visibility=PersonVisibility.PRIVATE)
         foo_bug.default_bugtask.transitionToAssignee(assignee)
 
         # Render the view.
@@ -1130,7 +1125,7 @@
             target=distro, owner=owner,
             information_type=InformationType.PRIVATESECURITY)
         # XXX wgrant 2012-08-30 bug=1041002: Distributions don't have
-        # sharing policies yet, so it isn't possible legitimately create
+        # sharing policies yet, so it isn't possible to legitimately create
         # a Proprietary distro bug.
         removeSecurityProxy(bug).information_type = (
             InformationType.PROPRIETARY)
@@ -1152,16 +1147,13 @@
 
     def setUp(self):
         super(TestBugTaskEditViewStatusField, self).setUp()
-        product_owner = self.factory.makePerson(name='product-owner')
         bug_supervisor = self.factory.makePerson(name='bug-supervisor')
-        product = self.factory.makeProduct(
-            owner=product_owner, bug_supervisor=bug_supervisor)
+        product = self.factory.makeProduct(bug_supervisor=bug_supervisor)
         self.bug = self.factory.makeBug(target=product)
 
     def getWidgetOptionTitles(self, widget):
         """Return the titles of options of the given choice widget."""
-        return [
-            item.value.title for item in widget.field.vocabulary]
+        return [item.value.title for item in widget.field.vocabulary]
 
     def test_status_field_items_for_anonymous(self):
         # Anonymous users see only the current value.
@@ -1622,8 +1614,8 @@
     def setUp(self):
         super(TestProjectGroupBugs, self).setUp()
         self.owner = self.factory.makePerson(name='bob')
-        self.target = self.factory.makeProject(name='container',
-                                                     owner=self.owner)
+        self.target = self.factory.makeProject(
+            name='container', owner=self.owner)
 
     def makeSubordinateProduct(self, tracks_bugs_in_lp):
         """Create a new product and add it to the project group."""
@@ -1844,8 +1836,7 @@
 
     def _assertThatUnbatchedAndBatchedActivityMatch(self, unbatched_activity,
                                                     batched_activity):
-        zipped_activity = zip(
-            unbatched_activity, batched_activity)
+        zipped_activity = zip(unbatched_activity, batched_activity)
         for index, items in enumerate(zipped_activity):
             unbatched_item, batched_item = items
             self.assertEqual(
@@ -1877,8 +1868,7 @@
         bug_task = self.factory.makeBugTask()
         view = create_initialized_view(bug_task, '+batched-comments')
         self.assertEqual(
-            config.malone.comments_list_default_batch_size,
-            view.batch_size)
+            config.malone.comments_list_default_batch_size, view.batch_size)
         view = create_initialized_view(
             bug_task, '+batched-comments', form={'batch_size': 20})
         self.assertEqual(20, view.batch_size)

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2012-09-07 20:15:30 +0000
+++ lib/lp/bugs/subscribers/bug.py	2012-09-20 04:52:24 +0000
@@ -160,6 +160,14 @@
         elif (isinstance(change, BugTaskAssigneeChange) and
               new_subscribers is not None):
             for person in new_subscribers:
+                # If this change involves multiple changes, other structural
+                # subscribers will leak into new_subscribers, and they may
+                # not be in the recipients list, due to having a LIFECYCLE
+                # structural subscription.
+                if person not in recipients:
+                    continue
+                # We are only interested in dropping the assignee out, since
+                # we send assignment notifications separately.
                 reason, rationale = recipients.getReason(person)
                 if 'Assignee' in rationale:
                     recipients.remove(person)