Merge lp:~bac/launchpad/bug-720147 into lp:launchpad

[Brad Crittenden]

[r=benji][bug=720147] Correctly handle filtering for bugs created without the default status.

= Summary =

Bugs created with a status other than NEW were sending notifications for
subscriptions with other filters.

== Proposed fix ==

The bug notification was kicked off before the status change was
effected resulting in improper notices being sent.

== Pre-implementation notes ==

This work was done by Graham and Gary. I'm merely shepherding it
through review and landing in their absence.

I did, however, clean up some lint.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.bugs -t test_bugnotification

== Demo and Q/A ==

Create a structural subscription with filter with NEW status. Create a
bug with initial status of TRIAGE. Ensure notification is not sent.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/scripts/tests/test_bugnotification.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_bug.py
  lib/lp/bugs/interfaces/bug.py

[Benji York (benji)]

This branch looks good.

The only thought I had was that the part starting on line 13 of the diff
is a bit repetitive and I had to look very closely to see if it was
exactly the same pattern for each line or if one or more were subtly
different. However, I'm not sure my version is any better:
http://pastebin.ubuntu.com/602393/ (untested).

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2011-04-15 02:56:03 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2011-05-02 17:11:01 +0000
@@ -556,20 +556,20 @@
 
         if extra_data.private:
             params.private = extra_data.private
-
-        self.added_bug = bug = context.createBug(params)
-
-        # Apply any extra options given by a bug supervisor.
-        bugtask = self.added_bug.default_bugtask
+        if 'status' in data:
+            params.status = data['status']
         if 'assignee' in data:
-            bugtask.transitionToAssignee(data['assignee'])
+            params.assignee = data['assignee']
         if 'status' in data:
-            bugtask.transitionToStatus(data['status'], self.user)
+            params.status = data['status']
         if 'importance' in data:
-            bugtask.transitionToImportance(data['importance'], self.user)
+            params.importance = data['importance']
         if 'milestone' in data:
-            bugtask.milestone = data['milestone']
-
+            params.milestone = data['milestone']
+
+        self.added_bug = bug = context.createBug(params)
+
+        # Apply any extra options given by a bug supervisor.
         for comment in extra_data.comments:
             bug.newMessage(self.user, bug.followup_subject(), comment)
             notifications.append(

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-04-29 11:20:01 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-05-02 17:11:01 +0000
@@ -97,7 +97,8 @@
     def __init__(self, owner, title, comment=None, description=None, msg=None,
                  status=None, datecreated=None, security_related=False,
                  private=False, subscribers=(), binarypackagename=None,
-                 tags=None, subscribe_owner=True, filed_by=None):
+                 tags=None, subscribe_owner=True, filed_by=None,
+                 importance=None, milestone=None, assignee=None):
         self.owner = owner
         self.title = title
         self.comment = comment
@@ -115,6 +116,9 @@
         self.tags = tags
         self.subscribe_owner = subscribe_owner
         self.filed_by = filed_by
+        self.importance = importance
+        self.milestone = milestone
+        self.assignee = assignee
 
     def setBugTarget(self, product=None, distribution=None,
                      sourcepackagename=None):

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-05-02 01:27:43 +0000
+++ lib/lp/bugs/model/bug.py	2011-05-02 17:11:01 +0000
@@ -222,7 +222,8 @@
             "datecreated", "security_related", "private",
             "distribution", "sourcepackagename", "binarypackagename",
             "product", "status", "subscribers", "tags",
-            "subscribe_owner", "filed_by"])
+            "subscribe_owner", "filed_by", "importance",
+            "milestone", "assignee"])
 
 
 class BugTag(SQLBase):
@@ -2442,15 +2443,24 @@
         # Create the task on a product if one was passed.
         if params.product:
             getUtility(IBugTaskSet).createTask(
-                bug=bug, product=params.product, owner=params.owner,
-                status=params.status)
+                bug=bug, product=params.product, owner=params.owner)
 
         # Create the task on a source package name if one was passed.
         if params.distribution:
             getUtility(IBugTaskSet).createTask(
                 bug=bug, distribution=params.distribution,
                 sourcepackagename=params.sourcepackagename,
-                owner=params.owner, status=params.status)
+                owner=params.owner)
+
+        bug_task = bug.default_bugtask
+        if params.assignee:
+            bug_task.transitionToAssignee(params.assignee)
+        if params.status:
+            bug_task.transitionToStatus(params.status, params.owner)
+        if params.importance:
+            bug_task.transitionToImportance(params.importance, params.owner)
+        if params.milestone:
+            bug_task.transitionToMilestone(params.milestone, params.owner)
 
         # Tell everyone.
         notify(event)

