launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/destroy-cause-unknownrecipienterror into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1036882 in Launchpad itself: "UnknownRecipientError OOPS updating series specific tasks"
  https://bugs.launchpad.net/launchpad/+bug/1036882

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/destroy-cause-unknownrecipienterror/+merge/125397

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 0 net LoC.
-- 
https://code.launchpad.net/~stevenk/launchpad/destroy-cause-unknownrecipienterror/+merge/125397
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/destroy-cause-unknownrecipienterror into lp:launchpad.

=== 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 03:06:28 +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,10 +1147,8 @@
 
     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):

=== 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 03:06:28 +0000
@@ -160,6 +160,13 @@
         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 level or so), and we
+                # are only interested in dropping the assignee out, since we
+                # send assignment notifications separately.
+                if person not in recipients:
+                    continue
                 reason, rationale = recipients.getReason(person)
                 if 'Assignee' in rationale:
                     recipients.remove(person)