launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05267
[Merge] lp:~wallyworld/launchpad/delete-bugtasks-1324 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/delete-bugtasks-1324 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtasks-1324/+merge/79541
Add a bugtask delete method to the IBugTask API and also export to the web service.
A followup branch will add an entry to the bug activity log when a task is deleted and an email will be sent to the bug supervisor of the pillar for which the task is deleted.
== Preimplementation ==
Talked with lifeless and wgrant. lifeless was concerned about deleting a bugtask if it were a conjoined master/slave. wgrant said this doesn't matter due to recent changes to allow such 'orphaned' bugtasks.
== Implementation ==
The delete implementation was done in a destroySelf method on BugTask. This fits with how other entities are deleted in Launchpad (even though my preference is for the lifecycle of entities to be managed by a service based interface rather than the entiy itself). In terms of security, a new Launchpad permission is introduced - "launchpad.Delete". I feel it is better more semantically correct to use this new permission rather than just piggyback onto the launchpad.Edit permission.
A security adaptor is provided to check the necessary role based permissions. A new interface is introduced to allow the security and permissions to be wired up that that everything works in both the server side and web service api - IBugTaskDelete.
There are also other business rules which stop a bugtask from being deleted eg the last bugtask cannot be deleted. These rules are checked in the canBeDeleted() method and a CannotDeleteBugtask exception is raised in such cases.
A feature flag is used to hide this operation - 'disclosure.delete_bugtask.enabled'. During testing, a problem was found with the feature flag infrastructure. The StormFeatureRulesSource.setAllRules() method needs to call flush(), or else the rules passed in when the fixture is set up are not guaranteed to be in the database when required. A case in point: the check_permission method uses a @block_implicit_flushes decorator.
== Demo and QA ==
An example script to invoke the api using launchpadlib:
from launchpadlib.launchpad import Launchpad
def test_delete_bugtask(bug_num, pillar_name):
lp = Launchpad.login_with(
'testing', service_root=uris.LPNET_SERVICE_ROOT, version='devel')
bug = lp.bugs[bug_num]
bugtasks = bug.bug_tasks
if len(bugtasks) == 1:
print "This bug has no extra bugtasks to be deleted."
else:
for bugtask in bugtasks:
if bugtask.target.name == pillar_name:
bugtask.lp_delete()
print "deleted task for %s on bug %d." % (pillar_name, bug_num)
== Tests ==
Add a new testcase TestBugTaskDeletion with some tests:
test_cannot_delete_if_not_logged_in
test_unauthorised_cannot_delete
test_pillar_owner_can_delete
test_bug_supervisor_can_delete
test_task_reporter_can_delete
test_cannot_delete_only_bugtask
test_delete_bugtask
test_delete_default_bugtask
test_bug_heat_updated
Add a testcase for the web service interface: TestWebService
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/launchpad/permissions.zcml
lib/lp/bugs/configure.zcml
lib/lp/bugs/security.py
lib/lp/bugs/interfaces/bugtask.py
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/model/tests/test_bugtask.py
lib/lp/services/features/flags.py
lib/lp/services/features/rulesource.py
--
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtasks-1324/+merge/79541
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/delete-bugtasks-1324 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/permissions.zcml'
--- lib/canonical/launchpad/permissions.zcml 2010-12-10 23:10:09 +0000
+++ lib/canonical/launchpad/permissions.zcml 2011-10-19 04:31:25 +0000
@@ -27,6 +27,9 @@
id="launchpad.Append" title="Adding something" access_level="write" />
<permission
+ id="launchpad.Delete" title="Deleting something" access_level="write" />
+
+ <permission
id="launchpad.Moderate" title="Moderate something" access_level="write" />
<permission
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-10-05 18:02:45 +0000
+++ lib/lp/bugs/configure.zcml 2011-10-19 04:31:25 +0000
@@ -236,6 +236,9 @@
findSimilarBugs
getContributorInfo"/>
<require
+ permission="launchpad.Delete"
+ interface="lp.bugs.interfaces.bugtask.IBugTaskDelete"/>
+ <require
permission="launchpad.Edit"
attributes="
setStatusFromDebbugs
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-10-11 11:11:10 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-10-19 04:31:25 +0000
@@ -17,6 +17,7 @@
'BugTaskStatus',
'BugTaskStatusSearch',
'BugTaskStatusSearchDisplay',
+ 'CannotDeleteBugtask',
'DB_INCOMPLETE_BUGTASK_STATUSES',
'DB_UNRESOLVED_BUGTASK_STATUSES',
'DEFAULT_SEARCH_BUGTASK_STATUSES_FOR_DISPLAY',
@@ -24,6 +25,7 @@
'IAddBugTaskForm',
'IAddBugTaskWithProductCreationForm',
'IBugTask',
+ 'IBugTaskDelete',
'IBugTaskDelta',
'IBugTaskSearch',
'IBugTaskSet',
@@ -58,6 +60,7 @@
call_with,
error_status,
export_as_webservice_entry,
+ export_destructor_operation,
export_read_operation,
export_write_operation,
exported,
@@ -435,6 +438,15 @@
for item in DEFAULT_SEARCH_BUGTASK_STATUSES]
+@error_status(httplib.BAD_REQUEST)
+class CannotDeleteBugtask(Exception):
+ """The bugtask cannot be deleted.
+
+ Raised when a user tries to delete a bugtask but the deletion cannot
+ proceed because of a model constraint or other business rule violation.
+ """
+
+
@error_status(httplib.UNAUTHORIZED)
class UserCannotEditBugTaskStatus(Unauthorized):
"""User not permitted to change status.
@@ -482,7 +494,21 @@
in a search for related bug tasks"""
-class IBugTask(IHasDateCreated, IHasBug):
+class IBugTaskDelete(Interface):
+ """An interface for operations allowed with the Delete permission."""
+ @export_destructor_operation()
+ @operation_for_version('devel')
+ def destroySelf():
+ """Delete this bugtask.
+
+ :raises: CannotDeleteBugtask if the bugtask cannot be deleted due to a
+ business rule or other model constraint.
+ :raises: Unauthorized if the user does not have permission
+ to delete the bugtask.
+ """
+
+
+class IBugTask(IHasDateCreated, IHasBug, IBugTaskDelete):
"""A bug needing fixing in a particular product or package."""
export_as_webservice_entry()
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-10-11 20:08:38 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-10-19 04:31:25 +0000
@@ -128,7 +128,7 @@
UserCannotEditBugTaskImportance,
UserCannotEditBugTaskMilestone,
UserCannotEditBugTaskStatus,
- )
+ CannotDeleteBugtask)
from lp.bugs.model.bugnomination import BugNomination
from lp.bugs.model.bugsubscription import BugSubscription
from lp.registry.interfaces.distribution import (
@@ -620,6 +620,27 @@
"""
return self._status in RESOLVED_BUGTASK_STATUSES
+ def canBeDeleted(self):
+ num_bugtasks = Store.of(self).find(
+ BugTask, bug=self.bug).count()
+
+ return num_bugtasks > 1
+
+ def destroySelf(self):
+ """See `IBugTask`."""
+
+ if not self.canBeDeleted():
+ raise CannotDeleteBugtask(
+ "Cannot delete bugtask: %s" % self.title)
+
+ bug = self.bug
+ target = self.target
+ super(SQLBase, self).destroySelf()
+ del get_property_cache(bug).bugtasks
+
+ # When a task is deleted the bug's heat needs to be recalculated.
+ target.recalculateBugHeatCache()
+
def findSimilarBugs(self, user, limit=10):
"""See `IBugTask`."""
if self.product is not None:
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2011-10-03 20:08:04 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2011-10-19 04:31:25 +0000
@@ -4,10 +4,12 @@
__metaclass__ = type
from datetime import timedelta
+import transaction
import unittest
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
+from lazr.restfulclient.errors import Unauthorized
from testtools.matchers import Equals
from zope.component import getUtility
from zope.event import notify
@@ -19,8 +21,13 @@
all,
any,
)
+from canonical.launchpad.webapp.authorization import (
+ check_permission,
+ clear_cache,
+ )
from canonical.launchpad.webapp.interfaces import ILaunchBag
from canonical.testing.layers import (
+ AppServerLayer,
DatabaseFunctionalLayer,
LaunchpadZopelessLayer,
)
@@ -32,6 +39,7 @@
BugTaskImportance,
BugTaskSearchParams,
BugTaskStatus,
+ CannotDeleteBugtask,
DB_UNRESOLVED_BUGTASK_STATUSES,
IBugTaskSet,
RESOLVED_BUGTASK_STATUSES,
@@ -58,6 +66,7 @@
)
from lp.registry.interfaces.product import IProductSet
from lp.registry.interfaces.projectgroup import IProjectGroupSet
+from lp.services.features.testing import FeatureFixture
from lp.soyuz.interfaces.archive import ArchivePurpose
from lp.testing import (
ANONYMOUS,
@@ -72,6 +81,7 @@
StormStatementRecorder,
TestCase,
TestCaseWithFactory,
+ ws_object,
)
from lp.testing.factory import LaunchpadObjectFactory
from lp.testing.fakemethod import FakeMethod
@@ -1385,6 +1395,107 @@
bug.default_bugtask.pillar.displayname, result['pillar_name'])
+class TestBugTaskDeletion(TestCaseWithFactory):
+ """Test the different cases that makes a bugtask deletable or not."""
+
+ layer = DatabaseFunctionalLayer
+
+ flags = {u"disclosure.delete_bugtask.enabled": u"on"}
+
+ def test_cannot_delete_if_not_logged_in(self):
+ # You cannot delete a bug task if not logged in.
+ bug = self.factory.makeBug()
+ with FeatureFixture(self.flags):
+ self.assertFalse(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+
+ def test_unauthorised_cannot_delete(self):
+ # Unauthorised users cannot delete a bug task.
+ bug = self.factory.makeBug()
+ unauthorised = self.factory.makePerson()
+ login_person(unauthorised)
+ with FeatureFixture(self.flags):
+ self.assertFalse(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+
+ def test_pillar_owner_can_delete(self):
+ # With the feature flag on, the pillar owner can delete a bug task.
+ bug = self.factory.makeBug()
+ login_person(bug.default_bugtask.pillar.owner)
+ with FeatureFixture(self.flags):
+ self.assertTrue(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+ # They can't delete the task without the feature flag.
+ clear_cache()
+ self.assertFalse(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+
+ def test_bug_supervisor_can_delete(self):
+ # With the feature flag on, the bug supervisor can delete a bug task.
+ bug_supervisor = self.factory.makePerson()
+ product = self.factory.makeProduct(bug_supervisor=bug_supervisor)
+ bug = self.factory.makeBug(product=product)
+ login_person(bug_supervisor)
+ with FeatureFixture(self.flags):
+ self.assertTrue(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+ # They can't delete the task without the feature flag.
+ clear_cache()
+ self.assertFalse(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+
+ def test_task_reporter_can_delete(self):
+ # With the feature flag on, the bug task reporter can delete bug task.
+ bug = self.factory.makeBug()
+ login_person(bug.default_bugtask.owner)
+ with FeatureFixture(self.flags):
+ self.assertTrue(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+ # They can't delete the task without the feature flag.
+ clear_cache()
+ self.assertFalse(
+ check_permission('launchpad.Delete', bug.default_bugtask))
+
+ def test_cannot_delete_only_bugtask(self):
+ # The only bugtask cannot be deleted.
+ bug = self.factory.makeBug()
+ bugtask = bug.default_bugtask
+ login_person(bugtask.owner)
+ with FeatureFixture(self.flags):
+ self.assertRaises(CannotDeleteBugtask, bugtask.destroySelf)
+
+ def test_delete_bugtask(self):
+ # A bugtask can be deleted.
+ bugtask = self.factory.makeBugTask()
+ bug = bugtask.bug
+ login_person(bugtask.owner)
+ with FeatureFixture(self.flags):
+ bugtask.destroySelf()
+ self.assertEqual([bug.default_bugtask], bug.bugtasks)
+
+ def test_delete_default_bugtask(self):
+ # The default bugtask can be deleted.
+ bugtask = self.factory.makeBugTask()
+ bug = bugtask.bug
+ login_person(bug.default_bugtask.owner)
+ with FeatureFixture(self.flags):
+ bug.default_bugtask.destroySelf()
+ self.assertEqual([bugtask], bug.bugtasks)
+ self.assertEqual(bugtask, bug.default_bugtask)
+
+ def test_bug_heat_updated(self):
+ # Test that the bug heat is updated when a bugtask is deleted.
+ bug = self.factory.makeBug()
+ distro = self.factory.makeDistribution()
+ dsp = self.factory.makeDistributionSourcePackage(distribution=distro)
+ login_person(distro.owner)
+ dsp_task = bug.addTask(bug.owner, dsp)
+ self.assertTrue(dsp.total_bug_heat > 0)
+ with FeatureFixture(self.flags):
+ dsp_task.destroySelf()
+ self.assertTrue(dsp.total_bug_heat == 0)
+
+
class TestConjoinedBugTasks(TestCaseWithFactory):
"""Tests for conjoined bug task functionality."""
@@ -2252,3 +2363,33 @@
"package in which the bug has not yet been reported."
% d.displayname,
validate_new_target, task.bug, d)
+
+
+class TestWebservice(TestCaseWithFactory):
+ """Tests for the webservice."""
+
+ layer = AppServerLayer
+
+ def test_delete_bugtask(self):
+ """Test that a bugtask can be deleted with the feature flag on."""
+ product = self.factory.makeProduct()
+ owner = self.factory.makePerson()
+ db_bugtask = self.factory.makeBugTask(target=product, owner=owner)
+ db_bug = db_bugtask.bug
+ transaction.commit()
+ logout()
+
+ # It will fail without feature flag enabled.
+ launchpad = self.factory.makeLaunchpadService(owner)
+ bugtask = ws_object(launchpad, db_bugtask)
+ self.assertRaises(Unauthorized, bugtask.lp_delete)
+
+ flags = {u"disclosure.delete_bugtask.enabled": u"on"}
+ with FeatureFixture(flags):
+ launchpad = self.factory.makeLaunchpadService(owner)
+ bugtask = ws_object(launchpad, db_bugtask)
+ bugtask.lp_delete()
+ transaction.commit()
+ # Check the delete really worked.
+ with person_logged_in(removeSecurityProxy(db_bug).owner):
+ self.assertEqual([db_bug.default_bugtask], db_bug.bugtasks)
=== modified file 'lib/lp/bugs/security.py'
--- lib/lp/bugs/security.py 2011-09-07 21:53:15 +0000
+++ lib/lp/bugs/security.py 2011-10-19 04:31:25 +0000
@@ -19,9 +19,13 @@
from lp.bugs.interfaces.bugnomination import IBugNomination
from lp.bugs.interfaces.bugsubscription import IBugSubscription
from lp.bugs.interfaces.bugsubscriptionfilter import IBugSubscriptionFilter
+from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
+from lp.bugs.interfaces.bugtask import IBugTaskDelete
from lp.bugs.interfaces.bugtracker import IBugTracker
from lp.bugs.interfaces.bugwatch import IBugWatch
from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
+from lp.registry.interfaces.role import IHasOwner
+from lp.services.features import getFeatureFlag
class EditBugNominationStatus(AuthorizationBase):
@@ -47,6 +51,41 @@
return self.obj.bug.userCanView(user)
+class DeleteBugTask(AuthorizationBase):
+ permission = 'launchpad.Delete'
+ usedfor = IBugTaskDelete
+
+ def checkAuthenticated(self, user):
+ """Check that a user may delete a bugtask.
+
+ A user may delete a bugtask if:
+ - The disclosure.delete_bugtask.enabled feature flag is enabled,
+ and they are:
+ - project maintainer
+ - task creator
+ - bug supervisor
+ """
+ if user is None:
+ return False
+
+ delete_allowed = bool(getFeatureFlag(
+ 'disclosure.delete_bugtask.enabled'))
+ if not delete_allowed:
+ return False
+
+ bugtask = self.obj
+ owner = None
+ if IHasOwner.providedBy(bugtask.pillar):
+ owner = bugtask.pillar.owner
+ bugsupervisor = None
+ if IHasBugSupervisor.providedBy(bugtask.pillar):
+ bugsupervisor = bugtask.pillar.bug_supervisor
+ return (
+ user.inTeam(owner) or
+ user.inTeam(bugsupervisor) or
+ user.inTeam(bugtask.owner))
+
+
class PublicToAllOrPrivateToExplicitSubscribersForBugTask(AuthorizationBase):
permission = 'launchpad.View'
usedfor = IHasBug
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2011-10-14 05:00:27 +0000
+++ lib/lp/services/features/flags.py 2011-10-19 04:31:25 +0000
@@ -143,6 +143,10 @@
('Enables the auto subscribing and unsubscribing of users as a bug '
'transitions between public, private and security related states.'),
''),
+ ('disclosure.delete_bugtask.enabled',
+ 'boolean',
+ ('Enables bugtasks to be deleted by authorised users.'),
+ ''),
('bugs.autoconfirm.enabled_distribution_names',
'space delimited',
('Enables auto-confirming bugtasks for distributions (and their '
=== modified file 'lib/lp/services/features/rulesource.py'
--- lib/lp/services/features/rulesource.py 2011-04-27 02:27:20 +0000
+++ lib/lp/services/features/rulesource.py 2011-10-19 04:31:25 +0000
@@ -87,7 +87,8 @@
def parseRules(self, text_form):
"""Return a list of tuples for the parsed form of the text input.
- For each non-blank line gives back a tuple of (flag, scope, priority, value).
+ For each non-blank line gives back a tuple of
+ (flag, scope, priority, value).
Returns a list rather than a generator so that you see any syntax
errors immediately.
@@ -139,8 +140,8 @@
:param new_rules: List of (name, scope, priority, value) tuples.
"""
- # XXX: would be slightly better to only update rules as necessary so we keep
- # timestamps, and to avoid the direct sql etc -- mbp 20100924
+ # XXX: would be slightly better to only update rules as necessary so
+ # we keep timestamps, and to avoid the direct sql etc -- mbp 20100924
store = getFeatureStore()
store.execute('DELETE FROM FeatureFlag')
for (flag, scope, priority, value) in new_rules:
@@ -149,6 +150,7 @@
flag=unicode(flag),
value=value,
priority=priority))
+ store.flush()
class NullFeatureRuleSource(FeatureRuleSource):