=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2011-04-05 22:34:35 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2011-05-02 17:11:01 +0000
@@ -21,6 +21,7 @@
     )
 from canonical.launchpad.ftests import login
 from canonical.launchpad.helpers import get_contact_email_addresses
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.interfaces.message import IMessageSet
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.bugs.adapters.bugchange import (
@@ -38,16 +39,20 @@
     CveUnlinkedFromBug,
     )
 from lp.bugs.interfaces.bug import (
+    CreateBugParams,
     IBug,
     IBugSet,
     )
 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
-from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.bugs.interfaces.bugtask import (
+    BugTaskImportance,
+    BugTaskStatus,
+    )
 from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
 from lp.bugs.model.bugnotification import (
     BugNotification,
     BugNotificationFilter,
-#    BugNotificationRecipient,
+    BugNotificationRecipient,
     )
 from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilterMute
 from lp.bugs.model.bugtask import BugTask
@@ -1021,7 +1026,7 @@
     def test_header_single(self):
         # A single filter with a description makes all emails
         # include that particular filter description in a header.
-        bug_filter = self.addFilter(u"Test filter")
+        self.addFilter(u"Test filter")
 
         self.assertContentEqual([u"Test filter"],
                                 self.getSubscriptionEmailHeaders())
@@ -1029,8 +1034,8 @@
     def test_header_multiple(self):
         # Multiple filters with a description make all emails
         # include all filter descriptions in the header.
-        bug_filter = self.addFilter(u"First filter")
-        bug_filter = self.addFilter(u"Second filter")
+        self.addFilter(u"First filter")
+        self.addFilter(u"Second filter")
 
         self.assertContentEqual([u"First filter", u"Second filter"],
                                 self.getSubscriptionEmailHeaders())
@@ -1042,10 +1047,10 @@
         other_person = self.factory.makePerson()
         other_subscription = self.bug.default_bugtask.target.addSubscription(
             other_person, other_person)
-        bug_filter = self.addFilter(u"Someone's filter", other_subscription)
+        self.addFilter(u"Someone's filter", other_subscription)
         self.addNotificationRecipient(self.notification, other_person)
 
-        this_filter = self.addFilter(u"Test filter")
+        self.addFilter(u"Test filter")
 
         the_subscriber = self.subscription.subscriber
         self.assertEquals(
@@ -1062,7 +1067,7 @@
     def test_body_single(self):
         # A single filter with a description makes all emails
         # include that particular filter description in the body.
-        bug_filter = self.addFilter(u"Test filter")
+        self.addFilter(u"Test filter")
 
         self.assertContentEqual([u"Matching subscriptions: Test filter"],
                                 self.getSubscriptionEmailBody())
@@ -1070,15 +1075,15 @@
     def test_body_multiple(self):
         # Multiple filters with description make all emails
         # include them in the email body.
-        bug_filter = self.addFilter(u"First filter")
-        bug_filter = self.addFilter(u"Second filter")
+        self.addFilter(u"First filter")
+        self.addFilter(u"Second filter")
 
         self.assertContentEqual(
             [u"Matching subscriptions: First filter, Second filter"],
             self.getSubscriptionEmailBody())
 
     def test_muted(self):
-        bug_filter = self.addFilter(u"Test filter")
+        self.addFilter(u"Test filter")
         BugSubscriptionFilterMute(
             person=self.subscription.subscriber,
             filter=self.notification.bug_filters.one())
@@ -1089,11 +1094,58 @@
     def test_header_multiple_one_muted(self):
         # Multiple filters with a description make all emails
         # include all filter descriptions in the header.
-        bug_filter = self.addFilter(u"First filter")
-        bug_filter = self.addFilter(u"Second filter")
+        self.addFilter(u"First filter")
+        self.addFilter(u"Second filter")
         BugSubscriptionFilterMute(
             person=self.subscription.subscriber,
             filter=self.notification.bug_filters[0])
 
         self.assertContentEqual([u"Second filter"],
                                 self.getSubscriptionEmailHeaders())
+
+
+class TestEmailNotificationsWithFiltersWhenBugCreated(TestCaseWithFactory):
+    # See bug 720147.
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestEmailNotificationsWithFiltersWhenBugCreated, self).setUp()
+        self.subscriber = self.factory.makePerson()
+        self.submitter = self.factory.makePerson()
+        self.product = self.factory.makeProduct(
+            bug_supervisor=self.submitter)
+        self.subscription = self.product.addSubscription(
+            self.subscriber, self.subscriber)
+        self.filter = self.subscription.bug_filters[0]
+        self.filter.description = u'Needs triage'
+        self.filter.statuses = [BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE]
+
+    def test_filters_match_when_bug_is_created(self):
+        message = u"this is an unfiltered comment"
+        params = CreateBugParams(
+            title=u"crashes all the time",
+            comment=message, owner=self.submitter,
+            status=BugTaskStatus.NEW)
+        bug = self.product.createBug(params)
+        notification = IStore(BugNotification).find(
+            BugNotification,
+            BugNotification.id==BugNotificationRecipient.bug_notificationID,
+            BugNotificationRecipient.personID == self.subscriber.id,
+            BugNotification.bug == bug).one()
+        self.assertEqual(notification.message.text_contents, message)
+
+    def test_filters_do_not_match_when_bug_is_created(self):
+        message = u"this is a filtered comment"
+        params = CreateBugParams(
+            title=u"crashes all the time",
+            comment=message, owner=self.submitter,
+            status=BugTaskStatus.TRIAGED,
+            importance=BugTaskImportance.HIGH)
+        bug = self.product.createBug(params)
+        notifications = IStore(BugNotification).find(
+            BugNotification,
+            BugNotification.id==BugNotificationRecipient.bug_notificationID,
+            BugNotificationRecipient.personID == self.subscriber.id,
+            BugNotification.bug == bug)
+        self.assertTrue(notifications.is_empty())

