launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04290
[Merge] lp:~gmb/launchpad/bug-491886 into lp:launchpad
Graham Binns has proposed merging lp:~gmb/launchpad/bug-491886 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #491886 in Launchpad itself: "UserCannotEditBugTaskStatus raised when moving task and editing its status or importance"
https://bugs.launchpad.net/launchpad/+bug/491886
For more details, see:
https://code.launchpad.net/~gmb/launchpad/bug-491886/+merge/68174
This branch fixes bug 491886.
The bug was caused by the fact that we never checked whether or not a user can set a given BugTaskStatus until we call transitionToStatus(). The problem with that is that transitionToStatus() will simply raise a UserCannotEditBugTaskStatus error, which results in an OOPS.
I've fixed this by doing the following:
- I've added a utility function, can_transition_to_status_for_target(), which as the name suggests returns True if $user can set $status for $bug_task on $target.
- The view now calls this in its validate() method, so a field error is set if there's a problem.
- BugTask.canTransitionToStatus() now calls can_transition_to_status_for_target() for the sake of DRY-ism.
--
https://code.launchpad.net/~gmb/launchpad/bug-491886/+merge/68174
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/bug-491886 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-06-20 07:03:18 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-07-17 14:02:38 +0000
@@ -74,7 +74,6 @@
from lazr.uri import URI
from pytz import utc
from simplejson import dumps
-from storm.expr import SQL
from z3c.ptcompat import ViewPageTemplateFile
from zope import (
component,
@@ -252,8 +251,7 @@
from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
from lp.bugs.interfaces.cve import ICveSet
from lp.bugs.interfaces.malone import IMaloneApplication
-from lp.bugs.model.bug import Bug
-from lp.bugs.model.bugtask import BugTask
+from lp.bugs.utilities.bugtask import can_transition_to_status_on_target
from lp.registry.interfaces.distribution import (
IDistribution,
IDistributionSet,
@@ -1436,6 +1434,17 @@
except WidgetsError, errors:
self.setFieldError('product', errors.args[0])
+ new_status = data.get('status')
+ if new_status != bugtask.status:
+ if not can_transition_to_status_on_target(
+ bugtask, new_product, new_status, self.user):
+ for name in ('product', 'status'):
+ self.setFieldError(
+ name,
+ "Only the Bug Supervisor for %s can set the bug's "
+ "status to %s" %
+ (new_product.displayname, new_status.title))
+
def updateContextFromData(self, data, context=None):
"""Updates the context object using the submitted form data.
@@ -1528,7 +1537,7 @@
# happen to be the only values that changed. We explicitly verify that
# we got a new status and/or assignee, because the form is not always
# guaranteed to pass all the values. For example: bugtasks linked to a
- # bug watch don't allow editting the form, and the value is missing
+ # bug watch don't allow editing the form, and the value is missing
# from the form.
missing = object()
new_status = new_values.pop("status", missing)
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-07-17 14:02:38 +0000
@@ -3,6 +3,8 @@
__metaclass__ = type
+import transaction
+
from datetime import datetime
from lazr.lifecycle.event import ObjectModifiedEvent
@@ -148,6 +150,39 @@
[activity] = view.interesting_activity
self.assertEqual("description", activity.whatchanged)
+ def test_error_for_changing_target_with_invalid_status(self):
+ # If a user moves a bug task with a restricted status (say,
+ # Triaged) to a target where they do not have permission to set
+ # that status, they will be unable to complete the retargeting
+ # and will instead receive an error in the UI.
+ person = self.factory.makePerson()
+ product = self.factory.makeProduct(
+ name='product1', owner=person, official_malone=True)
+ with person_logged_in(person):
+ product.setBugSupervisor(person, person)
+ product_2 = self.factory.makeProduct(
+ name='product2', official_malone=True)
+ with person_logged_in(product_2.owner):
+ product_2.setBugSupervisor(product_2.owner, product_2.owner)
+ bug = self.factory.makeBug(
+ product=product, owner=person)
+ # We need to commit here, otherwise all the sample data we
+ # created gets destroyed when the transaction is rolled back.
+ transaction.commit()
+ with person_logged_in(person):
+ form_data = {
+ '%s.product' % product.name: product_2.name,
+ '%s.status' % product.name: BugTaskStatus.TRIAGED.title,
+ '%s.actions.save' % product.name: 'Save Changes',
+ }
+ view = create_initialized_view(
+ bug.default_bugtask, name=u'+editstatus',
+ form=form_data)
+ # The bugtask's target won't have changed, since an error
+ # happend. The error will be listed in the view.
+ self.assertEqual(product, bug.default_bugtask.target)
+ self.assertEqual(2, len(view.errors))
+
class TestBugTasksAndNominationsView(TestCaseWithFactory):
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-07-05 11:08:41 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-07-17 14:02:38 +0000
@@ -57,7 +57,6 @@
alsoProvides,
implements,
)
-from zope.interface.interfaces import IMethod
from zope.security.proxy import (
isinstance as zope_isinstance,
removeSecurityProxy,
@@ -106,7 +105,6 @@
from lp.bugs.interfaces.bugattachment import BugAttachmentType
from lp.bugs.interfaces.bugnomination import BugNominationStatus
from lp.bugs.interfaces.bugtask import (
- BUG_SUPERVISOR_BUGTASK_STATUSES,
BugBlueprintSearch,
BugBranchSearch,
BugTaskImportance,
@@ -132,6 +130,7 @@
from lp.bugs.model.bugnomination import BugNomination
from lp.bugs.model.bugsubscription import BugSubscription
from lp.bugs.model.structuralsubscription import StructuralSubscription
+from lp.bugs.utilities.bugtask import can_transition_to_status_on_target
from lp.registry.interfaces.distribution import (
IDistribution,
IDistributionSet,
@@ -817,20 +816,8 @@
def canTransitionToStatus(self, new_status, user):
"""See `IBugTask`."""
- celebrities = getUtility(ILaunchpadCelebrities)
- if (self.status == BugTaskStatus.FIXRELEASED and
- (user.id == self.bug.ownerID or user.inTeam(self.bug.owner))):
- return True
- elif (user.inTeam(self.pillar.bug_supervisor) or
- user.inTeam(self.pillar.owner) or
- user.id == celebrities.bug_watch_updater.id or
- user.id == celebrities.bug_importer.id or
- user.id == celebrities.janitor.id):
- return True
- else:
- return (self.status not in (
- BugTaskStatus.WONTFIX, BugTaskStatus.FIXRELEASED)
- and new_status not in BUG_SUPERVISOR_BUGTASK_STATUSES)
+ return can_transition_to_status_on_target(
+ self, self.pillar, new_status, user)
def transitionToStatus(self, new_status, user, when=None):
"""See `IBugTask`."""
=== added file 'lib/lp/bugs/tests/test_bugtask_utilities.py'
--- lib/lp/bugs/tests/test_bugtask_utilities.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bugtask_utilities.py 2011-07-17 14:02:38 +0000
@@ -0,0 +1,82 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugTask utilities."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.bugs.interfaces.bugtask import (
+ BUG_SUPERVISOR_BUGTASK_STATUSES,
+ BugTaskStatus,
+ )
+from lp.bugs.utilities.bugtask import can_transition_to_status_on_target
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+
+
+class TestBugTaskUtilities(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_can_transition_to_status_on_target_accepts_valid_changes(self):
+ # can_transition_to_status_on_target returns True when a given
+ # user has permission to set a given BugTaskStatus for bug tasks
+ # on a given bug target.
+ user = self.factory.makePerson()
+ target = self.factory.makeProduct(
+ official_malone=True, owner=user)
+ # The user can set any status for a bug on `target`.
+ bug = self.factory.makeBug(product=target)
+ for status in BugTaskStatus.items:
+ self.assertTrue(
+ can_transition_to_status_on_target(
+ bug.default_bugtask, target, status, user),
+ "User should be able to set status %s" % status.title)
+
+ def test_can_transition_to_status_on_target_rejects_invalid_changes(self):
+ # can_transition_to_status_on_target returns False when a user
+ # doesn't have permission to set a given status on a given
+ # target.
+ target = self.factory.makeProduct(
+ official_malone=True)
+ with person_logged_in(target.owner):
+ target.setBugSupervisor(target.owner, target.owner)
+ # The user can set any status for a bug on `target`.
+ not_the_bug_supervisor = self.factory.makePerson()
+ bug = self.factory.makeBug(product=target)
+ for status in BUG_SUPERVISOR_BUGTASK_STATUSES:
+ self.assertFalse(
+ can_transition_to_status_on_target(
+ bug.default_bugtask, target, status,
+ not_the_bug_supervisor),
+ "User should not be able to set status %s" % status.title)
+
+ def test_celebrities_can_do_anything(self):
+ # can_transition_to_status_on_target will always return True for
+ # the Janitor, the Bug Importer or the Bug Watch Updater
+ # celebrities.
+ target = self.factory.makeProduct(
+ official_malone=True)
+ with person_logged_in(target.owner):
+ target.setBugSupervisor(target.owner, target.owner)
+ # The user can set any status for a bug on `target`.
+ celebs = (
+ getUtility(ILaunchpadCelebrities).janitor,
+ getUtility(ILaunchpadCelebrities).bug_watch_updater,
+ getUtility(ILaunchpadCelebrities).bug_importer,
+ )
+ bug = self.factory.makeBug(product=target)
+ for celeb in celebs:
+ for status in BugTaskStatus.items:
+ self.assertTrue(
+ can_transition_to_status_on_target(
+ bug.default_bugtask, target, status,
+ celeb),
+ "Celebrity %s should be able to set status %s" %
+ (celeb.name, status.title))
=== added file 'lib/lp/bugs/utilities/bugtask.py'
--- lib/lp/bugs/utilities/bugtask.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/utilities/bugtask.py 2011-07-17 14:02:38 +0000
@@ -0,0 +1,40 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Utilities for the BugTasks."""
+
+__metaclass__ = type
+__all__ = [
+ 'can_transition_to_status_on_target',
+ ]
+
+
+from zope.component import getUtility
+
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.bugs.interfaces.bugtask import (
+ BUG_SUPERVISOR_BUGTASK_STATUSES,
+ BugTaskStatus,
+ )
+
+
+def can_transition_to_status_on_target(bug_task, target, new_status, user):
+ """Return True if `user` can set `new_status` for `bug_task` on `target`.
+
+ This function is useful in situations where the user wants to change
+ both the target and the status of a bug task at the same time.
+ """
+ celebrities = getUtility(ILaunchpadCelebrities)
+ if (bug_task.status == BugTaskStatus.FIXRELEASED and
+ (user.id == bug_task.bug.ownerID or user.inTeam(bug_task.bug.owner))):
+ return True
+ elif (user.inTeam(target.bug_supervisor) or
+ user.inTeam(target.owner) or
+ user.id == celebrities.bug_watch_updater.id or
+ user.id == celebrities.bug_importer.id or
+ user.id == celebrities.janitor.id):
+ return True
+ else:
+ return (bug_task.status not in (
+ BugTaskStatus.WONTFIX, BugTaskStatus.FIXRELEASED)
+ and new_status not in BUG_SUPERVISOR_BUGTASK_STATUSES)
