launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03488
[Merge] lp:~bac/launchpad/bug-720147 into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-720147 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #720147 in Launchpad itself: "Subscription to New or Incomplete bugs is allowing just-filed bugs of all statuses through"
https://bugs.launchpad.net/launchpad/+bug/720147
For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-720147/+merge/59677
= 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
--
https://code.launchpad.net/~bac/launchpad/bug-720147/+merge/59677
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-720147 into lp:launchpad.
=== 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)