=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py	2011-04-13 15:23:54 +0000
+++ lib/lp/bugs/tests/test_bug.py	2011-05-02 17:11:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for lp.bugs.model.Bug."""
@@ -6,11 +6,23 @@
 __metaclass__ = type
 
 from lazr.lifecycle.snapshot import Snapshot
+from zope.component import getUtility
 from zope.interface import providedBy
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 
 from lp.bugs.enum import BugNotificationLevel
+from lp.bugs.interfaces.bug import(
+    CreateBugParams,
+    IBugSet,
+    )
+from lp.bugs.interfaces.bugtask import (
+    BugTaskImportance,
+    BugTaskStatus,
+    UserCannotEditBugTaskAssignee,
+    UserCannotEditBugTaskImportance,
+    UserCannotEditBugTaskMilestone,
+    )
 from lp.testing import (
     person_logged_in,
     StormStatementRecorder,
@@ -30,7 +42,7 @@
         # Bug.isMuted() will return True if the passed to it has a
         # BugSubscription with a BugNotificationLevel of NOTHING.
         with person_logged_in(self.person):
-            subscription = self.bug.subscribe(
+            self.bug.subscribe(
                 self.person, self.person, level=BugNotificationLevel.NOTHING)
             self.assertEqual(True, self.bug.isMuted(self.person))
 
@@ -38,7 +50,7 @@
         # Bug.isMuted() will return False if the user has a subscription
         # with BugNotificationLevel that's not NOTHING.
         with person_logged_in(self.person):
-            subscription = self.bug.subscribe(
+            self.bug.subscribe(
                 self.person, self.person, level=BugNotificationLevel.METADATA)
             self.assertEqual(False, self.bug.isMuted(self.person))
 
@@ -122,7 +134,7 @@
         # optimizations that might trigger the problem again.
         with person_logged_in(self.person):
             with StormStatementRecorder() as recorder:
-                snapshot = Snapshot(self.bug, providing=providedBy(self.bug))
+                Snapshot(self.bug, providing=providedBy(self.bug))
             sql_statements = recorder.statements
         # This uses "self" as a marker to show that the attribute does not
         # exist.  We do not use hasattr because it eats exceptions.
@@ -139,3 +151,113 @@
                 [token for token in sql_tokens
                  if token.startswith('message')],
                 [])
+
+
+class TestBugCreation(TestCaseWithFactory):
+    """Tests for bug creation."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_CreateBugParams_accepts_importance(self):
+        # The importance of the initial bug task can be set using
+        # CreateBugParams
+        owner = self.factory.makePerson()
+        target = self.factory.makeProduct(owner=owner)
+        with person_logged_in(owner):
+            params = CreateBugParams(
+                owner=owner, title="A bug", comment="Nothing important.",
+                importance=BugTaskImportance.HIGH)
+            params.setBugTarget(product=target)
+            bug = getUtility(IBugSet).createBug(params)
+            self.assertEqual(
+                bug.default_bugtask.importance, params.importance)
+
+    def test_CreateBugParams_accepts_assignee(self):
+        # The assignee of the initial bug task can be set using
+        # CreateBugParams
+        owner = self.factory.makePerson()
+        target = self.factory.makeProduct(owner=owner)
+        with person_logged_in(owner):
+            params = CreateBugParams(
+                owner=owner, title="A bug", comment="Nothing important.",
+                assignee=owner)
+            params.setBugTarget(product=target)
+            bug = getUtility(IBugSet).createBug(params)
+            self.assertEqual(
+                bug.default_bugtask.assignee, params.assignee)
+
+    def test_CreateBugParams_accepts_milestone(self):
+        # The milestone of the initial bug task can be set using
+        # CreateBugParams
+        owner = self.factory.makePerson()
+        target = self.factory.makeProduct(owner=owner)
+        with person_logged_in(owner):
+            params = CreateBugParams(
+                owner=owner, title="A bug", comment="Nothing important.",
+                milestone=self.factory.makeMilestone(product=target))
+            params.setBugTarget(product=target)
+            bug = getUtility(IBugSet).createBug(params)
+            self.assertEqual(
+                bug.default_bugtask.milestone, params.milestone)
+
+    def test_CreateBugParams_accepts_status(self):
+        # The status of the initial bug task can be set using
+        # CreateBugParams
+        owner = self.factory.makePerson()
+        target = self.factory.makeProduct(owner=owner)
+        with person_logged_in(owner):
+            params = CreateBugParams(
+                owner=owner, title="A bug", comment="Nothing important.",
+                status=BugTaskStatus.TRIAGED)
+            params.setBugTarget(product=target)
+            bug = getUtility(IBugSet).createBug(params)
+            self.assertEqual(
+                bug.default_bugtask.status, params.status)
+
+    def test_CreateBugParams_rejects_not_allowed_importance_changes(self):
+        # createBug() will reject any importance value passed by users
+        # who don't have the right to set the importance.
+        person = self.factory.makePerson()
+        target = self.factory.makeProduct()
+        with person_logged_in(person):
+            params = CreateBugParams(
+                owner=person, title="A bug", comment="Nothing important.",
+                importance=BugTaskImportance.HIGH)
+            params.setBugTarget(product=target)
+            self.assertRaises(
+                UserCannotEditBugTaskImportance,
+                getUtility(IBugSet).createBug, params)
+
+    def test_CreateBugParams_rejects_not_allowed_assignee_changes(self):
+        # createBug() will reject any importance value passed by users
+        # who don't have the right to set the assignee.
+        person = self.factory.makePerson()
+        person_2 = self.factory.makePerson()
+        target = self.factory.makeProduct()
+        # Setting the target's bug supervisor means that
+        # canTransitionToAssignee() will return False for `person` if
+        # another Person is passed as `assignee`.
+        with person_logged_in(target.owner):
+            target.setBugSupervisor(target.owner, target.owner)
+        with person_logged_in(person):
+            params = CreateBugParams(
+                owner=person, title="A bug", comment="Nothing important.",
+                assignee=person_2)
+            params.setBugTarget(product=target)
+            self.assertRaises(
+                UserCannotEditBugTaskAssignee,
+                getUtility(IBugSet).createBug, params)
+
+    def test_CreateBugParams_rejects_not_allowed_milestone_changes(self):
+        # createBug() will reject any importance value passed by users
+        # who don't have the right to set the milestone.
+        person = self.factory.makePerson()
+        target = self.factory.makeProduct()
+        with person_logged_in(person):
+            params = CreateBugParams(
+                owner=person, title="A bug", comment="Nothing important.",
+                milestone=self.factory.makeMilestone(product=target))
+            params.setBugTarget(product=target)
+            self.assertRaises(
+                UserCannotEditBugTaskMilestone,
+                getUtility(IBugSet).createBug, params)